diegomedina248 commented on a change in pull request #19112:
URL: https://github.com/apache/superset/pull/19112#discussion_r828715416
##########
File path: superset-frontend/src/views/CRUD/hooks.ts
##########
@@ -464,7 +467,7 @@ export function useImportResource(
updateState({ loading: false });
});
},
- [],
+ [handleErrorMsg, resourceLabel, resourceName],
Review comment:
handleErrorMsg is not memoized upstream, which could cause troubles,
even if this is a callback (the performance impact of constant rerenders could
yield some issues).
@evans maybe this is a bit out of scope, so up to you, but if we can check
the code and see if we can properly memoize the chain here that would be great.
##########
File path: superset-frontend/src/views/CRUD/hooks.ts
##########
@@ -448,7 +448,10 @@ export function useImportResource(
t(
'An error occurred while importing %s: %s',
resourceLabel,
- error.errors.map(payload => payload.message).join('\n'),
+ [
+ ...error.errors.map(payload => payload.message),
+ 'Please re-export your file and try importing again',
Review comment:
was this required/checked with the team?
Also, this should be translatable
##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -401,15 +401,17 @@ export const getAlreadyExists = (errors: Record<string,
any>[]) =>
.flat();
export const hasTerminalValidation = (errors: Record<string, any>[]) =>
- errors.some(
- error =>
- !Object.entries(error.extra)
- .filter(([key, _]) => key !== 'issue_codes')
- .every(
- ([_, payload]) =>
- isNeedsPassword(payload) || isAlreadyExists(payload),
- ),
- );
+ errors.some(error => {
+ const not_issues_codes = Object.entries(error.extra).filter(
+ ([key, _]) => key !== 'issue_codes',
+ );
+
+ if (not_issues_codes.length === 0) return true;
+
+ return !not_issues_codes.every(
+ ([_, payload]) => isNeedsPassword(payload) || isAlreadyExists(payload),
Review comment:
`[, payload]`.
I know this was not done by you, but we might as well clean it up
##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -401,15 +401,17 @@ export const getAlreadyExists = (errors: Record<string,
any>[]) =>
.flat();
export const hasTerminalValidation = (errors: Record<string, any>[]) =>
- errors.some(
- error =>
- !Object.entries(error.extra)
- .filter(([key, _]) => key !== 'issue_codes')
- .every(
- ([_, payload]) =>
- isNeedsPassword(payload) || isAlreadyExists(payload),
- ),
- );
+ errors.some(error => {
+ const not_issues_codes = Object.entries(error.extra).filter(
Review comment:
this should be CamelCased
##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -401,15 +401,17 @@ export const getAlreadyExists = (errors: Record<string,
any>[]) =>
.flat();
export const hasTerminalValidation = (errors: Record<string, any>[]) =>
- errors.some(
- error =>
- !Object.entries(error.extra)
- .filter(([key, _]) => key !== 'issue_codes')
- .every(
- ([_, payload]) =>
- isNeedsPassword(payload) || isAlreadyExists(payload),
- ),
- );
+ errors.some(error => {
+ const not_issues_codes = Object.entries(error.extra).filter(
+ ([key, _]) => key !== 'issue_codes',
Review comment:
the `_` here is not needed
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
