fix: delete chart, dashboards, dbs with assoc reports (#11801)

* fix: delete chart or dashboards with assoc reports

* database constraint to reports and tests

* add tests for dashboards and database

* fix exceptions default text

superset/charts/commands/bulk_delete.py

@@ -18,9 +18,11 @@
 from typing import List, Optional
 
 from flask_appbuilder.security.sqla.models import User
+from flask_babel import lazy_gettext as _
 
 from superset.charts.commands.exceptions import (
     ChartBulkDeleteFailedError,
+    ChartBulkDeleteFailedReportsExistError,
     ChartForbiddenError,
     ChartNotFoundError,
 )
@@ -29,6 +31,7 @@
 from superset.commands.exceptions import DeleteFailedError
 from superset.exceptions import SupersetSecurityException
 from superset.models.slice import Slice
+from superset.reports.dao import ReportScheduleDAO
 from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
@@ -53,6 +56,13 @@ def validate(self) -> None:
         self._models = ChartDAO.find_by_ids(self._model_ids)
         if not self._models or len(self._models) != len(self._model_ids):
             raise ChartNotFoundError()
+        # Check there are no associated ReportSchedules
+        reports = ReportScheduleDAO.find_by_chart_ids(self._model_ids)
+        if reports:
+            report_names = [report.name for report in reports]
+            raise ChartBulkDeleteFailedReportsExistError(
+                _("There are associated alerts or reports: %s" % ",".join(report_names))
+            )
         # Check ownership
         for model in self._models:
             try:

superset/charts/commands/delete.py

@@ -19,9 +19,11 @@
 
 from flask_appbuilder.models.sqla import Model
 from flask_appbuilder.security.sqla.models import User
+from flask_babel import lazy_gettext as _
 
 from superset.charts.commands.exceptions import (
     ChartDeleteFailedError,
+    ChartDeleteFailedReportsExistError,
     ChartForbiddenError,
     ChartNotFoundError,
 )
@@ -31,6 +33,7 @@
 from superset.exceptions import SupersetSecurityException
 from superset.models.dashboard import Dashboard
 from superset.models.slice import Slice
+from superset.reports.dao import ReportScheduleDAO
 from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
@@ -57,6 +60,13 @@ def validate(self) -> None:
         self._model = ChartDAO.find_by_id(self._model_id)
         if not self._model:
             raise ChartNotFoundError()
+        # Check there are no associated ReportSchedules
+        reports = ReportScheduleDAO.find_by_chart_id(self._model_id)
+        if reports:
+            report_names = [report.name for report in reports]
+            raise ChartDeleteFailedReportsExistError(
+                _("There are associated alerts or reports: %s" % ",".join(report_names))
+            )
         # Check ownership
         try:
             check_ownership(self._model)

superset/charts/commands/exceptions.py

@@ -78,13 +78,21 @@ class ChartDeleteFailedError(DeleteFailedError):
     message = _("Chart could not be deleted.")
 
 
+class ChartDeleteFailedReportsExistError(ChartDeleteFailedError):
+    message = _("There are associated alerts or reports")
+
+
 class ChartForbiddenError(ForbiddenError):
     message = _("Changing this chart is forbidden")
 
 
-class ChartBulkDeleteFailedError(CreateFailedError):
+class ChartBulkDeleteFailedError(DeleteFailedError):
     message = _("Charts could not be deleted.")
 
 
+class ChartBulkDeleteFailedReportsExistError(ChartBulkDeleteFailedError):
+    message = _("There are associated alerts or reports")
+
+
 class ChartImportError(ImportFailedError):
     message = _("Import chart failed for an unknown reason")

superset/dashboards/commands/bulk_delete.py

@@ -18,17 +18,20 @@
 from typing import List, Optional
 
 from flask_appbuilder.security.sqla.models import User
+from flask_babel import lazy_gettext as _
 
 from superset.commands.base import BaseCommand
 from superset.commands.exceptions import DeleteFailedError
 from superset.dashboards.commands.exceptions import (
     DashboardBulkDeleteFailedError,
+    DashboardBulkDeleteFailedReportsExistError,
     DashboardForbiddenError,
     DashboardNotFoundError,
 )
 from superset.dashboards.dao import DashboardDAO
 from superset.exceptions import SupersetSecurityException
 from superset.models.dashboard import Dashboard
+from superset.reports.dao import ReportScheduleDAO
 from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
@@ -54,6 +57,13 @@ def validate(self) -> None:
         self._models = DashboardDAO.find_by_ids(self._model_ids)
         if not self._models or len(self._models) != len(self._model_ids):
             raise DashboardNotFoundError()
+        # Check there are no associated ReportSchedules
+        reports = ReportScheduleDAO.find_by_dashboard_ids(self._model_ids)
+        if reports:
+            report_names = [report.name for report in reports]
+            raise DashboardBulkDeleteFailedReportsExistError(
+                _("There are associated alerts or reports: %s" % ",".join(report_names))
+            )
         # Check ownership
         for model in self._models:
             try:

superset/dashboards/commands/delete.py

@@ -19,17 +19,20 @@
 
 from flask_appbuilder.models.sqla import Model
 from flask_appbuilder.security.sqla.models import User
+from flask_babel import lazy_gettext as _
 
 from superset.commands.base import BaseCommand
 from superset.dao.exceptions import DAODeleteFailedError
 from superset.dashboards.commands.exceptions import (
     DashboardDeleteFailedError,
+    DashboardDeleteFailedReportsExistError,
     DashboardForbiddenError,
     DashboardNotFoundError,
 )
 from superset.dashboards.dao import DashboardDAO
 from superset.exceptions import SupersetSecurityException
 from superset.models.dashboard import Dashboard
+from superset.reports.dao import ReportScheduleDAO
 from superset.views.base import check_ownership
 
 logger = logging.getLogger(__name__)
@@ -55,6 +58,13 @@ def validate(self) -> None:
         self._model = DashboardDAO.find_by_id(self._model_id)
         if not self._model:
             raise DashboardNotFoundError()
+        # Check there are no associated ReportSchedules
+        reports = ReportScheduleDAO.find_by_dashboard_id(self._model_id)
+        if reports:
+            report_names = [report.name for report in reports]
+            raise DashboardDeleteFailedReportsExistError(
+                _("There are associated alerts or reports: %s" % ",".join(report_names))
+            )
         # Check ownership
         try:
             check_ownership(self._model)

superset/dashboards/commands/exceptions.py

@@ -53,6 +53,10 @@ class DashboardBulkDeleteFailedError(CreateFailedError):
     message = _("Dashboards could not be deleted.")
 
 
+class DashboardBulkDeleteFailedReportsExistError(DashboardBulkDeleteFailedError):
+    message = _("There are associated alerts or reports")
+
+
 class DashboardUpdateFailedError(UpdateFailedError):
     message = _("Dashboard could not be updated.")
 
@@ -61,6 +65,10 @@ class DashboardDeleteFailedError(DeleteFailedError):
     message = _("Dashboard could not be deleted.")
 
 
+class DashboardDeleteFailedReportsExistError(DashboardDeleteFailedError):
+    message = _("There are associated alerts or reports")
+
+
 class DashboardForbiddenError(ForbiddenError):
     message = _("Changing this Dashboard is forbidden")
 

superset/databases/commands/delete.py

@@ -19,16 +19,19 @@
 
 from flask_appbuilder.models.sqla import Model
 from flask_appbuilder.security.sqla.models import User
+from flask_babel import lazy_gettext as _
 
 from superset.commands.base import BaseCommand
 from superset.dao.exceptions import DAODeleteFailedError
 from superset.databases.commands.exceptions import (
     DatabaseDeleteDatasetsExistFailedError,
     DatabaseDeleteFailedError,
+    DatabaseDeleteFailedReportsExistError,
     DatabaseNotFoundError,
 )
 from superset.databases.dao import DatabaseDAO
 from superset.models.core import Database
+from superset.reports.dao import ReportScheduleDAO
 
 logger = logging.getLogger(__name__)
 
@@ -53,6 +56,14 @@ def validate(self) -> None:
         self._model = DatabaseDAO.find_by_id(self._model_id)
         if not self._model:
             raise DatabaseNotFoundError()
+        # Check there are no associated ReportSchedules
+        reports = ReportScheduleDAO.find_by_database_id(self._model_id)
+
+        if reports:
+            report_names = [report.name for report in reports]
+            raise DatabaseDeleteFailedReportsExistError(
+                _("There are associated alerts or reports: %s" % ",".join(report_names))
+            )
         # Check if there are datasets for this database
         if self._model.tables:
             raise DatabaseDeleteDatasetsExistFailedError()

superset/databases/commands/exceptions.py

@@ -113,6 +113,10 @@ class DatabaseDeleteFailedError(DeleteFailedError):
     message = _("Database could not be deleted.")
 
 
+class DatabaseDeleteFailedReportsExistError(DatabaseDeleteFailedError):
+    message = _("There are associated alerts or reports")
+
+
 class DatabaseSecurityUnsafeError(DBSecurityException):
     message = _("Stopped an unsafe database connection")
 

superset/reports/dao.py

@@ -38,6 +38,54 @@
 class ReportScheduleDAO(BaseDAO):
     model_cls = ReportSchedule
 
+    @staticmethod
+    def find_by_chart_id(chart_id: int) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.chart_id == chart_id)
+            .all()
+        )
+
+    @staticmethod
+    def find_by_chart_ids(chart_ids: List[int]) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.chart_id.in_(chart_ids))
+            .all()
+        )
+
+    @staticmethod
+    def find_by_dashboard_id(dashboard_id: int) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.dashboard_id == dashboard_id)
+            .all()
+        )
+
+    @staticmethod
+    def find_by_dashboard_ids(dashboard_ids: List[int]) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.dashboard_id.in_(dashboard_ids))
+            .all()
+        )
+
+    @staticmethod
+    def find_by_database_id(database_id: int) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.database_id == database_id)
+            .all()
+        )
+
+    @staticmethod
+    def find_by_database_ids(database_ids: List[int]) -> List[ReportSchedule]:
+        return (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.database_id.in_(database_ids))
+            .all()
+        )
+
     @staticmethod
     def bulk_delete(
         models: Optional[List[ReportSchedule]], commit: bool = True

superset/reports/schemas.py

@@ -136,7 +136,7 @@ class ReportSchedulePostSchema(Schema):
     crontab = fields.String(
         description=crontab_description,
         validate=[validate_crontab, Length(1, 50)],
-        example="*/5 * * * * *",
+        example="*/5 * * * *",
         allow_none=False,
         required=True,
     )

tests/charts/api_tests.py

@@ -38,6 +38,7 @@
 from superset.extensions import db, security_manager
 from superset.models.core import Database, FavStar, FavStarClassName
 from superset.models.dashboard import Dashboard
+from superset.models.reports import ReportSchedule, ReportScheduleType
 from superset.models.slice import Slice
 from superset.utils import core as utils
 from tests.base_api_tests import ApiOwnersTestCaseMixin
@@ -117,6 +118,26 @@ def create_charts(self):
                 db.session.delete(fav_chart)
             db.session.commit()
 
+    @pytest.fixture()
+    def create_chart_with_report(self):
+        with self.create_app().app_context():
+            admin = self.get_user("admin")
+            chart = self.insert_chart(f"chart_report", [admin.id], 1)
+            report_schedule = ReportSchedule(
+                type=ReportScheduleType.REPORT,
+                name="report_with_chart",
+                crontab="* * * * *",
+                chart=chart,
+            )
+            db.session.commit()
+
+            yield chart
+
+            # rollback changes
+            db.session.delete(report_schedule)
+            db.session.delete(chart)
+            db.session.commit()
+
     def test_delete_chart(self):
         """
         Chart API: Test delete
@@ -174,18 +195,62 @@ def test_delete_not_found_chart(self):
         rv = self.delete_assert_metric(uri, "delete")
         self.assertEqual(rv.status_code, 404)
 
+    @pytest.mark.usefixtures("create_chart_with_report")
+    def test_delete_chart_with_report(self):
+        """
+        Chart API: Test delete with associated report
+        """
+        self.login(username="admin")
+        chart = (
+            db.session.query(Slice)
+            .filter(Slice.slice_name == "chart_report")
+            .one_or_none()
+        )
+        uri = f"api/v1/chart/{chart.id}"
+        rv = self.client.delete(uri)
+        response = json.loads(rv.data.decode("utf-8"))
+        self.assertEqual(rv.status_code, 422)
+        expected_response = {
+            "message": "There are associated alerts or reports: report_with_chart"
+        }
+        self.assertEqual(response, expected_response)
+
     def test_delete_bulk_charts_not_found(self):
         """
         Chart API: Test delete bulk not found
         """
         max_id = db.session.query(func.max(Slice.id)).scalar()
         chart_ids = [max_id + 1, max_id + 2]
         self.login(username="admin")
-        argument = chart_ids
-        uri = f"api/v1/chart/?q={prison.dumps(argument)}"
+        uri = f"api/v1/chart/?q={prison.dumps(chart_ids)}"
         rv = self.delete_assert_metric(uri, "bulk_delete")
         self.assertEqual(rv.status_code, 404)
 
+    @pytest.mark.usefixtures("create_chart_with_report", "create_charts")
+    def test_bulk_delete_chart_with_report(self):
+        """
+        Chart API: Test bulk delete with associated report
+        """
+        self.login(username="admin")
+        chart_with_report = (
+            db.session.query(Slice.id)
+            .filter(Slice.slice_name == "chart_report")
+            .one_or_none()
+        )
+
+        charts = db.session.query(Slice.id).filter(Slice.slice_name.like("name%")).all()
+        chart_ids = [chart.id for chart in charts]
+        chart_ids.append(chart_with_report.id)
+
+        uri = f"api/v1/chart/?q={prison.dumps(chart_ids)}"
+        rv = self.client.delete(uri)
+        response = json.loads(rv.data.decode("utf-8"))
+        self.assertEqual(rv.status_code, 422)
+        expected_response = {
+            "message": "There are associated alerts or reports: report_with_chart"
+        }
+        self.assertEqual(response, expected_response)
+
     def test_delete_chart_admin_not_owned(self):
         """
         Chart API: Test admin delete not owned