From b323bf0fb661dcaaa1786ef92352139aa7a5619d Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Fri, 9 Aug 2024 17:29:57 +0300 Subject: [PATCH] fix(Embedded): Deleting Embedded Dashboards does not commit the transaction (#29894) --- superset/commands/dashboard/delete.py | 16 +++++++- superset/commands/dashboard/exceptions.py | 4 ++ superset/dashboards/api.py | 7 +++- .../dashboards/commands_tests.py | 37 +++++++++++++++++++ 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/superset/commands/dashboard/delete.py b/superset/commands/dashboard/delete.py index 0135c4303..fd53519a5 100644 --- a/superset/commands/dashboard/delete.py +++ b/superset/commands/dashboard/delete.py @@ -23,12 +23,13 @@ from flask_babel import lazy_gettext as _ from superset import security_manager from superset.commands.base import BaseCommand from superset.commands.dashboard.exceptions import ( + DashboardDeleteEmbeddedFailedError, DashboardDeleteFailedError, DashboardDeleteFailedReportsExistError, DashboardForbiddenError, DashboardNotFoundError, ) -from superset.daos.dashboard import DashboardDAO +from superset.daos.dashboard import DashboardDAO, EmbeddedDashboardDAO from superset.daos.report import ReportScheduleDAO from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard @@ -37,6 +38,19 @@ from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) +class DeleteEmbeddedDashboardCommand(BaseCommand): + def __init__(self, dashboard: Dashboard): + self._dashboard = dashboard + + @transaction(on_error=partial(on_error, reraise=DashboardDeleteEmbeddedFailedError)) + def run(self) -> None: + self.validate() + return EmbeddedDashboardDAO.delete(self._dashboard.embedded) + + def validate(self) -> None: + pass + + class DeleteDashboardCommand(BaseCommand): def __init__(self, model_ids: list[int]): self._model_ids = model_ids diff --git a/superset/commands/dashboard/exceptions.py b/superset/commands/dashboard/exceptions.py index ea753034b..91a4ec683 100644 --- a/superset/commands/dashboard/exceptions.py +++ b/superset/commands/dashboard/exceptions.py @@ -62,6 +62,10 @@ class DashboardDeleteFailedError(DeleteFailedError): message = _("Dashboard could not be deleted.") +class DashboardDeleteEmbeddedFailedError(DeleteFailedError): + message = _("Embedded dashboard could not be deleted.") + + class DashboardDeleteFailedReportsExistError(DashboardDeleteFailedError): message = _("There are associated alerts or reports") diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 49a9ccd8b..53371925a 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -36,7 +36,10 @@ from superset import db, is_feature_enabled, thumbnail_cache from superset.charts.schemas import ChartEntityResponseSchema from superset.commands.dashboard.copy import CopyDashboardCommand from superset.commands.dashboard.create import CreateDashboardCommand -from superset.commands.dashboard.delete import DeleteDashboardCommand +from superset.commands.dashboard.delete import ( + DeleteDashboardCommand, + DeleteEmbeddedDashboardCommand, +) from superset.commands.dashboard.exceptions import ( DashboardAccessDeniedError, DashboardCopyError, @@ -1549,7 +1552,7 @@ class DashboardRestApi(BaseSupersetModelRestApi): 500: $ref: '#/components/responses/500' """ - EmbeddedDashboardDAO.delete(dashboard.embedded) + DeleteEmbeddedDashboardCommand(dashboard).run() return self.response(200, message="OK") @expose("//copy/", methods=("POST",)) diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index d7f0a43ce..f38774a1b 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -23,6 +23,7 @@ from werkzeug.utils import secure_filename from superset import db, security_manager from superset.commands.dashboard.copy import CopyDashboardCommand +from superset.commands.dashboard.delete import DeleteEmbeddedDashboardCommand from superset.commands.dashboard.exceptions import ( DashboardForbiddenError, DashboardInvalidError, @@ -39,6 +40,7 @@ from superset.commands.importers.exceptions import IncorrectVersionError from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database from superset.models.dashboard import Dashboard +from superset.models.embedded_dashboard import EmbeddedDashboard from superset.models.slice import Slice from superset.utils import json from superset.utils.core import override_user @@ -722,3 +724,38 @@ class TestCopyDashboardCommand(SupersetTestCase): command = CopyDashboardCommand(example_dashboard, invalid_copy_data) with self.assertRaises(DashboardInvalidError): command.run() + + +class TestDeleteEmbeddedDashboardCommand(SupersetTestCase): + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_delete_embedded_dashboard_command(self): + """Test that an admin user can add and then delete an embedded dashboard""" + with self.client.application.test_request_context(): + example_dashboard = ( + db.session.query(Dashboard).filter_by(slug="world_health").one() + ) + + # Step 1: Add an embedded dashboard + new_embedded_dashboard = EmbeddedDashboard( + dashboard_id=example_dashboard.id + ) + db.session.add(new_embedded_dashboard) + db.session.commit() + + # Step 2: Assert that the embedded dashboard was added + embedded_dashboards = example_dashboard.embedded + assert len(embedded_dashboards) > 0 + assert new_embedded_dashboard in embedded_dashboards + + # Step 3: Delete the embedded dashboard + with override_user(security_manager.find_user("admin")): + command = DeleteEmbeddedDashboardCommand(example_dashboard) + command.run() + + # Step 4: Assert that the embedded dashboard was deleted + deleted_embedded_dashboard = ( + db.session.query(EmbeddedDashboard) + .filter_by(uuid=new_embedded_dashboard.uuid) + .one_or_none() + ) + assert deleted_embedded_dashboard is None