Skip to content

Commit

Permalink
feat: only send alert error emails to owners of the alert (apache#13862)
Browse files Browse the repository at this point in the history
* only send alert error emails to owners of the alert

* reformat long lines

* fix send to owners and add tests

* fix pylint errors

* fix formatting
  • Loading branch information
samtfm authored and Allan Caetano de Oliveira committed May 21, 2021
1 parent 2b37aec commit f33e0c1
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 11 deletions.
24 changes: 20 additions & 4 deletions superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
import json
import logging
from datetime import datetime, timedelta
from typing import Any, List, Optional
Expand All @@ -29,6 +30,8 @@
from superset.extensions import feature_flag_manager
from superset.models.reports import (
ReportExecutionLog,
ReportRecipients,
ReportRecipientType,
ReportSchedule,
ReportScheduleType,
ReportState,
Expand Down Expand Up @@ -226,14 +229,17 @@ def _get_notification_content(self) -> NotificationContent:
description=self._report_schedule.description,
)

def _send(self, notification_content: NotificationContent) -> None:
@staticmethod
def _send(
notification_content: NotificationContent, recipients: List[ReportRecipients]
) -> None:
"""
Sends a notification to all recipients
:raises: ReportScheduleNotificationError
"""
notification_errors = []
for recipient in self._report_schedule.recipients:
for recipient in recipients:
notification = create_notification(recipient, notification_content)
try:
notification.send()
Expand All @@ -250,7 +256,7 @@ def send(self) -> None:
:raises: ReportScheduleNotificationError
"""
notification_content = self._get_notification_content()
self._send(notification_content)
self._send(notification_content, self._report_schedule.recipients)

def send_error(self, name: str, message: str) -> None:
"""
Expand All @@ -259,7 +265,17 @@ def send_error(self, name: str, message: str) -> None:
:raises: ReportScheduleNotificationError
"""
notification_content = NotificationContent(name=name, text=message)
self._send(notification_content)

# filter recipients to recipients who are also owners
owner_recipients = [
ReportRecipients(
type=ReportRecipientType.EMAIL,
recipient_config_json=json.dumps({"target": owner.email}),
)
for owner in self._report_schedule.owners
]

self._send(notification_content, owner_recipients)

def is_in_grace_period(self) -> bool:
"""
Expand Down
20 changes: 13 additions & 7 deletions tests/reports/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from freezegun import freeze_time
from sqlalchemy.sql import func

from superset import db
from superset import db, security_manager
from superset.models.core import Database
from superset.models.dashboard import Dashboard
from superset.models.reports import (
Expand Down Expand Up @@ -130,6 +130,12 @@ def create_report_notification(
report_type = report_type or ReportScheduleType.REPORT
target = email_target or slack_channel
config_json = {"target": target}
owner = (
db.session.query(security_manager.user_model)
.filter_by(email="admin@fab.org")
.one_or_none()
)

if slack_channel:
recipient = ReportRecipients(
type=ReportRecipientType.SLACK,
Expand All @@ -151,6 +157,7 @@ def create_report_notification(
dashboard=dashboard,
database=database,
recipients=[recipient],
owners=[owner],
validator_type=validator_type,
validator_config_json=validator_config_json,
grace_period=grace_period,
Expand Down Expand Up @@ -890,7 +897,7 @@ def test_soft_timeout_alert(email_mock, create_alert_email_chart):

notification_targets = get_target_from_report_schedule(create_alert_email_chart)
# Assert the email smtp address, asserts a notification was sent with the error
assert email_mock.call_args[0][0] == notification_targets[0]
assert email_mock.call_args[0][0] == "admin@fab.org"

assert_log(
ReportState.ERROR, error_message="A timeout occurred while executing the query."
Expand Down Expand Up @@ -919,9 +926,8 @@ def test_soft_timeout_screenshot(screenshot_mock, email_mock, create_alert_email
test_id, create_alert_email_chart.id, datetime.utcnow()
).run()

notification_targets = get_target_from_report_schedule(create_alert_email_chart)
# Assert the email smtp address, asserts a notification was sent with the error
assert email_mock.call_args[0][0] == notification_targets[0]
assert email_mock.call_args[0][0] == "admin@fab.org"

assert_log(
ReportState.ERROR, error_message="A timeout occurred while taking a screenshot."
Expand All @@ -948,7 +954,7 @@ def test_fail_screenshot(screenshot_mock, email_mock, create_report_email_chart)

notification_targets = get_target_from_report_schedule(create_report_email_chart)
# Assert the email smtp address, asserts a notification was sent with the error
assert email_mock.call_args[0][0] == notification_targets[0]
assert email_mock.call_args[0][0] == "admin@fab.org"

assert_log(
ReportState.ERROR, error_message="Failed taking a screenshot Unexpected error"
Expand Down Expand Up @@ -997,7 +1003,7 @@ def test_invalid_sql_alert(email_mock, create_invalid_sql_alert_email_chart):
create_invalid_sql_alert_email_chart
)
# Assert the email smtp address, asserts a notification was sent with the error
assert email_mock.call_args[0][0] == notification_targets[0]
assert email_mock.call_args[0][0] == "admin@fab.org"


@pytest.mark.usefixtures("create_invalid_sql_alert_email_chart")
Expand All @@ -1018,7 +1024,7 @@ def test_grace_period_error(email_mock, create_invalid_sql_alert_email_chart):
create_invalid_sql_alert_email_chart
)
# Assert the email smtp address, asserts a notification was sent with the error
assert email_mock.call_args[0][0] == notification_targets[0]
assert email_mock.call_args[0][0] == "admin@fab.org"
assert (
get_notification_error_sent_count(create_invalid_sql_alert_email_chart) == 1
)
Expand Down

0 comments on commit f33e0c1

Please sign in to comment.