fix(dashboard): Fill form with the latest values when undo in native filters (#16851)

* Set undoFormValues

* Reorganize

* Revert check

* Fix and clean up

* Fix pre-filter and sort values

(cherry picked from commit d3f6145aba)

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-frontend/src/dashboard/components/FiltersBadge/selectors.ts

@@ -212,7 +212,7 @@ export const selectIndicatorsForChart = (
 };
 
 const cachedNativeIndicatorsForChart = {};
-let cachedNativeFilterDataForChart: any = {};
+const cachedNativeFilterDataForChart: any = {};
 const defaultChartConfig = {};
 export const selectNativeIndicatorsForChart = (
   nativeFilters: Filters,
@@ -230,10 +230,10 @@ export const selectNativeIndicatorsForChart = (
     cachedNativeIndicatorsForChart[chartId] &&
     areObjectsEqual(cachedFilterData?.appliedColumns, appliedColumns) &&
     areObjectsEqual(cachedFilterData?.rejectedColumns, rejectedColumns) &&
-    cachedNativeFilterDataForChart?.nativeFilters === nativeFilters &&
-    cachedNativeFilterDataForChart?.dashboardLayout === dashboardLayout &&
-    cachedNativeFilterDataForChart?.chartConfiguration === chartConfiguration &&
-    cachedNativeFilterDataForChart?.dataMask === dataMask
+    cachedFilterData?.nativeFilters === nativeFilters &&
+    cachedFilterData?.dashboardLayout === dashboardLayout &&
+    cachedFilterData?.chartConfiguration === chartConfiguration &&
+    cachedFilterData?.dataMask === dataMask
   ) {
     return cachedNativeIndicatorsForChart[chartId];
   }
@@ -326,14 +326,11 @@ export const selectNativeIndicatorsForChart = (
   }
   const indicators = crossFilterIndicators.concat(nativeFilterIndicators);
   cachedNativeIndicatorsForChart[chartId] = indicators;
-  cachedNativeFilterDataForChart = {
-    ...cachedNativeFilterDataForChart,
+  cachedNativeFilterDataForChart[chartId] = {
     nativeFilters,
     dashboardLayout,
     chartConfiguration,
     dataMask,
-  };
-  cachedNativeFilterDataForChart[chartId] = {
     appliedColumns,
     rejectedColumns,
   };

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx

@@ -31,6 +31,7 @@ import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
 import { updateDataMask, clearDataMask } from 'src/dataMask/actions';
 import { DataMaskStateWithId, DataMaskWithId } from 'src/dataMask/types';
 import { useImmer } from 'use-immer';
+import { isEmpty, isEqual } from 'lodash';
 import { testWithId } from 'src/utils/testUtils';
 import { Filter } from 'src/dashboard/components/nativeFilters/types';
 import Loading from 'src/components/Loading';
@@ -162,28 +163,6 @@ const FilterBar: React.FC<FiltersBarProps> = ({
   const filterValues = Object.values<Filter>(filters);
   const [isFilterSetChanged, setIsFilterSetChanged] = useState(false);
 
-  useEffect(() => {
-    setDataMaskSelected(() => dataMaskApplied);
-  }, [JSON.stringify(dataMaskApplied), setDataMaskSelected]);
-
-  // reset filter state if filter type changes
-  useEffect(() => {
-    setDataMaskSelected(draft => {
-      Object.values(filters).forEach(filter => {
-        if (
-          filter.filterType !== previousFilters?.[filter.id]?.filterType &&
-          previousFilters?.[filter.id]?.filterType !== undefined
-        ) {
-          draft[filter.id] = getInitialDataMask(filter.id) as DataMaskWithId;
-        }
-      });
-    });
-  }, [
-    JSON.stringify(filters),
-    JSON.stringify(previousFilters),
-    setDataMaskSelected,
-  ]);
-
   const handleFilterSelectionChange = (
     filter: Pick<Filter, 'id'> & Partial<Filter>,
     dataMask: Partial<DataMask>,
@@ -232,6 +211,37 @@ const FilterBar: React.FC<FiltersBarProps> = ({
     [history],
   );
 
+  useEffect(() => {
+    if (previousFilters) {
+      const updates = {};
+      Object.values(filters).forEach(currentFilter => {
+        const currentType = currentFilter.filterType;
+        const currentTargets = currentFilter.targets;
+        const currentDataMask = currentFilter.defaultDataMask;
+        const previousFilter = previousFilters?.[currentFilter.id];
+        const previousType = previousFilter?.filterType;
+        const previousTargets = previousFilter?.targets;
+        const previousDataMask = previousFilter?.defaultDataMask;
+        const typeChanged = currentType !== previousType;
+        const targetsChanged = !isEqual(currentTargets, previousTargets);
+        const dataMaskChanged = !isEqual(currentDataMask, previousDataMask);
+
+        if (typeChanged || targetsChanged || dataMaskChanged) {
+          updates[currentFilter.id] = getInitialDataMask(currentFilter.id);
+        }
+      });
+
+      if (!isEmpty(updates)) {
+        setDataMaskSelected(draft => ({ ...draft, ...updates }));
+        Object.keys(updates).forEach(key => dispatch(clearDataMask(key)));
+      }
+    }
+  }, [JSON.stringify(filters), JSON.stringify(previousFilters)]);
+
+  useEffect(() => {
+    setDataMaskSelected(() => dataMaskApplied);
+  }, [JSON.stringify(dataMaskApplied), setDataMaskSelected]);
+
   const dataMaskAppliedText = JSON.stringify(dataMaskApplied);
   useEffect(() => {
     publishDataMask(dataMaskApplied);

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

@@ -315,12 +315,15 @@ const FiltersConfigForm = (
   const [activeFilterPanelKey, setActiveFilterPanelKey] = useState<
     string | string[]
   >(FilterPanels.basic.key);
-
+  const [undoFormValues, setUndoFormValues] = useState<Record<
+    string,
+    any
+  > | null>(null);
   const forceUpdate = useForceUpdate();
   const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
   const defaultFormFilter = useMemo(() => ({}), []);
-  const formFilter =
-    form.getFieldValue('filters')?.[filterId] || defaultFormFilter;
+  const formValues = form.getFieldValue('filters')?.[filterId];
+  const formFilter = formValues || undoFormValues || defaultFormFilter;
 
   const nativeFilterItems = getChartMetadataRegistry().items;
   const nativeFilterVizTypes = Object.entries(nativeFilterItems)
@@ -346,11 +349,11 @@ const FiltersConfigForm = (
 
   const showTimeRangePicker = useMemo(() => {
     const currentDataset = Object.values(loadedDatasets).find(
-      dataset => dataset.id === formFilter.dataset?.value,
+      dataset => dataset.id === formFilter?.dataset?.value,
     );
 
     return currentDataset ? hasTemporalColumns(currentDataset) : true;
-  }, [formFilter.dataset?.value, loadedDatasets]);
+  }, [formFilter?.dataset?.value, loadedDatasets]);
 
   // @ts-ignore
   const hasDataset = !!nativeFilterItems[formFilter?.filterType]?.value
@@ -368,7 +371,7 @@ const FiltersConfigForm = (
         forceUpdate,
         form,
         filterId,
-        filterType: formFilter.filterType,
+        filterType: formFilter?.filterType,
         filterToEdit,
         formFilter,
         removed,
@@ -380,31 +383,6 @@ const FiltersConfigForm = (
   // @ts-ignore
   const enableNoResults = !!nativeFilterItem.value?.enableNoResults;
 
-  useEffect(() => {
-    if (datasetId) {
-      cachedSupersetGet({
-        endpoint: `/api/v1/dataset/${datasetId}`,
-      })
-        .then((response: JsonResponse) => {
-          setMetrics(response.json?.result?.metrics);
-          const dataset = response.json?.result;
-          // modify the response to fit structure expected by AdhocFilterControl
-          dataset.type = dataset.datasource_type;
-          dataset.filter_select = true;
-          setDatasetDetails(dataset);
-        })
-        .catch((response: SupersetApiError) => {
-          addDangerToast(response.message);
-        });
-    }
-  }, [datasetId]);
-
-  useImperativeHandle(ref, () => ({
-    changeTab(tab: 'configuration' | 'scoping') {
-      setActiveTabKey(tab);
-    },
-  }));
-
   const hasMetrics = hasColumn && !!metrics.length;
 
   const hasFilledDataset =
@@ -418,8 +396,6 @@ const FiltersConfigForm = (
 
   const isDataDirty = formFilter?.isDataDirty ?? true;
 
-  useBackendFormUpdate(form, filterId);
-
   const setNativeFilterFieldValuesWrapper = (values: object) => {
     setNativeFilterFieldValues(form, filterId, values);
     setError('');
@@ -513,20 +489,6 @@ const FiltersConfigForm = (
   const showDataset =
     !datasetId || datasetDetails || formFilter?.dataset?.label;
 
-  useEffect(() => {
-    if (hasDataset && hasFilledDataset && hasDefaultValue && isDataDirty) {
-      refreshHandler();
-    }
-  }, [
-    hasDataset,
-    hasFilledDataset,
-    hasDefaultValue,
-    formFilter,
-    isDataDirty,
-    refreshHandler,
-    showDataset,
-  ]);
-
   const formChanged = useCallback(() => {
     form.setFields([
       {
@@ -550,15 +512,21 @@ const FiltersConfigForm = (
   }));
 
   const parentFilter = parentFilterOptions.find(
-    ({ value }) => value === filterToEdit?.cascadeParentIds[0],
+    ({ value }) =>
+      value === formFilter?.parentFilter?.value ||
+      value === filterToEdit?.cascadeParentIds?.[0],
   );
 
   const hasParentFilter = !!parentFilter;
 
   const hasPreFilter =
-    !!filterToEdit?.adhoc_filters || !!filterToEdit?.time_range;
+    !!formFilter?.adhoc_filters ||
+    !!formFilter?.time_range ||
+    !!filterToEdit?.adhoc_filters?.length ||
+    !!filterToEdit?.time_range;
 
   const hasSorting =
+    typeof formFilter?.controlValues?.sortAscending === 'boolean' ||
     typeof filterToEdit?.controlValues?.sortAscending === 'boolean';
 
   let sort = filterToEdit?.controlValues?.sortAscending;
@@ -604,7 +572,7 @@ const FiltersConfigForm = (
     formFilter?.filterType === 'filter_range';
 
   const initialDefaultValue =
-    formFilter.filterType === filterToEdit?.filterType
+    formFilter?.filterType === filterToEdit?.filterType
       ? filterToEdit?.defaultDataMask
       : null;
 
@@ -622,6 +590,62 @@ const FiltersConfigForm = (
       .some(key => controlItems[key].checked);
   }
 
+  const ParentSelect = ({
+    value,
+    ...rest
+  }: {
+    value?: { value: string | number };
+  }) => (
+    <Select
+      ariaLabel={t('Parent filter')}
+      placeholder={t('None')}
+      options={parentFilterOptions}
+      allowClear
+      value={value?.value}
+      {...rest}
+    />
+  );
+
+  useEffect(() => {
+    if (datasetId) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      })
+        .then((response: JsonResponse) => {
+          setMetrics(response.json?.result?.metrics);
+          const dataset = response.json?.result;
+          // modify the response to fit structure expected by AdhocFilterControl
+          dataset.type = dataset.datasource_type;
+          dataset.filter_select = true;
+          setDatasetDetails(dataset);
+        })
+        .catch((response: SupersetApiError) => {
+          addDangerToast(response.message);
+        });
+    }
+  }, [datasetId]);
+
+  useImperativeHandle(ref, () => ({
+    changeTab(tab: 'configuration' | 'scoping') {
+      setActiveTabKey(tab);
+    },
+  }));
+
+  useBackendFormUpdate(form, filterId);
+
+  useEffect(() => {
+    if (hasDataset && hasFilledDataset && hasDefaultValue && isDataDirty) {
+      refreshHandler();
+    }
+  }, [
+    hasDataset,
+    hasFilledDataset,
+    hasDefaultValue,
+    isDataDirty,
+    refreshHandler,
+    showDataset,
+  ]);
+
   useEffect(() => {
     const activeFilterPanelKey = [FilterPanels.basic.key];
     if (hasCheckedAdvancedControl) {
@@ -652,21 +676,20 @@ const FiltersConfigForm = (
     JSON.stringify(loadedDatasets),
   ]);
 
-  const ParentSelect = ({
-    value,
-    ...rest
-  }: {
-    value?: { value: string | number };
-  }) => (
-    <Select
-      ariaLabel={t('Parent filter')}
-      placeholder={t('None')}
-      options={parentFilterOptions}
-      allowClear
-      value={value?.value}
-      {...rest}
-    />
-  );
+  useEffect(() => {
+    // just removed, saving current form items for eventual undo
+    if (removed) {
+      setUndoFormValues(formValues);
+    }
+  }, [removed]);
+
+  useEffect(() => {
+    // the filter was just restored after undo
+    if (undoFormValues && !removed) {
+      setNativeFilterFieldValues(form, filterId, undoFormValues);
+      setUndoFormValues(null);
+    }
+  }, [formValues, filterId, form, removed, undoFormValues]);
 
   if (removed) {
     return <RemovedFilter onClick={() => restoreFilter(filterId)} />;
@@ -814,7 +837,7 @@ const FiltersConfigForm = (
                 formChanged();
               }}
             >
-              {formFilter.filterType && (
+              {!removed && (
                 <StyledRowSubFormItem
                   name={['filters', filterId, 'defaultDataMask']}
                   initialValue={initialDefaultValue}

superset-frontend/src/dataMask/actions.ts

@@ -71,14 +71,7 @@ export function updateDataMask(
 }
 
 export function clearDataMask(filterId: string | number) {
-  return updateDataMask(
-    filterId,
-    getInitialDataMask(filterId, {
-      filterState: {
-        value: null,
-      },
-    }),
-  );
+  return updateDataMask(filterId, getInitialDataMask(filterId));
 }
 
 export type AnyDataMaskAction =

superset-frontend/src/dataMask/reducer.ts

@@ -56,9 +56,7 @@ export function getInitialDataMask(
   return {
     ...otherProps,
     extraFormData: {},
-    filterState: {
-      value: undefined,
-    },
+    filterState: {},
     ownState: {},
     ...moreProps,
   } as DataMaskWithId;

superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

@@ -37,11 +37,7 @@ import { useImmerReducer } from 'use-immer';
 import { FormItemProps } from 'antd/lib/form';
 import { PluginFilterSelectProps, SelectValue } from './types';
 import { StyledFormItem, FilterPluginStyle, StatusMessage } from '../common';
-import {
-  formatFilterValue,
-  getDataRecordFormatter,
-  getSelectExtraFormData,
-} from '../../utils';
+import { getDataRecordFormatter, getSelectExtraFormData } from '../../utils';
 
 type DataMaskAction =
   | { type: 'ownState'; ownState: JsonObject }
@@ -104,6 +100,15 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
     extraFormData: {},
     filterState,
   });
+  const datatype: GenericDataType = coltypeMap[col];
+  const labelFormatter = useMemo(
+    () =>
+      getDataRecordFormatter({
+        timeFormatter: smartDateDetailedFormatter,
+      }),
+    [],
+  );
+
   const updateDataMask = useCallback(
     (values: SelectValue) => {
       const emptyFilter =
@@ -124,7 +129,9 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
         filterState: {
           ...filterState,
           label: values?.length
-            ? `${(values || []).map(formatFilterValue).join(', ')}${suffix}`
+            ? `${(values || [])
+                .map(value => labelFormatter(value, datatype))
+                .join(', ')}${suffix}`
             : undefined,
           value:
             appSection === AppSection.FILTER_CONFIG_MODAL && defaultToFirstItem
@@ -133,14 +140,17 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
         },
       });
     },
+    // eslint-disable-next-line react-hooks/exhaustive-deps
     [
       appSection,
       col,
+      datatype,
       defaultToFirstItem,
       dispatchDataMask,
       enableEmptyFilter,
       inverseSelection,
       JSON.stringify(filterState),
+      labelFormatter,
     ],
   );
 
@@ -187,15 +197,6 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
     unsetFocusedFilter();
   };
 
-  const datatype: GenericDataType = coltypeMap[col];
-  const labelFormatter = useMemo(
-    () =>
-      getDataRecordFormatter({
-        timeFormatter: smartDateDetailedFormatter,
-      }),
-    [],
-  );
-
   const handleChange = (value?: SelectValue | number | string) => {
     const values = ensureIsArray(value);
     if (values.length === 0) {

superset-frontend/src/filters/utils.ts

@@ -117,18 +117,3 @@ export function getDataRecordFormatter({
     return String(value);
   };
 }
-
-export function formatFilterValue(
-  value: string | number | boolean | null,
-): string {
-  if (value === null) {
-    return NULL_STRING;
-  }
-  if (typeof value === 'string') {
-    return value;
-  }
-  if (typeof value === 'number') {
-    return String(value);
-  }
-  return value ? TRUE_STRING : FALSE_STRING;
-}

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()