refactor(ReportModal): simplify state reducer and improve error handling (#19942)

superset-frontend/src/components/ReportModal/index.tsx

@@ -19,16 +19,15 @@
 import React, {
   useState,
   useEffect,
-  useCallback,
   useReducer,
-  Reducer,
   FunctionComponent,
+  useCallback,
+  useMemo,
 } from 'react';
 import { t, SupersetTheme } from '@superset-ui/core';
 import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import { useDispatch, useSelector } from 'react-redux';
 import { addReport, editReport } from 'src/reports/actions/reports';
-import { AlertObject } from 'src/views/CRUD/alert/types';
 
 import Alert from 'src/components/Alert';
 import TimezoneSelector from 'src/components/TimezoneSelector';
@@ -37,6 +36,12 @@ import Icons from 'src/components/Icons';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import { CronError } from 'src/components/CronPicker';
 import { RadioChangeEvent } from 'src/components';
+import { ChartState } from 'src/explore/types';
+import {
+  ReportCreationMethod,
+  ReportRecipientType,
+  ReportScheduleType,
+} from 'src/reports/types';
 import {
   antDErrorAlertStyles,
   StyledModal,
@@ -65,30 +70,17 @@ export interface ReportObject {
   log_retention: number;
   name: string;
   owners: number[];
-  recipients: [{ recipient_config_json: { target: string }; type: string }];
+  recipients: [
+    { recipient_config_json: { target: string }; type: ReportRecipientType },
+  ];
   report_format: string;
   timezone: string;
-  type: string;
+  type: ReportScheduleType;
   validator_config_json: {} | null;
   validator_type: string;
   working_timeout: number;
   creation_method: string;
   force_screenshot: boolean;
-  error: string;
-}
-
-interface ChartObject {
-  id: number;
-  chartAlert: string;
-  chartStatus: string;
-  chartUpdateEndTime: number;
-  chartUpdateStartTime: number;
-  latestQueryFormData: object;
-  sliceFormData: Record<string, any>;
-  queryController: { abort: () => {} };
-  queriesResponse: object;
-  triggerQuery: boolean;
-  lastRendered: number;
 }
 
 interface ReportProps {
@@ -98,40 +90,13 @@ interface ReportProps {
   show: boolean;
   userId: number;
   userEmail: string;
+  chart?: ChartState;
+  chartName?: string;
   dashboardId?: number;
-  chart?: ChartObject;
+  dashboardName?: string;
-  creationMethod: string;
+  creationMethod: ReportCreationMethod;
 }
 
-interface ReportPayloadType {
-  name: string;
-  value: string;
-}
-
-enum ActionType {
-  inputChange,
-  fetched,
-  reset,
-  error,
-}
-
-type ReportActionType =
-  | {
-      type: ActionType.inputChange;
-      payload: ReportPayloadType;
-    }
-  | {
-      type: ActionType.fetched;
-      payload: Partial<ReportObject>;
-    }
-  | {
-      type: ActionType.reset;
-    }
-  | {
-      type: ActionType.error;
-      payload: { name?: string[] };
-    };
-
 const TEXT_BASED_VISUALIZATION_TYPES = [
   'pivot_table',
   'pivot_table_v2',
@@ -145,41 +110,16 @@ const NOTIFICATION_FORMATS = {
   CSV: 'CSV',
 };
 
-const defaultErrorMsg = t(
+const INITIAL_STATE = {
-  'We were unable to create your report. Please try again.',
+  crontab: '0 12 * * 1',
-);
+};
 
-const reportReducer = (
+type ReportObjectState = Partial<ReportObject> & {
-  state: Partial<ReportObject> | null,
+  error?: string;
-  action: ReportActionType,
+  /**
-): Partial<ReportObject> | null => {
+   * Is submitting changes to the backend.
-  const initialState = {
+   */
-    name: 'Weekly Report',
+  isSubmitting?: boolean;
-    crontab: '0 12 * * 1',
-  };
-
-  switch (action.type) {
-    case ActionType.inputChange:
-      return {
-        ...initialState,
-        ...state,
-        [action.payload.name]: action.payload.value,
-      };
-    case ActionType.fetched:
-      return {
-        ...initialState,
-        ...action.payload,
-      };
-    case ActionType.reset:
-      return { ...initialState };
-    case ActionType.error:
-      return {
-        ...state,
-        error: action.payload?.name?.[0] || defaultErrorMsg,
-      };
-    default:
-      return state;
-  }
 };
 
 const ReportModal: FunctionComponent<ReportProps> = ({
@@ -190,45 +130,64 @@ const ReportModal: FunctionComponent<ReportProps> = ({
 }) => {
   const vizType = props.chart?.sliceFormData?.viz_type;
   const isChart = !!props.chart;
-  const defaultNotificationFormat =
+  const isTextBasedChart =
-    isChart && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType)
+    isChart && vizType && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType);
-      ? NOTIFICATION_FORMATS.TEXT
+  const defaultNotificationFormat = isTextBasedChart
-      : NOTIFICATION_FORMATS.PNG;
+    ? NOTIFICATION_FORMATS.TEXT
-  const [currentReport, setCurrentReport] = useReducer<
+    : NOTIFICATION_FORMATS.PNG;
-    Reducer<Partial<ReportObject> | null, ReportActionType>
+  const entityName = props.dashboardName || props.chartName;
-  >(reportReducer, null);
+  const initialState: ReportObjectState = useMemo(
-  const onReducerChange = useCallback((type: any, payload: any) => {
+    () => ({
-    setCurrentReport({ type, payload });
+      ...INITIAL_STATE,
-  }, []);
+      name: entityName
+        ? t('Weekly Report for %s', entityName)
+        : t('Weekly Report'),
+    }),
+    [entityName],
+  );
+
+  const reportReducer = useCallback(
+    (state: ReportObjectState | null, action: 'reset' | ReportObjectState) => {
+      if (action === 'reset') {
+        return initialState;
+      }
+      return {
+        ...state,
+        ...action,
+      };
+    },
+    [initialState],
+  );
+
+  const [currentReport, setCurrentReport] = useReducer(
+    reportReducer,
+    initialState,
+  );
   const [cronError, setCronError] = useState<CronError>();
+
   const dispatch = useDispatch();
-  // Report fetch logic
+  const reports = useSelector<any, ReportObject>(state => state.reports);
-  const reports = useSelector<any, AlertObject>(state => state.reports);
   const isEditMode = reports && Object.keys(reports).length;
 
   useEffect(() => {
     if (isEditMode) {
       const reportsIds = Object.keys(reports);
       const report = reports[reportsIds[0]];
-      setCurrentReport({
+      setCurrentReport(report);
-        type: ActionType.fetched,
-        payload: report,
-      });
     } else {
-      setCurrentReport({
+      setCurrentReport('reset');
-        type: ActionType.reset,
-      });
     }
-  }, [reports]);
+  }, [isEditMode, reports]);
 
   const onSave = async () => {
     // Create new Report
     const newReportValues: Partial<ReportObject> = {
-      crontab: currentReport?.crontab,
+      type: 'Report',
+      active: true,
+      force_screenshot: false,
+      creation_method: props.creationMethod,
       dashboard: props.dashboardId,
       chart: props.chart?.id,
-      description: currentReport?.description,
-      name: currentReport?.name,
       owners: [props.userId],
       recipients: [
         {
@@ -236,28 +195,28 @@ const ReportModal: FunctionComponent<ReportProps> = ({
           type: 'Email',
         },
       ],
-      type: 'Report',
+      name: currentReport.name,
-      creation_method: props.creationMethod,
+      description: currentReport.description,
-      active: true,
+      crontab: currentReport.crontab,
-      report_format: currentReport?.report_format || defaultNotificationFormat,
+      report_format: currentReport.report_format || defaultNotificationFormat,
-      timezone: currentReport?.timezone,
+      timezone: currentReport.timezone,
-      force_screenshot: false,
     };
 
-    if (isEditMode) {
+    setCurrentReport({ isSubmitting: true, error: undefined });
-      await dispatch(
+    try {
-        editReport(currentReport?.id, newReportValues as ReportObject),
+      if (isEditMode) {
-      );
+        await dispatch(
-      onHide();
+          editReport(currentReport.id, newReportValues as ReportObject),
-    } else {
+        );
-      try {
+      } else {
         await dispatch(addReport(newReportValues as ReportObject));
-        onHide();
-      } catch (e) {
-        const { message } = await getClientErrorObject(e);
-        onReducerChange(ActionType.error, message);
       }
+      onHide();
+    } catch (e) {
+      const { error } = await getClientErrorObject(e);
+      setCurrentReport({ error });
     }
+    setCurrentReport({ isSubmitting: false });
 
     if (onReportAdd) onReportAdd();
   };
@@ -266,7 +225,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
     <StyledIconWrapper>
       <Icons.Calendar />
       <span className="text">
-        {isEditMode ? t('Edit Email Report') : t('New Email Report')}
+        {isEditMode ? t('Edit email report') : t('Schedule a new email report')}
       </span>
     </StyledIconWrapper>
   );
@@ -280,7 +239,8 @@ const ReportModal: FunctionComponent<ReportProps> = ({
         key="submit"
         buttonStyle="primary"
         onClick={onSave}
-        disabled={!currentReport?.name}
+        disabled={!currentReport.name}
+        loading={currentReport.isSubmitting}
       >
         {isEditMode ? t('Save') : t('Add')}
       </StyledFooterButton>
@@ -290,19 +250,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({
   const renderMessageContentSection = (
     <>
       <StyledMessageContentTitle>
-        <h4>{t('Message Content')}</h4>
+        <h4>{t('Message content')}</h4>
       </StyledMessageContentTitle>
       <div className="inline-container">
         <StyledRadioGroup
           onChange={(event: RadioChangeEvent) => {
-            onReducerChange(ActionType.inputChange, {
+            setCurrentReport({ report_format: event.target.value });
-              name: 'report_format',
-              value: event.target.value,
-            });
           }}
-          value={currentReport?.report_format || defaultNotificationFormat}
+          value={currentReport.report_format || defaultNotificationFormat}
         >
-          {TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) && (
+          {isTextBasedChart && (
             <StyledRadio value={NOTIFICATION_FORMATS.TEXT}>
               {t('Text embedded in email')}
             </StyledRadio>
@@ -318,15 +275,6 @@ const ReportModal: FunctionComponent<ReportProps> = ({
     </>
   );
 
-  const errorAlert = () => (
-    <Alert
-      type="error"
-      css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)}
-      message={t('Report Creation Error')}
-      description={currentReport?.error}
-    />
-  );
-
   return (
     <StyledModal
       show={show}
@@ -340,15 +288,12 @@ const ReportModal: FunctionComponent<ReportProps> = ({
         <LabeledErrorBoundInput
           id="name"
           name="name"
-          value={currentReport?.name || ''}
+          value={currentReport.name || ''}
-          placeholder={t('Weekly Report')}
+          placeholder={initialState.name}
           required
           validationMethods={{
             onChange: ({ target }: { target: HTMLInputElement }) =>
-              onReducerChange(ActionType.inputChange, {
+              setCurrentReport({ name: target.value }),
-                name: target.name,
-                value: target.value,
-              }),
           }}
           label="Report Name"
           data-test="report-name-test"
@@ -358,11 +303,9 @@ const ReportModal: FunctionComponent<ReportProps> = ({
           name="description"
           value={currentReport?.description || ''}
           validationMethods={{
-            onChange: ({ target }: { target: HTMLInputElement }) =>
+            onChange: ({ target }: { target: HTMLInputElement }) => {
-              onReducerChange(ActionType.inputChange, {
+              setCurrentReport({ description: target.value });
-                name: target.name,
+            },
-                value: target.value,
-              }),
           }}
           label={t('Description')}
           placeholder={t(
@@ -378,17 +321,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({
           <h4 css={(theme: SupersetTheme) => SectionHeaderStyle(theme)}>
             {t('Schedule')}
           </h4>
-          <p>{t('Scheduled reports will be sent to your email as a PNG')}</p>
+          <p>
+            {t('A screenshot of the dashboard will be sent to your email at')}
+          </p>
         </StyledScheduleTitle>
 
         <StyledCronPicker
           clearButton={false}
-          value={t(currentReport?.crontab || '0 12 * * 1')}
+          value={currentReport.crontab || '0 12 * * 1'}
           setValue={(newValue: string) => {
-            onReducerChange(ActionType.inputChange, {
+            setCurrentReport({ crontab: newValue });
-              name: 'crontab',
-              value: newValue,
-            });
           }}
           onError={setCronError}
         />
@@ -400,17 +342,25 @@ const ReportModal: FunctionComponent<ReportProps> = ({
           {t('Timezone')}
         </div>
         <TimezoneSelector
+          timezone={currentReport.timezone}
           onTimezoneChange={value => {
-            setCurrentReport({
+            setCurrentReport({ timezone: value });
-              type: ActionType.inputChange,
-              payload: { name: 'timezone', value },
-            });
           }}
-          timezone={currentReport?.timezone}
         />
         {isChart && renderMessageContentSection}
       </StyledBottomSection>
-      {currentReport?.error && errorAlert()}
+      {currentReport.error && (
+        <Alert
+          type="error"
+          css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)}
+          message={
+            isEditMode
+              ? t('Failed to update report')
+              : t('Failed to create report')
+          }
+          description={currentReport.error}
+        />
+      )}
     </StyledModal>
   );
 };

superset-frontend/src/components/ReportModal/styles.tsx

@@ -111,7 +111,8 @@ export const StyledRadioGroup = styled(Radio.Group)`
 export const antDErrorAlertStyles = (theme: SupersetTheme) => css`
   border: ${theme.colors.error.base} 1px solid;
   padding: ${theme.gridUnit * 4}px;
-  margin: ${theme.gridUnit * 8}px ${theme.gridUnit * 4}px;
+  margin: ${theme.gridUnit * 4}px;
+  margin-top: 0;
   color: ${theme.colors.error.dark2};
   .ant-alert-message {
     font-size: ${theme.typography.sizes.m}px;

superset-frontend/src/dashboard/components/Header/index.jsx

@@ -665,6 +665,7 @@ class Header extends React.PureComponent {
               userId={user.userId}
               userEmail={user.email}
               dashboardId={dashboardInfo.id}
+              dashboardName={dashboardInfo.name}
               creationMethod="dashboards"
             />
           )}

superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx

@@ -24,7 +24,6 @@ import ReportModal from 'src/components/ReportModal';
 import { ExplorePageState } from 'src/explore/reducers/getInitialState';
 import DeleteModal from 'src/components/DeleteModal';
 import { deleteActiveReport } from 'src/reports/actions/reports';
-import { ChartState } from 'src/explore/types';
 
 type ReportMenuItemsProps = {
   report: Record<string, any>;
@@ -41,16 +40,11 @@ export const ExploreReport = ({
   setIsDeleting,
 }: ReportMenuItemsProps) => {
   const dispatch = useDispatch();
-  const chart = useSelector<ExplorePageState, ChartState | undefined>(state => {
+  const { chart, chartName } = useSelector((state: ExplorePageState) => ({
-    if (!state.charts) {
+    chart: Object.values(state.charts || {})[0],
-      return undefined;
+    chartName: state.explore.sliceName,
-    }
+  }));
-    const charts = Object.values(state.charts);
+
-    if (charts.length > 0) {
-      return charts[0];
-    }
-    return undefined;
-  });
   const { userId, email } = useSelector<
     ExplorePageState,
     { userId?: number; email?: string }
@@ -69,6 +63,7 @@ export const ExploreReport = ({
         userId={userId}
         userEmail={email}
         chart={chart}
+        chartName={chartName}
         creationMethod="charts"
       />
       {isDeleting && (

superset-frontend/src/reports/actions/reports.js

@@ -111,22 +111,14 @@ export const addReport = report => dispatch =>
 
 export const EDIT_REPORT = 'EDIT_REPORT';
 
-export function editReport(id, report) {
+export const editReport = (id, report) => dispatch =>
-  return function (dispatch) {
+  SupersetClient.put({
-    SupersetClient.put({
+    endpoint: `/api/v1/report/${id}`,
-      endpoint: `/api/v1/report/${id}`,
+    jsonPayload: report,
-      jsonPayload: report,
+  }).then(({ json }) => {
-    })
+    dispatch({ type: EDIT_REPORT, json });
-      .then(({ json }) => {
+    dispatch(addSuccessToast(t('Report updated')));
-        dispatch({ type: EDIT_REPORT, json });
+  });
-      })
-      .catch(() =>
-        dispatch(
-          addDangerToast(t('An error occurred while editing this report.')),
-        ),
-      );
-  };
-}
 
 export function toggleActive(report, isActive) {
   return function toggleActiveThunk(dispatch) {

superset-frontend/src/reports/types.ts

@@ -0,0 +1,26 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
+export type ReportCreationMethod = 'charts' | 'dashboards' | 'alerts_reports';
+
+export type ReportRecipientType = 'Email' | 'Slack';

superset-frontend/src/utils/getClientErrorObject.ts

@@ -29,15 +29,15 @@ export type ClientErrorObject = {
   error: string;
   errors?: SupersetError[];
   link?: string;
-  // marshmallow field validation returns the error mssage in the format
-  // of { field: [msg1, msg2] }
   message?: string;
   severity?: string;
   stacktrace?: string;
   statusText?: string;
 } & Partial<SupersetClientResponse>;
 
-interface ResponseWithTimeout extends Response {
+// see rejectAfterTimeout.ts
+interface TimeoutError {
+  statusText: 'timeout';
   timeout: number;
 }
 
@@ -48,7 +48,13 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
     error.error = error.description = error.errors[0].message;
     error.link = error.errors[0]?.extra?.link;
   }
-
+  // Marshmallow field validation returns the error mssage in the format
+  // of { message: { field1: [msg1, msg2], field2: [msg], } }
+  if (error.message && typeof error.message === 'object' && !error.error) {
+    error.error =
+      Object.values(error.message as Record<string, string[]>)[0]?.[0] ||
+      t('Invalid input');
+  }
   if (error.stack) {
     error = {
       ...error,
@@ -68,78 +74,95 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
 }
 
 export function getClientErrorObject(
-  response: SupersetClientResponse | ResponseWithTimeout | string,
+  response:
+    | SupersetClientResponse
+    | TimeoutError
+    | { response: Response }
+    | string,
 ): Promise<ClientErrorObject> {
   // takes a SupersetClientResponse as input, attempts to read response as Json if possible,
   // and returns a Promise that resolves to a plain object with error key and text value.
   return new Promise(resolve => {
     if (typeof response === 'string') {
       resolve({ error: response });
-    } else {
+      return;
-      const responseObject =
-        response instanceof Response ? response : response.response;
-      if (responseObject && !responseObject.bodyUsed) {
-        // attempt to read the body as json, and fallback to text. we must clone the
-        // response in order to fallback to .text() because Response is single-read
-        responseObject
-          .clone()
-          .json()
-          .then(errorJson => {
-            const error = { ...responseObject, ...errorJson };
-            resolve(parseErrorJson(error));
-          })
-          .catch(() => {
-            // fall back to reading as text
-            responseObject.text().then((errorText: any) => {
-              resolve({ ...responseObject, error: errorText });
-            });
-          });
-      } else if (
-        'statusText' in response &&
-        response.statusText === 'timeout' &&
-        'timeout' in response
-      ) {
-        resolve({
-          ...responseObject,
-          error: 'Request timed out',
-          errors: [
-            {
-              error_type: ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
-              extra: {
-                timeout: response.timeout / 1000,
-                issue_codes: [
-                  {
-                    code: 1000,
-                    message: t(
-                      'Issue 1000 - The dataset is too large to query.',
-                    ),
-                  },
-                  {
-                    code: 1001,
-                    message: t(
-                      'Issue 1001 - The database is under an unusual load.',
-                    ),
-                  },
-                ],
-              },
-              level: 'error',
-              message: 'Request timed out',
-            },
-          ],
-        });
-      } else {
-        // fall back to Response.statusText or generic error of we cannot read the response
-        let error = (response as any).statusText || (response as any).message;
-        if (!error) {
-          // eslint-disable-next-line no-console
-          console.error('non-standard error:', response);
-          error = t('An error occurred');
-        }
-        resolve({
-          ...responseObject,
-          error,
-        });
-      }
     }
+
+    if (
+      response instanceof TypeError &&
+      response.message === 'Failed to fetch'
+    ) {
+      resolve({
+        error: t('Network error'),
+      });
+      return;
+    }
+
+    if (
+      'timeout' in response &&
+      'statusText' in response &&
+      response.statusText === 'timeout'
+    ) {
+      resolve({
+        ...response,
+        error: t('Request timed out'),
+        errors: [
+          {
+            error_type: ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
+            extra: {
+              timeout: response.timeout / 1000,
+              issue_codes: [
+                {
+                  code: 1000,
+                  message: t('Issue 1000 - The dataset is too large to query.'),
+                },
+                {
+                  code: 1001,
+                  message: t(
+                    'Issue 1001 - The database is under an unusual load.',
+                  ),
+                },
+              ],
+            },
+            level: 'error',
+            message: 'Request timed out',
+          },
+        ],
+      });
+      return;
+    }
+
+    const responseObject =
+      response instanceof Response ? response : response.response;
+    if (responseObject && !responseObject.bodyUsed) {
+      // attempt to read the body as json, and fallback to text. we must clone the
+      // response in order to fallback to .text() because Response is single-read
+      responseObject
+        .clone()
+        .json()
+        .then(errorJson => {
+          const error = { ...responseObject, ...errorJson };
+          resolve(parseErrorJson(error));
+        })
+        .catch(() => {
+          // fall back to reading as text
+          responseObject.text().then((errorText: any) => {
+            resolve({ ...responseObject, error: errorText });
+          });
+        });
+      return;
+    }
+
+    // fall back to Response.statusText or generic error of we cannot read the response
+    let error = (response as any).statusText || (response as any).message;
+    if (!error) {
+      // eslint-disable-next-line no-console
+      console.error('non-standard error:', response);
+      error = t('An error occurred');
+    }
+    resolve({
+      ...responseObject,
+      error,
+    });
   });
 }

superset/models/reports.py

@@ -77,7 +77,7 @@ class ReportDataFormat(str, enum.Enum):
     TEXT = "TEXT"
 
 
-class ReportCreationMethodType(str, enum.Enum):
+class ReportCreationMethod(str, enum.Enum):
     CHARTS = "charts"
     DASHBOARDS = "dashboards"
     ALERTS_REPORTS = "alerts_reports"
@@ -112,7 +112,7 @@ class ReportSchedule(Model, AuditMixinNullable):
     active = Column(Boolean, default=True, index=True)
     crontab = Column(String(1000), nullable=False)
     creation_method = Column(
-        String(255), server_default=ReportCreationMethodType.ALERTS_REPORTS
+        String(255), server_default=ReportCreationMethod.ALERTS_REPORTS
     )
     timezone = Column(String(100), default="UTC", nullable=False)
     report_format = Column(String(50), default=ReportDataFormat.VISUALIZATION)

superset/reports/commands/base.py

@@ -22,7 +22,7 @@ from marshmallow import ValidationError
 from superset.charts.dao import ChartDAO
 from superset.commands.base import BaseCommand
 from superset.dashboards.dao import DashboardDAO
-from superset.models.reports import ReportCreationMethodType
+from superset.models.reports import ReportCreationMethod
 from superset.reports.commands.exceptions import (
     ChartNotFoundValidationError,
     ChartNotSavedValidationError,
@@ -52,12 +52,12 @@ class BaseReportScheduleCommand(BaseCommand):
         dashboard_id = self._properties.get("dashboard")
         creation_method = self._properties.get("creation_method")
 
-        if creation_method == ReportCreationMethodType.CHARTS and not chart_id:
+        if creation_method == ReportCreationMethod.CHARTS and not chart_id:
             # User has not saved chart yet in Explore view
             exceptions.append(ChartNotSavedValidationError())
             return
 
-        if creation_method == ReportCreationMethodType.DASHBOARDS and not dashboard_id:
+        if creation_method == ReportCreationMethod.DASHBOARDS and not dashboard_id:
             exceptions.append(DashboardNotSavedValidationError())
             return
 

superset/reports/commands/create.py

@@ -25,7 +25,7 @@ from marshmallow import ValidationError
 from superset.commands.base import CreateMixin
 from superset.dao.exceptions import DAOCreateFailedError
 from superset.databases.dao import DatabaseDAO
-from superset.models.reports import ReportCreationMethodType, ReportScheduleType
+from superset.models.reports import ReportCreationMethod, ReportScheduleType
 from superset.reports.commands.base import BaseReportScheduleCommand
 from superset.reports.commands.exceptions import (
     DatabaseNotFoundValidationError,
@@ -73,7 +73,11 @@ class CreateReportScheduleCommand(CreateMixin, BaseReportScheduleCommand):
         if report_type and not ReportScheduleDAO.validate_update_uniqueness(
             name, report_type
         ):
-            exceptions.append(ReportScheduleNameUniquenessValidationError())
+            exceptions.append(
+                ReportScheduleNameUniquenessValidationError(
+                    report_type=report_type, name=name
+                )
+            )
 
         # validate relation by report type
         if report_type == ReportScheduleType.ALERT:
@@ -93,7 +97,7 @@ class CreateReportScheduleCommand(CreateMixin, BaseReportScheduleCommand):
         # Validate that each chart or dashboard only has one report with
         # the respective creation method.
         if (
-            creation_method != ReportCreationMethodType.ALERTS_REPORTS
+            creation_method != ReportCreationMethod.ALERTS_REPORTS
             and not ReportScheduleDAO.validate_unique_creation_method(
                 user_id, dashboard_id, chart_id
             )

superset/reports/commands/exceptions.py

@@ -24,6 +24,7 @@ from superset.commands.exceptions import (
     ForbiddenError,
     ValidationError,
 )
+from superset.models.reports import ReportScheduleType
 
 
 class DatabaseNotFoundValidationError(ValidationError):
@@ -163,13 +164,16 @@ class ReportScheduleNameUniquenessValidationError(ValidationError):
     Marshmallow validation error for Report Schedule name and type already exists
     """
 
-    def __init__(self) -> None:
+    def __init__(self, report_type: ReportScheduleType, name: str) -> None:
-        super().__init__([_("Name must be unique")], field_name="name")
+        message = _('A report named "%(name)s" already exists', name=name)
+        if report_type == ReportScheduleType.ALERT:
+            message = _('An alert named "%(name)s" already exists', name=name)
+        super().__init__([message], field_name="name")
 
 
 class ReportScheduleCreationMethodUniquenessValidationError(CommandException):
     status = 409
-    message = "Resource already has an attached report."
+    message = _("Resource already has an attached report.")
 
 
 class AlertQueryMultipleRowsError(CommandException):

superset/reports/commands/update.py

@@ -86,9 +86,13 @@ class UpdateReportScheduleCommand(UpdateMixin, BaseReportScheduleCommand):
 
         # Validate name type uniqueness
         if not ReportScheduleDAO.validate_update_uniqueness(
-            name, report_type, report_schedule_id=self._model_id
+            name, report_type, expect_id=self._model_id
         ):
-            exceptions.append(ReportScheduleNameUniquenessValidationError())
+            exceptions.append(
+                ReportScheduleNameUniquenessValidationError(
+                    report_type=report_type, name=name
+                )
+            )
 
         if report_type == ReportScheduleType.ALERT:
             database_id = self._properties.get("database")

superset/reports/dao.py

@@ -30,6 +30,7 @@ from superset.models.reports import (
     ReportExecutionLog,
     ReportRecipients,
     ReportSchedule,
+    ReportScheduleType,
     ReportState,
 )
 
@@ -133,23 +134,24 @@ class ReportScheduleDAO(BaseDAO):
 
     @staticmethod
     def validate_update_uniqueness(
-        name: str, report_type: str, report_schedule_id: Optional[int] = None
+        name: str, report_type: ReportScheduleType, expect_id: Optional[int] = None
     ) -> bool:
         """
         Validate if this name and type is unique.
 
         :param name: The report schedule name
         :param report_type: The report schedule type
-        :param report_schedule_id: The report schedule current id
+        :param expect_id: The id of the expected report schedule with the
-        (only for validating on updates)
+          name + type combination. Useful for validating existing report schedule.
         :return: bool
         """
-        query = db.session.query(ReportSchedule).filter(
+        found_id = (
-            ReportSchedule.name == name, ReportSchedule.type == report_type
+            db.session.query(ReportSchedule.id)
+            .filter(ReportSchedule.name == name, ReportSchedule.type == report_type)
+            .limit(1)
+            .scalar()
         )
-        if report_schedule_id:
+        return found_id is None or found_id == expect_id
-            query = query.filter(ReportSchedule.id != report_schedule_id)
-        return not db.session.query(query.exists()).scalar()
 
     @classmethod
     def create(cls, properties: Dict[str, Any], commit: bool = True) -> Model:

superset/reports/schemas.py

@@ -24,7 +24,7 @@ from marshmallow_enum import EnumField
 from pytz import all_timezones
 
 from superset.models.reports import (
-    ReportCreationMethodType,
+    ReportCreationMethod,
     ReportDataFormat,
     ReportRecipientType,
     ReportScheduleType,
@@ -164,7 +164,7 @@ class ReportSchedulePostSchema(Schema):
     )
     chart = fields.Integer(required=False, allow_none=True)
     creation_method = EnumField(
-        ReportCreationMethodType,
+        ReportCreationMethod,
         by_value=True,
         required=False,
         description=creation_method_description,
@@ -256,7 +256,7 @@ class ReportSchedulePutSchema(Schema):
     )
     chart = fields.Integer(required=False, allow_none=True)
     creation_method = EnumField(
-        ReportCreationMethodType,
+        ReportCreationMethod,
         by_value=True,
         allow_none=True,
         description=creation_method_description,

tests/integration_tests/reports/api_tests.py

@@ -31,7 +31,7 @@ from superset.models.slice import Slice
 from superset.models.dashboard import Dashboard
 from superset.models.reports import (
     ReportSchedule,
-    ReportCreationMethodType,
+    ReportCreationMethod,
     ReportRecipients,
     ReportExecutionLog,
     ReportScheduleType,
@@ -452,7 +452,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "new3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "recipients": [
                 {
                     "type": ReportRecipientType.EMAIL,
@@ -499,7 +499,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "name3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "chart": chart.id,
             "database": example_db.id,
@@ -508,7 +508,7 @@ class TestReportSchedulesApi(SupersetTestCase):
         rv = self.client.post(uri, json=report_schedule_data)
         assert rv.status_code == 422
         data = json.loads(rv.data.decode("utf-8"))
-        assert data == {"message": {"name": ["Name must be unique"]}}
+        assert data == {"message": {"name": ['An alert named "name3" already exists']}}
 
         # Check that uniqueness is composed by name and type
         report_schedule_data = {
@@ -516,7 +516,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "name3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "chart": chart.id,
         }
         uri = "api/v1/report/"
@@ -546,7 +546,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "chart": chart.id,
             "database": example_db.id,
@@ -560,7 +560,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -585,7 +585,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -609,7 +609,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -635,7 +635,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new4",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -661,7 +661,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new5",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -687,7 +687,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new5",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -714,7 +714,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new5",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -745,7 +745,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new6",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "recipients": [
                 {
@@ -784,7 +784,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.CHARTS,
+            "creation_method": ReportCreationMethod.CHARTS,
             "crontab": "0 9 * * *",
             "chart": 0,
         }
@@ -812,7 +812,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.DASHBOARDS,
+            "creation_method": ReportCreationMethod.DASHBOARDS,
             "crontab": "0 9 * * *",
         }
         uri = "api/v1/report/"
@@ -839,7 +839,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name4",
             "description": "description",
-            "creation_method": ReportCreationMethodType.CHARTS,
+            "creation_method": ReportCreationMethod.CHARTS,
             "crontab": "0 9 * * *",
             "working_timeout": 3600,
             "chart": chart.id,
@@ -855,7 +855,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name5",
             "description": "description",
-            "creation_method": ReportCreationMethodType.CHARTS,
+            "creation_method": ReportCreationMethod.CHARTS,
             "crontab": "0 9 * * *",
             "working_timeout": 3600,
             "chart": chart.id,
@@ -897,7 +897,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name4",
             "description": "description",
-            "creation_method": ReportCreationMethodType.DASHBOARDS,
+            "creation_method": ReportCreationMethod.DASHBOARDS,
             "crontab": "0 9 * * *",
             "working_timeout": 3600,
             "dashboard": dashboard.id,
@@ -913,7 +913,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.REPORT,
             "name": "name5",
             "description": "description",
-            "creation_method": ReportCreationMethodType.DASHBOARDS,
+            "creation_method": ReportCreationMethod.DASHBOARDS,
             "crontab": "0 9 * * *",
             "working_timeout": 3600,
             "dashboard": dashboard.id,
@@ -956,7 +956,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "new3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "chart": chart.id,
             "dashboard": dashboard.id,
             "database": example_db.id,
@@ -980,7 +980,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "chart": chart.id,
         }
@@ -1006,7 +1006,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "type": ReportScheduleType.ALERT,
             "name": "new3",
             "description": "description",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "crontab": "0 9 * * *",
             "chart": chart_max_id + 1,
             "database": database_max_id + 1,
@@ -1029,7 +1029,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "new3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "dashboard": dashboard_max_id + 1,
             "database": examples_db.id,
         }
@@ -1197,7 +1197,7 @@ class TestReportSchedulesApi(SupersetTestCase):
         rv = self.client.put(uri, json=report_schedule_data)
         data = json.loads(rv.data.decode("utf-8"))
         assert rv.status_code == 422
-        assert data == {"message": {"name": ["Name must be unique"]}}
+        assert data == {"message": {"name": ['An alert named "name3" already exists']}}
 
     @pytest.mark.usefixtures("create_report_schedules")
     def test_update_report_schedule_not_found(self):
@@ -1546,7 +1546,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "new3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "recipients": [
                 {
                     "type": ReportRecipientType.EMAIL,
@@ -1581,7 +1581,7 @@ class TestReportSchedulesApi(SupersetTestCase):
             "name": "new3",
             "description": "description",
             "crontab": "0 9 * * *",
-            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "creation_method": ReportCreationMethod.ALERTS_REPORTS,
             "recipients": [
                 {
                     "type": ReportRecipientType.EMAIL,