diff --git a/migrations/20260623080105-remove-versions-metadata.js b/migrations/20260623080105-remove-versions-metadata.js new file mode 100644 index 00000000..3c9dc339 --- /dev/null +++ b/migrations/20260623080105-remove-versions-metadata.js @@ -0,0 +1,74 @@ +/* eslint-disable no-console */ +const METADATA_COLLECTION_NAME = 'form-metadata' + +/** + * @param {MongoClient} client + * @param {Collection} metadataCollection + */ +async function removeVersionsAttribute(client, metadataCollection) { + const stats = { + updated: 0, + errors: 0 + } + + const session = client.startSession() + + await session.withTransaction(async () => { + const updatesCursor = metadataCollection.find({ + versions: { $exists: true } + }) + + for await (const forUpdate of updatesCursor) { + try { + await metadataCollection.updateOne( + { + _id: forUpdate._id + }, + { $unset: { versions: '' } } + ) + stats.updated++ + } catch (error) { + console.error( + `Removing 'versions' attribute failed for slug ${forUpdate.slug}:`, + error instanceof Error ? error.message : String(error) + ) + stats.errors++ + } + } + }) + + console.log(`\n=== Migration Summary - remove 'versions' attributes ===`) + console.log(`Successfully updated: ${stats.updated}`) + console.log(`Errors: ${stats.errors}`) + + console.log(' ') + console.log(' ') +} + +/** + * Removes the 'versions' attribute from all metadata records + * @param {import('mongodb').Db} db + * @param {import('mongodb').MongoClient} client + * @returns {Promise} + */ +export async function up(db, client) { + const metadataCollection = /** @type {Collection} */ ( + db.collection(METADATA_COLLECTION_NAME) + ) + await removeVersionsAttribute(client, metadataCollection) +} + +/** + * This migration is a one-way data consolidation fix. + * @returns {Promise} + */ +export function down() { + return Promise.reject( + new Error('Migration rollback is not supported for data safety reasons') + ) +} + +/** + * @import { FormMetadata } from '@defra/forms-model' + * @import { Collection, MongoClient, Db } from 'mongodb' + */ diff --git a/src/api/forms/repositories/form-definition-repository.test.js b/src/api/forms/repositories/form-definition-repository.test.js index 7c1a7ebc..c548cc99 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 exactly once, stamp the inserted draft, and snapshot to form-versions exactly once', async () => { + it('should allocate a version exactly once, stamp the inserted draft', async () => { const definitionV1 = { ...draft, conditions: [] } const createdAt = new Date('2026-04-24T10:00:00Z') mockCollection.findOneAndUpdate.mockResolvedValue({ definitionV1 }) @@ -970,12 +970,6 @@ describe('form-definition-repository', () => { expect( formMetadataRepository.getAndIncrementVersionNumber ).toHaveBeenCalledWith(formId, mockSession) - expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledTimes(1) - expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledWith( - formId, - { versionNumber: 7, createdAt }, - mockSession - ) const [, update] = mockCollection.findOneAndUpdate.mock.calls[0] /** @type {UpdateFilter<{ draft: FormDefinition }>} */ @@ -1066,16 +1060,6 @@ describe('form-definition-repository', () => { expect( formMetadataRepository.getAndIncrementVersionNumber ).toHaveBeenCalledWith(formId, mockSession) - expect( - formMetadataRepository.addVersionMetadata - ).toHaveBeenCalledTimes(1) - expect( - formMetadataRepository.addVersionMetadata - ).toHaveBeenCalledWith( - formId, - { versionNumber: 11, createdAt }, - mockSession - ) expect(persistedDraft.metadata?.[FORM_VERSION_METADATA_KEY]).toEqual({ versionNumber: 11, createdAt diff --git a/src/api/forms/repositories/form-metadata-repository.js b/src/api/forms/repositories/form-metadata-repository.js index 269bc964..6be7421c 100644 --- a/src/api/forms/repositories/form-metadata-repository.js +++ b/src/api/forms/repositories/form-metadata-repository.js @@ -401,37 +401,6 @@ export async function remove(formId, session) { logger.info(`Removed form metadata with ID ${formId}`) } -/** - * Adds version metadata to a form - * @param {string} formId - ID of the form - * @param {FormVersionMetadata} versionMetadata - Version metadata to add - * @param {ClientSession} session - mongo transaction session - */ -export async function addVersionMetadata(formId, versionMetadata, session) { - logger.info( - `Adding version metadata ${versionMetadata.versionNumber} to form ID ${formId}` - ) - - const result = await update( - formId, - { - $push: { - versions: { - $each: [versionMetadata], - $sort: { versionNumber: -1 } - } - } - }, - session - ) - - logger.info( - `Added version metadata ${versionMetadata.versionNumber} to form ID ${formId}` - ) - - return result -} - /** * Gets version metadata for a form * @param {string} formId - ID of the form diff --git a/src/api/forms/repositories/form-metadata-repository.test.js b/src/api/forms/repositories/form-metadata-repository.test.js index 6b98dbd0..f6f8a427 100644 --- a/src/api/forms/repositories/form-metadata-repository.test.js +++ b/src/api/forms/repositories/form-metadata-repository.test.js @@ -9,7 +9,6 @@ import { import { buildMockCollection } from '~/src/api/forms/__stubs__/mongo.js' import { FormAlreadyExistsError } from '~/src/api/forms/errors.js' import { - addVersionMetadata, create, get, getAndIncrementVersionNumber, @@ -648,40 +647,6 @@ describe('form-metadata-repository', () => { }) }) - describe('addVersionMetadata', () => { - const versionMetadata = { - versionNumber: 1, - createdAt: new Date() - } - - it('should add version metadata to form', async () => { - mockCollection.updateOne.mockResolvedValue({ - modifiedCount: 1 - }) - mockCollection.findOne.mockResolvedValue(metadataAfter) - - const result = await addVersionMetadata( - metadataId, - versionMetadata, - mockSession - ) - - expect(mockCollection.updateOne).toHaveBeenCalledWith( - { _id: new ObjectId(metadataId) }, - { - $push: { - versions: { - $each: [versionMetadata], - $sort: { versionNumber: -1 } - } - } - }, - { session: mockSession } - ) - expect(result).toEqual(metadataAfter) - }) - }) - describe('getVersionMetadata', () => { it('should get version metadata for a form', async () => { const versions = [ diff --git a/src/api/forms/repositories/helpers.js b/src/api/forms/repositories/helpers.js index aa60b4fb..03fb6e19 100644 --- a/src/api/forms/repositories/helpers.js +++ b/src/api/forms/repositories/helpers.js @@ -337,18 +337,13 @@ export async function insertDraft( 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 } diff --git a/src/api/forms/service/__stubs__/service.js b/src/api/forms/service/__stubs__/service.js index 087f2812..9e5f756a 100644 --- a/src/api/forms/service/__stubs__/service.js +++ b/src/api/forms/service/__stubs__/service.js @@ -44,8 +44,7 @@ export const formMetadataOutput = { createdAt: BASE_CREATED_DATE, createdBy: author, updatedAt: BASE_CREATED_DATE, - updatedBy: author, - versions: [] + updatedBy: author } /** @@ -85,8 +84,7 @@ export const formMetadataDocument = { createdAt: BASE_CREATED_DATE, createdBy: author, updatedAt: BASE_CREATED_DATE, - updatedBy: author, - versions: [] + updatedBy: author } /** diff --git a/src/api/forms/service/index.js b/src/api/forms/service/index.js index 040c72c1..e3753a93 100644 --- a/src/api/forms/service/index.js +++ b/src/api/forms/service/index.js @@ -20,10 +20,7 @@ import { mapForm, partialAuditFields } from '~/src/api/forms/service/shared.js' -import { - createFormVersion, - removeFormVersions -} from '~/src/api/forms/service/versioning.js' +import { removeFormVersions } from '~/src/api/forms/service/versioning.js' import * as formTemplates from '~/src/api/forms/templates.js' import { config } from '~/src/config/index.js' import { logger } from '~/src/helpers/logging/logger.js' @@ -111,20 +108,6 @@ export async function handleTitleUpdate( await publishFormTitleUpdatedEvent({ ...form, ...updatedForm }, form) } -/** - * Handles versioning for non-title metadata updates - * @param {string} formId - The form ID - * @param {Partial} formUpdate - The update payload - * @param {ClientSession} session - MongoDB session - */ -export async function handleMetadataVersioning(formId, formUpdate, session) { - if (Object.keys(formUpdate).length > 0) { - await createFormVersion(formId, session) - } else { - logger.debug(`No metadata changes to process for form ID ${formId}`) - } -} - /** * Creates a new empty form * @param {FormMetadataInput} metadataInput - the form metadata to save @@ -159,8 +142,7 @@ export async function createForm(metadataInput, author) { createdAt: now, createdBy: author, updatedAt: now, - updatedBy: author, - versions: [] + updatedBy: author } const session = client.startSession() @@ -232,8 +214,6 @@ export async function updateFormMetadata(formId, formUpdate, author) { if (formUpdate.title) { await handleTitleUpdate(formId, form, formUpdate, updatedForm, session) - } else { - await handleMetadataVersioning(formId, formUpdate, session) } await sendEmailIfRequired(form, updatedForm) diff --git a/src/api/forms/service/index.test.js b/src/api/forms/service/index.test.js index 4bd5115a..9b977382 100644 --- a/src/api/forms/service/index.test.js +++ b/src/api/forms/service/index.test.js @@ -858,22 +858,6 @@ describe('Forms service', () => { }) }) - describe('handleMetadataVersioning', () => { - it('should not create version when there are no changes', async () => { - const formUpdate = {} - const mockSession = /** @type {import('mongodb').ClientSession} */ ({}) - - jest.clearAllMocks() - - const { handleMetadataVersioning } = - await import('~/src/api/forms/service/index.js') - - await handleMetadataVersioning(id, formUpdate, mockSession) - - expect(versioningService.createFormVersion).not.toHaveBeenCalled() - }) - }) - describe('sendEmailIfRequired', () => { it('should not send if no team email setup', async () => { const metadata = /** @type {FormMetadata} */ ({ diff --git a/src/api/forms/service/shared.js b/src/api/forms/service/shared.js index 872efa56..6403522b 100644 --- a/src/api/forms/service/shared.js +++ b/src/api/forms/service/shared.js @@ -74,8 +74,7 @@ export function mapForm(document) { createdBy: created.createdBy, createdAt: created.createdAt, updatedBy: lastUpdated.updatedBy, - updatedAt: lastUpdated.updatedAt, - versions: document.versions + updatedAt: lastUpdated.updatedAt } } diff --git a/src/api/forms/service/shared.test.js b/src/api/forms/service/shared.test.js index e7b1b281..24380576 100644 --- a/src/api/forms/service/shared.test.js +++ b/src/api/forms/service/shared.test.js @@ -23,25 +23,19 @@ const baseDocument = { } describe('mapForm', () => { - it('should return versions from document when present', () => { - const versions = [ - { versionNumber: 2, createdAt: new Date('2025-10-01') }, - { versionNumber: 1, createdAt: new Date('2025-09-01') } - ] - const result = mapForm({ ...baseDocument, versions }) - - expect(result.versions).toEqual(versions) - }) - - it('should return undefined for versions when document has no versions field', () => { + it('should return document', () => { const result = mapForm(baseDocument) - expect(result.versions).toBeUndefined() + expect(result.draft).toBeDefined() + expect(result.id).toBe(baseDocument._id.toString()) }) - it('should return empty array for versions when document has empty versions array', () => { - const result = mapForm({ ...baseDocument, versions: [] }) - - expect(result.versions).toEqual([]) + it('should throw if invalid', () => { + expect(() => + mapForm({ + ...baseDocument, + title: undefined + }) + ).toThrow('Form is malformed in the database. Expected fields are missing.') }) }) diff --git a/src/api/forms/service/versioning.test.js b/src/api/forms/service/versioning.test.js index 492d64fb..64836ea4 100644 --- a/src/api/forms/service/versioning.test.js +++ b/src/api/forms/service/versioning.test.js @@ -78,9 +78,6 @@ describe('versioning service', () => { jest .mocked(formDefinitionRepository.get) .mockResolvedValue(mockFormDefinition) - jest - .mocked(formMetadataRepository.addVersionMetadata) - .mockResolvedValue(/** @type {any} */ ({})) jest .mocked(formVersionsRepository.createVersion) .mockResolvedValue(mockVersionDocument) @@ -98,11 +95,6 @@ describe('versioning service', () => { FormStatus.Draft, expect.any(Object) ) - expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledWith( - formId, - { versionNumber: 1, createdAt: now }, - expect.any(Object) - ) expect(formDefinitionRepository.setFormVersion).toHaveBeenCalledWith( formId, FormStatus.Draft, @@ -145,11 +137,6 @@ describe('versioning service', () => { expect( formMetadataRepository.getAndIncrementVersionNumber ).toHaveBeenCalledWith(formId, expect.any(Object)) - expect(formMetadataRepository.addVersionMetadata).toHaveBeenCalledWith( - formId, - { versionNumber: 4, createdAt: now }, - expect.any(Object) - ) }) it('should create its own session when none provided', async () => { diff --git a/src/helpers/feedback-form/metadata.js b/src/helpers/feedback-form/metadata.js index 85a85c5a..15c8dac7 100644 --- a/src/helpers/feedback-form/metadata.js +++ b/src/helpers/feedback-form/metadata.js @@ -23,7 +23,6 @@ export const feedbackMetadata = /** @type {FormMetadata} */ ({ createdBy: user, updatedAt: createdUpdatedDate, updatedBy: user, - versions: [], notificationEmail: 'defraforms.dynamic-target@defra.gov.uk', contact: { phone: 'Telephone: 020 7946 0101\r\nMonday to Friday, 8am to 6pm' diff --git a/src/messaging/__snapshots__/publish.test.js.snap b/src/messaging/__snapshots__/publish.test.js.snap index b18e0dd1..ba6703ef 100644 --- a/src/messaging/__snapshots__/publish.test.js.snap +++ b/src/messaging/__snapshots__/publish.test.js.snap @@ -131,34 +131,6 @@ exports[`publish publishFormMigratedEvent should publish FORM_MIGRATED event 1`] } `; -exports[`publish publishFormOfflineUpdatedEvent should publish a FORM_UPDATED event 1`] = ` -{ - "category": "FORM", - "createdAt": 2025-07-24T00:00:00.000Z, - "createdBy": { - "displayName": "Gandalf", - "id": "a53b4360-bdf6-4d13-8975-25032ce76312", - }, - "data": { - "changes": { - "new": { - "offline": true, - }, - "previous": { - "offline": false, - }, - }, - "formId": "689b7ab1d0eeac9711a7fb33", - "slug": "audit-form", - }, - "entityId": "689b7ab1d0eeac9711a7fb33", - "messageCreatedAt": Any, - "schemaVersion": 1, - "source": "FORMS_MANAGER", - "type": "FORM_OFFLINE_UPDATED", -} -`; - exports[`publish publishFormTitleUpdatedEvent should publish FORM_TITLE_UPDATED event 1`] = ` { "category": "FORM", diff --git a/src/messaging/mappers/form-events-bulk.js b/src/messaging/mappers/form-events-bulk.js index 230bdcfd..c944b956 100644 --- a/src/messaging/mappers/form-events-bulk.js +++ b/src/messaging/mappers/form-events-bulk.js @@ -11,7 +11,7 @@ import { } from '~/src/messaging/mappers/form-events.js' /** - * @type {Record} + * @type {Record} */ const mapperLookup = { organisation: formOrganisationUpdatedMapper, @@ -59,7 +59,10 @@ export function getFormMetadataAuditMessages(metadata, formUpdated) { if (hasKey) { const mapperFn = mapperLookup[key] - messages.push(mapperFn(metadata, formUpdated)) + const message = mapperFn(metadata, formUpdated) + if (message) { + messages.push(message) + } } } diff --git a/src/messaging/mappers/form-events-bulk.test.js b/src/messaging/mappers/form-events-bulk.test.js index 93a2ecc5..32875ffc 100644 --- a/src/messaging/mappers/form-events-bulk.test.js +++ b/src/messaging/mappers/form-events-bulk.test.js @@ -418,5 +418,62 @@ describe('publish', () => { } }) }) + + it('should get FORM_OFFLINE_UPDATED audit message', () => { + const updatedAt = new Date('2025-07-27') + const updatedBy = { + displayName: 'Gandalf', + id: '29a8b10d-1d7a-40d4-b312-c57f74e1a606' + } + const formUpdated = buildPartialFormMetadataDocument({ + offline: true, + updatedBy, + updatedAt + }) + const messages = getFormMetadataAuditMessages(metadata, formUpdated) + const [formUpdatedMessage] = messages + expect(messages).toHaveLength(1) + expect(formUpdatedMessage).toEqual({ + entityId: formId, + source: AuditEventMessageSource.FORMS_MANAGER, + messageCreatedAt: expect.any(Date), + schemaVersion: AuditEventMessageSchemaVersion.V1, + category: AuditEventMessageCategory.FORM, + type: AuditEventMessageType.FORM_OFFLINE_UPDATED, + createdAt: updatedAt, + createdBy: updatedBy, + data: { + formId, + slug: 'audit-form', + changes: { + previous: { + offline: false + }, + new: { + offline: true + } + } + } + }) + }) + + it('should NOT get audit message since no change', () => { + const updatedAt = new Date('2025-07-27') + const updatedBy = { + displayName: 'Gandalf', + id: '29a8b10d-1d7a-40d4-b312-c57f74e1a606' + } + const formUpdated = buildPartialFormMetadataDocument({ + offline: true, + updatedBy, + updatedAt + }) + const origMetadata = { + ...metadata, + offline: true + } + const messages = getFormMetadataAuditMessages(origMetadata, formUpdated) + expect(messages).toHaveLength(0) + }) }) }) diff --git a/src/messaging/mappers/form-events.js b/src/messaging/mappers/form-events.js index 3119b3b6..0acfa62b 100644 --- a/src/messaging/mappers/form-events.js +++ b/src/messaging/mappers/form-events.js @@ -536,12 +536,17 @@ export function deletedFormSecretMapper(metadata, secretName, createdBy) { /** * @param {FormMetadata} metadata * @param {PartialFormMetadataDocument} updatedForm - * @returns {FormOfflineUpdatedMessage} + * @returns { FormOfflineUpdatedMessage | undefined } */ export function formOfflineUpdatedMapper(metadata, updatedForm) { const { offline: oldOffline } = metadata const { offline } = updatedForm + // Ignore if no change + if (oldOffline === offline) { + return undefined + } + const baseData = createFormMessageDataBase(metadata) /** @type {FormOfflineUpdatedMessageData} */ diff --git a/src/messaging/mappers/form-events.test.js b/src/messaging/mappers/form-events.test.js index 5215d6f5..8d28516f 100644 --- a/src/messaging/mappers/form-events.test.js +++ b/src/messaging/mappers/form-events.test.js @@ -13,6 +13,7 @@ import { import author from '~/src/api/forms/service/__stubs__/author.js' import { + formOfflineUpdatedMapper, formOrganisationUpdatedMapper, formTeamEmailUpdatedMapper, formTeamNameUpdatedMapper, @@ -102,4 +103,54 @@ describe('form-events', () => { }) }) }) + + describe('formOfflineUpdatedMapper', () => { + const formId = '6883d8667a2a64da10af4312' + const updatedAt = new Date('2025-08-31') + + const metadata = buildMetaData({ + id: formId, + updatedAt, + updatedBy: author, + slug: 'my-form', + offline: false + }) + it('should map a payload into a FORM_UPDATED replaced event', () => { + const updatedMetadata = { + ...metadata, + offline: true + } + expect(formOfflineUpdatedMapper(metadata, updatedMetadata)).toEqual({ + schemaVersion: AuditEventMessageSchemaVersion.V1, + category: AuditEventMessageCategory.FORM, + source: AuditEventMessageSource.FORMS_MANAGER, + type: AuditEventMessageType.FORM_OFFLINE_UPDATED, + entityId: formId, + createdAt: updatedAt, + createdBy: author, + messageCreatedAt: expect.any(Date), + data: { + changes: { + new: { + offline: true + }, + previous: { + offline: false + } + }, + formId: '6883d8667a2a64da10af4312', + slug: 'my-form' + } + }) + }) + + it('should ignore if no change', () => { + const updatedMetadata = { + ...metadata + } + expect( + formOfflineUpdatedMapper(metadata, updatedMetadata) + ).toBeUndefined() + }) + }) }) diff --git a/src/messaging/publish.js b/src/messaging/publish.js index 3c8609e9..693f1618 100644 --- a/src/messaging/publish.js +++ b/src/messaging/publish.js @@ -9,7 +9,6 @@ import { formDraftDeletedMapper, formLiveCreatedFromDraftMapper, formMigratedMapper, - formOfflineUpdatedMapper, formTitleUpdatedMapper, formUpdatedMapper, savedFormSecretMapper @@ -90,17 +89,6 @@ export async function publishDraftCreatedFromLiveEvent( return validateAndPublishEvent(auditMessage) } -/** - * Publishes a 'form offline updated' event - * @param {FormMetadata} metadata - * @param {FormMetadata} oldMetadata - */ -export async function publishFormOfflineUpdatedEvent(metadata, oldMetadata) { - const auditMessage = formOfflineUpdatedMapper(oldMetadata, metadata) - - return validateAndPublishEvent(auditMessage) -} - /** * Publishes a form migrated event * @param {string} formId diff --git a/src/messaging/publish.test.js b/src/messaging/publish.test.js index 5c00dff4..0aad124c 100644 --- a/src/messaging/publish.test.js +++ b/src/messaging/publish.test.js @@ -27,7 +27,6 @@ import { publishFormDraftDeletedEvent, publishFormDraftReplacedEvent, publishFormMigratedEvent, - publishFormOfflineUpdatedEvent, publishFormTitleUpdatedEvent, publishFormUpdatedEvent, publishLiveCreatedFromDraftEvent, @@ -297,46 +296,6 @@ describe('publish', () => { }) }) - describe('publishFormOfflineUpdatedEvent', () => { - it('should publish a FORM_UPDATED event', async () => { - const newMetadata = buildMetaData({ - ...metadata, - offline: true - }) - const response = await publishFormOfflineUpdatedEvent( - newMetadata, - metadata - ) - expect(response?.MessageId).toBe(messageId) - expect(publishEvent).toHaveBeenCalledWith({ - entityId: formId, - messageCreatedAt: expect.any(Date), - source: AuditEventMessageSource.FORMS_MANAGER, - schemaVersion: AuditEventMessageSchemaVersion.V1, - category: AuditEventMessageCategory.FORM, - type: AuditEventMessageType.FORM_OFFLINE_UPDATED, - createdAt: updatedAt, - createdBy: updatedBy, - data: { - formId, - slug, - changes: { - previous: { - offline: false - }, - new: { - offline: true - } - } - } - }) - const [publishEventCall] = jest.mocked(publishEvent).mock.calls[0] - expect(publishEventCall).toMatchSnapshot({ - messageCreatedAt: expect.any(Date) - }) - }) - }) - describe('publishFormDraftReplacedEvent', () => { it('should publish a REPLACE_DRAFT event', async () => { const s3Meta = {