CCM-15042: letter render error handling by ClareJonesBJSS · Pull Request #901 · NHSDigital/nhs-notify-web-template-management

ClareJonesBJSS · 2026-04-08T15:24:32Z

Description

Adds render check to 'Approve template'
Adds empty personalisations check to 'Update preview'
Adds new context for the errors so all error summaries can appear at the top whilst remaining separate forms
Tweaked the letter render tabs so they don't use unneeded form providers when we're in a read-only context
Amended Select, Input and FileInput atoms to correctly apply error styling, and to do so based on field id rather than name - we have to use id so that our use of htmlFor is correct, and the links in the summary go to the right place - Also switched a couple of places that weren't using those atoms to use them, resulting in a few fields changing outside of the scope of the ticket, along with some other snapshots updating now the styling is correct, but it felt like too small a bug to bother ticketing separately.

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I am familiar with the contributing guidelines
I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming

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.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

chris-elliott-nhsd · 2026-04-10T09:28:12Z

+  const { setParentErrorState, parentErrorState, letterRenderErrorState } =
+    useLetterRenderError();
+
+  useEffect(() => {


why does this need to be in a useEffect? also seen elsewhere

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

nicki-nhs · 2026-04-14T15:21:06Z

  customSection: {
    heading: 'Custom personalisation fields',
+    error: {
+      required: `Enter example data for {{field}}`,


Suggested change

required: `Enter example data for {{field}}`,

required: 'Enter example data for {{field}}',

harrim91 · 2026-04-14T15:32:21Z

  ...props
-}: Omit<HTMLProps<HTMLInputElement>, 'type'>) {
+}: Omit<HTMLProps<HTMLInputElement>, 'type'> & { id: string; name: string }) {
+  const [state] = useNHSNotifyForm();


Just wondering if it would be worth moving this or wrapping this in the NHSNotifyForm directory. We have a few "connected" components in there and exposed like NHSNotifyForm.Input, NHSNotifyForm.RadioInput etc. Since this now has to be rendered inside an NHSNotifyForm provider, then it might be better to move it into that mini module.

Updated, and got rid of the full div, since it was only used in the pdf upload so is going away anyway

nicki-nhs · 2026-04-16T13:32:45Z

    this.errorSummary = page.locator('[class="nhsuk-error-summary"]');
+    this.errorSummaryLinks = this.errorSummary.locator('a');


I feel like errorSummary exists on the base page so probs dont need this one?
and might be good idea to add the links locator there too, what do you think?

nicki-nhs · 2026-04-16T13:36:07Z

+      getInlineError: (fieldId: string): Locator =>
+        panel
+          .locator(`[id="${fieldId}"]`)
+          .locator('xpath=..')


what you doing with this?

it steps up to the parent div in order to find the correct sibling .nhsuk-error-message. Though now you ask it, I can't recall why I ended up doing it this way rather than just giving the error messages IDs 🤷‍♀️ will give the latter a try in the morning

nicki-nhs · 2026-04-21T11:48:01Z

+        await expect(shortErrorLink).toBeHidden();
+      });
+
+      test('shows both short and long example errors when personalised renders have failed', async ({


I'm confused about this one - if the render has failed, will "Enter short example data" fix it?

It has when I've done this scenario manually, but that's because there was nothing wrong with the ability to render and I'd manually edited in ddb to recreate it. No idea what could cause a genuine failure here so hard to say what we should be advising - if it were a transient carbone glitch or aws issue then rerender would be appropriate, but I suppose either way the message isn't accurate.

Chatted with Lee about it, I'm thinking any scenario where a render is FAILED, either it will be resolved by a rerender, or we're in servicenow territory anyway (some system issue or eg the original docx has vanished from s3). Either way its not normal operations, so it's probably not worth giving it special handling, as long as we're pointing them at trying the render again (which will presumably be displaying 'No preview available' so it'll be obvious there's a problem), and blocking them from approving the template?

harrim91 · 2026-04-22T11:00:14Z

+  test('activates the hidden tab and focuses the field when its error link is clicked', async () => {
+    renderTabsWithError('hidden-tab-field');
+
+    const rafSpy = jest


My main thought here is "is there a way to test this without all of this mocking?"

So e.g.

const field = document.querySelector('#hidden-tab-field'); expect(field).toHaveFocus()

const field = document.querySelector('#hidden-tab-field'); expect(focusMock).toHaveBeenCalledTimes(1); expect(focusMock.mock.instances[0]).toBe(field);

userEvent tends to be a better option than fireEvent, I don't know if that'd help?

Or can it be covered in playwright on the relevant pages?

I think this is an improvement? 😵 and added a playwright test

nicki-nhs · 2026-04-22T12:40:18Z


+/**
+ * Handles clicks on error summary links that point to form fields inside unselected NHS UK tab panels.
+ * Iff the target field is inside a hidden tab panel (`nhsuk-tabs__panel--hidden`), the default


Suggested change

* Iff the target field is inside a hidden tab panel (`nhsuk-tabs__panel--hidden`), the default

* If the target field is inside a hidden tab panel (`nhsuk-tabs__panel--hidden`), the default

Was actually intentional (if and only if), but fair 😅

nicki-nhs · 2026-04-27T14:09:32Z

  );

  expect(container.asFragment()).toMatchSnapshot();
-  expect(focusMock).toHaveBeenCalled();


are we still testing this somewhere else?

nicki-nhs · 2026-04-27T14:38:15Z

+          'Choose example recipient'
+        );
+
+        const summaryLink = previewPage.errorSummaryLinks.filter({


are we clicking these links anywhere?

ClareJonesBJSS requested a review from a team as a code owner April 8, 2026 15:24

ClareJonesBJSS force-pushed the feature/CCM-15042_letter-gen-errors branch 4 times, most recently from ab401d0 to f935271 Compare April 9, 2026 16:01

chris-elliott-nhsd reviewed Apr 10, 2026

View reviewed changes

Comment thread frontend/src/__tests__/components/providers/letter-render-error-provider.test.tsx Outdated

chris-elliott-nhsd reviewed Apr 10, 2026

View reviewed changes

Comment thread frontend/src/app/preview-letter-template/[templateId]/server-action.ts

ClareJonesBJSS commented Apr 10, 2026

View reviewed changes

Comment thread frontend/src/app/preview-approved-letter-template/[templateId]/page.tsx Outdated

chris-elliott-nhsd reviewed Apr 10, 2026

View reviewed changes

ClareJonesBJSS changed the title ~~CCM-15042: letter render error handling~~ Draft: CCM-15042: letter render error handling Apr 10, 2026

chris-elliott-nhsd reviewed Apr 10, 2026

View reviewed changes

Comment thread frontend/src/components/molecules/LetterRender/CombinedLetterErrorSummary.tsx

ClareJonesBJSS changed the title ~~Draft: CCM-15042: letter render error handling~~ CCM-15042: letter render error handling Apr 10, 2026

chris-elliott-nhsd previously approved these changes Apr 13, 2026

View reviewed changes

nicki-nhs reviewed Apr 14, 2026

View reviewed changes

harrim91 reviewed Apr 14, 2026

View reviewed changes

harrim91 reviewed Apr 15, 2026

View reviewed changes

Comment thread frontend/src/components/atoms/FileUpload/FileUpload.tsx Outdated

ClareJonesBJSS dismissed chris-elliott-nhsd’s stale review via d2b1512 April 15, 2026 10:15

ClareJonesBJSS force-pushed the feature/CCM-15042_letter-gen-errors branch 2 times, most recently from 0a016e6 to 549b0f1 Compare April 15, 2026 13:01

harrim91 previously approved these changes Apr 15, 2026

View reviewed changes

nicki-nhs reviewed Apr 16, 2026

View reviewed changes

ClareJonesBJSS dismissed harrim91’s stale review via 758f4f1 April 20, 2026 16:38

ClareJonesBJSS force-pushed the feature/CCM-15042_letter-gen-errors branch 2 times, most recently from 1d94c65 to b64137a Compare April 21, 2026 10:35

nicki-nhs reviewed Apr 21, 2026

View reviewed changes

chris-elliott-nhsd previously approved these changes Apr 22, 2026

View reviewed changes

harrim91 reviewed Apr 22, 2026

View reviewed changes

ClareJonesBJSS dismissed chris-elliott-nhsd’s stale review via c71e4c0 April 22, 2026 11:36

nicki-nhs reviewed Apr 22, 2026

View reviewed changes

Comment thread frontend/src/components/forms/PdfLetterTemplateForm/PdfLetterTemplateForm.tsx

nicki-nhs reviewed Apr 22, 2026

View reviewed changes

Comment thread frontend/src/components/molecules/NhsNotifyErrorSummary/NhsNotifyErrorSummary.tsx Outdated

ClareJonesBJSS force-pushed the feature/CCM-15042_letter-gen-errors branch from e215de9 to b49897e Compare April 22, 2026 16:24

andykay-nhs reviewed Apr 23, 2026

View reviewed changes

Comment thread frontend/src/components/molecules/NhsNotifyErrorSummary/NhsNotifyErrorSummary.tsx Outdated

andykay-nhs reviewed Apr 23, 2026

View reviewed changes

Comment thread frontend/src/components/molecules/NhsNotifyErrorSummary/NhsNotifyErrorSummary.tsx Outdated

andykay-nhs previously approved these changes Apr 24, 2026

View reviewed changes

CCM-15042: letter render error handlng

f9300b9

chris-elliott-nhsd dismissed andykay-nhs’s stale review via f9300b9 April 27, 2026 10:45

chris-elliott-nhsd force-pushed the feature/CCM-15042_letter-gen-errors branch from cc47125 to f9300b9 Compare April 27, 2026 10:45

chris-elliott-nhsd added 3 commits April 27, 2026 11:50

CCM-15042: Fix typecheck

a847a5e

CCM-15042: Fix linting

3735d92

Merge branch 'main' into feature/CCM-15042_letter-gen-errors

7b54c86

andykay-nhs approved these changes Apr 27, 2026

View reviewed changes

nicki-nhs reviewed Apr 27, 2026

View reviewed changes

	required: `Enter example data for {{field}}`,
	required: 'Enter example data for {{field}}',

		this.errorSummary = page.locator('[class="nhsuk-error-summary"]');
		this.errorSummaryLinks = this.errorSummary.locator('a');

	* Iff the target field is inside a hidden tab panel (`nhsuk-tabs__panel--hidden`), the default
	* If the target field is inside a hidden tab panel (`nhsuk-tabs__panel--hidden`), the default

Conversation

ClareJonesBJSS commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chris-elliott-nhsd Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClareJonesBJSS Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClareJonesBJSS Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClareJonesBJSS Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ClareJonesBJSS commented Apr 8, 2026 •

edited

Loading

chris-elliott-nhsd Apr 10, 2026 •

edited

Loading

ClareJonesBJSS Apr 10, 2026 •

edited

Loading

ClareJonesBJSS Apr 22, 2026 •

edited

Loading

ClareJonesBJSS Apr 22, 2026 •

edited

Loading