feat: add test email functionality to SQL-based email alerts #10476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bkyryliuk
requested changes
Jul 29, 2020
superset/tasks/schedules.py
Outdated
| @@ -551,7 +551,12 @@ def schedule_alert_query( # pylint: disable=unused-argument | |||
| return | |||
|
|
|||
| if report_type == ScheduleType.alert: | |||
| if run_alert_query(schedule, dbsession): | |||
| # recipients is only specified when task is called from view for a test email | |||
There was a problem hiding this comment.
add additional flag to determine if that is a test run e.g. you already have a flag in the form
superset/views/alerts.py
Outdated
| "test_alert": BooleanField( | ||
| "Send Test Alert", | ||
| default=False, | ||
| description="If enabled, a test alert will be sent on the creation / update of active alerts.", |
There was a problem hiding this comment.
explain that email will be send only if alert will fail
| else: | ||
| test_email_recipients = None | ||
|
|
||
| self._extra_data["test_alert"] = form.test_alert.data |
There was a problem hiding this comment.
https://github.com/apache/incubator-superset/blob/master/superset/views/schedules.py#L120 could be needed as well, e.g. normalizing the email list
| edit_columns = add_columns | ||
| related_views = [AlertLogModelView] | ||
|
|
||
| def process_form(self, form: Form, is_created: bool) -> None: | ||
| if form.test_email_recipients.data: |
There was a problem hiding this comment.
do this instead
test_email_recipients = None
if form.test_email_recipients.data:
test_email_recipients = form.test_email_recipients.data.strip()
|
|
||
| @patch("superset.tasks.schedules.deliver_alert") | ||
| @patch("superset.tasks.schedules.run_alert_query") | ||
| def test_schedule_alert_query(mock_run_alert, mock_deliver_alert, setup_database): |
There was a problem hiding this comment.
it would be nice to add the end to end test as well, it can be done in the separate PR.
Example: https://github.com/apache/incubator-superset/blob/master/tests/core_tests.py#L827
JasonD28
commented
Jul 29, 2020
superset/views/alerts.py
Outdated
| ) | ||
|
|
||
| self._extra_data["test_alert"] = form.test_alert.data | ||
| self._extra_data["test_email_recipients"] = test_email_recipients # type: ignore |
There was a problem hiding this comment.
mypy was complaining: Incompatible types in assignment (expression has type "Optional[str]", target has type "Optional[bool]")
All other type definitions matched
JasonD28
force-pushed
the
jdavis_test_email
branch
from
4205621
to
c398e8c
Compare
bkyryliuk
approved these changes
Jul 29, 2020
bkyryliuk
reviewed
Jul 29, 2020
superset/views/alerts.py
Outdated
| @@ -44,6 +50,7 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors | |||
| datamodel = SQLAInterface(Alert) | |||
| route_base = "/alert" | |||
| include_route_methods = RouteMethod.CRUD_SET | |||
| _extra_data = {"test_alert": False, "test_email_recipients": None} | |||
bkyryliuk
reviewed
Jul 29, 2020
superset/views/alerts.py
Outdated
| email_recipients = get_email_address_str(form.test_email_recipients.data) | ||
|
|
||
| self._extra_data["test_alert"] = form.test_alert.data | ||
| self._extra_data["test_email_recipients"] = email_recipients # type: ignore |
There was a problem hiding this comment.
resolve type: ignore - you can cast
JasonD28
force-pushed
the
jdavis_test_email
branch
from
c50a4e2
to
6958ea8
Compare
JasonD28
added a commit
to JasonD28/incubator-superset
that referenced
this pull request
Aug 3, 2020
…10476) * added test email functionality * formatting changes * more formatting * applied requested changes * mypy Co-authored-by: Jason Davis <@dropbox.com>
JasonD28
added a commit
to JasonD28/incubator-superset
that referenced
this pull request
Aug 3, 2020
…10476) * added test email functionality * formatting changes * more formatting * applied requested changes * mypy Co-authored-by: Jason Davis <@dropbox.com>
auxten
pushed a commit
to auxten/incubator-superset
that referenced
this pull request
Nov 20, 2020
…10476) * added test email functionality * formatting changes * more formatting * applied requested changes * mypy Co-authored-by: Jason Davis <@dropbox.com>
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Added functionality for users to request a test email to be sent whenever an active SQL-based alert is created or updated.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
In AlertModelView
Before:
After:
TEST PLAN
ADDITIONAL INFORMATION