Skip to content

Commit

Permalink
🥅(frontend) catch mailbox creation errors
Browse files Browse the repository at this point in the history
- display error message for error 500 and 400 for
missing mail domain secret and force focus on
first name form field when api error is caught
- update related e2e test, component tests and
translations
- rename CreateMailboxForm.tsx into FormCreateMailbox.tsx

**Attention**
Writing e2e tests to assert this behavior checking both
the frontend and the backend is complex. So for the
moment, the requests interceptions are kept. To write
proper tests, it takes to use a mail domain already
enabled and with a valid secret, which are actions
performed manually on Django Admin.
To test these behaviors manually, it takes to use
a mail domain with an empty secret for the error 400,
and a invalid secret for the error 500.
  • Loading branch information
daproclaima committed Aug 27, 2024
1 parent 5362f13 commit 9847f15
Show file tree
Hide file tree
Showing 13 changed files with 510 additions and 98 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ and this project adheres to

### Fixed

- 🐛(frontend) user can submit form to add mail domain by pressing "Enter" key
🐛(frontend) fix mai domain addition submission #355
🥅(frontend) improve api error handling on mailbox creation form #355

## [1.0.1] - 2024-08-19

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export const createMailbox = async ({
});

if (!response.ok) {
// TODO: extend errorCauses to return the name of the invalid field names to highlight in the form?
throw new APIError(
'Failed to create the mailbox',
await errorCauses(response),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { default as MailDomainsLogo } from '../assets/mail-domains-logo.svg';
import { PAGE_SIZE } from '../conf';
import { MailDomain, MailDomainMailbox } from '../types';

import { CreateMailboxForm } from './forms/CreateMailboxForm';
import { FormCreateMailbox } from './forms/FormCreateMailbox';

export type ViewMailbox = {
name: string;
Expand Down Expand Up @@ -87,7 +87,7 @@ export function MailDomainsContent({ mailDomain }: { mailDomain: MailDomain }) {
) : (
<>
{isCreateMailboxFormVisible && mailDomain ? (
<CreateMailboxForm
<FormCreateMailbox
mailDomain={mailDomain}
closeModal={() => setIsCreateMailboxFormVisible(false)}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,83 +8,18 @@ import {
} from '@openfun/cunningham-react';
import { useRouter } from 'next/navigation';
import React from 'react';
import {
Controller,
FormProvider,
UseFormReturn,
useForm,
} from 'react-hook-form';
import { Controller, FormProvider, useForm } from 'react-hook-form';
import { useTranslation } from 'react-i18next';
import { z } from 'zod';

import { APIError } from '@/api';
import { Box, StyledLink, Text, TextErrors } from '@/components';
import { useCreateMailDomain } from '@/features/mail-domains';

import { default as MailDomainsLogo } from '../assets/mail-domains-logo.svg';

const FORM_ID = 'form-add-mail-domain';

const useAddMailDomainApiError = ({
error,
methods,
}: {
error: APIError | null;
methods: UseFormReturn<{ name: string }> | null;
}): string[] | undefined => {
const [errorCauses, setErrorCauses] = React.useState<undefined | string[]>(
undefined,
);
const { t } = useTranslation();

React.useEffect(() => {
if (methods && t && error) {
let causes = undefined;

if (error.cause?.length) {
const parseCauses = (causes: string[]) =>
causes.reduce((arrayCauses, cause) => {
switch (cause) {
case 'Mail domain with this name already exists.':
case 'Mail domain with this Slug already exists.':
methods.setError('name', {
type: 'manual',
message: t(
'This mail domain is already used. Please, choose another one.',
),
});
break;
default:
arrayCauses.push(cause);
}

return arrayCauses;
}, [] as string[]);
import { useAddMailDomainApiError } from './useAddMailDomainApiError';

causes = parseCauses(error.cause);
}

if (error.status === 500 || !error.cause) {
causes = [
t(
'Your request cannot be processed because the server is experiencing an error. If the problem ' +
'persists, please contact our support to resolve the issue: suiteterritoriale@anct.gouv.fr.',
),
];
}

setErrorCauses(causes);
}
}, [methods, t, error]);

React.useEffect(() => {
if (errorCauses && methods) {
methods.setFocus('name');
}
}, [methods, errorCauses]);

return errorCauses;
};
const FORM_ID = 'form-add-mail-domain';

export const ModalAddMailDomain = () => {
const { t } = useTranslation();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import { renderHook } from '@testing-library/react';
import { UseFormReturn } from 'react-hook-form';

import { APIError } from '@/api';
import { useAddMailDomainApiError } from '@/features/mail-domains/components/useAddMailDomainApiError';

jest.mock('react-i18next', () => ({
useTranslation: () => ({
t: (key: string) => key, // Simple mock to return the key as the translation
}),
}));

describe('useAddMailDomainApiError', () => {
const setErrorMock = jest.fn();
const setFocusMock = jest.fn();

const methodsMock: Partial<UseFormReturn<{ name: string }>> = {
setError: setErrorMock,
setFocus: setFocusMock,
};

afterEach(() => {
jest.clearAllMocks();
});

it('should set the correct error message when the mail domain already exists', () => {
const error = {
cause: ['Mail domain with this name already exists.'],
status: 400,
} as APIError;

const { result } = renderHook(() =>
useAddMailDomainApiError({
error,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(setErrorMock).toHaveBeenCalledWith('name', {
type: 'manual',
message: 'This mail domain is already used. Please, choose another one.',
});

expect(result.current).toHaveLength(0);
});

it('should handle multiple causes and set the correct error message', () => {
const error = {
cause: ['Mail domain with this name already exists.', 'Other error'],
status: 400,
} as APIError;

const { result } = renderHook(() =>
useAddMailDomainApiError({
error,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(setErrorMock).toHaveBeenCalledWith('name', {
type: 'manual',
message: 'This mail domain is already used. Please, choose another one.',
});

expect(result.current).toEqual(['Other error']);
});

it('should prepend server error message when the status is 500', () => {
const error = {
cause: ['Some other error'],
status: 500,
} as APIError;

const { result } = renderHook(() =>
useAddMailDomainApiError({
error,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(result.current).toEqual([
'Your request cannot be processed because the server is experiencing an error. If the problem persists, please contact our support to resolve the issue: suiteterritoriale@anct.gouv.fr',
'Some other error',
]);
});

it('should handle cases with no specific cause and only server error', () => {
const error = {
status: 500,
} as APIError;

const { result } = renderHook(() =>
useAddMailDomainApiError({
error,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(result.current).toEqual([
'Your request cannot be processed because the server is experiencing an error. If the problem persists, please contact our support to resolve the issue: suiteterritoriale@anct.gouv.fr',
]);
});

it('should focus on the name field if there are error causes', () => {
const error = {
cause: ['Some other error'],
status: 400,
} as APIError;

const { result } = renderHook(() =>
useAddMailDomainApiError({
error,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(setFocusMock).toHaveBeenCalledWith('name');
expect(result.current).toEqual(['Some other error']);
});

it('should do nothing if no error is passed', () => {
const { result } = renderHook(() =>
useAddMailDomainApiError({
error: null,
methods: methodsMock as UseFormReturn<{ name: string }>,
}),
);

expect(result.current).toHaveLength(0);
expect(setErrorMock).not.toHaveBeenCalled();
expect(setFocusMock).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { Box, Text, TextErrors } from '@/components';
import { CreateMailboxParams, useCreateMailbox } from '../../api';
import { MailDomain } from '../../types';

import { useCreateMailboxApiError } from './useCreateMailboxApiError';

const FORM_ID: string = 'form-create-mailbox';

const GlobalStyle = createGlobalStyle`
Expand All @@ -32,7 +34,7 @@ const GlobalStyle = createGlobalStyle`
}
`;

export const CreateMailboxForm = ({
export const FormCreateMailbox = ({
mailDomain,
closeModal,
}: {
Expand Down Expand Up @@ -88,27 +90,15 @@ export const CreateMailboxForm = ({
},
});

const errorCauses = useCreateMailboxApiError({ error, methods });

const onSubmitCallback = (event: React.FormEvent) => {
event.preventDefault();
void methods.handleSubmit((data) =>
createMailbox({ ...data, mailDomainSlug: mailDomain.slug }),
)();
};

const causes = error?.cause?.filter((cause) => {
const isFound =
cause === 'Mailbox with this Local_part and Domain already exists.';

if (isFound) {
methods.setError('local_part', {
type: 'manual',
message: t('This email prefix is already used.'),
});
}

return !isFound;
});

return (
<FormProvider {...methods}>
<Modal
Expand Down Expand Up @@ -152,8 +142,12 @@ export const CreateMailboxForm = ({
>
<GlobalStyle />
<Box $width="100%" $margin={{ top: 'none', bottom: 'xl' }}>
{!!causes?.length && (
<TextErrors $margin={{ bottom: 'small' }} causes={causes} />
{!!errorCauses?.length && (
<TextErrors
$margin={{ bottom: 'small' }}
causes={errorCauses}
$textAlign="left"
/>
)}
<Text
$margin={{ horizontal: 'none', vertical: 'big' }}
Expand Down
Loading

0 comments on commit 9847f15

Please sign in to comment.