diff --git a/superset-frontend/src/components/ReportModal/index.test.tsx b/superset-frontend/src/components/ReportModal/ReportModal.test.tsx similarity index 100% rename from superset-frontend/src/components/ReportModal/index.test.tsx rename to superset-frontend/src/components/ReportModal/ReportModal.test.tsx diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index c45d8fad4762..0601c4b86773 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/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; - 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; - creationMethod: string; + dashboardName?: 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; - } - | { - 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( - 'We were unable to create your report. Please try again.', -); - -const reportReducer = ( - state: Partial | null, - action: ReportActionType, -): Partial | null => { - const initialState = { - name: 'Weekly Report', - crontab: '0 12 * * 1', - }; +const INITIAL_STATE = { + 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; - } +type ReportObjectState = Partial & { + error?: string; + /** + * Is submitting changes to the backend. + */ + isSubmitting?: boolean; }; const ReportModal: FunctionComponent = ({ @@ -190,45 +130,64 @@ const ReportModal: FunctionComponent = ({ }) => { const vizType = props.chart?.sliceFormData?.viz_type; const isChart = !!props.chart; - const defaultNotificationFormat = - isChart && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) - ? NOTIFICATION_FORMATS.TEXT - : NOTIFICATION_FORMATS.PNG; - const [currentReport, setCurrentReport] = useReducer< - Reducer | null, ReportActionType> - >(reportReducer, null); - const onReducerChange = useCallback((type: any, payload: any) => { - setCurrentReport({ type, payload }); - }, []); + const isTextBasedChart = + isChart && vizType && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType); + const defaultNotificationFormat = isTextBasedChart + ? NOTIFICATION_FORMATS.TEXT + : NOTIFICATION_FORMATS.PNG; + const entityName = props.dashboardName || props.chartName; + const initialState: ReportObjectState = useMemo( + () => ({ + ...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(); + const dispatch = useDispatch(); - // Report fetch logic - const reports = useSelector(state => state.reports); + const reports = useSelector(state => state.reports); const isEditMode = reports && Object.keys(reports).length; useEffect(() => { if (isEditMode) { const reportsIds = Object.keys(reports); const report = reports[reportsIds[0]]; - setCurrentReport({ - type: ActionType.fetched, - payload: report, - }); + setCurrentReport(report); } else { - setCurrentReport({ - type: ActionType.reset, - }); + setCurrentReport('reset'); } - }, [reports]); + }, [isEditMode, reports]); const onSave = async () => { // Create new Report const newReportValues: Partial = { - 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 = ({ type: 'Email', }, ], - type: 'Report', - creation_method: props.creationMethod, - active: true, - report_format: currentReport?.report_format || defaultNotificationFormat, - timezone: currentReport?.timezone, - force_screenshot: false, + name: currentReport.name, + description: currentReport.description, + crontab: currentReport.crontab, + report_format: currentReport.report_format || defaultNotificationFormat, + timezone: currentReport.timezone, }; - if (isEditMode) { - await dispatch( - editReport(currentReport?.id, newReportValues as ReportObject), - ); - onHide(); - } else { - try { + setCurrentReport({ isSubmitting: true, error: undefined }); + try { + if (isEditMode) { + await dispatch( + editReport(currentReport.id, newReportValues as ReportObject), + ); + } 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 = ({ - {isEditMode ? t('Edit Email Report') : t('New Email Report')} + {isEditMode ? t('Edit email report') : t('Schedule a new email report')} ); @@ -280,7 +239,8 @@ const ReportModal: FunctionComponent = ({ key="submit" buttonStyle="primary" onClick={onSave} - disabled={!currentReport?.name} + disabled={!currentReport.name} + loading={currentReport.isSubmitting} > {isEditMode ? t('Save') : t('Add')} @@ -290,19 +250,16 @@ const ReportModal: FunctionComponent = ({ const renderMessageContentSection = ( <> -

{t('Message Content')}

+

{t('Message content')}
{ - onReducerChange(ActionType.inputChange, { - name: 'report_format', - value: event.target.value, - }); + setCurrentReport({ report_format: event.target.value }); }} - value={currentReport?.report_format || defaultNotificationFormat} + value={currentReport.report_format || defaultNotificationFormat} > - {TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) && ( + {isTextBasedChart && ( {t('Text embedded in email')} @@ -318,15 +275,6 @@ const ReportModal: FunctionComponent = ({ ); - const errorAlert = () => ( - antDErrorAlertStyles(theme)} - message={t('Report Creation Error')} - description={currentReport?.error} - /> - ); - return ( = ({ - onReducerChange(ActionType.inputChange, { - name: target.name, - value: target.value, - }), + setCurrentReport({ name: target.value }), }} label="Report Name" data-test="report-name-test" @@ -358,11 +303,9 @@ const ReportModal: FunctionComponent = ({ name="description" value={currentReport?.description || ''} validationMethods={{ - onChange: ({ target }: { target: HTMLInputElement }) => - onReducerChange(ActionType.inputChange, { - name: target.name, - value: target.value, - }), + onChange: ({ target }: { target: HTMLInputElement }) => { + setCurrentReport({ description: target.value }); + }, }} label={t('Description')} placeholder={t( @@ -378,17 +321,16 @@ const ReportModal: FunctionComponent = ({

SectionHeaderStyle(theme)}> {t('Schedule')}

-

{t('Scheduled reports will be sent to your email as a PNG')}

+

+ {t('A screenshot of the dashboard will be sent to your email at')} +

{ - onReducerChange(ActionType.inputChange, { - name: 'crontab', - value: newValue, - }); + setCurrentReport({ crontab: newValue }); }} onError={setCronError} /> @@ -400,17 +342,25 @@ const ReportModal: FunctionComponent = ({ {t('Timezone')}
{ - setCurrentReport({ - type: ActionType.inputChange, - payload: { name: 'timezone', value }, - }); + setCurrentReport({ timezone: value }); }} - timezone={currentReport?.timezone} /> {isChart && renderMessageContentSection} - {currentReport?.error && errorAlert()} + {currentReport.error && ( + antDErrorAlertStyles(theme)} + message={ + isEditMode + ? t('Failed to update report') + : t('Failed to create report') + } + description={currentReport.error} + /> + )} ); }; diff --git a/superset-frontend/src/components/ReportModal/styles.tsx b/superset-frontend/src/components/ReportModal/styles.tsx index 752dbbdbddb2..960da9b10e47 100644 --- a/superset-frontend/src/components/ReportModal/styles.tsx +++ b/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; diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index af68742b4ab1..b1ae9cb4b148 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/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" /> )} diff --git a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx index 4ec5ceb0ad08..4a4c00248648 100644 --- a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx +++ b/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; @@ -41,16 +40,11 @@ export const ExploreReport = ({ setIsDeleting, }: ReportMenuItemsProps) => { const dispatch = useDispatch(); - const chart = useSelector(state => { - if (!state.charts) { - return undefined; - } - const charts = Object.values(state.charts); - if (charts.length > 0) { - return charts[0]; - } - return undefined; - }); + const { chart, chartName } = useSelector((state: ExplorePageState) => ({ + chart: Object.values(state.charts || {})[0], + chartName: state.explore.sliceName, + })); + 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 && ( diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 8f23e283522d..f0ae4012b12f 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/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) { - return function (dispatch) { - SupersetClient.put({ - endpoint: `/api/v1/report/${id}`, - jsonPayload: report, - }) - .then(({ json }) => { - dispatch({ type: EDIT_REPORT, json }); - }) - .catch(() => - dispatch( - addDangerToast(t('An error occurred while editing this report.')), - ), - ); - }; -} +export const editReport = (id, report) => dispatch => + SupersetClient.put({ + endpoint: `/api/v1/report/${id}`, + jsonPayload: report, + }).then(({ json }) => { + dispatch({ type: EDIT_REPORT, json }); + dispatch(addSuccessToast(t('Report updated'))); + }); export function toggleActive(report, isActive) { return function toggleActiveThunk(dispatch) { diff --git a/superset-frontend/src/reports/types.ts b/superset-frontend/src/reports/types.ts new file mode 100644 index 000000000000..cde02544c00c --- /dev/null +++ b/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'; diff --git a/superset-frontend/src/utils/getClientErrorObject.ts b/superset-frontend/src/utils/getClientErrorObject.ts index 8ab6e5b435ff..3451528f025c 100644 --- a/superset-frontend/src/utils/getClientErrorObject.ts +++ b/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; -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)[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 { // 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 { - 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', + return; + } + + 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.', + ), + }, + ], }, - ], - }); - } 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, + 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, + }); }); } diff --git a/superset/models/reports.py b/superset/models/reports.py index d2d5db513dad..b0aa94d9a77c 100644 --- a/superset/models/reports.py +++ b/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) diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index b17975a5a63e..ee2eba7139ed 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -22,7 +22,7 @@ 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 @@ def validate_chart_dashboard( 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 diff --git a/superset/reports/commands/create.py b/superset/reports/commands/create.py index 9e6a892c3cc0..6d9161445a26 100644 --- a/superset/reports/commands/create.py +++ b/superset/reports/commands/create.py @@ -25,7 +25,7 @@ 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 @@ def validate(self) -> None: 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 @@ def validate(self) -> None: # 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 ) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index 27fcd1eef0ea..b78f99afe99d 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -24,6 +24,7 @@ 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: - super().__init__([_("Name must be unique")], field_name="name") + def __init__(self, report_type: ReportScheduleType, name: str) -> None: + 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): diff --git a/superset/reports/commands/update.py b/superset/reports/commands/update.py index a38ca983e85e..201d961863d4 100644 --- a/superset/reports/commands/update.py +++ b/superset/reports/commands/update.py @@ -86,9 +86,13 @@ def validate(self) -> None: # 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") diff --git a/superset/reports/dao.py b/superset/reports/dao.py index 079788fbd0e5..312710fe037c 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -30,6 +30,7 @@ ReportExecutionLog, ReportRecipients, ReportSchedule, + ReportScheduleType, ReportState, ) @@ -133,23 +134,24 @@ def validate_unique_creation_method( @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 - (only for validating on updates) + :param expect_id: The id of the expected report schedule with the + name + type combination. Useful for validating existing report schedule. :return: bool """ - query = db.session.query(ReportSchedule).filter( - ReportSchedule.name == name, ReportSchedule.type == report_type + found_id = ( + db.session.query(ReportSchedule.id) + .filter(ReportSchedule.name == name, ReportSchedule.type == report_type) + .limit(1) + .scalar() ) - if report_schedule_id: - query = query.filter(ReportSchedule.id != report_schedule_id) - return not db.session.query(query.exists()).scalar() + return found_id is None or found_id == expect_id @classmethod def create(cls, properties: Dict[str, Any], commit: bool = True) -> Model: diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 4076a2041cd9..a0ce67d80dbb 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -24,7 +24,7 @@ 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, diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 9440855a62a5..948fe519e1be 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -31,7 +31,7 @@ from superset.models.dashboard import Dashboard from superset.models.reports import ( ReportSchedule, - ReportCreationMethodType, + ReportCreationMethod, ReportRecipients, ReportExecutionLog, ReportScheduleType, @@ -452,7 +452,7 @@ def test_create_report_schedule(self): "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 @@ def test_create_report_schedule_uniqueness(self): "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 @@ def test_create_report_schedule_uniqueness(self): 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 @@ def test_create_report_schedule_uniqueness(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_create_report_schedule_schema(self): "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 @@ def test_unsaved_report_schedule_schema(self): "type": ReportScheduleType.REPORT, "name": "name3", "description": "description", - "creation_method": ReportCreationMethodType.CHARTS, + "creation_method": ReportCreationMethod.CHARTS, "crontab": "0 9 * * *", "chart": 0, } @@ -812,7 +812,7 @@ def test_no_dashboard_report_schedule_schema(self): "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 @@ def test_create_multiple_creation_method_report_schedule_charts(self): "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 @@ def test_create_multiple_creation_method_report_schedule_charts(self): "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 @@ def test_create_multiple_creation_method_report_schedule_dashboards(self): "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 @@ def test_create_multiple_creation_method_report_schedule_dashboards(self): "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 @@ def test_create_report_schedule_chart_dash_validation(self): "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 @@ def test_create_report_schedule_chart_db_validation(self): "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 @@ def test_create_report_schedule_relations_exist(self): "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 @@ def test_create_report_schedule_relations_exist(self): "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 @@ def test_update_report_schedule_uniqueness(self): 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 @@ def test_when_invalid_tab_ids_are_given_it_raises_bad_request(self): "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 @@ def test_when_tab_ids_are_given_it_gets_added_to_extra(self): "name": "new3", "description": "description", "crontab": "0 9 * * *", - "creation_method": ReportCreationMethodType.ALERTS_REPORTS, + "creation_method": ReportCreationMethod.ALERTS_REPORTS, "recipients": [ { "type": ReportRecipientType.EMAIL,