You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/01/10 20:53:24 UTC
[GitHub] [superset] lyndsiWilliams opened a new pull request #17988: fix: Keep Report modal open when there's an error
lyndsiWilliams opened a new pull request #17988:
URL: https://github.com/apache/superset/pull/17988
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
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
<!--- Skip this if not applicable -->
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
https://user-images.githubusercontent.com/55605634/148836701-411716b2-064d-4b07-b4e9-c805a4f658b0.mov
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- 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
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781638769
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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();
Review comment:
Oh okay awesome, I'll keep an eye out for that behavior in the future just in case! Thank you! 😁
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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')));
+ });
Review comment:
No problem! 😁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r782651600
##########
File path: 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) => {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r782455061
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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=""
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781634107
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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();
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams merged pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams merged pull request #17988:
URL: https://github.com/apache/superset/pull/17988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r782458242
##########
File path: 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) => {
Review comment:
In cases like [this one](https://github.com/preset-io/superset/blob/5812a79c0e8eb6cf70297de197534ea9e62a4b52/superset-frontend/src/components/ReportModal/index.tsx#L294-L299), 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r783369458
##########
File path: 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) => {
Review comment:
Good call, `onInputChange` sounds a lot better! I'll change the naming once I reopen this to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781682820
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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=""
Review comment:
where do you display the error message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781637083
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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')));
+ });
Review comment:
Ah, makes sense, because we're no longer showing the error in the toaster, but in the modal. Thanks for clarifying! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r783371366
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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 || ''}
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781684120
##########
File path: 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) => {
Review comment:
curious why the switch from onChange to onReducerChange. If I'm reading this right, when the reducer changes, you update the reducer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781636711
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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();
Review comment:
I think a single line is OK, since it's simple enough. The use that is discouraged is:
```js
if (onReportAdd)
onReportAdd();
```
Which I think `prettier` would remove.
I think it's fine to leave the way you did, I was just curious. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #17988:
URL: https://github.com/apache/superset/pull/17988#issuecomment-1009364456
# [Codecov](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17988](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5812a79) into [master](https://codecov.io/gh/apache/superset/commit/9e699401713460a46b28e6fed19f6ebd6cd228ca?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9e69940) will **increase** coverage by `0.00%`.
> The diff coverage is `71.42%`.
[](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rset-frontend/src/components/ReportModal/index.tsx](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUmVwb3J0TW9kYWwvaW5kZXgudHN4) | `82.22% <68.42%> (-0.34%)` | :arrow_down: |
| [superset-frontend/src/reports/actions/reports.js](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlcG9ydHMvYWN0aW9ucy9yZXBvcnRzLmpz) | `39.39% <100.00%> (+5.18%)` | :arrow_up: |
| [...et-frontend/src/dashboard/actions/nativeFilters.ts](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL25hdGl2ZUZpbHRlcnMudHM=) | `39.79% <0.00%> (-6.80%)` | :arrow_down: |
| [superset-frontend/src/common/components/index.tsx](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL2luZGV4LnRzeA==) | `100.00% <0.00%> (ø)` | |
| [.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL0Jhci9pbmRleC5qcw==) | `66.66% <0.00%> (ø)` | |
| [...plugins/legacy-preset-chart-nvd3/src/ReactNVD3.jsx](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL1JlYWN0TlZEMy5qc3g=) | `0.00% <0.00%> (ø)` | |
| [...gins/legacy-preset-chart-nvd3/src/DistBar/index.js](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL0Rpc3RCYXIvaW5kZXguanM=) | `66.66% <0.00%> (ø)` | |
| [...mponents/nativeFilters/FiltersConfigModal/state.ts](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL3N0YXRlLnRz) | `73.33% <0.00%> (ø)` | |
| [...et-frontend/src/components/Menu/LanguagePicker.tsx](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9MYW5ndWFnZVBpY2tlci50c3g=) | | |
| [...frontend/src/common/hooks/useChangeEffect/index.ts](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9ob29rcy91c2VDaGFuZ2VFZmZlY3QvaW5kZXgudHM=) | | |
| ... and [40 more](https://codecov.io/gh/apache/superset/pull/17988/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9e69940...5812a79](https://codecov.io/gh/apache/superset/pull/17988?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781635395
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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')));
+ });
Review comment:
I moved the catch functionality into the report modal [here](https://github.com/apache/superset/blob/5812a79c0e8eb6cf70297de197534ea9e62a4b52/superset-frontend/src/components/ReportModal/index.tsx#L252-L256), this should handle any errors for this endpoint. Should I also put a catch in the action?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r783371366
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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 || ''}
Review comment:
Currently the error stays until the user submits the form again. Once the 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781683254
##########
File path: 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')}
Review comment:
I don't think a translation is necessary here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r781631788
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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();
Review comment:
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.
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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')));
+ });
Review comment:
What happens when the endpoint returns an error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #17988: fix: Keep Report modal open when there's an error
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #17988:
URL: https://github.com/apache/superset/pull/17988#discussion_r782652464
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -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 || ''}
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org