From a6c4ee98242f9d9429c5459bd33d01b5878c9344 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 12 May 2026 16:44:10 +0100 Subject: [PATCH 1/6] fix(payment): allow GOV.UK Pay submission without notification email --- .../plugins/engine/beta/form-context.test.ts | 3 +- .../plugins/engine/beta/form-context.ts | 4 ++- src/server/plugins/engine/helpers.ts | 25 +++++++++++-- .../pageControllers/SummaryPageController.ts | 36 +++++++++++++++---- 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/server/plugins/engine/beta/form-context.test.ts b/src/server/plugins/engine/beta/form-context.test.ts index aed997266..8196a78aa 100644 --- a/src/server/plugins/engine/beta/form-context.test.ts +++ b/src/server/plugins/engine/beta/form-context.test.ts @@ -292,7 +292,8 @@ describe('resolveFormModel helper', () => { expect(mockFormsService.getFormDefinition).toHaveBeenCalledTimes(2) expect(mockCheckEmailAddressForLiveFormSubmission).toHaveBeenCalledWith( undefined, - true + true, + false ) expect(FormModel).toHaveBeenCalledWith( definition, diff --git a/src/server/plugins/engine/beta/form-context.ts b/src/server/plugins/engine/beta/form-context.ts index 556f5aa99..2fbde3f64 100644 --- a/src/server/plugins/engine/beta/form-context.ts +++ b/src/server/plugins/engine/beta/form-context.ts @@ -5,6 +5,7 @@ import { isEqual } from 'date-fns' import { PREVIEW_PATH_PREFIX } from '~/src/server/constants.js' import { checkEmailAddressForLiveFormSubmission, + definitionHasPaymentField, getCacheService } from '~/src/server/plugins/engine/helpers.js' import { FormModel } from '~/src/server/plugins/engine/models/index.js' @@ -172,7 +173,8 @@ export async function resolveFormModel( checkEmailAddressForLiveFormSubmission( metadata.notificationEmail, - isPreview + isPreview, + definitionHasPaymentField(definition) ) const routePrefix = diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index 3be9f98bd..640d967d2 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -1,4 +1,5 @@ import { + ComponentType, ControllerPath, Engine, getErrorMessage, @@ -280,11 +281,31 @@ export function checkFormStatus(params?: FormParams) { } } +/** + * True when the form definition contains at least one PaymentField. + * Used to gate the email-required check, since payment-only forms can + * submit through GOV.UK Pay without a notification email. + */ +export function definitionHasPaymentField(definition: FormDefinition): boolean { + return definition.pages.some((page) => { + if (!hasComponents(page)) { + return false + } + return page.components.some( + (component) => component.type === ComponentType.PaymentField + ) + }) +} + export function checkEmailAddressForLiveFormSubmission( emailAddress: string | undefined, - isPreview: boolean + isPreview: boolean, + hasPayment = false ) { - if (!emailAddress && !isPreview) { + // Payment-only live forms can submit through GOV.UK Pay without a + // notification email, so we skip the requirement when a PaymentField + // is present. + if (!emailAddress && !isPreview && !hasPayment) { throw Boom.internal( 'An email address is required to complete the form submission' ) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 2863e51ba..f1adec3b9 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -269,10 +269,9 @@ export class SummaryPageController extends QuestionPageController { checkEmailAddressForLiveFormSubmission(notificationEmail, isPreview) - if (notificationEmail) { - const viewModel = this.getSummaryViewModel(request, context) - - try { + try { + if (notificationEmail) { + const viewModel = this.getSummaryViewModel(request, context) await submitForm( context, formMetadata, @@ -281,9 +280,14 @@ export class SummaryPageController extends QuestionPageController { model, notificationEmail ) - } catch (error) { - return this.handleSubmissionError(error, request, h) + } else { + // No notification email is configured (e.g. preview), but a + // PaymentField may still require preauth/capture. Run only the + // PaymentField onSubmit hooks so the GOV.UK Pay redirect fires. + await finalisePaymentFields(request, formMetadata, context, model) } + } catch (error) { + return this.handleSubmissionError(error, request, h) } await cacheService.setConfirmationState(request, { @@ -516,6 +520,26 @@ async function finaliseComponents( } } +/** + * Runs only the PaymentField onSubmit hooks. Used when there is no + * notification email so the GOV.UK Pay preauth/redirect still fires + * without triggering email-dependent components like FileUploadField. + */ +async function finalisePaymentFields( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext, + model: FormModel +) { + for (const page of model.pages) { + for (const field of page.collection.fields) { + if (field instanceof PaymentField) { + await field.onSubmit(request, metadata, context) + } + } + } +} + function submitData( model: FormModel, items: DetailItem[], From 802346aa3b060d9a3d93e218f2cb8c39ada15554 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 12 May 2026 17:05:06 +0100 Subject: [PATCH 2/6] test(payment): cover no-email submission path --- src/server/plugins/engine/helpers.test.ts | 61 ++++++++++ .../SummaryPageController.test.ts | 114 ++++++++++++++++++ .../pageControllers/SummaryPageController.ts | 7 +- 3 files changed, 181 insertions(+), 1 deletion(-) diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index b75d0f537..28b3d48c4 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -11,6 +11,7 @@ import { ValidationError } from 'joi' import { checkEmailAddressForLiveFormSubmission, checkFormStatus, + definitionHasPaymentField, encodeUrl, engine, evaluateTemplate, @@ -426,6 +427,66 @@ describe('Helpers', () => { checkEmailAddressForLiveFormSubmission('test@example.com', true) ).not.toThrow() }) + + it('should not throw when emailAddress is undefined and the form has a PaymentField', () => { + expect(() => + checkEmailAddressForLiveFormSubmission(undefined, false, true) + ).not.toThrow() + }) + + it('should still throw when neither emailAddress nor PaymentField is present on a live form', () => { + expect(() => + checkEmailAddressForLiveFormSubmission(undefined, false, false) + ).toThrow( + Boom.internal( + 'An email address is required to complete the form submission' + ) + ) + }) + }) + + describe('definitionHasPaymentField', () => { + /** + * @param {Array<{ type: ComponentType }>} components + * @returns {FormDefinition} + */ + const buildDef = (components = []) => /** @type {FormDefinition} */ ({ + pages: [ + { + title: 'p', + path: '/p', + components, + next: [] + } + ] + }) + + it('returns true when a PaymentField is present', () => { + expect( + definitionHasPaymentField( + buildDef([{ type: ComponentType.PaymentField }]) + ) + ).toBe(true) + }) + + it('returns false when no PaymentField is present', () => { + expect( + definitionHasPaymentField(buildDef([{ type: ComponentType.TextField }])) + ).toBe(false) + }) + + it('returns false when the form has no components', () => { + expect(definitionHasPaymentField(buildDef([]))).toBe(false) + }) + + it('returns false for pages without a components array', () => { + const def = /** @type {FormDefinition} */ { + pages: [ + { title: 'p', path: '/p', controller: 'TerminalPageController' } + ] + } + expect(definitionHasPaymentField(def)).toBe(false) + }) }) describe('getErrors', () => { diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 4c704d111..a024f6d85 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -1,3 +1,4 @@ +import { PaymentField } from '~/src/server/plugins/engine/components/PaymentField.js' import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { SummaryPageController, @@ -429,4 +430,117 @@ describe('SummaryPageController - Payment (DF-832)', () => { ).rejects.toBe(err) }) }) + + describe('handleFormSubmit - notification email gate', () => { + /** @param {{ notificationEmail?: string }} args */ + function buildHandleFormSubmitHarness({ notificationEmail }) { + const state = /** @type {FormSubmissionState} */ { + $$__referenceNumber: 'ref', + yesNoField: false + } + + const cacheService = { + setConfirmationState: jest.fn().mockResolvedValue(undefined), + clearState: jest.fn().mockResolvedValue(undefined), + resetComponentStates: jest.fn().mockResolvedValue(undefined) + } as unknown as CacheService + + const getFormMetadata = jest.fn().mockResolvedValue({ + notificationEmail + }) + + model.services = { + ...model.services, + formsService: { ...model.services.formsService, getFormMetadata } + } as typeof model.services + + const request = { + ...requestPage, + method: 'post', + params: { ...requestPage.params, path: 'summary' }, + server: { + plugins: { 'forms-engine-plugin': { cacheService } } + }, + yar: { id: 'session-id' }, + logger: { info: jest.fn(), error: jest.fn() } + } as unknown as FormRequestPayload + + const context = model.getFormContext(request, state) + jest + .spyOn(controller, 'proceed') + .mockReturnValue( + undefined as unknown as ReturnType + ) + + return { request, context, cacheService } + } + + it('runs PaymentField onSubmit when notificationEmail is empty', async () => { + const onSubmitSpy = jest + .spyOn(PaymentField.prototype, 'onSubmit') + .mockResolvedValue(undefined) + + const { request, context } = buildHandleFormSubmitHarness({ + notificationEmail: '' + }) + + await controller.handleFormSubmit(request, context, h) + + expect(onSubmitSpy).toHaveBeenCalled() + }) + + it('does not run PaymentField onSubmit when notificationEmail is set (submitForm path handles it)', async () => { + const onSubmitSpy = jest + .spyOn(PaymentField.prototype, 'onSubmit') + .mockResolvedValue(undefined) + + // Stub the submission services so submitForm completes cleanly. + model.services = { + ...model.services, + formSubmissionService: { + ...model.services.formSubmissionService, + submit: jest.fn().mockResolvedValue({ data: { reference: 'r' } }) + }, + outputService: { + ...model.services.outputService, + submit: jest.fn().mockResolvedValue(undefined) + } + } as typeof model.services + + const { request, context } = buildHandleFormSubmitHarness({ + notificationEmail: 'notify@example.com' + }) + + await controller.handleFormSubmit(request, context, h) + + // submitForm runs finaliseComponents which also calls PaymentField + // onSubmit, so onSubmit IS called here too. The point of this test + // is that the no-email path is not taken — we assert by checking the + // outputService.submit was reached (i.e. submitForm ran). + expect(model.services.outputService.submit).toHaveBeenCalled() + onSubmitSpy.mockRestore() + }) + + it('routes errors through handleSubmissionError when finalisePaymentFields throws', async () => { + const err = new Error('pay boom') + jest.spyOn(PaymentField.prototype, 'onSubmit').mockRejectedValue(err) + + const { request, context } = buildHandleFormSubmitHarness({ + notificationEmail: '' + }) + + const handleSubmissionErrorSpy = jest + .spyOn( + controller as unknown as { + handleSubmissionError: (...args: unknown[]) => unknown + }, + 'handleSubmissionError' + ) + .mockReturnValue(undefined) + + await controller.handleFormSubmit(request, context, h) + + expect(handleSubmissionErrorSpy).toHaveBeenCalledWith(err, request, h) + }) + }) }) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index f1adec3b9..0e2d52995 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -267,7 +267,12 @@ export class SummaryPageController extends QuestionPageController { const { notificationEmail } = formMetadata const { isPreview } = checkFormStatus(request.params) - checkEmailAddressForLiveFormSubmission(notificationEmail, isPreview) + const hasPayment = this.findPaymentField() !== undefined + checkEmailAddressForLiveFormSubmission( + notificationEmail, + isPreview, + hasPayment + ) try { if (notificationEmail) { From 0f20b9ef3b7bf2597ef7c3b692f20a105b6619af Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 12 May 2026 17:12:06 +0100 Subject: [PATCH 3/6] fix(payment): use TS type assertions in test fixtures --- src/server/plugins/engine/helpers.test.ts | 29 +++++++++---------- .../SummaryPageController.test.ts | 11 ++++--- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index 28b3d48c4..1e2c7156a 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -446,20 +446,17 @@ describe('Helpers', () => { }) describe('definitionHasPaymentField', () => { - /** - * @param {Array<{ type: ComponentType }>} components - * @returns {FormDefinition} - */ - const buildDef = (components = []) => /** @type {FormDefinition} */ ({ - pages: [ - { - title: 'p', - path: '/p', - components, - next: [] - } - ] - }) + const buildDef = (components: { type: ComponentType }[] = []) => + ({ + pages: [ + { + title: 'p', + path: '/p', + components, + next: [] + } + ] + }) as unknown as FormDefinition it('returns true when a PaymentField is present', () => { expect( @@ -480,11 +477,11 @@ describe('Helpers', () => { }) it('returns false for pages without a components array', () => { - const def = /** @type {FormDefinition} */ { + const def = { pages: [ { title: 'p', path: '/p', controller: 'TerminalPageController' } ] - } + } as unknown as FormDefinition expect(definitionHasPaymentField(def)).toBe(false) }) }) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index a024f6d85..17719397b 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -432,12 +432,15 @@ describe('SummaryPageController - Payment (DF-832)', () => { }) describe('handleFormSubmit - notification email gate', () => { - /** @param {{ notificationEmail?: string }} args */ - function buildHandleFormSubmitHarness({ notificationEmail }) { - const state = /** @type {FormSubmissionState} */ { + function buildHandleFormSubmitHarness({ + notificationEmail + }: { + notificationEmail?: string + }) { + const state = { $$__referenceNumber: 'ref', yesNoField: false - } + } as unknown as FormSubmissionState const cacheService = { setConfirmationState: jest.fn().mockResolvedValue(undefined), From c35e3bd91639fb9e79413ecf9949f1b9d9889fca Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 12 May 2026 17:32:29 +0100 Subject: [PATCH 4/6] refactor(payment): address adversarial review findings --- .../plugins/engine/beta/form-context.ts | 4 +- src/server/plugins/engine/helpers.test.ts | 60 ++----------------- src/server/plugins/engine/helpers.ts | 23 +------ .../SummaryPageController.test.ts | 10 +--- .../pageControllers/SummaryPageController.ts | 15 +++-- 5 files changed, 17 insertions(+), 95 deletions(-) diff --git a/src/server/plugins/engine/beta/form-context.ts b/src/server/plugins/engine/beta/form-context.ts index 2fbde3f64..70c41eaad 100644 --- a/src/server/plugins/engine/beta/form-context.ts +++ b/src/server/plugins/engine/beta/form-context.ts @@ -1,3 +1,4 @@ +import { hasPaymentQuestionInForm } from '@defra/forms-model' import Boom from '@hapi/boom' import { type Request, type Server } from '@hapi/hapi' import { isEqual } from 'date-fns' @@ -5,7 +6,6 @@ import { isEqual } from 'date-fns' import { PREVIEW_PATH_PREFIX } from '~/src/server/constants.js' import { checkEmailAddressForLiveFormSubmission, - definitionHasPaymentField, getCacheService } from '~/src/server/plugins/engine/helpers.js' import { FormModel } from '~/src/server/plugins/engine/models/index.js' @@ -174,7 +174,7 @@ export async function resolveFormModel( checkEmailAddressForLiveFormSubmission( metadata.notificationEmail, isPreview, - definitionHasPaymentField(definition) + hasPaymentQuestionInForm(definition) ) const routePrefix = diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index 1e2c7156a..4487d62f0 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -11,7 +11,6 @@ import { ValidationError } from 'joi' import { checkEmailAddressForLiveFormSubmission, checkFormStatus, - definitionHasPaymentField, encodeUrl, engine, evaluateTemplate, @@ -402,7 +401,7 @@ describe('Helpers', () => { describe('checkEmailAddressForLiveFormSubmission', () => { it('should throw an error if emailAddress is undefined and isPreview is false', () => { expect(() => - checkEmailAddressForLiveFormSubmission(undefined, false) + checkEmailAddressForLiveFormSubmission(undefined, false, false) ).toThrow( Boom.internal( 'An email address is required to complete the form submission' @@ -412,19 +411,19 @@ describe('Helpers', () => { it('should not throw an error if emailAddress is defined and isPreview is false', () => { expect(() => - checkEmailAddressForLiveFormSubmission('test@example.com', false) + checkEmailAddressForLiveFormSubmission('test@example.com', false, false) ).not.toThrow() }) it('should not throw an error if emailAddress is undefined and isPreview is true', () => { expect(() => - checkEmailAddressForLiveFormSubmission(undefined, true) + checkEmailAddressForLiveFormSubmission(undefined, true, false) ).not.toThrow() }) it('should not throw an error if emailAddress is defined and isPreview is true', () => { expect(() => - checkEmailAddressForLiveFormSubmission('test@example.com', true) + checkEmailAddressForLiveFormSubmission('test@example.com', true, false) ).not.toThrow() }) @@ -433,57 +432,6 @@ describe('Helpers', () => { checkEmailAddressForLiveFormSubmission(undefined, false, true) ).not.toThrow() }) - - it('should still throw when neither emailAddress nor PaymentField is present on a live form', () => { - expect(() => - checkEmailAddressForLiveFormSubmission(undefined, false, false) - ).toThrow( - Boom.internal( - 'An email address is required to complete the form submission' - ) - ) - }) - }) - - describe('definitionHasPaymentField', () => { - const buildDef = (components: { type: ComponentType }[] = []) => - ({ - pages: [ - { - title: 'p', - path: '/p', - components, - next: [] - } - ] - }) as unknown as FormDefinition - - it('returns true when a PaymentField is present', () => { - expect( - definitionHasPaymentField( - buildDef([{ type: ComponentType.PaymentField }]) - ) - ).toBe(true) - }) - - it('returns false when no PaymentField is present', () => { - expect( - definitionHasPaymentField(buildDef([{ type: ComponentType.TextField }])) - ).toBe(false) - }) - - it('returns false when the form has no components', () => { - expect(definitionHasPaymentField(buildDef([]))).toBe(false) - }) - - it('returns false for pages without a components array', () => { - const def = { - pages: [ - { title: 'p', path: '/p', controller: 'TerminalPageController' } - ] - } as unknown as FormDefinition - expect(definitionHasPaymentField(def)).toBe(false) - }) }) describe('getErrors', () => { diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index 640d967d2..bdcf683b4 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -1,5 +1,4 @@ import { - ComponentType, ControllerPath, Engine, getErrorMessage, @@ -281,30 +280,12 @@ export function checkFormStatus(params?: FormParams) { } } -/** - * True when the form definition contains at least one PaymentField. - * Used to gate the email-required check, since payment-only forms can - * submit through GOV.UK Pay without a notification email. - */ -export function definitionHasPaymentField(definition: FormDefinition): boolean { - return definition.pages.some((page) => { - if (!hasComponents(page)) { - return false - } - return page.components.some( - (component) => component.type === ComponentType.PaymentField - ) - }) -} - export function checkEmailAddressForLiveFormSubmission( emailAddress: string | undefined, isPreview: boolean, - hasPayment = false + hasPayment: boolean ) { - // Payment-only live forms can submit through GOV.UK Pay without a - // notification email, so we skip the requirement when a PaymentField - // is present. + // Payment-only forms submit via GOV.UK Pay without a notification email. if (!emailAddress && !isPreview && !hasPayment) { throw Boom.internal( 'An email address is required to complete the form submission' diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 17719397b..b41cf5a57 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -492,12 +492,11 @@ describe('SummaryPageController - Payment (DF-832)', () => { expect(onSubmitSpy).toHaveBeenCalled() }) - it('does not run PaymentField onSubmit when notificationEmail is set (submitForm path handles it)', async () => { - const onSubmitSpy = jest + it('routes through submitForm when notificationEmail is set', async () => { + jest .spyOn(PaymentField.prototype, 'onSubmit') .mockResolvedValue(undefined) - // Stub the submission services so submitForm completes cleanly. model.services = { ...model.services, formSubmissionService: { @@ -516,12 +515,7 @@ describe('SummaryPageController - Payment (DF-832)', () => { await controller.handleFormSubmit(request, context, h) - // submitForm runs finaliseComponents which also calls PaymentField - // onSubmit, so onSubmit IS called here too. The point of this test - // is that the no-email path is not taken — we assert by checking the - // outputService.submit was reached (i.e. submitForm ran). expect(model.services.outputService.submit).toHaveBeenCalled() - onSubmitSpy.mockRestore() }) it('routes errors through handleSubmissionError when finalisePaymentFields throws', async () => { diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 0e2d52995..d97ffb7ae 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -274,9 +274,12 @@ export class SummaryPageController extends QuestionPageController { hasPayment ) + const viewModel = notificationEmail + ? this.getSummaryViewModel(request, context) + : undefined + try { - if (notificationEmail) { - const viewModel = this.getSummaryViewModel(request, context) + if (notificationEmail && viewModel) { await submitForm( context, formMetadata, @@ -286,9 +289,7 @@ export class SummaryPageController extends QuestionPageController { notificationEmail ) } else { - // No notification email is configured (e.g. preview), but a - // PaymentField may still require preauth/capture. Run only the - // PaymentField onSubmit hooks so the GOV.UK Pay redirect fires. + // Payment-only submission path — runs PaymentField capture hooks only. await finalisePaymentFields(request, formMetadata, context, model) } } catch (error) { @@ -526,9 +527,7 @@ async function finaliseComponents( } /** - * Runs only the PaymentField onSubmit hooks. Used when there is no - * notification email so the GOV.UK Pay preauth/redirect still fires - * without triggering email-dependent components like FileUploadField. + * Runs PaymentField capture hooks for the no-email submission path. */ async function finalisePaymentFields( request: FormRequestPayload, From cb89545f53fa2bc84d417872b18d064afd401dcc Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Wed, 13 May 2026 18:48:47 +0100 Subject: [PATCH 5/6] feat(engine): include notification email in form model cache key --- package-lock.json | 35 +-------------- .../plugins/engine/beta/form-context.ts | 8 ++-- src/server/plugins/engine/models/FormModel.ts | 6 ++- src/server/plugins/engine/types.ts | 1 + .../engine/views/partials/preview-banner.html | 14 +++++- .../views/partials/preview-banner.test.js | 44 ++++++++++++++++++- 6 files changed, 68 insertions(+), 40 deletions(-) diff --git a/package-lock.json b/package-lock.json index 262490521..da2859278 100644 --- a/package-lock.json +++ b/package-lock.json @@ -236,7 +236,6 @@ "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.29.0.tgz", "integrity": "sha512-CGOfOJqWjg2qW/Mb6zNsDm+u5vFQ8DxXfbM09z69p5Z6+mE1ikP2jUXw+j42Pf1XTYED2Rni5f95npYeuwMDQA==", "license": "MIT", - "peer": true, "dependencies": { "@babel/code-frame": "^7.29.0", "@babel/generator": "^7.29.0", @@ -2082,7 +2081,6 @@ "integrity": "sha512-CYDD3SOtsHtyXeEORYRx2qBtpDJFjRTGXUtmNEMGyzYOKj1TE3tycdlho7kA1Ufx9OYWZzg52QFBGALTirzDSw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@keyv/serialize": "^1.1.1" } @@ -2225,7 +2223,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" }, @@ -2265,7 +2262,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" } @@ -4416,7 +4412,6 @@ "resolved": "https://registry.npmjs.org/@docusaurus/core/-/core-3.10.1.tgz", "integrity": "sha512-3pf2fXXw0eVk8WnC3T4LIigRDupcpvngpKo9Vy7mYyBhuddc0klDUuZAIfzMoK6z05pdlk6EFC/vBSX43+1O5w==", "license": "MIT", - "peer": true, "dependencies": { "@docusaurus/babel": "3.10.1", "@docusaurus/bundler": "3.10.1", @@ -4591,7 +4586,6 @@ "resolved": "https://registry.npmjs.org/@docusaurus/plugin-content-docs/-/plugin-content-docs-3.10.1.tgz", "integrity": "sha512-2jRVrtzjf8LClGTHQlwlwuD3wQXRx3WEoF7XUarJ8Ou+0onV+SLtejsyfY9JLpfUh9hPhXM4pbBGkyAY4Bi3HQ==", "license": "MIT", - "peer": true, "dependencies": { "@docusaurus/core": "3.10.1", "@docusaurus/logger": "3.10.1", @@ -4625,7 +4619,6 @@ "resolved": "https://registry.npmjs.org/@docusaurus/theme-common/-/theme-common-3.10.1.tgz", "integrity": "sha512-0YtmIeoNo1fIw65LO8+/1dPgmDV86UmhMkow37gzjytuiCSQm9xob6PJy0L4kuQEMTLfUOGvkXvZr7GPrHquMA==", "license": "MIT", - "peer": true, "dependencies": { "@docusaurus/mdx-loader": "3.10.1", "@docusaurus/module-type-aliases": "3.10.1", @@ -7365,7 +7358,6 @@ "resolved": "https://registry.npmjs.org/@mdx-js/react/-/react-3.1.1.tgz", "integrity": "sha512-f++rKLQgUVYDAtECQ6fn/is15GkEH9+nZPM3MS0RcxVqoTfawHvDlSCH7JbMhAM6uJ32v3eXLvLmLvjGu7PTQw==", "license": "MIT", - "peer": true, "dependencies": { "@types/mdx": "^2.0.0" }, @@ -11313,7 +11305,6 @@ "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.14.tgz", "integrity": "sha512-ilcTH/UniCkMdtexkoCN0bI7pMcJDvmQFPvuPvmEaYA/NSfFTAgdUSLAoVjaRJm7+6PvcM+q1zYOwS4wTYMF9w==", "license": "MIT", - "peer": true, "dependencies": { "csstype": "^3.2.2" } @@ -11510,7 +11501,6 @@ "integrity": "sha512-Jz9ZztpB37dNC+HU2HI28Bs9QXpzCz+y/twHOwhyrIRdbuVDxSytJNDl6z/aAKlaRIwC7y8wJdkBv7FxYGgi0A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@eslint-community/regexpp": "^4.12.2", "@typescript-eslint/scope-manager": "8.56.1", @@ -11550,7 +11540,6 @@ "integrity": "sha512-klQbnPAAiGYFyI02+znpBRLyjL4/BrBd0nyWkdC0s/6xFLkXYQ8OoRrSkqacS1ddVxf/LDyODIKbQ5TgKAf/Fg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.56.1", "@typescript-eslint/types": "8.56.1", @@ -12355,7 +12344,6 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.16.0.tgz", "integrity": "sha512-UVJyE9MttOsBQIDKw1skb9nAwQuR5wuGD3+82K6JgJlm/Y+KI92oNsMNGZCYdDsVtRHSak0pcV5Dno5+4jh9sw==", "license": "MIT", - "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -12432,7 +12420,6 @@ "resolved": "https://registry.npmjs.org/ajv/-/ajv-6.14.0.tgz", "integrity": "sha512-IWrosm/yrn43eiKqkfkHis7QioDleaXQHdDVPKg0FSwwd/DuvyX79TZnFOnYpB7dcsFAMmtFztZuXPDvSePkFw==", "license": "MIT", - "peer": true, "dependencies": { "fast-deep-equal": "^3.1.1", "fast-json-stable-stringify": "^2.0.0", @@ -13481,7 +13468,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "baseline-browser-mapping": "^2.9.0", "caniuse-lite": "^1.0.30001759", @@ -13840,7 +13826,6 @@ "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-3.6.0.tgz", "integrity": "sha512-7VT13fmjotKpGipCW9JEQAusEPE+Ei8nl6/g4FBAmIm0GOOLMua9NDDo/DWp0ZAxCr3cPq5ZpBqmPAQgDda2Pw==", "license": "MIT", - "peer": true, "dependencies": { "anymatch": "~3.1.2", "braces": "~3.0.2", @@ -17382,7 +17367,6 @@ "integrity": "sha512-VmQ+sifHUbI/IcSopBCF/HO3YiHQx/AVd3UVyYL6weuwW+HvON9VYn5l6Zl1WZzPWXPNZrSQpxwkkZ/VuvJZzg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.1", @@ -17570,7 +17554,6 @@ "integrity": "sha512-vPZZsiOKaBAIATpFE2uMI4w5IRwdv/FpQ+qZZMR4E+PeOcM4OeoEbqxRMnywdxP19TyB/3h6QBB0EWon7letSQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/types": "^8.35.0", "comment-parser": "^1.4.1", @@ -21364,7 +21347,6 @@ "integrity": "sha512-F26gjC0yWN8uAA5m5Ss8ZQf5nDHWGlN/xWZIh8S5SRbsEKBovwZhxGd6LJlbZYxBgCYOtreSUyb8hpXyGC5O4A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@jest/core": "30.2.0", "@jest/types": "30.2.0", @@ -22582,7 +22564,6 @@ "resolved": "https://registry.npmjs.org/joi/-/joi-17.13.3.tgz", "integrity": "sha512-otDA4ldcIx+ZXsKHWmp0YizCweVRZG96J10b0FevjfuncLO1oX59THoAmHkNubYJ+9gWsYsp5k8v4ib6oDv1fA==", "license": "BSD-3-Clause", - "peer": true, "dependencies": { "@hapi/hoek": "^9.3.0", "@hapi/topo": "^5.1.0", @@ -22649,7 +22630,6 @@ "integrity": "sha512-Cvc9WUhxSMEo4McES3P7oK3QaXldCfNWp7pl2NNeiIFlCoLr3kfq9kb1fxftiwk1FLV7CvpvDfonxtzUDeSOPg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "cssstyle": "^4.2.1", "data-urls": "^5.0.0", @@ -27563,7 +27543,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "nanoid": "^3.3.11", "picocolors": "^1.1.1", @@ -28960,7 +28939,6 @@ "resolved": "https://registry.npmjs.org/postcss-selector-parser/-/postcss-selector-parser-7.1.1.tgz", "integrity": "sha512-orRsuYpJVw8LdAwqqLykBj9ecS5/cRHlI5+nvTo8LcCKmzDmqVORXtOIYEEQuL9D4BxtA1lm5isAqzQZCoQ6Eg==", "license": "MIT", - "peer": true, "dependencies": { "cssesc": "^3.0.0", "util-deprecate": "^1.0.2" @@ -29538,7 +29516,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.4.tgz", "integrity": "sha512-9nfp2hYpCwOjAN+8TZFGhtWEwgvWHXqESH8qT89AT/lWklpLON22Lc8pEtnpsZz7VmawabSU0gCjnj8aC0euHQ==", "license": "MIT", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -29548,7 +29525,6 @@ "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.2.4.tgz", "integrity": "sha512-AXJdLo8kgMbimY95O2aKQqsz2iWi9jMgKJhRBAxECE4IFxfcazB2LmzloIoibJI3C12IlY20+KFaLv+71bUJeQ==", "license": "MIT", - "peer": true, "dependencies": { "scheduler": "^0.27.0" }, @@ -29593,7 +29569,6 @@ "resolved": "https://registry.npmjs.org/@docusaurus/react-loadable/-/react-loadable-6.0.0.tgz", "integrity": "sha512-YMMxTUQV/QFSnbgrP3tjDzLHRg7vsbMn8e9HAa8o/1iXoiomo48b7sk/kkmWEuWNDPJVlKSJRB6Y2fHqdJk+SQ==", "license": "MIT", - "peer": true, "dependencies": { "@types/react": "*" }, @@ -29622,7 +29597,6 @@ "resolved": "https://registry.npmjs.org/react-router/-/react-router-5.3.4.tgz", "integrity": "sha512-Ys9K+ppnJah3QuaRiLxk+jDWOR1MekYQrlytiXxC1RyfbdsZkS5pvKAzCCr031xHixZwpnsYNT5xysdFHQaYsA==", "license": "MIT", - "peer": true, "dependencies": { "@babel/runtime": "^7.12.13", "history": "^4.9.0", @@ -31149,7 +31123,6 @@ "resolved": "https://registry.npmjs.org/ajv/-/ajv-8.18.0.tgz", "integrity": "sha512-PlXPeEWMXMZ7sPYOHqmDyCJzcfNrUr3fGNKtezX14ykXOEIvyK81d+qydx89KY5O71FKMPaQ2vBfBFI5NHR63A==", "license": "MIT", - "peer": true, "dependencies": { "fast-deep-equal": "^3.1.3", "fast-uri": "^3.0.1", @@ -32402,7 +32375,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "@csstools/css-parser-algorithms": "^3.0.5", "@csstools/css-syntax-patches-for-csstree": "^1.0.19", @@ -33152,7 +33124,6 @@ "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -33389,8 +33360,7 @@ "version": "2.8.1", "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.8.1.tgz", "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", - "license": "0BSD", - "peer": true + "license": "0BSD" }, "node_modules/tsx": { "version": "4.21.0", @@ -33398,7 +33368,6 @@ "integrity": "sha512-5C1sg4USs1lfG0GFb2RLXsdpXqBSEhAaA/0kPL01wxzpMqLILNxIxIOKiILz+cdg/pLnOUxFYOR5yhHU666wbw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "~0.27.0", "get-tsconfig": "^4.7.5" @@ -33593,7 +33562,6 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "devOptional": true, "license": "Apache-2.0", - "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -34270,7 +34238,6 @@ "resolved": "https://registry.npmjs.org/webpack/-/webpack-5.105.4.tgz", "integrity": "sha512-jTywjboN9aHxFlToqb0K0Zs9SbBoW4zRUlGzI2tYNxVYcEi/IPpn+Xi4ye5jTLvX2YeLuic/IvxNot+Q1jMoOw==", "license": "MIT", - "peer": true, "dependencies": { "@types/eslint-scope": "^3.7.7", "@types/estree": "^1.0.8", diff --git a/src/server/plugins/engine/beta/form-context.ts b/src/server/plugins/engine/beta/form-context.ts index 70c41eaad..40b6a4316 100644 --- a/src/server/plugins/engine/beta/form-context.ts +++ b/src/server/plugins/engine/beta/form-context.ts @@ -72,7 +72,8 @@ export async function getFormModel( options.basePath ?? buildBasePath(options.routePrefix ?? '', slug, formState, isPreview), ordnanceSurveyApiKey: options.ordnanceSurveyApiKey, - formId: options.formId ?? metadata.id + formId: options.formId ?? metadata.id, + notificationEmail: metadata.notificationEmail }, services, options.controllers @@ -156,7 +157,7 @@ export async function resolveFormModel( { model: FormModel; updatedAt: Date } > - const cacheKey = `${metadata.id}_${formState}_${isPreview}` + const cacheKey = `${metadata.id}_${formState}_${isPreview}_${metadata.notificationEmail}` let entry = cache.get(cacheKey) if (!entry || !isEqual(entry.updatedAt, stateMetadata.updatedAt)) { @@ -187,7 +188,8 @@ export async function resolveFormModel( options.basePath ?? buildBasePath(routePrefix, slug, formState, isPreview), ordnanceSurveyApiKey: options.ordnanceSurveyApiKey, - formId: options.formId ?? metadata.id + formId: options.formId ?? metadata.id, + notificationEmail: metadata.notificationEmail }, services, options.controllers diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 30f6c2463..b5890c44e 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -92,6 +92,7 @@ export class FormModel { pageMap: Map componentMap: Map + notificationEmail?: string constructor( def: typeof this.def, @@ -99,6 +100,7 @@ export class FormModel { basePath: string ordnanceSurveyApiKey?: string formId?: string + notificationEmail?: string }, services: Services = defaultServices, controllers?: Record @@ -155,6 +157,7 @@ export class FormModel { this.values = result.value this.basePath = options.basePath this.ordnanceSurveyApiKey = options.ordnanceSurveyApiKey + this.notificationEmail = options.notificationEmail this.conditions = {} this.services = services this.controllers = controllers @@ -357,7 +360,8 @@ export class FormModel { componentDefMap: this.componentDefMap, pageMap: this.pageMap, componentMap: this.componentMap, - referenceNumber: getReferenceNumber(state) + referenceNumber: getReferenceNumber(state), + notificationEmail: this.notificationEmail } // Validate current page diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 9d9fecd75..24e8bd7c3 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -199,6 +199,7 @@ export interface FormContext { pageMap: Map componentMap: Map referenceNumber: string + notificationEmail?: string } export type FormContextRequest = ( diff --git a/src/server/plugins/engine/views/partials/preview-banner.html b/src/server/plugins/engine/views/partials/preview-banner.html index 66dfb75e6..bddfd3f19 100644 --- a/src/server/plugins/engine/views/partials/preview-banner.html +++ b/src/server/plugins/engine/views/partials/preview-banner.html @@ -11,10 +11,22 @@ {% call govukNotificationBanner() %} {% if not context.isForceAccess %}

- This is a preview of a {{ previewMode }} form. Do not enter personal information. + This is a preview of a {{ previewMode }} form. Do not enter personal information.

{{ _closeLink() }} + + {% if not context.notificationEmail %} +

+ You can complete this form to test the user journey. The form will appear to submit successfully, but because a destination email address has not been set, the following features will not trigger: +

+
    +
  • email delivery
    • +
  • file uploads
    • +
  • payments
    • +
  • SQS deliveries
    • +
+ {% endif %} {% else %}

This is a preview of a {{ previewMode }} form page you are editing. diff --git a/src/server/plugins/engine/views/partials/preview-banner.test.js b/src/server/plugins/engine/views/partials/preview-banner.test.js index 6bb6153bb..98cc0c5b0 100644 --- a/src/server/plugins/engine/views/partials/preview-banner.test.js +++ b/src/server/plugins/engine/views/partials/preview-banner.test.js @@ -13,7 +13,7 @@ describe('Preview banner partial', () => { let $component = /** @type {HTMLElement | null} */ (null) let $paragraphs = /** @type {HTMLElement[]} */ ([]) - describe('Form preview', () => { + describe('Form preview (no notification email)', () => { beforeEach(() => { const { container } = renderView('partials/preview-banner.html', { context: { @@ -29,6 +29,48 @@ describe('Preview banner partial', () => { $paragraphs = container.getAllByRole('paragraph') }) + it('should render contents', () => { + expect($component).toBeInTheDocument() + expect($component).toContainElement($paragraphs[0]) + expect($component).toHaveClass('govuk-notification-banner') + + expect($paragraphs).toHaveLength(2) + expect($paragraphs[0]).toHaveClass('govuk-notification-banner__heading') + expect($paragraphs[1]).toHaveClass('govuk-body') + }) + + it('should have accessible name', () => { + expect($component).toHaveAccessibleName('Important') + }) + + it('should have text content', () => { + expect($paragraphs[0]).toHaveTextContent( + `This is a preview of a ${status} form. Do not enter personal information.` + ) + + expect($paragraphs[1]).toHaveTextContent( + 'You can complete this form to test the user journey. The form will appear to submit successfully, but because a destination email address has not been set, the following features will not trigger:' + ) + }) + }) + + describe('Form preview (with notification email)', () => { + beforeEach(() => { + const { container } = renderView('partials/preview-banner.html', { + context: { + previewMode: status, + context: { + isForceAccess: false, + notificationEmail: 'test@example.com', + relevantPages: [{ title: 'Page 1' }] + } + } + }) + + $component = container.getByRole('region') + $paragraphs = container.getAllByRole('paragraph') + }) + it('should render contents', () => { expect($component).toBeInTheDocument() expect($component).toContainElement($paragraphs[0]) From e5aa34b18ea044e0b5aeddfb9b0f47dff44455f7 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Thu, 14 May 2026 10:31:14 +0100 Subject: [PATCH 6/6] fix(engine): update cache key in tests to match new format --- src/server/index.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/server/index.test.ts b/src/server/index.test.ts index ebd747845..edb0cb9e3 100644 --- a/src/server/index.test.ts +++ b/src/server/index.test.ts @@ -259,7 +259,9 @@ describe('Model cache', () => { // Assert the live/live cache item has the correct updatedAt timestamp expect( - getCacheItem(`${fixtures.form.metadata.id}_live_false`)?.updatedAt + getCacheItem( + `${fixtures.form.metadata.id}_live_false_${fixtures.form.metadata.notificationEmail}` + )?.updatedAt ).toBe(now2) // Expect the cache size to remain unchanged