-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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 header_data into emails #20903
Changes from 6 commits
2faa388
d2dce2b
f5c9c09
489ed63
5b29358
141bd50
821e662
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,7 @@ | |
ReportRecipientType, | ||
ReportSchedule, | ||
ReportScheduleType, | ||
ReportSourceFormat, | ||
ReportState, | ||
) | ||
from superset.reports.notifications import create_notification | ||
|
@@ -73,6 +74,8 @@ | |
from superset.utils.urls import get_url_path | ||
from superset.utils.webdriver import DashboardStandaloneMode | ||
|
||
from ...utils.core import HeaderDataType | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make this import absolute instead of relative? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I keep on doing this and the black reformats it. So annoying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 please fix this format There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah latest commit fixes it. |
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
|
@@ -305,6 +308,27 @@ def _update_query_context(self) -> None: | |
"Please try loading the chart and saving it again." | ||
) from ex | ||
|
||
def _get_log_data(self) -> HeaderDataType: | ||
chart_id = None | ||
dashboard_id = None | ||
report_source = None | ||
if self._report_schedule.chart: | ||
report_source = ReportSourceFormat.CHART | ||
chart_id = self._report_schedule.chart_id | ||
else: | ||
report_source = ReportSourceFormat.DASHBOARD | ||
dashboard_id = self._report_schedule.dashboard_id | ||
|
||
log_data: HeaderDataType = { | ||
"notification_type": self._report_schedule.type, | ||
"notification_source": report_source, | ||
"notification_format": self._report_schedule.report_format, | ||
"chart_id": chart_id, | ||
"dashboard_id": dashboard_id, | ||
"owners": self._report_schedule.owners, | ||
} | ||
return log_data | ||
|
||
def _get_notification_content(self) -> NotificationContent: | ||
""" | ||
Gets a notification content, this is composed by a title and a screenshot | ||
|
@@ -315,6 +339,7 @@ def _get_notification_content(self) -> NotificationContent: | |
embedded_data = None | ||
error_text = None | ||
screenshot_data = [] | ||
|
||
url = self._get_url(user_friendly=True) | ||
if ( | ||
feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS") | ||
|
@@ -352,13 +377,17 @@ def _get_notification_content(self) -> NotificationContent: | |
f"{self._report_schedule.name}: " | ||
f"{self._report_schedule.dashboard.dashboard_title}" | ||
) | ||
|
||
header_data = self._get_log_data() | ||
|
||
return NotificationContent( | ||
name=name, | ||
url=url, | ||
screenshots=screenshot_data, | ||
description=self._report_schedule.description, | ||
csv=csv_data, | ||
embedded_data=embedded_data, | ||
header_data=header_data, | ||
) | ||
|
||
def _send( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,11 +21,15 @@ | |
import pandas as pd | ||
|
||
from superset.reports.models import ReportRecipients, ReportRecipientType | ||
from superset.utils.core import HeaderDataType | ||
|
||
|
||
@dataclass | ||
class NotificationContent: | ||
name: str | ||
header_data: Optional[ | ||
HeaderDataType | ||
] = None # this is optional to account for error states | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain why this is optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we also create notification_content when there is an error, should we be pushing header data into those cases as well? I assumed we wouldn't be but happy to make it not optional, and create header data in those instances as well. |
||
csv: Optional[bytes] = None # bytes for csv file | ||
screenshots: Optional[List[bytes]] = None # bytes for a list of screenshots | ||
text: Optional[str] = None | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,15 @@ class DatasourceType(str, Enum): | |
VIEW = "view" | ||
|
||
|
||
class HeaderDataType(TypedDict): | ||
notification_format: str | ||
owners: List[int] | ||
notification_type: str | ||
notification_source: Optional[str] | ||
chart_id: Optional[int] | ||
dashboard_id: Optional[int] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all of these are optional, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type and format shouldn't be since they are required in order to make a report. I can make all of them optional however. |
||
|
||
|
||
class DatasourceDict(TypedDict): | ||
type: str # todo(hugh): update this to be DatasourceType | ||
id: int | ||
|
@@ -904,6 +913,7 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many | |
cc: Optional[str] = None, | ||
bcc: Optional[str] = None, | ||
mime_subtype: str = "mixed", | ||
header_data: Optional[HeaderDataType] = None, | ||
) -> None: | ||
""" | ||
Send an email with html content, eg: | ||
|
@@ -917,6 +927,7 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many | |
msg["Subject"] = subject | ||
msg["From"] = smtp_mail_from | ||
msg["To"] = ", ".join(smtp_mail_to) | ||
|
||
msg.preamble = "This is a multi-part message in MIME format." | ||
|
||
recipients = smtp_mail_to | ||
|
@@ -963,8 +974,10 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many | |
image.add_header("Content-ID", "<%s>" % msgid) | ||
image.add_header("Content-Disposition", "inline") | ||
msg.attach(image) | ||
|
||
send_mime_email(smtp_mail_from, recipients, msg, config, dryrun=dryrun) | ||
msg_mutator = config["EMAIL_HEADER_MUTATOR"] | ||
# the base notification returns the message without any editing. | ||
new_msg = msg_mutator(msg, **header_data or {}) | ||
eschutho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
send_mime_email(smtp_mail_from, recipients, new_msg, config, dryrun=dryrun) | ||
|
||
|
||
def send_mime_email( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
) | ||
from superset.models.dashboard import Dashboard | ||
from superset.reports.commands.execute import AsyncExecuteReportScheduleCommand | ||
from superset.reports.models import ReportSourceFormat | ||
from tests.integration_tests.fixtures.tabbed_dashboard import tabbed_dashboard | ||
from tests.integration_tests.reports.utils import create_dashboard_report | ||
|
||
|
@@ -66,3 +67,47 @@ def test_report_for_dashboard_with_tabs( | |
assert digest == dashboard.digest | ||
assert send_email_smtp_mock.call_count == 1 | ||
assert len(send_email_smtp_mock.call_args.kwargs["images"]) == 1 | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider adding unit tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would that exist in this repo or in one with an actual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can mock an example |
||
@patch("superset.reports.notifications.email.send_email_smtp") | ||
@patch( | ||
"superset.reports.commands.execute.DashboardScreenshot", | ||
) | ||
@patch( | ||
"superset.dashboards.permalink.commands.create.CreateDashboardPermalinkCommand.run" | ||
) | ||
def test_report_with_header_data( | ||
create_dashboard_permalink_mock: MagicMock, | ||
dashboard_screenshot_mock: MagicMock, | ||
send_email_smtp_mock: MagicMock, | ||
tabbed_dashboard: Dashboard, | ||
) -> None: | ||
create_dashboard_permalink_mock.return_value = "permalink" | ||
dashboard_screenshot_mock.get_screenshot.return_value = b"test-image" | ||
current_app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False | ||
|
||
with create_dashboard_report( | ||
dashboard=tabbed_dashboard, | ||
extra={"active_tabs": ["TAB-L1B"]}, | ||
name="test report tabbed dashboard", | ||
) as report_schedule: | ||
dashboard: Dashboard = report_schedule.dashboard | ||
AsyncExecuteReportScheduleCommand( | ||
str(uuid4()), report_schedule.id, datetime.utcnow() | ||
).run() | ||
dashboard_state = report_schedule.extra.get("dashboard", {}) | ||
permalink_key = CreateDashboardPermalinkCommand( | ||
dashboard.id, dashboard_state | ||
).run() | ||
|
||
assert dashboard_screenshot_mock.call_count == 1 | ||
(url, digest) = dashboard_screenshot_mock.call_args.args | ||
assert url.endswith(f"/superset/dashboard/p/{permalink_key}/") | ||
assert digest == dashboard.digest | ||
assert send_email_smtp_mock.call_count == 1 | ||
header_data = send_email_smtp_mock.call_args.kwargs["header_data"] | ||
assert header_data.get("dashboard_id") == dashboard.id | ||
assert header_data.get("notification_format") == report_schedule.report_format | ||
assert header_data.get("notification_source") == ReportSourceFormat.DASHBOARD | ||
assert header_data.get("notification_type") == report_schedule.type | ||
assert len(send_email_smtp_mock.call_args.kwargs["header_data"]) == 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related, but there seems to be a slight discrepancy on callable config keys for example:
For example
DATASET_HEALTH_CHECK
default isNone
and is declared like this:Any thoughts @eschutho @AAfghahi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and see the benefits of both. I know that the SQL mutator uses the structure that i am using right now. Should we bring this up to the superset developers at large?