gabotorresruiz commented on code in PR #43191:
URL: https://github.com/apache/superset/pull/43191#discussion_r3806865188
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -17,10 +17,49 @@
* under the License.
*/
import { t } from '@apache-superset/core/translation';
-import { SupersetClient } from '@superset-ui/core';
+import { getClientErrorObject, SupersetClient } from '@superset-ui/core';
import { SelectOption } from 'src/components/ListView';
+import { Actions } from 'src/constants';
import { FormValues } from './types';
+type AddDangerToast = (message: string) => void;
+
+export const handleUserError = async (
+ err: Response,
+ action: Actions.CREATE | Actions.UPDATE,
+ addDangerToast: AddDangerToast,
+): Promise<never> => {
+ let errorMessage =
+ action === Actions.CREATE
+ ? t('There was an error creating the user. Please, try again.')
+ : t('There was an error updating the user. Please, try again.');
+
+ if (err.status === 400 || err.status === 422) {
+ const errorData = await getClientErrorObject(err);
+ const message: unknown = errorData.message;
+
+ if (err.status === 400 && message && typeof message === 'object') {
Review Comment:
Just a small NIT, not a blocker: this branch only fires when `message` is an
object, but the user PUT endpoint also returns 400s with a plain string
message. I verified on this branch that `PUT /api/v1/security/users/<pk>` with
`{"roles": [], "groups": []}` returns `400 {"message": "User must have at least
one role or group!"}`, which still falls through to the generic toast. Since
`parseErrorJson` already copies a string `message` into `.error`, relaxing this
to `if (err.status === 400 && message)` would surface both shapes, and your
non-JSON test still passes because `message` is undefined in that case.
Admittedly the client-side `atLeastOneRoleOrGroup` validator makes this hard to
hit from the modal, so feel free to leave it as is.
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -17,10 +17,49 @@
* under the License.
*/
import { t } from '@apache-superset/core/translation';
-import { SupersetClient } from '@superset-ui/core';
+import { getClientErrorObject, SupersetClient } from '@superset-ui/core';
import { SelectOption } from 'src/components/ListView';
+import { Actions } from 'src/constants';
import { FormValues } from './types';
+type AddDangerToast = (message: string) => void;
+
+export const handleUserError = async (
+ err: Response,
+ action: Actions.CREATE | Actions.UPDATE,
+ addDangerToast: AddDangerToast,
+): Promise<never> => {
+ let errorMessage =
+ action === Actions.CREATE
+ ? t('There was an error creating the user. Please, try again.')
+ : t('There was an error updating the user. Please, try again.');
+
+ if (err.status === 400 || err.status === 422) {
+ const errorData = await getClientErrorObject(err);
+ const message: unknown = errorData.message;
+
+ if (err.status === 400 && message && typeof message === 'object') {
+ errorMessage = errorData.error;
+ } else if (
+ err.status === 422 &&
+ errorData.error.includes('duplicate key value')
Review Comment:
Another small one: `errorData.error` is typed `string` but `parseErrorJson`
can return it as `undefined` for a JSON 422 body that has neither `message` nor
`errors`, and the `TypeError` here would fire before `addDangerToast`, leaving
the user with no toast at all. None of the current backend paths can produce
that (the only 422 from this endpoint is `response_422(message=str(e.orig))`,
always a string), but `errorData.error?.includes('duplicate key value')` is
cheap insurance.
--
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]
