Skip to content

Commit

Permalink
refactor(ReportModal): simplify state reducer and improve error handl…
Browse files Browse the repository at this point in the history
…ing (apache#19942)
  • Loading branch information
ktmud authored May 11, 2022
1 parent 9c7bf83 commit c241759
Show file tree
Hide file tree
Showing 16 changed files with 315 additions and 313 deletions.
282 changes: 116 additions & 166 deletions superset-frontend/src/components/ReportModal/index.tsx

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion superset-frontend/src/components/ReportModal/styles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ class Header extends React.PureComponent {
userId={user.userId}
userEmail={user.email}
dashboardId={dashboardInfo.id}
dashboardName={dashboardInfo.name}
creationMethod="dashboards"
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand All @@ -41,16 +40,11 @@ export const ExploreReport = ({
setIsDeleting,
}: ReportMenuItemsProps) => {
const dispatch = useDispatch();
const chart = useSelector<ExplorePageState, ChartState | undefined>(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 }
Expand All @@ -69,6 +63,7 @@ export const ExploreReport = ({
userId={userId}
userEmail={email}
chart={chart}
chartName={chartName}
creationMethod="charts"
/>
{isDeleting && (
Expand Down
24 changes: 8 additions & 16 deletions superset-frontend/src/reports/actions/reports.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
26 changes: 26 additions & 0 deletions superset-frontend/src/reports/types.ts
Original file line number Diff line number Diff line change
@@ -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';
159 changes: 91 additions & 68 deletions superset-frontend/src/utils/getClientErrorObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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,
Expand All @@ -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 {
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,
});
});
}
4 changes: 2 additions & 2 deletions superset/models/reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions superset/reports/commands/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand Down
10 changes: 7 additions & 3 deletions superset/reports/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand All @@ -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
)
Expand Down
Loading

0 comments on commit c241759

Please sign in to comment.