fix(report): fix last_eval_dttm sort and more tests

superset/app.py

@@ -157,7 +157,8 @@ def init_views(self) -> None:
             AlertLogModelView,
             AlertModelView,
             AlertObservationModelView,
-            AlertReportModelView,
+            AlertView,
+            ReportView,
         )
         from superset.views.annotations import (
             AnnotationLayerModelView,
@@ -439,8 +440,17 @@ def init_views(self) -> None:
             )
             appbuilder.add_view_no_menu(AlertLogModelView)
             appbuilder.add_view_no_menu(AlertObservationModelView)
-            if feature_flag_manager.is_feature_enabled("SIP_34_ALERTS_UI"):
-                appbuilder.add_view_no_menu(AlertReportModelView)
+
+        if feature_flag_manager.is_feature_enabled("ALERT_REPORTS"):
+            appbuilder.add_view(
+                AlertView,
+                "Alerts & Report",
+                label=__("Alerts & Reports"),
+                category="Manage",
+                category_label=__("Manage"),
+                icon="fa-exclamation-triangle",
+            )
+            appbuilder.add_view_no_menu(ReportView)
 
         #
         # Conditionally add Access Request Model View

superset/reports/api.py

@@ -155,6 +155,7 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi):
         "changed_on_delta_humanized",
         "created_on",
         "crontab",
+        "last_eval_dttm",
         "name",
         "type",
         "crontab_humanized",

superset/reports/commands/update.py

@@ -26,7 +26,7 @@
 from superset.dao.exceptions import DAOUpdateFailedError
 from superset.databases.dao import DatabaseDAO
 from superset.exceptions import SupersetSecurityException
-from superset.models.reports import ReportSchedule, ReportScheduleType
+from superset.models.reports import ReportSchedule, ReportScheduleType, ReportState
 from superset.reports.commands.base import BaseReportScheduleCommand
 from superset.reports.commands.exceptions import (
     DatabaseNotFoundValidationError,
@@ -70,6 +70,16 @@ def validate(self) -> None:
         if not self._model:
             raise ReportScheduleNotFoundError()
 
+        # Change the state to not triggered when the user deactivates
+        # A report that is currently in a working state. This prevents
+        # an alert/report from being kept in a working state if activated back
+        if (
+            self._model.last_state == ReportState.WORKING
+            and "active" in self._properties
+            and not self._properties["active"]
+        ):
+            self._properties["last_state"] = ReportState.NOOP
+
         # Validate name uniqueness
         if not ReportScheduleDAO.validate_update_uniqueness(
             name, report_schedule_id=self._model_id

superset/views/alerts.py

@@ -15,7 +15,8 @@
 # specific language governing permissions and limitations
 # under the License.
 from croniter import croniter
-from flask_appbuilder import CompactCRUDMixin
+from flask import abort
+from flask_appbuilder import CompactCRUDMixin, permission_name
 from flask_appbuilder.api import expose
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_appbuilder.security.decorators import has_access
@@ -24,14 +25,13 @@
 from superset import is_feature_enabled
 from superset.constants import RouteMethod
 from superset.models.alerts import Alert, AlertLog, SQLObservation
-from superset.models.reports import ReportSchedule
 from superset.tasks.alerts.validator import check_validator
 from superset.typing import FlaskResponse
 from superset.utils import core as utils
 from superset.utils.core import get_email_address_str, markdown
 
 from ..exceptions import SupersetException
-from .base import SupersetModelView
+from .base import BaseSupersetView, SupersetModelView
 
 # TODO: access control rules for this module
 
@@ -68,37 +68,48 @@ class AlertObservationModelView(
     }
 
 
-class AlertReportModelView(SupersetModelView):
-    datamodel = SQLAInterface(ReportSchedule)
+class BaseAlertReportView(BaseSupersetView):
     route_base = "/report"
-    include_route_methods = RouteMethod.CRUD_SET | {"log"}
+    class_permission_name = "ReportSchedule"
 
     @expose("/list/")
     @has_access
+    @permission_name("read")
     def list(self) -> FlaskResponse:
         if not (
             is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
             and is_feature_enabled("ALERT_REPORTS")
         ):
-            return super().list()
+            return abort(404)
 
         return super().render_app_template()
 
     @expose("/<pk>/log/", methods=["GET"])
     @has_access
+    @permission_name("read")
     def log(self, pk: int) -> FlaskResponse:  # pylint: disable=unused-argument
         if not (
             is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
             and is_feature_enabled("ALERT_REPORTS")
         ):
-            return super().list()
+            return abort(404)
 
         return super().render_app_template()
 
 
+class AlertView(BaseAlertReportView):
+    route_base = "/alert"
+    class_permission_name = "ReportSchedule"
+
+
+class ReportView(BaseAlertReportView):
+    route_base = "/report"
+    class_permission_name = "ReportSchedule"
+
+
 class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
     datamodel = SQLAInterface(Alert)
-    route_base = "/alert"
+    route_base = "/alerts"
     include_route_methods = RouteMethod.CRUD_SET | {"log"}
 
     list_columns = (
@@ -197,28 +208,6 @@ class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
         AlertLogModelView,
     ]
 
-    @expose("/list/")
-    @has_access
-    def list(self) -> FlaskResponse:
-        if not (
-            is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
-            and is_feature_enabled("ALERT_REPORTS")
-        ):
-            return super().list()
-
-        return super().render_app_template()
-
-    @expose("/<pk>/log/", methods=["GET"])
-    @has_access
-    def log(self, pk: int) -> FlaskResponse:  # pylint: disable=unused-argument
-        if not (
-            is_feature_enabled("ENABLE_REACT_CRUD_VIEWS")
-            and is_feature_enabled("ALERT_REPORTS")
-        ):
-            return super().list()
-
-        return super().render_app_template()
-
     def pre_add(self, item: "AlertModelView") -> None:
         item.recipients = get_email_address_str(item.recipients)
 

tests/alerts_tests.py

@@ -341,7 +341,7 @@ def test_deliver_alert_screenshot(
 
     # TODO: fix AlertModelView.show url call from test
     url_mock.side_effect = [
-        f"http://0.0.0.0:8080/alert/show/{alert.id}",
+        f"http://0.0.0.0:8080/alerts/show/{alert.id}",
         f"http://0.0.0.0:8080/superset/slice/{alert.slice_id}/",
     ]
 
@@ -354,7 +354,7 @@ def test_deliver_alert_screenshot(
         f"*Query*:```{alert.sql}```\n"
         f"*Result*: {alert.observations[-1].value}\n"
         f"*Reason*: {alert.observations[-1].value} {alert.pretty_config}\n"
-        f"<http://0.0.0.0:8080/alert/show/{alert.id}"
+        f"<http://0.0.0.0:8080/alerts/show/{alert.id}"
         f"|View Alert Details>\n<http://0.0.0.0:8080/superset/slice/{alert.slice_id}/"
         "|*Explore in Superset*>",
         "title": f"[Alert] {alert.label}",

tests/reports/api_tests.py

@@ -17,10 +17,8 @@
 # isort:skip_file
 """Unit tests for Superset"""
 from datetime import datetime
-from typing import List, Optional
 import json
 
-from flask_appbuilder.security.sqla.models import User
 import pytest
 import prison
 from sqlalchemy.sql import func
@@ -48,6 +46,32 @@
 
 
 class TestReportSchedulesApi(SupersetTestCase):
+    @pytest.fixture()
+    def create_working_report_schedule(self):
+        with self.create_app().app_context():
+
+            admin_user = self.get_user("admin")
+            alpha_user = self.get_user("alpha")
+            chart = db.session.query(Slice).first()
+            example_db = get_example_database()
+
+            report_schedule = insert_report_schedule(
+                type=ReportScheduleType.ALERT,
+                name=f"name_working",
+                crontab=f"* * * * *",
+                sql=f"SELECT value from table",
+                description=f"Report working",
+                chart=chart,
+                database=example_db,
+                owners=[admin_user, alpha_user],
+                last_state=ReportState.WORKING,
+            )
+
+            yield
+
+            db.session.delete(report_schedule)
+            db.session.commit()
+
     @pytest.fixture()
     def create_report_schedules(self):
         with self.create_app().app_context():
@@ -260,8 +284,11 @@ def test_get_list_report_schedule_sorting(self):
             "changed_on",
             "changed_on_delta_humanized",
             "created_on",
+            "crontab",
+            "last_eval_dttm",
             "name",
             "type",
+            "crontab_humanized",
         ]
 
         for order_column in order_columns:
@@ -578,6 +605,29 @@ def test_update_report_schedule(self):
         assert updated_model.chart_id == report_schedule_data["chart"]
         assert updated_model.database_id == report_schedule_data["database"]
 
+    @pytest.mark.usefixtures("create_working_report_schedule")
+    def test_update_report_schedule_state_working(self):
+        """
+        ReportSchedule Api: Test update state in a working report
+        """
+        report_schedule = (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.name == "name_working")
+            .one_or_none()
+        )
+
+        self.login(username="admin")
+        report_schedule_data = {"active": False}
+        uri = f"api/v1/report/{report_schedule.id}"
+        rv = self.client.put(uri, json=report_schedule_data)
+        assert rv.status_code == 200
+        report_schedule = (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.name == "name_working")
+            .one_or_none()
+        )
+        assert report_schedule.last_state == ReportState.NOOP
+
     @pytest.mark.usefixtures("create_report_schedules")
     def test_update_report_schedule_uniqueness(self):
         """
@@ -864,12 +914,15 @@ def test_get_list_report_schedule_logs_sorting(self):
             "error_message",
             "end_dttm",
             "start_dttm",
+            "scheduled_dttm",
         ]
 
         for order_column in order_columns:
             arguments = {"order_column": order_column, "order_direction": "asc"}
             uri = f"api/v1/report/{report_schedule.id}/log/?q={prison.dumps(arguments)}"
             rv = self.get_assert_metric(uri, "get_list")
+            if rv.status_code == 400:
+                raise Exception(json.loads(rv.data.decode("utf-8")))
             assert rv.status_code == 200
 
     @pytest.mark.usefixtures("create_report_schedules")

tests/reports/commands_tests.py

@@ -38,6 +38,8 @@
 )
 from superset.models.slice import Slice
 from superset.reports.commands.exceptions import (
+    AlertQueryError,
+    AlertQueryInvalidTypeError,
     AlertQueryMultipleColumnsError,
     AlertQueryMultipleRowsError,
     ReportScheduleNotFoundError,
@@ -397,6 +399,41 @@ def create_mul_alert_email_chart(request):
             cleanup_report_schedule(report_schedule)
 
 
+@pytest.yield_fixture(params=["alert1", "alert2"])
+def create_invalid_sql_alert_email_chart(request):
+    param_config = {
+        "alert1": {
+            "sql": "SELECT 'string' ",
+            "validator_type": ReportScheduleValidatorType.OPERATOR,
+            "validator_config_json": '{"op": "<", "threshold": 10}',
+        },
+        "alert2": {
+            "sql": "SELECT first from foo_table",
+            "validator_type": ReportScheduleValidatorType.OPERATOR,
+            "validator_config_json": '{"op": "<", "threshold": 10}',
+        },
+    }
+    with app.app_context():
+        chart = db.session.query(Slice).first()
+        example_database = get_example_database()
+        with create_test_table_context(example_database):
+
+            report_schedule = create_report_notification(
+                email_target="[email protected]",
+                chart=chart,
+                report_type=ReportScheduleType.ALERT,
+                database=example_database,
+                sql=param_config[request.param]["sql"],
+                validator_type=param_config[request.param]["validator_type"],
+                validator_config_json=param_config[request.param][
+                    "validator_config_json"
+                ],
+            )
+            yield report_schedule
+
+            cleanup_report_schedule(report_schedule)
+
+
 @pytest.mark.usefixtures("create_report_email_chart")
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.compute_and_cache")
@@ -652,3 +689,15 @@ def test_email_mul_alert(create_mul_alert_email_chart):
             AsyncExecuteReportScheduleCommand(
                 create_mul_alert_email_chart.id, datetime.utcnow()
             ).run()
+
+
+@pytest.mark.usefixtures("create_invalid_sql_alert_email_chart")
+def test_invalid_sql_alert(create_invalid_sql_alert_email_chart):
+    """
+    ExecuteReport Command: Test alert with invalid SQL statements
+    """
+    with freeze_time("2020-01-01T00:00:00Z"):
+        with pytest.raises((AlertQueryError, AlertQueryInvalidTypeError)):
+            AsyncExecuteReportScheduleCommand(
+                create_invalid_sql_alert_email_chart.id, datetime.utcnow()
+            ).run()

tests/reports/utils.py

@@ -22,7 +22,12 @@
 from superset import db
 from superset.models.core import Database
 from superset.models.dashboard import Dashboard
-from superset.models.reports import ReportExecutionLog, ReportRecipients, ReportSchedule
+from superset.models.reports import (
+    ReportExecutionLog,
+    ReportRecipients,
+    ReportSchedule,
+    ReportState,
+)
 from superset.models.slice import Slice
 
 
@@ -39,13 +44,15 @@ def insert_report_schedule(
     validator_type: Optional[str] = None,
     validator_config_json: Optional[str] = None,
     log_retention: Optional[int] = None,
+    last_state: Optional[ReportState] = None,
     grace_period: Optional[int] = None,
     recipients: Optional[List[ReportRecipients]] = None,
     logs: Optional[List[ReportExecutionLog]] = None,
 ) -> ReportSchedule:
     owners = owners or []
     recipients = recipients or []
     logs = logs or []
+    last_state = last_state or ReportState.NOOP
     report_schedule = ReportSchedule(
         type=type,
         name=name,
@@ -62,6 +69,7 @@ def insert_report_schedule(
         grace_period=grace_period,
         recipients=recipients,
         logs=logs,
+        last_state=last_state,
     )
     db.session.add(report_schedule)
     db.session.commit()