diff --git a/docs/features/code-based/COMPONENTS.md b/docs/features/code-based/COMPONENTS.md new file mode 100644 index 000000000..9456e0cf6 --- /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 (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. + +## 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 + } +} +``` diff --git a/src/server/constants.js b/src/server/constants.js index fd659a09e..301539c14 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 COMPONENT_STATE_ERROR = 'COMPONENT_STATE_ERROR' 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/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/beta/form-context.test.ts b/src/server/plugins/engine/beta/form-context.test.ts index 6aa3ecef8..609441d88 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, + undefined, 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 1f955073c..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 ?? definition.outputEmail - - checkEmailAddressForLiveFormSubmission(emailAddress, isPreview) + checkEmailAddressForLiveFormSubmission( + metadata.notificationEmail, + isPreview + ) const routePrefix = options.routePrefix ?? server.realm.modifiers.route.prefix diff --git a/src/server/plugins/engine/components/FileUploadField.test.ts b/src/server/plugins/engine/components/FileUploadField.test.ts index 5c16e7c00..053b5f1ad 100644 --- a/src/server/plugins/engine/components/FileUploadField.test.ts +++ b/src/server/plugins/engine/components/FileUploadField.test.ts @@ -1,25 +1,34 @@ 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 } 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 { 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, + 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 +837,196 @@ 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 + }) + + // 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 fail when notificationEmail is not set', async () => { + mockMetadata.notificationEmail = undefined + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow('Unexpected missing notificationEmail in metadata') + }) + + 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 () => { + 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 throw Error when formSubmissionService is not available', async () => { + 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) + ).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) + + 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 () => { + const goneError = Boom.resourceGone('File has expired') + mockPersistFiles.mockRejectedValue(goneError) + + await expect( + fileUploadField.onSubmit(mockRequest, mockMetadata, mockContext) + ).rejects.toThrow(InvalidComponentStateError) + + 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 () => { + 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/components/FileUploadField.ts b/src/server/plugins/engine/components/FileUploadField.ts index ec2f1be33..4a6ff220c 100644 --- a/src/server/plugins/engine/components/FileUploadField.ts +++ b/src/server/plugins/engine/components/FileUploadField.ts @@ -1,10 +1,15 @@ -import { 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' 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 +18,7 @@ import { type FileState, type FileUpload, type FileUploadMetadata, + type FormContext, type FormPayload, type FormState, type FormStateValue, @@ -26,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 } 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 +293,56 @@ export class FileUploadField extends FormComponent { return FileUploadField.getAllPossibleErrors() } + async onSubmit( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext + ) { + 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') + } + + 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, 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..015274975 100644 --- a/src/server/plugins/engine/components/FormComponent.ts +++ b/src/server/plugins/engine/components/FormComponent.ts @@ -1,7 +1,15 @@ -import { 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 ErrorMessageTemplateList, type FileState, @@ -220,6 +228,14 @@ export class FormComponent extends ComponentBase { advancedSettingsErrors: [] } } + + onSubmit( + _request: FormRequestPayload, + _metadata: FormMetadata, + _context: FormContext + ): Promise { + return Promise.resolve() + } } /** diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index 8cfaa21da..a438aec3f 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/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..928ba86fc 100644 --- a/src/server/plugins/engine/pageControllers/FileUploadPageController.ts +++ b/src/server/plugins/engine/pageControllers/FileUploadPageController.ts @@ -371,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[] = [ @@ -423,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) @@ -433,10 +435,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/QuestionPageController.ts b/src/server/plugins/engine/pageControllers/QuestionPageController.ts index ec778d6a1..d6186fd22 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,11 @@ 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] : [] + + 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/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 2241cf805..d9f05baea 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -83,4 +83,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/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 2e26dd76d..8507b9731 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -7,12 +7,13 @@ import { import Boom from '@hapi/boom' import { type RouteOptions } from '@hapi/hapi' +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 { getAnswer } from '~/src/server/plugins/engine/components/helpers/components.js' import { checkEmailAddressForLiveFormSubmission, checkFormStatus, + createError, getCacheService } from '~/src/server/plugins/engine/helpers.js' import { @@ -24,11 +25,11 @@ 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, - type FormContextRequest, - type FormSubmissionState + type FormContextRequest } from '~/src/server/plugins/engine/types.js' import { FormAction, @@ -136,21 +137,39 @@ 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) - await submitForm( - context, - request, - viewModel, - model, - emailAddress, - formMetadata - ) + + try { + await submitForm( + context, + formMetadata, + request, + viewModel, + model, + notificationEmail, + formMetadata + ) + } catch (error) { + if (error instanceof InvalidComponentStateError) { + 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.component.page?.path) + } + + throw error + } } await cacheService.setConfirmationState(request, { @@ -179,13 +198,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 extendFileRetention(model, context.state, emailAddress) + await finaliseComponents(request, metadata, context) const formStatus = checkFormStatus(request.params) const logTags = ['submit', 'submissionApi'] @@ -222,39 +242,28 @@ export async function submitForm( ) } -async function extendFileRetention( - model: FormModel, - state: FormSubmissionState, - updatedRetrievalKey: string +/** + * 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 + */ +async function finaliseComponents( + request: FormRequestPayload, + metadata: FormMetadata, + context: FormContext ) { - const { formSubmissionService } = model.services - const { persistFiles } = formSubmissionService - const files: { fileId: string; initiatedRetrievalKey: string }[] = [] - - // 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( - (component) => component instanceof FileUploadField - ) - - fileUploadComponents.forEach((component) => { - const values = component.getFormValueFromState(state) - if (!values?.length) { - return - } - - files.push( - ...values.map(({ status }) => ({ - fileId: status.form.file.fileId, - initiatedRetrievalKey: status.metadata.retrievalKey - })) - ) - }) - }) + const relevantFields = context.relevantPages.flatMap( + (page) => page.collection.fields + ) - if (files.length) { - return persistFiles(files, updatedRetrievalKey) + for (const component of relevantFields) { + /* + Each component will throw InvalidComponent if its state is invalid, which is handled + by handleFormSubmit + */ + await component.onSubmit(request, metadata, context) } } 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/pageControllers/errors.test.ts b/src/server/plugins/engine/pageControllers/errors.test.ts new file mode 100644 index 000000000..ac7c3910b --- /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 } + ) + + 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 } + ) + + const error = new InvalidComponentStateError( + component, + 'Test error message' + ) + const stateKeys = error.getStateKeys() + + expect(stateKeys).toEqual(['textField']) + }) + }) +}) diff --git a/src/server/plugins/engine/pageControllers/errors.ts b/src/server/plugins/engine/pageControllers/errors.ts new file mode 100644 index 000000000..b0361b186 --- /dev/null +++ b/src/server/plugins/engine/pageControllers/errors.ts @@ -0,0 +1,30 @@ +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 + + 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'] : [] + + return [this.component.name].concat(extraStateKeys) + } +} 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() } diff --git a/src/server/plugins/engine/views/index.html b/src/server/plugins/engine/views/index.html index af3258c88..c5cc8d8f3 100644 --- a/src/server/plugins/engine/views/index.html +++ b/src/server/plugins/engine/views/index.html @@ -10,7 +10,7 @@ {% include "partials/preview-banner.html" %} {% endif %} - {% if errors %} + {% if errors | length %} {{ govukErrorSummary({ titleText: "There is a problem", errorList: checkErrorTemplates(errors) 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() + } }) ) 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..5a7be807d 100644 --- a/src/server/services/cacheService.ts +++ b/src/server/services/cacheService.ts @@ -99,6 +99,22 @@ export class CacheService { request.yar.flash(key.id, message) } + async resetComponentStates( + request: AnyFormRequest, + componentNames: string[] + ) { + const state = await this.getState(request) + + for (const componentName of componentNames) { + if (componentName in state) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete state[componentName] + } + } + + 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..ef364f63b 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 { + type COMPONENT_STATE_ERROR, type EXTERNAL_STATE_APPENDAGE, type EXTERNAL_STATE_PAYLOAD } from '~/src/server/constants.js' @@ -20,6 +21,7 @@ declare module '@hapi/yar' { interface YarFlashes { [EXTERNAL_STATE_APPENDAGE]: object [EXTERNAL_STATE_PAYLOAD]: object + [COMPONENT_STATE_ERROR]: string [key: string]: { errors: FormSubmissionError[] } } } 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 ef4e5e66c..780cefd70 100644 --- a/test/fixtures/form.js +++ b/test/fixtures/form.js @@ -37,7 +37,25 @@ export const metadata = { createdAt: now, createdBy: author, updatedAt: now, - updatedBy: author + updatedBy: author, + notificationEmail: 'enrique.chase@defra.gov.uk' +} + +/** + * @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' } /** @@ -77,8 +95,7 @@ export const definition = { } ], conditions: [], - lists: [], - outputEmail: 'enrique.chase@defra.gov.uk' + lists: [] } export const componentId = '1491981d-99cd-485e-ab4a-f88275edeadc' @@ -121,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 new file mode 100644 index 000000000..d80c1dbec --- /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 + } + ], + 'enrique.chase@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 + } + ], + 'enrique.chase@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' + */ 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 579654845..8203a3d41 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} */ ({ @@ -174,8 +173,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/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', 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`,