Skip to content

Commit

Permalink
refactor(ReportModal): simplify state reducer
Browse files Browse the repository at this point in the history
  • Loading branch information
ktmud committed May 5, 2022
1 parent 902ac05 commit d88aa85
Show file tree
Hide file tree
Showing 13 changed files with 283 additions and 278 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
29 changes: 29 additions & 0 deletions superset-frontend/src/reports/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* 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 ReportCreationMethodType =
| '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 (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,
});
});
}
6 changes: 5 additions & 1 deletion superset/reports/commands/create.py
Original file line number Diff line number Diff line change
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 Down
10 changes: 7 additions & 3 deletions superset/reports/commands/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
ForbiddenError,
ValidationError,
)
from superset.models.reports import ReportScheduleType


class DatabaseNotFoundValidationError(ValidationError):
Expand Down Expand Up @@ -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):
Expand Down
8 changes: 6 additions & 2 deletions superset/reports/commands/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Loading

0 comments on commit d88aa85

Please sign in to comment.