Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(v2): add clearer error messages when updating collaborators #3864

Merged
merged 9 commits into from
May 19, 2022
Merged

feat(v2): add clearer error messages when updating collaborators #3864

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dfb6291
feat: add clearer error messages when updating collaborators
May 12, 2022
fadfb1d
fix: use non-null assertion to assert optional requestEmail variable …
May 13, 2022
fd629fc
fix: add non-null check of email in request for 422 error messages
May 17, 2022
0f06cb6
fix: refactor type of error declared in func
May 17, 2022
b1143ed
fix: add return type to function
May 17, 2022
878b4ef
feat: add tests
May 18, 2022
e503a8f
feat: remove interactions for collaborator modal stories
May 18, 2022
a0e3423
fix: remove unprocessable entity error story
May 18, 2022
133fcfa
feat: update error message for bad request error
May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 100 additions & 7 deletions frontend/src/features/admin-form/common/mutations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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')
Expand All @@ -55,6 +64,72 @@ 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:
errorMessage = `${!requestEmail} is not part of a whitelisted agency`
Copy link
Contributor

@timotheeg timotheeg May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ! a not a non-null assertion, it’s the logical not operator. It will convert whatever truthiness requestEmail represents to its opposite true or false, and so the resulting message will be either :

true is not part of a whitelisted agency

or

false is not part of a whitelisted agency

which is not what we want 😅 .

If requestEmail is always defined on 422 errors, we can make it clear with a cast:
errorMessage = ${requestEmail as string} is not part of a whitelisted agency

If requestEmail may be undefined, we’d be better off with a conditional block:

errorMessage = requestEmail
    ? `${requestEmail} is not part of a whitelisted agency`
    : `An unexpected error 422 happened`

One small issue I have with doing a straight cast in this case is code self-consistency and implicit assumptions. Maybe we can try to make things a little clearer 🤔 . Based on the call tree of getMappedErrorMessage(), requestEmail is only supplied if formCollaboratorAction is FormCollaboratorAction.ADD. The 422 error code is not inspected in a context where formCollaboratorAction has been checked, so if we just cast requestEmail as string to show we know it's true, we've implicitly stated only FormCollaboratorAction.ADD returns 422. It may be true, but it's a bit dirty and not obvious from the code itself (the function signature shows requestEmail can be undefined).

Long story short: I'd recommend going the defensive programming route here. Either do a check to verify requestEmail is not undefined like above, and have an error message for unexpected cases. Or check on formCollaboratorAction first, and then cast to string, and you likely still need an error message for unexpected cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation. I initially assumed that it was okay to add an empty string if in the case requestEmail is empty because the 422 error thrown only occurs for FormCollaboratorAction.Add. I do realise now that it is not clear in the code that that is the case and it is better just to do a simple non-null check and throw a different error if requestEmail is null.

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,
Expand All @@ -75,10 +150,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',
})
},
Expand Down Expand Up @@ -113,7 +196,9 @@ export const useMutateCollaborators = () => {
} has been updated to the ${permissionsToRole(permissionToUpdate)} role`
handleSuccess({ newData, toastDescription })
},
onError: handleError,
onError: (error: Error) => {
handleError(error, FormCollaboratorAction.UPDATE)
},
},
)

Expand All @@ -129,7 +214,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)
},
},
)

Expand All @@ -149,7 +236,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)
},
},
)

Expand All @@ -173,7 +262,9 @@ export const useMutateCollaborators = () => {
newData.form,
)
},
onError: handleError,
onError: (error: Error) => {
handleError(error, FormCollaboratorAction.TRANSFER_OWNERSHIP)
},
},
)

Expand All @@ -189,7 +280,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)
},
},
)

Expand Down