Revert "fix: Ensure alerts & reports aren't schduled when flag is off (#16639)"

This reverts commit 4dc859f89e.

superset-frontend/spec/javascripts/sqllab/actions/sqlLab_spec.js

@@ -612,44 +612,44 @@ describe('async actions', () => {
     });
 
     describe('queryEditorSetSql', () => {
+      const sql = 'SELECT * ';
+      const expectedActions = [
+        {
+          type: actions.QUERY_EDITOR_SET_SQL,
+          queryEditor,
+          sql,
+        },
+      ];
       describe('with backend persistence flag on', () => {
         it('updates the tab state in the backend', () => {
           expect.assertions(2);
 
-          const sql = 'SELECT * ';
           const store = mockStore({});
 
           return store
             .dispatch(actions.queryEditorSetSql(queryEditor, sql))
             .then(() => {
-              expect(store.getActions()).toHaveLength(0);
+              expect(store.getActions()).toEqual(expectedActions);
               expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(1);
             });
         });
       });
-    });
-    describe('with backend persistence flag off', () => {
-      it('does not update the tab state in the backend', () => {
-        const backendPersistenceOffMock = jest
-          .spyOn(featureFlags, 'isFeatureEnabled')
-          .mockImplementation(
-            feature => !(feature === 'SQLLAB_BACKEND_PERSISTENCE'),
-          );
-        const sql = 'SELECT * ';
-        const store = mockStore({});
-        const expectedActions = [
-          {
-            type: actions.QUERY_EDITOR_SET_SQL,
-            queryEditor,
-            sql,
-          },
-        ];
+      describe('with backend persistence flag off', () => {
+        it('does not update the tab state in the backend', () => {
+          const backendPersistenceOffMock = jest
+            .spyOn(featureFlags, 'isFeatureEnabled')
+            .mockImplementation(
+              feature => !(feature === 'SQLLAB_BACKEND_PERSISTENCE'),
+            );
 
-        store.dispatch(actions.queryEditorSetSql(queryEditor, sql));
+          const store = mockStore({});
 
-        expect(store.getActions()).toEqual(expectedActions);
-        expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0);
-        backendPersistenceOffMock.mockRestore();
+          store.dispatch(actions.queryEditorSetSql(queryEditor, sql));
+
+          expect(store.getActions()).toEqual(expectedActions);
+          expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0);
+          backendPersistenceOffMock.mockRestore();
+        });
       });
     });
 

superset-frontend/src/SqlLab/actions/sqlLab.js

@@ -898,6 +898,8 @@ export function updateSavedQuery(query) {
 
 export function queryEditorSetSql(queryEditor, sql) {
   return function (dispatch) {
+    // saved query and set tab state use this action
+    dispatch({ type: QUERY_EDITOR_SET_SQL, queryEditor, sql });
     if (isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)) {
       return SupersetClient.put({
         endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
@@ -914,7 +916,7 @@ export function queryEditorSetSql(queryEditor, sql) {
         ),
       );
     }
-    return dispatch({ type: QUERY_EDITOR_SET_SQL, queryEditor, sql });
+    return Promise.resolve();
   };
 }
 

superset-frontend/src/SqlLab/components/AceEditorWrapper.tsx

@@ -30,6 +30,7 @@ import {
   AceCompleterKeyword,
   FullSQLEditor as AceEditor,
 } from 'src/components/AsyncAceEditor';
+import { QueryEditor } from '../types';
 
 type HotKey = {
   key: string;
@@ -51,7 +52,7 @@ interface Props {
   tables: any[];
   functionNames: string[];
   extendedTables: Array<{ name: string; columns: any[] }>;
-  queryEditor: any;
+  queryEditor: QueryEditor;
   height: string;
   hotkeys: HotKey[];
   onChange: (sql: string) => void;
@@ -86,10 +87,12 @@ class AceEditorWrapper extends React.PureComponent<Props, State> {
   componentDidMount() {
     // Making sure no text is selected from previous mount
     this.props.actions.queryEditorSetSelectedText(this.props.queryEditor, null);
-    this.props.actions.queryEditorSetFunctionNames(
-      this.props.queryEditor,
-      this.props.queryEditor.dbId,
-    );
+    if (this.props.queryEditor.dbId) {
+      this.props.actions.queryEditorSetFunctionNames(
+        this.props.queryEditor,
+        this.props.queryEditor.dbId,
+      );
+    }
     this.setAutoCompleter(this.props);
   }
 
@@ -228,8 +231,8 @@ class AceEditorWrapper extends React.PureComponent<Props, State> {
 
   getAceAnnotations() {
     const { validationResult } = this.props.queryEditor;
-    const resultIsReady = validationResult && validationResult.completed;
-    if (resultIsReady && validationResult.errors.length > 0) {
+    const resultIsReady = validationResult?.completed;
+    if (resultIsReady && validationResult?.errors?.length) {
       const errors = validationResult.errors.map((err: any) => ({
         type: 'error',
         row: err.line_number - 1,

superset-frontend/src/SqlLab/components/ShareSqlLabQuery.tsx

@@ -26,16 +26,10 @@ import CopyToClipboard from 'src/components/CopyToClipboard';
 import { storeQuery } from 'src/utils/common';
 import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import { FeatureFlag, isFeatureEnabled } from '../../featureFlags';
+import { QueryEditor } from '../types';
 
 interface ShareSqlLabQueryPropTypes {
-  queryEditor: {
-    dbId: number;
-    title: string;
-    schema: string;
-    autorun: boolean;
-    sql: string;
-    remoteId: number | null;
-  };
+  queryEditor: QueryEditor;
   addDangerToast: (msg: string) => void;
 }
 

superset-frontend/src/SqlLab/types.ts

@@ -69,3 +69,16 @@ export type Query = {
   queryLimit: number;
   limitingFactor: string;
 };
+
+export interface QueryEditor {
+  dbId?: number;
+  title: string;
+  schema: string;
+  autorun: boolean;
+  sql: string;
+  remoteId: number | null;
+  validationResult?: {
+    completed: boolean;
+    errors: SupersetError[];
+  };
+}

superset/reports/commands/execute.py

@@ -40,7 +40,6 @@ from superset.models.reports import (
 )
 from superset.reports.commands.alert import AlertCommand
 from superset.reports.commands.exceptions import (
-    ReportScheduleAlertEndGracePeriodError,
     ReportScheduleAlertGracePeriodError,
     ReportScheduleCsvFailedError,
     ReportScheduleCsvTimeout,
@@ -403,7 +402,7 @@ class BaseReportState:
 
     def is_in_grace_period(self) -> bool:
         """
-        Checks if an alert is on it's grace period
+        Checks if an alert is in it's grace period
         """
         last_success = ReportScheduleDAO.find_last_success_log(
             self._report_schedule, session=self._session
@@ -418,7 +417,7 @@ class BaseReportState:
 
     def is_in_error_grace_period(self) -> bool:
         """
-        Checks if an alert/report on error is on it's notification grace period
+        Checks if an alert/report on error is in it's notification grace period
         """
         last_success = ReportScheduleDAO.find_last_error_notification(
             self._report_schedule, session=self._session
@@ -435,7 +434,7 @@ class BaseReportState:
 
     def is_on_working_timeout(self) -> bool:
         """
-        Checks if an alert is on a working timeout
+        Checks if an alert is in a working timeout
         """
         last_working = ReportScheduleDAO.find_last_entered_working_log(
             self._report_schedule, session=self._session
@@ -533,7 +532,6 @@ class ReportSuccessState(BaseReportState):
     current_states = [ReportState.SUCCESS, ReportState.GRACE]
 
     def next(self) -> None:
-        self.set_state_and_log(ReportState.WORKING)
         if self._report_schedule.type == ReportScheduleType.ALERT:
             if self.is_in_grace_period():
                 self.set_state_and_log(
@@ -541,11 +539,23 @@ class ReportSuccessState(BaseReportState):
                     error_message=str(ReportScheduleAlertGracePeriodError()),
                 )
                 return
-            self.set_state_and_log(
-                ReportState.NOOP,
-                error_message=str(ReportScheduleAlertEndGracePeriodError()),
-            )
-            return
+            self.set_state_and_log(ReportState.WORKING)
+            try:
+                if not AlertCommand(self._report_schedule).run():
+                    self.set_state_and_log(ReportState.NOOP)
+                    return
+            except CommandException as ex:
+                self.send_error(
+                    f"Error occurred for {self._report_schedule.type}:"
+                    f" {self._report_schedule.name}",
+                    str(ex),
+                )
+                self.set_state_and_log(
+                    ReportState.ERROR,
+                    error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
+                )
+                raise ex
+
         try:
             self.send()
             self.set_state_and_log(ReportState.SUCCESS)

superset/tasks/scheduler.py

@@ -19,7 +19,7 @@ import logging
 from celery.exceptions import SoftTimeLimitExceeded
 from dateutil import parser
 
-from superset import app, is_feature_enabled
+from superset import app
 from superset.commands.exceptions import CommandException
 from superset.extensions import celery_app
 from superset.reports.commands.exceptions import ReportScheduleUnexpectedError
@@ -37,8 +37,6 @@ def scheduler() -> None:
     """
     Celery beat main scheduler for reports
     """
-    if not is_feature_enabled("ALERT_REPORTS"):
-        return
     with session_scope(nullpool=True) as session:
         active_schedules = ReportScheduleDAO.find_active(session)
         for active_schedule in active_schedules:

tests/integration_tests/reports/commands_tests.py

@@ -365,30 +365,47 @@ def create_alert_slack_chart_success():
         cleanup_report_schedule(report_schedule)
 
 
-@pytest.fixture()
-def create_alert_slack_chart_grace():
+@pytest.fixture(
+    params=["alert1",]
+)
+def create_alert_slack_chart_grace(request):
+    param_config = {
+        "alert1": {
+            "sql": "SELECT count(*) from test_table",
+            "validator_type": ReportScheduleValidatorType.OPERATOR,
+            "validator_config_json": '{"op": "<", "threshold": 10}',
+        },
+    }
     with app.app_context():
         chart = db.session.query(Slice).first()
-        report_schedule = create_report_notification(
-            slack_channel="slack_channel",
-            chart=chart,
-            report_type=ReportScheduleType.ALERT,
-        )
-        report_schedule.last_state = ReportState.GRACE
-        report_schedule.last_eval_dttm = datetime(2020, 1, 1, 0, 0)
+        example_database = get_example_database()
+        with create_test_table_context(example_database):
+            report_schedule = create_report_notification(
+                slack_channel="slack_channel",
+                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"
+                ],
+            )
+            report_schedule.last_state = ReportState.GRACE
+            report_schedule.last_eval_dttm = datetime(2020, 1, 1, 0, 0)
 
-        log = ReportExecutionLog(
-            report_schedule=report_schedule,
-            state=ReportState.SUCCESS,
-            start_dttm=report_schedule.last_eval_dttm,
-            end_dttm=report_schedule.last_eval_dttm,
-            scheduled_dttm=report_schedule.last_eval_dttm,
-        )
-        db.session.add(log)
-        db.session.commit()
-        yield report_schedule
+            log = ReportExecutionLog(
+                report_schedule=report_schedule,
+                state=ReportState.SUCCESS,
+                start_dttm=report_schedule.last_eval_dttm,
+                end_dttm=report_schedule.last_eval_dttm,
+                scheduled_dttm=report_schedule.last_eval_dttm,
+            )
+            db.session.add(log)
+            db.session.commit()
+            yield report_schedule
 
-        cleanup_report_schedule(report_schedule)
+            cleanup_report_schedule(report_schedule)
 
 
 @pytest.fixture(
@@ -1051,11 +1068,18 @@ def test_report_schedule_success_grace(create_alert_slack_chart_success):
 
 
 @pytest.mark.usefixtures("create_alert_slack_chart_grace")
-def test_report_schedule_success_grace_end(create_alert_slack_chart_grace):
+@patch("superset.reports.notifications.slack.WebClient.files_upload")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_report_schedule_success_grace_end(
+    screenshot_mock, file_upload_mock, create_alert_slack_chart_grace
+):
     """
     ExecuteReport Command: Test report schedule on grace to noop
     """
-    # set current time to within the grace period
+
+    screenshot_mock.return_value = SCREENSHOT_FILE
+
+    # set current time to after the grace period
     current_time = create_alert_slack_chart_grace.last_eval_dttm + timedelta(
         seconds=create_alert_slack_chart_grace.grace_period + 1
     )
@@ -1066,7 +1090,7 @@ def test_report_schedule_success_grace_end(create_alert_slack_chart_grace):
         ).run()
 
     db.session.commit()
-    assert create_alert_slack_chart_grace.last_state == ReportState.NOOP
+    assert create_alert_slack_chart_grace.last_state == ReportState.SUCCESS
 
 
 @pytest.mark.usefixtures("create_alert_email_chart")

tests/integration_tests/reports/scheduler_tests.py

@@ -115,25 +115,3 @@ def test_scheduler_celery_no_timeout_utc(execute_mock):
         db.session.delete(report_schedule)
         db.session.commit()
         app.config["ALERT_REPORTS_WORKING_TIME_OUT_KILL"] = True
-
-
-@patch("superset.tasks.scheduler.is_feature_enabled")
-@patch("superset.tasks.scheduler.execute.apply_async")
-def test_scheduler_feature_flag_off(execute_mock, is_feature_enabled):
-    """
-    Reports scheduler: Test scheduler with feature flag off
-    """
-    with app.app_context():
-        is_feature_enabled.return_value = False
-        report_schedule = insert_report_schedule(
-            type=ReportScheduleType.ALERT,
-            name="report",
-            crontab="0 9 * * *",
-            timezone="UTC",
-        )
-
-        with freeze_time("2020-01-01T09:00:00Z"):
-            scheduler()
-            execute_mock.assert_not_called()
-        db.session.delete(report_schedule)
-        db.session.commit()