Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Keep Report modal open when there's an error #17988

Merged
merged 1 commit into from
Jan 10, 2022
+47 −49
Merged

fix: Keep Report modal open when there's an error #17988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 43 additions & 32 deletions superset-frontend/src/components/ReportModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import React, {
FunctionComponent,
} from 'react';
import { t, SupersetTheme } from '@superset-ui/core';
import { getClientErrorObject } from 'src/utils/getClientErrorObject';
import { bindActionCreators } from 'redux';
import { connect, useDispatch, useSelector } from 'react-redux';
import { addReport, editReport } from 'src/reports/actions/reports';
Expand Down Expand Up @@ -72,6 +73,7 @@ export interface ReportObject {
working_timeout: number;
creation_method: string;
force_screenshot: boolean;
error?: string;
}

interface ChartObject {
Expand All @@ -88,8 +90,6 @@ interface ChartObject {
}

interface ReportProps {
addDangerToast: (msg: string) => void;
addSuccessToast: (msg: string) => void;
addReport: (report?: ReportObject) => {};
onHide: () => {};
onReportAdd: (report?: ReportObject) => {};
Expand All @@ -111,6 +111,7 @@ enum ActionType {
inputChange,
fetched,
reset,
error,
}

type ReportActionType =
Expand All @@ -124,6 +125,12 @@ type ReportActionType =
}
| {
type: ActionType.reset;
}
| {
type: ActionType.error;
payload: {
name: string[];
};
};

const TEXT_BASED_VISUALIZATION_TYPES = [
Expand Down Expand Up @@ -161,6 +168,11 @@ const reportReducer = (
};
case ActionType.reset:
return { ...initialState };
case ActionType.error:
return {
...state,
error: action.payload.name[0],
};
default:
return state;
}
Expand All @@ -181,11 +193,10 @@ const ReportModal: FunctionComponent<ReportProps> = ({
const [currentReport, setCurrentReport] = useReducer<
Reducer<Partial<ReportObject> | null, ReportActionType>
>(reportReducer, null);
const onChange = useCallback((type: any, payload: any) => {
const onReducerChange = useCallback((type: any, payload: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why the switch from onChange to onReducerChange. If I'm reading this right, when the reducer changes, you update the reducer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases like this one, there was originally an onChange nested into another onChange. I thought that since there were two onChange functions in the component it could get confusing, so I renamed the reducer's onChange to onReducerChange to clarify that onReducerChange is specifically the function that the reducer uses to change state. Is that an unnecessary clarification, or is there maybe a better name I can use here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I gotcha. Normally the naming convention will be the element that is changing, rather than the function that is used to do the change, thus the confusion. Maybe something like onInputChange would make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, onInputChange sounds a lot better! I'll change the naming once I reopen this to fix it.

setCurrentReport({ type, payload });
}, []);
const [error, setError] = useState<CronError>();
// const [isLoading, setLoading] = useState<boolean>(false);
const [cronError, setCronError] = useState<CronError>();
const dispatch = useDispatch();
// Report fetch logic
const reports = useSelector<any, AlertObject>(state => state.reports);
Expand All @@ -205,9 +216,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
});
}
}, [reports]);
const onClose = () => {
onHide();
};

const onSave = async () => {
// Create new Report
const newReportValues: Partial<ReportObject> = {
Expand Down Expand Up @@ -235,15 +244,19 @@ const ReportModal: FunctionComponent<ReportProps> = ({
await dispatch(
editReport(currentReport?.id, newReportValues as ReportObject),
);
onHide();
} else {
await dispatch(addReport(newReportValues as ReportObject));
try {
await dispatch(addReport(newReportValues as ReportObject));
onHide();
} catch (e) {
const parsedError = await getClientErrorObject(e);
const errorMessage = parsedError.message;
onReducerChange(ActionType.error, errorMessage);
}
}

if (onReportAdd) {
onReportAdd();
}

onClose();
if (onReportAdd) onReportAdd();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the format preferred by prettier these days? I learned that braceless if clauses should be avoided because they can lead to subtle errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not preferred by prettier, I thought it was just a prettier way to do it, haha. I didn't realize there were subtle errors possible with this though, thanks for this catch! I'll change it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a single line is OK, since it's simple enough. The use that is discouraged is:

if (onReportAdd)
  onReportAdd();

Which I think prettier would remove.

I think it's fine to leave the way you did, I was just curious. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay awesome, I'll keep an eye out for that behavior in the future just in case! Thank you! 😁

};

const wrappedTitle = (
Expand All @@ -257,7 +270,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({

const renderModalFooter = (
<>
<StyledFooterButton key="back" onClick={onClose}>
<StyledFooterButton key="back" onClick={onHide}>
{t('Cancel')}
</StyledFooterButton>
<StyledFooterButton
Expand All @@ -279,7 +292,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
<div className="inline-container">
<StyledRadioGroup
onChange={(event: RadioChangeEvent) => {
onChange(ActionType.inputChange, {
onReducerChange(ActionType.inputChange, {
name: 'report_format',
value: event.target.value,
});
Expand All @@ -305,7 +318,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
return (
<StyledModal
show={show}
onHide={onClose}
onHide={onHide}
title={wrappedTitle}
footer={renderModalFooter}
width="432"
Expand All @@ -316,18 +329,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({
id="name"
name="name"
value={currentReport?.name || ''}
placeholder="Weekly Report"
placeholder={t('Weekly Report')}
required
validationMethods={{
onChange: ({ target }: { target: HTMLInputElement }) =>
onChange(ActionType.inputChange, {
onReducerChange(ActionType.inputChange, {
name: target.name,
value: target.value,
}),
}}
errorMessage={
currentReport?.name === 'error' ? t('REPORT NAME ERROR') : ''
}
errorMessage={currentReport?.error || ''}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how/when does this error get cleared out? Does it stay visible until they hit submit again? I'm not sure if we discussed how this should look @yousoph?

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the error stays until the user submits the form again. Once they hit submit it will rerun, if there's still an error it will stay open with that error. If the error is corrected it will close. I think it would be nice to have the error cleared as soon as the user fixes it, but I think that would require front end validation vs. just back end.

label="Report Name"
data-test="report-name-test"
/>
Expand All @@ -338,16 +349,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({
value={currentReport?.description || ''}
validationMethods={{
onChange: ({ target }: { target: HTMLInputElement }) =>
onChange(ActionType.inputChange, {
onReducerChange(ActionType.inputChange, {
name: target.name,
value: target.value,
}),
}}
errorMessage={
currentReport?.description === 'error' ? t('DESCRIPTION ERROR') : ''
}
label="Description"
placeholder="Include a description that will be sent with your report"
errorMessage=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do you display the error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with Sophie separately and she said that this field doesn't currently have any restrictions so it won't have any errors applicable to its field. I left this blank since errorMessage is a required field, and figured the error handling for this could be set up in the future if the description field ever ends up getting restrictions.

label={t('Description')}
placeholder={t(
'Include a description that will be sent with your report',
)}
css={noBottomMargin}
data-test="report-description-test"
/>
Expand All @@ -363,16 +374,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({

<StyledCronPicker
clearButton={false}
value={currentReport?.crontab || '0 12 * * 1'}
value={t(currentReport?.crontab || '0 12 * * 1')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a translation is necessary here.

setValue={(newValue: string) => {
onChange(ActionType.inputChange, {
onReducerChange(ActionType.inputChange, {
name: 'crontab',
value: newValue,
});
}}
onError={setError}
onError={setCronError}
/>
<StyledCronError>{error}</StyledCronError>
<StyledCronError>{cronError}</StyledCronError>
<div
className="control-label"
css={(theme: SupersetTheme) => TimezoneHeaderStyle(theme)}
Expand Down
21 changes: 4 additions & 17 deletions superset-frontend/src/reports/actions/reports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
/* eslint camelcase: 0 */
import { t, SupersetClient } from '@superset-ui/core';
import rison from 'rison';
import { getClientErrorObject } from 'src/utils/getClientErrorObject';
import {
addDangerToast,
addSuccessToast,
Expand Down Expand Up @@ -105,22 +104,10 @@ export const addReport = report => dispatch =>
SupersetClient.post({
endpoint: `/api/v1/report/`,
jsonPayload: report,
})
.then(({ json }) => {
dispatch({ type: ADD_REPORT, json });
dispatch(addSuccessToast(t('The report has been created')));
})
.catch(async e => {
const parsedError = await getClientErrorObject(e);
const errorMessage = parsedError.message;
const errorArr = Object.keys(errorMessage);
const error = errorMessage[errorArr[0]];
dispatch(
addDangerToast(
t('An error occurred while editing this report: %s', error),
),
);
});
}).then(({ json }) => {
dispatch({ type: ADD_REPORT, json });
dispatch(addSuccessToast(t('The report has been created')));
});
Comment on lines +107 to +110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the endpoint returns an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the catch functionality into the report modal here, this should handle any errors for this endpoint. Should I also put a catch in the action?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense, because we're no longer showing the error in the toaster, but in the modal. Thanks for clarifying! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! 😁


export const EDIT_REPORT = 'EDIT_REPORT';

Expand Down