From 2fda221b7ca51cd08a18c2e9d14a9728f8ca1c1f Mon Sep 17 00:00:00 2001 From: Jack <41238731+fisjac@users.noreply.github.com> Date: Fri, 23 Aug 2024 15:26:26 -0500 Subject: [PATCH] feat(alert/reports): adding logic to handle downstream reports when tab is deleted from dashboard (#29333) --- .../components/gridComponents/Tabs.jsx | 2 +- superset/commands/dashboard/update.py | 79 ++++++- superset/daos/report.py | 8 + .../dashboards/update_tabs_test.py | 219 ++++++++++++++++++ tests/integration_tests/reports/utils.py | 22 +- 5 files changed, 318 insertions(+), 12 deletions(-) create mode 100644 tests/integration_tests/dashboards/update_tabs_test.py diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx index 2b47eb7bb399d..929d259158952 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx @@ -215,7 +215,7 @@ export class Tabs extends PureComponent { content: ( {t( - 'Deleting a tab will remove all content within it. You may still ' + + 'Deleting a tab will remove all content within it and will deactivate any related alerts or reports. You may still ' + 'reverse this action with the', )}{' '} {t('undo')}{' '} diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index 2effd7bd2ece1..031db1af31fdf 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -15,13 +15,14 @@ # specific language governing permissions and limitations # under the License. import logging +import textwrap from functools import partial from typing import Any, Optional from flask_appbuilder.models.sqla import Model from marshmallow import ValidationError -from superset import security_manager +from superset import app, security_manager from superset.commands.base import BaseCommand, UpdateMixin from superset.commands.dashboard.exceptions import ( DashboardForbiddenError, @@ -32,10 +33,13 @@ ) from superset.commands.utils import populate_roles, update_tags, validate_tags from superset.daos.dashboard import DashboardDAO +from superset.daos.report import ReportScheduleDAO from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard +from superset.reports.models import ReportSchedule from superset.tags.models import ObjectType from superset.utils import json +from superset.utils.core import send_email_smtp from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) @@ -50,7 +54,8 @@ def __init__(self, model_id: int, data: dict[str, Any]): @transaction(on_error=partial(on_error, reraise=DashboardUpdateFailedError)) def run(self) -> Model: self.validate() - assert self._model + assert self._model is not None + self.process_tab_diff() # Update tags if (tags := self._properties.pop("tags", None)) is not None: @@ -112,3 +117,73 @@ def validate(self) -> None: exceptions.append(ex) if exceptions: raise DashboardInvalidError(exceptions=exceptions) + + def process_tab_diff(self) -> None: + def find_deleted_tabs() -> list[str]: + position_json = self._properties.get("position_json", "") + current_tabs = self._model.tabs # type: ignore + if position_json and current_tabs: + position = json.loads(position_json) + deleted_tabs = [ + tab for tab in current_tabs["all_tabs"] if tab not in position + ] + return deleted_tabs + return [] + + def find_reports_containing_tabs(tabs: list[str]) -> list[ReportSchedule]: + alert_reports_list = [] + for tab in tabs: + for report in ReportScheduleDAO.find_by_extra_metadata(tab): + alert_reports_list.append(report) + return alert_reports_list + + def send_deactivated_email_warning(report: ReportSchedule) -> None: + description = textwrap.dedent( + """ + The dashboard tab used in this report has been deleted and your report has been deactivated. + Please update your report settings to remove or change the tab used. + """ + ) + + html_content = textwrap.dedent( + f""" + + + + + +
{description}
+
+ + + """ + ) + for report_owner in report.owners: + if email := report_owner.email: + send_email_smtp( + to=email, + subject=f"[Report: {report.name}] Deactivated", + html_content=html_content, + config=app.config, + ) + + def deactivate_reports(reports_list: list[ReportSchedule]) -> None: + for report in reports_list: + # deactivate + ReportScheduleDAO.update(report, {"active": False}) + # send email to report owner + send_deactivated_email_warning(report) + + deleted_tabs = find_deleted_tabs() + reports = find_reports_containing_tabs(deleted_tabs) + deactivate_reports(reports) diff --git a/superset/daos/report.py b/superset/daos/report.py index 8cf305c13f26e..ab7af8e9763f3 100644 --- a/superset/daos/report.py +++ b/superset/daos/report.py @@ -90,6 +90,14 @@ def find_by_database_ids(database_ids: list[int]) -> list[ReportSchedule]: .all() ) + @staticmethod + def find_by_extra_metadata(slug: str) -> list[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.extra_json.like(f"%{slug}%")) + .all() + ) + @staticmethod def validate_unique_creation_method( dashboard_id: int | None = None, chart_id: int | None = None diff --git a/tests/integration_tests/dashboards/update_tabs_test.py b/tests/integration_tests/dashboards/update_tabs_test.py new file mode 100644 index 0000000000000..708b75d10bc24 --- /dev/null +++ b/tests/integration_tests/dashboards/update_tabs_test.py @@ -0,0 +1,219 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from unittest.mock import ANY, call, MagicMock, patch + +import pytest + +from superset import db, security_manager +from superset.commands.dashboard.update import UpdateDashboardCommand +from superset.models.dashboard import Dashboard +from superset.utils.json import dumps +from tests.integration_tests.fixtures.tabbed_dashboard import ( + tabbed_dashboard, # noqa: F401 +) +from tests.integration_tests.reports.utils import ( + cleanup_report_schedule, + create_report_notification, +) +from tests.integration_tests.test_app import app + +tab1 = "TAB-L1AA" +tab2 = "TAB-L1AB" +tab3 = "TAB-L1BB" + + +@pytest.fixture(autouse=True, scope="module") +def initial_report_cleanup(): + with app.app_context(): + cleanup_report_schedule() + + +def remove_tabs_from_dashboard(dashboard: Dashboard, tabs: list[str]): + data = dashboard.data.copy() + position = data["position_json"] + for tab in tabs: + del position[tab] + data["position_json"] = dumps(position) + return data + + +@patch("superset.commands.dashboard.update.send_email_smtp") +@patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", ALERT_REPORT_TABS=True +) +def test_tab_deletion_single_report( + send_email_smtp_mock: MagicMock, + tabbed_dashboard: Dashboard, # noqa: F811 + login_as_admin, +): + tab_to_delete = tab1 + users = db.session.query(security_manager.user_model).all() + report = create_report_notification( + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": tab_to_delete}}, + owners=[users[0]], + ) + assert report.active is True + UpdateDashboardCommand( + tabbed_dashboard.id, + remove_tabs_from_dashboard(tabbed_dashboard, [tab_to_delete]), + ).run() + + assert report.active is False + send_email_smtp_mock.assert_called_with( + to=users[0].email, + subject="[Report: report] Deactivated", + html_content=ANY, + config=ANY, + ) + cleanup_report_schedule(report) + + +@patch("superset.commands.dashboard.update.send_email_smtp") +@patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", ALERT_REPORT_TABS=True +) +def test_tab_deletion_multiple_reports( + send_email_smtp_mock: MagicMock, + tabbed_dashboard: Dashboard, # noqa: F811 + login_as_admin, +): + tab_to_delete = tab1 + retained_tab = tab2 + users = db.session.query(security_manager.user_model).all() + report1 = create_report_notification( + name="report 1", + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": tab_to_delete}}, + owners=[users[0], users[1]], + ) + report2 = create_report_notification( + name="report 2", + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": tab_to_delete}}, + owners=[users[1]], + ) + report3 = create_report_notification( + name="report 3", + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": retained_tab}}, + owners=[users[2]], + ) + + assert report1.active is True + assert report2.active is True + assert report3.active is True + + UpdateDashboardCommand( + tabbed_dashboard.id, + remove_tabs_from_dashboard(tabbed_dashboard, [tab_to_delete]), + ).run() + + assert report1.active is False + assert report2.active is False + assert report3.active is True + + expected_calls = [ + call( + to=users[0].email, + subject="[Report: report 1] Deactivated", + html_content=ANY, + config=ANY, + ), + call( + to=users[1].email, + subject="[Report: report 1] Deactivated", + html_content=ANY, + config=ANY, + ), + call( + to=users[1].email, + subject="[Report: report 2] Deactivated", + html_content=ANY, + config=ANY, + ), + ] + + assert send_email_smtp_mock.call_count == 3 + assert send_email_smtp_mock.call_args_list == expected_calls + + cleanup_report_schedule(report1) + cleanup_report_schedule(report2) + cleanup_report_schedule(report3) + + +@patch("superset.commands.dashboard.update.send_email_smtp") +@patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", ALERT_REPORT_TABS=True +) +def test_multitple_tabs_removed( + send_email_smtp_mock: MagicMock, + tabbed_dashboard: Dashboard, # noqa: F811 + login_as_admin, +): + tabs_to_delete = [tab1, tab2] + users = db.session.query(security_manager.user_model).all() + report1 = create_report_notification( + name="report 1", + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": tabs_to_delete[0]}}, + owners=[users[0], users[1]], + ) + report2 = create_report_notification( + name="report 2", + dashboard=tabbed_dashboard, + extra={"dashboard": {"anchor": tabs_to_delete[1]}}, + owners=[users[2]], + ) + + assert report1.active is True + assert report2.active is True + + UpdateDashboardCommand( + tabbed_dashboard.id, + remove_tabs_from_dashboard(tabbed_dashboard, tabs_to_delete), + ).run() + + assert report1.active is False + assert report2.active is False + + expected_calls = [ + call( + to=users[0].email, + subject="[Report: report 1] Deactivated", + html_content=ANY, + config=ANY, + ), + call( + to=users[1].email, + subject="[Report: report 1] Deactivated", + html_content=ANY, + config=ANY, + ), + call( + to=users[2].email, + subject="[Report: report 2] Deactivated", + html_content=ANY, + config=ANY, + ), + ] + + assert send_email_smtp_mock.call_count == 3 + assert send_email_smtp_mock.call_args_list == expected_calls + + cleanup_report_schedule(report1) + cleanup_report_schedule(report2) diff --git a/tests/integration_tests/reports/utils.py b/tests/integration_tests/reports/utils.py index d0f00270b9cd2..45b919c2b134d 100644 --- a/tests/integration_tests/reports/utils.py +++ b/tests/integration_tests/reports/utils.py @@ -170,15 +170,19 @@ def create_report_notification( return report_schedule -def cleanup_report_schedule(report_schedule: ReportSchedule) -> None: - db.session.query(ReportExecutionLog).filter( - ReportExecutionLog.report_schedule == report_schedule - ).delete() - db.session.query(ReportRecipients).filter( - ReportRecipients.report_schedule == report_schedule - ).delete() - - db.session.delete(report_schedule) +def cleanup_report_schedule(report_schedule: Optional[ReportSchedule] = None) -> None: + if report_schedule: + db.session.query(ReportExecutionLog).filter( + ReportExecutionLog.report_schedule == report_schedule + ).delete() + db.session.query(ReportRecipients).filter( + ReportRecipients.report_schedule == report_schedule + ).delete() + db.session.delete(report_schedule) + else: + db.session.query(ReportExecutionLog).delete() + db.session.query(ReportRecipients).delete() + db.session.query(ReportSchedule).delete() db.session.commit()