From 527404ac225c108adebeecbab4de2ee2e578e18c Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Fri, 24 Apr 2026 11:00:16 +0100 Subject: [PATCH 1/3] refactor(DF-1004): stamp $$__formVersion at write time in modifyDraft Moves version stamping from read-time injection in getFormDefinition to write-time stamping inside modifyDraft and insertDraft. Every draft mutation now atomically allocates a version, stamps the definition, and snapshots to form-versions. Service callers no longer need to call createFormVersion after modifyDraft. getFormDefinition reverts to a pure read. createLiveFromDraft and createDraftFromLive inherit the stamp on copy. Fixes the submission crash where live reads on forms with unsaved drafts were tagged with draft-save version numbers. --- .../form-definition-repository.js | 31 ++++++- .../form-definition-repository.test.js | 6 ++ src/api/forms/repositories/helpers.js | 90 ++++++++++++++++--- src/api/forms/service/component.js | 7 -- src/api/forms/service/conditions.js | 7 -- src/api/forms/service/definition.js | 48 +--------- src/api/forms/service/definition.test.js | 25 +++--- src/api/forms/service/index.js | 5 -- src/api/forms/service/index.test.js | 8 -- src/api/forms/service/lists.js | 7 -- src/api/forms/service/migration.js | 3 - src/api/forms/service/options.js | 3 - src/api/forms/service/page.js | 7 -- src/api/forms/service/sections.js | 3 - src/api/forms/service/sections.test.js | 6 -- src/api/forms/service/versioning.js | 32 ++++--- src/api/forms/service/versioning.test.js | 14 ++- src/helpers/feedback-form/reinstate.js | 14 ++- src/helpers/feedback-form/reinstate.test.js | 3 + src/routes/forms.js | 4 +- 20 files changed, 175 insertions(+), 148 deletions(-) diff --git a/src/api/forms/repositories/form-definition-repository.js b/src/api/forms/repositories/form-definition-repository.js index aa5b1baa..fa8ddfcc 100644 --- a/src/api/forms/repositories/form-definition-repository.js +++ b/src/api/forms/repositories/form-definition-repository.js @@ -3,6 +3,7 @@ import Boom from '@hapi/boom' import { ObjectId } from 'mongodb' import { + FORM_VERSION_METADATA_KEY, buildSectionsResponse, getComponent, getCondition, @@ -76,6 +77,34 @@ export async function update(id, formDefinition, session, schema) { return updateResult.draft } +/** + * Stamps a form version onto the stored definition for the given state. + * No-op if the state sub-document does not exist. + * @param {string} formId - the form id + * @param {FormStatus} state - the form state (Draft or Live) + * @param {FormVersionMetadata} versionMetadata + * @param {ClientSession} session - mongo transaction session + */ +export async function setFormVersion(formId, state, versionMetadata, session) { + const coll = + /** @satisfies {Collection>} */ ( + db.collection(DEFINITION_COLLECTION_NAME) + ) + + await coll.updateOne( + { + _id: new ObjectId(formId), + [state]: { $exists: true } + }, + { + $set: { + [`${state}.metadata.${FORM_VERSION_METADATA_KEY}`]: versionMetadata + } + }, + { session } + ) +} + /** * Copy the draft form to live in the Form Store * @param {string} id - id @@ -727,7 +756,7 @@ export async function updateOption(formId, optionName, optionValue, session) { } /** - * @import { FormDefinition, Page, ComponentDef, PatchPageFields, List, Engine, ConditionWrapperV2, SectionAssignmentItem } from '@defra/forms-model' + * @import { FormDefinition, FormVersionMetadata, Page, ComponentDef, PatchPageFields, List, Engine, ConditionWrapperV2, SectionAssignmentItem } from '@defra/forms-model' * @import { ClientSession, Collection, FindOptions } from 'mongodb' * @import { ObjectSchema } from 'joi' * @import { UpdateCallback, RemovePagePredicate } from '~/src/api/forms/repositories/helpers.js' diff --git a/src/api/forms/repositories/form-definition-repository.test.js b/src/api/forms/repositories/form-definition-repository.test.js index 56b5f9e5..174ad54e 100644 --- a/src/api/forms/repositories/form-definition-repository.test.js +++ b/src/api/forms/repositories/form-definition-repository.test.js @@ -83,6 +83,12 @@ const condition2Id = '91c10139-a0dd-46a4-a2c5-4d7a02fdf923' const section1Id = 'f07566be-dd04-49df-890f-b226b92f3907' const section2Id = 'cb185708-203d-4560-9929-ecc27750244a' +// modifyDraft/insertDraft now call into form-metadata-repository and +// form-versions-repository for version allocation + snapshotting. Mock them so +// their Mongo ops don't collide with the definition-repository assertions. +jest.mock('~/src/api/forms/repositories/form-metadata-repository.js') +jest.mock('~/src/api/forms/repositories/form-versions-repository.js') + jest.mock('~/src/mongo.js', () => { let isPrepared = false const collection = diff --git a/src/api/forms/repositories/helpers.js b/src/api/forms/repositories/helpers.js index b1020577..1e619287 100644 --- a/src/api/forms/repositories/helpers.js +++ b/src/api/forms/repositories/helpers.js @@ -18,11 +18,15 @@ import { import Boom from '@hapi/boom' import { ObjectId } from 'mongodb' +import * as formMetadataRepository from '~/src/api/forms/repositories/form-metadata-repository.js' +import * as formVersionsRepository from '~/src/api/forms/repositories/form-versions-repository.js' import { validate } from '~/src/api/forms/service/helpers/definition.js' import { repositionPaymentAndSummary } from '~/src/api/forms/service/migration-helpers.js' import { createLogger } from '~/src/helpers/logging/logger.js' import { DEFINITION_COLLECTION_NAME, db } from '~/src/mongo.js' +export const FORM_VERSION_METADATA_KEY = '$$__formVersion' + const logger = createLogger() /** @@ -275,7 +279,8 @@ export function uniquePathGate( } /** - * Inserts a draft form definition + * Inserts a draft form definition, stamping the initial version and + * snapshotting to form-versions in the same transaction. * @param {string} formId - the form id * @param {FormDefinition} definition - the form definitiom * @param {ClientSession} session - the mongo transaction session @@ -287,12 +292,13 @@ export async function insertDraft( session, schema = formDefinitionV2Schema ) { - // Validate form definition - const draft = validate(definition, schema) + const validated = validate(definition, schema) + + const versionMetadata = await allocateDraftVersion(formId, session) + const draft = stampFormVersion(validated, versionMetadata) const id = { _id: new ObjectId(formId) } - // Persist the new draft const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( db.collection(DEFINITION_COLLECTION_NAME) ) @@ -311,11 +317,65 @@ export async function insertDraft( throw Boom.notFound(`Unexpected empty result from 'findOneAndUpdate'`) } + await formVersionsRepository.createVersion( + { + formId, + versionNumber: versionMetadata.versionNumber, + formDefinition: draft, + createdAt: versionMetadata.createdAt + }, + session + ) + return insertResult } /** - * Updates a draft form definition + * Allocates the next version number for a form. + * @param {string} formId - the form id + * @param {ClientSession} session - the mongo transaction session + * @returns {Promise} + */ +export async function allocateDraftVersion(formId, session) { + const versionNumber = + await formMetadataRepository.getAndIncrementVersionNumber(formId, session) + const createdAt = new Date() + const versionMetadata = /** @type {FormVersionMetadata} */ ({ + versionNumber, + createdAt + }) + + await formMetadataRepository.addVersionMetadata( + formId, + versionMetadata, + session + ) + + return versionMetadata +} + +/** + * Stamps a form version onto a definition's metadata. + * @param {FormDefinition} definition + * @param {{ versionNumber: number, createdAt: Date }} versionMetadata + * @returns {FormDefinition} + */ +export function stampFormVersion(definition, versionMetadata) { + return { + ...definition, + metadata: { + ...definition.metadata, + [FORM_VERSION_METADATA_KEY]: { + versionNumber: versionMetadata.versionNumber, + createdAt: versionMetadata.createdAt + } + } + } +} + +/** + * Updates a draft form definition, stamping a new version onto it and + * snapshotting to form-versions in the same transaction. * @param {string} formId - the form id * @param {UpdateCallback} updateCallback - the update callback * @param {ClientSession} session - the mongo transaction session @@ -342,15 +402,13 @@ export async function modifyDraft( throw Boom.notFound(`Draft not found in document '${formId}'`) } - // Apply the update const updated = updateCallback(document.draft) - const repositioned = repositionPaymentAndSummary(updated) + const validated = validate(repositioned, schema) - // Validate form definition - const draft = validate(repositioned, schema) + const versionMetadata = await allocateDraftVersion(formId, session) + const draft = stampFormVersion(validated, versionMetadata) - // Persist the updated draft const coll2 = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( db.collection(DEFINITION_COLLECTION_NAME) ) @@ -368,6 +426,16 @@ export async function modifyDraft( throw Boom.notFound(`Unexpected empty result from 'findOneAndUpdate'`) } + await formVersionsRepository.createVersion( + { + formId, + versionNumber: versionMetadata.versionNumber, + formDefinition: draft, + createdAt: versionMetadata.createdAt + }, + session + ) + return updateResult } @@ -898,7 +966,7 @@ export function modifyUpdateOption(definition, optionName, optionValue) { */ /** - * @import { FormDefinition, FormOptions, Page, ComponentDef, List, PatchPageFields, Engine, ConditionWrapperV2, PageSummary, PageSummaryWithConfirmationEmail, SectionAssignmentItem } from '@defra/forms-model' + * @import { FormDefinition, FormOptions, FormVersionMetadata, Page, ComponentDef, List, PatchPageFields, Engine, ConditionWrapperV2, PageSummary, PageSummaryWithConfirmationEmail, SectionAssignmentItem } from '@defra/forms-model' * @import { ClientSession, Collection } from 'mongodb' * @import { ObjectSchema } from 'joi' */ diff --git a/src/api/forms/service/component.js b/src/api/forms/service/component.js index e6f59e52..f817821c 100644 --- a/src/api/forms/service/component.js +++ b/src/api/forms/service/component.js @@ -11,7 +11,6 @@ import { findComponent } from '~/src/api/forms/repositories/helpers.js' import { getFormDefinition } from '~/src/api/forms/service/definition.js' import { getFormDefinitionPage } from '~/src/api/forms/service/page.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' @@ -88,8 +87,6 @@ export async function createComponentOnDraftDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, component, @@ -151,8 +148,6 @@ export async function updateComponentOnDraftDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, formDefinitionPageComponent, @@ -209,8 +204,6 @@ export async function deleteComponentOnDraftDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { pageId, componentId }, diff --git a/src/api/forms/service/conditions.js b/src/api/forms/service/conditions.js index aff922f2..58330bc8 100644 --- a/src/api/forms/service/conditions.js +++ b/src/api/forms/service/conditions.js @@ -3,7 +3,6 @@ import { FormDefinitionRequestType, getErrorMessage } from '@defra/forms-model' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' @@ -37,8 +36,6 @@ export async function addConditionToDraftFormDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, condition, @@ -96,8 +93,6 @@ export async function updateConditionOnDraftFormDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, condition, @@ -148,8 +143,6 @@ export async function removeConditionOnDraftFormDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { conditionId }, diff --git a/src/api/forms/service/definition.js b/src/api/forms/service/definition.js index a3bb366f..fc21070a 100644 --- a/src/api/forms/service/definition.js +++ b/src/api/forms/service/definition.js @@ -11,7 +11,6 @@ import { makeFormLiveErrorMessages } from '~/src/api/forms/constants.js' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import { deleteDraft } from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' -import * as formVersions from '~/src/api/forms/repositories/form-versions-repository.js' import { deleteSecret, exists, @@ -25,7 +24,6 @@ import { mapForm, partialAuditFields } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishDraftCreatedFromLiveEvent, publishFormDraftDeletedEvent, @@ -37,26 +35,6 @@ import { client } from '~/src/mongo.js' export const PAYMENT_LIVE_API_KEY = 'payment-live-api-key' export const PAYMENT_LIVE_API_KEY_PENDING = 'payment-live-api-key-pending' -export const FORM_VERSION_METADATA_KEY = '$$__formVersion' - -/** - * Returns a new definition with version metadata injected into definition.metadata. - * @param {FormDefinition} definition - * @param {{ versionNumber: number, createdAt: Date }} version - * @returns {FormDefinition} - */ -export function injectFormVersion(definition, version) { - return { - ...definition, - metadata: { - ...definition.metadata, - [FORM_VERSION_METADATA_KEY]: { - versionNumber: version.versionNumber, - createdAt: version.createdAt - } - } - } -} /** * Retrieves a paginated list of forms with filter options @@ -72,7 +50,6 @@ export async function listForms(options) { /** * Retrieves the form definition content for a given form ID. - * Injects the latest version metadata into definition.metadata.$$__formVersion. * @param {string} formId - the ID of the form * @param {FormStatus} state - the form state * @param {ClientSession | undefined} [session] @@ -82,16 +59,7 @@ export async function getFormDefinition( state = FormStatus.Draft, session ) { - const [definition, latestVersion] = await Promise.all([ - formDefinition.get(formId, state, session), - formVersions.getLatestVersion(formId, session) - ]) - - if (!latestVersion) { - return definition - } - - return injectFormVersion(definition, latestVersion) + return formDefinition.get(formId, state, session) } /** @@ -129,8 +97,6 @@ export async function updateDraftFormDefinition(formId, definition, author) { session ) - await createFormVersion(formId, session) - // Publish audit message await publishFormDraftReplacedEvent(updatedMetadata, definition) }) @@ -391,8 +357,6 @@ export async function createLiveFromDraft(formId, author) { session ) - await createFormVersion(formId, session) - // Make payment key live if a pending one is stored await makePaymentKeyLive(formHasPayment, formId, session) @@ -448,15 +412,13 @@ export async function createDraftFromLive(formId, author) { try { await session.withTransaction(async () => { - // Copy the draft form definition + // Copy the live definition to draft await formDefinition.createDraftFromLive(formId, session) logger.info(`Adding form metadata (draft) for form ID ${formId}`) await formMetadata.update(formId, { $set: set }, session) - await createFormVersion(formId, session) - // Publish audit message await publishDraftCreatedFromLiveEvent(formId, now, author) }) @@ -513,8 +475,6 @@ export async function reorderDraftFormDefinitionPages( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { pageOrder: orderOfPageIds }, @@ -578,8 +538,6 @@ export async function reorderDraftFormDefinitionSections( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { sectionOrder: orderOfSectionIds }, @@ -646,8 +604,6 @@ export async function reorderDraftFormDefinitionComponents( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { pageId, componentOrder: orderOfComponentIds }, diff --git a/src/api/forms/service/definition.test.js b/src/api/forms/service/definition.test.js index abcd0087..e63075b9 100644 --- a/src/api/forms/service/definition.test.js +++ b/src/api/forms/service/definition.test.js @@ -25,6 +25,7 @@ import * as formMetadata from '~/src/api/forms/repositories/form-metadata-reposi import { MAX_RESULTS } from '~/src/api/forms/repositories/form-metadata-repository.js' import * as formVersions from '~/src/api/forms/repositories/form-versions-repository.js' import { + FORM_VERSION_METADATA_KEY, modifyReorderComponents, modifyReorderPages, modifyReorderSections @@ -43,7 +44,6 @@ import { } from '~/src/api/forms/service/__stubs__/service.js' import { mockFormVersionDocument } from '~/src/api/forms/service/__stubs__/versioning.js' import { - FORM_VERSION_METADATA_KEY, createDraftFromLive, createLiveFromDraft, deleteDraftFormDefinition, @@ -514,29 +514,30 @@ describe('Forms service', () => { await expect(createForm(input, author)).rejects.toThrow() }) - it('should return the form definition with $$__formVersion injected', async () => { - jest.mocked(formDefinition.get).mockResolvedValueOnce(definition) - - const result = await getFormDefinition('123') - - expect(result).toEqual({ + it('should return the stored definition verbatim (the $$__formVersion stamp is persisted at write time)', async () => { + const stampedDefinition = { ...definition, metadata: { [FORM_VERSION_METADATA_KEY]: { - versionNumber: mockFormVersionDocument.versionNumber, - createdAt: mockFormVersionDocument.createdAt + versionNumber: 7, + createdAt: new Date('2026-04-01') } } - }) + } + jest.mocked(formDefinition.get).mockResolvedValueOnce(stampedDefinition) + + const result = await getFormDefinition('123') + + expect(result).toEqual(stampedDefinition) }) - it('should return the form definition without $$__formVersion when no versions exist', async () => { + it('should return the definition unchanged when no stamp is present (legacy forms)', async () => { jest.mocked(formDefinition.get).mockResolvedValueOnce(definition) - jest.mocked(formVersions.getLatestVersion).mockResolvedValueOnce(null) const result = await getFormDefinition('123') expect(result).toEqual(definition) + expect(result.metadata?.[FORM_VERSION_METADATA_KEY]).toBeUndefined() }) it('should throw an error if the form associated with the definition does not exist', async () => { diff --git a/src/api/forms/service/index.js b/src/api/forms/service/index.js index 66fa90f1..df70d4f5 100644 --- a/src/api/forms/service/index.js +++ b/src/api/forms/service/index.js @@ -100,8 +100,6 @@ export async function handleTitleUpdate( const schema = getValidationSchema(definition) await formDefinition.updateName(formId, formUpdate.title, session, schema) - - await createFormVersion(formId, session) } await publishFormTitleUpdatedEvent({ ...form, ...updatedForm }, form) @@ -174,9 +172,6 @@ export async function createForm(metadataInput, author) { // prettier-ignore await formDefinition.insert(metadata.id, definition, session, formDefinitionV2Schema) - // Create the first version of the form - await createFormVersion(metadata.id, session) - await publishFormCreatedEvent(metadata) }) } finally { diff --git a/src/api/forms/service/index.test.js b/src/api/forms/service/index.test.js index f0528665..4e288574 100644 --- a/src/api/forms/service/index.test.js +++ b/src/api/forms/service/index.test.js @@ -497,10 +497,6 @@ describe('Forms service', () => { expect.anything(), expect.anything() ) - expect(versioningService.createFormVersion).toHaveBeenCalledWith( - '123', - expect.anything() - ) }) it('should publish title audit event for a live form without a draft', async () => { @@ -713,10 +709,6 @@ describe('Forms service', () => { expect.anything(), expect.anything() ) - expect(versioningService.createFormVersion).toHaveBeenCalledWith( - '123', - expect.anything() - ) }) it('should publish title audit event for a live V2 form without a draft', async () => { diff --git a/src/api/forms/service/lists.js b/src/api/forms/service/lists.js index dba76b40..f2a45949 100644 --- a/src/api/forms/service/lists.js +++ b/src/api/forms/service/lists.js @@ -9,7 +9,6 @@ import Boom from '@hapi/boom' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { saveToS3 } from '~/src/messaging/s3.js' import { client } from '~/src/mongo.js' @@ -58,8 +57,6 @@ export async function addListToDraftFormDefinition(formId, list, author) { session ) - await createFormVersion(formId, session) - const filename = `${formId}_list_${returnedList.id}.json` const s3Meta = await saveToS3(filename, returnedList) @@ -123,8 +120,6 @@ export async function updateListOnDraftFormDefinition( session ) - await createFormVersion(formId, session) - const filename = `${formId}_list_${returnedList.id}.json` const s3Meta = await saveToS3(filename, returnedList) @@ -175,8 +170,6 @@ export async function removeListOnDraftFormDefinition(formId, listId, author) { session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { listId }, diff --git a/src/api/forms/service/migration.js b/src/api/forms/service/migration.js index 5dd67fcc..6617260f 100644 --- a/src/api/forms/service/migration.js +++ b/src/api/forms/service/migration.js @@ -9,7 +9,6 @@ import * as formDefinition from '~/src/api/forms/repositories/form-definition-re import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { migrateToV2 } from '~/src/api/forms/service/migration-helpers.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormMigratedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' /** @@ -42,8 +41,6 @@ export async function migrateDefinitionToV2(formId, author) { await formMetadata.updateAudit(formId, author, session) - await createFormVersion(formId, session) - await publishFormMigratedEvent(formId, new Date(), author) }) } catch (err) { diff --git a/src/api/forms/service/options.js b/src/api/forms/service/options.js index c074cb52..9b3f0b02 100644 --- a/src/api/forms/service/options.js +++ b/src/api/forms/service/options.js @@ -3,7 +3,6 @@ import { FormDefinitionRequestType, getErrorMessage } from '@defra/forms-model' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' @@ -41,8 +40,6 @@ export async function updateOptionOnDraftDefinition( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, payload, diff --git a/src/api/forms/service/page.js b/src/api/forms/service/page.js index 767faf92..08a089ab 100644 --- a/src/api/forms/service/page.js +++ b/src/api/forms/service/page.js @@ -13,7 +13,6 @@ import { } from '~/src/api/forms/repositories/helpers.js' import { getFormDefinition } from '~/src/api/forms/service/definition.js' import { SUMMARY_PAGE_ID, logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' @@ -77,8 +76,6 @@ export async function createPageOnDraftDefinition(formId, page, author) { session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, page, @@ -152,8 +149,6 @@ export async function patchFieldsOnDraftDefinitionPage( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { pageId, pageFieldsToUpdate }, @@ -195,8 +190,6 @@ export async function deletePageOnDraftDefinition(formId, pageId, author) { session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { pageId }, diff --git a/src/api/forms/service/sections.js b/src/api/forms/service/sections.js index a968b8aa..9acdd588 100644 --- a/src/api/forms/service/sections.js +++ b/src/api/forms/service/sections.js @@ -3,7 +3,6 @@ import { getErrorMessage } from '@defra/forms-model' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { logger } from '~/src/api/forms/service/shared.js' -import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { publishFormUpdatedEvent } from '~/src/messaging/publish.js' import { client } from '~/src/mongo.js' @@ -40,8 +39,6 @@ export async function assignSectionsToForm( session ) - await createFormVersion(formId, session) - await publishFormUpdatedEvent( metadataDocument, { sections: sectionAssignments }, diff --git a/src/api/forms/service/sections.test.js b/src/api/forms/service/sections.test.js index ddf05b5c..b89f51fb 100644 --- a/src/api/forms/service/sections.test.js +++ b/src/api/forms/service/sections.test.js @@ -113,12 +113,6 @@ describe('sections', () => { // Verify metadata was updated expectMetadataUpdate() - // Verify version was created - expect(versioningService.createFormVersion).toHaveBeenCalledWith( - id, - expect.anything() - ) - // Verify audit event was published const [auditMessage] = publishEventSpy.mock.calls[0] expect(auditMessage).toMatchObject({ diff --git a/src/api/forms/service/versioning.js b/src/api/forms/service/versioning.js index 5d4ff022..3036f6d4 100644 --- a/src/api/forms/service/versioning.js +++ b/src/api/forms/service/versioning.js @@ -1,9 +1,12 @@ import { FormStatus, getErrorMessage } from '@defra/forms-model' import * as formDefinitionRepository from '~/src/api/forms/repositories/form-definition-repository.js' -import * as formMetadataRepository from '~/src/api/forms/repositories/form-metadata-repository.js' import * as formVersionsRepository from '~/src/api/forms/repositories/form-versions-repository.js' import { MAX_VERSIONS } from '~/src/api/forms/repositories/form-versions-repository.js' +import { + allocateDraftVersion, + stampFormVersion +} from '~/src/api/forms/repositories/helpers.js' import { createLogger } from '~/src/helpers/logging/logger.js' import { client } from '~/src/mongo.js' @@ -27,7 +30,8 @@ export async function createFormVersionAndSession(formId) { } /** - * Creates a new version of a form definition + * Creates a new version for paths that don't go through modifyDraft — + * non-title metadata edits and the feedback form reinstate flow. * @param {string} formId - The form ID * @param {ClientSession} session - Existing MongoDB session (if called within a transaction) * @returns {Promise} @@ -50,39 +54,33 @@ export async function createFormVersion(formId, session) { } /** - * Internal function to create version within a transaction * @private * @param {string} formId * @param {ClientSession} session */ async function createVersionInTransaction(formId, session) { - const nextVersionNumber = - await formMetadataRepository.getAndIncrementVersionNumber(formId, session) - - const formDefinition = await formDefinitionRepository.get( + const draft = await formDefinitionRepository.get( formId, FormStatus.Draft, session ) - const createdAt = new Date() - - const versionMetadata = /** @type {FormVersionMetadata} */ ({ - versionNumber: nextVersionNumber, - createdAt - }) + const versionMetadata = await allocateDraftVersion(formId, session) - await formMetadataRepository.addVersionMetadata( + await formDefinitionRepository.setFormVersion( formId, + FormStatus.Draft, versionMetadata, session ) + const formDefinition = stampFormVersion(draft, versionMetadata) + const versionDocument = /** @type {FormVersionDocument} */ ({ formId, - versionNumber: nextVersionNumber, + versionNumber: versionMetadata.versionNumber, formDefinition, - createdAt + createdAt: versionMetadata.createdAt }) try { @@ -90,7 +88,7 @@ async function createVersionInTransaction(formId, session) { } catch (err) { logger.error( err, - `Unexpected error creating version ${nextVersionNumber} for form ID ${formId} after atomic increment` + `Unexpected error creating version ${versionMetadata.versionNumber} for form ID ${formId}` ) throw err } diff --git a/src/api/forms/service/versioning.test.js b/src/api/forms/service/versioning.test.js index 490c71cd..492d64fb 100644 --- a/src/api/forms/service/versioning.test.js +++ b/src/api/forms/service/versioning.test.js @@ -57,11 +57,17 @@ describe('versioning service', () => { }) describe('createFormVersion', () => { + const stampedFormDefinition = { + ...mockFormDefinition, + metadata: { + $$__formVersion: { versionNumber: 1, createdAt: now } + } + } const mockVersionDocument = buildFormVersionDocument({ _id: undefined, formId, versionNumber: 1, - formDefinition: mockFormDefinition, + formDefinition: stampedFormDefinition, createdAt: now }) @@ -97,6 +103,12 @@ describe('versioning service', () => { { versionNumber: 1, createdAt: now }, expect.any(Object) ) + expect(formDefinitionRepository.setFormVersion).toHaveBeenCalledWith( + formId, + FormStatus.Draft, + { versionNumber: 1, createdAt: now }, + expect.any(Object) + ) expect(formVersionsRepository.createVersion).toHaveBeenCalledWith( mockVersionDocument, expect.any(Object) diff --git a/src/helpers/feedback-form/reinstate.js b/src/helpers/feedback-form/reinstate.js index 021863e2..3183495a 100644 --- a/src/helpers/feedback-form/reinstate.js +++ b/src/helpers/feedback-form/reinstate.js @@ -1,4 +1,4 @@ -import { getErrorMessage } from '@defra/forms-model' +import { FormStatus, getErrorMessage } from '@defra/forms-model' import * as def from '~/src/api/forms/repositories/form-definition-repository.js' import * as meta from '~/src/api/forms/repositories/form-metadata-repository.js' @@ -53,7 +53,17 @@ export async function saveDefinition(formId, session, logger) { * @param {ClientSession} session */ export async function saveFormVersion(formId, session) { - return createFormVersion(formId, session) + const result = await createFormVersion(formId, session) + + // Mirror the stamp onto live, since upsertDraftAndLive writes both states. + await def.setFormVersion( + formId, + FormStatus.Live, + { versionNumber: result.versionNumber, createdAt: result.createdAt }, + session + ) + + return result } /** diff --git a/src/helpers/feedback-form/reinstate.test.js b/src/helpers/feedback-form/reinstate.test.js index a82e68e2..93f4db29 100644 --- a/src/helpers/feedback-form/reinstate.test.js +++ b/src/helpers/feedback-form/reinstate.test.js @@ -1,4 +1,6 @@ import { buildMockCollection } from '~/src/api/forms/__stubs__/mongo.js' +import { mockFormVersionDocument } from '~/src/api/forms/service/__stubs__/versioning.js' +import { createFormVersion } from '~/src/api/forms/service/versioning.js' import { createdUpdatedDate } from '~/src/helpers/feedback-form/metadata.js' import { reinstateFeedbackForm } from '~/src/helpers/feedback-form/reinstate.js' import { client, db } from '~/src/mongo.js' @@ -51,6 +53,7 @@ describe('reinstate', () => { jest .mocked(db.collection) .mockReturnValue(/** @type {any} */ (mockCollection)) + jest.mocked(createFormVersion).mockResolvedValue(mockFormVersionDocument) jest.doMock('~/src/helpers/logging/logger.js', () => ({ createLogger: jest.fn().mockReturnValue(mockLogger) })) diff --git a/src/routes/forms.js b/src/routes/forms.js index c4b7deb9..08e0b551 100644 --- a/src/routes/forms.js +++ b/src/routes/forms.js @@ -7,12 +7,12 @@ import { } from '@defra/forms-model' import Boom from '@hapi/boom' +import { stampFormVersion } from '~/src/api/forms/repositories/helpers.js' import { createDraftFromLive, createLiveFromDraft, deleteDraftFormDefinition, getFormDefinition, - injectFormVersion, listForms, updateDraftFormDefinition } from '~/src/api/forms/service/definition.js' @@ -450,7 +450,7 @@ export default [ const version = await getFormVersion(id, parseInt(versionNumber)) - return injectFormVersion(version.formDefinition, version) + return stampFormVersion(version.formDefinition, version) }, options: { auth: false, From 0deb6cea5035d3edf1a15b6d92d5bb53104bfc4d Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Fri, 24 Apr 2026 12:10:03 +0100 Subject: [PATCH 2/3] fix(DF-1004): use session + projection on modifyDraft read, add stamping tests Addresses Copilot review feedback on PR #794: - modifyDraft's initial findOne now participates in the transaction (was reading outside the session, risking stale reads under concurrent draft updates). Added projection { draft: 1 } since that's all we need. - Added two tests covering the version stamping behaviour introduced in modifyDraft and insertDraft (allocates version, stamps draft with $$__formVersion, snapshots to form-versions). --- .../form-definition-repository.test.js | 104 ++++++++++++++++++ src/api/forms/repositories/helpers.js | 5 +- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/api/forms/repositories/form-definition-repository.test.js b/src/api/forms/repositories/form-definition-repository.test.js index 174ad54e..e9d2db8b 100644 --- a/src/api/forms/repositories/form-definition-repository.test.js +++ b/src/api/forms/repositories/form-definition-repository.test.js @@ -52,6 +52,9 @@ import { updatePage, updatePageFields } from '~/src/api/forms/repositories/form-definition-repository.js' +import * as formMetadataRepository from '~/src/api/forms/repositories/form-metadata-repository.js' +import * as formVersionsRepository from '~/src/api/forms/repositories/form-versions-repository.js' +import { FORM_VERSION_METADATA_KEY } from '~/src/api/forms/repositories/helpers.js' import { empty, emptyV2 } from '~/src/api/forms/templates.js' import { getAuthor } from '~/src/helpers/get-author.js' import { db } from '~/src/mongo.js' @@ -949,6 +952,53 @@ describe('form-definition-repository', () => { throw new Error('Unexpected empty draft on $setOnInsert') } }) + + it('should allocate a version, stamp the inserted draft, and snapshot to form-versions', async () => { + const definitionV1 = { ...draft, conditions: [] } + const createdAt = new Date('2026-04-24T10:00:00Z') + mockCollection.findOneAndUpdate.mockResolvedValue({ definitionV1 }) + jest + .mocked(formMetadataRepository.getAndIncrementVersionNumber) + .mockResolvedValue(7) + jest.spyOn(global, 'Date').mockImplementation(() => createdAt) + + await insert(formId, definitionV1, mockSession, formDefinitionSchema) + + expect( + formMetadataRepository.getAndIncrementVersionNumber + ).toHaveBeenCalledWith(formId, mockSession) + expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledWith( + formId, + { versionNumber: 7, createdAt }, + mockSession + ) + + const [, update] = mockCollection.findOneAndUpdate.mock.calls[0] + /** @type {UpdateFilter<{ draft: FormDefinition }>} */ + const updateFilter = update + /** @type {any} */ + const stampedDraft = updateFilter.$setOnInsert?.draft + expect(stampedDraft?.metadata?.[FORM_VERSION_METADATA_KEY]).toEqual({ + versionNumber: 7, + createdAt + }) + + expect(formVersionsRepository.createVersion).toHaveBeenCalledWith( + expect.objectContaining({ + formId, + versionNumber: 7, + createdAt, + formDefinition: expect.objectContaining({ + metadata: expect.objectContaining({ + [FORM_VERSION_METADATA_KEY]: { versionNumber: 7, createdAt } + }) + }) + }), + mockSession + ) + + jest.restoreAllMocks() + }) }) describe('update', () => { @@ -986,6 +1036,60 @@ describe('form-definition-repository', () => { } ) }) + + it('should allocate a version, stamp the updated draft, and snapshot to form-versions', async () => { + const newDefinition = emptyV2() + const createdAt = new Date('2026-04-24T10:00:00Z') + jest + .mocked(formMetadataRepository.getAndIncrementVersionNumber) + .mockResolvedValue(11) + jest.spyOn(global, 'Date').mockImplementation(() => createdAt) + + await helper( + async () => { + await update( + formId, + newDefinition, + mockSession, + formDefinitionV2Schema + ) + }, + (persistedDraft) => { + expect( + formMetadataRepository.getAndIncrementVersionNumber + ).toHaveBeenCalledWith(formId, mockSession) + expect( + formMetadataRepository.addVersionMetadata + ).toHaveBeenCalledWith( + formId, + { versionNumber: 11, createdAt }, + mockSession + ) + expect(persistedDraft.metadata?.[FORM_VERSION_METADATA_KEY]).toEqual({ + versionNumber: 11, + createdAt + }) + expect(formVersionsRepository.createVersion).toHaveBeenCalledWith( + expect.objectContaining({ + formId, + versionNumber: 11, + createdAt, + formDefinition: expect.objectContaining({ + metadata: expect.objectContaining({ + [FORM_VERSION_METADATA_KEY]: { + versionNumber: 11, + createdAt + } + }) + }) + }), + mockSession + ) + } + ) + + jest.restoreAllMocks() + }) }) describe('createLiveFromDraft', () => { diff --git a/src/api/forms/repositories/helpers.js b/src/api/forms/repositories/helpers.js index 1e619287..3edd9552 100644 --- a/src/api/forms/repositories/helpers.js +++ b/src/api/forms/repositories/helpers.js @@ -392,7 +392,10 @@ export async function modifyDraft( ) const id = { _id: new ObjectId(formId) } - const document = await coll.findOne(id) + const document = await coll.findOne(id, { + session, + projection: { draft: 1 } + }) if (!document) { throw Boom.notFound(`Document not found '${formId}'`) From f412b8acb16fa4cf9e0f77670826df29ad824d19 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Fri, 24 Apr 2026 14:56:48 +0100 Subject: [PATCH 3/3] test(DF-1004): assert version allocation + snapshot happen exactly once per mutation --- .../form-definition-repository.test.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/api/forms/repositories/form-definition-repository.test.js b/src/api/forms/repositories/form-definition-repository.test.js index e9d2db8b..7c1a7ebc 100644 --- a/src/api/forms/repositories/form-definition-repository.test.js +++ b/src/api/forms/repositories/form-definition-repository.test.js @@ -953,7 +953,7 @@ describe('form-definition-repository', () => { } }) - it('should allocate a version, stamp the inserted draft, and snapshot to form-versions', async () => { + it('should allocate a version exactly once, stamp the inserted draft, and snapshot to form-versions exactly once', async () => { const definitionV1 = { ...draft, conditions: [] } const createdAt = new Date('2026-04-24T10:00:00Z') mockCollection.findOneAndUpdate.mockResolvedValue({ definitionV1 }) @@ -964,9 +964,13 @@ describe('form-definition-repository', () => { await insert(formId, definitionV1, mockSession, formDefinitionSchema) + expect( + formMetadataRepository.getAndIncrementVersionNumber + ).toHaveBeenCalledTimes(1) expect( formMetadataRepository.getAndIncrementVersionNumber ).toHaveBeenCalledWith(formId, mockSession) + expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledTimes(1) expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledWith( formId, { versionNumber: 7, createdAt }, @@ -983,6 +987,7 @@ describe('form-definition-repository', () => { createdAt }) + expect(formVersionsRepository.createVersion).toHaveBeenCalledTimes(1) expect(formVersionsRepository.createVersion).toHaveBeenCalledWith( expect.objectContaining({ formId, @@ -1037,7 +1042,7 @@ describe('form-definition-repository', () => { ) }) - it('should allocate a version, stamp the updated draft, and snapshot to form-versions', async () => { + it('should allocate a version exactly once, stamp the updated draft, and snapshot to form-versions exactly once', async () => { const newDefinition = emptyV2() const createdAt = new Date('2026-04-24T10:00:00Z') jest @@ -1055,9 +1060,15 @@ describe('form-definition-repository', () => { ) }, (persistedDraft) => { + expect( + formMetadataRepository.getAndIncrementVersionNumber + ).toHaveBeenCalledTimes(1) expect( formMetadataRepository.getAndIncrementVersionNumber ).toHaveBeenCalledWith(formId, mockSession) + expect( + formMetadataRepository.addVersionMetadata + ).toHaveBeenCalledTimes(1) expect( formMetadataRepository.addVersionMetadata ).toHaveBeenCalledWith( @@ -1069,6 +1080,7 @@ describe('form-definition-repository', () => { versionNumber: 11, createdAt }) + expect(formVersionsRepository.createVersion).toHaveBeenCalledTimes(1) expect(formVersionsRepository.createVersion).toHaveBeenCalledWith( expect.objectContaining({ formId,