CCM-15042: letter render error handling#901
Conversation
ClareJonesBJSS
force-pushed
the
feature/CCM-15042_letter-gen-errors
branch
3 times, most recently
from
988710d to
ab401d0
Compare
ClareJonesBJSS
force-pushed
the
feature/CCM-15042_letter-gen-errors
branch
from
ab401d0 to
f935271
Compare
| }); | ||
|
|
||
| describe('LetterRenderErrorProvider', () => { | ||
| it('renders children without error', () => { |
There was a problem hiding this comment.
this isn't testing that it renders the child components, just that it doesn't error
| const validatedTemplate = validateLetterTemplate(template); | ||
|
|
||
| if (!validatedTemplate || validatedTemplate.letterVersion !== 'AUTHORING') { | ||
| return redirect('/invalid-template', RedirectType.replace); |
There was a problem hiding this comment.
i'm probably missing something here - but why do we need this check? how would someone get here without a valid authoring letter template?
There was a problem hiding this comment.
Mostly just following established patterns of validating templates on retrieval, since I was doing the letterVersion check for type narrowing anyway
frontend/src/app/preview-approved-letter-template/[templateId]/page.tsx
Outdated
Show resolved
Hide resolved
| const { setParentErrorState, parentErrorState, letterRenderErrorState } = | ||
| useLetterRenderError(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
why does this need to be in a useEffect? also seen elsewhere
There was a problem hiding this comment.
React will throw complaining that we're updating one component whilst rendering another if called directly, because we're bridging between NHSNotifyFormProvider and LetterRenderErrorProvider.
I could potentially get rid of some useEffects by making it more of a one-way thing from the letter render component and handling it in the page, but clearing one form's errors when the other triggers was tricky, and I found it much easier to pick through what was going on when I broke it out into the separate provider, plus it feels more easily reusable if we end up genning renders anywhere else
ClareJonesBJSS
changed the title
|
|
||
| return ( | ||
| <NhsNotifyErrorSummary | ||
| errorState={parentErrorState || letterRenderErrorState} |
There was a problem hiding this comment.
why is it called CombinedLetterErrorSummary when it's only showing one of the sets of errors?
There was a problem hiding this comment.
Because its pulling errors from two different sources, even if only showing one at a time. Honestly this component and parentErrorState/letterRenderErrorState got renamed a few times, due to repeatedly confusing myself when it was WIP, so very happy to hear if people have other preferences 😆
ClareJonesBJSS
changed the title
2 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.