Fix mail box creation error handling and mail domain addition form #355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daproclaima
force-pushed
the
fix/mail-domains
branch
2 times, most recently
from
7a96b17
to
2ebd036
Compare
sdemagny
reviewed
Aug 26, 2024
src/frontend/apps/desk/src/features/mail-domains/components/ModalAddMailDomain.tsx
Outdated
Show resolved
Hide resolved
sdemagny
reviewed
Aug 26, 2024
There was a problem hiding this comment.
En attendant le sentry, est-ce qu'on pourrait pas avoir quelque soit le code d'erreur (tous les codes d'erreur) le code d'erreur récupéré et le message associé affiché sur l'interface ?
Dès qu'on aura sentry on aura des log clairs et on cachera tout ça à l'utilisateur.
Le but c'est de ne plus avoir d'interface/validation de formulaire qui ne fonctionne pas en silence.
Qu'est-ce que vous en dites ? @AntoLC @daproclaima
daproclaima
force-pushed
the
fix/mail-domains
branch
2 times, most recently
from
9847f15
to
011732b
Compare
|
@sdemagny, je suis du même avis. C'est ce que j'ai fait ici: add mail domain error handling et create mailbox error handling |
daproclaima
force-pushed
the
fix/mail-domains
branch
from
011732b
to
8625438
Compare
daproclaima
force-pushed
the
fix/mail-domains
branch
from
8625438
to
70c7c87
Compare
AntoLC
reviewed
Aug 29, 2024
src/frontend/yarn.lock
Outdated
There was a problem hiding this comment.
You don't seem to add or update dependencies, this file should not have changes.
src/frontend/apps/e2e/__tests__/app-desk/mail-domains-add.spec.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/desk/src/features/mail-domains/components/useAddMailDomainApiError.tsx
Outdated
Show resolved
Hide resolved
daproclaima
force-pushed
the
fix/mail-domains
branch
5 times, most recently
from
0a3cdfd
to
2d94bc4
Compare
daproclaima
force-pushed
the
fix/mail-domains
branch
from
2d94bc4
to
baec47d
Compare
daproclaima
commented
Sep 5, 2024
src/frontend/apps/desk/src/features/mail-domains/components/ModalCreateMailbox.tsx
Outdated
Show resolved
Hide resolved
AntoLC
reviewed
Sep 5, 2024
...rontend/apps/desk/src/features/mail-domains/components/__tests__/ModalCreateMailbox.test.tsx
Outdated
Show resolved
Hide resolved
...rontend/apps/desk/src/features/mail-domains/components/__tests__/ModalAddMailDomain.test.tsx
Outdated
Show resolved
Hide resolved
...rontend/apps/desk/src/features/mail-domains/components/__tests__/ModalCreateMailbox.test.tsx
Outdated
Show resolved
Hide resolved
...rontend/apps/desk/src/features/mail-domains/components/__tests__/ModalCreateMailbox.test.tsx
Outdated
Show resolved
Hide resolved
...rontend/apps/desk/src/features/mail-domains/components/__tests__/ModalAddMailDomain.test.tsx
Outdated
Show resolved
Hide resolved
daproclaima
force-pushed
the
fix/mail-domains
branch
3 times, most recently
from
ec9ee3a
to
813497a
Compare
daproclaima
force-pushed
the
fix/mail-domains
branch
from
813497a
to
ee47ec2
Compare
daproclaima
force-pushed
the
fix/mail-domains
branch
from
ee47ec2
to
12ce4d0
Compare
- allow to submit form by pressing "Enter" key - force focus on form when form is submited but is invalid - add error 500 handling - update related e2e tests
- add hook to handle api errors. - add related component tests
daproclaima
force-pushed
the
fix/mail-domains
branch
from
12ce4d0
to
13ac2d8
Compare
- rename CreateMailboxForm into ModalCreateMailbox, and useCreateMailDomain into useAddMailDomain - use useAPIError hook in ModalCreateMailbox.tsx and ModalAddMailDomain - update translations and tests (include removal of e2e test able to be asserted by component tests)
daproclaima
force-pushed
the
fix/mail-domains
branch
from
13ac2d8
to
fe967a6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
When we want to create a mailbox for a mail domain, it takes for this mail domain to have a valid secret configured. As far as I understand, this is done manually. So, this is a complex use case to cover with automated tests, which could not be tested with the e2e tests.
Concerning the form to add a mail domain, it could not be submitted when pressing the 'enter' key and would instead update the URL with a query and a page refresh.
Proposal
(watch video from 00:30 to 1:15)
before.fix.api.error.handling.mp4
after.fix.api.error.handling.mp4
before.form.add.mail.domain.fix.mp4
after.form.add.mail.domain.fix.mp4
Error catching and cause replacement were already done on the app, but not with a centralized method. Also, no error message used to be shown for the error 500 type (not shown in the video).
Fix #350, resolves #351