Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add test email functionality to SQL-based email alerts #10476

Merged
merged 5 commits into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions superset/tasks/schedules.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ def schedule_alert_query( # pylint: disable=unused-argument
report_type: ScheduleType,
schedule_id: int,
recipients: Optional[str] = None,
is_test_alert: Optional[bool] = False,
) -> None:
model_cls = get_scheduler_model(report_type)
dbsession = db.create_scoped_session()
Expand All @@ -551,6 +552,10 @@ def schedule_alert_query( # pylint: disable=unused-argument
return

if report_type == ScheduleType.alert:
if is_test_alert and recipients:
deliver_alert(schedule, recipients)
return

if run_alert_query(schedule, dbsession):
# deliver_dashboard OR deliver_slice
return
Expand All @@ -564,10 +569,12 @@ class AlertState:
PASS = "pass"


def deliver_alert(alert: Alert) -> None:
def deliver_alert(alert: Alert, recipients: Optional[str] = None) -> None:
logging.info("Triggering alert: %s", alert)
img_data = None
images = {}
recipients = recipients or alert.recipients

if alert.slice:

chart_url = get_url_path(
Expand Down Expand Up @@ -604,7 +611,7 @@ def deliver_alert(alert: Alert) -> None:
image_url=image_url,
)

_deliver_email(alert.recipients, deliver_as_group, subject, body, data, images)
_deliver_email(recipients, deliver_as_group, subject, body, data, images)


def run_alert_query(alert: Alert, dbsession: Session) -> Optional[bool]:
Expand Down
7 changes: 7 additions & 0 deletions superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,13 @@ def get_email_address_list(address_string: str) -> List[str]:
return [x.strip() for x in address_string_list if x.strip()]


def get_email_address_str(address_string: str) -> str:
address_list = get_email_address_list(address_string)
address_list_str = ", ".join(address_list)

return address_list_str


def choicify(values: Iterable[Any]) -> List[Tuple[Any, Any]]:
"""Takes an iterable and makes an iterable of tuples with it"""
return [(v, v) for v in values]
Expand Down
56 changes: 55 additions & 1 deletion superset/views/alerts.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,21 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from typing import Dict, Optional, Union

from croniter import croniter
from flask_appbuilder import CompactCRUDMixin
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_babel import lazy_gettext as _
from wtforms import BooleanField, Form, StringField

from superset.constants import RouteMethod
from superset.models.alerts import Alert, AlertLog
from superset.utils.core import markdown
from superset.models.schedules import ScheduleType
from superset.tasks.schedules import schedule_alert_query
from superset.utils.core import get_email_address_str, markdown

from ..exceptions import SupersetException
from .base import SupersetModelView

# TODO: access control rules for this module
Expand All @@ -44,6 +51,10 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
datamodel = SQLAInterface(Alert)
route_base = "/alert"
include_route_methods = RouteMethod.CRUD_SET
_extra_data: Dict[str, Union[bool, Optional[str]]] = {
"test_alert": False,
"test_email_recipients": None,
}

list_columns = (
"label",
Expand All @@ -68,6 +79,8 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors
# "dashboard",
"log_retention",
"grace_period",
"test_alert",
"test_email_recipients",
)
label_columns = {
"sql": "SQL",
Expand Down Expand Up @@ -95,5 +108,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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do this instead

test_email_recipients = None
if form.test_email_recipients.data:
  test_email_recipients = form.test_email_recipients.data.strip()

email_recipients = get_email_address_str(form.test_email_recipients.data)

self._extra_data["test_alert"] = form.test_alert.data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._extra_data["test_email_recipients"] = email_recipients

def pre_add(self, item: "AlertModelView") -> None:
item.recipients = get_email_address_str(item.recipients)

if not croniter.is_valid(item.crontab):
raise SupersetException("Invalid crontab format")

def post_add(self, item: "AlertModelView") -> None:
if self._extra_data["test_alert"]:
recipients = self._extra_data["test_email_recipients"] or item.recipients
args = (ScheduleType.alert, item.id)
kwargs = dict(recipients=recipients, is_test_alert=True)
schedule_alert_query.apply_async(args=args, kwargs=kwargs)

def post_update(self, item: "AlertModelView") -> None:
self.post_add(item)
31 changes: 30 additions & 1 deletion tests/alerts_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@

from superset import db
from superset.models.alerts import Alert, AlertLog
from superset.models.schedules import ScheduleType
from superset.models.slice import Slice
from superset.tasks.schedules import run_alert_query
from superset.tasks.schedules import run_alert_query, schedule_alert_query
from superset.utils import core as utils
from tests.test_app import app

Expand Down Expand Up @@ -112,3 +113,31 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

database = setup_database
active_alert = database.query(Alert).filter_by(id=1).one()
inactive_alert = database.query(Alert).filter_by(id=3).one()

# Test that inactive alerts are no processed
schedule_alert_query(report_type=ScheduleType.alert, schedule_id=inactive_alert.id)
assert mock_run_alert.call_count == 0
assert mock_deliver_alert.call_count == 0

# Test that active alerts with no recipients passed in are processed regularly
schedule_alert_query(report_type=ScheduleType.alert, schedule_id=active_alert.id)
assert mock_run_alert.call_count == 1
assert mock_deliver_alert.call_count == 0

# Test that active alerts sent as a test are delivered immediately
schedule_alert_query(
report_type=ScheduleType.alert,
schedule_id=active_alert.id,
recipients="testing@email.com",
is_test_alert=True,
)
assert mock_run_alert.call_count == 1
assert mock_deliver_alert.call_count == 1