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

lyndsiWilliams · 2022-01-10T20:53:07Z

SUMMARY

The report modal was only using toasts for responses - error or success - so the user would have to reopen the modal to fix an errored report. The error is now brought into the modal directly and displayed underneath the name field, and will not close until the error is corrected.

Note: The name field is currently the only field that takes this type of error. The cron scheduler is the only other thing that has an error in this modal, and it has its own separate error handling already set up.

Note II: I also did some code cleanup in the report modal.

ANIMATED MOV

This demonstrates the modal correctly staying open and closing and also keeping correct values in the following events:

Create new report
- With error occurring first
Edit existing report
Delete existing report
Create a new report after deleting
- A clean un-errored creation

successfulError.mov

TESTING INSTRUCTIONS

Create new report
- With error occurring first
Edit existing report
Delete existing report
Create a new report after deleting
- A clean un-errored creation
Observe that the modal stays open when there's an error and closes when there is not
- Also observe that the modal keeps all the correct values throughout these events.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-01-10T21:32:52Z

Codecov Report

Merging #17988 (5812a79) into master (9e69940) will increase coverage by 0.00%.
The diff coverage is 71.42%.

@@           Coverage Diff           @@
##           master   #17988   +/-   ##
=======================================
  Coverage   67.07%   67.08%           
=======================================
  Files        1609     1611    +2     
  Lines       64905    64929   +24     
  Branches     6868     6872    +4     
=======================================
+ Hits        43537    43556   +19     
- Misses      19502    19506    +4     
- Partials     1866     1867    +1

Flag	Coverage Δ
javascript	`53.79% <71.42%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rset-frontend/src/components/ReportModal/index.tsx	`82.22% <68.42%> (-0.34%)`	⬇️
superset-frontend/src/reports/actions/reports.js	`39.39% <100.00%> (+5.18%)`	⬆️
...et-frontend/src/dashboard/actions/nativeFilters.ts	`39.79% <0.00%> (-6.80%)`	⬇️
superset-frontend/src/common/components/index.tsx	`100.00% <0.00%> (ø)`
.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js	`66.66% <0.00%> (ø)`
...plugins/legacy-preset-chart-nvd3/src/ReactNVD3.jsx	`0.00% <0.00%> (ø)`
...gins/legacy-preset-chart-nvd3/src/DistBar/index.js	`66.66% <0.00%> (ø)`
...mponents/nativeFilters/FiltersConfigModal/state.ts	`73.33% <0.00%> (ø)`
...et-frontend/src/components/Menu/LanguagePicker.tsx
...frontend/src/common/hooks/useChangeEffect/index.ts
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e69940...5812a79. Read the comment docs.

betodealmeida

Looks good, I just have a couple questions.

betodealmeida · 2022-01-10T22:46:14Z

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

-    }
-
-    onClose();
+    if (onReportAdd) onReportAdd();


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.

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.

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. :)

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

betodealmeida · 2022-01-10T22:47:16Z

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

+  }).then(({ json }) => {
+    dispatch({ type: ADD_REPORT, json });
+    dispatch(addSuccessToast(t('The report has been created')));
+  });


What happens when the endpoint returns an error?

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?

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

No problem! 😁

eschutho · 2022-01-11T00:52:38Z

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

-          }
-          label="Description"
-          placeholder="Include a description that will be sent with your report"
+          errorMessage=""


where do you display the error message?

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.

eschutho · 2022-01-11T00:53:54Z

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

@@ -363,16 +374,16 @@ const ReportModal: FunctionComponent<ReportProps> = ({

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


I don't think a translation is necessary here.

eschutho · 2022-01-11T00:56:20Z

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

@@ -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) => {


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

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?

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.

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

eschutho · 2022-01-12T01:50:33Z

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

-          errorMessage={
-            currentReport?.name === 'error' ? t('REPORT NAME ERROR') : ''
-          }
+          errorMessage={currentReport?.error || ''}


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?

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.

Fixed error handling in Report modal

5812a79

superset-github-bot bot added the preset-io label Jan 10, 2022

pull-request-size bot added the size/M label Jan 10, 2022

AAfghahi approved these changes Jan 10, 2022

View reviewed changes

betodealmeida reviewed Jan 10, 2022

View reviewed changes

betodealmeida approved these changes Jan 10, 2022

View reviewed changes

lyndsiWilliams merged commit f7add54 into apache:master Jan 10, 2022

lyndsiWilliams deleted the lyndsi/keep-report-modal-open-when-errored branch January 10, 2022 23:01

eschutho reviewed Jan 11, 2022

View reviewed changes

eschutho reviewed Jan 12, 2022

View reviewed changes

shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022

Fixed error handling in Report modal (apache#17988)

1cb6ddf

bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022

Fixed error handling in Report modal (apache#17988)

21c2ebd

diegomedina248 mentioned this pull request Mar 11, 2022

fix(report-add): make to add error toast message when already attached report is added again into dashboard or chart #19122

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lyndsiWilliams commented Jan 10, 2022

codecov bot commented Jan 10, 2022

betodealmeida left a comment

betodealmeida Jan 10, 2022

lyndsiWilliams Jan 10, 2022

betodealmeida Jan 10, 2022

lyndsiWilliams Jan 10, 2022

betodealmeida Jan 10, 2022

lyndsiWilliams Jan 10, 2022

betodealmeida Jan 10, 2022

lyndsiWilliams Jan 10, 2022

eschutho Jan 11, 2022

lyndsiWilliams Jan 11, 2022

eschutho Jan 11, 2022

eschutho Jan 11, 2022

lyndsiWilliams Jan 11, 2022

eschutho Jan 12, 2022

lyndsiWilliams Jan 12, 2022

eschutho Jan 12, 2022

lyndsiWilliams Jan 12, 2022 •

edited

Loading

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

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

Conversation

lyndsiWilliams commented Jan 10, 2022

SUMMARY

ANIMATED MOV

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 10, 2022

Codecov Report

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndsiWilliams Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

lyndsiWilliams Jan 12, 2022 •

edited

Loading