From ad767cdb346f2ed0d336034597c0bdc2d31f07a2 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 2 Dec 2025 18:06:21 +0000 Subject: [PATCH 01/27] Gracefully handle file upload persistence errors by clearing state --- src/server/constants.js | 1 + .../pageControllers/SummaryPageController.ts | 57 ++++++++++++++----- .../plugins/engine/pageControllers/errors.ts | 19 +++++++ src/server/plugins/engine/views/index.html | 12 ++++ src/server/plugins/nunjucks/context.js | 9 ++- src/server/plugins/nunjucks/types.js | 1 + src/server/services/cacheService.ts | 12 ++++ src/typings/hapi/index.d.ts | 1 + 8 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 src/server/plugins/engine/pageControllers/errors.ts diff --git a/src/server/constants.js b/src/server/constants.js index fd659a09e..b6460c901 100644 --- a/src/server/constants.js +++ b/src/server/constants.js @@ -2,3 +2,4 @@ export const PREVIEW_PATH_PREFIX = '/preview' export const FORM_PREFIX = '' export const EXTERNAL_STATE_PAYLOAD = 'EXTERNAL_STATE_PAYLOAD' export const EXTERNAL_STATE_APPENDAGE = 'EXTERNAL_STATE_APPENDAGE' +export const FILE_UPLOAD_STATE_ERROR = 'FILE_UPLOAD_ERROR' \ No newline at end of file diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 2e26dd76d..6d9de6734 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -6,6 +6,8 @@ import { } from '@defra/forms-model' import Boom from '@hapi/boom' import { type RouteOptions } from '@hapi/hapi' +import { cache } from 'joi' +import { FILE_UPLOAD_STATE_ERROR } from '~/src/server/constants.js' import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' @@ -37,6 +39,8 @@ import { type FormRequestPayloadRefs, type FormResponseToolkit } from '~/src/server/routes/types.js' +import { InvalidComponentStateError } from './errors.js' +import { FormComponent } from '../components/FormComponent.js' export class SummaryPageController extends QuestionPageController { declare pageDef: Page @@ -143,14 +147,28 @@ export class SummaryPageController extends QuestionPageController { // Send submission email if (emailAddress) { const viewModel = this.getSummaryViewModel(request, context) - await submitForm( - context, - request, - viewModel, - model, - emailAddress, - formMetadata - ) + + try { + await submitForm( + context, + request, + viewModel, + model, + emailAddress, + formMetadata + ) + } catch (error) { + if (error instanceof InvalidComponentStateError) { + // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is + // by resetting the problematic components and letting the user re-try. + // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. + request.yar.flash(FILE_UPLOAD_STATE_ERROR, 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') + await cacheService.resetComponentStates(request, error.getStateKeys()) + return this.proceed(request, h, error.components[0].page?.path) + } + + throw error + } } await cacheService.setConfirmationState(request, { @@ -185,7 +203,7 @@ export async function submitForm( emailAddress: string, formMetadata: FormMetadata ) { - await extendFileRetention(model, context.state, emailAddress) + await extendFileRetention(request, model, context.state, emailAddress) const formStatus = checkFormStatus(request.params) const logTags = ['submit', 'submissionApi'] @@ -223,22 +241,28 @@ export async function submitForm( } async function extendFileRetention( + request: FormRequestPayload, model: FormModel, state: FormSubmissionState, updatedRetrievalKey: string ) { const { formSubmissionService } = model.services + const cacheService = getCacheService(request.server) const { persistFiles } = formSubmissionService const files: { fileId: string; initiatedRetrievalKey: string }[] = [] + + const formFileUploadComponents: FormComponent[] = [] // For each file upload component with files in // state, add the files to the batch getting persisted model.pages.forEach((page) => { - const fileUploadComponents = page.collection.fields.filter( + const pageFileUploadComponents = page.collection.fields.filter( (component) => component instanceof FileUploadField ) - fileUploadComponents.forEach((component) => { + pageFileUploadComponents.forEach((component) => { + formFileUploadComponents.push(component) + const values = component.getFormValueFromState(state) if (!values?.length) { return @@ -253,8 +277,15 @@ async function extendFileRetention( }) }) - if (files.length) { - return persistFiles(files, updatedRetrievalKey) + if (!files.length) { + return + } + + try { + await persistFiles(files, updatedRetrievalKey) + } catch (error) { + // TODO only throw if the error is related to file upload problem and not a general error like networking + throw new InvalidComponentStateError(formFileUploadComponents) } } diff --git a/src/server/plugins/engine/pageControllers/errors.ts b/src/server/plugins/engine/pageControllers/errors.ts new file mode 100644 index 000000000..55e152114 --- /dev/null +++ b/src/server/plugins/engine/pageControllers/errors.ts @@ -0,0 +1,19 @@ +import { FileUploadField } from "../components/FileUploadField.js"; +import { FormComponent } from "../components/FormComponent.js"; + +export class InvalidComponentStateError extends Error { + public readonly components: FormComponent[]; + + constructor(components: FormComponent[]) { + const message = `Invalid component state for: ${components.map(c => c.name).join(', ')}` + super(message) + this.name = 'InvalidComponentStateError' + this.components = components + } + + getStateKeys() { + const extraStateKeys = this.components.some(c => c instanceof FileUploadField) ? ['upload'] : [] + + return this.components.map(c => c.name).concat(extraStateKeys) + } +} \ No newline at end of file diff --git a/src/server/plugins/engine/views/index.html b/src/server/plugins/engine/views/index.html index af3258c88..a3bc1b278 100644 --- a/src/server/plugins/engine/views/index.html +++ b/src/server/plugins/engine/views/index.html @@ -10,6 +10,18 @@ {% include "partials/preview-banner.html" %} {% endif %} + {% if error | length %} + {{ govukErrorSummary({ + titleText: "There is a problem", + errorList: [ + { + text: error, + href: "#" + } + ] + }) }} + {% endif %} + {% if errors %} {{ govukErrorSummary({ titleText: "There is a problem", diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index ef5b9dfca..9861e2e95 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -11,6 +11,7 @@ import { encodeUrl, safeGenerateCrumb } from '~/src/server/plugins/engine/helpers.js' +import { FILE_UPLOAD_STATE_ERROR } from '../../constants.js' const logger = createLogger() @@ -54,9 +55,15 @@ export async function context(request) { crumb: safeGenerateCrumb(request), currentPath: `${request.path}${request.url.search}`, previewMode: isPreviewMode ? formState : undefined, - slug: isResponseOK ? params?.slug : undefined + slug: isResponseOK ? params?.slug : undefined, + error: request.yar.flash(FILE_UPLOAD_STATE_ERROR) // TODO merge into 'errors' and link back to form component properly } + // TODO get rid of this hack + request.yar.commit(/** @type {import('@hapi/hapi').ResponseToolkit} */ ({ + state: () => {} + })) + return ctx } diff --git a/src/server/plugins/nunjucks/types.js b/src/server/plugins/nunjucks/types.js index b9bc8f774..e608689e8 100644 --- a/src/server/plugins/nunjucks/types.js +++ b/src/server/plugins/nunjucks/types.js @@ -17,6 +17,7 @@ * @property {string} [currentPath] - Current path * @property {string} [previewMode] - Preview mode * @property {string} [slug] - Form slug + * @property {string} [error] - Error message for temporary error messages (not related to form state) * @property {FormContext} [context] - the current form context */ diff --git a/src/server/services/cacheService.ts b/src/server/services/cacheService.ts index 2b08aaefb..91d7d6261 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -99,6 +99,18 @@ export class CacheService { request.yar.flash(key.id, message) } + async resetComponentStates(request: AnyFormRequest, componentNames: string[]) { + const state = await this.getState(request) + + for(const componentId of componentNames) { + if (state[componentId]) { + delete state[componentId] + } + } + + return this.setState(request, state) + } + /** * The key used to store user session data against. * If there are multiple forms on the same runner instance, for example `form-a` and `form-a-feedback` this will prevent CacheService from clearing data from `form-a` if a user gave feedback before they finished `form-a` diff --git a/src/typings/hapi/index.d.ts b/src/typings/hapi/index.d.ts index 4c9b7abc0..abfbf9c66 100644 --- a/src/typings/hapi/index.d.ts +++ b/src/typings/hapi/index.d.ts @@ -20,6 +20,7 @@ declare module '@hapi/yar' { interface YarFlashes { [EXTERNAL_STATE_APPENDAGE]: object [EXTERNAL_STATE_PAYLOAD]: object + [FILE_UPLOAD_STATE_ERROR]: string [key: string]: { errors: FormSubmissionError[] } } } From e8f1693c2f9540cf6e00f879e97b727605eaa5f3 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Fri, 5 Dec 2025 16:38:40 +0000 Subject: [PATCH 02/27] handle missing files that have expired --- .../pageControllers/SummaryPageController.ts | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 6d9de6734..f75c98fce 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -6,11 +6,11 @@ import { } from '@defra/forms-model' import Boom from '@hapi/boom' import { type RouteOptions } from '@hapi/hapi' -import { cache } from 'joi' -import { FILE_UPLOAD_STATE_ERROR } from '~/src/server/constants.js' +import { FILE_UPLOAD_STATE_ERROR } from '~/src/server/constants.js' import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' +import { type FormComponent } from '~/src/server/plugins/engine/components/FormComponent.js' import { getAnswer } from '~/src/server/plugins/engine/components/helpers/components.js' import { checkEmailAddressForLiveFormSubmission, @@ -26,6 +26,7 @@ import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/QuestionPageController.js' +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { type FormConfirmationState, type FormContext, @@ -39,8 +40,6 @@ import { type FormRequestPayloadRefs, type FormResponseToolkit } from '~/src/server/routes/types.js' -import { InvalidComponentStateError } from './errors.js' -import { FormComponent } from '../components/FormComponent.js' export class SummaryPageController extends QuestionPageController { declare pageDef: Page @@ -159,10 +158,13 @@ export class SummaryPageController extends QuestionPageController { ) } catch (error) { if (error instanceof InvalidComponentStateError) { - // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is + // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is // by resetting the problematic components and letting the user re-try. // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. - request.yar.flash(FILE_UPLOAD_STATE_ERROR, 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') + request.yar.flash( + FILE_UPLOAD_STATE_ERROR, + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) await cacheService.resetComponentStates(request, error.getStateKeys()) return this.proceed(request, h, error.components[0].page?.path) } @@ -203,7 +205,7 @@ export async function submitForm( emailAddress: string, formMetadata: FormMetadata ) { - await extendFileRetention(request, model, context.state, emailAddress) + await extendFileRetention(model, context.state, emailAddress) const formStatus = checkFormStatus(request.params) const logTags = ['submit', 'submissionApi'] @@ -241,16 +243,14 @@ export async function submitForm( } async function extendFileRetention( - request: FormRequestPayload, model: FormModel, state: FormSubmissionState, updatedRetrievalKey: string ) { const { formSubmissionService } = model.services - const cacheService = getCacheService(request.server) const { persistFiles } = formSubmissionService const files: { fileId: string; initiatedRetrievalKey: string }[] = [] - + const formFileUploadComponents: FormComponent[] = [] // For each file upload component with files in @@ -280,12 +280,19 @@ async function extendFileRetention( if (!files.length) { return } - + try { await persistFiles(files, updatedRetrievalKey) } catch (error) { - // TODO only throw if the error is related to file upload problem and not a general error like networking - throw new InvalidComponentStateError(formFileUploadComponents) + if ( + Boom.isBoom(error) && + (error.output.statusCode === 403 || // Forbidden - retrieval key invalid + error.output.statusCode === 410) // Gone - file expired (took to long to submit, etc) + ) { + throw new InvalidComponentStateError(formFileUploadComponents) + } + + throw error } } From 928c5020a1e7443837a8ace2a579e8c4d85c361b Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 8 Dec 2025 16:55:46 +0000 Subject: [PATCH 03/27] Move file upload persistence logic into file upload component --- mise.toml | 3 + src/server/constants.js | 2 +- .../engine/components/FileUploadField.ts | 44 ++++++++- .../engine/components/FormComponent.ts | 5 + .../pageControllers/SummaryPageController.ts | 91 ++++++------------- .../plugins/engine/pageControllers/errors.ts | 8 +- src/server/plugins/engine/types.ts | 2 + src/server/plugins/nunjucks/context.js | 7 +- src/typings/hapi/index.d.ts | 3 +- 9 files changed, 92 insertions(+), 73 deletions(-) create mode 100644 mise.toml diff --git a/mise.toml b/mise.toml new file mode 100644 index 000000000..db60d29dc --- /dev/null +++ b/mise.toml @@ -0,0 +1,3 @@ +[tools] +1password = "latest" +node = "latest" diff --git a/src/server/constants.js b/src/server/constants.js index b6460c901..301539c14 100644 --- a/src/server/constants.js +++ b/src/server/constants.js @@ -2,4 +2,4 @@ export const PREVIEW_PATH_PREFIX = '/preview' export const FORM_PREFIX = '' export const EXTERNAL_STATE_PAYLOAD = 'EXTERNAL_STATE_PAYLOAD' export const EXTERNAL_STATE_APPENDAGE = 'EXTERNAL_STATE_APPENDAGE' -export const FILE_UPLOAD_STATE_ERROR = 'FILE_UPLOAD_ERROR' \ No newline at end of file +export const COMPONENT_STATE_ERROR = 'COMPONENT_STATE_ERROR' diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index ec2f1be33..3bc06c9e0 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -1,10 +1,12 @@ import { type FileUploadFieldComponent } from '@defra/forms-model' +import Boom from '@hapi/boom' import joi, { type ArraySchema } from 'joi' import { FormComponent, isUploadState } from '~/src/server/plugins/engine/components/FormComponent.js' +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { messageTemplate } from '~/src/server/plugins/engine/pageControllers/validationOptions.js' import { FileStatus, @@ -13,6 +15,7 @@ import { type FileState, type FileUpload, type FileUploadMetadata, + type FormContext, type FormPayload, type FormState, type FormStateValue, @@ -26,7 +29,7 @@ import { type UploadStatusResponse } from '~/src/server/plugins/engine/types.js' import { render } from '~/src/server/plugins/nunjucks/index.js' -import { type FormQuery } from '~/src/server/routes/types.js' +import { type FormQuery, type FormRequestPayload } from '~/src/server/routes/types.js' export const uploadIdSchema = joi.string().uuid().required() @@ -284,6 +287,45 @@ export class FileUploadField extends FormComponent { return FileUploadField.getAllPossibleErrors() } + async onSubmit(request: FormRequestPayload, context: FormContext) { + if(!context.metadata.notificationEmail) { + throw new Error('No notification email set in form context metadata') + } + + if(!request.app.model?.services.formSubmissionService) { + throw new Error('No form submission service available in app model') + } + + const { formSubmissionService } = request.app.model?.services + const values = this.getFormValueFromState(context.state) ?? [] + + const files = values.map((value) => ({ + fileId: value.status.form.file.fileId, + initiatedRetrievalKey: value.status.metadata.retrievalKey + })) + + if (!files.length) { + return + } + + try { + await formSubmissionService.persistFiles(files, context.metadata.notificationEmail) + } catch (error) { + if ( + Boom.isBoom(error) && + (error.output.statusCode === 403 || // Forbidden - retrieval key invalid + error.output.statusCode === 410) // Gone - file expired (took to long to submit, etc) + ) { + // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is + // by resetting the problematic components and letting the user re-try. + // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. + throw new InvalidComponentStateError([this], 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') + } + + throw error + } + } + /** * Static version of getAllPossibleErrors that doesn't require a component instance. */ diff --git a/src/server/plugins/engine/components/FormComponent.ts b/src/server/plugins/engine/components/FormComponent.ts index ff04cccab..bb986497f 100644 --- a/src/server/plugins/engine/components/FormComponent.ts +++ b/src/server/plugins/engine/components/FormComponent.ts @@ -2,6 +2,7 @@ import { type FormComponentsDef, type Item } from '@defra/forms-model' import { ComponentBase } from '~/src/server/plugins/engine/components/ComponentBase.js' import { optionalText } from '~/src/server/plugins/engine/components/constants.js' +import { type FormContext, type FormRequestPayload } from '~/src/server/plugins/engine/types/index.js' import { type ErrorMessageTemplateList, type FileState, @@ -220,6 +221,10 @@ export class FormComponent extends ComponentBase { advancedSettingsErrors: [] } } + + onSubmit(_request: FormRequestPayload, _context: FormContext): void { + // No default implementation + } } /** diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index f75c98fce..19aebbe76 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -7,10 +7,8 @@ import { import Boom from '@hapi/boom' import { type RouteOptions } from '@hapi/hapi' -import { FILE_UPLOAD_STATE_ERROR } from '~/src/server/constants.js' +import { COMPONENT_STATE_ERROR } from '~/src/server/constants.js' import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' -import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' -import { type FormComponent } from '~/src/server/plugins/engine/components/FormComponent.js' import { getAnswer } from '~/src/server/plugins/engine/components/helpers/components.js' import { checkEmailAddressForLiveFormSubmission, @@ -30,8 +28,7 @@ import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageCont import { type FormConfirmationState, type FormContext, - type FormContextRequest, - type FormSubmissionState + type FormContextRequest } from '~/src/server/plugins/engine/types.js' import { FormAction, @@ -158,14 +155,10 @@ export class SummaryPageController extends QuestionPageController { ) } catch (error) { if (error instanceof InvalidComponentStateError) { - // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is - // by resetting the problematic components and letting the user re-try. - // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. - request.yar.flash( - FILE_UPLOAD_STATE_ERROR, - 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' - ) + request.yar.flash(COMPONENT_STATE_ERROR, error.userMessage) + await cacheService.resetComponentStates(request, error.getStateKeys()) + return this.proceed(request, h, error.components[0].page?.path) } @@ -205,7 +198,7 @@ export async function submitForm( emailAddress: string, formMetadata: FormMetadata ) { - await extendFileRetention(model, context.state, emailAddress) + await finaliseComponents(model, request, context) const formStatus = checkFormStatus(request.params) const logTags = ['submit', 'submissionApi'] @@ -218,6 +211,8 @@ export async function submitForm( summaryViewModel.details ) + + // Submit data request.logger.info(logTags, 'Submitting data') const submitResponse = await submitData( @@ -242,57 +237,25 @@ export async function submitForm( ) } -async function extendFileRetention( - model: FormModel, - state: FormSubmissionState, - updatedRetrievalKey: string -) { - const { formSubmissionService } = model.services - const { persistFiles } = formSubmissionService - const files: { fileId: string; initiatedRetrievalKey: string }[] = [] - - const formFileUploadComponents: FormComponent[] = [] - - // For each file upload component with files in - // state, add the files to the batch getting persisted - model.pages.forEach((page) => { - const pageFileUploadComponents = page.collection.fields.filter( - (component) => component instanceof FileUploadField - ) - - pageFileUploadComponents.forEach((component) => { - formFileUploadComponents.push(component) - - const values = component.getFormValueFromState(state) - if (!values?.length) { - return - } - - files.push( - ...values.map(({ status }) => ({ - fileId: status.form.file.fileId, - initiatedRetrievalKey: status.metadata.retrievalKey - })) - ) - }) - }) - - if (!files.length) { - return - } - - try { - await persistFiles(files, updatedRetrievalKey) - } catch (error) { - if ( - Boom.isBoom(error) && - (error.output.statusCode === 403 || // Forbidden - retrieval key invalid - error.output.statusCode === 410) // Gone - file expired (took to long to submit, etc) - ) { - throw new InvalidComponentStateError(formFileUploadComponents) - } - - throw error +/** + * Finalises any components that need post-processing before form submission. Candidates usually involve + * those that have external state. + * Examples include: + * - file uploads which are 'persisted' before submission + * - payments which are 'captured' before submission + * @param model + * @param request + * @param context + */ +async function finaliseComponents(model: FormModel, request: FormRequestPayload, context: FormContext) { + const relevantPages = context.relevantPages.flatMap((page) => page.collection.fields) + + for (const component of relevantPages) { + /* + Each component will throw InvalidComponent if its state is invalid, which is handled + by handleFormSubmit + */ + await component.onSubmit(request, context) } } diff --git a/src/server/plugins/engine/pageControllers/errors.ts b/src/server/plugins/engine/pageControllers/errors.ts index 55e152114..47a1ae2ff 100644 --- a/src/server/plugins/engine/pageControllers/errors.ts +++ b/src/server/plugins/engine/pageControllers/errors.ts @@ -1,14 +1,16 @@ -import { FileUploadField } from "../components/FileUploadField.js"; -import { FormComponent } from "../components/FormComponent.js"; +import { FileUploadField } from "~/src/server/plugins/engine/components/FileUploadField.js"; +import { type FormComponent } from "~/src/server/plugins/engine/components/FormComponent.js"; export class InvalidComponentStateError extends Error { public readonly components: FormComponent[]; + public readonly userMessage: string; - constructor(components: FormComponent[]) { + constructor(components: FormComponent[], userMessage: string) { const message = `Invalid component state for: ${components.map(c => c.name).join(', ')}` super(message) this.name = 'InvalidComponentStateError' this.components = components + this.userMessage = userMessage } getStateKeys() { diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 447a5b1db..84672e792 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -1,6 +1,7 @@ import { type ComponentDef, type Event, + type FormMetadata, type FormVersionMetadata, type Item, type List, @@ -185,6 +186,7 @@ export interface FormContext { componentDefMap: Map pageMap: Map componentMap: Map + metadata: FormMetadata referenceNumber: string submittedVersionNumber?: number } diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index 9861e2e95..b292fd9ac 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -4,6 +4,8 @@ import { basename, join } from 'node:path' import Boom from '@hapi/boom' import { StatusCodes } from 'http-status-codes' +import { COMPONENT_STATE_ERROR } from '~/src/server/constants.js' + import { config } from '~/src/config/index.js' import { createLogger } from '~/src/server/common/helpers/logging/logger.js' import { @@ -11,7 +13,6 @@ import { encodeUrl, safeGenerateCrumb } from '~/src/server/plugins/engine/helpers.js' -import { FILE_UPLOAD_STATE_ERROR } from '../../constants.js' const logger = createLogger() @@ -56,11 +57,11 @@ export async function context(request) { currentPath: `${request.path}${request.url.search}`, previewMode: isPreviewMode ? formState : undefined, slug: isResponseOK ? params?.slug : undefined, - error: request.yar.flash(FILE_UPLOAD_STATE_ERROR) // TODO merge into 'errors' and link back to form component properly + error: request.yar.flash(COMPONENT_STATE_ERROR) // TODO merge into 'errors' and link back to form component properly } // TODO get rid of this hack - request.yar.commit(/** @type {import('@hapi/hapi').ResponseToolkit} */ ({ + await request.yar.commit(/** @type {import('@hapi/hapi').ResponseToolkit} */ ({ state: () => {} })) diff --git a/src/typings/hapi/index.d.ts b/src/typings/hapi/index.d.ts index abfbf9c66..37ad6408c 100644 --- a/src/typings/hapi/index.d.ts +++ b/src/typings/hapi/index.d.ts @@ -5,6 +5,7 @@ import { type ServerYar, type Yar } from '@hapi/yar' import { type Logger } from 'pino' import { + COMPONENT_STATE_ERROR, type EXTERNAL_STATE_APPENDAGE, type EXTERNAL_STATE_PAYLOAD } from '~/src/server/constants.js' @@ -20,7 +21,7 @@ declare module '@hapi/yar' { interface YarFlashes { [EXTERNAL_STATE_APPENDAGE]: object [EXTERNAL_STATE_PAYLOAD]: object - [FILE_UPLOAD_STATE_ERROR]: string + [COMPONENT_STATE_ERROR]: string [key: string]: { errors: FormSubmissionError[] } } } From b8ba086b904bf1344aa84262ec477c392ff1f2eb Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 9 Dec 2025 11:33:04 +0000 Subject: [PATCH 04/27] Move metadata into param for onSubmit function --- src/server/plugins/engine/components/FileUploadField.ts | 8 ++++---- src/server/plugins/engine/components/FormComponent.ts | 4 ++-- .../engine/pageControllers/SummaryPageController.ts | 4 ++-- src/server/plugins/engine/types.ts | 2 -- src/server/plugins/nunjucks/context.js | 6 ++++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index 3bc06c9e0..ece8b45e3 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -1,4 +1,4 @@ -import { type FileUploadFieldComponent } from '@defra/forms-model' +import { FormMetadata, type FileUploadFieldComponent } from '@defra/forms-model' import Boom from '@hapi/boom' import joi, { type ArraySchema } from 'joi' @@ -287,8 +287,8 @@ export class FileUploadField extends FormComponent { return FileUploadField.getAllPossibleErrors() } - async onSubmit(request: FormRequestPayload, context: FormContext) { - if(!context.metadata.notificationEmail) { + async onSubmit(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { + if(!metadata.notificationEmail) { throw new Error('No notification email set in form context metadata') } @@ -309,7 +309,7 @@ export class FileUploadField extends FormComponent { } try { - await formSubmissionService.persistFiles(files, context.metadata.notificationEmail) + await formSubmissionService.persistFiles(files, metadata.notificationEmail) } catch (error) { if ( Boom.isBoom(error) && diff --git a/src/server/plugins/engine/components/FormComponent.ts b/src/server/plugins/engine/components/FormComponent.ts index bb986497f..fbf7f4502 100644 --- a/src/server/plugins/engine/components/FormComponent.ts +++ b/src/server/plugins/engine/components/FormComponent.ts @@ -1,4 +1,4 @@ -import { type FormComponentsDef, type Item } from '@defra/forms-model' +import { FormMetadata, type FormComponentsDef, type Item } from '@defra/forms-model' import { ComponentBase } from '~/src/server/plugins/engine/components/ComponentBase.js' import { optionalText } from '~/src/server/plugins/engine/components/constants.js' @@ -222,7 +222,7 @@ export class FormComponent extends ComponentBase { } } - onSubmit(_request: FormRequestPayload, _context: FormContext): void { + onSubmit(_request: FormRequestPayload, _metadata: FormMetadata, _context: FormContext): void { // No default implementation } } diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 19aebbe76..084c4400d 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -247,7 +247,7 @@ export async function submitForm( * @param request * @param context */ -async function finaliseComponents(model: FormModel, request: FormRequestPayload, context: FormContext) { +async function finaliseComponents(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { const relevantPages = context.relevantPages.flatMap((page) => page.collection.fields) for (const component of relevantPages) { @@ -255,7 +255,7 @@ async function finaliseComponents(model: FormModel, request: FormRequestPayload, Each component will throw InvalidComponent if its state is invalid, which is handled by handleFormSubmit */ - await component.onSubmit(request, context) + await component.onSubmit(request, metadata, context) } } diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 84672e792..447a5b1db 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -1,7 +1,6 @@ import { type ComponentDef, type Event, - type FormMetadata, type FormVersionMetadata, type Item, type List, @@ -186,7 +185,6 @@ export interface FormContext { componentDefMap: Map pageMap: Map componentMap: Map - metadata: FormMetadata referenceNumber: string submittedVersionNumber?: number } diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index b292fd9ac..aeaaf93d3 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -57,11 +57,13 @@ export async function context(request) { currentPath: `${request.path}${request.url.search}`, previewMode: isPreviewMode ? formState : undefined, slug: isResponseOK ? params?.slug : undefined, - error: request.yar.flash(COMPONENT_STATE_ERROR) // TODO merge into 'errors' and link back to form component properly + // @ts-expect-error TODO fix and merge into 'errors' to link back to the form component + error: request.yar.flash(COMPONENT_STATE_ERROR) } - // TODO get rid of this hack + // @ts-expect-error temporary code while developing -- TODO remove await request.yar.commit(/** @type {import('@hapi/hapi').ResponseToolkit} */ ({ + // @ts-ignore state: () => {} })) From bb71d401eca2e300f59aed2aeb79cbfa237c5916 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 9 Dec 2025 12:27:40 +0000 Subject: [PATCH 05/27] Retrieve metadata to get the email to persist files with --- src/server/forms/simple-form.yaml | 64 +++++++++++++++++++ .../engine/components/FileUploadField.ts | 6 +- .../pageControllers/SummaryPageController.ts | 4 +- .../engine/services/localFormsService.js | 7 ++ 4 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/server/forms/simple-form.yaml diff --git a/src/server/forms/simple-form.yaml b/src/server/forms/simple-form.yaml new file mode 100644 index 000000000..2f72d1fe4 --- /dev/null +++ b/src/server/forms/simple-form.yaml @@ -0,0 +1,64 @@ +--- +name: Page events +engine: V2 +schema: 2 +startPage: '/summary' +pages: + - title: Your name + path: '/your-name' + components: + - type: TextField + title: What is your first name? + name: applicantFirstName + shortDescription: Your first name + hint: '' + options: + required: true + schema: {} + id: 1fb8e182-c709-4792-8f83-e01d8b1fee1a + - type: TextField + title: What is your last name? + name: applicantLastName + shortDescription: Your last name + hint: '' + options: + required: true + schema: {} + id: b68df7f1-d4f4-4c17-83c8-402f584906c9 + next: [] + id: 622a35ec-3795-418a-81f3-a45746959045 + - title: Upload a copy of your passport + controller: FileUploadPageController + path: '/upload-passport' + components: + - type: FileUploadField + title: Please upload a copy of your passport + name: passportUpload + shortDescription: Upload passport + hint: '' + options: + required: true + schema: {} + id: 987c1234-56d7-89e0-1234-56789abcdef0 + id: 23456789-0abc-def1-2345-67890abcdef1 + - title: '' + path: '/date-of-birth' + components: + - type: DatePartsField + title: When is {{ applicantFirstName }} {{ applicantLastName }}'s birthday? + name: dateOfBirth + shortDescription: Your birthday + hint: '' + options: + required: true + schema: {} + id: '00738799-3489-4ab2-a57b-542eecb31bfa' + next: [] + id: da0fbdb4-a2de-4650-be16-9ba552af135f + - id: 449a45f6-4541-4a46-91bd-8b8931b07b50 + title: '' + path: '/summary' + controller: SummaryPageController +conditions: [] +sections: [] +lists: [] diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index ece8b45e3..90a7787ba 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -288,7 +288,9 @@ export class FileUploadField extends FormComponent { } async onSubmit(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { - if(!metadata.notificationEmail) { + const notificationEmail = metadata.notificationEmail ?? 'defraforms@defra.gov.uk' + + if(!notificationEmail) { throw new Error('No notification email set in form context metadata') } @@ -309,7 +311,7 @@ export class FileUploadField extends FormComponent { } try { - await formSubmissionService.persistFiles(files, metadata.notificationEmail) + await formSubmissionService.persistFiles(files, notificationEmail) } catch (error) { if ( Boom.isBoom(error) && diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 084c4400d..40a12f2f2 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -147,6 +147,7 @@ export class SummaryPageController extends QuestionPageController { try { await submitForm( context, + formMetadata, request, viewModel, model, @@ -192,13 +193,14 @@ export class SummaryPageController extends QuestionPageController { export async function submitForm( context: FormContext, + metadata: FormMetadata, request: FormRequestPayload, summaryViewModel: SummaryViewModel, model: FormModel, emailAddress: string, formMetadata: FormMetadata ) { - await finaliseComponents(model, request, context) + await finaliseComponents(request, metadata, context) const formStatus = checkFormStatus(request.params) const logTags = ['submit', 'submissionApi'] diff --git a/src/server/plugins/engine/services/localFormsService.js b/src/server/plugins/engine/services/localFormsService.js index c824bad34..6dc0d2d3f 100644 --- a/src/server/plugins/engine/services/localFormsService.js +++ b/src/server/plugins/engine/services/localFormsService.js @@ -51,5 +51,12 @@ export const formsService = async () => { slug: 'components' }) + await loader.addForm('src/server/forms/simple-form.yaml', { + ...metadata, + id: 'a1b2c3d4-e5f6-7890-abcd-ef0123456789', + title: 'Simple Form', + slug: 'simple-form' + }) + return loader.toFormsService() } From a85b5edde953b5f932577ca197e1060d38ed4b36 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 9 Dec 2025 17:23:35 +0000 Subject: [PATCH 06/27] link flashed error to input --- src/server/plugins/engine/helpers.ts | 8 +++++++ .../pageControllers/SummaryPageController.ts | 5 +++- src/server/plugins/engine/views/index.html | 23 +++++++++---------- src/server/plugins/nunjucks/context.js | 5 +++- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index 43a934562..70aed777e 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -321,6 +321,14 @@ export function getError(detail: ValidationErrorItem): FormSubmissionError { } } +export function createError(componentName: string, message: string) { + return { + href: `#${componentName}`, + name: componentName, + text: message + } +} + /** * A small helper to safely generate a crumb token. * Checks that the crumb plugin is available, that crumb diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 40a12f2f2..3954d3bf8 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -13,6 +13,7 @@ import { getAnswer } from '~/src/server/plugins/engine/components/helpers/compon import { checkEmailAddressForLiveFormSubmission, checkFormStatus, + createError, getCacheService } from '~/src/server/plugins/engine/helpers.js' import { @@ -156,7 +157,9 @@ export class SummaryPageController extends QuestionPageController { ) } catch (error) { if (error instanceof InvalidComponentStateError) { - request.yar.flash(COMPONENT_STATE_ERROR, error.userMessage) + const govukError = createError(error.components[0].name, error.userMessage) + + request.yar.flash(COMPONENT_STATE_ERROR, govukError, true) await cacheService.resetComponentStates(request, error.getStateKeys()) diff --git a/src/server/plugins/engine/views/index.html b/src/server/plugins/engine/views/index.html index a3bc1b278..a5fa69b03 100644 --- a/src/server/plugins/engine/views/index.html +++ b/src/server/plugins/engine/views/index.html @@ -10,22 +10,21 @@ {% include "partials/preview-banner.html" %} {% endif %} - {% if error | length %} - {{ govukErrorSummary({ - titleText: "There is a problem", - errorList: [ - { - text: error, - href: "#" - } - ] - }) }} - {% endif %} + {% set allErrors = [] %} {% if errors %} + {% set allErrors = allErrors.concat(errors) %} + {% endif %} + + {% if flashedErrors %} + {% set allErrors = allErrors.concat(flashedErrors) %} + {% endif %} + + + {% if allErrors | length %} {{ govukErrorSummary({ titleText: "There is a problem", - errorList: checkErrorTemplates(errors) + errorList: checkErrorTemplates(allErrors) }) }} {% endif %} diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index aeaaf93d3..51715fd6d 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -48,6 +48,9 @@ export async function context(request) { consumerViewContext = await pluginStorage.viewContext(request) } + const flashedError = request.yar.flash(COMPONENT_STATE_ERROR) + const flashedErrors = !Array.isArray(flashedError) ? [flashedError] : undefined + /** @type {ViewContext} */ const ctx = { // take consumers props first so we can override it @@ -58,7 +61,7 @@ export async function context(request) { previewMode: isPreviewMode ? formState : undefined, slug: isResponseOK ? params?.slug : undefined, // @ts-expect-error TODO fix and merge into 'errors' to link back to the form component - error: request.yar.flash(COMPONENT_STATE_ERROR) + flashedErrors } // @ts-expect-error temporary code while developing -- TODO remove From ad2f5c5f398cdb366085c745e8f44d705f0531b7 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 9 Dec 2025 17:25:48 +0000 Subject: [PATCH 07/27] Use single component for invalid state --- .../plugins/engine/components/FileUploadField.ts | 2 +- .../engine/pageControllers/SummaryPageController.ts | 4 ++-- src/server/plugins/engine/pageControllers/errors.ts | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index 90a7787ba..be070fe47 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -321,7 +321,7 @@ export class FileUploadField extends FormComponent { // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is // by resetting the problematic components and letting the user re-try. // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. - throw new InvalidComponentStateError([this], 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') + throw new InvalidComponentStateError(this, 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') } throw error diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 3954d3bf8..ac8504fdb 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -157,13 +157,13 @@ export class SummaryPageController extends QuestionPageController { ) } catch (error) { if (error instanceof InvalidComponentStateError) { - const govukError = createError(error.components[0].name, error.userMessage) + const govukError = createError(error.component.name, error.userMessage) request.yar.flash(COMPONENT_STATE_ERROR, govukError, true) await cacheService.resetComponentStates(request, error.getStateKeys()) - return this.proceed(request, h, error.components[0].page?.path) + return this.proceed(request, h, error.component.page?.path) } throw error diff --git a/src/server/plugins/engine/pageControllers/errors.ts b/src/server/plugins/engine/pageControllers/errors.ts index 47a1ae2ff..8ddce5783 100644 --- a/src/server/plugins/engine/pageControllers/errors.ts +++ b/src/server/plugins/engine/pageControllers/errors.ts @@ -2,20 +2,20 @@ import { FileUploadField } from "~/src/server/plugins/engine/components/FileUplo import { type FormComponent } from "~/src/server/plugins/engine/components/FormComponent.js"; export class InvalidComponentStateError extends Error { - public readonly components: FormComponent[]; + public readonly component: FormComponent; public readonly userMessage: string; - constructor(components: FormComponent[], userMessage: string) { - const message = `Invalid component state for: ${components.map(c => c.name).join(', ')}` + constructor(component: FormComponent, userMessage: string) { + const message = `Invalid component state for: ${component.name}` super(message) this.name = 'InvalidComponentStateError' - this.components = components + this.component = component this.userMessage = userMessage } getStateKeys() { - const extraStateKeys = this.components.some(c => c instanceof FileUploadField) ? ['upload'] : [] + const extraStateKeys = this.component instanceof FileUploadField ? ['upload'] : [] - return this.components.map(c => c.name).concat(extraStateKeys) + return [this.component.name].concat(extraStateKeys) } } \ No newline at end of file From 4d4c377fc5859364a73ef80756b6c8e6cdf9a4be Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 9 Dec 2025 18:27:41 +0000 Subject: [PATCH 08/27] Test that invalid component state is handled in a form journey --- test/form/component-state-errors.test.js | 437 +++++++++++++++++++++++ 1 file changed, 437 insertions(+) create mode 100644 test/form/component-state-errors.test.js diff --git a/test/form/component-state-errors.test.js b/test/form/component-state-errors.test.js new file mode 100644 index 000000000..fc8a6c9d0 --- /dev/null +++ b/test/form/component-state-errors.test.js @@ -0,0 +1,437 @@ +import { join } from 'node:path' + +import Boom from '@hapi/boom' +import { StatusCodes } from 'http-status-codes' + +import { FORM_PREFIX } from '~/src/server/constants.js' +import { createServer } from '~/src/server/index.js' +import { + persistFiles, + submit +} from '~/src/server/plugins/engine/services/formSubmissionService.js' +import { getFormMetadata } from '~/src/server/plugins/engine/services/formsService.js' +import * as uploadService from '~/src/server/plugins/engine/services/uploadService.js' +import { FileStatus, UploadStatus } from '~/src/server/plugins/engine/types.js' +import { FormAction } from '~/src/server/routes/types.js' +import { CacheService } from '~/src/server/services/cacheService.js' +import * as fixtures from '~/test/fixtures/index.js' +import { getCookieHeader } from '~/test/utils/get-cookie.js' + +const basePath = `${FORM_PREFIX}/file-upload-basic` + +jest.mock('~/src/server/utils/notify.ts') +jest.mock('~/src/server/plugins/engine/services/formsService.js') +jest.mock('~/src/server/plugins/engine/services/formSubmissionService.js') +jest.mock('~/src/server/plugins/engine/services/uploadService.js') + +/** + * @satisfies {FileState} + */ +const readyFile = { + uploadId: '404a31b2-8ee8-49b5-a6e8-23da9e69ba9e', + status: { + uploadStatus: UploadStatus.ready, + metadata: { + retrievalKey: 'foo.bar@defra.gov.uk' + }, + form: { + file: { + fileId: 'a9e7470b-86a5-4826-a908-360a36aac71d', + filename: 'api details.pdf', + fileStatus: FileStatus.complete, + contentLength: 735163 + } + }, + numberOfRejectedFiles: 0 + } +} + +describe('Component State Error Tests - File Upload', () => { + /** @type {Server} */ + let server + + beforeAll(async () => { + server = await createServer({ + formFileName: 'file-upload-basic.js', + formFilePath: join(import.meta.dirname, 'definitions') + }) + + await server.initialize() + }) + + beforeEach(() => { + jest.mocked(getFormMetadata).mockResolvedValue(fixtures.form.metadata) + jest.clearAllMocks() + }) + + afterAll(async () => { + await server.stop() + }) + + test('POST /summary with 403 error from persistFiles resets component state and redirects', async () => { + // Set up initial state with uploaded file + jest.spyOn(CacheService.prototype, 'getState').mockResolvedValueOnce( + // @ts-expect-error - Allow upload property mismatch with `FormState` + /** @type {FormSubmissionState} */ ({ + upload: { + '/file-upload-component': { + files: [readyFile], + upload: { + uploadId: '123-546-788', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-788', + statusUrl: 'http://localhost:7337/status/123-546-788' + } + } + } + }) + ) + + jest + .mocked(uploadService.getUploadStatus) + .mockResolvedValueOnce(readyFile.status) + + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-790', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-790', + statusUrl: 'http://localhost:7337/status/123-546-790' + }) + + // POST the form data to navigate to summary + const res1 = await server.inject({ + url: `${basePath}/file-upload-component`, + method: 'POST', + payload: { + crumb: 'dummyCrumb', + action: FormAction.Validate + } + }) + + expect(res1.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(res1.headers.location).toBe(`${basePath}/summary`) + + // Extract the session cookie + const headers = getCookieHeader(res1, 'session') + + // Mock persistFiles to throw a 403 Forbidden error (invalid retrieval key) + const forbiddenError = Boom.forbidden('Invalid retrieval key') + jest.mocked(persistFiles).mockRejectedValueOnce(forbiddenError) + + // POST the summary form - should catch the error and redirect back + const submitRes = await server.inject({ + url: `${basePath}/summary`, + method: 'POST', + headers, + payload: {} + }) + + // Should redirect back to the file upload component page + expect(submitRes.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(submitRes.headers.location).toBe(`${basePath}/file-upload-component`) + + // Verify persistFiles was called + expect(persistFiles).toHaveBeenCalledTimes(1) + expect(persistFiles).toHaveBeenCalledWith( + [ + { + fileId: readyFile.status.form.file.fileId, + initiatedRetrievalKey: readyFile.status.metadata.retrievalKey + } + ], + 'defraforms@defra.gov.uk' + ) + + // Verify submit was NOT called + expect(submit).not.toHaveBeenCalled() + }) + + test('POST /summary with 410 error from persistFiles resets component state and redirects', async () => { + // Set up initial state with uploaded file + jest.spyOn(CacheService.prototype, 'getState').mockResolvedValueOnce( + // @ts-expect-error - Allow upload property mismatch with `FormState` + /** @type {FormSubmissionState} */ ({ + upload: { + '/file-upload-component': { + files: [readyFile], + upload: { + uploadId: '123-546-788', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-788', + statusUrl: 'http://localhost:7337/status/123-546-788' + } + } + } + }) + ) + + jest + .mocked(uploadService.getUploadStatus) + .mockResolvedValueOnce(readyFile.status) + + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-790', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-790', + statusUrl: 'http://localhost:7337/status/123-546-790' + }) + + // POST the form data to navigate to summary + const res1 = await server.inject({ + url: `${basePath}/file-upload-component`, + method: 'POST', + payload: { + crumb: 'dummyCrumb', + action: FormAction.Validate + } + }) + + expect(res1.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(res1.headers.location).toBe(`${basePath}/summary`) + + // Extract the session cookie + const headers = getCookieHeader(res1, 'session') + + // Mock persistFiles to throw a 410 Gone error (file expired) + const goneError = Boom.resourceGone('File has expired') + jest.mocked(persistFiles).mockRejectedValueOnce(goneError) + + // POST the summary form - should catch the error and redirect back + const submitRes = await server.inject({ + url: `${basePath}/summary`, + method: 'POST', + headers, + payload: {} + }) + + // Should redirect back to the file upload component page + expect(submitRes.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(submitRes.headers.location).toBe(`${basePath}/file-upload-component`) + + // Verify persistFiles was called + expect(persistFiles).toHaveBeenCalledTimes(1) + expect(persistFiles).toHaveBeenCalledWith( + [ + { + fileId: readyFile.status.form.file.fileId, + initiatedRetrievalKey: readyFile.status.metadata.retrievalKey + } + ], + 'defraforms@defra.gov.uk' + ) + + // Verify submit was NOT called + expect(submit).not.toHaveBeenCalled() + }) + + test('POST /summary with other Boom errors throws and does not redirect', async () => { + // Set up initial state with uploaded file + jest.spyOn(CacheService.prototype, 'getState').mockResolvedValueOnce( + // @ts-expect-error - Allow upload property mismatch with `FormState` + /** @type {FormSubmissionState} */ ({ + upload: { + '/file-upload-component': { + files: [readyFile], + upload: { + uploadId: '123-546-788', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-788', + statusUrl: 'http://localhost:7337/status/123-546-788' + } + } + } + }) + ) + + jest + .mocked(uploadService.getUploadStatus) + .mockResolvedValueOnce(readyFile.status) + + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-790', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-790', + statusUrl: 'http://localhost:7337/status/123-546-790' + }) + + // POST the form data to navigate to summary + const res1 = await server.inject({ + url: `${basePath}/file-upload-component`, + method: 'POST', + payload: { + crumb: 'dummyCrumb', + action: FormAction.Validate + } + }) + + expect(res1.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(res1.headers.location).toBe(`${basePath}/summary`) + + // Extract the session cookie + const headers = getCookieHeader(res1, 'session') + + // Mock persistFiles to throw a 500 Internal Server Error + const serverError = Boom.internal('Internal server error') + jest.mocked(persistFiles).mockRejectedValueOnce(serverError) + + // POST the summary form - should throw the error + const submitRes = await server.inject({ + url: `${basePath}/summary`, + method: 'POST', + headers, + payload: {} + }) + + // Should return an error response (not redirect) + expect(submitRes.statusCode).toBe(StatusCodes.INTERNAL_SERVER_ERROR) + + // Verify persistFiles was called + expect(persistFiles).toHaveBeenCalledTimes(1) + + // Verify submit was NOT called + expect(submit).not.toHaveBeenCalled() + }) + + test('POST /summary with non-Boom error throws and does not redirect', async () => { + // Set up initial state with uploaded file + jest.spyOn(CacheService.prototype, 'getState').mockResolvedValueOnce( + // @ts-expect-error - Allow upload property mismatch with `FormState` + /** @type {FormSubmissionState} */ ({ + upload: { + '/file-upload-component': { + files: [readyFile], + upload: { + uploadId: '123-546-788', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-788', + statusUrl: 'http://localhost:7337/status/123-546-788' + } + } + } + }) + ) + + jest + .mocked(uploadService.getUploadStatus) + .mockResolvedValueOnce(readyFile.status) + + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-790', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-790', + statusUrl: 'http://localhost:7337/status/123-546-790' + }) + + // POST the form data to navigate to summary + const res1 = await server.inject({ + url: `${basePath}/file-upload-component`, + method: 'POST', + payload: { + crumb: 'dummyCrumb', + action: FormAction.Validate + } + }) + + expect(res1.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(res1.headers.location).toBe(`${basePath}/summary`) + + // Extract the session cookie + const headers = getCookieHeader(res1, 'session') + + // Mock persistFiles to throw a regular error + const regularError = new Error('Something went wrong') + jest.mocked(persistFiles).mockRejectedValueOnce(regularError) + + // POST the summary form - should throw the error + const submitRes = await server.inject({ + url: `${basePath}/summary`, + method: 'POST', + headers, + payload: {} + }) + + // Should return an error response (not redirect) + expect(submitRes.statusCode).toBe(StatusCodes.INTERNAL_SERVER_ERROR) + + // Verify persistFiles was called + expect(persistFiles).toHaveBeenCalledTimes(1) + + // Verify submit was NOT called + expect(submit).not.toHaveBeenCalled() + }) + + test('GET /file-upload-component after 403 error shows error message', async () => { + // Set up initial state with uploaded file + jest.spyOn(CacheService.prototype, 'getState').mockResolvedValueOnce( + // @ts-expect-error - Allow upload property mismatch with `FormState` + /** @type {FormSubmissionState} */ ({ + upload: { + '/file-upload-component': { + files: [readyFile], + upload: { + uploadId: '123-546-788', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-788', + statusUrl: 'http://localhost:7337/status/123-546-788' + } + } + } + }) + ) + + jest + .mocked(uploadService.getUploadStatus) + .mockResolvedValueOnce(readyFile.status) + + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-790', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-790', + statusUrl: 'http://localhost:7337/status/123-546-790' + }) + + // POST the form data to navigate to summary + const res1 = await server.inject({ + url: `${basePath}/file-upload-component`, + method: 'POST', + payload: { + crumb: 'dummyCrumb', + action: FormAction.Validate + } + }) + + expect(res1.statusCode).toBe(StatusCodes.SEE_OTHER) + + // Extract the session cookie + const headers = getCookieHeader(res1, 'session') + + // Mock persistFiles to throw a 403 Forbidden error + const forbiddenError = Boom.forbidden('Invalid retrieval key') + jest.mocked(persistFiles).mockRejectedValueOnce(forbiddenError) + + // POST the summary form - should catch the error and redirect back + const submitRes = await server.inject({ + url: `${basePath}/summary`, + method: 'POST', + headers, + payload: {} + }) + + expect(submitRes.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(submitRes.headers.location).toBe(`${basePath}/file-upload-component`) + + // Now GET the file upload page and verify the error message is displayed + jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ + uploadId: '123-546-791', + uploadUrl: 'http://localhost:7337/upload-and-scan/123-546-791', + statusUrl: 'http://localhost:7337/status/123-546-791' + }) + + const pageRes = await server.inject({ + url: `${basePath}/file-upload-component`, + headers + }) + + expect(pageRes.statusCode).toBe(StatusCodes.OK) + + // Verify the error message appears in the response + const html = pageRes.payload + expect(html).toContain('There was a problem with your uploaded files') + expect(html).toContain('Re-upload them before submitting the form again') + }) +}) + +/** + * @import { Server } from '@hapi/hapi' + * @import { FileState, FormSubmissionState } from '~/src/server/plugins/engine/types.js' + */ From 7ce77c836f19e0b531a48ce68123cce4d68c2c98 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 10 Dec 2025 11:10:56 +0000 Subject: [PATCH 09/27] Remove unused condition --- src/server/plugins/engine/components/FileUploadField.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index be070fe47..27a074372 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -289,10 +289,6 @@ export class FileUploadField extends FormComponent { async onSubmit(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { const notificationEmail = metadata.notificationEmail ?? 'defraforms@defra.gov.uk' - - if(!notificationEmail) { - throw new Error('No notification email set in form context metadata') - } if(!request.app.model?.services.formSubmissionService) { throw new Error('No form submission service available in app model') From 37a062d584220675ec4c4d82d19437e808d7cd44 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 10 Dec 2025 11:11:11 +0000 Subject: [PATCH 10/27] Documentation for InvalidComponentStateError --- .../plugins/engine/pageControllers/errors.ts | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/errors.ts b/src/server/plugins/engine/pageControllers/errors.ts index 8ddce5783..b0361b186 100644 --- a/src/server/plugins/engine/pageControllers/errors.ts +++ b/src/server/plugins/engine/pageControllers/errors.ts @@ -1,21 +1,30 @@ -import { FileUploadField } from "~/src/server/plugins/engine/components/FileUploadField.js"; -import { type FormComponent } from "~/src/server/plugins/engine/components/FormComponent.js"; +import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' +import { type FormComponent } from '~/src/server/plugins/engine/components/FormComponent.js' +/** + * Thrown when a component has an invalid state. This is typically only required where state needs + * to be checked against an external source upon submission of a form. For example: file upload + * has internal state (file upload IDs) but also external state (files in S3). The internal state + * is always validated by the engine, but the external state needs validating too. + * + * This should be used within a formComponent.onSubmit(...). + */ export class InvalidComponentStateError extends Error { - public readonly component: FormComponent; - public readonly userMessage: string; + public readonly component: FormComponent + public readonly userMessage: string - constructor(component: FormComponent, userMessage: string) { - const message = `Invalid component state for: ${component.name}` - super(message) - this.name = 'InvalidComponentStateError' - this.component = component - this.userMessage = userMessage - } + constructor(component: FormComponent, userMessage: string) { + const message = `Invalid component state for: ${component.name}` + super(message) + this.name = 'InvalidComponentStateError' + this.component = component + this.userMessage = userMessage + } - getStateKeys() { - const extraStateKeys = this.component instanceof FileUploadField ? ['upload'] : [] + getStateKeys() { + const extraStateKeys = + this.component instanceof FileUploadField ? ['upload'] : [] - return [this.component.name].concat(extraStateKeys) - } -} \ No newline at end of file + return [this.component.name].concat(extraStateKeys) + } +} From 8b80ed8b800c853debe44d8624e5fc2732028d25 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 10 Dec 2025 14:45:49 +0000 Subject: [PATCH 11/27] unit tests for new onsubmit logic --- .../engine/components/FileUploadField.test.ts | 203 +++++++++++++++++- .../SummaryPageController.test.ts | 9 + .../engine/pageControllers/errors.test.ts | 63 ++++++ 3 files changed, 273 insertions(+), 2 deletions(-) create mode 100644 src/server/plugins/engine/pageControllers/errors.test.ts diff --git a/src/server/plugins/engine/components/FileUploadField.test.ts b/src/server/plugins/engine/components/FileUploadField.test.ts index 5c16e7c00..de8be0369 100644 --- a/src/server/plugins/engine/components/FileUploadField.test.ts +++ b/src/server/plugins/engine/components/FileUploadField.test.ts @@ -1,10 +1,12 @@ import { ComponentType, - type FileUploadFieldComponent + type FileUploadFieldComponent, + type FormMetadata } from '@defra/forms-model' +import Boom from '@hapi/boom' import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' -import { tempItemSchema } from '~/src/server/plugins/engine/components/FileUploadField.js' +import { FileUploadField, tempItemSchema } from '~/src/server/plugins/engine/components/FileUploadField.js' import { getAnswer, type Field @@ -14,12 +16,15 @@ import { createPage, type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { validationOptions as opts } from '~/src/server/plugins/engine/pageControllers/validationOptions.js' import { FileStatus, UploadStatus, + type FormContext, type UploadState } from '~/src/server/plugins/engine/types.js' +import { type FormRequestPayload } from '~/src/server/routes/types.js' import definition from '~/test/form/definitions/file-upload-basic.js' import { getFormData, getFormState } from '~/test/helpers/component-helpers.js' @@ -828,4 +833,198 @@ describe('FileUploadField', () => { ) }) }) + + describe('onSubmit', () => { + let fileUploadField: FileUploadField + let mockRequest: FormRequestPayload + let mockMetadata: FormMetadata + let mockContext: FormContext + let mockPersistFiles: jest.Mock + + beforeEach(() => { + // Create a FileUploadField instance + const componentDef: FileUploadFieldComponent = { + name: 'fileUpload', + title: 'Upload something', + type: ComponentType.FileUploadField, + options: {}, + schema: {} + } + + const page = model.pages.find((p) => p.path === '/file-upload-component') + fileUploadField = new FileUploadField(componentDef, { + model, + page: page! + }) + + // Mock persistFiles + mockPersistFiles = jest.fn().mockResolvedValue(undefined) + + // Mock request + mockRequest = { + app: { + model: { + services: { + formSubmissionService: { + persistFiles: mockPersistFiles + } + } + } + } + } as unknown as FormRequestPayload + + // Mock metadata + mockMetadata = { + notificationEmail: 'test@example.com' + } as FormMetadata + + // Mock context with state + mockContext = { + state: { + fileUpload: validState + } + } as unknown as FormContext + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + it('should successfully persist files', async () => { + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + + expect(mockPersistFiles).toHaveBeenCalledTimes(1) + expect(mockPersistFiles).toHaveBeenCalledWith( + [ + { + fileId: 'fcb4f0f8-6862-4836-86dc-f56ff900b0ff', + initiatedRetrievalKey: 'enrique.chase@defra.gov.uk' + }, + { + fileId: 'e1d6cf98-35a7-4f97-8a28-cdd2b115d8fa', + initiatedRetrievalKey: 'enrique.chase@defra.gov.uk' + }, + { + fileId: '71fb359c-dee7-4c2e-8701-239eb892765a', + initiatedRetrievalKey: 'enrique.chase@defra.gov.uk' + } + ], + 'test@example.com' + ) + }) + + it('should use default email when notificationEmail is not set', async () => { + mockMetadata.notificationEmail = undefined + + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + + expect(mockPersistFiles).toHaveBeenCalledWith( + expect.any(Array), + 'defraforms@defra.gov.uk' + ) + }) + + it('should not call persistFiles when no files in state', async () => { + mockContext.state = {} + + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + + expect(mockPersistFiles).not.toHaveBeenCalled() + }) + + it('should not call persistFiles when empty array in state', async () => { + mockContext.state = { fileUpload: [] } + + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + + expect(mockPersistFiles).not.toHaveBeenCalled() + }) + + it('should pass empty string when notificationEmail is empty string', async () => { + mockMetadata.notificationEmail = '' + + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + + expect(mockPersistFiles).toHaveBeenCalledWith( + expect.any(Array), + '' + ) + }) + + it('should throw Error when formSubmissionService is not available', async () => { + mockRequest.app.model!.services = {} as any + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow('No form submission service available in app model') + }) + + it('should throw InvalidComponentStateError when persistFiles throws 403 Forbidden', async () => { + const forbiddenError = Boom.forbidden('Invalid retrieval key') + mockPersistFiles.mockRejectedValue(forbiddenError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow(InvalidComponentStateError) + + try { + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + } catch (error) { + expect(error).toBeInstanceOf(InvalidComponentStateError) + expect((error as InvalidComponentStateError).component).toBe( + fileUploadField + ) + expect((error as InvalidComponentStateError).userMessage).toBe( + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) + } + }) + + it('should throw InvalidComponentStateError when persistFiles throws 410 Gone', async () => { + const goneError = Boom.resourceGone('File has expired') + mockPersistFiles.mockRejectedValue(goneError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow(InvalidComponentStateError) + + try { + await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + } catch (error) { + expect(error).toBeInstanceOf(InvalidComponentStateError) + expect((error as InvalidComponentStateError).component).toBe( + fileUploadField + ) + expect((error as InvalidComponentStateError).userMessage).toBe( + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) + } + }) + + it('should re-throw other Boom errors without wrapping', async () => { + const serverError = Boom.internal('Internal server error') + mockPersistFiles.mockRejectedValue(serverError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow(serverError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.not.toThrow(InvalidComponentStateError) + }) + + it('should re-throw non-Boom errors without wrapping', async () => { + const genericError = new Error('Something went wrong') + mockPersistFiles.mockRejectedValue(genericError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow(genericError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.not.toThrow(InvalidComponentStateError) + }) + }) }) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 2241cf805..6da507319 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -1,4 +1,9 @@ +import { ComponentType } from '@defra/forms-model' +import Boom from '@hapi/boom' + +import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { SummaryPageController } from '~/src/server/plugins/engine/pageControllers/SummaryPageController.js' import { buildFormRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' import { type FormSubmissionState } from '~/src/server/plugins/engine/types.js' @@ -9,6 +14,7 @@ import { } from '~/src/server/routes/types.js' import { type CacheService } from '~/src/server/services/cacheService.js' import definition from '~/test/form/definitions/basic.js' +import fileUploadDefinition from '~/test/form/definitions/file-upload-basic.js' describe('SummaryPageController', () => { let model: FormModel @@ -83,4 +89,7 @@ describe('SummaryPageController', () => { expect(saveAndExitMock).toHaveBeenCalledWith(request, h, context) }) }) + + // Note: InvalidComponentStateError handling is comprehensively tested + // in the integration test: test/form/component-state-errors.test.js }) diff --git a/src/server/plugins/engine/pageControllers/errors.test.ts b/src/server/plugins/engine/pageControllers/errors.test.ts new file mode 100644 index 000000000..b76d9dae6 --- /dev/null +++ b/src/server/plugins/engine/pageControllers/errors.test.ts @@ -0,0 +1,63 @@ +import { ComponentType } from '@defra/forms-model' + +import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' +import { TextField } from '~/src/server/plugins/engine/components/TextField.js' +import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' +import definition from '~/test/form/definitions/file-upload-basic.js' + +describe('InvalidComponentStateError', () => { + let model: FormModel + + beforeEach(() => { + model = new FormModel(definition, { + basePath: 'test' + }) + }) + + describe('getStateKeys', () => { + it('should return component name and upload for FileUploadField', () => { + const page = model.pages.find((p) => p.path === '/file-upload-component') + const component = new FileUploadField( + { + name: 'fileUpload', + title: 'Upload something', + type: ComponentType.FileUploadField, + options: {}, + schema: {} + }, + { model, page: page! } + ) + + const error = new InvalidComponentStateError( + component, + 'Test error message' + ) + const stateKeys = error.getStateKeys() + + expect(stateKeys).toEqual(['fileUpload', 'upload']) + }) + + it('should return only component name for non-FileUploadField components', () => { + const page = model.pages.find((p) => p.path === '/file-upload-component') + const component = new TextField( + { + name: 'textField', + title: 'Text field', + type: ComponentType.TextField, + options: {}, + schema: {} + }, + { model, page: page! } + ) + + const error = new InvalidComponentStateError( + component, + 'Test error message' + ) + const stateKeys = error.getStateKeys() + + expect(stateKeys).toEqual(['textField']) + }) + }) +}) From 43b39ad86c91e2365489ac53b9b0f0fdf19fb446 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 11 Dec 2025 10:35:04 +0000 Subject: [PATCH 12/27] Lint and format --- .../engine/components/FileUploadField.test.ts | 65 ++++++++++--------- .../engine/components/FileUploadField.ts | 34 +++++++--- .../engine/components/FormComponent.ts | 19 ++++-- .../SummaryPageController.test.ts | 6 -- .../pageControllers/SummaryPageController.ts | 22 ++++--- .../engine/pageControllers/errors.test.ts | 4 +- src/server/plugins/nunjucks/context.js | 21 +++--- src/server/services/cacheService.ts | 10 ++- src/typings/hapi/index.d.ts | 2 +- 9 files changed, 110 insertions(+), 73 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.test.ts b/src/server/plugins/engine/components/FileUploadField.test.ts index de8be0369..a3a0aebfa 100644 --- a/src/server/plugins/engine/components/FileUploadField.test.ts +++ b/src/server/plugins/engine/components/FileUploadField.test.ts @@ -6,18 +6,22 @@ import { import Boom from '@hapi/boom' import { ComponentCollection } from '~/src/server/plugins/engine/components/ComponentCollection.js' -import { FileUploadField, tempItemSchema } from '~/src/server/plugins/engine/components/FileUploadField.js' +import { + FileUploadField, + tempItemSchema +} from '~/src/server/plugins/engine/components/FileUploadField.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 { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { createPage, type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' -import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { validationOptions as opts } from '~/src/server/plugins/engine/pageControllers/validationOptions.js' +import { type Services } from '~/src/server/plugins/engine/types/index.js' import { FileStatus, UploadStatus, @@ -854,7 +858,7 @@ describe('FileUploadField', () => { const page = model.pages.find((p) => p.path === '/file-upload-component') fileUploadField = new FileUploadField(componentDef, { model, - page: page! + page }) // Mock persistFiles @@ -945,14 +949,15 @@ describe('FileUploadField', () => { await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) - expect(mockPersistFiles).toHaveBeenCalledWith( - expect.any(Array), - '' - ) + expect(mockPersistFiles).toHaveBeenCalledWith(expect.any(Array), '') }) it('should throw Error when formSubmissionService is not available', async () => { - mockRequest.app.model!.services = {} as any + if (!mockRequest.app.model) { + throw new Error('Invalid test setup') + } + + mockRequest.app.model.services = {} as unknown as Services await expect( fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) @@ -967,17 +972,17 @@ describe('FileUploadField', () => { fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) ).rejects.toThrow(InvalidComponentStateError) - try { - await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) - } catch (error) { - expect(error).toBeInstanceOf(InvalidComponentStateError) - expect((error as InvalidComponentStateError).component).toBe( - fileUploadField - ) - expect((error as InvalidComponentStateError).userMessage).toBe( - 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' - ) - } + const error = await fileUploadField + .onSubmit(mockRequest, mockMetadata, mockContext) + .catch((e: unknown) => e) + + expect(error).toBeInstanceOf(InvalidComponentStateError) + expect((error as InvalidComponentStateError).component).toBe( + fileUploadField + ) + expect((error as InvalidComponentStateError).userMessage).toBe( + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) }) it('should throw InvalidComponentStateError when persistFiles throws 410 Gone', async () => { @@ -988,17 +993,17 @@ describe('FileUploadField', () => { fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) ).rejects.toThrow(InvalidComponentStateError) - try { - await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) - } catch (error) { - expect(error).toBeInstanceOf(InvalidComponentStateError) - expect((error as InvalidComponentStateError).component).toBe( - fileUploadField - ) - expect((error as InvalidComponentStateError).userMessage).toBe( - 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' - ) - } + const error = await fileUploadField + .onSubmit(mockRequest, mockMetadata, mockContext) + .catch((e: unknown) => e) + + expect(error).toBeInstanceOf(InvalidComponentStateError) + expect((error as InvalidComponentStateError).component).toBe( + fileUploadField + ) + expect((error as InvalidComponentStateError).userMessage).toBe( + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) }) it('should re-throw other Boom errors without wrapping', async () => { diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index 27a074372..e46076b3f 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -1,4 +1,7 @@ -import { FormMetadata, type FileUploadFieldComponent } from '@defra/forms-model' +import { + type FileUploadFieldComponent, + type FormMetadata +} from '@defra/forms-model' import Boom from '@hapi/boom' import joi, { type ArraySchema } from 'joi' @@ -29,7 +32,10 @@ import { type UploadStatusResponse } from '~/src/server/plugins/engine/types.js' import { render } from '~/src/server/plugins/nunjucks/index.js' -import { type FormQuery, type FormRequestPayload } from '~/src/server/routes/types.js' +import { + type FormQuery, + type FormRequestPayload +} from '~/src/server/routes/types.js' export const uploadIdSchema = joi.string().uuid().required() @@ -287,14 +293,19 @@ export class FileUploadField extends FormComponent { return FileUploadField.getAllPossibleErrors() } - async onSubmit(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { - const notificationEmail = metadata.notificationEmail ?? 'defraforms@defra.gov.uk' - - if(!request.app.model?.services.formSubmissionService) { + async onSubmit( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext + ) { + const notificationEmail = + metadata.notificationEmail ?? 'defraforms@defra.gov.uk' + + if (!request.app.model?.services.formSubmissionService) { throw new Error('No form submission service available in app model') } - - const { formSubmissionService } = request.app.model?.services + + const { formSubmissionService } = request.app.model.services const values = this.getFormValueFromState(context.state) ?? [] const files = values.map((value) => ({ @@ -317,9 +328,12 @@ export class FileUploadField extends FormComponent { // Failed to persist files. We can't recover from this, the only real way we can recover the submissions is // by resetting the problematic components and letting the user re-try. // Scenarios: file missing from S3, invalid retrieval key (timing problem), etc. - throw new InvalidComponentStateError(this, 'There was a problem with your uploaded files. Re-upload them before submitting the form again.') + throw new InvalidComponentStateError( + this, + 'There was a problem with your uploaded files. Re-upload them before submitting the form again.' + ) } - + throw error } } diff --git a/src/server/plugins/engine/components/FormComponent.ts b/src/server/plugins/engine/components/FormComponent.ts index fbf7f4502..015274975 100644 --- a/src/server/plugins/engine/components/FormComponent.ts +++ b/src/server/plugins/engine/components/FormComponent.ts @@ -1,8 +1,15 @@ -import { FormMetadata, type FormComponentsDef, type Item } from '@defra/forms-model' +import { + type FormComponentsDef, + type FormMetadata, + type Item +} from '@defra/forms-model' import { ComponentBase } from '~/src/server/plugins/engine/components/ComponentBase.js' import { optionalText } from '~/src/server/plugins/engine/components/constants.js' -import { type FormContext, type FormRequestPayload } from '~/src/server/plugins/engine/types/index.js' +import { + type FormContext, + type FormRequestPayload +} from '~/src/server/plugins/engine/types/index.js' import { type ErrorMessageTemplateList, type FileState, @@ -222,8 +229,12 @@ export class FormComponent extends ComponentBase { } } - onSubmit(_request: FormRequestPayload, _metadata: FormMetadata, _context: FormContext): void { - // No default implementation + onSubmit( + _request: FormRequestPayload, + _metadata: FormMetadata, + _context: FormContext + ): Promise { + return Promise.resolve() } } diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 6da507319..d9f05baea 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -1,9 +1,4 @@ -import { ComponentType } from '@defra/forms-model' -import Boom from '@hapi/boom' - -import { FileUploadField } from '~/src/server/plugins/engine/components/FileUploadField.js' import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' -import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { SummaryPageController } from '~/src/server/plugins/engine/pageControllers/SummaryPageController.js' import { buildFormRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' import { type FormSubmissionState } from '~/src/server/plugins/engine/types.js' @@ -14,7 +9,6 @@ import { } from '~/src/server/routes/types.js' import { type CacheService } from '~/src/server/services/cacheService.js' import definition from '~/test/form/definitions/basic.js' -import fileUploadDefinition from '~/test/form/definitions/file-upload-basic.js' describe('SummaryPageController', () => { let model: FormModel diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index ac8504fdb..623cd4660 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -157,7 +157,10 @@ export class SummaryPageController extends QuestionPageController { ) } catch (error) { if (error instanceof InvalidComponentStateError) { - const govukError = createError(error.component.name, error.userMessage) + const govukError = createError( + error.component.name, + error.userMessage + ) request.yar.flash(COMPONENT_STATE_ERROR, govukError, true) @@ -216,8 +219,6 @@ export async function submitForm( summaryViewModel.details ) - - // Submit data request.logger.info(logTags, 'Submitting data') const submitResponse = await submitData( @@ -248,19 +249,22 @@ export async function submitForm( * Examples include: * - file uploads which are 'persisted' before submission * - payments which are 'captured' before submission - * @param model - * @param request - * @param context */ -async function finaliseComponents(request: FormRequestPayload, metadata: FormMetadata, context: FormContext) { - const relevantPages = context.relevantPages.flatMap((page) => page.collection.fields) +async function finaliseComponents( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext +) { + const relevantPages = context.relevantPages.flatMap( + (page) => page.collection.fields + ) for (const component of relevantPages) { /* Each component will throw InvalidComponent if its state is invalid, which is handled by handleFormSubmit */ - await component.onSubmit(request, metadata, context) + await component.onSubmit(request, metadata, context) } } diff --git a/src/server/plugins/engine/pageControllers/errors.test.ts b/src/server/plugins/engine/pageControllers/errors.test.ts index b76d9dae6..efbf4a548 100644 --- a/src/server/plugins/engine/pageControllers/errors.test.ts +++ b/src/server/plugins/engine/pageControllers/errors.test.ts @@ -26,7 +26,7 @@ describe('InvalidComponentStateError', () => { options: {}, schema: {} }, - { model, page: page! } + { model, page } ) const error = new InvalidComponentStateError( @@ -48,7 +48,7 @@ describe('InvalidComponentStateError', () => { options: {}, schema: {} }, - { model, page: page! } + { model, page: page } ) const error = new InvalidComponentStateError( diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index 51715fd6d..b063eecc5 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -4,10 +4,9 @@ import { basename, join } from 'node:path' import Boom from '@hapi/boom' import { StatusCodes } from 'http-status-codes' -import { COMPONENT_STATE_ERROR } from '~/src/server/constants.js' - import { config } from '~/src/config/index.js' import { createLogger } from '~/src/server/common/helpers/logging/logger.js' +import { COMPONENT_STATE_ERROR } from '~/src/server/constants.js' import { checkFormStatus, encodeUrl, @@ -49,7 +48,9 @@ export async function context(request) { } const flashedError = request.yar.flash(COMPONENT_STATE_ERROR) - const flashedErrors = !Array.isArray(flashedError) ? [flashedError] : undefined + const flashedErrors = !Array.isArray(flashedError) + ? [flashedError] + : undefined /** @type {ViewContext} */ const ctx = { @@ -64,11 +65,15 @@ export async function context(request) { flashedErrors } - // @ts-expect-error temporary code while developing -- TODO remove - await request.yar.commit(/** @type {import('@hapi/hapi').ResponseToolkit} */ ({ - // @ts-ignore - state: () => {} - })) + // TODO remove this temporary hack + await request.yar.commit( + /** @type {import('@hapi/hapi').ResponseToolkit} */ ( + /** @type {unknown} */ ({ + // eslint-disable-next-line @typescript-eslint/no-empty-function + state: () => {} + }) + ) + ) return ctx } diff --git a/src/server/services/cacheService.ts b/src/server/services/cacheService.ts index 91d7d6261..c1039063c 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -99,11 +99,15 @@ export class CacheService { request.yar.flash(key.id, message) } - async resetComponentStates(request: AnyFormRequest, componentNames: string[]) { + async resetComponentStates( + request: AnyFormRequest, + componentNames: string[] + ) { const state = await this.getState(request) - for(const componentId of componentNames) { - if (state[componentId]) { + for (const componentId of componentNames) { + if (componentId in state) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete delete state[componentId] } } diff --git a/src/typings/hapi/index.d.ts b/src/typings/hapi/index.d.ts index 37ad6408c..ef364f63b 100644 --- a/src/typings/hapi/index.d.ts +++ b/src/typings/hapi/index.d.ts @@ -5,7 +5,7 @@ import { type ServerYar, type Yar } from '@hapi/yar' import { type Logger } from 'pino' import { - COMPONENT_STATE_ERROR, + type COMPONENT_STATE_ERROR, type EXTERNAL_STATE_APPENDAGE, type EXTERNAL_STATE_PAYLOAD } from '~/src/server/constants.js' From afc61a37d76ec0711d2985e38376e6bf6bf461f6 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 11 Dec 2025 12:52:16 +0000 Subject: [PATCH 13/27] fix tests failing because of missing yar mock --- src/server/plugins/nunjucks/context.test.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/nunjucks/context.test.js b/src/server/plugins/nunjucks/context.test.js index 35fec85f8..351623f77 100644 --- a/src/server/plugins/nunjucks/context.test.js +++ b/src/server/plugins/nunjucks/context.test.js @@ -87,7 +87,11 @@ describe('Nunjucks context', () => { } }, path: '/test', - url: { search: '' } + url: { search: '' }, + yar: { + flash: jest.fn().mockReturnValue([]), + commit: jest.fn() + } // state intentionally omitted to test real malformed requests }) ) @@ -121,7 +125,11 @@ describe('Nunjucks context', () => { }, path: '/test', url: { search: '' }, - state: {} + state: {}, + yar: { + flash: jest.fn().mockReturnValue([]), + commit: jest.fn() + } }) ) From 1e788878c791d1dcb71a5f60b5af48b6a71f7956 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 11 Dec 2025 13:43:13 +0000 Subject: [PATCH 14/27] test that metadata email is used for file persistence --- test/fixtures/form.js | 17 +++++++++++++++++ test/form/persist-files.test.js | 4 +++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/test/fixtures/form.js b/test/fixtures/form.js index ef4e5e66c..577156ec4 100644 --- a/test/fixtures/form.js +++ b/test/fixtures/form.js @@ -40,6 +40,23 @@ export const metadata = { updatedBy: author } +/** + * @satisfies {FormMetadata} + */ +export const metadataWithNotificationEmail = { + id: '661e4ca5039739ef2902b214', + slug: 'test-form', + title: 'Test form', + organisation: 'Defra', + teamName: 'Defra Forms', + teamEmail: 'defraforms@defra.gov.uk', + createdAt: now, + createdBy: author, + updatedAt: now, + updatedBy: author, + notificationEmail: 'enrique.chase@defra.gov.uk' +} + /** * @satisfies {FormDefinition} */ diff --git a/test/form/persist-files.test.js b/test/form/persist-files.test.js index 134a84703..37f893820 100644 --- a/test/form/persist-files.test.js +++ b/test/form/persist-files.test.js @@ -128,7 +128,9 @@ describe('Submission journey test', () => { .mockResolvedValueOnce(readyFile.status) .mockResolvedValueOnce(readyFile2.status) - jest.mocked(getFormMetadata).mockResolvedValue(fixtures.form.metadata) + jest + .mocked(getFormMetadata) + .mockResolvedValue(fixtures.form.metadataWithNotificationEmail) jest.mocked(uploadService.initiateUpload).mockResolvedValueOnce({ uploadId: '123-546-790', From 17ac0b9ade44eb9a67fde5db48535e444d36e8df Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 11 Dec 2025 13:51:37 +0000 Subject: [PATCH 15/27] fix whitespace issue --- .../plugins/engine/pageControllers/SummaryPageController.ts | 2 +- src/server/plugins/engine/pageControllers/errors.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 623cd4660..c15fe86d8 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -260,7 +260,7 @@ async function finaliseComponents( ) for (const component of relevantPages) { - /* + /* Each component will throw InvalidComponent if its state is invalid, which is handled by handleFormSubmit */ diff --git a/src/server/plugins/engine/pageControllers/errors.test.ts b/src/server/plugins/engine/pageControllers/errors.test.ts index efbf4a548..ac7c3910b 100644 --- a/src/server/plugins/engine/pageControllers/errors.test.ts +++ b/src/server/plugins/engine/pageControllers/errors.test.ts @@ -48,7 +48,7 @@ describe('InvalidComponentStateError', () => { options: {}, schema: {} }, - { model, page: page } + { model, page } ) const error = new InvalidComponentStateError( From 001cf3ca0b76907113fa59c2789f9ab1ac9ffd5d Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Mon, 15 Dec 2025 20:14:50 +0000 Subject: [PATCH 16/27] abandon definition.outputEmail in favour of metadata.notificationEmail --- .../forms/register-as-a-unicorn-breeder.yaml | 1 - .../FileUploadPageController.test.ts | 9 +++++++++ .../pageControllers/FileUploadPageController.ts | 14 ++++++++++---- .../pageControllers/SummaryPageController.ts | 7 +++---- src/server/plugins/engine/routes/index.ts | 2 +- test/condition/text.test.js | 6 ++++++ test/fixtures/form.js | 9 ++++----- test/form/component-state-errors.test.js | 4 ++-- test/form/definitions/basic.js | 3 +-- test/form/definitions/components-optional.json | 3 +-- test/form/definitions/components.json | 3 +-- test/form/definitions/conditional-reveal.js | 3 +-- test/form/definitions/conditions-basic.js | 6 ++---- test/form/definitions/conditions-complex.js | 3 +-- test/form/definitions/conditions-dates.js | 3 +-- test/form/definitions/demo-cph-number.js | 3 +-- test/form/definitions/fields-optional.js | 3 +-- test/form/definitions/fields-required.js | 3 +-- test/form/definitions/file-upload-basic.js | 3 +-- test/form/definitions/file-upload.js | 3 +-- test/form/definitions/minimal.js | 3 +-- test/form/definitions/plugin-options.js | 3 +-- test/form/definitions/repeat.js | 3 +-- test/form/journey-basic.test.js | 14 +++++++++++++- test/form/summary-submission-email.test.js | 6 +++++- 25 files changed, 69 insertions(+), 51 deletions(-) diff --git a/src/server/forms/register-as-a-unicorn-breeder.yaml b/src/server/forms/register-as-a-unicorn-breeder.yaml index a92b727ab..fa653641d 100644 --- a/src/server/forms/register-as-a-unicorn-breeder.yaml +++ b/src/server/forms/register-as-a-unicorn-breeder.yaml @@ -306,5 +306,4 @@ lists: value: Aquatic - text: Rainbow value: Rainbow -outputEmail: defraforms@defra.gov.uk startPage: '/whats-your-name' diff --git a/src/server/plugins/engine/pageControllers/FileUploadPageController.test.ts b/src/server/plugins/engine/pageControllers/FileUploadPageController.test.ts index 1f5c5bcbe..14f82d0bb 100644 --- a/src/server/plugins/engine/pageControllers/FileUploadPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/FileUploadPageController.test.ts @@ -15,6 +15,7 @@ import { import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/QuestionPageController.js' import { serverWithSaveAndExit } from '~/src/server/plugins/engine/pageControllers/__stubs__/server.js' import * as pageHelpers from '~/src/server/plugins/engine/pageControllers/helpers/index.js' +import { getFormMetadata } from '~/src/server/plugins/engine/services/formsService.js' import * as uploadService from '~/src/server/plugins/engine/services/uploadService.js' import { FileStatus, @@ -33,8 +34,11 @@ import { type FormResponseToolkit } from '~/src/server/routes/types.js' import { type CacheService } from '~/src/server/services/index.js' +import * as fixtures from '~/test/fixtures/index.js' import definition from '~/test/form/definitions/file-upload-basic.js' +jest.mock('~/src/server/plugins/engine/services/formsService.js') + type TestableFileUploadPageController = FileUploadPageController & { initiateAndStoreNewUpload( req: FormRequest, @@ -65,8 +69,13 @@ describe('FileUploadPageController', () => { basePath: 'test' }) + jest.mocked(getFormMetadata).mockResolvedValue(fixtures.form.metadata) + controller = new FileUploadPageController(model, pages[0]) request = { + params: { + slug: 'test-form' + }, logger: { info: jest.fn(), error: jest.fn(), diff --git a/src/server/plugins/engine/pageControllers/FileUploadPageController.ts b/src/server/plugins/engine/pageControllers/FileUploadPageController.ts index 3bf51f8fc..364ee54a9 100644 --- a/src/server/plugins/engine/pageControllers/FileUploadPageController.ts +++ b/src/server/plugins/engine/pageControllers/FileUploadPageController.ts @@ -15,6 +15,7 @@ import { import { type FormModel } from '~/src/server/plugins/engine/models/index.js' import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/QuestionPageController.js' import { getProxyUrlForLocalDevelopment } from '~/src/server/plugins/engine/pageControllers/helpers/index.js' +import { getFormMetadata } from '~/src/server/plugins/engine/services/formsService.js' import { getUploadStatus, initiateUpload @@ -433,10 +434,15 @@ export class FileUploadPageController extends QuestionPageController { const max = Math.min(schema.max ?? MAX_UPLOADS, MAX_UPLOADS) if (files.length < max) { - const outputEmail = - this.model.def.outputEmail ?? 'defraforms@defra.gov.uk' - - const newUpload = await initiateUpload(href, outputEmail, options.accept) + const formMetadata = await getFormMetadata(request.params.slug) + const notificationEmail = + formMetadata.notificationEmail ?? 'defraforms@defra.gov.uk' + + const newUpload = await initiateUpload( + href, + notificationEmail, + options.accept + ) if (newUpload === undefined) { throw Boom.badRequest('Unexpected empty response from initiateUpload') diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index c15fe86d8..5fc6f5b72 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -137,12 +137,11 @@ export class SummaryPageController extends QuestionPageController { const formMetadata = await getFormMetadata(params.slug) const { notificationEmail } = formMetadata const { isPreview } = checkFormStatus(request.params) - const emailAddress = notificationEmail ?? this.model.def.outputEmail - checkEmailAddressForLiveFormSubmission(emailAddress, isPreview) + checkEmailAddressForLiveFormSubmission(notificationEmail, isPreview) // Send submission email - if (emailAddress) { + if (notificationEmail) { const viewModel = this.getSummaryViewModel(request, context) try { @@ -152,7 +151,7 @@ export class SummaryPageController extends QuestionPageController { request, viewModel, model, - emailAddress, + notificationEmail, formMetadata ) } catch (error) { diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 250d005eb..dcdccd923 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -223,7 +223,7 @@ export function makeLoadFormPreHandler(server: Server, options: PluginOptions) { ) } - const emailAddress = metadata.notificationEmail ?? definition.outputEmail + const emailAddress = metadata.notificationEmail checkEmailAddressForLiveFormSubmission(emailAddress, isPreview) diff --git a/test/condition/text.test.js b/test/condition/text.test.js index 473895757..994990b1a 100644 --- a/test/condition/text.test.js +++ b/test/condition/text.test.js @@ -36,6 +36,12 @@ describe('TextField based conditions', () => { }) test('TextField is rendered', async () => { + // Test scenario: missing notificationEmail should show warning + jest.mocked(getFormMetadata).mockResolvedValue({ + ...fixtures.form.metadata, + notificationEmail: undefined + }) + const { container } = await renderResponse(server, { url: `${basePath}/first-page` }) diff --git a/test/fixtures/form.js b/test/fixtures/form.js index 577156ec4..780cefd70 100644 --- a/test/fixtures/form.js +++ b/test/fixtures/form.js @@ -37,7 +37,8 @@ export const metadata = { createdAt: now, createdBy: author, updatedAt: now, - updatedBy: author + updatedBy: author, + notificationEmail: 'enrique.chase@defra.gov.uk' } /** @@ -94,8 +95,7 @@ export const definition = { } ], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] } export const componentId = '1491981d-99cd-485e-ab4a-f88275edeadc' @@ -138,8 +138,7 @@ export const definitionWithComponentId = { } ], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] } /** diff --git a/test/form/component-state-errors.test.js b/test/form/component-state-errors.test.js index fc8a6c9d0..d80c1dbec 100644 --- a/test/form/component-state-errors.test.js +++ b/test/form/component-state-errors.test.js @@ -137,7 +137,7 @@ describe('Component State Error Tests - File Upload', () => { initiatedRetrievalKey: readyFile.status.metadata.retrievalKey } ], - 'defraforms@defra.gov.uk' + 'enrique.chase@defra.gov.uk' ) // Verify submit was NOT called @@ -213,7 +213,7 @@ describe('Component State Error Tests - File Upload', () => { initiatedRetrievalKey: readyFile.status.metadata.retrievalKey } ], - 'defraforms@defra.gov.uk' + 'enrique.chase@defra.gov.uk' ) // Verify submit was NOT called diff --git a/test/form/definitions/basic.js b/test/form/definitions/basic.js index 475c17315..e3817d9b2 100644 --- a/test/form/definitions/basic.js +++ b/test/form/definitions/basic.js @@ -100,8 +100,7 @@ export default /** @satisfies {FormDefinition} */ ({ }) ] }) - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/components-optional.json b/test/form/definitions/components-optional.json index 1af9fc430..55676cc60 100644 --- a/test/form/definitions/components-optional.json +++ b/test/form/definitions/components-optional.json @@ -497,6 +497,5 @@ { "text": "4 points", "value": 4 } ] } - ], - "outputEmail": "enrique.chase@defra.gov.uk" + ] } diff --git a/test/form/definitions/components.json b/test/form/definitions/components.json index 313af108f..562dfcce8 100644 --- a/test/form/definitions/components.json +++ b/test/form/definitions/components.json @@ -463,6 +463,5 @@ { "text": "4 points", "value": 4 } ] } - ], - "outputEmail": "enrique.chase@defra.gov.uk" + ] } diff --git a/test/form/definitions/conditional-reveal.js b/test/form/definitions/conditional-reveal.js index a72aba6de..460fd0a0b 100644 --- a/test/form/definitions/conditional-reveal.js +++ b/test/form/definitions/conditional-reveal.js @@ -131,8 +131,7 @@ export default /** @satisfies {FormDefinition} */ ({ ] } } - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/conditions-basic.js b/test/form/definitions/conditions-basic.js index dc0581e4d..c52fef601 100644 --- a/test/form/definitions/conditions-basic.js +++ b/test/form/definitions/conditions-basic.js @@ -92,8 +92,7 @@ export default /** @satisfies {FormDefinition} */ ({ ] } } - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) export const V2 = /** @satisfies {FormDefinition} */ ({ @@ -172,8 +171,7 @@ export const V2 = /** @satisfies {FormDefinition} */ ({ } ] } - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/conditions-complex.js b/test/form/definitions/conditions-complex.js index 141ed2d13..672b25614 100644 --- a/test/form/definitions/conditions-complex.js +++ b/test/form/definitions/conditions-complex.js @@ -329,8 +329,7 @@ export default /** @satisfies {FormDefinition} */ ({ ] } } - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/conditions-dates.js b/test/form/definitions/conditions-dates.js index 1daebe4da..6072e8f36 100644 --- a/test/form/definitions/conditions-dates.js +++ b/test/form/definitions/conditions-dates.js @@ -69,8 +69,7 @@ export default /** @satisfies {FormDefinition} */ ({ ] } } - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/demo-cph-number.js b/test/form/definitions/demo-cph-number.js index 4292b4c98..12940795c 100644 --- a/test/form/definitions/demo-cph-number.js +++ b/test/form/definitions/demo-cph-number.js @@ -3090,8 +3090,7 @@ export default /** @satisfies {FormDefinition} */ ({ ] } } - ], - outputEmail: 'cph_online_form@rpa.gov.uk' + ] }) /** diff --git a/test/form/definitions/fields-optional.js b/test/form/definitions/fields-optional.js index beaca2f1a..6c9c555af 100644 --- a/test/form/definitions/fields-optional.js +++ b/test/form/definitions/fields-optional.js @@ -410,8 +410,7 @@ export default /** @satisfies {FormDefinition} */ ({ } ], sections: [], - conditions: [], - outputEmail: 'enrique.chase@defra.gov.uk' + conditions: [] }) /** diff --git a/test/form/definitions/fields-required.js b/test/form/definitions/fields-required.js index fa5ea16e3..6c07a252b 100644 --- a/test/form/definitions/fields-required.js +++ b/test/form/definitions/fields-required.js @@ -410,8 +410,7 @@ export default /** @satisfies {FormDefinition} */ ({ } ], sections: [], - conditions: [], - outputEmail: 'enrique.chase@defra.gov.uk' + conditions: [] }) /** diff --git a/test/form/definitions/file-upload-basic.js b/test/form/definitions/file-upload-basic.js index f6f140d7e..90e755eb5 100644 --- a/test/form/definitions/file-upload-basic.js +++ b/test/form/definitions/file-upload-basic.js @@ -35,8 +35,7 @@ export default /** @satisfies {FormDefinition} */ ({ ]), lists: [], sections: [], - conditions: [], - outputEmail: 'enrique.chase@defra.gov.uk' + conditions: [] }) /** diff --git a/test/form/definitions/file-upload.js b/test/form/definitions/file-upload.js index e044d036c..4c101585e 100644 --- a/test/form/definitions/file-upload.js +++ b/test/form/definitions/file-upload.js @@ -57,8 +57,7 @@ export default /** @satisfies {FormDefinition} */ ({ } ], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] }) /** diff --git a/test/form/definitions/minimal.js b/test/form/definitions/minimal.js index 46f107f74..31c038faa 100644 --- a/test/form/definitions/minimal.js +++ b/test/form/definitions/minimal.js @@ -30,8 +30,7 @@ export default /** @satisfies {FormDefinition} */ ({ ], sections: [], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] }) /** diff --git a/test/form/definitions/plugin-options.js b/test/form/definitions/plugin-options.js index 4f7d1a2b5..dd8d6f70c 100644 --- a/test/form/definitions/plugin-options.js +++ b/test/form/definitions/plugin-options.js @@ -88,8 +88,7 @@ export default /** @satisfies {FormDefinition} */ ({ }) ] }) - ], - outputEmail: 'enrique.chase@defra.gov.uk' + ] }) /** diff --git a/test/form/definitions/repeat.js b/test/form/definitions/repeat.js index 4748c51c9..c2c615663 100644 --- a/test/form/definitions/repeat.js +++ b/test/form/definitions/repeat.js @@ -61,8 +61,7 @@ export default /** @satisfies {FormDefinition} */ ({ } ], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] }) /** diff --git a/test/form/journey-basic.test.js b/test/form/journey-basic.test.js index ff2d27fb9..a004c0ed5 100644 --- a/test/form/journey-basic.test.js +++ b/test/form/journey-basic.test.js @@ -265,7 +265,19 @@ describe('Form journey', () => { ) }) - it('should render the page heading with email notification warning', () => { + it('should render the page heading with email notification warning', async () => { + // Override the mock to remove notificationEmail so the warning appears + jest.mocked(getFormMetadata).mockResolvedValueOnce({ + ...fixtures.form.metadata, + notificationEmail: undefined + }) + + // Re-render the page without notificationEmail + const { container } = await renderResponse(server, { + url: `${basePath}/summary`, + headers + }) + const $heading = container.getByRole('heading', { name: 'Summary', level: 1 diff --git a/test/form/summary-submission-email.test.js b/test/form/summary-submission-email.test.js index 295f7b501..1ca97de08 100644 --- a/test/form/summary-submission-email.test.js +++ b/test/form/summary-submission-email.test.js @@ -58,7 +58,11 @@ describe('Page: /summary', () => { }) it('should render the page with email notification warning', async () => { - jest.mocked(getFormMetadata).mockResolvedValue(fixtures.form.metadata) + // Test scenario: missing notificationEmail should show warning + jest.mocked(getFormMetadata).mockResolvedValue({ + ...fixtures.form.metadata, + notificationEmail: undefined + }) const { container } = await renderResponse(server, { url: `${basePath}/summary`, From 986f58cbe8a83a2d00ffbd775d8e46f1412d90b1 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 16 Dec 2025 09:37:51 +0000 Subject: [PATCH 17/27] Remove mise.toml --- mise.toml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 mise.toml diff --git a/mise.toml b/mise.toml deleted file mode 100644 index db60d29dc..000000000 --- a/mise.toml +++ /dev/null @@ -1,3 +0,0 @@ -[tools] -1password = "latest" -node = "latest" From 3492e017a7263384139ec46ef5defd05ef5ff1d1 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Tue, 16 Dec 2025 10:47:58 +0000 Subject: [PATCH 18/27] resolve test failure following merge --- src/server/plugins/engine/beta/form-context.test.ts | 7 ++++--- src/server/plugins/engine/beta/form-context.ts | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/server/plugins/engine/beta/form-context.test.ts b/src/server/plugins/engine/beta/form-context.test.ts index 6aa3ecef8..b0c8ad9b6 100644 --- a/src/server/plugins/engine/beta/form-context.test.ts +++ b/src/server/plugins/engine/beta/form-context.test.ts @@ -209,11 +209,12 @@ describe('getFormModel helper', () => { describe('resolveFormModel helper', () => { const slug = 'tb-origin' - const definition = { pages: [], outputEmail: 'fallback@example.com' } + const definition = { pages: [] } const metadata = { id: 'metadata-123', live: { updatedAt: new Date('2024-10-15T10:00:00Z') }, - versions: [{ versionNumber: 9 }] + versions: [{ versionNumber: 9 }], + notificationEmail: 'enrique.chase@defra.gov.uk' } let server: Request['server'] let formModelInstance: { id: string } @@ -265,7 +266,7 @@ describe('resolveFormModel helper', () => { expect(FormModel).toHaveBeenCalledTimes(2) expect(mockFormsService.getFormDefinition).toHaveBeenCalledTimes(2) expect(mockCheckEmailAddressForLiveFormSubmission).toHaveBeenCalledWith( - definition.outputEmail, + metadata.notificationEmail, true ) expect(FormModel).toHaveBeenCalledWith( diff --git a/src/server/plugins/engine/beta/form-context.ts b/src/server/plugins/engine/beta/form-context.ts index a28f023af..304a1de5f 100644 --- a/src/server/plugins/engine/beta/form-context.ts +++ b/src/server/plugins/engine/beta/form-context.ts @@ -174,9 +174,10 @@ export async function resolveFormModel( ) } - const emailAddress = metadata.notificationEmail - - checkEmailAddressForLiveFormSubmission(emailAddress, isPreview) + checkEmailAddressForLiveFormSubmission( + metadata.notificationEmail, + isPreview + ) const routePrefix = options.routePrefix ?? server.realm.modifiers.route.prefix From 585a89f9bdcbeae83f7aa2efcb5c02208c03640c Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 17 Dec 2025 10:52:13 +0000 Subject: [PATCH 19/27] move flashed errors into QuestionPageController so yar can commit the operation (requires hapi toolkit) --- .../FileUploadPageController.ts | 3 ++- .../pageControllers/QuestionPageController.ts | 8 ++++++++ src/server/plugins/engine/types.ts | 3 ++- src/server/plugins/nunjucks/context.js | 20 +------------------ 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/FileUploadPageController.ts b/src/server/plugins/engine/pageControllers/FileUploadPageController.ts index 364ee54a9..928ba86fc 100644 --- a/src/server/plugins/engine/pageControllers/FileUploadPageController.ts +++ b/src/server/plugins/engine/pageControllers/FileUploadPageController.ts @@ -15,7 +15,6 @@ import { import { type FormModel } from '~/src/server/plugins/engine/models/index.js' import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/QuestionPageController.js' import { getProxyUrlForLocalDevelopment } from '~/src/server/plugins/engine/pageControllers/helpers/index.js' -import { getFormMetadata } from '~/src/server/plugins/engine/services/formsService.js' import { getUploadStatus, initiateUpload @@ -372,6 +371,7 @@ export class FileUploadPageController extends QuestionPageController { // Flash the error message. const { fileUpload } = this const cacheService = getCacheService(request.server) + const name = fileUpload.name const text = file.errorMessage ?? 'Unknown error' const errors: FormSubmissionError[] = [ @@ -424,6 +424,7 @@ export class FileUploadPageController extends QuestionPageController { ) { const { fileUpload, href, path } = this const { options, schema } = fileUpload + const { getFormMetadata } = this.model.services.formsService const files = this.getFilesFromState(state) diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index ec778d6a1..c63b8b297 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.ts @@ -13,6 +13,7 @@ import { type RouteOptions } from '@hapi/hapi' import { type ValidationErrorItem } from 'joi' import { + COMPONENT_STATE_ERROR, EXTERNAL_STATE_APPENDAGE, EXTERNAL_STATE_PAYLOAD } from '~/src/server/constants.js' @@ -413,6 +414,13 @@ export class QuestionPageController extends PageController { const viewModel = this.getViewModel(request, context) viewModel.errors = collection.getViewErrors(viewModel.errors) + const flashedError = request.yar.flash(COMPONENT_STATE_ERROR) + const flashedErrors = !Array.isArray(flashedError) + ? [flashedError] + : undefined + + viewModel.flashedErrors = flashedErrors + /** * Content components can be hidden based on a condition. If the condition evaluates to true, it is safe to be kept, otherwise discard it */ diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 447a5b1db..272119864 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -325,7 +325,8 @@ export interface ItemDeletePageViewModel extends PageViewModelBase { export interface FormPageViewModel extends PageViewModelBase { components: ComponentViewModel[] context: FormContext - errors?: FormSubmissionError[] + errors?: FormSubmissionError[] // errors relating to form state + flashedErrors?: FormSubmissionError[] // errors relating to session or external state hasMissingNotificationEmail?: boolean allowSaveAndExit: boolean } diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index b063eecc5..ef5b9dfca 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -6,7 +6,6 @@ import { StatusCodes } from 'http-status-codes' import { config } from '~/src/config/index.js' import { createLogger } from '~/src/server/common/helpers/logging/logger.js' -import { COMPONENT_STATE_ERROR } from '~/src/server/constants.js' import { checkFormStatus, encodeUrl, @@ -47,11 +46,6 @@ export async function context(request) { consumerViewContext = await pluginStorage.viewContext(request) } - const flashedError = request.yar.flash(COMPONENT_STATE_ERROR) - const flashedErrors = !Array.isArray(flashedError) - ? [flashedError] - : undefined - /** @type {ViewContext} */ const ctx = { // take consumers props first so we can override it @@ -60,21 +54,9 @@ export async function context(request) { crumb: safeGenerateCrumb(request), currentPath: `${request.path}${request.url.search}`, previewMode: isPreviewMode ? formState : undefined, - slug: isResponseOK ? params?.slug : undefined, - // @ts-expect-error TODO fix and merge into 'errors' to link back to the form component - flashedErrors + slug: isResponseOK ? params?.slug : undefined } - // TODO remove this temporary hack - await request.yar.commit( - /** @type {import('@hapi/hapi').ResponseToolkit} */ ( - /** @type {unknown} */ ({ - // eslint-disable-next-line @typescript-eslint/no-empty-function - state: () => {} - }) - ) - ) - return ctx } From dd71fcdfb31ad1bfe16eb160a4b1d4ed33a8dda4 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 17 Dec 2025 11:17:02 +0000 Subject: [PATCH 20/27] consolidate flashedErrors into errors --- .../pageControllers/QuestionPageController.ts | 6 ++---- .../pageControllers/__stubs__/request.ts | 18 ++++++++++++++---- src/server/plugins/engine/types.ts | 3 +-- src/server/plugins/engine/views/index.html | 15 ++------------- 4 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index c63b8b297..d6186fd22 100644 --- a/src/server/plugins/engine/pageControllers/QuestionPageController.ts +++ b/src/server/plugins/engine/pageControllers/QuestionPageController.ts @@ -415,11 +415,9 @@ export class QuestionPageController extends PageController { viewModel.errors = collection.getViewErrors(viewModel.errors) const flashedError = request.yar.flash(COMPONENT_STATE_ERROR) - const flashedErrors = !Array.isArray(flashedError) - ? [flashedError] - : undefined + const flashedErrors = !Array.isArray(flashedError) ? [flashedError] : [] - viewModel.flashedErrors = flashedErrors + viewModel.errors = (viewModel.errors ?? []).concat(flashedErrors) /** * Content components can be hidden based on a condition. If the condition evaluates to true, it is safe to be kept, otherwise discard it diff --git a/src/server/plugins/engine/pageControllers/__stubs__/request.ts b/src/server/plugins/engine/pageControllers/__stubs__/request.ts index 3e7bc877a..2f89ca741 100644 --- a/src/server/plugins/engine/pageControllers/__stubs__/request.ts +++ b/src/server/plugins/engine/pageControllers/__stubs__/request.ts @@ -2,20 +2,30 @@ import { server } from '~/src/server/plugins/engine/pageControllers/__stubs__/se import { type FormContextRequest } from '~/src/server/plugins/engine/types.js' import { type FormRequest } from '~/src/server/routes/types.js' +const mockYar = { + flash: jest.fn().mockReturnValue([]), + clear: jest.fn(), + get: jest.fn(), + set: jest.fn(), + reset: jest.fn() +} + export function buildFormRequest( - request: Omit + request: Omit ): FormRequest { return { ...request, + yar: mockYar, server - } as FormRequest + } as unknown as FormRequest } export function buildFormContextRequest( - request: Omit + request: Omit ): FormContextRequest { return { ...request, + yar: mockYar, server - } as FormContextRequest + } as unknown as FormContextRequest } diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 272119864..447a5b1db 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -325,8 +325,7 @@ export interface ItemDeletePageViewModel extends PageViewModelBase { export interface FormPageViewModel extends PageViewModelBase { components: ComponentViewModel[] context: FormContext - errors?: FormSubmissionError[] // errors relating to form state - flashedErrors?: FormSubmissionError[] // errors relating to session or external state + errors?: FormSubmissionError[] hasMissingNotificationEmail?: boolean allowSaveAndExit: boolean } diff --git a/src/server/plugins/engine/views/index.html b/src/server/plugins/engine/views/index.html index a5fa69b03..c5cc8d8f3 100644 --- a/src/server/plugins/engine/views/index.html +++ b/src/server/plugins/engine/views/index.html @@ -10,21 +10,10 @@ {% include "partials/preview-banner.html" %} {% endif %} - {% set allErrors = [] %} - - {% if errors %} - {% set allErrors = allErrors.concat(errors) %} - {% endif %} - - {% if flashedErrors %} - {% set allErrors = allErrors.concat(flashedErrors) %} - {% endif %} - - - {% if allErrors | length %} + {% if errors | length %} {{ govukErrorSummary({ titleText: "There is a problem", - errorList: checkErrorTemplates(allErrors) + errorList: checkErrorTemplates(errors) }) }} {% endif %} From 1db48cbf2e94b21a05c402ca5702f226ec5c20b0 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 17 Dec 2025 12:50:27 +0000 Subject: [PATCH 21/27] fix failing test --- src/server/plugins/engine/beta/form-context.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/plugins/engine/beta/form-context.test.ts b/src/server/plugins/engine/beta/form-context.test.ts index b0c8ad9b6..609441d88 100644 --- a/src/server/plugins/engine/beta/form-context.test.ts +++ b/src/server/plugins/engine/beta/form-context.test.ts @@ -266,7 +266,7 @@ describe('resolveFormModel helper', () => { expect(FormModel).toHaveBeenCalledTimes(2) expect(mockFormsService.getFormDefinition).toHaveBeenCalledTimes(2) expect(mockCheckEmailAddressForLiveFormSubmission).toHaveBeenCalledWith( - metadata.notificationEmail, + undefined, true ) expect(FormModel).toHaveBeenCalledWith( From dc004ebd77551e1cddbe5e302503a330fba4c62b Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Wed, 17 Dec 2025 13:38:40 +0000 Subject: [PATCH 22/27] Document components --- docs/features/code-based/COMPONENTS.md | 144 +++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 docs/features/code-based/COMPONENTS.md diff --git a/docs/features/code-based/COMPONENTS.md b/docs/features/code-based/COMPONENTS.md new file mode 100644 index 000000000..a1d060490 --- /dev/null +++ b/docs/features/code-based/COMPONENTS.md @@ -0,0 +1,144 @@ +--- +layout: default +title: Components +parent: Code-based Features +grand_parent: Features +nav_order: 6 +--- + +# Components + +This guide covers key concepts for developing components. + +## Overview + +Components are the building blocks of forms in the engine. Components usually extend from two base classes: + +- ComponentBase: general components that display content on the page, such as Markdown or HTML components. + +- FormComponents: these components are specialised components that take user input. They represent individual form fields and controls (text inputs, file uploads, radios, checkboxes, etc.) and handle their own validation, state management, and rendering. + +> [!NOTE] +> Custom components are currently not supported when registering the plugin. Whilst you can develop custom pages as a plugin consumer, custom components must be built into the core engine. + +## Post-processing component state on form submission + +### Overview + +The `InvalidComponentStateError` is a specialised exception thrown by form components during submission when their external state has become invalid or inconsistent with their internal state. This mechanism provides a graceful recovery path for components that interact with external systems, allowing the form engine to reset the component's state and prompt the user to re-enter data rather than failing the entire submission. + +### Why it's necessary + +#### Internal vs external state + +The forms engine automatically validates **internal state** - data stored in the user's session that represents their answers to form fields. However, some components maintain **external state** in addition to internal state: + +- **Internal State**: Data stored in the user's session (e.g., file upload IDs, reference numbers) +- **External State**: Data stored in external systems (e.g., files in S3, payment records in a payment provider) + +#### The problem + +External state can become invalid between the time a user enters data and when they submit the form. For example: + +- **File Uploads**: A user uploads files, receiving file IDs representing an external resource. Those files are stored in S3 with a retrieval key. Later, when submitting the form: + - The files may have expired (TTL exceeded) + - The retrieval key may have become invalid + - The files may have been deleted from S3 +- **Payments**: A user initiates a payment and returns to the form. When submitting: + - The payment session may have expired + - The payment may have been cancelled externally + +#### The solution + +Rather than failing the entire submission with an unrecoverable error, components can throw `InvalidComponentStateError` to trigger a controlled recovery process: + +1. The component throws the error with a user-friendly message +2. The form engine catches the error +3. The component's internal state is cleared from the session +4. The user is redirected back to the component's page +5. An error message is displayed to the user +6. The user can re-enter the data and continue + +### How it works + +Components throw the exception in their `onSubmit()` method when they detect invalid external state. The component does not handle state clearing - it only throws the error. + +#### Submission flow + +``` +1. User clicks "Submit" on summary page + ↓ +2. SummaryPageController.handleFormSubmit() + ↓ +3. finaliseComponents() - validates external state for each component + ↓ +4. component.onSubmit() called for each component + ↓ +5. Component validates external state + ↓ +6a. [Success Path] 6b. [Invalid State Path] + External state valid External state invalid + ↓ ↓ + Continue submission throw InvalidComponentStateError + ↓ + 7. SummaryPageController catches error + ↓ + 8. Flash error message to user + ↓ + 9. CacheService.resetComponentStates() + - Clears component data from session + ↓ + 10. Redirect to component's page + ↓ + 11. User sees error and can re-enter data +``` + +#### Catching the exception (controller level) + +The exception is caught and handled by the **SummaryPageController** during form submission. + +**Location**: `src/server/plugins/engine/pageControllers/SummaryPageController.ts` + +The controller: + +1. Catches `InvalidComponentStateError` during `finaliseComponents()` +2. Creates a GOV.UK error message object +3. Flashes the error to the user's session +4. Calls `cacheService.resetComponentStates()` to clear the component's state +5. Redirects the user back to the component's page + +### How to implement it for a component + +Override the `onSubmit()` method in your component class. This method is called during form submission to finalise any external state. + +```typescript +import { InvalidComponentStateError } from '~/src/server/plugins/engine/pageControllers/errors.js' + +async onSubmit( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext +) { + const value = this.getData(context.state) + + if (!value) { + return // No data to validate + } + + try { + // Attempt to validate/finalise external state + await this.validateExternalState(value) + } catch (error) { + // Check if this is a recoverable error + if (this.isRecoverableError(error)) { + throw new InvalidComponentStateError( + this, + 'There was a problem with your [data type]. Please [action] before submitting the form again.' + ) + } + + // Non-recoverable errors should be re-thrown + throw error + } +} +``` From 5d56978e8170bad41ac24c67963ec0fb24b7515e Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 18 Dec 2025 09:39:07 +0000 Subject: [PATCH 23/27] satisfy editorconfig: spaces with multiples of two --- docs/features/code-based/COMPONENTS.md | 36 +++++++++++++------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/docs/features/code-based/COMPONENTS.md b/docs/features/code-based/COMPONENTS.md index a1d060490..da908527d 100644 --- a/docs/features/code-based/COMPONENTS.md +++ b/docs/features/code-based/COMPONENTS.md @@ -67,30 +67,30 @@ Components throw the exception in their `onSubmit()` method when they detect inv ``` 1. User clicks "Submit" on summary page - ↓ + ↓ 2. SummaryPageController.handleFormSubmit() - ↓ + ↓ 3. finaliseComponents() - validates external state for each component - ↓ + ↓ 4. component.onSubmit() called for each component - ↓ + ↓ 5. Component validates external state - ↓ + ↓ 6a. [Success Path] 6b. [Invalid State Path] - External state valid External state invalid - ↓ ↓ - Continue submission throw InvalidComponentStateError - ↓ - 7. SummaryPageController catches error - ↓ - 8. Flash error message to user - ↓ - 9. CacheService.resetComponentStates() + External state valid External state invalid + ↓ ↓ + Continue submission throw InvalidComponentStateError + ↓ + 7. SummaryPageController catches error + ↓ + 8. Flash error message to user + ↓ + 9. CacheService.resetComponentStates() - Clears component data from session - ↓ - 10. Redirect to component's page - ↓ - 11. User sees error and can re-enter data + ↓ + 10. Redirect to component's page + ↓ + 11. User sees error and can re-enter data ``` #### Catching the exception (controller level) From cdf09ed022a7bd497a3b41a1bef05c4f0b826d72 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 18 Dec 2025 09:49:47 +0000 Subject: [PATCH 24/27] clarify component state wording --- docs/features/code-based/COMPONENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/features/code-based/COMPONENTS.md b/docs/features/code-based/COMPONENTS.md index da908527d..9456e0cf6 100644 --- a/docs/features/code-based/COMPONENTS.md +++ b/docs/features/code-based/COMPONENTS.md @@ -16,7 +16,7 @@ Components are the building blocks of forms in the engine. Components usually ex - ComponentBase: general components that display content on the page, such as Markdown or HTML components. -- FormComponents: these components are specialised components that take user input. They represent individual form fields and controls (text inputs, file uploads, radios, checkboxes, etc.) and handle their own validation, state management, and rendering. +- FormComponents: these components are specialised components that take user input. They represent individual form fields and controls (text inputs, file uploads, radios, checkboxes, etc.) and handle their own validation, state (data) and rendering. > [!NOTE] > Custom components are currently not supported when registering the plugin. Whilst you can develop custom pages as a plugin consumer, custom components must be built into the core engine. From b23cc006fcb830e9b1b588084d62ad78cc73b7b9 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 18 Dec 2025 10:21:49 +0000 Subject: [PATCH 25/27] Improved variable name --- .../plugins/engine/pageControllers/SummaryPageController.ts | 4 ++-- src/server/services/cacheService.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 5fc6f5b72..8507b9731 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -254,11 +254,11 @@ async function finaliseComponents( metadata: FormMetadata, context: FormContext ) { - const relevantPages = context.relevantPages.flatMap( + const relevantFields = context.relevantPages.flatMap( (page) => page.collection.fields ) - for (const component of relevantPages) { + for (const component of relevantFields) { /* Each component will throw InvalidComponent if its state is invalid, which is handled by handleFormSubmit diff --git a/src/server/services/cacheService.ts b/src/server/services/cacheService.ts index c1039063c..5a7be807d 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -105,10 +105,10 @@ export class CacheService { ) { const state = await this.getState(request) - for (const componentId of componentNames) { - if (componentId in state) { + for (const componentName of componentNames) { + if (componentName in state) { // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete state[componentId] + delete state[componentName] } } From 32c7feff208f0b6ec6c0a3cf8cedbc92f6a24eb3 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 18 Dec 2025 12:16:33 +0000 Subject: [PATCH 26/27] notificationEmail should always be present at the point of onSubmit of a component --- src/server/plugins/engine/components/FileUploadField.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index e46076b3f..4a6ff220c 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -298,8 +298,13 @@ export class FileUploadField extends FormComponent { metadata: FormMetadata, context: FormContext ) { - const notificationEmail = - metadata.notificationEmail ?? 'defraforms@defra.gov.uk' + const notificationEmail = metadata.notificationEmail + + if (!notificationEmail) { + // this should not happen because notificationEmail is checked further up + // the chain in SummaryPageController before submitForm is called. + throw new Error('Unexpected missing notificationEmail in metadata') + } if (!request.app.model?.services.formSubmissionService) { throw new Error('No form submission service available in app model') From 9bb6812551802784aafe8d051bf9e0b2235bc397 Mon Sep 17 00:00:00 2001 From: Alex Luckett Date: Thu, 8 Jan 2026 15:03:15 +0000 Subject: [PATCH 27/27] chore(release): #major --- .../engine/components/FileUploadField.test.ts | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/server/plugins/engine/components/FileUploadField.test.ts b/src/server/plugins/engine/components/FileUploadField.test.ts index a3a0aebfa..053b5f1ad 100644 --- a/src/server/plugins/engine/components/FileUploadField.test.ts +++ b/src/server/plugins/engine/components/FileUploadField.test.ts @@ -917,15 +917,20 @@ describe('FileUploadField', () => { ) }) - it('should use default email when notificationEmail is not set', async () => { + it('should fail when notificationEmail is not set', async () => { mockMetadata.notificationEmail = undefined - await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow('Unexpected missing notificationEmail in metadata') + }) - expect(mockPersistFiles).toHaveBeenCalledWith( - expect.any(Array), - 'defraforms@defra.gov.uk' - ) + it('should fail when notificationEmail is empty string', async () => { + mockMetadata.notificationEmail = '' + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow('Unexpected missing notificationEmail in metadata') }) it('should not call persistFiles when no files in state', async () => { @@ -944,14 +949,6 @@ describe('FileUploadField', () => { expect(mockPersistFiles).not.toHaveBeenCalled() }) - it('should pass empty string when notificationEmail is empty string', async () => { - mockMetadata.notificationEmail = '' - - await fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) - - expect(mockPersistFiles).toHaveBeenCalledWith(expect.any(Array), '') - }) - it('should throw Error when formSubmissionService is not available', async () => { if (!mockRequest.app.model) { throw new Error('Invalid test setup')