From dfb6291595df4c5506526f0c881fd7a5fe21ac7e Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Thu, 12 May 2022 15:25:31 +0800 Subject: [PATCH 1/9] feat: add clearer error messages when updating collaborators --- .../features/admin-form/common/mutations.ts | 108 ++++++++++++++++-- 1 file changed, 101 insertions(+), 7 deletions(-) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 532c6bd7c7..29e358296f 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -10,6 +10,7 @@ import { import { ROOT_ROUTE } from '~constants/routes' import { useToast } from '~hooks/useToast' +import { HttpError } from '~services/ApiService' import { permissionsToRole } from './components/CollaboratorModal/utils' import { @@ -29,6 +30,14 @@ export type MutateRemoveCollaboratorArgs = { currentPermissions: FormPermissionsDto } +enum FormCollaboratorAction { + UPDATE, + ADD, + REMOVE, + TRANSFER_OWNERSHIP, + REMOVE_SELF, +} + export const useMutateCollaborators = () => { const { formId } = useParams() if (!formId) throw new Error('No formId provided') @@ -55,6 +64,73 @@ export const useMutateCollaborators = () => { [formId, queryClient], ) + const getMappedBadRequestErrorMessage = ( + formCollaboratorAction: FormCollaboratorAction, + ) => { + let badRequestErrorMessage + switch (formCollaboratorAction) { + case FormCollaboratorAction.ADD: + badRequestErrorMessage = `Please ensure that the email entered is a valid government email. If the error still persists, refresh and try again later.` + break + default: + badRequestErrorMessage = `Sorry, an error occurred. Please refresh the page and try again later.` + } + + return badRequestErrorMessage + } + + const getMappedDefaultErrorMessage = ( + formCollaboratorAction: FormCollaboratorAction, + ) => { + let defaultErrorMessage + switch (formCollaboratorAction) { + case FormCollaboratorAction.ADD: + defaultErrorMessage = 'Error adding collaborator.' + break + case FormCollaboratorAction.UPDATE: + defaultErrorMessage = 'Error updating collaborator.' + break + case FormCollaboratorAction.REMOVE: + defaultErrorMessage = 'Error removing collaborator.' + break + case FormCollaboratorAction.REMOVE_SELF: + defaultErrorMessage = 'Error removing self.' + break + case FormCollaboratorAction.TRANSFER_OWNERSHIP: + defaultErrorMessage = 'Error transfering form ownership.' + break + //should not reach + default: + defaultErrorMessage = 'Error.' + } + return defaultErrorMessage + } + + const getMappedErrorMessage = ( + error: HttpError | Error, + formCollaboratorAction: FormCollaboratorAction, + requestEmail?: string, + ) => { + // check if error is an instance of HttpError to be able to access status code of error + if (error instanceof HttpError) { + let errorMessage + switch (error.code) { + case 422: + requestEmail = typeof requestEmail !== 'undefined' ? requestEmail : '' + errorMessage = `${requestEmail} is not part of a whitelisted agency` + break + case 400: + errorMessage = getMappedBadRequestErrorMessage(formCollaboratorAction) + break + default: + errorMessage = getMappedDefaultErrorMessage(formCollaboratorAction) + } + return errorMessage + } + // if error is not of type HttpError return the error message encapsulated in Error object + return error.message + } + const handleSuccess = useCallback( ({ newData, @@ -75,10 +151,18 @@ export const useMutateCollaborators = () => { ) const handleError = useCallback( - (error: Error) => { + ( + error: Error, + formCollaboratorAction: FormCollaboratorAction, + requestEmail?: string, + ) => { toast.closeAll() toast({ - description: error.message, + description: getMappedErrorMessage( + error, + formCollaboratorAction, + requestEmail, + ), status: 'danger', }) }, @@ -113,7 +197,9 @@ export const useMutateCollaborators = () => { } has been updated to the ${permissionsToRole(permissionToUpdate)} role` handleSuccess({ newData, toastDescription }) }, - onError: handleError, + onError: (error: Error) => { + handleError(error, FormCollaboratorAction.UPDATE) + }, }, ) @@ -129,7 +215,9 @@ export const useMutateCollaborators = () => { } has been added as a ${permissionsToRole(newPermission)}` handleSuccess({ newData, toastDescription }) }, - onError: handleError, + onError: (error: Error, { newPermission }) => { + handleError(error, FormCollaboratorAction.ADD, newPermission.email) + }, }, ) @@ -149,7 +237,9 @@ export const useMutateCollaborators = () => { const toastDescription = `${permissionToRemove.email} has been removed as a collaborator` handleSuccess({ newData, toastDescription }) }, - onError: handleError, + onError: (error: Error) => { + handleError(error, FormCollaboratorAction.REMOVE) + }, }, ) @@ -173,7 +263,9 @@ export const useMutateCollaborators = () => { newData.form, ) }, - onError: handleError, + onError: (error: Error) => { + handleError(error, FormCollaboratorAction.TRANSFER_OWNERSHIP) + }, }, ) @@ -189,7 +281,9 @@ export const useMutateCollaborators = () => { // Remove all related queries from cache. queryClient.removeQueries(adminFormKeys.id(formId)) }, - onError: handleError, + onError: (error: Error) => { + handleError(error, FormCollaboratorAction.REMOVE_SELF) + }, }, ) From fadfb1de4b2e1f112bd0e285a80130af505c531d Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Fri, 13 May 2022 12:53:56 +0800 Subject: [PATCH 2/9] fix: use non-null assertion to assert optional requestEmail variable will never be undefined or null --- frontend/src/features/admin-form/common/mutations.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 29e358296f..1b82778af5 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -116,8 +116,7 @@ export const useMutateCollaborators = () => { let errorMessage switch (error.code) { case 422: - requestEmail = typeof requestEmail !== 'undefined' ? requestEmail : '' - errorMessage = `${requestEmail} is not part of a whitelisted agency` + errorMessage = `${!requestEmail} is not part of a whitelisted agency` break case 400: errorMessage = getMappedBadRequestErrorMessage(formCollaboratorAction) From fd629fc25893fefc5b4761db953516939313918f Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Tue, 17 May 2022 09:30:19 +0800 Subject: [PATCH 3/9] fix: add non-null check of email in request for 422 error messages --- frontend/src/features/admin-form/common/mutations.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 1b82778af5..774032d4d7 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -116,7 +116,9 @@ export const useMutateCollaborators = () => { let errorMessage switch (error.code) { case 422: - errorMessage = `${!requestEmail} is not part of a whitelisted agency` + errorMessage = requestEmail + ? `${requestEmail} is not part of a whitelisted agency` + : `An unexpected error 422 happened` break case 400: errorMessage = getMappedBadRequestErrorMessage(formCollaboratorAction) From 0f06cb60bc1831a5f2abf02c6f995f77eaaa2cac Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Tue, 17 May 2022 10:22:03 +0800 Subject: [PATCH 4/9] fix: refactor type of error declared in func tion argument --- frontend/src/features/admin-form/common/mutations.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 774032d4d7..544ac5982d 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -81,7 +81,7 @@ export const useMutateCollaborators = () => { const getMappedDefaultErrorMessage = ( formCollaboratorAction: FormCollaboratorAction, - ) => { + ): string => { let defaultErrorMessage switch (formCollaboratorAction) { case FormCollaboratorAction.ADD: @@ -107,10 +107,10 @@ export const useMutateCollaborators = () => { } const getMappedErrorMessage = ( - error: HttpError | Error, + error: Error, formCollaboratorAction: FormCollaboratorAction, requestEmail?: string, - ) => { + ): string => { // check if error is an instance of HttpError to be able to access status code of error if (error instanceof HttpError) { let errorMessage From b1143edf911759f8426fbf5464745d7323f554fd Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Tue, 17 May 2022 10:22:31 +0800 Subject: [PATCH 5/9] fix: add return type to function --- frontend/src/features/admin-form/common/mutations.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 544ac5982d..1f4e1ba50e 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -66,7 +66,7 @@ export const useMutateCollaborators = () => { const getMappedBadRequestErrorMessage = ( formCollaboratorAction: FormCollaboratorAction, - ) => { + ): string => { let badRequestErrorMessage switch (formCollaboratorAction) { case FormCollaboratorAction.ADD: From 878b4ef0f8aa7ff19fe0be7d4fb50e7575b99ecc Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Wed, 18 May 2022 10:48:37 +0800 Subject: [PATCH 6/9] feat: add tests --- .../CollaboratorModal.stories.tsx | 31 ++++++++++++++++ .../CollaboratorModal.test.tsx | 35 +++++++++++++++++++ .../msw/handlers/admin-form/collaborators.ts | 15 ++++++++ 3 files changed, 81 insertions(+) create mode 100644 frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx diff --git a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx index e06897c0f5..c9c4b8ab27 100644 --- a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx +++ b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx @@ -1,11 +1,14 @@ import { useEffect } from 'react' import ReactDOM from 'react-dom' import { useDisclosure } from '@chakra-ui/hooks' +import { expect } from '@storybook/jest' import { Meta, Story } from '@storybook/react' +import { screen, userEvent, waitFor } from '@storybook/testing-library' import { createFormBuilderMocks, getAdminFormCollaborators, + updateFormCollaborators, } from '~/mocks/msw/handlers/admin-form' import { getUser, MOCK_USER } from '~/mocks/msw/handlers/user' @@ -164,3 +167,31 @@ ViewerViewLoading.parameters = { }), ], } + +export const EditCollaboratorBadRequestError = Template.bind({}) +EditCollaboratorBadRequestError.parameters = { + msw: [ + updateFormCollaborators({ delay: 0, errorCode: 400 }), + ...baseMswRoutes, + ], +} + +export const EditCollaboratorUnprocessableEntityError = Template.bind({}) +EditCollaboratorUnprocessableEntityError.parameters = { + msw: [ + updateFormCollaborators({ delay: 0, errorCode: 422 }), + ...baseMswRoutes, + ], +} +EditCollaboratorUnprocessableEntityError.play = async () => { + await waitFor(async () => + expect(screen.getByText(/manage collaborators/i)).toBeVisible(), + ) + + const input = screen.getByPlaceholderText(/me@example.com/i) + const submitButton = screen.getByRole('button', { name: /add collaborator/i }) + const mockEmail = '123@gmail.com' + + await userEvent.type(input, mockEmail) + await userEvent.click(submitButton) +} diff --git a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx new file mode 100644 index 0000000000..f50a3c8573 --- /dev/null +++ b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx @@ -0,0 +1,35 @@ +import { composeStories } from '@storybook/testing-react' +import { act, render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' + +import * as stories from './CollaboratorModal.stories' + +const { EditCollaboratorBadRequestError } = composeStories(stories) + +describe('400 bad request error', () => { + it('should throw the appropriate 400 error message when user attempts to add collaborator', async () => { + await act(async () => { + render() + }) + + // Wait until msw has fetched all the necessary resources + await waitFor(() => + expect(screen.getByText(/manage collaborators/i)).toBeVisible(), + ) + + const input = screen.getByPlaceholderText(/me@example.com/i) + const submitButton = screen.getByRole('button', { + name: /add collaborator/i, + }) + const mockEmail = '123@open.gov.sg' + + await act(async () => userEvent.type(input, mockEmail)) + await act(async () => userEvent.click(submitButton)) + + await screen.findByRole('alert') + + expect(screen.getByRole('alert').textContent).toBe( + 'Please ensure that the email entered is a valid government email. If the error still persists, refresh and try again later.', + ) + }) +}) diff --git a/frontend/src/mocks/msw/handlers/admin-form/collaborators.ts b/frontend/src/mocks/msw/handlers/admin-form/collaborators.ts index 6e0fc9d70e..c30c1dd510 100644 --- a/frontend/src/mocks/msw/handlers/admin-form/collaborators.ts +++ b/frontend/src/mocks/msw/handlers/admin-form/collaborators.ts @@ -16,3 +16,18 @@ export const getAdminFormCollaborators = ({ }, ) } + +export const updateFormCollaborators = ({ + delay = 0, + errorCode, +}: { + delay?: number | 'infinite' + errorCode: number +}): ReturnType => { + return rest.put( + '/api/v3/admin/forms/:formId/collaborators', + (_req, res, ctx) => { + return res(ctx.delay(delay), ctx.status(errorCode), ctx.json([])) + }, + ) +} From e503a8f1a846b00d96315aa1bcf3e0efeda2643b Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Wed, 18 May 2022 16:08:49 +0800 Subject: [PATCH 7/9] feat: remove interactions for collaborator modal stories --- .../CollaboratorModal.stories.tsx | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx index c9c4b8ab27..e8428a0ddf 100644 --- a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx +++ b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx @@ -1,9 +1,7 @@ import { useEffect } from 'react' import ReactDOM from 'react-dom' import { useDisclosure } from '@chakra-ui/hooks' -import { expect } from '@storybook/jest' import { Meta, Story } from '@storybook/react' -import { screen, userEvent, waitFor } from '@storybook/testing-library' import { createFormBuilderMocks, @@ -183,15 +181,3 @@ EditCollaboratorUnprocessableEntityError.parameters = { ...baseMswRoutes, ], } -EditCollaboratorUnprocessableEntityError.play = async () => { - await waitFor(async () => - expect(screen.getByText(/manage collaborators/i)).toBeVisible(), - ) - - const input = screen.getByPlaceholderText(/me@example.com/i) - const submitButton = screen.getByRole('button', { name: /add collaborator/i }) - const mockEmail = '123@gmail.com' - - await userEvent.type(input, mockEmail) - await userEvent.click(submitButton) -} From a0e3423e9ccc560002b442eab5232f3bc97caf61 Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Wed, 18 May 2022 16:31:24 +0800 Subject: [PATCH 8/9] fix: remove unprocessable entity error story --- .../CollaboratorModal/CollaboratorModal.stories.tsx | 8 -------- 1 file changed, 8 deletions(-) diff --git a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx index e8428a0ddf..c6a4201006 100644 --- a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx +++ b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.stories.tsx @@ -173,11 +173,3 @@ EditCollaboratorBadRequestError.parameters = { ...baseMswRoutes, ], } - -export const EditCollaboratorUnprocessableEntityError = Template.bind({}) -EditCollaboratorUnprocessableEntityError.parameters = { - msw: [ - updateFormCollaborators({ delay: 0, errorCode: 422 }), - ...baseMswRoutes, - ], -} From 133fcfa774f824c2717065ac448820022dbd44b5 Mon Sep 17 00:00:00 2001 From: Siddarth Nandanahosur Suresh Date: Wed, 18 May 2022 17:09:44 +0800 Subject: [PATCH 9/9] feat: update error message for bad request error --- .../components/CollaboratorModal/CollaboratorModal.test.tsx | 2 +- frontend/src/features/admin-form/common/mutations.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx index f50a3c8573..71d52e2216 100644 --- a/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx +++ b/frontend/src/features/admin-form/common/components/CollaboratorModal/CollaboratorModal.test.tsx @@ -29,7 +29,7 @@ describe('400 bad request error', () => { await screen.findByRole('alert') expect(screen.getByRole('alert').textContent).toBe( - 'Please ensure that the email entered is a valid government email. If the error still persists, refresh and try again later.', + 'The collaborator was unable to be added or edited. Please try again or refresh the page.', ) }) }) diff --git a/frontend/src/features/admin-form/common/mutations.ts b/frontend/src/features/admin-form/common/mutations.ts index 1f4e1ba50e..0722ba459f 100644 --- a/frontend/src/features/admin-form/common/mutations.ts +++ b/frontend/src/features/admin-form/common/mutations.ts @@ -70,7 +70,7 @@ export const useMutateCollaborators = () => { let badRequestErrorMessage switch (formCollaboratorAction) { case FormCollaboratorAction.ADD: - badRequestErrorMessage = `Please ensure that the email entered is a valid government email. If the error still persists, refresh and try again later.` + badRequestErrorMessage = `The collaborator was unable to be added or edited. Please try again or refresh the page.` break default: badRequestErrorMessage = `Sorry, an error occurred. Please refresh the page and try again later.`