You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:31:11 UTC
[GitHub] [incubator-superset] JasonD28 opened a new pull request #10476: feat: add test email functionality to SQL-based email alerts
JasonD28 opened a new pull request #10476:
URL: https://github.com/apache/incubator-superset/pull/10476
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
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
<!--- Skip this if not applicable -->
In AlertModelView
Before:
After:
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
- [x] unit test
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk merged pull request #10476: feat: add test email functionality to SQL-based email alerts
Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #10476:
URL: https://github.com/apache/incubator-superset/pull/10476
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] JasonD28 commented on a change in pull request #10476: feat: add test email functionality to SQL-based email alerts
Posted by GitBox <gi...@apache.org>.
JasonD28 commented on a change in pull request #10476:
URL: https://github.com/apache/incubator-superset/pull/10476#discussion_r462624105
##########
File path: superset/views/alerts.py
##########
@@ -95,5 +105,47 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
"Superset nags you again."
),
}
+
+ add_form_extra_fields = {
+ "test_alert": BooleanField(
+ "Send Test Alert",
+ default=False,
+ description="If enabled, a test alert will be sent on the creation / update of active alerts. "
+ "All alerts after will be sent only if the SQL statement defined above returns True.",
+ ),
+ "test_email_recipients": StringField(
+ "Test Email Recipients",
+ default=None,
+ description="List of recipients to send test email to. "
+ "If empty, an email will be sent to the original recipients.",
+ ),
+ }
+ edit_form_extra_fields = add_form_extra_fields
edit_columns = add_columns
related_views = [AlertLogModelView]
+
+ def process_form(self, form: Form, is_created: bool) -> None:
+ test_email_recipients = None
+ if form.test_email_recipients.data:
+ test_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"] = test_email_recipients # type: ignore
Review comment:
mypy was complaining: `Incompatible types in assignment (expression has type "Optional[str]", target has type "Optional[bool]")`
All other type definitions matched
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10476: feat: add test email functionality to SQL-based email alerts
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10476:
URL: https://github.com/apache/incubator-superset/pull/10476#discussion_r462564227
##########
File path: superset/tasks/schedules.py
##########
@@ -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
Review comment:
add additional flag to determine if that is a test run e.g. you already have a flag in the form
##########
File path: superset/views/alerts.py
##########
@@ -95,5 +101,40 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
"Superset nags you again."
),
}
+
+ add_form_extra_fields = {
+ "test_alert": BooleanField(
+ "Send Test Alert",
+ default=False,
+ description="If enabled, a test alert will be sent on the creation / update of active alerts.",
Review comment:
explain that email will be send only if alert will fail
##########
File path: superset/views/alerts.py
##########
@@ -95,5 +101,40 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
"Superset nags you again."
),
}
+
+ add_form_extra_fields = {
+ "test_alert": BooleanField(
+ "Send Test Alert",
+ default=False,
+ description="If enabled, a test alert will be sent on the creation / update of active alerts.",
+ ),
+ "test_email_recipients": StringField(
+ "Test Email Recipients",
+ default=None,
+ description="List of recipients to send test email to. "
+ "If empty, an email will be sent to the original recipients.",
+ ),
+ }
+ edit_form_extra_fields = add_form_extra_fields
edit_columns = add_columns
related_views = [AlertLogModelView]
+
+ def process_form(self, form: Form, is_created: bool) -> None:
+ if form.test_email_recipients.data:
+ test_email_recipients = form.test_email_recipients.data.strip()
+ else:
+ test_email_recipients = None
+
+ self._extra_data["test_alert"] = form.test_alert.data
Review 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
##########
File path: tests/alerts_tests.py
##########
@@ -112,3 +113,30 @@ def test_run_alert_query(mock_error, mock_deliver, setup_database):
run_alert_query(database.query(Alert).filter_by(id=5).one(), database)
assert mock_deliver.call_count == 1
assert mock_error.call_count == 4
+
+
+@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):
Review 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
##########
File path: superset/views/alerts.py
##########
@@ -95,5 +101,40 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
"Superset nags you again."
),
}
+
+ add_form_extra_fields = {
+ "test_alert": BooleanField(
+ "Send Test Alert",
+ default=False,
+ description="If enabled, a test alert will be sent on the creation / update of active alerts.",
+ ),
+ "test_email_recipients": StringField(
+ "Test Email Recipients",
+ default=None,
+ description="List of recipients to send test email to. "
+ "If empty, an email will be sent to the original recipients.",
+ ),
+ }
+ edit_form_extra_fields = add_form_extra_fields
edit_columns = add_columns
related_views = [AlertLogModelView]
+
+ def process_form(self, form: Form, is_created: bool) -> None:
+ if form.test_email_recipients.data:
Review comment:
do this instead
```
test_email_recipients = None
if form.test_email_recipients.data:
test_email_recipients = form.test_email_recipients.data.strip()
```
##########
File path: superset/views/alerts.py
##########
@@ -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}
Review comment:
add typing annotation here
##########
File path: superset/views/alerts.py
##########
@@ -95,5 +104,46 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
"Superset nags you again."
),
}
+
+ add_form_extra_fields = {
+ "test_alert": BooleanField(
+ "Send Test Alert",
+ default=False,
+ description="If enabled, a test alert will be sent on the creation / update"
+ " of an active alert. All alerts after will be sent only if the SQL "
+ "statement defined above returns True.",
+ ),
+ "test_email_recipients": StringField(
+ "Test Email Recipients",
+ default=None,
+ description="List of recipients to send test email to. "
+ "If empty, an email will be sent to the original recipients.",
+ ),
+ }
+ edit_form_extra_fields = add_form_extra_fields
edit_columns = add_columns
related_views = [AlertLogModelView]
+
+ def process_form(self, form: Form, is_created: bool) -> None:
+ email_recipients = None
+ if form.test_email_recipients.data:
+ 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
Review comment:
resolve type: ignore - you can cast
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org