From 6724751956874f1e9df1127e23da76787df5839a Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Fri, 7 Aug 2026 18:19:26 +0100 Subject: [PATCH 01/12] Building notification targets and recording them when a form is submitted. --- src/server/plugins/engine/models/FormModel.ts | 31 +- src/server/plugins/engine/models/types.ts | 31 ++ .../SummaryPageController.test.ts | 51 +++ .../pageControllers/SummaryPageController.ts | 22 +- .../helpers/submission.test.ts | 368 +++++++++++++++++- .../pageControllers/helpers/submission.ts | 224 ++++++++++- test/form/govuk-notify.test.js | 10 +- test/form/journey-basic.test.js | 10 +- test/form/persist-files.test.js | 10 +- test/form/repeat.test.js | 10 +- 10 files changed, 753 insertions(+), 14 deletions(-) diff --git a/src/server/plugins/engine/models/FormModel.ts b/src/server/plugins/engine/models/FormModel.ts index 5bbf07aa5..e47b03bb4 100644 --- a/src/server/plugins/engine/models/FormModel.ts +++ b/src/server/plugins/engine/models/FormModel.ts @@ -1,5 +1,6 @@ import { ComponentType, + ConditionEvaluationOutcome, ConditionsModel, ControllerPath, ControllerType, @@ -8,6 +9,7 @@ import { formDefinitionSchema, formDefinitionV2Schema, generateConditionAlias, + getErrorMessage, hasComponents, hasComponentsEvenIfNoNext, hasRepeater, @@ -50,7 +52,10 @@ import { extractBaseTranslations } from '~/src/server/plugins/engine/i18n/extrac import { createFormI18nInstance } from '~/src/server/plugins/engine/i18n/index.js' import { getAvailableLanguages } from '~/src/server/plugins/engine/i18n/languages.js' import { type Translator } from '~/src/server/plugins/engine/i18n/types.js' -import { type ExecutableCondition } from '~/src/server/plugins/engine/models/types.js' +import { + type ConditionEvaluation, + type ExecutableCondition +} from '~/src/server/plugins/engine/models/types.js' import { type PageController } from '~/src/server/plugins/engine/pageControllers/PageController.js' import { createPage, @@ -294,21 +299,35 @@ export class FormModel { const { name, displayName, value } = condition const expr = this.toConditionExpression(value, parser) - const fn = (evaluationState: FormState) => { + const evaluate = (evaluationState: FormState): ConditionEvaluation => { const ctx = this.toConditionContext(evaluationState, this.conditions) + try { - return expr.evaluate(ctx) as boolean - } catch { - return false + return { + outcome: (expr.evaluate(ctx) as boolean) + ? ConditionEvaluationOutcome.True + : ConditionEvaluationOutcome.False + } + } catch (err) { + return { + outcome: ConditionEvaluationOutcome.Error, + error: getErrorMessage(err) + } } } + // A failed evaluation continues to route as `false`. `evaluate` exists so + // that the two can be told apart when recording outcomes for submission. + const fn = (evaluationState: FormState) => + evaluate(evaluationState).outcome === ConditionEvaluationOutcome.True + return { name, displayName, value, expr, - fn + fn, + evaluate } } diff --git a/src/server/plugins/engine/models/types.ts b/src/server/plugins/engine/models/types.ts index 980ad8f0a..db177d605 100644 --- a/src/server/plugins/engine/models/types.ts +++ b/src/server/plugins/engine/models/types.ts @@ -1,4 +1,5 @@ import { + type ConditionEvaluationOutcome, type ConditionWrapper, type FormComponentsDef, type Section @@ -16,9 +17,39 @@ import { type FormSubmissionError } from '~/src/server/plugins/engine/types.js' +/** + * The result of evaluating a condition, keeping a failed evaluation distinct + * from one that legitimately returned `false`. + * @see {@link ExecutableCondition.evaluate} + */ +export interface ConditionEvaluation { + outcome: ConditionEvaluationOutcome + error?: string +} + +/** + * A form condition paired with the parsed expression and callbacks needed to + * run it against a form submission state + * Created by `FormModel.makeCondition` + */ export type ExecutableCondition = ConditionWrapper & { + /** + * Parsed expression for the condition's {@link ConditionWrapper.value}, + * evaluated against a context built from the submission state + */ expr: Expression + + /** + * Evaluates the condition, used for page routing and component visibility + * A failed evaluation is reported as `false` + */ fn: (evaluationState: FormState) => boolean + + /** + * As `fn`, but reports whether evaluation failed rather than defaulting a + * failure to `false`. Used to record condition outcomes on submission. + */ + evaluate: (evaluationState: FormState) => ConditionEvaluation } /** diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 5d83d9674..828a5f14b 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -590,5 +590,56 @@ describe('SummaryPageController - Payment (DF-832)', () => { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access expect(paymentCall.language).toBe('cy') }) + + it('submits the notification email and qualifying outputs as notification targets', async () => { + const outputs = model.def.outputs + + model.def.outputs = [ + { + emailAddress: 'casework@defra.gov.uk', + audience: 'human', + version: '1' + } + ] + + try { + const { request, context, viewModel, formSubmissionSubmit } = + buildSubmitHarness({ captured: true }) + + const formMetadata = { + contact: { online: { url: '/help' } }, + notificationEmail: 'notify@defra.gov.uk' + } as unknown as Parameters[1] + + await submitForm( + context, + formMetadata, + request, + viewModel, + model, + 'notify@defra.gov.uk', + translator + ) + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const paymentCall = formSubmissionSubmit.mock.calls[0][0] + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + expect(paymentCall.notificationTargets).toEqual([ + { + emailAddress: 'notify@defra.gov.uk', + audience: 'human', + version: '1' + }, + { + emailAddress: 'casework@defra.gov.uk', + audience: 'human', + version: '1' + } + ]) + } finally { + model.def.outputs = outputs + } + }) }) }) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 4c63d4bd4..42b9a9278 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -1,4 +1,5 @@ import { + Engine, hasComponentsEvenIfNoNext, type FormMetadata, type Page, @@ -38,7 +39,9 @@ import { PaymentSubmissionError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { + buildConditionEvaluations, buildMainRecords, + buildNotificationTargets, buildRepeaterRecords } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' import { @@ -473,7 +476,9 @@ export async function submitForm( emailAddress, request.yar.id, translator, - summaryViewModel.context.referenceNumber + summaryViewModel.context.referenceNumber, + context, + formMetadata ) if (submitResponse === undefined) { @@ -553,7 +558,9 @@ function submitData( retrievalKey: string, sessionId: string, translator: Translator, - referenceNumber: string + referenceNumber: string, + context: FormContext, + formMetadata: FormMetadata ) { const { formSubmissionService } = model.services const { submit } = formSubmissionService @@ -564,6 +571,17 @@ function submitData( referenceNumber, main: buildMainRecords(items, translator), repeaters: buildRepeaterRecords(items, translator), + // Condition ids are only stable in V2, so there is nothing to report + // against for a V1 definition + conditionEvaluations: + model.engine === Engine.V2 + ? buildConditionEvaluations(model, context) + : undefined, + notificationTargets: buildNotificationTargets( + model, + context, + formMetadata.notificationEmail + ), // Only populate the language property if the form is multi-language enabled // @ts-expect-error - dynamic language property language: model.def.metadata?.translations?.cy diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts index 25ad933d5..9d451c859 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts @@ -1,3 +1,10 @@ +import { + ConditionEvaluationOutcome, + type Output, + type OutputAudience +} from '@defra/forms-model' + +import { logger } from '~/src/server/common/helpers/logging/logger.js' import { GeospatialField } from '~/src/server/plugins/engine/components/GeospatialField.js' import { PaymentField } from '~/src/server/plugins/engine/components/PaymentField.js' import { TextField } from '~/src/server/plugins/engine/components/TextField.js' @@ -5,12 +12,19 @@ import { validSingleState } from '~/src/server/plugins/engine/components/helpers import { FormModel } from '~/src/server/plugins/engine/models/index.js' import { type DetailItemField } from '~/src/server/plugins/engine/models/types.js' import { + buildConditionEvaluations, buildMainRecords, + buildNotificationTargets, buildPaymentRecords, buildRepeaterRecords } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' -import { type FormSubmissionState } from '~/src/server/plugins/engine/types.js' +import { + type FormContext, + type FormState, + type FormSubmissionState +} from '~/src/server/plugins/engine/types.js' import { definition } from '~/test/fixtures/form.js' +import joinedConditionsDefinition from '~/test/form/definitions/joined-conditions-simple-v2.js' const translator = new FormModel(definition, { basePath: '/' @@ -474,3 +488,355 @@ describe('Submission helpers', () => { }) }) }) + +describe('buildConditionEvaluations', () => { + const userNameComponentId = '87b987e8-bcf9-4ff9-92af-57c34c45995a' + const isOverEighteenComponentId = 'c977e76e-49ab-4443-b93e-e19e8d9c81ac' + const isBobConditionId = 'd15aff7a-6224-40a2-8e5f-51a5af2f7910' + const isOverEighteenConditionId = 'd1f9fcc7-f098-47e7-9d31-4f5ee57ba985' + const joinedConditionId = 'db43c6bc-9ce6-478b-8345-4fff5eff2ba3' + + const model = new FormModel(joinedConditionsDefinition, { basePath: '/' }) + + /** + * The engine seeds every component with `null` before the page walk, so an + * unanswered form reaches submission with keys present but empty + * @param {FormState} evaluationState + */ + const build = (evaluationState: FormState) => + buildConditionEvaluations(model, { evaluationState } as FormContext) + + it('should record every condition in the definition', () => { + const evaluations = build({ userName: null, isOverEighteen: null }) + + expect(evaluations.map((evaluation) => evaluation.conditionId)).toEqual([ + isBobConditionId, + isOverEighteenConditionId, + joinedConditionId + ]) + }) + + it('should record answered conditions that match', () => { + const evaluations = build({ userName: 'Bob', isOverEighteen: true }) + + expect(evaluations).toEqual([ + { + conditionId: isBobConditionId, + outcome: ConditionEvaluationOutcome.True, + references: [ + { + componentId: userNameComponentId, + componentName: 'userName', + answered: true + } + ] + }, + { + conditionId: isOverEighteenConditionId, + outcome: ConditionEvaluationOutcome.True, + references: [ + { + componentId: isOverEighteenComponentId, + componentName: 'isOverEighteen', + answered: true + } + ] + }, + { + conditionId: joinedConditionId, + outcome: ConditionEvaluationOutcome.True, + references: [ + { + componentId: userNameComponentId, + componentName: 'userName', + answered: true + }, + { + componentId: isOverEighteenComponentId, + componentName: 'isOverEighteen', + answered: true + } + ] + } + ]) + }) + + it('should record answered conditions that do not match', () => { + const evaluations = build({ userName: 'Alice', isOverEighteen: false }) + + expect( + evaluations.map(({ conditionId, outcome }) => ({ conditionId, outcome })) + ).toEqual([ + { + conditionId: isBobConditionId, + outcome: ConditionEvaluationOutcome.False + }, + { + conditionId: isOverEighteenConditionId, + outcome: ConditionEvaluationOutcome.False + }, + { + conditionId: joinedConditionId, + outcome: ConditionEvaluationOutcome.False + } + ]) + }) + + it('should flag unanswered references so a vacuous outcome can be spotted', () => { + const evaluations = build({ userName: null, isOverEighteen: null }) + + expect(evaluations[0].outcome).toBe(ConditionEvaluationOutcome.False) + expect(evaluations[0].references).toEqual([ + { + componentId: userNameComponentId, + componentName: 'userName', + answered: false + } + ]) + }) + + it('should treat an empty answer as unanswered', () => { + const evaluations = build({ userName: '', isOverEighteen: null }) + + expect(evaluations[0].references[0].answered).toBe(false) + }) + + it('should flatten nested condition references to their components', () => { + const evaluations = build({ userName: 'Bob', isOverEighteen: null }) + const joined = evaluations.find( + ({ conditionId }) => conditionId === joinedConditionId + ) + + expect(joined?.references).toEqual([ + { + componentId: userNameComponentId, + componentName: 'userName', + answered: true + }, + { + componentId: isOverEighteenComponentId, + componentName: 'isOverEighteen', + answered: false + } + ]) + }) + + it('should record an error outcome when evaluation throws', () => { + // A component missing from the evaluation state - a repeater field, say - + // throws `undefined variable` rather than evaluating to false + const evaluations = build({}) + + expect(evaluations[0].outcome).toBe(ConditionEvaluationOutcome.Error) + }) + + it('should return no evaluations for a V1 definition', () => { + const v1Model = new FormModel(definition, { basePath: '/' }) + + expect( + buildConditionEvaluations(v1Model, { evaluationState: {} } as FormContext) + ).toEqual([]) + }) +}) + +describe('buildNotificationTargets', () => { + const isBobConditionId = 'd15aff7a-6224-40a2-8e5f-51a5af2f7910' + const isOverEighteenConditionId = 'd1f9fcc7-f098-47e7-9d31-4f5ee57ba985' + const notificationEmail = 'submitted.forms@defra.gov.uk' + + /** + * @param {Output[]} outputs + */ + const modelWithOutputs = (outputs: Output[]) => + new FormModel({ ...joinedConditionsDefinition, outputs }, { basePath: '/' }) + + const output = ( + emailAddress: string, + condition?: string, + audience: OutputAudience = 'human', + version = '1' + ): Output => ({ + emailAddress, + audience, + version, + ...(condition ? { condition } : {}) + }) + + /** + * The notification email defaults to the audience and version the form is + * already sent with + * @param {string} emailAddress + */ + const target = ( + emailAddress: string, + audience: OutputAudience = 'human', + version = '1' + ) => ({ emailAddress, audience, version }) + + /** + * @param {FormModel} model + * @param {FormState} evaluationState + */ + const build = (model: FormModel, evaluationState: FormState) => + buildNotificationTargets( + model, + { evaluationState } as FormContext, + notificationEmail + ) + + beforeEach(() => { + jest.spyOn(logger, 'error').mockImplementation(() => logger) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + it('should return the notification email when there are no outputs', () => { + const model = new FormModel(joinedConditionsDefinition, { basePath: '/' }) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail) + ]) + }) + + it('should send the notification email in the format the form is configured for', () => { + const model = new FormModel(joinedConditionsDefinition, { basePath: '/' }) + + model.def.output = { audience: 'machine', version: '2' } + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail, 'machine', '2') + ]) + }) + + it('should include unconditional outputs', () => { + const model = modelWithOutputs([ + output('casework@defra.gov.uk'), + output('archive@defra.gov.uk') + ]) + + expect(build(model, { userName: 'Alice', isOverEighteen: false })).toEqual([ + target(notificationEmail), + target('casework@defra.gov.uk'), + target('archive@defra.gov.uk') + ]) + }) + + it('should carry the audience and version of each output', () => { + const model = modelWithOutputs([ + output('casework@defra.gov.uk', undefined, 'machine', '2') + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail), + target('casework@defra.gov.uk', 'machine', '2') + ]) + }) + + it('should include a conditional output only when its condition passes', () => { + const model = modelWithOutputs([ + output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail), + target('over-eighteen@defra.gov.uk') + ]) + }) + + it('should exclude a conditional output when its condition fails', () => { + const model = modelWithOutputs([ + output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: false })).toEqual([ + target(notificationEmail) + ]) + }) + + it('should mix conditional and unconditional outputs', () => { + const model = modelWithOutputs([ + output('casework@defra.gov.uk'), + output('bob@defra.gov.uk', isBobConditionId), + output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: false })).toEqual([ + target(notificationEmail), + target('casework@defra.gov.uk'), + target('bob@defra.gov.uk') + ]) + }) + + it('should deduplicate an address configured more than once in the same format', () => { + const model = modelWithOutputs([ + output(notificationEmail), + output(notificationEmail.toUpperCase()), + output('casework@defra.gov.uk'), + output('casework@defra.gov.uk', isBobConditionId) + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail), + target('casework@defra.gov.uk') + ]) + }) + + it('should keep the same address in different output formats', () => { + const model = modelWithOutputs([ + output(notificationEmail, undefined, 'machine', '1'), + output('casework@defra.gov.uk', undefined, 'human', '1'), + output('casework@defra.gov.uk', undefined, 'machine', '1'), + output('casework@defra.gov.uk', undefined, 'machine', '2') + ]) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail), + target(notificationEmail, 'machine', '1'), + target('casework@defra.gov.uk'), + target('casework@defra.gov.uk', 'machine', '1'), + target('casework@defra.gov.uk', 'machine', '2') + ]) + }) + + it('should exclude an output whose condition no longer exists, and log it', () => { + const model = modelWithOutputs([ + output('casework@defra.gov.uk', isBobConditionId) + ]) + + // The definition validates the reference, so the only way to reach this is + // a condition removed after the model was built + model.def.outputs = [ + output('casework@defra.gov.uk', '8d6b1b17-1d1e-4b7f-a4bc-3b0d1e4f5a6c') + ] + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target(notificationEmail) + ]) + + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining('8d6b1b17-1d1e-4b7f-a4bc-3b0d1e4f5a6c') + ) + }) + + it('should omit the notification email when the form has none', () => { + const model = modelWithOutputs([output('casework@defra.gov.uk')]) + const evaluationState: FormState = { userName: 'Bob', isOverEighteen: true } + + expect( + buildNotificationTargets(model, { evaluationState } as FormContext) + ).toEqual([target('casework@defra.gov.uk')]) + }) + + it('should include V1 outputs, which carry no condition', () => { + const v1Model = new FormModel( + { ...definition, outputs: [output('casework@defra.gov.uk')] }, + { basePath: '/' } + ) + + expect(build(v1Model, {})).toEqual([ + target(notificationEmail), + target('casework@defra.gov.uk') + ]) + }) +}) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index 69590a18a..07cb1d457 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -1,13 +1,31 @@ -import { type SubmitPayload } from '@defra/forms-model' +import { + isConditionWrapperV2, + type ConditionDataV2, + type ConditionRefDataV2, + type ConditionWrapperV2, + type Output, + type OutputAudience, + type SubmitConditionEvaluation, + type SubmitConditionReference, + type SubmitNotificationTarget, + type SubmitPayload +} from '@defra/forms-model' +import { logger } from '~/src/server/common/helpers/logging/logger.js' import { GeospatialField } from '~/src/server/plugins/engine/components/GeospatialField.js' import { PaymentField } from '~/src/server/plugins/engine/components/PaymentField.js' import { getAnswer } from '~/src/server/plugins/engine/components/helpers/components.js' import { type Translator } from '~/src/server/plugins/engine/i18n/types.js' +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type DetailItem, type DetailItemField } from '~/src/server/plugins/engine/models/types.js' +import { + type FormContext, + type FormState, + type FormStateValue +} from '~/src/server/plugins/engine/types.js' import { formatCurrency, formatPaymentDate @@ -141,3 +159,207 @@ export function buildRepeaterRecords( ) })) } + +/** + * Records the outcome of every condition in the form definition, evaluated + * against the answers as they stand at the point of submission. + * + * Each record carries the components the condition depends on and whether each + * was answered. An unanswered question still yields a boolean - negative + * operators such as "is not" return `true` against the seeded `null` - so the + * outcome alone cannot be read as evidence that the user gave that answer. + * + * V2 definitions only. `conditionId` is the V2 condition id, and the + * references are resolved from the component ids V2 conditions carry - V1 + * conditions reference components by name, and V1 components need not have + * an id at all. + */ +export function buildConditionEvaluations( + model: FormModel, + context: FormContext +): SubmitConditionEvaluation[] { + const { evaluationState } = context + + return model.def.conditions + .filter(isConditionWrapperV2) + .flatMap((conditionDef) => { + const condition = model.conditions[conditionDef.id] + + if (!condition) { + return [] + } + + const { outcome } = condition.evaluate(evaluationState) + + const references = collectReferences( + model, + conditionDef, + evaluationState, + new Map(), + new Set() + ) + + return { + conditionId: conditionDef.id, + outcome, + references: [...references.values()] + } + }) +} + +/** + * Resolves where this submission should be sent: the form's notification email + * ("Submitted forms sent to"), followed by every output that qualifies against + * the final answers. + * + * The notification email carries the same audience and version the form is + * already sent with, so a form with no outputs behaves as it always has. + * @see {@link file://./../../services/notifyService.ts} + * + * Targets are deduplicated on address, audience and version together, keeping + * the first spelling of the address seen. An address configured both as the + * notification email and as an output should not receive the same email twice, + * but the same address may legitimately receive both the human-readable and + * the machine-processable output. + * + * Applies to V1 and V2. V1 outputs carry no condition, so they all qualify. + */ +export function buildNotificationTargets( + model: FormModel, + context: FormContext, + notificationEmail?: string +): SubmitNotificationTarget[] { + const { evaluationState } = context + const targets = new Map() + + const add = ( + emailAddress: string | undefined, + audience: OutputAudience, + version: string + ) => { + if (emailAddress) { + const key = `${emailAddress.toLowerCase()}|${audience}|${version}` + + if (!targets.has(key)) { + targets.set(key, { emailAddress, audience, version }) + } + } + } + + add( + notificationEmail, + model.def.output?.audience ?? 'human', // Same defaults as src/server/plugins/engine/services/notifyService.ts at time of writing + model.def.output?.version ?? '1' + ) + + for (const output of model.def.outputs ?? []) { + if (outputQualifies(model, output, evaluationState)) { + add(output.emailAddress, output.audience, output.version) + } + } + + return [...targets.values()] +} + +/** + * Whether a component held an answer at the point a condition was evaluated. + * + * The engine seeds every component in `evaluationState` with `null` before the + * page walk begins, so an unanswered question is present but empty rather than + * absent. + * @see {@link FormModel.initialiseContext} + */ +function isAnswered(value: FormStateValue | undefined) { + if (value === undefined || value === null) { + return false + } + + if (Array.isArray(value)) { + return value.length > 0 + } + + return value !== '' +} + +function isConditionDataV2( + item: ConditionDataV2 | ConditionRefDataV2 +): item is ConditionDataV2 { + return 'componentId' in item +} + +/** + * Collects every component a condition depends on, following nested condition + * references. Results are keyed by component id so a component referenced more + * than once is reported once. + */ +function collectReferences( + model: FormModel, + conditionDef: ConditionWrapperV2, + evaluationState: FormState, + references: Map, + visited: Set +) { + if (visited.has(conditionDef.id)) { + return references + } + + visited.add(conditionDef.id) + + for (const item of conditionDef.items) { + if (isConditionDataV2(item)) { + const component = model.getComponentById(item.componentId) + + // A condition referencing a component that no longer exists cannot be + // resolved to a name, so there is nothing meaningful to report for it + if (component) { + references.set(item.componentId, { + componentId: item.componentId, + componentName: component.name, + answered: isAnswered(evaluationState[component.name]) + }) + } + + continue + } + + const referenced = model.getConditionById(item.conditionId) + + if (referenced) { + collectReferences(model, referenced, evaluationState, references, visited) + } + } + + return references +} + +/** + * Whether an output should receive this submission. + * + * An output with no condition is unconditional. An output whose condition + * cannot be resolved is treated as not qualifying: the gate the author put on + * that address cannot be shown to have passed, and sending anyway would leak + * the submission to a recipient who was meant to be filtered out. The + * definition validates output condition references, so this should not happen + * and is logged as an error. + */ +function outputQualifies( + model: FormModel, + output: Output, + evaluationState: FormState +) { + if (!output.condition) { + return true + } + + const condition = model.conditions[output.condition] + + if (!condition) { + logger.error( + `Form "${model.name}" has an output conditioned on "${output.condition}", which is not a condition in the definition. The output has been excluded from this submission.` + ) + + return false + } + + return condition.fn(evaluationState) +} diff --git a/test/form/govuk-notify.test.js b/test/form/govuk-notify.test.js index 472d452ee..c4bcad09e 100644 --- a/test/form/govuk-notify.test.js +++ b/test/form/govuk-notify.test.js @@ -342,7 +342,15 @@ describe('Submission journey test', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined + language: undefined, + conditionEvaluations: undefined, + notificationTargets: [ + { + emailAddress: 'enrique.chase@defra.gov.uk', + audience: 'human', + version: '1' + } + ] }) // Status page diff --git a/test/form/journey-basic.test.js b/test/form/journey-basic.test.js index dffa882b6..4998536f8 100644 --- a/test/form/journey-basic.test.js +++ b/test/form/journey-basic.test.js @@ -417,7 +417,15 @@ describe('Form journey', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined + language: undefined, + conditionEvaluations: undefined, + notificationTargets: [ + { + emailAddress: 'enrique.chase@defra.gov.uk', + audience: 'human', + version: '1' + } + ] }) expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) diff --git a/test/form/persist-files.test.js b/test/form/persist-files.test.js index cf8c3fb7e..425399680 100644 --- a/test/form/persist-files.test.js +++ b/test/form/persist-files.test.js @@ -197,7 +197,15 @@ describe('Submission journey test', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined + language: undefined, + conditionEvaluations: undefined, + notificationTargets: [ + { + emailAddress: 'enrique.chase@defra.gov.uk', + audience: 'human', + version: '1' + } + ] }) expect(submitRes.statusCode).toBe(StatusCodes.SEE_OTHER) diff --git a/test/form/repeat.test.js b/test/form/repeat.test.js index 626ebff23..2cbf36848 100644 --- a/test/form/repeat.test.js +++ b/test/form/repeat.test.js @@ -632,7 +632,15 @@ describe('Repeat POST tests', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined + language: undefined, + conditionEvaluations: undefined, + notificationTargets: [ + { + emailAddress: 'enrique.chase@defra.gov.uk', + audience: 'human', + version: '1' + } + ] }) }) From 5180136594e3596feea438a81097d6e241ee2745 Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Mon, 17 Aug 2026 15:25:14 +0100 Subject: [PATCH 02/12] Conditional e-mails: more work in building the output targets on models sent through the system. --- .../engine/outputFormatters/adapter/common.ts | 97 ++++++++ .../engine/outputFormatters/adapter/v1.ts | 82 +------ .../outputFormatters/adapter/v2.test.ts | 220 ++++++++++++++++++ .../engine/outputFormatters/adapter/v2.ts | 54 +++++ .../engine/outputFormatters/index.test.ts | 7 + .../plugins/engine/outputFormatters/index.ts | 4 +- .../helpers/submission.test.ts | 9 +- .../pageControllers/helpers/submission.ts | 19 +- src/server/plugins/engine/types.ts | 57 +++++ src/server/plugins/engine/types/enums.ts | 8 +- .../plugins/engine/types/schema.test.ts | 130 +++++++++++ src/server/plugins/engine/types/schema.ts | 52 ++++- 12 files changed, 655 insertions(+), 84 deletions(-) create mode 100644 src/server/plugins/engine/outputFormatters/adapter/common.ts create mode 100644 src/server/plugins/engine/outputFormatters/adapter/v2.test.ts create mode 100644 src/server/plugins/engine/outputFormatters/adapter/v2.ts diff --git a/src/server/plugins/engine/outputFormatters/adapter/common.ts b/src/server/plugins/engine/outputFormatters/adapter/common.ts new file mode 100644 index 000000000..d633d59e5 --- /dev/null +++ b/src/server/plugins/engine/outputFormatters/adapter/common.ts @@ -0,0 +1,97 @@ +import { + type FormMetadata, + type SubmitResponsePayload +} from '@defra/forms-model' + +import { EN_GB } from '~/src/server/constants.js' +import { + getFormVersion, + type checkFormStatus +} from '~/src/server/plugins/engine/helpers.js' +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' +import { categoriseData } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' +import { type FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' +import { + type FormAdapterSubmissionMessageData, + type FormAdapterSubmissionMessageMeta, + type FormAdapterSubmissionMessagePayload, + type FormAdapterSubmissionMessageResult, + type FormContext +} from '~/src/server/plugins/engine/types.js' + +/** + * The payload every adapter schema version shares. + * + * Later versions add to this rather than reshape it, so each version's + * formatter is only the difference between it and the one before. + */ +export function buildPayload( + schemaVersion: FormAdapterSubmissionSchemaVersion, + context: FormContext, + items: DetailItem[], + model: FormModel, + submitResponse: SubmitResponsePayload, + formStatus: ReturnType, + formMetadata?: FormMetadata +): FormAdapterSubmissionMessagePayload { + const csvFiles = extractCsvFiles(submitResponse) + + const { main: v2Main, ...v2Data } = categoriseData(items) + + const versionMetadata = getFormVersion(model.def) + + const meta: FormAdapterSubmissionMessageMeta = { + schemaVersion, + timestamp: new Date(), + referenceNumber: context.referenceNumber, + formName: model.name, + formId: formMetadata?.id ?? '', + formSlug: formMetadata?.slug ?? '', + status: formStatus.state, + isPreview: formStatus.isPreview, + notificationEmail: formMetadata?.notificationEmail ?? '', + language: context.translator?.language ?? EN_GB + } + + if (versionMetadata) { + meta.versionMetadata = versionMetadata + } + + const main = Object.fromEntries( + Object.entries(v2Main).map(([key, value]) => { + if (value === undefined) { + return [key, null] + } + + return [key, value] + }) + ) + + const data: FormAdapterSubmissionMessageData = { + main, + ...v2Data + } + + const result: FormAdapterSubmissionMessageResult = { + files: csvFiles + } + + return { + meta, + data, + result + } +} + +function extractCsvFiles( + submitResponse: SubmitResponsePayload +): FormAdapterSubmissionMessageResult['files'] { + const result = + submitResponse.result as Partial + + return { + main: result.files?.main ?? '', + repeaters: result.files?.repeaters ?? {} + } +} diff --git a/src/server/plugins/engine/outputFormatters/adapter/v1.ts b/src/server/plugins/engine/outputFormatters/adapter/v1.ts index a548185bd..a388e4e2d 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v1.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v1.ts @@ -3,22 +3,12 @@ import { type SubmitResponsePayload } from '@defra/forms-model' -import { EN_GB } from '~/src/server/constants.js' -import { - getFormVersion, - type checkFormStatus -} from '~/src/server/plugins/engine/helpers.js' +import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' -import { categoriseData } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' +import { buildPayload } from '~/src/server/plugins/engine/outputFormatters/adapter/common.js' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' -import { - type FormAdapterSubmissionMessageData, - type FormAdapterSubmissionMessageMeta, - type FormAdapterSubmissionMessagePayload, - type FormAdapterSubmissionMessageResult, - type FormContext -} from '~/src/server/plugins/engine/types.js' +import { type FormContext } from '~/src/server/plugins/engine/types.js' export function format( context: FormContext, @@ -28,65 +18,15 @@ export function format( formStatus: ReturnType, formMetadata?: FormMetadata ): string { - const csvFiles = extractCsvFiles(submitResponse) - - const { main: v2Main, ...v2Data } = categoriseData(items) - - const versionMetadata = getFormVersion(model.def) - - const meta: FormAdapterSubmissionMessageMeta = { - schemaVersion: FormAdapterSubmissionSchemaVersion.V1, - timestamp: new Date(), - referenceNumber: context.referenceNumber, - formName: model.name, - formId: formMetadata?.id ?? '', - formSlug: formMetadata?.slug ?? '', - status: formStatus.state, - isPreview: formStatus.isPreview, - notificationEmail: formMetadata?.notificationEmail ?? '', - language: context.translator?.language ?? EN_GB - } - - if (versionMetadata) { - meta.versionMetadata = versionMetadata - } - - const main = Object.fromEntries( - Object.entries(v2Main).map(([key, value]) => { - if (value === undefined) { - return [key, null] - } - - return [key, value] - }) + const payload = buildPayload( + FormAdapterSubmissionSchemaVersion.V1, + context, + items, + model, + submitResponse, + formStatus, + formMetadata ) - const data: FormAdapterSubmissionMessageData = { - main, - ...v2Data - } - - const result: FormAdapterSubmissionMessageResult = { - files: csvFiles - } - - const payload: FormAdapterSubmissionMessagePayload = { - meta, - data, - result - } - return JSON.stringify(payload) } - -function extractCsvFiles( - submitResponse: SubmitResponsePayload -): FormAdapterSubmissionMessageResult['files'] { - const result = - submitResponse.result as Partial - - return { - main: result.files?.main ?? '', - repeaters: result.files?.repeaters ?? {} - } -} diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts new file mode 100644 index 000000000..72744b18a --- /dev/null +++ b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts @@ -0,0 +1,220 @@ +import { + type FormDefinition, + type FormMetadata, + type Output, + type SubmitResponsePayload +} from '@defra/forms-model' + +import { type Field } from '~/src/server/plugins/engine/components/helpers/components.js' +import { FormModel } from '~/src/server/plugins/engine/models/index.js' +import { + type DetailItem, + type DetailItemField +} from '~/src/server/plugins/engine/models/types.js' +import { format as formatV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' +import { format } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' +import { buildFormContextRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' +import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/index.js' +import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' +import { type FormAdapterSubmissionMessagePayload } from '~/src/server/plugins/engine/types.js' +import { FormStatus } from '~/src/server/routes/types.js' +import definition from '~/test/form/definitions/repeat-mixed.js' + +const submitResponse = { + message: 'Submit completed', + result: { + files: { + main: '00000000-0000-0000-0000-000000000000', + repeaters: { + exampleRepeat: '11111111-1111-1111-1111-111111111111' + } + } + } +} as SubmitResponsePayload + +const formStatus = { + isPreview: false, + state: FormStatus.Live +} + +const dummyField: Field = { + getFormValueFromState: (_) => 'hello world' +} as Field + +const items: DetailItem[] = [ + { + name: 'exampleField', + label: 'Example Field', + href: '/example-field', + title: 'Example Field Title', + field: dummyField, + value: 'Example Value' + } as DetailItemField +] + +const model = new FormModel(definition, { basePath: 'test' }) + +const pageUrl = new URL('http://example.com/repeat/pizza-order/summary') + +const request = buildFormContextRequest({ + method: 'get', + url: pageUrl, + path: pageUrl.pathname, + params: { + path: 'pizza-order', + slug: 'repeat' + }, + query: {}, + app: { model } +}) + +const context = model.getFormContext(request, { + $$__referenceNumber: 'foobar', + orderType: 'delivery' +}) + +/** + * Formats against a copy of the definition carrying the given outputs, so the + * shared `model` is left alone for the other tests in this file. + */ +function formatWith( + outputs?: Output[], + output?: FormDefinition['output'], + notificationEmail = 'submissions@example.com' +) { + const withOutputs = new FormModel( + { ...definition, outputs, output }, + { basePath: 'test' } + ) + + const body = format(context, items, withOutputs, submitResponse, formStatus, { + id: '68a8b0449ab460290c28940a', + slug: 'order-a-pizza', + notificationEmail + } as FormMetadata) + + return JSON.parse(body) as FormAdapterSubmissionMessagePayload +} + +describe('Adapter v2 formatter', () => { + beforeEach(() => { + jest.useFakeTimers() + jest.setSystemTime(new Date('2024-01-15T10:30:00.000Z')) + }) + + afterEach(() => { + jest.useRealTimers() + }) + + it('is the v1 payload plus notificationTargets', () => { + const formMetadata = { + id: '68a8b0449ab460290c28940a', + slug: 'order-a-pizza', + notificationEmail: 'submissions@example.com' + } as FormMetadata + + const v1 = JSON.parse( + formatV1(context, items, model, submitResponse, formStatus, formMetadata) + ) as FormAdapterSubmissionMessagePayload + + const { notificationTargets, ...rest } = JSON.parse( + format(context, items, model, submitResponse, formStatus, formMetadata) + ) as FormAdapterSubmissionMessagePayload + + expect(notificationTargets).toBeDefined() + expect(rest).toEqual({ + ...v1, + meta: { + ...v1.meta, + schemaVersion: FormAdapterSubmissionSchemaVersion.V2 + } + }) + }) + + it('leaves v1 emitting the v1 schema version', () => { + const v1 = JSON.parse( + formatV1(context, items, model, submitResponse, formStatus) + ) as FormAdapterSubmissionMessagePayload + + expect(v1.meta.schemaVersion).toBe(FormAdapterSubmissionSchemaVersion.V1) + expect(v1.notificationTargets).toBeUndefined() + }) + + describe('notificationTargets', () => { + it('always includes the form notification email', () => { + expect(formatWith().notificationTargets).toEqual([ + { + emailAddress: 'submissions@example.com', + audience: 'human', + version: '2' + } + ]) + }) + + it('falls back to human v2 for the notification email', () => { + // forms-notify-listener has always defaulted a form with no explicit + // `output` to human v2. Defaulting to v1 here would silently change the + // format every such form is sent in. + expect(formatWith().notificationTargets?.[0]).toMatchObject({ + audience: 'human', + version: '2' + }) + }) + + it('honours an explicit output audience and version', () => { + const targets = formatWith(undefined, { + audience: 'machine', + version: '1' + }).notificationTargets + + expect(targets?.[0]).toMatchObject({ audience: 'machine', version: '1' }) + }) + + it('includes unconditional outputs alongside the notification email', () => { + const targets = formatWith([ + { emailAddress: 'team@example.com', audience: 'machine', version: '2' } + ]).notificationTargets + + expect(targets).toEqual([ + { + emailAddress: 'submissions@example.com', + audience: 'human', + version: '2' + }, + { emailAddress: 'team@example.com', audience: 'machine', version: '2' } + ]) + }) + + it('emits no progress state - that is the adapter’s to write', () => { + const targets = formatWith([ + { emailAddress: 'team@example.com', audience: 'human', version: '2' } + ]).notificationTargets + + for (const target of targets ?? []) { + expect(target).not.toHaveProperty('sent') + expect(target).not.toHaveProperty('sendAttempts') + expect(target).not.toHaveProperty('type') + } + }) + + // Condition evaluation itself is covered against buildNotificationTargets + // in pageControllers/helpers/submission.test.ts - the formatter only wires + // it up, and this file's fixture is a V1 definition, which rejects + // conditioned outputs outright. + + it('produces a payload the schema accepts', () => { + const payload = formatWith([ + { emailAddress: 'team@example.com', audience: 'machine', version: '2' } + ]) + + // The runner publishes with allowUnknown: false and throws on failure, + // so a formatter and schema that disagree would fail every submission + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payload, + { abortEarly: false, allowUnknown: false } + ) + + expect(error).toBeUndefined() + }) + }) +}) diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.ts new file mode 100644 index 000000000..05ba510e1 --- /dev/null +++ b/src/server/plugins/engine/outputFormatters/adapter/v2.ts @@ -0,0 +1,54 @@ +import { + type FormMetadata, + type SubmitResponsePayload +} from '@defra/forms-model' + +import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' +import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' +import { buildPayload } from '~/src/server/plugins/engine/outputFormatters/adapter/common.js' +import { buildNotificationTargets } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' +import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' +import { type FormContext } from '~/src/server/plugins/engine/types.js' + +/** + * Adapter V1 plus `notificationTargets` - see + * {@link FormAdapterSubmissionSchemaVersion.V2}. + */ +export function format( + context: FormContext, + items: DetailItem[], + model: FormModel, + submitResponse: SubmitResponsePayload, + formStatus: ReturnType, + formMetadata?: FormMetadata +): string { + const payload = buildPayload( + FormAdapterSubmissionSchemaVersion.V2, + context, + items, + model, + submitResponse, + formStatus, + formMetadata + ) + + // Resolved here rather than by the adapter so that output conditions are + // evaluated against the answers as they stood at submission. An adapter + // re-reading the definition later would see whatever the form has since been + // edited into, and has no submission state to evaluate against. + payload.notificationTargets = buildNotificationTargets( + model, + context, + formMetadata?.notificationEmail, + // Fallback for the `notificationEmail` target when the definition has no + // `output` block. V1 messages carry no `notificationTargets`, so + // forms-notify-listener recovers them from the live definition and applies + // this same `human`/`2` fallback - see `sendNotifyEmailsLegacy` in + // `src/service/notify-legacy.js` there. Changing either side alone means a + // form with no `output` starts being sent against a different template. + { audience: 'human', version: '2' } + ) + + return JSON.stringify(payload) +} diff --git a/src/server/plugins/engine/outputFormatters/index.test.ts b/src/server/plugins/engine/outputFormatters/index.test.ts index 2674a7440..e3a461f52 100644 --- a/src/server/plugins/engine/outputFormatters/index.test.ts +++ b/src/server/plugins/engine/outputFormatters/index.test.ts @@ -1,3 +1,5 @@ +import { format as formatAdapterV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' +import { format as formatAdapterV2 } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' import { format as formatHumanV1 } from '~/src/server/plugins/engine/outputFormatters/human/v1.js' import { getFormatter } from '~/src/server/plugins/engine/outputFormatters/index.js' @@ -7,6 +9,11 @@ describe('Page controller helpers', () => { expect(formatter).toBe(formatHumanV1) }) + it('should keep each adapter version on its own formatter', () => { + expect(getFormatter('adapter', '1')).toBe(formatAdapterV1) + expect(getFormatter('adapter', '2')).toBe(formatAdapterV2) + }) + it("should return an error if the audience doesn't exist", () => { expect(() => getFormatter('foobar', '1')).toThrow('Unknown audience') }) diff --git a/src/server/plugins/engine/outputFormatters/index.ts b/src/server/plugins/engine/outputFormatters/index.ts index 4356dea02..eff305e5d 100644 --- a/src/server/plugins/engine/outputFormatters/index.ts +++ b/src/server/plugins/engine/outputFormatters/index.ts @@ -7,6 +7,7 @@ import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/index.js' import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' import { format as formatAdapterV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' +import { format as formatAdapterV2 } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' import { format as formatHumanV1 } from '~/src/server/plugins/engine/outputFormatters/human/v1.js' import { format as formatMachineV1 } from '~/src/server/plugins/engine/outputFormatters/machine/v1.js' import { format as formatMachineV2 } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' @@ -33,7 +34,8 @@ const formatters: Record< '2': formatMachineV2 }, adapter: { - '1': formatAdapterV1 + '1': formatAdapterV1, + '2': formatAdapterV2 } } diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts index 9d451c859..6f4cd8e99 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts @@ -769,15 +769,20 @@ describe('buildNotificationTargets', () => { }) it('should deduplicate an address configured more than once in the same format', () => { + // One duplicate: the notification email is already part of the form + // metadata and will be included in the outputs. Adding it here a second + // time in uppercase. + // + // The casework@defra.gov.uk e-mail will not be seen as a duplicate + // because the second instance has a condtion attached to it. const model = modelWithOutputs([ - output(notificationEmail), output(notificationEmail.toUpperCase()), output('casework@defra.gov.uk'), output('casework@defra.gov.uk', isBobConditionId) ]) expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail), + target(notificationEmail), // The first casing is the one kept, which is the one from the metadata (ie lowercase version) target('casework@defra.gov.uk') ]) }) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index 07cb1d457..735ee34da 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -213,11 +213,16 @@ export function buildConditionEvaluations( * the final answers. * * The notification email carries the same audience and version the form is - * already sent with, so a form with no outputs behaves as it always has. + * already sent with, so a form with no outputs behaves as it always has. Where + * the definition does not say, `defaultOutput` decides - and it has to be the + * caller's decision, because the consumers disagree: the engine's own notify + * service falls back to human v1, while the adapter message is consumed by + * forms-notify-listener, which has always fallen back to human v2. Getting + * this wrong silently changes the format recipients receive. * @see {@link file://./../../services/notifyService.ts} * * Targets are deduplicated on address, audience and version together, keeping - * the first spelling of the address seen. An address configured both as the + * the first casing of the address seen. An address configured both as the * notification email and as an output should not receive the same email twice, * but the same address may legitimately receive both the human-readable and * the machine-processable output. @@ -227,7 +232,11 @@ export function buildConditionEvaluations( export function buildNotificationTargets( model: FormModel, context: FormContext, - notificationEmail?: string + notificationEmail?: string, + defaultOutput: { audience: OutputAudience; version: string } = { + audience: 'human', + version: '1' + } ): SubmitNotificationTarget[] { const { evaluationState } = context const targets = new Map() @@ -248,8 +257,8 @@ export function buildNotificationTargets( add( notificationEmail, - model.def.output?.audience ?? 'human', // Same defaults as src/server/plugins/engine/services/notifyService.ts at time of writing - model.def.output?.version ?? '1' + model.def.output?.audience ?? defaultOutput.audience, + model.def.output?.version ?? defaultOutput.version ) for (const output of model.def.outputs ?? []) { diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 76a96349c..3dbbb112e 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -6,6 +6,7 @@ import { type List, type Page, type PaymentFieldComponent, + type SubmitNotificationTarget, type UkAddressFieldComponent } from '@defra/forms-model' import { @@ -603,10 +604,66 @@ export interface FormAdapterSubmissionMessageData { payment?: FormAdapterPayment } +/** + * What an adapter notification target is for. + * + * `submission` targets are the addresses managing the form - the form's + * notification email and its outputs. `confirmation` is the receipt sent to + * the person who submitted the form, which is a different email entirely. + * + * The engine only ever emits `submission` targets: the confirmation address is + * not known at the point the message is formatted, and is attached downstream + * by the publishing application. Adapters that send the confirmation email add + * their own target so they can track it alongside the rest. + */ +export type FormAdapterNotificationTargetType = 'submission' | 'confirmation' + +/** + * An address this submission should be sent to, with the delivery progress an + * adapter has made against it. + * + * Extends the model's `SubmitNotificationTarget` - the immutable record of + * where the submission was destined, as stored against the submission - with + * the mutable state an adapter needs to retry individual addresses without + * resending to the ones that already succeeded. + * + * The progress properties are absent on a first delivery, and are only written + * by an adapter republishing a partially-sent message. + */ +export interface FormAdapterNotificationTarget extends SubmitNotificationTarget { + /** + * What this target is for. Absent means `submission` - the engine emits no + * type, so a message that has never been through an adapter has none. + */ + type?: FormAdapterNotificationTargetType + + /** + * Whether this address has already been sent to successfully. A target + * marked `true` must not be sent to again. + */ + sent?: boolean + + /** + * How many delivery attempts have been made against this address, across + * every time the message has been processed. + */ + sendAttempts?: number +} + export interface FormAdapterSubmissionMessagePayload { meta: FormAdapterSubmissionMessageMeta data: FormAdapterSubmissionMessageData result: FormAdapterSubmissionMessageResult + + /** + * Where this submission should be sent, resolved at the point of submission + * with any output conditions already evaluated. + * + * Required from {@link FormAdapterSubmissionSchemaVersion.V2}. Absent on V1 + * messages, which consumers must still handle by resolving the recipients + * from the form definition themselves. + */ + notificationTargets?: FormAdapterNotificationTarget[] } export interface FormAdapterSubmissionMessage extends FormAdapterSubmissionMessagePayload { diff --git a/src/server/plugins/engine/types/enums.ts b/src/server/plugins/engine/types/enums.ts index c7b38428e..f7a20fa1f 100644 --- a/src/server/plugins/engine/types/enums.ts +++ b/src/server/plugins/engine/types/enums.ts @@ -11,5 +11,11 @@ export enum FileStatus { } export enum FormAdapterSubmissionSchemaVersion { - V1 = 1 + V1 = 1, + + /** + * Adds `notificationTargets` - the resolved list of addresses the submission + * should be sent to, with any output conditions already evaluated. + */ + V2 = 2 } diff --git a/src/server/plugins/engine/types/schema.test.ts b/src/server/plugins/engine/types/schema.test.ts index 65c4988e4..9af2c52e7 100644 --- a/src/server/plugins/engine/types/schema.test.ts +++ b/src/server/plugins/engine/types/schema.test.ts @@ -7,6 +7,8 @@ import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' import { + type FormAdapterNotificationTarget, + type FormAdapterNotificationTargetType, type FormAdapterSubmissionMessageData, type FormAdapterSubmissionMessageMeta, type FormAdapterSubmissionMessagePayload, @@ -253,4 +255,132 @@ describe('Schema validation', () => { expect(error?.message).toContain('must be a number') }) }) + + describe('notificationTargets', () => { + const baseMeta: FormAdapterSubmissionMessageMeta = { + schemaVersion: FormAdapterSubmissionSchemaVersion.V2, + timestamp: new Date('2025-08-22T18:15:10.785Z'), + referenceNumber: '576-225-943', + formName: 'Order a pizza', + formId: '68a8b0449ab460290c28940a', + formSlug: 'order-a-pizza', + status: FormStatus.Live, + isPreview: false, + notificationEmail: 'info@example.com' + } + + const v1Meta = { + ...baseMeta, + schemaVersion: FormAdapterSubmissionSchemaVersion.V1 + } + + const result = { + files: { main: '3d289230-83a3-4852-a68a-cb3569e9b0fe', repeaters: {} } + } + + const target: FormAdapterNotificationTarget = { + emailAddress: 'info@example.com', + audience: 'human', + version: '2' + } + + const payloadV2 = ( + notificationTargets: FormAdapterNotificationTarget[] + ) => ({ meta: baseMeta, data: validData, result, notificationTargets }) + + it('accepts a V2 payload carrying targets', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([target]) + ) + expect(error).toBeUndefined() + }) + + it('accepts an empty target list - a form can resolve to no recipients', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([]) + ) + expect(error).toBeUndefined() + }) + + it('rejects a V2 payload with no targets property at all', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + meta: baseMeta, + data: validData, + result + }) + expect(error).toBeDefined() + expect(error?.message).toContain('"notificationTargets" is required') + }) + + it('rejects targets on a V1 payload, so an old message cannot carry them', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + meta: v1Meta, + data: validData, + result, + notificationTargets: [target] + }) + expect(error).toBeDefined() + expect(error?.message).toContain('"notificationTargets" is not allowed') + }) + + it('accepts a V1 payload without targets, keeping in-flight messages valid', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + meta: v1Meta, + data: validData, + result + }) + expect(error).toBeUndefined() + }) + + it('accepts adapter progress state on a target', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([ + { ...target, type: 'confirmation', sent: true, sendAttempts: 3 } + ]) + ) + expect(error).toBeUndefined() + }) + + it('preserves progress state under stripUnknown', () => { + // forms-notify-listener validates with stripUnknown. If the schema did + // not know about `sent`, it would be silently dropped on redelivery and + // every requeue would resend to addresses that had already succeeded. + const { value } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([{ ...target, sent: true, sendAttempts: 2 }]), + { stripUnknown: true } + ) + const validated = value as FormAdapterSubmissionMessagePayload + + expect(validated.notificationTargets?.[0]).toEqual({ + ...target, + sent: true, + sendAttempts: 2 + }) + }) + + it('rejects an unknown target type', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([ + { ...target, type: 'nonsense' as FormAdapterNotificationTargetType } + ]) + ) + expect(error).toBeDefined() + }) + + it('rejects a target with no email address', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([ + { audience: 'human', version: '2' } as FormAdapterNotificationTarget + ]) + ) + expect(error).toBeDefined() + }) + + it('rejects a negative send attempt count', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([{ ...target, sendAttempts: -1 }]) + ) + expect(error).toBeDefined() + }) + }) }) diff --git a/src/server/plugins/engine/types/schema.ts b/src/server/plugins/engine/types/schema.ts index 53567e2ca..6027edb7f 100644 --- a/src/server/plugins/engine/types/schema.ts +++ b/src/server/plugins/engine/types/schema.ts @@ -10,6 +10,7 @@ import Joi from 'joi' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' import { + type FormAdapterNotificationTarget, type FormAdapterSubmissionMessageData, type FormAdapterSubmissionMessageMeta, type FormAdapterSubmissionMessagePayload, @@ -18,9 +19,13 @@ import { export const formAdapterSubmissionMessageMetaSchema = Joi.object().keys({ - schemaVersion: Joi.string().valid( - ...Object.values(FormAdapterSubmissionSchemaVersion) - ), + schemaVersion: Joi.number() + .valid( + ...Object.values(FormAdapterSubmissionSchemaVersion).filter( + (version) => typeof version === 'number' + ) + ) + .required(), timestamp: Joi.date().required(), referenceNumber: Joi.string().required(), formName: titleSchema, @@ -76,9 +81,48 @@ export const formAdapterSubmissionMessageResultSchema = .required() }) +export const formAdapterNotificationTargetSchema = + Joi.object().keys({ + emailAddress: Joi.string() + .email({ tlds: { allow: false } }) + .required() + .description('Address the submission should be sent to'), + audience: Joi.string() + .valid('human', 'machine') + .required() + .description( + 'Whether to send the human-readable or machine-processable output' + ), + version: Joi.string() + .required() + .description('Version of the output format to send'), + type: Joi.string() + .valid('submission', 'confirmation') + .optional() + .description('What this target is for. Absent means "submission"'), + sent: Joi.boolean() + .optional() + .description('Whether this address has already been sent to'), + sendAttempts: Joi.number() + .integer() + .min(0) + .optional() + .description('Delivery attempts made against this address so far') + }) + export const formAdapterSubmissionMessagePayloadSchema = Joi.object().keys({ meta: formAdapterSubmissionMessageMetaSchema.required(), data: formAdapterSubmissionMessageDataSchema.required(), - result: formAdapterSubmissionMessageResultSchema.required() + result: formAdapterSubmissionMessageResultSchema.required(), + notificationTargets: Joi.array() + .items(formAdapterNotificationTargetSchema) + .when(Joi.ref('meta.schemaVersion'), { + is: FormAdapterSubmissionSchemaVersion.V2, + then: Joi.required(), + otherwise: Joi.forbidden() + }) + .description( + 'Addresses to send this submission to, with output conditions already evaluated' + ) }) From 64066260ffb98bf387b4703f14d87ec7a96794cc Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Tue, 18 Aug 2026 14:44:00 +0100 Subject: [PATCH 03/12] Fixing up superfluous properties and ensuring that they are on the correct model. --- package-lock.json | 2 +- .../outputFormatters/adapter/v2.test.ts | 58 ++++++++++++++++++- .../engine/outputFormatters/adapter/v2.ts | 14 ++++- .../SummaryPageController.test.ts | 51 ---------------- .../pageControllers/SummaryPageController.ts | 22 +------ src/server/plugins/engine/types.ts | 13 +++++ src/server/plugins/engine/types/enums.ts | 4 +- .../plugins/engine/types/schema.test.ts | 47 +++++++++++++++ src/server/plugins/engine/types/schema.ts | 31 +++++----- 9 files changed, 152 insertions(+), 90 deletions(-) diff --git a/package-lock.json b/package-lock.json index d0e5de31f..741d3d0bb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -153,7 +153,7 @@ }, "engines": { "node": ">=22.11.0 <25.0.0", - "npm": ">=10.9.0 <=11.17.0" + "npm": ">=10.9.0 <=12" } }, "node_modules/@11ty/gray-matter": { diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts index 72744b18a..889da1027 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts @@ -16,8 +16,12 @@ import { format } from '~/src/server/plugins/engine/outputFormatters/adapter/v2. import { buildFormContextRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/index.js' import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' -import { type FormAdapterSubmissionMessagePayload } from '~/src/server/plugins/engine/types.js' +import { + type FormAdapterSubmissionMessagePayload, + type FormContext +} from '~/src/server/plugins/engine/types.js' import { FormStatus } from '~/src/server/routes/types.js' +import joinedConditionsDefinition from '~/test/form/definitions/joined-conditions-simple-v2.js' import definition from '~/test/form/definitions/repeat-mixed.js' const submitResponse = { @@ -217,4 +221,56 @@ describe('Adapter v2 formatter', () => { expect(error).toBeUndefined() }) }) + + describe('conditionEvaluations', () => { + const v2Model = new FormModel(joinedConditionsDefinition, { + basePath: 'test' + }) + + // buildPayload only reads the reference number and translator from the + // context, so the page-walk state the real engine would carry is not needed + const v2Context = { + referenceNumber: 'foobar', + evaluationState: { userName: 'Bob', isOverEighteen: true } + } as unknown as FormContext + + const formatV2Definition = () => + JSON.parse( + format(v2Context, items, v2Model, submitResponse, formStatus, { + id: '68a8b0449ab460290c28940a', + slug: 'joined-conditions', + notificationEmail: 'submissions@example.com' + } as FormMetadata) + ) as FormAdapterSubmissionMessagePayload + + it('records the outcome of every condition for a V2 definition', () => { + const { conditionEvaluations } = formatV2Definition() + + expect(conditionEvaluations).toHaveLength(3) + expect(conditionEvaluations?.[0]).toMatchObject({ + conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', + outcome: 'true', + references: [ + { + componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', + componentName: 'userName', + answered: true + } + ] + }) + }) + + it('is omitted for a V1 definition', () => { + expect(formatWith()).not.toHaveProperty('conditionEvaluations') + }) + + it('produces a payload the schema accepts', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + formatV2Definition(), + { abortEarly: false, allowUnknown: false } + ) + + expect(error).toBeUndefined() + }) + }) }) diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.ts index 05ba510e1..408ec8087 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v2.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v2.ts @@ -1,4 +1,5 @@ import { + Engine, type FormMetadata, type SubmitResponsePayload } from '@defra/forms-model' @@ -7,7 +8,10 @@ import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' import { buildPayload } from '~/src/server/plugins/engine/outputFormatters/adapter/common.js' -import { buildNotificationTargets } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' +import { + buildConditionEvaluations, + buildNotificationTargets +} from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' import { type FormContext } from '~/src/server/plugins/engine/types.js' @@ -50,5 +54,13 @@ export function format( { audience: 'human', version: '2' } ) + // Recorded so the submission record stores why the submission went where it + // did. Condition ids are only stable in V2, so there is nothing to report + // against for a V1 definition. + payload.conditionEvaluations = + model.engine === Engine.V2 + ? buildConditionEvaluations(model, context) + : undefined + return JSON.stringify(payload) } diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts index 828a5f14b..5d83d9674 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.test.ts @@ -590,56 +590,5 @@ describe('SummaryPageController - Payment (DF-832)', () => { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access expect(paymentCall.language).toBe('cy') }) - - it('submits the notification email and qualifying outputs as notification targets', async () => { - const outputs = model.def.outputs - - model.def.outputs = [ - { - emailAddress: 'casework@defra.gov.uk', - audience: 'human', - version: '1' - } - ] - - try { - const { request, context, viewModel, formSubmissionSubmit } = - buildSubmitHarness({ captured: true }) - - const formMetadata = { - contact: { online: { url: '/help' } }, - notificationEmail: 'notify@defra.gov.uk' - } as unknown as Parameters[1] - - await submitForm( - context, - formMetadata, - request, - viewModel, - model, - 'notify@defra.gov.uk', - translator - ) - - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const paymentCall = formSubmissionSubmit.mock.calls[0][0] - - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - expect(paymentCall.notificationTargets).toEqual([ - { - emailAddress: 'notify@defra.gov.uk', - audience: 'human', - version: '1' - }, - { - emailAddress: 'casework@defra.gov.uk', - audience: 'human', - version: '1' - } - ]) - } finally { - model.def.outputs = outputs - } - }) }) }) diff --git a/src/server/plugins/engine/pageControllers/SummaryPageController.ts b/src/server/plugins/engine/pageControllers/SummaryPageController.ts index 42b9a9278..4c63d4bd4 100644 --- a/src/server/plugins/engine/pageControllers/SummaryPageController.ts +++ b/src/server/plugins/engine/pageControllers/SummaryPageController.ts @@ -1,5 +1,4 @@ import { - Engine, hasComponentsEvenIfNoNext, type FormMetadata, type Page, @@ -39,9 +38,7 @@ import { PaymentSubmissionError } from '~/src/server/plugins/engine/pageControllers/errors.js' import { - buildConditionEvaluations, buildMainRecords, - buildNotificationTargets, buildRepeaterRecords } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' import { @@ -476,9 +473,7 @@ export async function submitForm( emailAddress, request.yar.id, translator, - summaryViewModel.context.referenceNumber, - context, - formMetadata + summaryViewModel.context.referenceNumber ) if (submitResponse === undefined) { @@ -558,9 +553,7 @@ function submitData( retrievalKey: string, sessionId: string, translator: Translator, - referenceNumber: string, - context: FormContext, - formMetadata: FormMetadata + referenceNumber: string ) { const { formSubmissionService } = model.services const { submit } = formSubmissionService @@ -571,17 +564,6 @@ function submitData( referenceNumber, main: buildMainRecords(items, translator), repeaters: buildRepeaterRecords(items, translator), - // Condition ids are only stable in V2, so there is nothing to report - // against for a V1 definition - conditionEvaluations: - model.engine === Engine.V2 - ? buildConditionEvaluations(model, context) - : undefined, - notificationTargets: buildNotificationTargets( - model, - context, - formMetadata.notificationEmail - ), // Only populate the language property if the form is multi-language enabled // @ts-expect-error - dynamic language property language: model.def.metadata?.translations?.cy diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 3dbbb112e..4dc09c263 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -6,6 +6,7 @@ import { type List, type Page, type PaymentFieldComponent, + type SubmitConditionEvaluation, type SubmitNotificationTarget, type UkAddressFieldComponent } from '@defra/forms-model' @@ -664,6 +665,18 @@ export interface FormAdapterSubmissionMessagePayload { * from the form definition themselves. */ notificationTargets?: FormAdapterNotificationTarget[] + + /** + * The outcome of every condition in the form definition, evaluated against + * the final answers at the point of submission. + * + * Carried on the message so it is stored against the submission record - an + * audit of why the submission went where it did. + * + * From {@link FormAdapterSubmissionSchemaVersion.V2}, and only for V2-engine + * forms - V1 conditions have no stable ids to report against. + */ + conditionEvaluations?: SubmitConditionEvaluation[] } export interface FormAdapterSubmissionMessage extends FormAdapterSubmissionMessagePayload { diff --git a/src/server/plugins/engine/types/enums.ts b/src/server/plugins/engine/types/enums.ts index f7a20fa1f..551de560c 100644 --- a/src/server/plugins/engine/types/enums.ts +++ b/src/server/plugins/engine/types/enums.ts @@ -15,7 +15,9 @@ export enum FormAdapterSubmissionSchemaVersion { /** * Adds `notificationTargets` - the resolved list of addresses the submission - * should be sent to, with any output conditions already evaluated. + * should be sent to, with any output conditions already evaluated - and + * `conditionEvaluations`, the recorded outcome of every condition at the + * point of submission. */ V2 = 2 } diff --git a/src/server/plugins/engine/types/schema.test.ts b/src/server/plugins/engine/types/schema.test.ts index 9af2c52e7..be7a459c3 100644 --- a/src/server/plugins/engine/types/schema.test.ts +++ b/src/server/plugins/engine/types/schema.test.ts @@ -382,5 +382,52 @@ describe('Schema validation', () => { ) expect(error).toBeDefined() }) + + describe('conditionEvaluations', () => { + const evaluation = { + conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', + outcome: 'true', + references: [ + { + componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', + componentName: 'userName', + answered: true + } + ] + } + + it('accepts a V2 payload carrying condition evaluations', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + ...payloadV2([target]), + conditionEvaluations: [evaluation] + }) + expect(error).toBeUndefined() + }) + + it('accepts a V2 payload without them - a V1-engine form has none', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payloadV2([target]) + ) + expect(error).toBeUndefined() + }) + + it('rejects them on a V1 payload, so an old message cannot carry them', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + meta: v1Meta, + data: validData, + result, + conditionEvaluations: [evaluation] + }) + expect(error).toBeDefined() + }) + + it('rejects a malformed evaluation', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + ...payloadV2([target]), + conditionEvaluations: [{ conditionId: 'abc' }] + }) + expect(error).toBeDefined() + }) + }) }) }) diff --git a/src/server/plugins/engine/types/schema.ts b/src/server/plugins/engine/types/schema.ts index 6027edb7f..b864d3e95 100644 --- a/src/server/plugins/engine/types/schema.ts +++ b/src/server/plugins/engine/types/schema.ts @@ -1,5 +1,7 @@ import { FormStatus, + formSubmitConditionEvaluationSchema, + formSubmitNotificationTargetSchema, formVersionMetadataSchema, idSchema, notificationEmailAddressSchema, @@ -81,21 +83,8 @@ export const formAdapterSubmissionMessageResultSchema = .required() }) -export const formAdapterNotificationTargetSchema = - Joi.object().keys({ - emailAddress: Joi.string() - .email({ tlds: { allow: false } }) - .required() - .description('Address the submission should be sent to'), - audience: Joi.string() - .valid('human', 'machine') - .required() - .description( - 'Whether to send the human-readable or machine-processable output' - ), - version: Joi.string() - .required() - .description('Version of the output format to send'), +export const formAdapterNotificationTargetSchema: Joi.ObjectSchema = + formSubmitNotificationTargetSchema.append({ type: Joi.string() .valid('submission', 'confirmation') .optional() @@ -124,5 +113,17 @@ export const formAdapterSubmissionMessagePayloadSchema = }) .description( 'Addresses to send this submission to, with output conditions already evaluated' + ), + conditionEvaluations: Joi.array() + .items(formSubmitConditionEvaluationSchema) + .when(Joi.ref('meta.schemaVersion'), { + is: FormAdapterSubmissionSchemaVersion.V2, + // Optional even on V2 - only V2 *engine* forms have stable condition + // ids to report against, and a V2 message can carry a V1-engine form + then: Joi.optional(), + otherwise: Joi.forbidden() + }) + .description( + 'Outcome of every condition in the form definition, evaluated against the final answers at submission' ) }) From 66b7eeb7906b6c3db0000d0c883c11a1d3e48270 Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Tue, 18 Aug 2026 15:56:34 +0100 Subject: [PATCH 04/12] Fixing tests --- test/form/govuk-notify.test.js | 10 +--------- test/form/journey-basic.test.js | 10 +--------- test/form/persist-files.test.js | 10 +--------- test/form/repeat.test.js | 10 +--------- 4 files changed, 4 insertions(+), 36 deletions(-) diff --git a/test/form/govuk-notify.test.js b/test/form/govuk-notify.test.js index c4bcad09e..472d452ee 100644 --- a/test/form/govuk-notify.test.js +++ b/test/form/govuk-notify.test.js @@ -342,15 +342,7 @@ describe('Submission journey test', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined, - conditionEvaluations: undefined, - notificationTargets: [ - { - emailAddress: 'enrique.chase@defra.gov.uk', - audience: 'human', - version: '1' - } - ] + language: undefined }) // Status page diff --git a/test/form/journey-basic.test.js b/test/form/journey-basic.test.js index 4998536f8..dffa882b6 100644 --- a/test/form/journey-basic.test.js +++ b/test/form/journey-basic.test.js @@ -417,15 +417,7 @@ describe('Form journey', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined, - conditionEvaluations: undefined, - notificationTargets: [ - { - emailAddress: 'enrique.chase@defra.gov.uk', - audience: 'human', - version: '1' - } - ] + language: undefined }) expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) diff --git a/test/form/persist-files.test.js b/test/form/persist-files.test.js index 425399680..cf8c3fb7e 100644 --- a/test/form/persist-files.test.js +++ b/test/form/persist-files.test.js @@ -197,15 +197,7 @@ describe('Submission journey test', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined, - conditionEvaluations: undefined, - notificationTargets: [ - { - emailAddress: 'enrique.chase@defra.gov.uk', - audience: 'human', - version: '1' - } - ] + language: undefined }) expect(submitRes.statusCode).toBe(StatusCodes.SEE_OTHER) diff --git a/test/form/repeat.test.js b/test/form/repeat.test.js index 2cbf36848..626ebff23 100644 --- a/test/form/repeat.test.js +++ b/test/form/repeat.test.js @@ -632,15 +632,7 @@ describe('Repeat POST tests', () => { retrievalKey: 'enrique.chase@defra.gov.uk', sessionId: expect.any(String), referenceNumber: expect.any(String), - language: undefined, - conditionEvaluations: undefined, - notificationTargets: [ - { - emailAddress: 'enrique.chase@defra.gov.uk', - audience: 'human', - version: '1' - } - ] + language: undefined }) }) From 55fc2b46bc9c8228574d2729aa61a698f948fa0d Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Tue, 18 Aug 2026 20:51:59 +0100 Subject: [PATCH 05/12] Make the default e-mail address (from form metadata) a fallback only, instead of always being included --- .../outputFormatters/adapter/v2.test.ts | 9 +--- .../helpers/submission.test.ts | 47 +++++++++++-------- .../pageControllers/helpers/submission.ts | 44 +++++++++-------- 3 files changed, 55 insertions(+), 45 deletions(-) diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts index 889da1027..aa2fc1fcd 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts @@ -145,7 +145,7 @@ describe('Adapter v2 formatter', () => { }) describe('notificationTargets', () => { - it('always includes the form notification email', () => { + it('includes the form notification email when there are no outputs', () => { expect(formatWith().notificationTargets).toEqual([ { emailAddress: 'submissions@example.com', @@ -174,17 +174,12 @@ describe('Adapter v2 formatter', () => { expect(targets?.[0]).toMatchObject({ audience: 'machine', version: '1' }) }) - it('includes unconditional outputs alongside the notification email', () => { + it('replaces the notification email with the configured outputs', () => { const targets = formatWith([ { emailAddress: 'team@example.com', audience: 'machine', version: '2' } ]).notificationTargets expect(targets).toEqual([ - { - emailAddress: 'submissions@example.com', - audience: 'human', - version: '2' - }, { emailAddress: 'team@example.com', audience: 'machine', version: '2' } ]) }) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts index 6f4cd8e99..d9a96b9df 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts @@ -709,6 +709,27 @@ describe('buildNotificationTargets', () => { ]) }) + it('should drop the notification email once an output qualifies', () => { + // The notification email is a fallback only - outputs replace it rather + // than adding to it, so it must not receive a copy as well. + const model = modelWithOutputs([output('casework@defra.gov.uk')]) + + expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ + target('casework@defra.gov.uk') + ]) + }) + + it('should fall back to the notification email when every output is gated out', () => { + const model = modelWithOutputs([ + output('bob@defra.gov.uk', isBobConditionId), + output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) + ]) + + expect(build(model, { userName: 'Alice', isOverEighteen: false })).toEqual([ + target(notificationEmail) + ]) + }) + it('should include unconditional outputs', () => { const model = modelWithOutputs([ output('casework@defra.gov.uk'), @@ -716,7 +737,6 @@ describe('buildNotificationTargets', () => { ]) expect(build(model, { userName: 'Alice', isOverEighteen: false })).toEqual([ - target(notificationEmail), target('casework@defra.gov.uk'), target('archive@defra.gov.uk') ]) @@ -728,7 +748,6 @@ describe('buildNotificationTargets', () => { ]) expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail), target('casework@defra.gov.uk', 'machine', '2') ]) }) @@ -739,7 +758,6 @@ describe('buildNotificationTargets', () => { ]) expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail), target('over-eighteen@defra.gov.uk') ]) }) @@ -762,28 +780,23 @@ describe('buildNotificationTargets', () => { ]) expect(build(model, { userName: 'Bob', isOverEighteen: false })).toEqual([ - target(notificationEmail), target('casework@defra.gov.uk'), target('bob@defra.gov.uk') ]) }) it('should deduplicate an address configured more than once in the same format', () => { - // One duplicate: the notification email is already part of the form - // metadata and will be included in the outputs. Adding it here a second - // time in uppercase. - // - // The casework@defra.gov.uk e-mail will not be seen as a duplicate - // because the second instance has a condtion attached to it. + // The definition rejects the same address twice in the same format + // outright, so the only way to reach a runtime duplicate is a conditional + // output that resolves to an address an unconditional one already covers. + // The casing differs to prove the match is case-insensitive. const model = modelWithOutputs([ - output(notificationEmail.toUpperCase()), output('casework@defra.gov.uk'), - output('casework@defra.gov.uk', isBobConditionId) + output('CASEWORK@DEFRA.GOV.UK', isBobConditionId) ]) expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail), // The first casing is the one kept, which is the one from the metadata (ie lowercase version) - target('casework@defra.gov.uk') + target('casework@defra.gov.uk') // The first casing seen is the one kept ]) }) @@ -796,7 +809,6 @@ describe('buildNotificationTargets', () => { ]) expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail), target(notificationEmail, 'machine', '1'), target('casework@defra.gov.uk'), target('casework@defra.gov.uk', 'machine', '1'), @@ -839,9 +851,6 @@ describe('buildNotificationTargets', () => { { basePath: '/' } ) - expect(build(v1Model, {})).toEqual([ - target(notificationEmail), - target('casework@defra.gov.uk') - ]) + expect(build(v1Model, {})).toEqual([target('casework@defra.gov.uk')]) }) }) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index 735ee34da..c599c1e79 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -208,24 +208,26 @@ export function buildConditionEvaluations( } /** - * Resolves where this submission should be sent: the form's notification email - * ("Submitted forms sent to"), followed by every output that qualifies against - * the final answers. + * Resolves where this submission should be sent: every output that qualifies + * against the final answers, or the form's notification email ("Submitted + * forms sent to") when nothing else qualifies. * - * The notification email carries the same audience and version the form is - * already sent with, so a form with no outputs behaves as it always has. Where - * the definition does not say, `defaultOutput` decides - and it has to be the - * caller's decision, because the consumers disagree: the engine's own notify - * service falls back to human v1, while the adapter message is consumed by - * forms-notify-listener, which has always fallen back to human v2. Getting + * Outputs take over from the notification email entirely - the notification + * email is only a fallback, so that a form with no outputs, or one whose + * outputs are all gated behind conditions that failed, still has somewhere to + * go rather than being dropped. + * + * That fallback carries the same audience and version the form is already sent + * with. Where the definition does not say, `defaultOutput` decides - and it has + * to be the caller's decision, because the consumers disagree: the engine's own + * notify service falls back to human v1, while the adapter message is consumed + * by forms-notify-listener, which has always fallen back to human v2. Getting * this wrong silently changes the format recipients receive. * @see {@link file://./../../services/notifyService.ts} * * Targets are deduplicated on address, audience and version together, keeping - * the first casing of the address seen. An address configured both as the - * notification email and as an output should not receive the same email twice, - * but the same address may legitimately receive both the human-readable and - * the machine-processable output. + * the first casing of the address seen. The same address may legitimately + * receive both the human-readable and the machine-processable output. * * Applies to V1 and V2. V1 outputs carry no condition, so they all qualify. */ @@ -255,18 +257,22 @@ export function buildNotificationTargets( } } - add( - notificationEmail, - model.def.output?.audience ?? defaultOutput.audience, - model.def.output?.version ?? defaultOutput.version - ) - for (const output of model.def.outputs ?? []) { if (outputQualifies(model, output, evaluationState)) { add(output.emailAddress, output.audience, output.version) } } + // We only ever want to have the notificationEmail as a fallback if + // there's nowhere else to send the submission. + if (targets.size === 0) { + add( + notificationEmail, + model.def.output?.audience ?? defaultOutput.audience, + model.def.output?.version ?? defaultOutput.version + ) + } + return [...targets.values()] } From 88ae4af2dfffdb7b9e82b5649339b943a23a966f Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Tue, 18 Aug 2026 22:02:33 +0100 Subject: [PATCH 06/12] Joi version --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 741d3d0bb..d422f63e2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -52,7 +52,7 @@ "humanize-duration": "^3.33.1", "i18next": "^26.0.5", "ioredis": "^5.8.2", - "joi": "^17.13.3", + "joi": "^17.13.4", "liquidjs": "^10.24.0", "lodash": "^4.17.21", "lru-cache": "^11.5.1", diff --git a/package.json b/package.json index bfa3e599f..5bf7542d6 100644 --- a/package.json +++ b/package.json @@ -134,7 +134,7 @@ "humanize-duration": "^3.33.1", "i18next": "^26.0.5", "ioredis": "^5.8.2", - "joi": "^17.13.3", + "joi": "^17.13.4", "liquidjs": "^10.24.0", "lodash": "^4.17.21", "lru-cache": "^11.5.1", From 8ae73a669f7f1729a126c70597c83d5d8527b332 Mon Sep 17 00:00:00 2001 From: Aaron Scully Date: Wed, 19 Aug 2026 16:05:26 +0100 Subject: [PATCH 07/12] Updates - submission targets no longer computed in plugin. --- .../engine/outputFormatters/adapter/common.ts | 97 ------- .../outputFormatters/adapter/v1.test.ts | 90 +++++- .../engine/outputFormatters/adapter/v1.ts | 96 ++++++- .../outputFormatters/adapter/v2.test.ts | 271 ------------------ .../engine/outputFormatters/adapter/v2.ts | 66 ----- .../engine/outputFormatters/index.test.ts | 7 - .../plugins/engine/outputFormatters/index.ts | 4 +- .../helpers/submission.test.ts | 225 +-------------- .../pageControllers/helpers/submission.ts | 112 +------- src/server/plugins/engine/types.ts | 67 +---- src/server/plugins/engine/types/enums.ts | 10 +- .../plugins/engine/types/schema.test.ts | 170 +++-------- src/server/plugins/engine/types/schema.ts | 41 +-- 13 files changed, 233 insertions(+), 1023 deletions(-) delete mode 100644 src/server/plugins/engine/outputFormatters/adapter/common.ts delete mode 100644 src/server/plugins/engine/outputFormatters/adapter/v2.test.ts delete mode 100644 src/server/plugins/engine/outputFormatters/adapter/v2.ts diff --git a/src/server/plugins/engine/outputFormatters/adapter/common.ts b/src/server/plugins/engine/outputFormatters/adapter/common.ts deleted file mode 100644 index d633d59e5..000000000 --- a/src/server/plugins/engine/outputFormatters/adapter/common.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { - type FormMetadata, - type SubmitResponsePayload -} from '@defra/forms-model' - -import { EN_GB } from '~/src/server/constants.js' -import { - getFormVersion, - type checkFormStatus -} from '~/src/server/plugins/engine/helpers.js' -import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' -import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' -import { categoriseData } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' -import { type FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' -import { - type FormAdapterSubmissionMessageData, - type FormAdapterSubmissionMessageMeta, - type FormAdapterSubmissionMessagePayload, - type FormAdapterSubmissionMessageResult, - type FormContext -} from '~/src/server/plugins/engine/types.js' - -/** - * The payload every adapter schema version shares. - * - * Later versions add to this rather than reshape it, so each version's - * formatter is only the difference between it and the one before. - */ -export function buildPayload( - schemaVersion: FormAdapterSubmissionSchemaVersion, - context: FormContext, - items: DetailItem[], - model: FormModel, - submitResponse: SubmitResponsePayload, - formStatus: ReturnType, - formMetadata?: FormMetadata -): FormAdapterSubmissionMessagePayload { - const csvFiles = extractCsvFiles(submitResponse) - - const { main: v2Main, ...v2Data } = categoriseData(items) - - const versionMetadata = getFormVersion(model.def) - - const meta: FormAdapterSubmissionMessageMeta = { - schemaVersion, - timestamp: new Date(), - referenceNumber: context.referenceNumber, - formName: model.name, - formId: formMetadata?.id ?? '', - formSlug: formMetadata?.slug ?? '', - status: formStatus.state, - isPreview: formStatus.isPreview, - notificationEmail: formMetadata?.notificationEmail ?? '', - language: context.translator?.language ?? EN_GB - } - - if (versionMetadata) { - meta.versionMetadata = versionMetadata - } - - const main = Object.fromEntries( - Object.entries(v2Main).map(([key, value]) => { - if (value === undefined) { - return [key, null] - } - - return [key, value] - }) - ) - - const data: FormAdapterSubmissionMessageData = { - main, - ...v2Data - } - - const result: FormAdapterSubmissionMessageResult = { - files: csvFiles - } - - return { - meta, - data, - result - } -} - -function extractCsvFiles( - submitResponse: SubmitResponsePayload -): FormAdapterSubmissionMessageResult['files'] { - const result = - submitResponse.result as Partial - - return { - main: result.files?.main ?? '', - repeaters: result.files?.repeaters ?? {} - } -} diff --git a/src/server/plugins/engine/outputFormatters/adapter/v1.test.ts b/src/server/plugins/engine/outputFormatters/adapter/v1.test.ts index 7bd103e25..b3f4d00d4 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v1.test.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v1.test.ts @@ -15,13 +15,16 @@ import { import { format } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' import { buildFormContextRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/index.js' +import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' import { FileStatus, UploadStatus, type FileState, - type FormAdapterSubmissionMessagePayload + type FormAdapterSubmissionMessagePayload, + type FormContext } from '~/src/server/plugins/engine/types.js' import { FormStatus } from '~/src/server/routes/types.js' +import joinedConditionsDefinition from '~/test/form/definitions/joined-conditions-simple-v2.js' import definition from '~/test/form/definitions/repeat-mixed.js' const submitResponse = { @@ -871,3 +874,88 @@ describe('Adapter v1 formatter', () => { }) }) }) + +describe('conditionEvaluations', () => { + const formStatus = { + isPreview: false, + state: FormStatus.Live + } + + const formMetadata = { + id: '68a8b0449ab460290c28940a', + slug: 'order-a-pizza', + notificationEmail: 'submissions@example.com' + } as FormMetadata + + const v2Model = new FormModel(joinedConditionsDefinition, { + basePath: 'test' + }) + + // The formatter only reads the reference number, translator and evaluation + // state from the context, so the full page-walk state is not needed here + const v2Context = { + referenceNumber: 'foobar', + evaluationState: { userName: 'Bob', isOverEighteen: true } + } as unknown as FormContext + + const formatV2Definition = () => + JSON.parse( + format( + v2Context, + items, + v2Model, + submitResponse, + formStatus, + formMetadata + ) + ) as FormAdapterSubmissionMessagePayload + + const formatV1Definition = () => + JSON.parse( + format( + context, + items, + model, + submitResponse as SubmitResponsePayload, + formStatus, + formMetadata + ) + ) as FormAdapterSubmissionMessagePayload + + it('should record the outcome of every condition for a V2 definition', () => { + const { conditionEvaluations } = formatV2Definition() + + expect(conditionEvaluations).toHaveLength(3) + expect(conditionEvaluations?.[0]).toMatchObject({ + conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', + outcome: 'true', + references: [ + { + componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', + componentName: 'userName', + answered: true + } + ] + }) + }) + + it('should be empty, not absent, for a V1 definition', () => { + // forms-notify-listener treats an absent property as "this message + // predates conditional emails" and resolves the recipients itself, so + // every message published from here has to carry the property + expect(formatV1Definition().conditionEvaluations).toEqual([]) + }) + + it('should produce a payload the schema accepts', () => { + // The runner publishes with allowUnknown: false and throws on failure, so + // a formatter and schema that disagree would fail every submission + for (const payload of [formatV1Definition(), formatV2Definition()]) { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate( + payload, + { abortEarly: false, allowUnknown: false } + ) + + expect(error).toBeUndefined() + } + }) +}) diff --git a/src/server/plugins/engine/outputFormatters/adapter/v1.ts b/src/server/plugins/engine/outputFormatters/adapter/v1.ts index a388e4e2d..00a86f827 100644 --- a/src/server/plugins/engine/outputFormatters/adapter/v1.ts +++ b/src/server/plugins/engine/outputFormatters/adapter/v1.ts @@ -1,14 +1,26 @@ import { + Engine, type FormMetadata, type SubmitResponsePayload } from '@defra/forms-model' -import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' +import { EN_GB } from '~/src/server/constants.js' +import { + getFormVersion, + type checkFormStatus +} from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' -import { buildPayload } from '~/src/server/plugins/engine/outputFormatters/adapter/common.js' +import { categoriseData } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' +import { buildConditionEvaluations } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' -import { type FormContext } from '~/src/server/plugins/engine/types.js' +import { + type FormAdapterSubmissionMessageData, + type FormAdapterSubmissionMessageMeta, + type FormAdapterSubmissionMessagePayload, + type FormAdapterSubmissionMessageResult, + type FormContext +} from '~/src/server/plugins/engine/types.js' export function format( context: FormContext, @@ -18,15 +30,77 @@ export function format( formStatus: ReturnType, formMetadata?: FormMetadata ): string { - const payload = buildPayload( - FormAdapterSubmissionSchemaVersion.V1, - context, - items, - model, - submitResponse, - formStatus, - formMetadata + const csvFiles = extractCsvFiles(submitResponse) + + const { main: v2Main, ...v2Data } = categoriseData(items) + + const versionMetadata = getFormVersion(model.def) + + const meta: FormAdapterSubmissionMessageMeta = { + schemaVersion: FormAdapterSubmissionSchemaVersion.V1, + timestamp: new Date(), + referenceNumber: context.referenceNumber, + formName: model.name, + formId: formMetadata?.id ?? '', + formSlug: formMetadata?.slug ?? '', + status: formStatus.state, + isPreview: formStatus.isPreview, + notificationEmail: formMetadata?.notificationEmail ?? '', + language: context.translator?.language ?? EN_GB + } + + if (versionMetadata) { + meta.versionMetadata = versionMetadata + } + + const main = Object.fromEntries( + Object.entries(v2Main).map(([key, value]) => { + if (value === undefined) { + return [key, null] + } + + return [key, value] + }) ) + const data: FormAdapterSubmissionMessageData = { + main, + ...v2Data + } + + const result: FormAdapterSubmissionMessageResult = { + files: csvFiles + } + + const payload: FormAdapterSubmissionMessagePayload = { + meta, + data, + result, + + // Recorded here because only the engine holds the walked evaluation state + // the conditions were judged against; forms-notify-listener receives the + // flat submitted answers and resolves the outputs that qualify from these + // outcomes alone. Condition ids are only stable in V2, so a V1 definition + // has nothing to report - the property is still emitted, because its + // absence is what marks a message as predating this and sends the listener + // down its legacy path. + conditionEvaluations: + model.engine === Engine.V2 + ? buildConditionEvaluations(model, context) + : [] + } + return JSON.stringify(payload) } + +function extractCsvFiles( + submitResponse: SubmitResponsePayload +): FormAdapterSubmissionMessageResult['files'] { + const result = + submitResponse.result as Partial + + return { + main: result.files?.main ?? '', + repeaters: result.files?.repeaters ?? {} + } +} diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts deleted file mode 100644 index aa2fc1fcd..000000000 --- a/src/server/plugins/engine/outputFormatters/adapter/v2.test.ts +++ /dev/null @@ -1,271 +0,0 @@ -import { - type FormDefinition, - type FormMetadata, - type Output, - type SubmitResponsePayload -} from '@defra/forms-model' - -import { type Field } from '~/src/server/plugins/engine/components/helpers/components.js' -import { FormModel } from '~/src/server/plugins/engine/models/index.js' -import { - type DetailItem, - type DetailItemField -} from '~/src/server/plugins/engine/models/types.js' -import { format as formatV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' -import { format } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' -import { buildFormContextRequest } from '~/src/server/plugins/engine/pageControllers/__stubs__/request.js' -import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/index.js' -import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' -import { - type FormAdapterSubmissionMessagePayload, - type FormContext -} from '~/src/server/plugins/engine/types.js' -import { FormStatus } from '~/src/server/routes/types.js' -import joinedConditionsDefinition from '~/test/form/definitions/joined-conditions-simple-v2.js' -import definition from '~/test/form/definitions/repeat-mixed.js' - -const submitResponse = { - message: 'Submit completed', - result: { - files: { - main: '00000000-0000-0000-0000-000000000000', - repeaters: { - exampleRepeat: '11111111-1111-1111-1111-111111111111' - } - } - } -} as SubmitResponsePayload - -const formStatus = { - isPreview: false, - state: FormStatus.Live -} - -const dummyField: Field = { - getFormValueFromState: (_) => 'hello world' -} as Field - -const items: DetailItem[] = [ - { - name: 'exampleField', - label: 'Example Field', - href: '/example-field', - title: 'Example Field Title', - field: dummyField, - value: 'Example Value' - } as DetailItemField -] - -const model = new FormModel(definition, { basePath: 'test' }) - -const pageUrl = new URL('http://example.com/repeat/pizza-order/summary') - -const request = buildFormContextRequest({ - method: 'get', - url: pageUrl, - path: pageUrl.pathname, - params: { - path: 'pizza-order', - slug: 'repeat' - }, - query: {}, - app: { model } -}) - -const context = model.getFormContext(request, { - $$__referenceNumber: 'foobar', - orderType: 'delivery' -}) - -/** - * Formats against a copy of the definition carrying the given outputs, so the - * shared `model` is left alone for the other tests in this file. - */ -function formatWith( - outputs?: Output[], - output?: FormDefinition['output'], - notificationEmail = 'submissions@example.com' -) { - const withOutputs = new FormModel( - { ...definition, outputs, output }, - { basePath: 'test' } - ) - - const body = format(context, items, withOutputs, submitResponse, formStatus, { - id: '68a8b0449ab460290c28940a', - slug: 'order-a-pizza', - notificationEmail - } as FormMetadata) - - return JSON.parse(body) as FormAdapterSubmissionMessagePayload -} - -describe('Adapter v2 formatter', () => { - beforeEach(() => { - jest.useFakeTimers() - jest.setSystemTime(new Date('2024-01-15T10:30:00.000Z')) - }) - - afterEach(() => { - jest.useRealTimers() - }) - - it('is the v1 payload plus notificationTargets', () => { - const formMetadata = { - id: '68a8b0449ab460290c28940a', - slug: 'order-a-pizza', - notificationEmail: 'submissions@example.com' - } as FormMetadata - - const v1 = JSON.parse( - formatV1(context, items, model, submitResponse, formStatus, formMetadata) - ) as FormAdapterSubmissionMessagePayload - - const { notificationTargets, ...rest } = JSON.parse( - format(context, items, model, submitResponse, formStatus, formMetadata) - ) as FormAdapterSubmissionMessagePayload - - expect(notificationTargets).toBeDefined() - expect(rest).toEqual({ - ...v1, - meta: { - ...v1.meta, - schemaVersion: FormAdapterSubmissionSchemaVersion.V2 - } - }) - }) - - it('leaves v1 emitting the v1 schema version', () => { - const v1 = JSON.parse( - formatV1(context, items, model, submitResponse, formStatus) - ) as FormAdapterSubmissionMessagePayload - - expect(v1.meta.schemaVersion).toBe(FormAdapterSubmissionSchemaVersion.V1) - expect(v1.notificationTargets).toBeUndefined() - }) - - describe('notificationTargets', () => { - it('includes the form notification email when there are no outputs', () => { - expect(formatWith().notificationTargets).toEqual([ - { - emailAddress: 'submissions@example.com', - audience: 'human', - version: '2' - } - ]) - }) - - it('falls back to human v2 for the notification email', () => { - // forms-notify-listener has always defaulted a form with no explicit - // `output` to human v2. Defaulting to v1 here would silently change the - // format every such form is sent in. - expect(formatWith().notificationTargets?.[0]).toMatchObject({ - audience: 'human', - version: '2' - }) - }) - - it('honours an explicit output audience and version', () => { - const targets = formatWith(undefined, { - audience: 'machine', - version: '1' - }).notificationTargets - - expect(targets?.[0]).toMatchObject({ audience: 'machine', version: '1' }) - }) - - it('replaces the notification email with the configured outputs', () => { - const targets = formatWith([ - { emailAddress: 'team@example.com', audience: 'machine', version: '2' } - ]).notificationTargets - - expect(targets).toEqual([ - { emailAddress: 'team@example.com', audience: 'machine', version: '2' } - ]) - }) - - it('emits no progress state - that is the adapter’s to write', () => { - const targets = formatWith([ - { emailAddress: 'team@example.com', audience: 'human', version: '2' } - ]).notificationTargets - - for (const target of targets ?? []) { - expect(target).not.toHaveProperty('sent') - expect(target).not.toHaveProperty('sendAttempts') - expect(target).not.toHaveProperty('type') - } - }) - - // Condition evaluation itself is covered against buildNotificationTargets - // in pageControllers/helpers/submission.test.ts - the formatter only wires - // it up, and this file's fixture is a V1 definition, which rejects - // conditioned outputs outright. - - it('produces a payload the schema accepts', () => { - const payload = formatWith([ - { emailAddress: 'team@example.com', audience: 'machine', version: '2' } - ]) - - // The runner publishes with allowUnknown: false and throws on failure, - // so a formatter and schema that disagree would fail every submission - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payload, - { abortEarly: false, allowUnknown: false } - ) - - expect(error).toBeUndefined() - }) - }) - - describe('conditionEvaluations', () => { - const v2Model = new FormModel(joinedConditionsDefinition, { - basePath: 'test' - }) - - // buildPayload only reads the reference number and translator from the - // context, so the page-walk state the real engine would carry is not needed - const v2Context = { - referenceNumber: 'foobar', - evaluationState: { userName: 'Bob', isOverEighteen: true } - } as unknown as FormContext - - const formatV2Definition = () => - JSON.parse( - format(v2Context, items, v2Model, submitResponse, formStatus, { - id: '68a8b0449ab460290c28940a', - slug: 'joined-conditions', - notificationEmail: 'submissions@example.com' - } as FormMetadata) - ) as FormAdapterSubmissionMessagePayload - - it('records the outcome of every condition for a V2 definition', () => { - const { conditionEvaluations } = formatV2Definition() - - expect(conditionEvaluations).toHaveLength(3) - expect(conditionEvaluations?.[0]).toMatchObject({ - conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', - outcome: 'true', - references: [ - { - componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', - componentName: 'userName', - answered: true - } - ] - }) - }) - - it('is omitted for a V1 definition', () => { - expect(formatWith()).not.toHaveProperty('conditionEvaluations') - }) - - it('produces a payload the schema accepts', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - formatV2Definition(), - { abortEarly: false, allowUnknown: false } - ) - - expect(error).toBeUndefined() - }) - }) -}) diff --git a/src/server/plugins/engine/outputFormatters/adapter/v2.ts b/src/server/plugins/engine/outputFormatters/adapter/v2.ts deleted file mode 100644 index 408ec8087..000000000 --- a/src/server/plugins/engine/outputFormatters/adapter/v2.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { - Engine, - type FormMetadata, - type SubmitResponsePayload -} from '@defra/forms-model' - -import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' -import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' -import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' -import { buildPayload } from '~/src/server/plugins/engine/outputFormatters/adapter/common.js' -import { - buildConditionEvaluations, - buildNotificationTargets -} from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' -import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' -import { type FormContext } from '~/src/server/plugins/engine/types.js' - -/** - * Adapter V1 plus `notificationTargets` - see - * {@link FormAdapterSubmissionSchemaVersion.V2}. - */ -export function format( - context: FormContext, - items: DetailItem[], - model: FormModel, - submitResponse: SubmitResponsePayload, - formStatus: ReturnType, - formMetadata?: FormMetadata -): string { - const payload = buildPayload( - FormAdapterSubmissionSchemaVersion.V2, - context, - items, - model, - submitResponse, - formStatus, - formMetadata - ) - - // Resolved here rather than by the adapter so that output conditions are - // evaluated against the answers as they stood at submission. An adapter - // re-reading the definition later would see whatever the form has since been - // edited into, and has no submission state to evaluate against. - payload.notificationTargets = buildNotificationTargets( - model, - context, - formMetadata?.notificationEmail, - // Fallback for the `notificationEmail` target when the definition has no - // `output` block. V1 messages carry no `notificationTargets`, so - // forms-notify-listener recovers them from the live definition and applies - // this same `human`/`2` fallback - see `sendNotifyEmailsLegacy` in - // `src/service/notify-legacy.js` there. Changing either side alone means a - // form with no `output` starts being sent against a different template. - { audience: 'human', version: '2' } - ) - - // Recorded so the submission record stores why the submission went where it - // did. Condition ids are only stable in V2, so there is nothing to report - // against for a V1 definition. - payload.conditionEvaluations = - model.engine === Engine.V2 - ? buildConditionEvaluations(model, context) - : undefined - - return JSON.stringify(payload) -} diff --git a/src/server/plugins/engine/outputFormatters/index.test.ts b/src/server/plugins/engine/outputFormatters/index.test.ts index e3a461f52..2674a7440 100644 --- a/src/server/plugins/engine/outputFormatters/index.test.ts +++ b/src/server/plugins/engine/outputFormatters/index.test.ts @@ -1,5 +1,3 @@ -import { format as formatAdapterV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' -import { format as formatAdapterV2 } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' import { format as formatHumanV1 } from '~/src/server/plugins/engine/outputFormatters/human/v1.js' import { getFormatter } from '~/src/server/plugins/engine/outputFormatters/index.js' @@ -9,11 +7,6 @@ describe('Page controller helpers', () => { expect(formatter).toBe(formatHumanV1) }) - it('should keep each adapter version on its own formatter', () => { - expect(getFormatter('adapter', '1')).toBe(formatAdapterV1) - expect(getFormatter('adapter', '2')).toBe(formatAdapterV2) - }) - it("should return an error if the audience doesn't exist", () => { expect(() => getFormatter('foobar', '1')).toThrow('Unknown audience') }) diff --git a/src/server/plugins/engine/outputFormatters/index.ts b/src/server/plugins/engine/outputFormatters/index.ts index eff305e5d..4356dea02 100644 --- a/src/server/plugins/engine/outputFormatters/index.ts +++ b/src/server/plugins/engine/outputFormatters/index.ts @@ -7,7 +7,6 @@ import { type checkFormStatus } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/index.js' import { type DetailItem } from '~/src/server/plugins/engine/models/types.js' import { format as formatAdapterV1 } from '~/src/server/plugins/engine/outputFormatters/adapter/v1.js' -import { format as formatAdapterV2 } from '~/src/server/plugins/engine/outputFormatters/adapter/v2.js' import { format as formatHumanV1 } from '~/src/server/plugins/engine/outputFormatters/human/v1.js' import { format as formatMachineV1 } from '~/src/server/plugins/engine/outputFormatters/machine/v1.js' import { format as formatMachineV2 } from '~/src/server/plugins/engine/outputFormatters/machine/v2.js' @@ -34,8 +33,7 @@ const formatters: Record< '2': formatMachineV2 }, adapter: { - '1': formatAdapterV1, - '2': formatAdapterV2 + '1': formatAdapterV1 } } diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts index d9a96b9df..ec24d8824 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts @@ -1,10 +1,5 @@ -import { - ConditionEvaluationOutcome, - type Output, - type OutputAudience -} from '@defra/forms-model' +import { ConditionEvaluationOutcome } from '@defra/forms-model' -import { logger } from '~/src/server/common/helpers/logging/logger.js' import { GeospatialField } from '~/src/server/plugins/engine/components/GeospatialField.js' import { PaymentField } from '~/src/server/plugins/engine/components/PaymentField.js' import { TextField } from '~/src/server/plugins/engine/components/TextField.js' @@ -14,7 +9,6 @@ import { type DetailItemField } from '~/src/server/plugins/engine/models/types.j import { buildConditionEvaluations, buildMainRecords, - buildNotificationTargets, buildPaymentRecords, buildRepeaterRecords } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' @@ -637,220 +631,3 @@ describe('buildConditionEvaluations', () => { ).toEqual([]) }) }) - -describe('buildNotificationTargets', () => { - const isBobConditionId = 'd15aff7a-6224-40a2-8e5f-51a5af2f7910' - const isOverEighteenConditionId = 'd1f9fcc7-f098-47e7-9d31-4f5ee57ba985' - const notificationEmail = 'submitted.forms@defra.gov.uk' - - /** - * @param {Output[]} outputs - */ - const modelWithOutputs = (outputs: Output[]) => - new FormModel({ ...joinedConditionsDefinition, outputs }, { basePath: '/' }) - - const output = ( - emailAddress: string, - condition?: string, - audience: OutputAudience = 'human', - version = '1' - ): Output => ({ - emailAddress, - audience, - version, - ...(condition ? { condition } : {}) - }) - - /** - * The notification email defaults to the audience and version the form is - * already sent with - * @param {string} emailAddress - */ - const target = ( - emailAddress: string, - audience: OutputAudience = 'human', - version = '1' - ) => ({ emailAddress, audience, version }) - - /** - * @param {FormModel} model - * @param {FormState} evaluationState - */ - const build = (model: FormModel, evaluationState: FormState) => - buildNotificationTargets( - model, - { evaluationState } as FormContext, - notificationEmail - ) - - beforeEach(() => { - jest.spyOn(logger, 'error').mockImplementation(() => logger) - }) - - afterEach(() => { - jest.restoreAllMocks() - }) - - it('should return the notification email when there are no outputs', () => { - const model = new FormModel(joinedConditionsDefinition, { basePath: '/' }) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail) - ]) - }) - - it('should send the notification email in the format the form is configured for', () => { - const model = new FormModel(joinedConditionsDefinition, { basePath: '/' }) - - model.def.output = { audience: 'machine', version: '2' } - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail, 'machine', '2') - ]) - }) - - it('should drop the notification email once an output qualifies', () => { - // The notification email is a fallback only - outputs replace it rather - // than adding to it, so it must not receive a copy as well. - const model = modelWithOutputs([output('casework@defra.gov.uk')]) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target('casework@defra.gov.uk') - ]) - }) - - it('should fall back to the notification email when every output is gated out', () => { - const model = modelWithOutputs([ - output('bob@defra.gov.uk', isBobConditionId), - output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) - ]) - - expect(build(model, { userName: 'Alice', isOverEighteen: false })).toEqual([ - target(notificationEmail) - ]) - }) - - it('should include unconditional outputs', () => { - const model = modelWithOutputs([ - output('casework@defra.gov.uk'), - output('archive@defra.gov.uk') - ]) - - expect(build(model, { userName: 'Alice', isOverEighteen: false })).toEqual([ - target('casework@defra.gov.uk'), - target('archive@defra.gov.uk') - ]) - }) - - it('should carry the audience and version of each output', () => { - const model = modelWithOutputs([ - output('casework@defra.gov.uk', undefined, 'machine', '2') - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target('casework@defra.gov.uk', 'machine', '2') - ]) - }) - - it('should include a conditional output only when its condition passes', () => { - const model = modelWithOutputs([ - output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target('over-eighteen@defra.gov.uk') - ]) - }) - - it('should exclude a conditional output when its condition fails', () => { - const model = modelWithOutputs([ - output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: false })).toEqual([ - target(notificationEmail) - ]) - }) - - it('should mix conditional and unconditional outputs', () => { - const model = modelWithOutputs([ - output('casework@defra.gov.uk'), - output('bob@defra.gov.uk', isBobConditionId), - output('over-eighteen@defra.gov.uk', isOverEighteenConditionId) - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: false })).toEqual([ - target('casework@defra.gov.uk'), - target('bob@defra.gov.uk') - ]) - }) - - it('should deduplicate an address configured more than once in the same format', () => { - // The definition rejects the same address twice in the same format - // outright, so the only way to reach a runtime duplicate is a conditional - // output that resolves to an address an unconditional one already covers. - // The casing differs to prove the match is case-insensitive. - const model = modelWithOutputs([ - output('casework@defra.gov.uk'), - output('CASEWORK@DEFRA.GOV.UK', isBobConditionId) - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target('casework@defra.gov.uk') // The first casing seen is the one kept - ]) - }) - - it('should keep the same address in different output formats', () => { - const model = modelWithOutputs([ - output(notificationEmail, undefined, 'machine', '1'), - output('casework@defra.gov.uk', undefined, 'human', '1'), - output('casework@defra.gov.uk', undefined, 'machine', '1'), - output('casework@defra.gov.uk', undefined, 'machine', '2') - ]) - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail, 'machine', '1'), - target('casework@defra.gov.uk'), - target('casework@defra.gov.uk', 'machine', '1'), - target('casework@defra.gov.uk', 'machine', '2') - ]) - }) - - it('should exclude an output whose condition no longer exists, and log it', () => { - const model = modelWithOutputs([ - output('casework@defra.gov.uk', isBobConditionId) - ]) - - // The definition validates the reference, so the only way to reach this is - // a condition removed after the model was built - model.def.outputs = [ - output('casework@defra.gov.uk', '8d6b1b17-1d1e-4b7f-a4bc-3b0d1e4f5a6c') - ] - - expect(build(model, { userName: 'Bob', isOverEighteen: true })).toEqual([ - target(notificationEmail) - ]) - - expect(logger.error).toHaveBeenCalledWith( - expect.stringContaining('8d6b1b17-1d1e-4b7f-a4bc-3b0d1e4f5a6c') - ) - }) - - it('should omit the notification email when the form has none', () => { - const model = modelWithOutputs([output('casework@defra.gov.uk')]) - const evaluationState: FormState = { userName: 'Bob', isOverEighteen: true } - - expect( - buildNotificationTargets(model, { evaluationState } as FormContext) - ).toEqual([target('casework@defra.gov.uk')]) - }) - - it('should include V1 outputs, which carry no condition', () => { - const v1Model = new FormModel( - { ...definition, outputs: [output('casework@defra.gov.uk')] }, - { basePath: '/' } - ) - - expect(build(v1Model, {})).toEqual([target('casework@defra.gov.uk')]) - }) -}) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index c599c1e79..bc3103407 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -3,15 +3,11 @@ import { type ConditionDataV2, type ConditionRefDataV2, type ConditionWrapperV2, - type Output, - type OutputAudience, type SubmitConditionEvaluation, type SubmitConditionReference, - type SubmitNotificationTarget, type SubmitPayload } from '@defra/forms-model' -import { logger } from '~/src/server/common/helpers/logging/logger.js' import { GeospatialField } from '~/src/server/plugins/engine/components/GeospatialField.js' import { PaymentField } from '~/src/server/plugins/engine/components/PaymentField.js' import { getAnswer } from '~/src/server/plugins/engine/components/helpers/components.js' @@ -164,6 +160,13 @@ export function buildRepeaterRecords( * Records the outcome of every condition in the form definition, evaluated * against the answers as they stand at the point of submission. * + * Evaluated here rather than by the consumer because only the engine holds + * the context to evaluate against: `evaluationState` is built by walking the + * form from its start page along the path taken, applying page conditions as + * it goes. A consumer receives the flat submitted answers, not that walked + * state, and reproducing the walk in a second codebase would leave two + * implementations of the same logic free to diverge. + * * Each record carries the components the condition depends on and whether each * was answered. An unanswered question still yields a boolean - negative * operators such as "is not" return `true` against the seeded `null` - so the @@ -207,75 +210,6 @@ export function buildConditionEvaluations( }) } -/** - * Resolves where this submission should be sent: every output that qualifies - * against the final answers, or the form's notification email ("Submitted - * forms sent to") when nothing else qualifies. - * - * Outputs take over from the notification email entirely - the notification - * email is only a fallback, so that a form with no outputs, or one whose - * outputs are all gated behind conditions that failed, still has somewhere to - * go rather than being dropped. - * - * That fallback carries the same audience and version the form is already sent - * with. Where the definition does not say, `defaultOutput` decides - and it has - * to be the caller's decision, because the consumers disagree: the engine's own - * notify service falls back to human v1, while the adapter message is consumed - * by forms-notify-listener, which has always fallen back to human v2. Getting - * this wrong silently changes the format recipients receive. - * @see {@link file://./../../services/notifyService.ts} - * - * Targets are deduplicated on address, audience and version together, keeping - * the first casing of the address seen. The same address may legitimately - * receive both the human-readable and the machine-processable output. - * - * Applies to V1 and V2. V1 outputs carry no condition, so they all qualify. - */ -export function buildNotificationTargets( - model: FormModel, - context: FormContext, - notificationEmail?: string, - defaultOutput: { audience: OutputAudience; version: string } = { - audience: 'human', - version: '1' - } -): SubmitNotificationTarget[] { - const { evaluationState } = context - const targets = new Map() - - const add = ( - emailAddress: string | undefined, - audience: OutputAudience, - version: string - ) => { - if (emailAddress) { - const key = `${emailAddress.toLowerCase()}|${audience}|${version}` - - if (!targets.has(key)) { - targets.set(key, { emailAddress, audience, version }) - } - } - } - - for (const output of model.def.outputs ?? []) { - if (outputQualifies(model, output, evaluationState)) { - add(output.emailAddress, output.audience, output.version) - } - } - - // We only ever want to have the notificationEmail as a fallback if - // there's nowhere else to send the submission. - if (targets.size === 0) { - add( - notificationEmail, - model.def.output?.audience ?? defaultOutput.audience, - model.def.output?.version ?? defaultOutput.version - ) - } - - return [...targets.values()] -} - /** * Whether a component held an answer at the point a condition was evaluated. * @@ -346,35 +280,3 @@ function collectReferences( return references } - -/** - * Whether an output should receive this submission. - * - * An output with no condition is unconditional. An output whose condition - * cannot be resolved is treated as not qualifying: the gate the author put on - * that address cannot be shown to have passed, and sending anyway would leak - * the submission to a recipient who was meant to be filtered out. The - * definition validates output condition references, so this should not happen - * and is logged as an error. - */ -function outputQualifies( - model: FormModel, - output: Output, - evaluationState: FormState -) { - if (!output.condition) { - return true - } - - const condition = model.conditions[output.condition] - - if (!condition) { - logger.error( - `Form "${model.name}" has an output conditioned on "${output.condition}", which is not a condition in the definition. The output has been excluded from this submission.` - ) - - return false - } - - return condition.fn(evaluationState) -} diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 4dc09c263..3dceb96c2 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -7,7 +7,6 @@ import { type Page, type PaymentFieldComponent, type SubmitConditionEvaluation, - type SubmitNotificationTarget, type UkAddressFieldComponent } from '@defra/forms-model' import { @@ -605,76 +604,20 @@ export interface FormAdapterSubmissionMessageData { payment?: FormAdapterPayment } -/** - * What an adapter notification target is for. - * - * `submission` targets are the addresses managing the form - the form's - * notification email and its outputs. `confirmation` is the receipt sent to - * the person who submitted the form, which is a different email entirely. - * - * The engine only ever emits `submission` targets: the confirmation address is - * not known at the point the message is formatted, and is attached downstream - * by the publishing application. Adapters that send the confirmation email add - * their own target so they can track it alongside the rest. - */ -export type FormAdapterNotificationTargetType = 'submission' | 'confirmation' - -/** - * An address this submission should be sent to, with the delivery progress an - * adapter has made against it. - * - * Extends the model's `SubmitNotificationTarget` - the immutable record of - * where the submission was destined, as stored against the submission - with - * the mutable state an adapter needs to retry individual addresses without - * resending to the ones that already succeeded. - * - * The progress properties are absent on a first delivery, and are only written - * by an adapter republishing a partially-sent message. - */ -export interface FormAdapterNotificationTarget extends SubmitNotificationTarget { - /** - * What this target is for. Absent means `submission` - the engine emits no - * type, so a message that has never been through an adapter has none. - */ - type?: FormAdapterNotificationTargetType - - /** - * Whether this address has already been sent to successfully. A target - * marked `true` must not be sent to again. - */ - sent?: boolean - - /** - * How many delivery attempts have been made against this address, across - * every time the message has been processed. - */ - sendAttempts?: number -} - export interface FormAdapterSubmissionMessagePayload { meta: FormAdapterSubmissionMessageMeta data: FormAdapterSubmissionMessageData result: FormAdapterSubmissionMessageResult - /** - * Where this submission should be sent, resolved at the point of submission - * with any output conditions already evaluated. - * - * Required from {@link FormAdapterSubmissionSchemaVersion.V2}. Absent on V1 - * messages, which consumers must still handle by resolving the recipients - * from the form definition themselves. - */ - notificationTargets?: FormAdapterNotificationTarget[] - /** * The outcome of every condition in the form definition, evaluated against * the final answers at the point of submission. * - * Carried on the message so it is stored against the submission record - an - * audit of why the submission went where it did. - * - * From {@link FormAdapterSubmissionSchemaVersion.V2}, and only for V2-engine - * forms - V1 conditions have no stable ids to report against. + * Always emitted, empty when there is nothing to report - a V1-engine form + * has no stable condition ids to record against. Optional only for + * backwards compatibility: a message published before this existed carries + * no evaluations at all, and consumers fall back to resolving the recipients + * from the form definition themselves. */ conditionEvaluations?: SubmitConditionEvaluation[] } diff --git a/src/server/plugins/engine/types/enums.ts b/src/server/plugins/engine/types/enums.ts index 551de560c..c7b38428e 100644 --- a/src/server/plugins/engine/types/enums.ts +++ b/src/server/plugins/engine/types/enums.ts @@ -11,13 +11,5 @@ export enum FileStatus { } export enum FormAdapterSubmissionSchemaVersion { - V1 = 1, - - /** - * Adds `notificationTargets` - the resolved list of addresses the submission - * should be sent to, with any output conditions already evaluated - and - * `conditionEvaluations`, the recorded outcome of every condition at the - * point of submission. - */ - V2 = 2 + V1 = 1 } diff --git a/src/server/plugins/engine/types/schema.test.ts b/src/server/plugins/engine/types/schema.test.ts index be7a459c3..2a0d43ede 100644 --- a/src/server/plugins/engine/types/schema.test.ts +++ b/src/server/plugins/engine/types/schema.test.ts @@ -7,8 +7,6 @@ import { formAdapterSubmissionMessagePayloadSchema } from '~/src/server/plugins/engine/types/schema.js' import { - type FormAdapterNotificationTarget, - type FormAdapterNotificationTargetType, type FormAdapterSubmissionMessageData, type FormAdapterSubmissionMessageMeta, type FormAdapterSubmissionMessagePayload, @@ -256,9 +254,9 @@ describe('Schema validation', () => { }) }) - describe('notificationTargets', () => { - const baseMeta: FormAdapterSubmissionMessageMeta = { - schemaVersion: FormAdapterSubmissionSchemaVersion.V2, + describe('conditionEvaluations', () => { + const meta: FormAdapterSubmissionMessageMeta = { + schemaVersion: FormAdapterSubmissionSchemaVersion.V1, timestamp: new Date('2025-08-22T18:15:10.785Z'), referenceNumber: '576-225-943', formName: 'Order a pizza', @@ -269,165 +267,73 @@ describe('Schema validation', () => { notificationEmail: 'info@example.com' } - const v1Meta = { - ...baseMeta, - schemaVersion: FormAdapterSubmissionSchemaVersion.V1 - } - const result = { files: { main: '3d289230-83a3-4852-a68a-cb3569e9b0fe', repeaters: {} } } - const target: FormAdapterNotificationTarget = { - emailAddress: 'info@example.com', - audience: 'human', - version: '2' + const evaluation = { + conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', + outcome: 'true', + references: [ + { + componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', + componentName: 'userName', + answered: true + } + ] } - const payloadV2 = ( - notificationTargets: FormAdapterNotificationTarget[] - ) => ({ meta: baseMeta, data: validData, result, notificationTargets }) - - it('accepts a V2 payload carrying targets', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([target]) - ) - expect(error).toBeUndefined() - }) - - it('accepts an empty target list - a form can resolve to no recipients', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([]) - ) - expect(error).toBeUndefined() - }) - - it('rejects a V2 payload with no targets property at all', () => { + it('accepts a payload carrying condition evaluations', () => { const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - meta: baseMeta, + meta, data: validData, - result + result, + conditionEvaluations: [evaluation] }) - expect(error).toBeDefined() - expect(error?.message).toContain('"notificationTargets" is required') + expect(error).toBeUndefined() }) - it('rejects targets on a V1 payload, so an old message cannot carry them', () => { + it('accepts an empty list - a V1-engine form has nothing to report', () => { const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - meta: v1Meta, + meta, data: validData, result, - notificationTargets: [target] + conditionEvaluations: [] }) - expect(error).toBeDefined() - expect(error?.message).toContain('"notificationTargets" is not allowed') + expect(error).toBeUndefined() }) - it('accepts a V1 payload without targets, keeping in-flight messages valid', () => { + it('accepts a payload without them, keeping older messages valid', () => { const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - meta: v1Meta, + meta, data: validData, result }) expect(error).toBeUndefined() }) - it('accepts adapter progress state on a target', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([ - { ...target, type: 'confirmation', sent: true, sendAttempts: 3 } - ]) - ) - expect(error).toBeUndefined() - }) - - it('preserves progress state under stripUnknown', () => { - // forms-notify-listener validates with stripUnknown. If the schema did - // not know about `sent`, it would be silently dropped on redelivery and - // every requeue would resend to addresses that had already succeeded. + it('preserves the evaluations under stripUnknown', () => { + // forms-notify-listener and forms-submission-api both validate with + // stripUnknown. If the schema did not know about the evaluations they + // would be silently dropped, and the listener would fall back to + // resolving recipients from the live definition. const { value } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([{ ...target, sent: true, sendAttempts: 2 }]), + { meta, data: validData, result, conditionEvaluations: [evaluation] }, { stripUnknown: true } ) const validated = value as FormAdapterSubmissionMessagePayload - expect(validated.notificationTargets?.[0]).toEqual({ - ...target, - sent: true, - sendAttempts: 2 - }) - }) - - it('rejects an unknown target type', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([ - { ...target, type: 'nonsense' as FormAdapterNotificationTargetType } - ]) - ) - expect(error).toBeDefined() + expect(validated.conditionEvaluations).toEqual([evaluation]) }) - it('rejects a target with no email address', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([ - { audience: 'human', version: '2' } as FormAdapterNotificationTarget - ]) - ) - expect(error).toBeDefined() - }) - - it('rejects a negative send attempt count', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([{ ...target, sendAttempts: -1 }]) - ) - expect(error).toBeDefined() - }) - - describe('conditionEvaluations', () => { - const evaluation = { - conditionId: 'd15aff7a-6224-40a2-8e5f-51a5af2f7910', - outcome: 'true', - references: [ - { - componentId: '87b987e8-bcf9-4ff9-92af-57c34c45995a', - componentName: 'userName', - answered: true - } - ] - } - - it('accepts a V2 payload carrying condition evaluations', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - ...payloadV2([target]), - conditionEvaluations: [evaluation] - }) - expect(error).toBeUndefined() - }) - - it('accepts a V2 payload without them - a V1-engine form has none', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate( - payloadV2([target]) - ) - expect(error).toBeUndefined() - }) - - it('rejects them on a V1 payload, so an old message cannot carry them', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - meta: v1Meta, - data: validData, - result, - conditionEvaluations: [evaluation] - }) - expect(error).toBeDefined() - }) - - it('rejects a malformed evaluation', () => { - const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ - ...payloadV2([target]), - conditionEvaluations: [{ conditionId: 'abc' }] - }) - expect(error).toBeDefined() + it('rejects a malformed evaluation', () => { + const { error } = formAdapterSubmissionMessagePayloadSchema.validate({ + meta, + data: validData, + result, + conditionEvaluations: [{ conditionId: 'abc' }] }) + expect(error).toBeDefined() }) }) }) diff --git a/src/server/plugins/engine/types/schema.ts b/src/server/plugins/engine/types/schema.ts index b864d3e95..340362ab5 100644 --- a/src/server/plugins/engine/types/schema.ts +++ b/src/server/plugins/engine/types/schema.ts @@ -1,7 +1,6 @@ import { FormStatus, formSubmitConditionEvaluationSchema, - formSubmitNotificationTargetSchema, formVersionMetadataSchema, idSchema, notificationEmailAddressSchema, @@ -12,7 +11,6 @@ import Joi from 'joi' import { FormAdapterSubmissionSchemaVersion } from '~/src/server/plugins/engine/types/enums.js' import { - type FormAdapterNotificationTarget, type FormAdapterSubmissionMessageData, type FormAdapterSubmissionMessageMeta, type FormAdapterSubmissionMessagePayload, @@ -83,46 +81,19 @@ export const formAdapterSubmissionMessageResultSchema = .required() }) -export const formAdapterNotificationTargetSchema: Joi.ObjectSchema = - formSubmitNotificationTargetSchema.append({ - type: Joi.string() - .valid('submission', 'confirmation') - .optional() - .description('What this target is for. Absent means "submission"'), - sent: Joi.boolean() - .optional() - .description('Whether this address has already been sent to'), - sendAttempts: Joi.number() - .integer() - .min(0) - .optional() - .description('Delivery attempts made against this address so far') - }) - export const formAdapterSubmissionMessagePayloadSchema = Joi.object().keys({ meta: formAdapterSubmissionMessageMetaSchema.required(), data: formAdapterSubmissionMessageDataSchema.required(), result: formAdapterSubmissionMessageResultSchema.required(), - notificationTargets: Joi.array() - .items(formAdapterNotificationTargetSchema) - .when(Joi.ref('meta.schemaVersion'), { - is: FormAdapterSubmissionSchemaVersion.V2, - then: Joi.required(), - otherwise: Joi.forbidden() - }) - .description( - 'Addresses to send this submission to, with output conditions already evaluated' - ), + // Optional so that messages published before this existed - which may + // still be in flight or sitting on a dead-letter queue - continue to + // validate. Consumers treat its absence as "resolve the recipients from + // the form definition yourself" + // This should be required at a later point in time. conditionEvaluations: Joi.array() .items(formSubmitConditionEvaluationSchema) - .when(Joi.ref('meta.schemaVersion'), { - is: FormAdapterSubmissionSchemaVersion.V2, - // Optional even on V2 - only V2 *engine* forms have stable condition - // ids to report against, and a V2 message can carry a V1-engine form - then: Joi.optional(), - otherwise: Joi.forbidden() - }) + .optional() .description( 'Outcome of every condition in the form definition, evaluated against the final answers at submission' ) From f5f3411bb663ebb85b2795ef4e519f943482dc92 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Fri, 21 Aug 2026 15:21:32 +0100 Subject: [PATCH 08/12] Re-sync'd package.json + lock --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index d422f63e2..741d3d0bb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -52,7 +52,7 @@ "humanize-duration": "^3.33.1", "i18next": "^26.0.5", "ioredis": "^5.8.2", - "joi": "^17.13.4", + "joi": "^17.13.3", "liquidjs": "^10.24.0", "lodash": "^4.17.21", "lru-cache": "^11.5.1", diff --git a/package.json b/package.json index 5bf7542d6..bfa3e599f 100644 --- a/package.json +++ b/package.json @@ -134,7 +134,7 @@ "humanize-duration": "^3.33.1", "i18next": "^26.0.5", "ioredis": "^5.8.2", - "joi": "^17.13.4", + "joi": "^17.13.3", "liquidjs": "^10.24.0", "lodash": "^4.17.21", "lru-cache": "^11.5.1", From 58486de29087cfc837f7a0c650d34e32efb0a73c Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Fri, 21 Aug 2026 16:00:19 +0100 Subject: [PATCH 09/12] Amended default params --- .../pageControllers/helpers/submission.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index bc3103407..d5303d7af 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -194,13 +194,7 @@ export function buildConditionEvaluations( const { outcome } = condition.evaluate(evaluationState) - const references = collectReferences( - model, - conditionDef, - evaluationState, - new Map(), - new Set() - ) + const references = collectReferences(model, conditionDef, evaluationState) return { conditionId: conditionDef.id, @@ -237,7 +231,7 @@ function isConditionDataV2( } /** - * Collects every component a condition depends on, following nested condition + * Recursive function that collects every component a condition depends on, following nested condition * references. Results are keyed by component id so a component referenced more * than once is reported once. */ @@ -245,8 +239,11 @@ function collectReferences( model: FormModel, conditionDef: ConditionWrapperV2, evaluationState: FormState, - references: Map, - visited: Set + references: Map = new Map< + string, + SubmitConditionReference + >(), + visited: Set = new Set() ) { if (visited.has(conditionDef.id)) { return references From 0c0eddfa1537cd23764a13a00f7b60f0ec5dae14 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Tue, 1 Sep 2026 12:18:42 +0100 Subject: [PATCH 10/12] Schema/enum correction --- docs/features/configuration-based/page-events.md | 2 +- src/server/plugins/engine/types.ts | 2 +- src/server/plugins/engine/types/enums.ts | 7 ++++--- src/server/plugins/engine/types/schema.ts | 6 +----- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/docs/features/configuration-based/page-events.md b/docs/features/configuration-based/page-events.md index 25dd544b7..322ff9abb 100644 --- a/docs/features/configuration-based/page-events.md +++ b/docs/features/configuration-based/page-events.md @@ -58,7 +58,7 @@ The payload takes the following shape: ```jsonc { "meta": { - "schemaVersion": "2", + "schemaVersion": 1, "timestamp": "2025-03-25T10:00:00Z", "definition": { // This object would be a full copy of the form definition at the time of submission. It is excluded for brevity. diff --git a/src/server/plugins/engine/types.ts b/src/server/plugins/engine/types.ts index 3dceb96c2..bb67c3ab8 100644 --- a/src/server/plugins/engine/types.ts +++ b/src/server/plugins/engine/types.ts @@ -530,7 +530,7 @@ export interface PluginOptions { } export interface FormAdapterSubmissionMessageMeta { - schemaVersion: FormAdapterSubmissionSchemaVersion + schemaVersion: (typeof FormAdapterSubmissionSchemaVersion)[keyof typeof FormAdapterSubmissionSchemaVersion] timestamp: Date referenceNumber: string formName: string diff --git a/src/server/plugins/engine/types/enums.ts b/src/server/plugins/engine/types/enums.ts index c7b38428e..b0ac3a135 100644 --- a/src/server/plugins/engine/types/enums.ts +++ b/src/server/plugins/engine/types/enums.ts @@ -10,6 +10,7 @@ export enum FileStatus { pending = 'pending' } -export enum FormAdapterSubmissionSchemaVersion { - V1 = 1 -} +// Changed from an enum to enforce numeric values +export const FormAdapterSubmissionSchemaVersion = { + V1: 1 +} as const satisfies Record diff --git a/src/server/plugins/engine/types/schema.ts b/src/server/plugins/engine/types/schema.ts index 340362ab5..9257c449c 100644 --- a/src/server/plugins/engine/types/schema.ts +++ b/src/server/plugins/engine/types/schema.ts @@ -20,11 +20,7 @@ import { export const formAdapterSubmissionMessageMetaSchema = Joi.object().keys({ schemaVersion: Joi.number() - .valid( - ...Object.values(FormAdapterSubmissionSchemaVersion).filter( - (version) => typeof version === 'number' - ) - ) + .valid(...Object.values(FormAdapterSubmissionSchemaVersion)) .required(), timestamp: Joi.date().required(), referenceNumber: Joi.string().required(), From 0a2e8185ac0230ea3848230ae4398ea29d884e7e Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Tue, 1 Sep 2026 14:30:15 +0100 Subject: [PATCH 11/12] Model bump --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index fdc458361..2ff8e96c4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "hasInstallScript": true, "license": "SEE LICENSE IN LICENSE", "dependencies": { - "@defra/forms-model": "^3.0.698", + "@defra/forms-model": "^3.0.704", "@defra/hapi-tracing": "^1.29.0", "@defra/interactive-map": "0.0.33-alpha", "@elastic/ecs-pino-format": "^1.5.0", @@ -3697,9 +3697,9 @@ } }, "node_modules/@defra/forms-model": { - "version": "3.0.698", - "resolved": "https://registry.npmjs.org/@defra/forms-model/-/forms-model-3.0.698.tgz", - "integrity": "sha512-HTW7NQrbX7kPbN7Msawyy414GZuiiqRKdOfLmos3W9myY6fTFtBgw8E0r7goHxwuCBePxFZdNgg3KK3t10WFGA==", + "version": "3.0.704", + "resolved": "https://registry.npmjs.org/@defra/forms-model/-/forms-model-3.0.704.tgz", + "integrity": "sha512-+yxRe8TxWMoBwlZl9FSobXWEJoqgTip1uq5GBoBylXUxG/r33oB+Jk2tdX5VqpEEZN7wYkfzOFqACd7Wf2kzfQ==", "license": "OGL-UK-3.0", "dependencies": { "@joi/date": "^2.1.1", diff --git a/package.json b/package.json index a83fc1f0e..74048435d 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ }, "license": "SEE LICENSE IN LICENSE", "dependencies": { - "@defra/forms-model": "^3.0.698", + "@defra/forms-model": "^3.0.704", "@defra/hapi-tracing": "^1.29.0", "@defra/interactive-map": "0.0.33-alpha", "@elastic/ecs-pino-format": "^1.5.0", From ac1f32d142b76148c718b86574448eab8cdf7e87 Mon Sep 17 00:00:00 2001 From: Jez Barnsley Date: Tue, 1 Sep 2026 15:27:17 +0100 Subject: [PATCH 12/12] Extra coverage --- .../helpers/submission.test.ts | 25 ++++++++++++++++++- .../pageControllers/helpers/submission.ts | 2 +- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts index ec24d8824..5d47907ad 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.test.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.test.ts @@ -10,7 +10,8 @@ import { buildConditionEvaluations, buildMainRecords, buildPaymentRecords, - buildRepeaterRecords + buildRepeaterRecords, + isAnswered } from '~/src/server/plugins/engine/pageControllers/helpers/submission.js' import { type FormContext, @@ -500,6 +501,23 @@ describe('buildConditionEvaluations', () => { const build = (evaluationState: FormState) => buildConditionEvaluations(model, { evaluationState } as FormContext) + it('should ignore condition if condition not found', () => { + const badModel = new FormModel(joinedConditionsDefinition, { + basePath: '/' + }) + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete badModel.conditions[isBobConditionId] + const badBuild = (evaluationState: FormState) => + buildConditionEvaluations(badModel, { evaluationState } as FormContext) + + const evaluations = badBuild({ userName: null, isOverEighteen: null }) + + expect(evaluations.map((evaluation) => evaluation.conditionId)).toEqual([ + isOverEighteenConditionId, + joinedConditionId + ]) + }) + it('should record every condition in the definition', () => { const evaluations = build({ userName: null, isOverEighteen: null }) @@ -630,4 +648,9 @@ describe('buildConditionEvaluations', () => { buildConditionEvaluations(v1Model, { evaluationState: {} } as FormContext) ).toEqual([]) }) + + it('isAnswered handle arrays', () => { + expect(isAnswered([])).toBe(false) + expect(isAnswered(['abc'])).toBe(true) + }) }) diff --git a/src/server/plugins/engine/pageControllers/helpers/submission.ts b/src/server/plugins/engine/pageControllers/helpers/submission.ts index d5303d7af..3f3014386 100644 --- a/src/server/plugins/engine/pageControllers/helpers/submission.ts +++ b/src/server/plugins/engine/pageControllers/helpers/submission.ts @@ -212,7 +212,7 @@ export function buildConditionEvaluations( * absent. * @see {@link FormModel.initialiseContext} */ -function isAnswered(value: FormStateValue | undefined) { +export function isAnswered(value: FormStateValue | undefined) { if (value === undefined || value === null) { return false }