diff --git a/superset/app.py b/superset/app.py index 7dddd40b668..a163c099c35 100644 --- a/superset/app.py +++ b/superset/app.py @@ -157,7 +157,8 @@ class SupersetAppInitializer: AlertLogModelView, AlertModelView, AlertObservationModelView, - AlertReportModelView, + AlertView, + ReportView, ) from superset.views.annotations import ( AnnotationLayerModelView, @@ -439,8 +440,17 @@ class SupersetAppInitializer: ) 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 diff --git a/superset/reports/api.py b/superset/reports/api.py index 5928cc336a2..4e72ac0fc30 100644 --- a/superset/reports/api.py +++ b/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", diff --git a/superset/reports/commands/update.py b/superset/reports/commands/update.py index d2cc9fd1510..02143c3e2a2 100644 --- a/superset/reports/commands/update.py +++ b/superset/reports/commands/update.py @@ -26,7 +26,7 @@ from superset.commands.utils import populate_owners 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 @@ class UpdateReportScheduleCommand(BaseReportScheduleCommand): 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 diff --git a/superset/views/alerts.py b/superset/views/alerts.py index e9b84ac8af7..a6e650c40f7 100644 --- a/superset/views/alerts.py +++ b/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 flask_babel import lazy_gettext as _ 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("//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("//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) diff --git a/tests/alerts_tests.py b/tests/alerts_tests.py index af92cbef22c..bf6f6a62d21 100644 --- a/tests/alerts_tests.py +++ b/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"\n", "title": f"[Alert] {alert.label}", diff --git a/tests/reports/api_tests.py b/tests/reports/api_tests.py index 6422d02f93e..e1b76559749 100644 --- a/tests/reports/api_tests.py +++ b/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 @@ REPORTS_COUNT = 10 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 @@ class TestReportSchedulesApi(SupersetTestCase): "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 @@ class TestReportSchedulesApi(SupersetTestCase): 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 @@ class TestReportSchedulesApi(SupersetTestCase): "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") diff --git a/tests/reports/commands_tests.py b/tests/reports/commands_tests.py index 89fa37556e5..3de782df5ac 100644 --- a/tests/reports/commands_tests.py +++ b/tests/reports/commands_tests.py @@ -38,6 +38,8 @@ from superset.models.reports import ( ) 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="target@email.com", + 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() diff --git a/tests/reports/utils.py b/tests/reports/utils.py index 841ae4d9752..2e92e153158 100644 --- a/tests/reports/utils.py +++ b/tests/reports/utils.py @@ -22,7 +22,12 @@ from flask_appbuilder.security.sqla.models import User 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,6 +44,7 @@ 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, @@ -46,6 +52,7 @@ def insert_report_schedule( 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()