From df905244cd03aee3f15b0551346d13bc63521b15 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Wed, 26 Nov 2025 08:53:02 +0000 Subject: [PATCH 01/11] Stash --- .../features/code-based/PRE_POPULATE_STATE.md | 21 ++ package-lock.json | 8 +- package.json | 2 +- .../engine/components/HiddenField.test.ts | 188 ++++++++++++++++++ .../plugins/engine/components/HiddenField.ts | 62 ++++++ .../engine/components/helpers/components.ts | 5 + .../engine/components/helpers/helpers.test.ts | 17 ++ src/server/plugins/engine/components/index.ts | 1 + src/server/plugins/engine/models/FormModel.ts | 3 + .../pageControllers/PageController.test.ts | 17 +- .../engine/pageControllers/PageController.ts | 13 +- .../pageControllers/StatusPageController.ts | 11 +- .../pageControllers/SummaryPageController.ts | 5 +- .../plugins/engine/routes/index.test.ts | 164 +++++++++++++-- src/server/plugins/engine/routes/index.ts | 76 ++++++- .../plugins/engine/services/formsService.js | 10 + .../engine/services/formsService.test.js | 21 ++ .../engine/views/components/hiddenfield.html | 3 + .../plugins/engine/views/confirmation.html | 5 + .../plugins/engine/views/partials/form.html | 10 +- src/server/services/cacheService.ts | 4 +- src/server/types.ts | 1 + src/server/utils/file-form-service.js | 28 ++- src/server/utils/file-form-service.test.js | 114 +++++++++++ 24 files changed, 742 insertions(+), 47 deletions(-) create mode 100644 docs/features/code-based/PRE_POPULATE_STATE.md create mode 100644 src/server/plugins/engine/components/HiddenField.test.ts create mode 100644 src/server/plugins/engine/components/HiddenField.ts create mode 100644 src/server/plugins/engine/services/formsService.test.js create mode 100644 src/server/plugins/engine/views/components/hiddenfield.html create mode 100644 src/server/utils/file-form-service.test.js diff --git a/docs/features/code-based/PRE_POPULATE_STATE.md b/docs/features/code-based/PRE_POPULATE_STATE.md new file mode 100644 index 000000000..658e1afcc --- /dev/null +++ b/docs/features/code-based/PRE_POPULATE_STATE.md @@ -0,0 +1,21 @@ +--- +layout: default +title: Pre-populate state +parent: Code-based Features +grand_parent: Features +render_with_liquid: false +--- + +# Pre-populate state + +The forms engine supports the ability to pre-populate form state using query string parameters. This feature enables applications to support passing specific parameter values through the form and on to the submission without the user having to enter these values. + +The feature uses the HiddenField component to prevent against rogue state injection. Only query string parameters whose names exist as HiddenField components will be copied into state. + +The parameter values get copied on first load of the form, and are simple key/value parameters e.g.: + +``` +?paramname1=paramval1,paramname2=paramname2 +``` + +There is no limit set on the number of parameters. The keys and values get copied as-is (no case changes get applied). diff --git a/package-lock.json b/package-lock.json index 91ce2ce21..2f20fec34 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "hasInstallScript": true, "license": "SEE LICENSE IN LICENSE", "dependencies": { - "@defra/forms-model": "^3.0.584", + "@defra/forms-model": "^3.0.585", "@defra/hapi-tracing": "^1.29.0", "@elastic/ecs-pino-format": "^1.5.0", "@hapi/boom": "^10.0.1", @@ -2218,9 +2218,9 @@ } }, "node_modules/@defra/forms-model": { - "version": "3.0.584", - "resolved": "https://registry.npmjs.org/@defra/forms-model/-/forms-model-3.0.584.tgz", - "integrity": "sha512-hghONjBY8ho7Z7I/QRwYT70Ot5BStPO5rbtaSsGMMhjOsiXM93+jGmclr8rw1A3w5DTiqmaagbtX1lgDojcSjg==", + "version": "3.0.585", + "resolved": "https://registry.npmjs.org/@defra/forms-model/-/forms-model-3.0.585.tgz", + "integrity": "sha512-lSJzQu0xTk+VqSjLcjt7zwPS88J7MVYl5kdKfKCQsqbnVTmBHi9a3MHvCa6xlZRu1GZgoEjwQWY+QKzTKfN8FA==", "license": "OGL-UK-3.0", "dependencies": { "@joi/date": "^2.1.1", diff --git a/package.json b/package.json index db91ab68a..7f9e012b0 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ }, "license": "SEE LICENSE IN LICENSE", "dependencies": { - "@defra/forms-model": "^3.0.584", + "@defra/forms-model": "^3.0.585", "@defra/hapi-tracing": "^1.29.0", "@elastic/ecs-pino-format": "^1.5.0", "@hapi/boom": "^10.0.1", diff --git a/src/server/plugins/engine/components/HiddenField.test.ts b/src/server/plugins/engine/components/HiddenField.test.ts new file mode 100644 index 000000000..bf02c3802 --- /dev/null +++ b/src/server/plugins/engine/components/HiddenField.test.ts @@ -0,0 +1,188 @@ +import { ComponentType, type HiddenFieldComponent } from '@defra/forms-model' + +import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' +import { + getAnswer, + type Field +} from '~/src/server/plugins/engine/components/helpers/components.js' +import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import definition from '~/test/form/definitions/blank.js' +import { getFormData, getFormState } from '~/test/helpers/component-helpers.js' + +describe('HiddenField', () => { + let model: FormModel + + beforeEach(() => { + model = new FormModel(definition, { + basePath: 'test' + }) + }) + + describe('Defaults', () => { + let def: HiddenFieldComponent + let collection: ComponentCollection + let field: Field + + beforeEach(() => { + def = { + title: 'Hidden field', + name: 'myComponent', + type: ComponentType.HiddenField, + options: {} + } satisfies HiddenFieldComponent + + collection = new ComponentCollection([def], { model }) + field = collection.fields[0] + }) + + describe('Schema', () => { + it('uses component title as label as default', () => { + const { formSchema } = collection + const { keys } = formSchema.describe() + + expect(keys).toHaveProperty( + 'myComponent', + expect.objectContaining({ + flags: expect.objectContaining({ + label: 'Hidden field' + }) + }) + ) + }) + + it('uses component name as keys', () => { + const { formSchema } = collection + const { keys } = formSchema.describe() + + expect(field.keys).toEqual(['myComponent']) + expect(field.collection).toBeUndefined() + + for (const key of field.keys) { + expect(keys).toHaveProperty(key) + } + }) + + it('is required by default', () => { + const { formSchema } = collection + const { keys } = formSchema.describe() + + expect(keys).toHaveProperty( + 'myComponent', + expect.objectContaining({ + flags: expect.objectContaining({ + presence: 'required' + }) + }) + ) + }) + it('accepts valid values', () => { + const result1 = collection.validate(getFormData('Hidden value')) + const result2 = collection.validate(getFormData('Hidden value 2')) + + expect(result1.errors).toBeUndefined() + expect(result2.errors).toBeUndefined() + }) + + it('adds errors for empty value', () => { + const result = collection.validate(getFormData('')) + + expect(result.errors).toEqual([ + expect.objectContaining({ + text: 'Enter hidden field' + }) + ]) + }) + + it('adds errors for invalid values', () => { + const result1 = collection.validate(getFormData(['invalid'])) + const result2 = collection.validate( + // @ts-expect-error - Allow invalid param for test + getFormData({ unknown: 'invalid' }) + ) + + expect(result1.errors).toBeTruthy() + expect(result2.errors).toBeTruthy() + }) + }) + + describe('State', () => { + it('returns text from state', () => { + const state1 = getFormState('Hidden field') + const state2 = getFormState(null) + + const answer1 = getAnswer(field, state1) + const answer2 = getAnswer(field, state2) + + expect(answer1).toBe('Hidden field') + expect(answer2).toBe('') + }) + + it('returns payload from state', () => { + const state1 = getFormState('Hidden field') + const state2 = getFormState(null) + + const payload1 = field.getFormDataFromState(state1) + const payload2 = field.getFormDataFromState(state2) + + expect(payload1).toEqual(getFormData('Hidden field')) + expect(payload2).toEqual(getFormData()) + }) + + it('returns value from state', () => { + const state1 = getFormState('Hidden field') + const state2 = getFormState(null) + + const value1 = field.getFormValueFromState(state1) + const value2 = field.getFormValueFromState(state2) + + expect(value1).toBe('Hidden field') + expect(value2).toBeUndefined() + }) + + it('returns context for conditions and form submission', () => { + const state1 = getFormState('Hidden field') + const state2 = getFormState(null) + + const value1 = field.getContextValueFromState(state1) + const value2 = field.getContextValueFromState(state2) + + expect(value1).toBe('Hidden field') + expect(value2).toBeNull() + }) + + it('returns state from payload', () => { + const payload1 = getFormData('Hidden field') + const payload2 = getFormData() + + const value1 = field.getStateFromValidForm(payload1) + const value2 = field.getStateFromValidForm(payload2) + + expect(value1).toEqual(getFormState('Hidden field')) + expect(value2).toEqual(getFormState(null)) + }) + }) + + describe('View model', () => { + it('sets Nunjucks component defaults', () => { + const viewModel = field.getViewModel(getFormData('Hidden field')) + + expect(viewModel).toEqual( + expect.objectContaining({ + label: { text: def.title }, + name: 'myComponent', + id: 'myComponent', + value: 'Hidden field' + }) + ) + }) + }) + + describe('AllPossibleErrors', () => { + it('should return errors', () => { + const errors = field.getAllPossibleErrors() + expect(errors.baseErrors).not.toBeEmpty() + expect(errors.advancedSettingsErrors).toBeEmpty() + }) + }) + }) +}) diff --git a/src/server/plugins/engine/components/HiddenField.ts b/src/server/plugins/engine/components/HiddenField.ts new file mode 100644 index 000000000..227652ab8 --- /dev/null +++ b/src/server/plugins/engine/components/HiddenField.ts @@ -0,0 +1,62 @@ +import { + type HiddenFieldComponent, + type TextFieldComponent +} from '@defra/forms-model' +import joi, { type StringSchema } from 'joi' + +import { FormComponent } from '~/src/server/plugins/engine/components/FormComponent.js' +import { TextField } from '~/src/server/plugins/engine/components/TextField.js' +import { messageTemplate } from '~/src/server/plugins/engine/pageControllers/validationOptions.js' +import { + type ErrorMessageTemplateList, + type FormState, + type FormStateValue, + type FormSubmissionState +} from '~/src/server/plugins/engine/types.js' + +export class HiddenField extends FormComponent { + declare formSchema: StringSchema + declare stateSchema: StringSchema + declare schema: TextFieldComponent['schema'] + declare options: TextFieldComponent['options'] + + constructor( + def: HiddenFieldComponent, + props: ConstructorParameters[1] + ) { + super(def, props) + + const formSchema = joi.string().trim().label(this.label).required() + + this.formSchema = formSchema.default('') + this.stateSchema = formSchema.default(null).allow(null) + this.schema = {} + this.options = {} + } + + getFormValueFromState(state: FormSubmissionState) { + const { name } = this + return this.getFormValue(state[name]) + } + + isValue(value?: FormStateValue | FormState): value is string { + return TextField.isText(value) + } + + /** + * For error preview page that shows all possible errors on a component + */ + getAllPossibleErrors(): ErrorMessageTemplateList { + return HiddenField.getAllPossibleErrors() + } + + /** + * Static version of getAllPossibleErrors that doesn't require a component instance. + */ + static getAllPossibleErrors(): ErrorMessageTemplateList { + return { + baseErrors: [{ type: 'required', template: messageTemplate.required }], + advancedSettingsErrors: [] + } + } +} diff --git a/src/server/plugins/engine/components/helpers/components.ts b/src/server/plugins/engine/components/helpers/components.ts index 880df9812..d307e6517 100644 --- a/src/server/plugins/engine/components/helpers/components.ts +++ b/src/server/plugins/engine/components/helpers/components.ts @@ -34,6 +34,7 @@ export type Field = InstanceType< | typeof Components.TextField | typeof Components.UkAddressField | typeof Components.FileUploadField + | typeof Components.HiddenField > // Guidance component instances only @@ -186,6 +187,10 @@ export function createComponent( case ComponentType.LatLongField: component = new Components.LatLongField(def, options) break + + case ComponentType.HiddenField: + component = new Components.HiddenField(def, options) + break } if (typeof component === 'undefined') { diff --git a/src/server/plugins/engine/components/helpers/helpers.test.ts b/src/server/plugins/engine/components/helpers/helpers.test.ts index de49574a3..e597fa0d1 100644 --- a/src/server/plugins/engine/components/helpers/helpers.test.ts +++ b/src/server/plugins/engine/components/helpers/helpers.test.ts @@ -2,6 +2,7 @@ import { ComponentType, type ComponentDef } from '@defra/forms-model' import { ComponentBase } from '~/src/server/plugins/engine/components/ComponentBase.js' import { EastingNorthingField } from '~/src/server/plugins/engine/components/EastingNorthingField.js' +import { HiddenField } from '~/src/server/plugins/engine/components/HiddenField.js' import { LatLongField } from '~/src/server/plugins/engine/components/LatLongField.js' import { NationalGridFieldNumberField } from '~/src/server/plugins/engine/components/NationalGridFieldNumberField.js' import { OsGridRefField } from '~/src/server/plugins/engine/components/OsGridRefField.js' @@ -96,6 +97,22 @@ describe('helpers tests', () => { expect(component.name).toBe('testField') expect(component.title).toBe('Test National Grid') }) + + test('should create HiddenField component', () => { + const component = createComponent( + { + type: ComponentType.HiddenField, + name: 'hiddenField', + title: 'Hidden field', + options: {} + }, + { model: formModel } + ) + + expect(component).toBeInstanceOf(HiddenField) + expect(component.name).toBe('hiddenField') + expect(component.title).toBe('Hidden field') + }) }) describe('ComponentBase tests', () => { diff --git a/src/server/plugins/engine/components/index.ts b/src/server/plugins/engine/components/index.ts index 43c342e26..da04c5a62 100644 --- a/src/server/plugins/engine/components/index.ts +++ b/src/server/plugins/engine/components/index.ts @@ -28,3 +28,4 @@ export { EastingNorthingField } from '~/src/server/plugins/engine/components/Eas export { OsGridRefField } from '~/src/server/plugins/engine/components/OsGridRefField.js' export { NationalGridFieldNumberField } from '~/src/server/plugins/engine/components/NationalGridFieldNumberField.js' export { LatLongField } from '~/src/server/plugins/engine/components/LatLongField.js' +export { HiddenField } from '~/src/server/plugins/engine/components/HiddenField.js' diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 936aa6568..42cfcad80 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -74,6 +74,7 @@ export class FormModel { lists: FormDefinition['lists'] sections: FormDefinition['sections'] = [] name: string + formId: string values: FormDefinition basePath: string versionNumber?: number @@ -100,6 +101,7 @@ export class FormModel { basePath: string versionNumber?: number ordnanceSurveyApiKey?: string + formId?: string }, services: Services = defaultServices, controllers?: Record @@ -152,6 +154,7 @@ export class FormModel { this.lists = def.lists this.sections = def.sections this.name = def.name ?? '' + this.formId = options.formId ?? '' this.values = result.value this.basePath = options.basePath this.versionNumber = options.versionNumber diff --git a/src/server/plugins/engine/pageControllers/PageController.test.ts b/src/server/plugins/engine/pageControllers/PageController.test.ts index 10d5c52c8..cd3491d40 100644 --- a/src/server/plugins/engine/pageControllers/PageController.test.ts +++ b/src/server/plugins/engine/pageControllers/PageController.test.ts @@ -24,7 +24,8 @@ describe('PageController', () => { const page2 = pages[1] model = new FormModel(definition, { - basePath: testBasePath + basePath: testBasePath, + formId: 'form-id' }) controller1 = new PageController(model, page1) @@ -61,23 +62,15 @@ describe('PageController', () => { }) }) - it('returns feedback link (from form definition)', () => { - expect(controller1).toHaveProperty('feedbackLink', undefined) - - const emailAddress = 'test@feedback.cat' - - model.def.feedback = { - emailAddress - } - + it('returns feedback link default', () => { expect(controller1).toHaveProperty( 'feedbackLink', - `mailto:${emailAddress}` + '/form/csat?formId=form-id' ) }) it('returns phase tag (from form definition)', () => { - expect(controller1).toHaveProperty('phaseTag', undefined) + expect(controller1).toHaveProperty('phaseTag', 'beta') model.def.phaseBanner = { phase: 'alpha' diff --git a/src/server/plugins/engine/pageControllers/PageController.ts b/src/server/plugins/engine/pageControllers/PageController.ts index 0ba533749..8f99aa74e 100644 --- a/src/server/plugins/engine/pageControllers/PageController.ts +++ b/src/server/plugins/engine/pageControllers/PageController.ts @@ -8,9 +8,9 @@ import { import Boom from '@hapi/boom' import { type Lifecycle, type RouteOptions, type Server } from '@hapi/hapi' +import { config } from '~/src/config/index.js' import { type ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' import { - encodeUrl, getSaveAndExitHelpers, getStartPath, normalisePath @@ -119,19 +119,12 @@ export class PageController { } get feedbackLink() { - const { def } = this - - // setting the feedbackLink to undefined here for feedback forms prevents the feedback link from being shown - const feedbackLink = def.feedback?.emailAddress - ? `mailto:${def.feedback.emailAddress}` - : def.feedback?.url - - return encodeUrl(feedbackLink) + return `/form/csat?formId=${this.model.formId}` } get phaseTag() { const { def } = this - return def.phaseBanner?.phase + return def.phaseBanner?.phase ?? config.get('phaseTag') } getHref(path: string): string { diff --git a/src/server/plugins/engine/pageControllers/StatusPageController.ts b/src/server/plugins/engine/pageControllers/StatusPageController.ts index 9f2f72d7e..08a57fac5 100644 --- a/src/server/plugins/engine/pageControllers/StatusPageController.ts +++ b/src/server/plugins/engine/pageControllers/StatusPageController.ts @@ -41,13 +41,20 @@ export class StatusPageController extends QuestionPageController { const slug = request.params.slug const { formsService } = this.model.services - const { getFormMetadata } = formsService + const { getFormMetadata, getFormMetadataById } = formsService const { submissionGuidance } = await getFormMetadata(slug) + // Re-read form name if overriding display (for example, in a feedback form) + const storedFormId = confirmationState.formId + const formName = storedFormId + ? (await getFormMetadataById(storedFormId)).title + : undefined + return h.view(viewName, { ...viewModel, - submissionGuidance + submissionGuidance, + formName }) } } diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index aac9d3b55..0da4396eb 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -152,7 +152,10 @@ export class SummaryPageController extends QuestionPageController { ) } - await cacheService.setConfirmationState(request, { confirmed: true }) + await cacheService.setConfirmationState(request, { + confirmed: true, + formId: context.state.formId as string | undefined + }) // Clear all form data await cacheService.clearState(request) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 45cb49128..a5b0f01ab 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -1,3 +1,4 @@ +import { ComponentType, type Page } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' @@ -9,15 +10,40 @@ import { } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' -import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' +import { + prefillStateFromQueryParameters, + redirectOrMakeHandler +} from '~/src/server/plugins/engine/routes/index.js' import { type AnyFormRequest, type OnRequestCallback } from '~/src/server/plugins/engine/types.js' import { type FormResponseToolkit } from '~/src/server/routes/types.js' +import { type FormsService, type Services } from '~/src/server/types.js' jest.mock('~/src/server/plugins/engine/helpers') +function buildMockModel( + pagesOverride = [] as Page[], + pagesControllerOverride = [] as PageControllerClass[], + servicesOverride = {} as Services +) { + return { + def: { + metadata: { + submission: { code: 'TEST-CODE' } + } as { submission: { code: string } }, + pages: pagesOverride + }, + getFormContext: jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} + }), + pages: pagesControllerOverride, + services: servicesOverride + } as unknown as FormModel +} + describe('redirectOrMakeHandler', () => { const mockServer = {} as unknown as Parameters< typeof redirectOrMakeHandler @@ -38,17 +64,7 @@ describe('redirectOrMakeHandler', () => { let mockPage: PageControllerClass - const mockModel: FormModel = { - def: { - metadata: { - submission: { code: 'TEST-CODE' } - } as { submission: { code: string } } - }, - getFormContext: jest.fn().mockReturnValue({ - isForceAccess: false, - data: {} - }) - } as unknown as FormModel + const mockModel = buildMockModel() const mockMakeHandler = jest .fn() @@ -314,4 +330,128 @@ describe('redirectOrMakeHandler', () => { expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') }) }) + + describe('prefillStateFromQueryParameters', () => { + const mockGetState = jest.fn() + const mockMergeState = jest.fn() + const mockRequestPrefill: AnyFormRequest = { + server: mockServer, + app: {}, + yar: { flash: () => [] }, + params: { path: 'test-path' }, + query: {} + } as unknown as AnyFormRequest + + it('should not add any state if no params', async () => { + const mockModelPrefill = buildMockModel( + [], + [ + { + getState: mockGetState, + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + await prefillStateFromQueryParameters( + mockRequestPrefill, + mockModelPrefill + ) + expect(mockMergeState).not.toHaveBeenCalled() + }) + + it('should only add state where param names match hidden field names', async () => { + const mockRequest2 = { + ...mockRequest, + query: { + param1: 'val1', + param2: 'val2', + param3: 'val3', + param4: 'val4' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'param2' + }, + { + type: ComponentType.HiddenField, + name: 'param4' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + await prefillStateFromQueryParameters(mockRequest2, mockModel) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + { + param2: 'val2', + param4: 'val4' + } + ) + }) + + it('should call lookup function for formId', async () => { + const mockRequest3 = { + ...mockRequest, + query: { + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'formId' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ], + { + formsService: { + getFormMetadata: jest.fn(), + getFormMetadataById: jest + .fn() + .mockResolvedValue({ title: 'My looked-up form name' }), + getFormDefinition: jest.fn() + } as unknown as FormsService + } as Services + ) + + await prefillStateFromQueryParameters(mockRequest3, mockModel) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + { + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', + formName: 'My looked-up form name' + } + ) + }) + }) }) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 9cb681c2c..8e9ccd5e2 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -1,3 +1,4 @@ +import { getHiddenFields } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, @@ -33,6 +34,7 @@ import { type ExternalStateAppendage, type FormContext, type FormPayload, + type FormStateValue, type FormSubmissionState, type OnRequestCallback, type PluginOptions @@ -41,6 +43,7 @@ import { type FormRequest, type FormResponseToolkit } from '~/src/server/routes/types.js' +import { type Services } from '~/src/server/types.js' export async function redirectOrMakeHandler( request: AnyFormRequest, @@ -108,6 +111,71 @@ export async function redirectOrMakeHandler( return proceed(request, h, page.getHref(relevantPath)) } +/** + * A series of functions that can transform a pre-fill input parameter e.g lookup a form title based on form id + */ +const paramLookupFunctions = { + formId: async (val: string, services: Services) => { + const meta = await services.formsService.getFormMetadataById(val) + return { + key: 'formName', + value: meta.title + } + } +} as Partial< + Record< + string, + ( + val: string, + services: Services + ) => Promise<{ key: string; value: string | undefined }> + > +> + +/** + * Any hidden parameters defined in the FormDefinition may be pre-filled by URL parameter values. + * Other parameters are ignored for security reasons. + * @param request + * @param model + */ +export async function prefillStateFromQueryParameters( + request: AnyFormRequest, + model: FormModel +): Promise { + const hiddenFieldNames = new Set( + getHiddenFields(model.def).map((field) => field.name) + ) + const query = Object.keys(request.query).length ? request.query : undefined + + if (!query) { + return + } + + const params = {} as Record + + for (const [key, value = ''] of Object.entries(query)) { + if (hiddenFieldNames.has(key)) { + const lookupFunc = paramLookupFunctions[key] + if (lookupFunc) { + const res = await lookupFunc(value, model.services) + // Store original value and result + params[key] = value + params[res.key] = res.value + } else { + params[key] = value + } + } + } + + if (!Object.keys(params).length) { + return + } + + const page = model.pages[0] // Any page will do so just take the first one + const formData = await page.getState(request) + await page.mergeState(request, formData, params) +} + async function importExternalComponentState( request: AnyFormRequest, page: PageControllerClass, @@ -185,6 +253,9 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { if (server.app.model) { request.app.model = server.app.model + // Copy any URL params into the form state + await prefillStateFromQueryParameters(request, request.app.model) + return h.continue } @@ -244,7 +315,7 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { // Construct the form model const model = new FormModel( definition, - { basePath, versionNumber, ordnanceSurveyApiKey }, + { basePath, versionNumber, ordnanceSurveyApiKey, formId: id }, services, controllers ) @@ -254,6 +325,9 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { server.app.models.set(key, item) } + // Copy any URL params into the form state + await prefillStateFromQueryParameters(request, item.model) + // Assign the model to the request data // for use in the downstream handler request.app.model = item.model diff --git a/src/server/plugins/engine/services/formsService.js b/src/server/plugins/engine/services/formsService.js index cb766daa7..79a538d2e 100644 --- a/src/server/plugins/engine/services/formsService.js +++ b/src/server/plugins/engine/services/formsService.js @@ -12,6 +12,16 @@ export function getFormMetadata(_slug) { throw error } +// eslint-disable-next-line jsdoc/require-returns-check +/** + * Dummy function to get form metadata. + * @param {string} _id - the id of the form + * @returns {Promise} + */ +export function getFormMetadataById(_id) { + throw error +} + // eslint-disable-next-line jsdoc/require-returns-check /** * Dummy function to get form metadata. diff --git a/src/server/plugins/engine/services/formsService.test.js b/src/server/plugins/engine/services/formsService.test.js new file mode 100644 index 000000000..e7ae06ccc --- /dev/null +++ b/src/server/plugins/engine/services/formsService.test.js @@ -0,0 +1,21 @@ +import { FormStatus } from '@defra/forms-model' + +import { + getFormDefinition, + getFormMetadata, + getFormMetadataById +} from '~/src/server/plugins/engine/services/formsService.js' + +describe('formsService', () => { + it('getFormMetadata should throw error', () => { + expect(() => getFormMetadata('slug')).toThrow() + }) + + it('getFormMetadataById should throw error', () => { + expect(() => getFormMetadataById('id')).toThrow() + }) + + it('getFormDefinition should throw error', () => { + expect(() => getFormDefinition('id', FormStatus.Draft)).toThrow() + }) +}) diff --git a/src/server/plugins/engine/views/components/hiddenfield.html b/src/server/plugins/engine/views/components/hiddenfield.html new file mode 100644 index 000000000..88f5f864f --- /dev/null +++ b/src/server/plugins/engine/views/components/hiddenfield.html @@ -0,0 +1,3 @@ +{% macro HiddenField(component) %} + +{% endmacro %} diff --git a/src/server/plugins/engine/views/confirmation.html b/src/server/plugins/engine/views/confirmation.html index 82b8c3cbf..bcdc50720 100644 --- a/src/server/plugins/engine/views/confirmation.html +++ b/src/server/plugins/engine/views/confirmation.html @@ -14,6 +14,11 @@

What happens next

{{ submissionGuidance | markdown(3) | safe }}
+ {% if feedbackLink %} +

+ What do you think of this service? (takes 30 seconds) +

+ {% endif %} {% endblock %} diff --git a/src/server/plugins/engine/views/partials/form.html b/src/server/plugins/engine/views/partials/form.html index c7a68d32e..a2732ba39 100644 --- a/src/server/plugins/engine/views/partials/form.html +++ b/src/server/plugins/engine/views/partials/form.html @@ -6,9 +6,17 @@ {{ componentList(components) }} + {% if isStartPage %} + {% set buttonText = "Start now" %} + {% elif submitButtonText %} + {% set buttonText = submitButtonText %} + {% else %} + {% set buttonText = "Continue" %} + {% endif %} +
{{ govukButton({ - text: "Start now" if isStartPage else "Continue", + text: buttonText, isStartButton: isStartPage, preventDoubleClick: true }) }} diff --git a/src/server/services/cacheService.ts b/src/server/services/cacheService.ts index 42d5c818d..63ac6b08c 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -55,7 +55,7 @@ export class CacheService { async getConfirmationState( request: AnyFormRequest - ): Promise<{ confirmed?: true }> { + ): Promise<{ confirmed?: true; formId?: string }> { const key = this.Key(request, ADDITIONAL_IDENTIFIER.Confirmation) const value = await this.cache.get(key) @@ -64,7 +64,7 @@ export class CacheService { async setConfirmationState( request: AnyFormRequest, - confirmationState: { confirmed?: true } + confirmationState: { confirmed?: true; formId?: string } ) { const key = this.Key(request, ADDITIONAL_IDENTIFIER.Confirmation) const ttl = config.get('confirmationSessionTimeout') diff --git a/src/server/types.ts b/src/server/types.ts index b20881f28..87823c0cd 100644 --- a/src/server/types.ts +++ b/src/server/types.ts @@ -23,6 +23,7 @@ import { type CacheService } from '~/src/server/services/cacheService.js' export interface FormsService { getFormMetadata: (slug: string) => Promise + getFormMetadataById: (id: string) => Promise getFormDefinition: ( id: string, state: FormStatus diff --git a/src/server/utils/file-form-service.js b/src/server/utils/file-form-service.js index f8306bc4d..6e5401f9b 100644 --- a/src/server/utils/file-form-service.js +++ b/src/server/utils/file-form-service.js @@ -97,6 +97,23 @@ export class FileFormService { return metadata } + /** + * Get the form metadata by form id + * @param {string} id - the form id + * @returns {FormMetadata} + */ + getFormMetadataById(id) { + const metadata = Array.from(this.#metadata.values()).find( + (form) => form.id === id + ) + + if (!metadata) { + throw new Error(`Form metadata id '${id}' not found`) + } + + return metadata + } + /** * Get the form defintion by id * @param {string} id - the form id @@ -127,6 +144,15 @@ export class FileFormService { return Promise.resolve(this.getFormMetadata(slug)) }, + /** + * Get the form metadata by form id + * @param {string} id + * @returns {Promise} + */ + getFormMetadataById: (id) => { + return Promise.resolve(this.getFormMetadataById(id)) + }, + /** * Get the form defintion by id * @param {string} id @@ -140,5 +166,5 @@ export class FileFormService { } /** - * @import { FormMetadata, FormDefinition } from '@defra/forms-model' + * @import { FormMetadata, FormDefinition, FormStatus } from '@defra/forms-model' */ diff --git a/src/server/utils/file-form-service.test.js b/src/server/utils/file-form-service.test.js new file mode 100644 index 000000000..266a00288 --- /dev/null +++ b/src/server/utils/file-form-service.test.js @@ -0,0 +1,114 @@ +import { join } from 'node:path' + +import { FormStatus } from '~/src/server/routes/types.js' +import { FileFormService } from '~/src/server/utils/file-form-service.js' + +describe('File-form-service', () => { + /** @type {FileFormService} */ + let service + beforeEach(async () => { + const now = new Date() + const user = { id: 'user', displayName: 'Username' } + const author = { + createdAt: now, + createdBy: user, + updatedAt: now, + updatedBy: user + } + service = new FileFormService() + const metadata = { + organisation: 'Defra', + teamName: 'Team name', + teamEmail: 'team@defra.gov.uk', + submissionGuidance: "Thanks for your submission, we'll be in touch", + notificationEmail: 'email@domain.com', + ...author, + live: author + } + await service.addForm( + `${join(import.meta.dirname, '../../../test/form/definitions')}/components.json`, + { + ...metadata, + id: '95e92559-968d-44ae-8666-2b1ad3dffd31', + title: 'Form test', + slug: 'form-test' + } + ) + }) + + describe('metadata by slug', () => { + it('should get form metadata by slug', () => { + const meta = service.getFormMetadata('form-test') + expect(meta.id).toBe('95e92559-968d-44ae-8666-2b1ad3dffd31') + expect(meta.title).toBe('Form test') + }) + + it('should throw if not found', () => { + expect(() => service.getFormMetadata('form-test-missing')).toThrow( + "Form metadata 'form-test-missing' not found" + ) + }) + }) + + describe('metadata by id', () => { + it('should get form metadata by id', () => { + const meta = service.getFormMetadataById( + '95e92559-968d-44ae-8666-2b1ad3dffd31' + ) + expect(meta.id).toBe('95e92559-968d-44ae-8666-2b1ad3dffd31') + expect(meta.title).toBe('Form test') + }) + + it('should throw if not found', () => { + expect(() => service.getFormMetadataById('id-missing')).toThrow( + "Form metadata id 'id-missing' not found" + ) + }) + }) + + describe('definition by id', () => { + it('should get form definition by id', () => { + const form = service.getFormDefinition( + '95e92559-968d-44ae-8666-2b1ad3dffd31' + ) + expect(form.name).toBe('All components') + expect(form.startPage).toBe('/all-components') + }) + + it('should throw if not found', () => { + expect(() => service.getFormDefinition('id-missing')).toThrow( + "Form definition 'id-missing' not found" + ) + }) + }) + + describe('toFormsService', () => { + it('should create interface', async () => { + const interfaceImpl = service.toFormsService() + const res1 = await interfaceImpl.getFormMetadata('form-test') + expect(res1.id).toBe('95e92559-968d-44ae-8666-2b1ad3dffd31') + expect(res1.title).toBe('Form test') + + const res2 = await interfaceImpl.getFormMetadataById( + '95e92559-968d-44ae-8666-2b1ad3dffd31' + ) + expect(res2.id).toBe('95e92559-968d-44ae-8666-2b1ad3dffd31') + expect(res2.title).toBe('Form test') + + const res3 = await interfaceImpl.getFormDefinition( + '95e92559-968d-44ae-8666-2b1ad3dffd31', + FormStatus.Draft + ) + expect(res3?.name).toBe('All components') + expect(res3?.startPage).toBe('/all-components') + }) + }) + + describe('readForm', () => { + it('should throw if invalid extension', async () => { + await expect( + service.readForm('/some-folder/some-file.bad') + ).rejects.toThrow("Invalid file extension '.bad'") + }) + }) +}) From b91fb95d1ef3f72589b9e851eaa9a99e0a9dafc3 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Wed, 26 Nov 2025 10:23:57 +0000 Subject: [PATCH 02/11] Allows HiddenField to be optional --- src/server/plugins/engine/components/HiddenField.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/server/plugins/engine/components/HiddenField.ts b/src/server/plugins/engine/components/HiddenField.ts index 227652ab8..4d6f539ad 100644 --- a/src/server/plugins/engine/components/HiddenField.ts +++ b/src/server/plugins/engine/components/HiddenField.ts @@ -26,7 +26,13 @@ export class HiddenField extends FormComponent { ) { super(def, props) - const formSchema = joi.string().trim().label(this.label).required() + const { options } = def + + let formSchema = joi.string().trim().label(this.label).required() + + if (options.required === false) { + formSchema = formSchema.allow('') + } this.formSchema = formSchema.default('') this.stateSchema = formSchema.default(null).allow(null) From 0425bf51918d5b35986d498983baacab39c35d79 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Wed, 26 Nov 2025 11:21:27 +0000 Subject: [PATCH 03/11] Handles missing formId value --- src/server/plugins/engine/routes/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 8e9ccd5e2..48f266923 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -116,10 +116,14 @@ export async function redirectOrMakeHandler( */ const paramLookupFunctions = { formId: async (val: string, services: Services) => { - const meta = await services.formsService.getFormMetadataById(val) + let formTitle = 'Submit a form to Defra' + if (val) { + const meta = await services.formsService.getFormMetadataById(val) + formTitle = meta.title + } return { key: 'formName', - value: meta.title + value: formTitle } } } as Partial< From 8f2926ca2ea89e925f77413ac0cfb495293c44b7 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Wed, 26 Nov 2025 11:23:21 +0000 Subject: [PATCH 04/11] Tidy --- src/server/plugins/engine/routes/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 48f266923..b1ddbd5fb 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -116,7 +116,7 @@ export async function redirectOrMakeHandler( */ const paramLookupFunctions = { formId: async (val: string, services: Services) => { - let formTitle = 'Submit a form to Defra' + let formTitle if (val) { const meta = await services.formsService.getFormMetadataById(val) formTitle = meta.title From 6772b6f0c3bd96bb5821de97a211597ee1c91624 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Wed, 26 Nov 2025 11:55:38 +0000 Subject: [PATCH 05/11] Revert phaseTag override --- .../plugins/engine/pageControllers/PageController.test.ts | 2 +- src/server/plugins/engine/pageControllers/PageController.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/PageController.test.ts b/src/server/plugins/engine/pageControllers/PageController.test.ts index cd3491d40..a7de8515b 100644 --- a/src/server/plugins/engine/pageControllers/PageController.test.ts +++ b/src/server/plugins/engine/pageControllers/PageController.test.ts @@ -70,7 +70,7 @@ describe('PageController', () => { }) it('returns phase tag (from form definition)', () => { - expect(controller1).toHaveProperty('phaseTag', 'beta') + expect(controller1).toHaveProperty('phaseTag', undefined) model.def.phaseBanner = { phase: 'alpha' diff --git a/src/server/plugins/engine/pageControllers/PageController.ts b/src/server/plugins/engine/pageControllers/PageController.ts index 8f99aa74e..014eb4ef6 100644 --- a/src/server/plugins/engine/pageControllers/PageController.ts +++ b/src/server/plugins/engine/pageControllers/PageController.ts @@ -8,7 +8,6 @@ import { import Boom from '@hapi/boom' import { type Lifecycle, type RouteOptions, type Server } from '@hapi/hapi' -import { config } from '~/src/config/index.js' import { type ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' import { getSaveAndExitHelpers, @@ -124,7 +123,7 @@ export class PageController { get phaseTag() { const { def } = this - return def.phaseBanner?.phase ?? config.get('phaseTag') + return def.phaseBanner?.phase } getHref(path: string): string { From 169a1260732e09dfd07b3482c1bedefcae6441c7 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 14:25:35 +0000 Subject: [PATCH 06/11] Stash - to compare --- .../features/code-based/PRE_POPULATE_STATE.md | 2 +- src/server/plugins/engine/helpers.ts | 3 +- .../pageControllers/PageController.test.ts | 2 +- .../engine/pageControllers/PageController.ts | 2 +- .../pageControllers/QuestionPageController.ts | 9 +- .../pageControllers/SummaryPageController.ts | 5 +- .../pageControllers/helpers/state.test.ts | 180 ++++++++++++++++++ .../engine/pageControllers/helpers/state.ts | 99 ++++++++++ .../plugins/engine/routes/index.test.ts | 133 +------------ src/server/plugins/engine/routes/index.ts | 78 -------- src/server/plugins/engine/types.ts | 5 + src/server/services/cacheService.ts | 5 +- 12 files changed, 306 insertions(+), 217 deletions(-) create mode 100644 src/server/plugins/engine/pageControllers/helpers/state.test.ts create mode 100644 src/server/plugins/engine/pageControllers/helpers/state.ts diff --git a/docs/features/code-based/PRE_POPULATE_STATE.md b/docs/features/code-based/PRE_POPULATE_STATE.md index 658e1afcc..ccf814753 100644 --- a/docs/features/code-based/PRE_POPULATE_STATE.md +++ b/docs/features/code-based/PRE_POPULATE_STATE.md @@ -15,7 +15,7 @@ The feature uses the HiddenField component to prevent against rogue state inject The parameter values get copied on first load of the form, and are simple key/value parameters e.g.: ``` -?paramname1=paramval1,paramname2=paramname2 +?paramname1=paramval1,paramname2=paramval2 ``` There is no limit set on the number of parameters. The keys and values get copied as-is (no case changes get applied). diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index b57c893cb..43a934562 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -22,6 +22,7 @@ import { } from '~/src/server/plugins/engine/components/helpers/components.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' +import { stripParam } from '~/src/server/plugins/engine/pageControllers/helpers/state.js' import { type AnyFormRequest, type FormContext, @@ -133,7 +134,7 @@ export function proceed( const response = isReturnAllowed && isPathRelative(returnUrl) ? h.redirect(returnUrl) - : h.redirect(redirectPath(nextUrl)) + : h.redirect(redirectPath(nextUrl, stripParam(query, 'returnUrl'))) // Redirect POST to GET to avoid resubmission return method === 'post' diff --git a/src/server/plugins/engine/pageControllers/PageController.test.ts b/src/server/plugins/engine/pageControllers/PageController.test.ts index a7de8515b..73c7cb15d 100644 --- a/src/server/plugins/engine/pageControllers/PageController.test.ts +++ b/src/server/plugins/engine/pageControllers/PageController.test.ts @@ -65,7 +65,7 @@ describe('PageController', () => { it('returns feedback link default', () => { expect(controller1).toHaveProperty( 'feedbackLink', - '/form/csat?formId=form-id' + '/form/feedback?formId=form-id' ) }) diff --git a/src/server/plugins/engine/pageControllers/PageController.ts b/src/server/plugins/engine/pageControllers/PageController.ts index 014eb4ef6..fef76c6ec 100644 --- a/src/server/plugins/engine/pageControllers/PageController.ts +++ b/src/server/plugins/engine/pageControllers/PageController.ts @@ -118,7 +118,7 @@ export class PageController { } get feedbackLink() { - return `/form/csat?formId=${this.model.formId}` + return `/form/feedback?formId=${this.model.formId}` } get phaseTag() { diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index 20ef943ae..3f1f566b2 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.ts @@ -28,6 +28,7 @@ import { } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/index.js' import { PageController } from '~/src/server/plugins/engine/pageControllers/PageController.js' +import { prefillStateFromQueryParameters } from '~/src/server/plugins/engine/pageControllers/helpers/state.js' import { type AnyFormRequest, type FormContext, @@ -403,6 +404,12 @@ export class QuestionPageController extends PageController { const { collection, model, viewName } = this const { evaluationState } = context + // Copy any URL params into the form state (if not already done so) + if (await prefillStateFromQueryParameters(request, model)) { + // Forward to same page without query string + return h.redirect(`${request.url.origin}${request.url.pathname}`) + } + const viewModel = this.getViewModel(request, context) viewModel.errors = collection.getViewErrors(viewModel.errors) @@ -439,7 +446,7 @@ export class QuestionPageController extends PageController { // Warn the user if the form has no notification email set only on start page and summary page if ([startPath, summaryPath].includes(path) && !isForceAccess) { - const { notificationEmail } = await getFormMetadata(params.slug) + const notificationEmail = await getFormMetadata(params.slug) return !notificationEmail } diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 0da4396eb..2e26dd76d 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -25,6 +25,7 @@ import { } from '~/src/server/plugins/engine/models/types.js' import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/QuestionPageController.js' import { + type FormConfirmationState, type FormContext, type FormContextRequest, type FormSubmissionState @@ -154,8 +155,8 @@ export class SummaryPageController extends QuestionPageController { await cacheService.setConfirmationState(request, { confirmed: true, - formId: context.state.formId as string | undefined - }) + formId: context.state.formId + } as FormConfirmationState) // Clear all form data await cacheService.clearState(request) diff --git a/src/server/plugins/engine/pageControllers/helpers/state.test.ts b/src/server/plugins/engine/pageControllers/helpers/state.test.ts new file mode 100644 index 000000000..a8db9ffa5 --- /dev/null +++ b/src/server/plugins/engine/pageControllers/helpers/state.test.ts @@ -0,0 +1,180 @@ +import { ComponentType, type Page } from '@defra/forms-model' + +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' +import { + prefillStateFromQueryParameters, + stripParam +} from '~/src/server/plugins/engine/pageControllers/helpers/state.js' +import { type AnyFormRequest } from '~/src/server/plugins/engine/types.js' +import { type FormsService, type Services } from '~/src/server/types.js' + +function buildMockModel( + pagesOverride = [] as Page[], + pagesControllerOverride = [] as PageControllerClass[], + servicesOverride = {} as Services +) { + return { + def: { + metadata: { + submission: { code: 'TEST-CODE' } + } as { submission: { code: string } }, + pages: pagesOverride + }, + getFormContext: jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} + }), + pages: pagesControllerOverride, + services: servicesOverride + } as unknown as FormModel +} + +describe('prefillStateFromQueryParameters', () => { + const mockGetState = jest.fn() + const mockMergeState = jest.fn() + const mockRequestPrefill: AnyFormRequest = { + app: {}, + yar: { flash: () => [] }, + params: { path: 'test-path' }, + query: {} + } as unknown as AnyFormRequest + + it('should not add any state if no params', async () => { + const mockModelPrefill = buildMockModel( + [], + [ + { + getState: mockGetState, + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + await prefillStateFromQueryParameters(mockRequestPrefill, mockModelPrefill) + expect(mockMergeState).not.toHaveBeenCalled() + }) + + it('should only add state where param names match hidden field names', async () => { + const mockRequest2 = { + ...mockRequestPrefill, + query: { + param1: 'val1', + param2: 'val2', + param3: 'val3', + param4: 'val4' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'param2' + }, + { + type: ComponentType.HiddenField, + name: 'param4' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + await prefillStateFromQueryParameters(mockRequest2, mockModel) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + { + __copiedToState: 'true', + param2: 'val2', + param4: 'val4' + } + ) + }) + + it('should call lookup function for formId', async () => { + const mockRequest3 = { + ...mockRequestPrefill, + query: { + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'formId' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ], + { + formsService: { + getFormMetadata: jest.fn(), + getFormMetadataById: jest + .fn() + .mockResolvedValue({ title: 'My looked-up form name' }), + getFormDefinition: jest.fn() + } as unknown as FormsService + } as Services + ) + + await prefillStateFromQueryParameters(mockRequest3, mockModel) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + { + __copiedToState: 'true', + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', + formName: 'My looked-up form name' + } + ) + }) + + describe('stripParam', () => { + it('should remove param when exists', () => { + const params = { + paramName1: 'val1', + returnUrl: 'http://somesite.com', + paramName2: 'val2' + } + expect(stripParam(params, 'returnUrl')).toStrictEqual({ + paramName1: 'val1', + paramName2: 'val2' + }) + }) + + it('should handle param missing', () => { + const params = { + paramName1: 'val1', + returnUrl: 'http://somesite.com', + paramName2: 'val2' + } + expect(stripParam(params, 'paramNotThere')).toStrictEqual({ + paramName1: 'val1', + returnUrl: 'http://somesite.com', + paramName2: 'val2' + }) + }) + }) +}) diff --git a/src/server/plugins/engine/pageControllers/helpers/state.ts b/src/server/plugins/engine/pageControllers/helpers/state.ts new file mode 100644 index 000000000..55427e0c3 --- /dev/null +++ b/src/server/plugins/engine/pageControllers/helpers/state.ts @@ -0,0 +1,99 @@ +import { getHiddenFields } from '@defra/forms-model' + +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { + type AnyFormRequest, + type FormStateValue +} from '~/src/server/plugins/engine/types.js' +import { type FormQuery } from '~/src/server/routes/types.js' +import { type Services } from '~/src/server/types.js' + +/** + * A series of functions that can transform a pre-fill input parameter e.g lookup a form title based on form id + */ +const paramLookupFunctions = { + formId: async (val: string, services: Services) => { + let formTitle + if (val) { + const meta = await services.formsService.getFormMetadataById(val) + formTitle = meta.title + } + return { + key: 'formName', + value: formTitle + } + } +} as Partial< + Record< + string, + ( + val: string, + services: Services + ) => Promise<{ key: string; value: string | undefined }> + > +> + +export function stripParam(query: FormQuery, paramToRemove: string) { + const params = {} as Record + for (const [key, value = ''] of Object.entries(query)) { + if (key !== paramToRemove) { + params[key] = value + } + } + return Object.keys(params).length ? (params as FormQuery) : undefined +} + +/** + * Any hidden parameters defined in the FormDefinition may be pre-filled by URL parameter values. + * Other parameters are ignored for security reasons. + * @param request + * @param model + */ +export async function prefillStateFromQueryParameters( + request: AnyFormRequest, + model: FormModel +): Promise { + const hiddenFieldNames = new Set( + getHiddenFields(model.def).map((field) => field.name) + ) + + if (!hiddenFieldNames.size) { + return false + } + + // Remove 'returnUrl' param + const query = stripParam(request.query, 'returnUrl') + + if (!query) { + return false + } + + const page = model.pages[0] // Any page will do so just take the first one + const formData = await page.getState(request) + if (formData.__copiedToState) { + return false + } + + // Flag to denote the 'copy to state' has happened, so don't need to do it again + const params = { + __copiedToState: 'true' + } as Record + + for (const [key, value = ''] of Object.entries(query)) { + if (hiddenFieldNames.has(key)) { + const lookupFunc = paramLookupFunctions[key] + if (lookupFunc) { + const res = await lookupFunc(value, model.services) + // Store original value and result + params[key] = value + params[res.key] = res.value + } else { + params[key] = value + } + } + } + + await page.mergeState(request, formData, params) + + return true +} diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index a5b0f01ab..331adf09f 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -1,4 +1,4 @@ -import { ComponentType, type Page } from '@defra/forms-model' +import { type Page } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' @@ -10,16 +10,13 @@ import { } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' -import { - prefillStateFromQueryParameters, - redirectOrMakeHandler -} from '~/src/server/plugins/engine/routes/index.js' +import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' import { type AnyFormRequest, type OnRequestCallback } from '~/src/server/plugins/engine/types.js' import { type FormResponseToolkit } from '~/src/server/routes/types.js' -import { type FormsService, type Services } from '~/src/server/types.js' +import { type Services } from '~/src/server/types.js' jest.mock('~/src/server/plugins/engine/helpers') @@ -330,128 +327,4 @@ describe('redirectOrMakeHandler', () => { expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') }) }) - - describe('prefillStateFromQueryParameters', () => { - const mockGetState = jest.fn() - const mockMergeState = jest.fn() - const mockRequestPrefill: AnyFormRequest = { - server: mockServer, - app: {}, - yar: { flash: () => [] }, - params: { path: 'test-path' }, - query: {} - } as unknown as AnyFormRequest - - it('should not add any state if no params', async () => { - const mockModelPrefill = buildMockModel( - [], - [ - { - getState: mockGetState, - mergeState: mockMergeState - } as unknown as PageControllerClass - ] - ) - - await prefillStateFromQueryParameters( - mockRequestPrefill, - mockModelPrefill - ) - expect(mockMergeState).not.toHaveBeenCalled() - }) - - it('should only add state where param names match hidden field names', async () => { - const mockRequest2 = { - ...mockRequest, - query: { - param1: 'val1', - param2: 'val2', - param3: 'val3', - param4: 'val4' - } - } as unknown as AnyFormRequest - - const mockModel = buildMockModel( - [ - { - components: [ - { - type: ComponentType.HiddenField, - name: 'param2' - }, - { - type: ComponentType.HiddenField, - name: 'param4' - } - ], - next: [] - } as unknown as Page - ], - [ - { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ] - ) - - await prefillStateFromQueryParameters(mockRequest2, mockModel) - expect(mockMergeState).toHaveBeenCalledWith( - expect.anything(), - expect.anything(), - { - param2: 'val2', - param4: 'val4' - } - ) - }) - - it('should call lookup function for formId', async () => { - const mockRequest3 = { - ...mockRequest, - query: { - formId: 'c644804b-2f23-4c96-a2fc-ad4975974723' - } - } as unknown as AnyFormRequest - - const mockModel = buildMockModel( - [ - { - components: [ - { - type: ComponentType.HiddenField, - name: 'formId' - } - ], - next: [] - } as unknown as Page - ], - [ - { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ], - { - formsService: { - getFormMetadata: jest.fn(), - getFormMetadataById: jest - .fn() - .mockResolvedValue({ title: 'My looked-up form name' }), - getFormDefinition: jest.fn() - } as unknown as FormsService - } as Services - ) - - await prefillStateFromQueryParameters(mockRequest3, mockModel) - expect(mockMergeState).toHaveBeenCalledWith( - expect.anything(), - expect.anything(), - { - formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', - formName: 'My looked-up form name' - } - ) - }) - }) }) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index b1ddbd5fb..250d005eb 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -1,4 +1,3 @@ -import { getHiddenFields } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, @@ -34,7 +33,6 @@ import { type ExternalStateAppendage, type FormContext, type FormPayload, - type FormStateValue, type FormSubmissionState, type OnRequestCallback, type PluginOptions @@ -43,7 +41,6 @@ import { type FormRequest, type FormResponseToolkit } from '~/src/server/routes/types.js' -import { type Services } from '~/src/server/types.js' export async function redirectOrMakeHandler( request: AnyFormRequest, @@ -111,75 +108,6 @@ export async function redirectOrMakeHandler( return proceed(request, h, page.getHref(relevantPath)) } -/** - * A series of functions that can transform a pre-fill input parameter e.g lookup a form title based on form id - */ -const paramLookupFunctions = { - formId: async (val: string, services: Services) => { - let formTitle - if (val) { - const meta = await services.formsService.getFormMetadataById(val) - formTitle = meta.title - } - return { - key: 'formName', - value: formTitle - } - } -} as Partial< - Record< - string, - ( - val: string, - services: Services - ) => Promise<{ key: string; value: string | undefined }> - > -> - -/** - * Any hidden parameters defined in the FormDefinition may be pre-filled by URL parameter values. - * Other parameters are ignored for security reasons. - * @param request - * @param model - */ -export async function prefillStateFromQueryParameters( - request: AnyFormRequest, - model: FormModel -): Promise { - const hiddenFieldNames = new Set( - getHiddenFields(model.def).map((field) => field.name) - ) - const query = Object.keys(request.query).length ? request.query : undefined - - if (!query) { - return - } - - const params = {} as Record - - for (const [key, value = ''] of Object.entries(query)) { - if (hiddenFieldNames.has(key)) { - const lookupFunc = paramLookupFunctions[key] - if (lookupFunc) { - const res = await lookupFunc(value, model.services) - // Store original value and result - params[key] = value - params[res.key] = res.value - } else { - params[key] = value - } - } - } - - if (!Object.keys(params).length) { - return - } - - const page = model.pages[0] // Any page will do so just take the first one - const formData = await page.getState(request) - await page.mergeState(request, formData, params) -} - async function importExternalComponentState( request: AnyFormRequest, page: PageControllerClass, @@ -257,9 +185,6 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { if (server.app.model) { request.app.model = server.app.model - // Copy any URL params into the form state - await prefillStateFromQueryParameters(request, request.app.model) - return h.continue } @@ -329,9 +254,6 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { server.app.models.set(key, item) } - // Copy any URL params into the form state - await prefillStateFromQueryParameters(request, item.model) - // Assign the model to the request data // for use in the downstream handler request.app.model = item.model diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 61af993be..447a5b1db 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -97,6 +97,11 @@ export interface FormSubmissionError text: string // e.g: 'Date field must be a real date' } +export interface FormConfirmationState { + confirmed?: true + formId?: string +} + export interface FormPayloadParams { action?: FormAction confirm?: true diff --git a/src/server/services/cacheService.ts b/src/server/services/cacheService.ts index 63ac6b08c..2b08aaefb 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -6,6 +6,7 @@ import { type createServer } from '~/src/server/index.js' import { type AnyFormRequest, type AnyRequest, + type FormConfirmationState, type FormPayload, type FormState, type FormSubmissionError, @@ -55,7 +56,7 @@ export class CacheService { async getConfirmationState( request: AnyFormRequest - ): Promise<{ confirmed?: true; formId?: string }> { + ): Promise { const key = this.Key(request, ADDITIONAL_IDENTIFIER.Confirmation) const value = await this.cache.get(key) @@ -64,7 +65,7 @@ export class CacheService { async setConfirmationState( request: AnyFormRequest, - confirmationState: { confirmed?: true; formId?: string } + confirmationState: FormConfirmationState ) { const key = this.Key(request, ADDITIONAL_IDENTIFIER.Confirmation) const ttl = config.get('confirmationSessionTimeout') From 51a516ab30175ab4b5ea485793e97f70c85230a2 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 14:28:10 +0000 Subject: [PATCH 07/11] Fixed tests after review rework --- .../plugins/engine/pageControllers/QuestionPageController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index 3f1f566b2..a5bb2a289 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.ts @@ -446,7 +446,7 @@ export class QuestionPageController extends PageController { // Warn the user if the form has no notification email set only on start page and summary page if ([startPath, summaryPath].includes(path) && !isForceAccess) { - const notificationEmail = await getFormMetadata(params.slug) + const { notificationEmail } = await getFormMetadata(params.slug) return !notificationEmail } From 7a3804141d98b953db472727f42a68566e14abf0 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 14:42:47 +0000 Subject: [PATCH 08/11] Extra coverage --- .../pageControllers/helpers/state.test.ts | 323 +++++++++++------- 1 file changed, 209 insertions(+), 114 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/helpers/state.test.ts b/src/server/plugins/engine/pageControllers/helpers/state.test.ts index a8db9ffa5..99eade882 100644 --- a/src/server/plugins/engine/pageControllers/helpers/state.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/state.test.ts @@ -30,125 +30,213 @@ function buildMockModel( } as unknown as FormModel } -describe('prefillStateFromQueryParameters', () => { - const mockGetState = jest.fn() - const mockMergeState = jest.fn() - const mockRequestPrefill: AnyFormRequest = { - app: {}, - yar: { flash: () => [] }, - params: { path: 'test-path' }, - query: {} - } as unknown as AnyFormRequest - - it('should not add any state if no params', async () => { - const mockModelPrefill = buildMockModel( - [], - [ - { - getState: mockGetState, - mergeState: mockMergeState - } as unknown as PageControllerClass - ] - ) - - await prefillStateFromQueryParameters(mockRequestPrefill, mockModelPrefill) - expect(mockMergeState).not.toHaveBeenCalled() - }) - - it('should only add state where param names match hidden field names', async () => { - const mockRequest2 = { - ...mockRequestPrefill, - query: { - param1: 'val1', - param2: 'val2', - param3: 'val3', - param4: 'val4' - } +describe('State helpers', () => { + describe('prefillStateFromQueryParameters', () => { + const mockGetState = jest.fn() + const mockMergeState = jest.fn() + const mockRequestPrefill: AnyFormRequest = { + app: {}, + yar: { flash: () => [] }, + params: { path: 'test-path' }, + query: {} } as unknown as AnyFormRequest - const mockModel = buildMockModel( - [ - { - components: [ - { - type: ComponentType.HiddenField, - name: 'param2' - }, - { - type: ComponentType.HiddenField, - name: 'param4' - } - ], - next: [] - } as unknown as Page - ], - [ + it('should not add any state if no params', async () => { + const mockModelPrefill = buildMockModel( + [], + [ + { + getState: mockGetState, + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + expect( + await prefillStateFromQueryParameters( + mockRequestPrefill, + mockModelPrefill + ) + ).toBeFalse() + expect(mockMergeState).not.toHaveBeenCalled() + }) + + it('should ignore if no params (but some hidden fields)', async () => { + const mockRequest2 = { + ...mockRequestPrefill, + query: {} + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'param2' + }, + { + type: ComponentType.HiddenField, + name: 'param4' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + expect( + await prefillStateFromQueryParameters(mockRequest2, mockModel) + ).toBeFalse() + expect(mockMergeState).not.toHaveBeenCalled() + }) + + it('should only add state where param names match hidden field names', async () => { + const mockRequest2 = { + ...mockRequestPrefill, + query: { + param1: 'val1', + param2: 'val2', + param3: 'val3', + param4: 'val4' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'param2' + }, + { + type: ComponentType.HiddenField, + name: 'param4' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + expect( + await prefillStateFromQueryParameters(mockRequest2, mockModel) + ).toBe(true) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ] - ) - - await prefillStateFromQueryParameters(mockRequest2, mockModel) - expect(mockMergeState).toHaveBeenCalledWith( - expect.anything(), - expect.anything(), - { - __copiedToState: 'true', - param2: 'val2', - param4: 'val4' - } - ) - }) + __copiedToState: 'true', + param2: 'val2', + param4: 'val4' + } + ) + }) - it('should call lookup function for formId', async () => { - const mockRequest3 = { - ...mockRequestPrefill, - query: { - formId: 'c644804b-2f23-4c96-a2fc-ad4975974723' - } - } as unknown as AnyFormRequest + it('should ignore if already previously called', async () => { + const mockRequest2 = { + ...mockRequestPrefill, + query: { + param1: 'val1', + param2: 'val2', + param3: 'val3', + param4: 'val4' + } + } as unknown as AnyFormRequest + + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'param2' + }, + { + type: ComponentType.HiddenField, + name: 'param4' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({ + __copiedToState: 'true' + }), + mergeState: mockMergeState + } as unknown as PageControllerClass + ] + ) + + expect( + await prefillStateFromQueryParameters(mockRequest2, mockModel) + ).toBe(false) + expect(mockMergeState).not.toHaveBeenCalled() + }) + + it('should call lookup function for formId', async () => { + const mockRequest3 = { + ...mockRequestPrefill, + query: { + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723' + } + } as unknown as AnyFormRequest - const mockModel = buildMockModel( - [ + const mockModel = buildMockModel( + [ + { + components: [ + { + type: ComponentType.HiddenField, + name: 'formId' + } + ], + next: [] + } as unknown as Page + ], + [ + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } as unknown as PageControllerClass + ], { - components: [ - { - type: ComponentType.HiddenField, - name: 'formId' - } - ], - next: [] - } as unknown as Page - ], - [ + formsService: { + getFormMetadata: jest.fn(), + getFormMetadataById: jest + .fn() + .mockResolvedValue({ title: 'My looked-up form name' }), + getFormDefinition: jest.fn() + } as unknown as FormsService + } as Services + ) + + await prefillStateFromQueryParameters(mockRequest3, mockModel) + expect(mockMergeState).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ], - { - formsService: { - getFormMetadata: jest.fn(), - getFormMetadataById: jest - .fn() - .mockResolvedValue({ title: 'My looked-up form name' }), - getFormDefinition: jest.fn() - } as unknown as FormsService - } as Services - ) - - await prefillStateFromQueryParameters(mockRequest3, mockModel) - expect(mockMergeState).toHaveBeenCalledWith( - expect.anything(), - expect.anything(), - { - __copiedToState: 'true', - formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', - formName: 'My looked-up form name' - } - ) + __copiedToState: 'true', + formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', + formName: 'My looked-up form name' + } + ) + }) }) describe('stripParam', () => { @@ -156,11 +244,13 @@ describe('prefillStateFromQueryParameters', () => { const params = { paramName1: 'val1', returnUrl: 'http://somesite.com', - paramName2: 'val2' + paramName2: 'val2', + paramName3: undefined } expect(stripParam(params, 'returnUrl')).toStrictEqual({ paramName1: 'val1', - paramName2: 'val2' + paramName2: 'val2', + paramName3: '' }) }) @@ -176,5 +266,10 @@ describe('prefillStateFromQueryParameters', () => { paramName2: 'val2' }) }) + + it('should handle no params', () => { + const params = {} + expect(stripParam(params, 'anyParam')).toBeUndefined() + }) }) }) From a95632785e6ad6e72ae98b52edd93465b71eaab6 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 15:45:40 +0000 Subject: [PATCH 09/11] Passes page, not model --- .../pageControllers/QuestionPageController.ts | 2 +- .../pageControllers/helpers/state.test.ts | 116 ++++++++---------- .../engine/pageControllers/helpers/state.ts | 10 +- 3 files changed, 60 insertions(+), 68 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index a5bb2a289..ec778d6a1 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.ts @@ -405,7 +405,7 @@ export class QuestionPageController extends PageController { const { evaluationState } = context // Copy any URL params into the form state (if not already done so) - if (await prefillStateFromQueryParameters(request, model)) { + if (await prefillStateFromQueryParameters(request, this)) { // Forward to same page without query string return h.redirect(`${request.url.origin}${request.url.pathname}`) } diff --git a/src/server/plugins/engine/pageControllers/helpers/state.test.ts b/src/server/plugins/engine/pageControllers/helpers/state.test.ts index 99eade882..1a2a22f83 100644 --- a/src/server/plugins/engine/pageControllers/helpers/state.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/state.test.ts @@ -9,25 +9,30 @@ import { import { type AnyFormRequest } from '~/src/server/plugins/engine/types.js' import { type FormsService, type Services } from '~/src/server/types.js' -function buildMockModel( - pagesOverride = [] as Page[], - pagesControllerOverride = [] as PageControllerClass[], +const formId = '9d8bbbc9-29f1-499b-9f76-b2648bcfdf20' + +function buildMockPage( + pagesOverride = {}, + stateOverride = {}, servicesOverride = {} as Services ) { return { - def: { - metadata: { - submission: { code: 'TEST-CODE' } - } as { submission: { code: string } }, - pages: pagesOverride - }, - getFormContext: jest.fn().mockReturnValue({ - isForceAccess: false, - data: {} - }), - pages: pagesControllerOverride, - services: servicesOverride - } as unknown as FormModel + model: { + formId, + def: { + metadata: { + submission: { code: 'TEST-CODE' } + } as { submission: { code: string } }, + pages: pagesOverride + }, + getFormContext: jest.fn().mockReturnValue({ + isForceAccess: false, + data: {} + }), + services: servicesOverride + } as unknown as FormModel, + ...stateOverride + } as unknown as PageControllerClass } describe('State helpers', () => { @@ -42,20 +47,15 @@ describe('State helpers', () => { } as unknown as AnyFormRequest it('should not add any state if no params', async () => { - const mockModelPrefill = buildMockModel( - [], - [ - { - getState: mockGetState, - mergeState: mockMergeState - } as unknown as PageControllerClass - ] - ) + const mockPagePrefill = buildMockPage([], { + getState: mockGetState, + mergeState: mockMergeState + }) expect( await prefillStateFromQueryParameters( mockRequestPrefill, - mockModelPrefill + mockPagePrefill ) ).toBeFalse() expect(mockMergeState).not.toHaveBeenCalled() @@ -67,7 +67,7 @@ describe('State helpers', () => { query: {} } as unknown as AnyFormRequest - const mockModel = buildMockModel( + const mockPagePrefill = buildMockPage( [ { components: [ @@ -83,16 +83,14 @@ describe('State helpers', () => { next: [] } as unknown as Page ], - [ - { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ] + { + getState: mockGetState, + mergeState: mockMergeState + } ) expect( - await prefillStateFromQueryParameters(mockRequest2, mockModel) + await prefillStateFromQueryParameters(mockRequest2, mockPagePrefill) ).toBeFalse() expect(mockMergeState).not.toHaveBeenCalled() }) @@ -108,7 +106,7 @@ describe('State helpers', () => { } } as unknown as AnyFormRequest - const mockModel = buildMockModel( + const mockPagePrefill = buildMockPage( [ { components: [ @@ -124,22 +122,20 @@ describe('State helpers', () => { next: [] } as unknown as Page ], - [ - { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ] + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + } ) expect( - await prefillStateFromQueryParameters(mockRequest2, mockModel) + await prefillStateFromQueryParameters(mockRequest2, mockPagePrefill) ).toBe(true) expect(mockMergeState).toHaveBeenCalledWith( expect.anything(), expect.anything(), { - __copiedToState: 'true', + '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true', param2: 'val2', param4: 'val4' } @@ -157,7 +153,7 @@ describe('State helpers', () => { } } as unknown as AnyFormRequest - const mockModel = buildMockModel( + const mockPagePrefill = buildMockPage( [ { components: [ @@ -173,18 +169,16 @@ describe('State helpers', () => { next: [] } as unknown as Page ], - [ - { - getState: mockGetState.mockResolvedValue({ - __copiedToState: 'true' - }), - mergeState: mockMergeState - } as unknown as PageControllerClass - ] + { + getState: mockGetState.mockResolvedValue({ + '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true' + }), + mergeState: mockMergeState + } ) expect( - await prefillStateFromQueryParameters(mockRequest2, mockModel) + await prefillStateFromQueryParameters(mockRequest2, mockPagePrefill) ).toBe(false) expect(mockMergeState).not.toHaveBeenCalled() }) @@ -197,7 +191,7 @@ describe('State helpers', () => { } } as unknown as AnyFormRequest - const mockModel = buildMockModel( + const mockPagePrefill = buildMockPage( [ { components: [ @@ -209,12 +203,10 @@ describe('State helpers', () => { next: [] } as unknown as Page ], - [ - { - getState: mockGetState.mockResolvedValue({}), - mergeState: mockMergeState - } as unknown as PageControllerClass - ], + { + getState: mockGetState.mockResolvedValue({}), + mergeState: mockMergeState + }, { formsService: { getFormMetadata: jest.fn(), @@ -226,12 +218,12 @@ describe('State helpers', () => { } as Services ) - await prefillStateFromQueryParameters(mockRequest3, mockModel) + await prefillStateFromQueryParameters(mockRequest3, mockPagePrefill) expect(mockMergeState).toHaveBeenCalledWith( expect.anything(), expect.anything(), { - __copiedToState: 'true', + '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true', formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', formName: 'My looked-up form name' } diff --git a/src/server/plugins/engine/pageControllers/helpers/state.ts b/src/server/plugins/engine/pageControllers/helpers/state.ts index 55427e0c3..c0b5fe17d 100644 --- a/src/server/plugins/engine/pageControllers/helpers/state.ts +++ b/src/server/plugins/engine/pageControllers/helpers/state.ts @@ -1,6 +1,6 @@ import { getHiddenFields } from '@defra/forms-model' -import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' import { type AnyFormRequest, type FormStateValue @@ -51,8 +51,9 @@ export function stripParam(query: FormQuery, paramToRemove: string) { */ export async function prefillStateFromQueryParameters( request: AnyFormRequest, - model: FormModel + page: PageControllerClass ): Promise { + const { model } = page const hiddenFieldNames = new Set( getHiddenFields(model.def).map((field) => field.name) ) @@ -68,15 +69,14 @@ export async function prefillStateFromQueryParameters( return false } - const page = model.pages[0] // Any page will do so just take the first one const formData = await page.getState(request) - if (formData.__copiedToState) { + if (formData[`__copiedToState${model.formId}`]) { return false } // Flag to denote the 'copy to state' has happened, so don't need to do it again const params = { - __copiedToState: 'true' + [`__copiedToState${model.formId}`]: 'true' } as Record for (const [key, value = ''] of Object.entries(query)) { From 59e4695a059c4484366e67a1579f4298d142824b Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 16:12:29 +0000 Subject: [PATCH 10/11] Removed session flag as no longer required --- .../pageControllers/helpers/state.test.ts | 46 ------------------- .../engine/pageControllers/helpers/state.ts | 12 ++--- 2 files changed, 3 insertions(+), 55 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/helpers/state.test.ts b/src/server/plugins/engine/pageControllers/helpers/state.test.ts index 1a2a22f83..a3d008d78 100644 --- a/src/server/plugins/engine/pageControllers/helpers/state.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/state.test.ts @@ -9,8 +9,6 @@ import { import { type AnyFormRequest } from '~/src/server/plugins/engine/types.js' import { type FormsService, type Services } from '~/src/server/types.js' -const formId = '9d8bbbc9-29f1-499b-9f76-b2648bcfdf20' - function buildMockPage( pagesOverride = {}, stateOverride = {}, @@ -18,7 +16,6 @@ function buildMockPage( ) { return { model: { - formId, def: { metadata: { submission: { code: 'TEST-CODE' } @@ -135,54 +132,12 @@ describe('State helpers', () => { expect.anything(), expect.anything(), { - '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true', param2: 'val2', param4: 'val4' } ) }) - it('should ignore if already previously called', async () => { - const mockRequest2 = { - ...mockRequestPrefill, - query: { - param1: 'val1', - param2: 'val2', - param3: 'val3', - param4: 'val4' - } - } as unknown as AnyFormRequest - - const mockPagePrefill = buildMockPage( - [ - { - components: [ - { - type: ComponentType.HiddenField, - name: 'param2' - }, - { - type: ComponentType.HiddenField, - name: 'param4' - } - ], - next: [] - } as unknown as Page - ], - { - getState: mockGetState.mockResolvedValue({ - '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true' - }), - mergeState: mockMergeState - } - ) - - expect( - await prefillStateFromQueryParameters(mockRequest2, mockPagePrefill) - ).toBe(false) - expect(mockMergeState).not.toHaveBeenCalled() - }) - it('should call lookup function for formId', async () => { const mockRequest3 = { ...mockRequestPrefill, @@ -223,7 +178,6 @@ describe('State helpers', () => { expect.anything(), expect.anything(), { - '__copiedToState9d8bbbc9-29f1-499b-9f76-b2648bcfdf20': 'true', formId: 'c644804b-2f23-4c96-a2fc-ad4975974723', formName: 'My looked-up form name' } diff --git a/src/server/plugins/engine/pageControllers/helpers/state.ts b/src/server/plugins/engine/pageControllers/helpers/state.ts index c0b5fe17d..01357329e 100644 --- a/src/server/plugins/engine/pageControllers/helpers/state.ts +++ b/src/server/plugins/engine/pageControllers/helpers/state.ts @@ -54,6 +54,7 @@ export async function prefillStateFromQueryParameters( page: PageControllerClass ): Promise { const { model } = page + const hiddenFieldNames = new Set( getHiddenFields(model.def).map((field) => field.name) ) @@ -69,15 +70,7 @@ export async function prefillStateFromQueryParameters( return false } - const formData = await page.getState(request) - if (formData[`__copiedToState${model.formId}`]) { - return false - } - - // Flag to denote the 'copy to state' has happened, so don't need to do it again - const params = { - [`__copiedToState${model.formId}`]: 'true' - } as Record + const params = {} as Record for (const [key, value = ''] of Object.entries(query)) { if (hiddenFieldNames.has(key)) { @@ -93,6 +86,7 @@ export async function prefillStateFromQueryParameters( } } + const formData = await page.getState(request) await page.mergeState(request, formData, params) return true From ddb86811891f29fd3ae97e41a06c1b4a400446ba Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Thu, 27 Nov 2025 16:18:27 +0000 Subject: [PATCH 11/11] chore(release): #minor