From 2cbb92f5896e3093c2e295ba3d87c496c18c1360 Mon Sep 17 00:00:00 2001 From: Chris Cole Date: Mon, 17 Mar 2025 15:28:38 +0000 Subject: [PATCH 1/5] refactor: 481476 - Remove state from repo methods --- .../form-definition-repository.js | 91 ++-------------- .../form-definition-repository.test.js | 102 ------------------ src/api/forms/service/component.js | 8 +- src/api/forms/service/component.test.js | 42 +++----- src/api/forms/service/lists.test.js | 3 +- src/api/forms/service/migration.test.js | 4 +- src/api/forms/service/page.js | 3 +- src/api/forms/service/page.test.js | 8 +- 8 files changed, 34 insertions(+), 227 deletions(-) diff --git a/src/api/forms/repositories/form-definition-repository.js b/src/api/forms/repositories/form-definition-repository.js index 51f65a5d..752e4d88 100644 --- a/src/api/forms/repositories/form-definition-repository.js +++ b/src/api/forms/repositories/form-definition-repository.js @@ -236,17 +236,8 @@ export async function updateName( * @param {string} formId - the ID of the form * @param {{ controller: ControllerType.Summary }} matchCriteria - new name for the form * @param {ClientSession} session - * @param {FormStatus} [state] - state of the form to update */ -export async function removeMatchingPages( - formId, - matchCriteria, - session, - state = FormStatus.Draft -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot remove page on live form ID ${formId}`) - } +export async function removeMatchingPages(formId, matchCriteria, session) { logger.info(`Removing page on ${formId}`) const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( @@ -268,18 +259,9 @@ export async function removeMatchingPages( * @param {string} formId - the ID of the form * @param {Page} page - new name for the form * @param {ClientSession} session - * @param {{ position?: number; state?: FormStatus }} options + * @param {{ position?: number }} options */ -export async function addPageAtPosition( - formId, - page, - session, - { position, state = FormStatus.Draft } -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot remove add on live form ID ${formId}`) - } - +export async function addPageAtPosition(formId, page, session, { position }) { logger.info(`Adding page on Form ID ${formId}`) const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( db.collection(DEFINITION_COLLECTION_NAME) @@ -312,20 +294,9 @@ export async function addPageAtPosition( * @param {string} pageId * @param {Page} page * @param {ClientSession} session - * @param {FormStatus} [state] * @returns {Promise} */ -export async function updatePage( - formId, - pageId, - page, - session, - state = FormStatus.Draft -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot update page on a live form - ${formId}`) - } - +export async function updatePage(formId, pageId, page, session) { logger.info(`Updating page ID ${pageId} on form ID ${formId}`) const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( @@ -347,7 +318,7 @@ export async function updatePage( * @param {string} pageId * @param {ComponentDef[]} components * @param {ClientSession} session - * @param {{ state?: FormStatus; position?: number }} [options] + * @param {{ position?: number }} [options] * @returns {Promise} */ export async function addComponents( @@ -355,12 +326,8 @@ export async function addComponents( pageId, components, session, - { position, state = FormStatus.Draft } = {} + { position } = {} ) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot add component to a live form - ${formId}`) - } - logger.info(`Adding a new component to form ID ${formId}`) const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( @@ -396,20 +363,14 @@ export async function addComponents( * @param {string} componentId * @param {ComponentDef} component * @param {ClientSession} session - * @param {FormStatus} [state] */ export async function updateComponent( formId, pageId, componentId, component, - session, - state = FormStatus.Draft + session ) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot update component on a live form - ${formId}`) - } - logger.info( `Updating component ID ${componentId} on page ID ${pageId} and form ID ${formId}` ) @@ -455,19 +416,8 @@ export async function updateComponent( * @param {string} pageId * @param {string} componentId * @param {ClientSession} session - * @param {FormStatus} [state] */ -export async function deleteComponent( - formId, - pageId, - componentId, - session, - state = FormStatus.Draft -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot delete component on a live form - ${formId}`) - } - +export async function deleteComponent(formId, pageId, componentId, session) { logger.info( `Deleting component ID ${componentId} on page ID ${pageId} and form ID ${formId}` ) @@ -499,19 +449,8 @@ export async function deleteComponent( * @param {string} pageId * @param {PatchPageFields} pageFields * @param {ClientSession} session - * @param {FormStatus} state */ -export async function updatePageFields( - formId, - pageId, - pageFields, - session, - state = FormStatus.Draft -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot update pageFields on a live form - ${formId}`) - } - +export async function updatePageFields(formId, pageId, pageFields, session) { const pageFieldKeys = Object.keys(pageFields) logger.info( @@ -559,18 +498,8 @@ export async function updatePageFields( * @param {string} formId * @param {List[]} lists * @param {ClientSession} session - * @param {FormStatus} state */ -export async function addLists( - formId, - lists, - session, - state = FormStatus.Draft -) { - if (state === FormStatus.Live) { - throw Boom.badRequest(`Cannot add lists to a live form - ${formId}`) - } - +export async function addLists(formId, lists, session) { logger.info(`Adding new lists to form ID ${formId}`) const coll = /** @satisfies {Collection<{draft: FormDefinition}>} */ ( diff --git a/src/api/forms/repositories/form-definition-repository.test.js b/src/api/forms/repositories/form-definition-repository.test.js index b50071ac..4f1ed6d0 100644 --- a/src/api/forms/repositories/form-definition-repository.test.js +++ b/src/api/forms/repositories/form-definition-repository.test.js @@ -112,21 +112,6 @@ describe('form-definition-repository', () => { }) describe('removeMatchingPages', () => { - it('should not edit a live summary', async () => { - await expect( - removeMatchingPages( - formId, - { controller: ControllerType.Summary }, - mockSession, - FormStatus.Live - ) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot remove page on live form ID 1eabd1437567fe1b26708bbb' - ) - ) - }) - it('should remove a page', async () => { await removeMatchingPages( formId, @@ -146,16 +131,6 @@ describe('form-definition-repository', () => { describe('addPageAtPosition', () => { const page = buildQuestionPage() - it('should not edit a live summary', async () => { - await expect( - addPageAtPosition('1234', page, mockSession, { - state: FormStatus.Live - }) - ).rejects.toThrow( - Boom.badRequest('Cannot remove add on live form ID 1234') - ) - }) - it('should add a page at position', async () => { await addPageAtPosition(formId, page, mockSession, { position: -1 }) @@ -191,15 +166,6 @@ describe('form-definition-repository', () => { components: [buildTextFieldComponent({})] }) - it('should fail if form is live', async () => { - await expect( - updatePage(formId, pageId, page, mockSession, FormStatus.Live) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot update page on a live form - 1eabd1437567fe1b26708bbb' - ) - ) - }) it('should update a page', async () => { await updatePage(formId, pageId, page, mockSession) const [filter, update] = mockCollection.updateOne.mock.calls[0] @@ -219,18 +185,6 @@ describe('form-definition-repository', () => { describe('addComponents', () => { const component = buildTextFieldComponent() - it('should fail if form is live', async () => { - await expect( - addComponents(formId, pageId, [component], mockSession, { - state: FormStatus.Live - }) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot add component to a live form - 1eabd1437567fe1b26708bbb' - ) - ) - }) - it('should add a component to a page', async () => { await addComponents(formId, pageId, [component], mockSession) const [filter, update] = mockCollection.updateOne.mock.calls[0] @@ -329,23 +283,6 @@ describe('form-definition-repository', () => { ) ) }) - - it('should fail if state is live', async () => { - await expect( - updateComponent( - formId, - pageId, - componentId, - component, - mockSession, - FormStatus.Live - ) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot update component on a live form - 1eabd1437567fe1b26708bbb' - ) - ) - }) }) describe('updatePageFields', () => { @@ -412,40 +349,9 @@ describe('form-definition-repository', () => { } }) }) - - it('should fail if form is live', async () => { - await expect( - updatePageFields( - formId, - pageId, - pageFields, - mockSession, - FormStatus.Live - ) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot update pageFields on a live form - 1eabd1437567fe1b26708bbb' - ) - ) - }) }) describe('deleteComponent', () => { - it('should fail if form is live', async () => { - await expect( - deleteComponent( - formId, - pageId, - componentId, - mockSession, - FormStatus.Live - ) - ).rejects.toThrow( - Boom.badRequest( - 'Cannot delete component on a live form - 1eabd1437567fe1b26708bbb' - ) - ) - }) it('should delete a component', async () => { await deleteComponent(formId, pageId, componentId, mockSession) const [filter, update] = mockCollection.updateOne.mock.calls[0] @@ -542,14 +448,6 @@ describe('form-definition-repository', () => { } }) }) - - it('should fail if form is live', async () => { - await expect( - addLists(formId, [], mockSession, FormStatus.Live) - ).rejects.toThrow( - Boom.badRequest(`Cannot add lists to a live form - ${formId}`) - ) - }) }) describe('updateList', () => { diff --git a/src/api/forms/service/component.js b/src/api/forms/service/component.js index 0ad698e1..0f2b39c2 100644 --- a/src/api/forms/service/component.js +++ b/src/api/forms/service/component.js @@ -95,7 +95,7 @@ export async function createComponentOnDraftDefinition( pageId, createdComponents, session, - { state: FormStatus.Draft, ...positionOptions } + positionOptions ) // Update the form with the new draft state @@ -150,8 +150,7 @@ export async function updateComponentOnDraftDefinition( pageId, componentId, componentPayload, - session, - FormStatus.Draft + session ) // Update the form with the new draft state @@ -206,8 +205,7 @@ export async function deleteComponentOnDraftDefinition( formId, pageId, componentId, - session, - FormStatus.Draft + session ) // Update the form with the new draft state diff --git a/src/api/forms/service/component.test.js b/src/api/forms/service/component.test.js index 5414c557..76dcc0d9 100644 --- a/src/api/forms/service/component.test.js +++ b/src/api/forms/service/component.test.js @@ -1,4 +1,3 @@ -import { FormStatus } from '@defra/forms-model' import Boom from '@hapi/boom' import { pino } from 'pino' @@ -61,7 +60,6 @@ const { empty: emptyFormWithSummary } = /** @type {typeof formTemplates} */ ( jest.requireActual('~/src/api/forms/templates.js') ) const author = getAuthor() -const DRAFT = 'draft' describe('Forms service', () => { const id = '661e4ca5039739ef2902b214' @@ -163,7 +161,7 @@ describe('Forms service', () => { expect(components).toEqual([ { ...textFieldComponent, id: expect.any(String) } ]) - expect(state).toEqual({ state: FormStatus.Draft }) + expect(state).toEqual({}) expect(metaFormId).toBe('123') @@ -192,7 +190,7 @@ describe('Forms service', () => { const [, , , , options] = dbDefinitionSpy.mock.calls[0] - expect(options).toEqual({ state: FormStatus.Draft, position: 0 }) + expect(options).toEqual({ position: 0 }) }) it('should fail if page does not exist', async () => { @@ -252,21 +250,14 @@ describe('Forms service', () => { expect(component).toEqual(newTextFieldComponent) expect(dbDefinitionSpy).toHaveBeenCalled() - const [ - calledFormId, - calledPageId, - calledComponentId, - calledComponent, - , - calledState - ] = dbDefinitionSpy.mock.calls[0] - - expect([ - calledFormId, - calledPageId, - calledComponentId, - calledState - ]).toEqual([id, pageId, componentId, DRAFT]) + const [calledFormId, calledPageId, calledComponentId, calledComponent] = + dbDefinitionSpy.mock.calls[0] + + expect([calledFormId, calledPageId, calledComponentId]).toEqual([ + id, + pageId, + componentId + ]) expect(calledComponent).toEqual(newTextFieldComponent) const [metaFormId, metaUpdateOperations] = dbMetadataSpy.mock.calls[0] expect(metaFormId).toBe(id) @@ -332,15 +323,14 @@ describe('Forms service', () => { await deleteComponentOnDraftDefinition(id, pageId, componentId2, author) expect(dbDefinitionSpy).toHaveBeenCalled() - const [calledFormId, calledPageId, calledComponentId, , calledState] = + const [calledFormId, calledPageId, calledComponentId] = dbDefinitionSpy.mock.calls[0] - expect([ - calledFormId, - calledPageId, - calledComponentId, - calledState - ]).toEqual([id, pageId, componentId2, DRAFT]) + expect([calledFormId, calledPageId, calledComponentId]).toEqual([ + id, + pageId, + componentId2 + ]) const [metaFormId, metaUpdateOperations] = dbMetadataSpy.mock.calls[0] expect(metaFormId).toBe(id) diff --git a/src/api/forms/service/lists.test.js b/src/api/forms/service/lists.test.js index 3816e120..4fbaba6d 100644 --- a/src/api/forms/service/lists.test.js +++ b/src/api/forms/service/lists.test.js @@ -60,10 +60,9 @@ describe('lists', () => { expectedLists, author ) - const [expectedFormId, listToInsert, , state] = addListsMock.mock.calls[0] + const [expectedFormId, listToInsert] = addListsMock.mock.calls[0] expect(expectedFormId).toBe(id) expect(listToInsert).toEqual(expectedLists) - expect(state).toBeUndefined() expect(result).toEqual(expectedLists) expectMetadataUpdate() }) diff --git a/src/api/forms/service/migration.test.js b/src/api/forms/service/migration.test.js index ba5c06e4..1de07224 100644 --- a/src/api/forms/service/migration.test.js +++ b/src/api/forms/service/migration.test.js @@ -119,8 +119,7 @@ describe('migration', () => { expect(addPageAtPositionSpy).toHaveBeenCalled() expect(formMetadataUpdateSpy).toHaveBeenCalled() - const [formId1, matchCriteria, , state] = - removeMatchingPagesSpy.mock.calls[0] + const [formId1, matchCriteria] = removeMatchingPagesSpy.mock.calls[0] const [formId2, calledSummary, , options] = addPageAtPositionSpy.mock.calls[0] const [formId3, updateFilter] = formMetadataUpdateSpy.mock.calls[0] @@ -130,7 +129,6 @@ describe('migration', () => { expect(formId3).toBe(id) expect(matchCriteria).toEqual({ controller: ControllerType.Summary }) expect(calledSummary).toEqual(summary) - expect(state).toBeUndefined() expect(options).toEqual({}) expect(updateFilter.$set).toEqual({ 'draft.updatedAt': dateUsedInFakeTime, diff --git a/src/api/forms/service/page.js b/src/api/forms/service/page.js index 8f4b1425..6c98b22b 100644 --- a/src/api/forms/service/page.js +++ b/src/api/forms/service/page.js @@ -159,8 +159,7 @@ export async function patchFieldsOnDraftDefinitionPage( formId, pageId, pageFieldsToUpdate, - session, - FormStatus.Draft + session ) page = await getFormDefinitionPage(formId, pageId, session) diff --git a/src/api/forms/service/page.test.js b/src/api/forms/service/page.test.js index 04573ecb..b5d493a5 100644 --- a/src/api/forms/service/page.test.js +++ b/src/api/forms/service/page.test.js @@ -8,10 +8,7 @@ import { } from '~/src/api/forms/__stubs__/definition.js' 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 { - DRAFT, - formMetadataDocument -} from '~/src/api/forms/service/__stubs__/service.js' +import { formMetadataDocument } from '~/src/api/forms/service/__stubs__/service.js' import { createPageOnDraftDefinition, getFormDefinitionPage, @@ -247,13 +244,12 @@ describe('Forms service', () => { }) expect(dbDefinitionSpy).toHaveBeenCalled() - const [formId, calledPageId, pageFieldsToUpdate, , state] = + const [formId, calledPageId, pageFieldsToUpdate] = dbDefinitionSpy.mock.calls[0] expect(formId).toBe('123') expect(calledPageId).toBe(pageId) expect(pageFieldsToUpdate).toEqual(pageFields) - expect(state).toBe(DRAFT) expect(dbDefinitionGetSpy.mock.calls[1][2]).toMatchObject({ withTransaction: expect.anything() From 0a713f1f311e4024a6a27100142ed1d9d46e3ef7 Mon Sep 17 00:00:00 2001 From: Chris Cole Date: Mon, 17 Mar 2025 16:32:53 +0000 Subject: [PATCH 2/5] refactor: 481476 - Use callback for service - suggestion --- src/api/forms/service/lists.js | 63 +++++++--------------- src/api/forms/service/shared.js | 57 +++++++++++++++++--- src/api/forms/service/shared.test.js | 81 ++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 49 deletions(-) create mode 100644 src/api/forms/service/shared.test.js diff --git a/src/api/forms/service/lists.js b/src/api/forms/service/lists.js index e547a844..a124bed9 100644 --- a/src/api/forms/service/lists.js +++ b/src/api/forms/service/lists.js @@ -1,6 +1,10 @@ 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, partialAuditFields } from '~/src/api/forms/service/shared.js' +import { + callSessionTransaction, + logger, + partialAuditFields +} from '~/src/api/forms/service/shared.js' import { client } from '~/src/mongo.js' /** @@ -67,49 +71,21 @@ export async function updateListOnDraftFormDefinition( list, author ) { - logger.info( - `Updating list ${listId} on Form Definition (draft) for form ID ${formId}` + /** + * @param {ClientSession} session + * @returns {Promise} + */ + const callUpdateListsHandler = (session) => + formDefinition.updateList(formId, listId, list, session) + + return callSessionTransaction( + formId, + callUpdateListsHandler, + author, + `Updating list ${listId} on Form Definition (draft) for form ID ${formId}`, + `Updated list ${listId} on Form Definition (draft) for form ID ${formId}`, + `Failed to update list ${listId} on Form Definition (draft) for form ID ${formId}` ) - - const session = client.startSession() - - try { - const updatedList = await session.withTransaction(async () => { - // Update the list on the form definition - const returnedLists = await formDefinition.updateList( - formId, - listId, - list, - session - ) - - const now = new Date() - await formMetadata.update( - formId, - { - $set: partialAuditFields(now, author) - }, - session - ) - - return returnedLists - }) - - logger.info( - `Updated list ${listId} on Form Definition (draft) for form ID ${formId}` - ) - - return updatedList - } catch (err) { - logger.error( - err, - `Failed to update list ${listId} on Form Definition (draft) for form ID ${formId}` - ) - - throw err - } finally { - await session.endSession() - } } /** @@ -157,4 +133,5 @@ export async function removeListOnDraftFormDefinition(formId, listId, author) { /** * @import { FormMetadataAuthor, List } from '@defra/forms-model' + * @import { ClientSession } from 'mongodb' */ diff --git a/src/api/forms/service/shared.js b/src/api/forms/service/shared.js index 26dfea73..6f4fa862 100644 --- a/src/api/forms/service/shared.js +++ b/src/api/forms/service/shared.js @@ -1,6 +1,8 @@ import { FormStatus } from '@defra/forms-model' +import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { createLogger } from '~/src/helpers/logging/logger.js' +import { client } from '~/src/mongo.js' export const logger = createLogger() export const defaultAuthor = { @@ -89,10 +91,6 @@ export function getLastUpdated(document) { } } -/** - * @import { FormMetadataAuthor } from '@defra/forms-model' - * @import { PartialFormMetadataDocument } from '~/src/api/types.js' - */ /** * @param {Partial} document - form metadata document * @returns {{ createdAt: Date, createdBy: FormMetadataAuthor }} @@ -111,6 +109,53 @@ export function getCreated(document) { } /** - * @import { FormMetadataDocument, FormMetadata } from '@defra/forms-model' - * @import { WithId } from 'mongodb' + * Abstraction of a generic service method + * @template T + * @param {string} formId + * @param {(session: ClientSession) => Promise} asyncHandler + * @param {FormMetadataAuthor} author + * @param {string} startLog + * @param {string} endLog + * @param {string} failLog + * @returns {Promise} + */ +export async function callSessionTransaction( + formId, + asyncHandler, + author, + startLog, + endLog, + failLog +) { + logger.info(startLog) + + const session = client.startSession() + + try { + const sessionReturn = await session.withTransaction(async () => { + const handlerReturn = await asyncHandler(session) + + // Update the form with the new draft state + await formMetadata.update( + formId, + { $set: partialAuditFields(new Date(), author) }, + session + ) + + return handlerReturn + }) + logger.info(endLog) + + return sessionReturn + } catch (err) { + logger.error(err, failLog) + throw err + } finally { + await session.endSession() + } +} +/** + * @import { FormMetadataDocument, FormMetadata, FormMetadataAuthor } from '@defra/forms-model' + * @import { PartialFormMetadataDocument } from '~/src/api/types.js' + * @import { WithId, ClientSession } from 'mongodb' */ diff --git a/src/api/forms/service/shared.test.js b/src/api/forms/service/shared.test.js new file mode 100644 index 00000000..f1bf1508 --- /dev/null +++ b/src/api/forms/service/shared.test.js @@ -0,0 +1,81 @@ +import Boom from '@hapi/boom' +import { pino } from 'pino' + +import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' +import { formMetadataDocument } from '~/src/api/forms/service/__stubs__/service.js' +import { callSessionTransaction } from '~/src/api/forms/service/shared.js' +import { getAuthor } from '~/src/helpers/get-author.js' +import { prepareDb } from '~/src/mongo.js' + +jest.mock('~/src/helpers/get-author.js') +jest.mock('~/src/api/forms/repositories/form-metadata-repository.js') +jest.mock('~/src/mongo.js') + +jest.useFakeTimers().setSystemTime(new Date('2020-01-01')) +describe('lists', () => { + beforeAll(async () => { + await prepareDb(pino()) + }) + + beforeEach(() => { + jest.mocked(formMetadata.get).mockResolvedValue(formMetadataDocument) + }) + + describe('callSessionTransaction', () => { + const id = '661e4ca5039739ef2902b214' + const author = getAuthor() + const dateUsedInFakeTime = new Date('2020-01-01') + const defaultAudit = { + 'draft.updatedAt': dateUsedInFakeTime, + 'draft.updatedBy': author, + updatedAt: dateUsedInFakeTime, + updatedBy: author + } + + beforeAll(async () => { + await prepareDb(pino()) + }) + + it('should update the component', async () => { + const transactionResolved = '265a71fd-f2c2-4028-94aa-7c1e2739730f' + const transactionHandler = jest + .fn() + .mockResolvedValue(transactionResolved) + + const dbMetadataSpy = jest.spyOn(formMetadata, 'update') + + const result = await callSessionTransaction( + id, + transactionHandler, + author, + 'started', + 'finished', + 'failed' + ) + + expect(transactionHandler).toHaveBeenCalled() + expect(result).toBe(transactionResolved) + const [metaFormId, metaUpdateOperations] = dbMetadataSpy.mock.calls[0] + expect(metaFormId).toBe(id) + + expect(metaUpdateOperations.$set).toEqual(defaultAudit) + }) + + it('should correctly surface the error is the component is not found', async () => { + const transactionHandler = jest + .fn() + .mockRejectedValue(Boom.notFound('Not found')) + + await expect( + callSessionTransaction( + id, + transactionHandler, + author, + 'started', + 'finished', + 'failed' + ) + ).rejects.toThrow(Boom.notFound('Not found')) + }) + }) +}) From 86435a5fd63ab13a280032df13a06af16bffee63 Mon Sep 17 00:00:00 2001 From: Chris Cole Date: Wed, 26 Mar 2025 13:26:38 +0000 Subject: [PATCH 3/5] refactor: 481476 - Move callSessionTransaction to own file --- .../forms/service/callSessionTransaction.js | 51 +++++++++++++++++++ ...test.js => callSessionTransaction.test.js} | 23 ++++----- 2 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 src/api/forms/service/callSessionTransaction.js rename src/api/forms/service/{shared.test.js => callSessionTransaction.test.js} (88%) diff --git a/src/api/forms/service/callSessionTransaction.js b/src/api/forms/service/callSessionTransaction.js new file mode 100644 index 00000000..0b1fb071 --- /dev/null +++ b/src/api/forms/service/callSessionTransaction.js @@ -0,0 +1,51 @@ +import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' +import { logger, partialAuditFields } from '~/src/api/forms/service/shared.js' +import { client } from '~/src/mongo.js' + +/** + * Abstraction of a generic service method + * @template T + * @param {string} formId + * @param {(session: ClientSession) => Promise} asyncHandler + * @param {FormMetadataAuthor} author + * @param {{ start: string; end: string; fail: string }} logs + * @returns {Promise} + */ +export async function callSessionTransaction( + formId, + asyncHandler, + author, + logs +) { + logger.info(logs.start) + + const session = client.startSession() + + try { + const sessionReturn = await session.withTransaction(async () => { + const handlerReturn = await asyncHandler(session) + + // Update the form with the new draft state + await formMetadata.update( + formId, + { $set: partialAuditFields(new Date(), author) }, + session + ) + + return handlerReturn + }) + logger.info(logs.end) + + return sessionReturn + } catch (err) { + logger.error(err, logs.fail) + throw err + } finally { + await session.endSession() + } +} + +/** + * @import { FormMetadataAuthor } from '@defra/forms-model' + * @import { ClientSession } from 'mongodb' + */ diff --git a/src/api/forms/service/shared.test.js b/src/api/forms/service/callSessionTransaction.test.js similarity index 88% rename from src/api/forms/service/shared.test.js rename to src/api/forms/service/callSessionTransaction.test.js index f1bf1508..04d7d484 100644 --- a/src/api/forms/service/shared.test.js +++ b/src/api/forms/service/callSessionTransaction.test.js @@ -3,7 +3,7 @@ import { pino } from 'pino' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { formMetadataDocument } from '~/src/api/forms/service/__stubs__/service.js' -import { callSessionTransaction } from '~/src/api/forms/service/shared.js' +import { callSessionTransaction } from '~/src/api/forms/service/callSessionTransaction.js' import { getAuthor } from '~/src/helpers/get-author.js' import { prepareDb } from '~/src/mongo.js' @@ -48,9 +48,11 @@ describe('lists', () => { id, transactionHandler, author, - 'started', - 'finished', - 'failed' + { + start: 'started', + end: 'finished', + fail: 'failed' + } ) expect(transactionHandler).toHaveBeenCalled() @@ -67,14 +69,11 @@ describe('lists', () => { .mockRejectedValue(Boom.notFound('Not found')) await expect( - callSessionTransaction( - id, - transactionHandler, - author, - 'started', - 'finished', - 'failed' - ) + callSessionTransaction(id, transactionHandler, author, { + start: 'started', + end: 'finished', + fail: 'failed' + }) ).rejects.toThrow(Boom.notFound('Not found')) }) }) From 73bbfd5e551fa7c9d780d667bb5d2f60eae2d0d4 Mon Sep 17 00:00:00 2001 From: Chris Cole Date: Wed, 26 Mar 2025 13:27:38 +0000 Subject: [PATCH 4/5] test: 481476 - Add high level test for lists --- src/api/forms/service/index.test.js | 38 +++++++++ src/api/forms/service/lists.js | 118 ++++++---------------------- 2 files changed, 64 insertions(+), 92 deletions(-) diff --git a/src/api/forms/service/index.test.js b/src/api/forms/service/index.test.js index bab369a4..42a8053d 100644 --- a/src/api/forms/service/index.test.js +++ b/src/api/forms/service/index.test.js @@ -2,6 +2,7 @@ import Boom from '@hapi/boom' import { MongoServerError, ObjectId } from 'mongodb' import { pino } from 'pino' +import { buildList } from '~/src/api/forms/__stubs__/definition.js' import { InvalidFormDefinitionError } from '~/src/api/forms/errors.js' import * as formDefinition from '~/src/api/forms/repositories/form-definition-repository.js' import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' @@ -21,6 +22,7 @@ import { removeForm, updateFormMetadata } from '~/src/api/forms/service/index.js' +import { addListsToDraftFormDefinition } from '~/src/api/forms/service/lists.js' import * as formTemplates from '~/src/api/forms/templates.js' import { getAuthor } from '~/src/helpers/get-author.js' import { prepareDb } from '~/src/mongo.js' @@ -355,6 +357,42 @@ describe('Forms service', () => { ) }) }) + + describe('high level tests', () => { + const defaultAudit = { + 'draft.updatedAt': dateUsedInFakeTime, + 'draft.updatedBy': author, + updatedAt: dateUsedInFakeTime, + updatedBy: author + } + const dbMetadataSpy = jest.spyOn(formMetadata, 'update') + const expectMetadataUpdate = () => { + expect(dbMetadataSpy).toHaveBeenCalled() + const [formId, updateFilter] = dbMetadataSpy.mock.calls[0] + expect(formId).toBe(id) + expect(updateFilter.$set).toEqual(defaultAudit) + } + + describe('addListsToDraftFormDefinition', () => { + it('should add a list of lists to the form definition', async () => { + const expectedLists = [buildList()] + const addListsMock = jest + .mocked(formDefinition.addLists) + .mockResolvedValueOnce(expectedLists) + + const result = await addListsToDraftFormDefinition( + id, + expectedLists, + author + ) + const [expectedFormId, listToInsert] = addListsMock.mock.calls[0] + expect(expectedFormId).toBe(id) + expect(listToInsert).toEqual(expectedLists) + expect(result).toEqual(expectedLists) + expectMetadataUpdate() + }) + }) + }) }) /** diff --git a/src/api/forms/service/lists.js b/src/api/forms/service/lists.js index a124bed9..61573e38 100644 --- a/src/api/forms/service/lists.js +++ b/src/api/forms/service/lists.js @@ -1,11 +1,5 @@ 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 { - callSessionTransaction, - logger, - partialAuditFields -} from '~/src/api/forms/service/shared.js' -import { client } from '~/src/mongo.js' +import { callSessionTransaction } from '~/src/api/forms/service/callSessionTransaction.js' /** * Add a list of new lists to the draft form definition @@ -14,48 +8,17 @@ import { client } from '~/src/mongo.js' * @param {FormMetadataAuthor} author */ export async function addListsToDraftFormDefinition(formId, lists, author) { - logger.info( - `Adding lists ${lists.map((list) => list.name).join(', ')} on Form Definition (draft) for form ID ${formId}` + const listStr = lists.map((list) => list.name).join(', ') + return callSessionTransaction( + formId, + (session) => formDefinition.addLists(formId, lists, session), + author, + { + start: `Adding lists ${listStr} on Form Definition (draft) for form ID ${formId}`, + end: `Added lists ${listStr} on Form Definition (draft) for form ID ${formId}`, + fail: `Failed to add lists ${listStr} on Form Definition (draft) for form ID ${formId}` + } ) - - const session = client.startSession() - - try { - const newForm = await session.withTransaction(async () => { - // Add the lists to the form definition - const returnedLists = await formDefinition.addLists( - formId, - lists, - session - ) - - const now = new Date() - await formMetadata.update( - formId, - { - $set: partialAuditFields(now, author) - }, - session - ) - - return returnedLists - }) - - logger.info( - `Added lists ${lists.map((list) => list.name).join(', ')} on Form Definition (draft) for form ID ${formId}` - ) - - return newForm - } catch (err) { - logger.error( - err, - `Failed to add lists ${lists.map((list) => list.id).join(', ')} on Form Definition (draft) for form ID ${formId}` - ) - - throw err - } finally { - await session.endSession() - } } /** @@ -71,20 +34,15 @@ export async function updateListOnDraftFormDefinition( list, author ) { - /** - * @param {ClientSession} session - * @returns {Promise} - */ - const callUpdateListsHandler = (session) => - formDefinition.updateList(formId, listId, list, session) - return callSessionTransaction( formId, - callUpdateListsHandler, + (session) => formDefinition.updateList(formId, listId, list, session), author, - `Updating list ${listId} on Form Definition (draft) for form ID ${formId}`, - `Updated list ${listId} on Form Definition (draft) for form ID ${formId}`, - `Failed to update list ${listId} on Form Definition (draft) for form ID ${formId}` + { + start: `Updating list ${listId} on Form Definition (draft) for form ID ${formId}`, + end: `Updated list ${listId} on Form Definition (draft) for form ID ${formId}`, + fail: `Failed to update list ${listId} on Form Definition (draft) for form ID ${formId}` + } ) } @@ -95,40 +53,16 @@ export async function updateListOnDraftFormDefinition( * @param {FormMetadataAuthor} author */ export async function removeListOnDraftFormDefinition(formId, listId, author) { - logger.info( - `Removing list ${listId} on Form Definition (draft) for form ID ${formId}` + await callSessionTransaction( + formId, + (session) => formDefinition.removeList(formId, listId, session), + author, + { + start: `Removing list ${listId} on Form Definition (draft) for form ID ${formId}`, + end: `Removed list ${listId} on Form Definition (draft) for form ID ${formId}`, + fail: `Failed to remove list ${listId} on Form Definition (draft) for form ID ${formId}` + } ) - - const session = client.startSession() - - try { - await session.withTransaction(async () => { - // Update the list on the form definition - await formDefinition.removeList(formId, listId, session) - - const now = new Date() - await formMetadata.update( - formId, - { - $set: partialAuditFields(now, author) - }, - session - ) - }) - - logger.info( - `Removed list ${listId} on Form Definition (draft) for form ID ${formId}` - ) - } catch (err) { - logger.error( - err, - `Failed to remove list ${listId} on Form Definition (draft) for form ID ${formId}` - ) - - throw err - } finally { - await session.endSession() - } } /** From e71fcb379fdd657cbef632452b6d8de555ecbc56 Mon Sep 17 00:00:00 2001 From: Chris Cole Date: Wed, 26 Mar 2025 13:27:57 +0000 Subject: [PATCH 5/5] test: 481476 - Update the lists test --- src/api/forms/service/lists.test.js | 142 ++++++++++++++-------------- src/api/forms/service/shared.js | 48 ---------- 2 files changed, 71 insertions(+), 119 deletions(-) diff --git a/src/api/forms/service/lists.test.js b/src/api/forms/service/lists.test.js index 4fbaba6d..f10c2b76 100644 --- a/src/api/forms/service/lists.test.js +++ b/src/api/forms/service/lists.test.js @@ -1,18 +1,14 @@ -import Boom from '@hapi/boom' -import { pino } from 'pino' - import { buildList } from '~/src/api/forms/__stubs__/definition.js' 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 { formMetadataDocument } from '~/src/api/forms/service/__stubs__/service.js' +import { callSessionTransaction } from '~/src/api/forms/service/callSessionTransaction.js' import { addListsToDraftFormDefinition, removeListOnDraftFormDefinition, updateListOnDraftFormDefinition } from '~/src/api/forms/service/lists.js' import { getAuthor } from '~/src/helpers/get-author.js' -import { prepareDb } from '~/src/mongo.js' +jest.mock('~/src/api/forms/service/callSessionTransaction.js') jest.mock('~/src/helpers/get-author.js') jest.mock('~/src/api/forms/repositories/form-definition-repository.js') jest.mock('~/src/api/forms/repositories/form-metadata-repository.js') @@ -23,63 +19,58 @@ jest.useFakeTimers().setSystemTime(new Date('2020-01-01')) describe('lists', () => { const id = '661e4ca5039739ef2902b214' const author = getAuthor() - const dateUsedInFakeTime = new Date('2020-01-01') - const defaultAudit = { - 'draft.updatedAt': dateUsedInFakeTime, - 'draft.updatedBy': author, - updatedAt: dateUsedInFakeTime, - updatedBy: author - } - - const dbMetadataSpy = jest.spyOn(formMetadata, 'update') - - const expectMetadataUpdate = () => { - expect(dbMetadataSpy).toHaveBeenCalled() - const [formId, updateFilter] = dbMetadataSpy.mock.calls[0] - expect(formId).toBe(id) - expect(updateFilter.$set).toEqual(defaultAudit) - } - - beforeAll(async () => { - await prepareDb(pino()) - }) + const callSessionTransactionMock = jest.mocked(callSessionTransaction) + /** + * @type {any} + */ + const mockSession = author + const listId = '47cfaf57-6cda-44aa-9268-f37c674823d2' beforeEach(() => { - jest.mocked(formMetadata.get).mockResolvedValue(formMetadataDocument) + callSessionTransactionMock.mockClear() }) describe('addListsToDraftFormDefinition', () => { - it('should add a list of lists to the form definition', async () => { - const expectedLists = [buildList()] + it('should call callSessionTransaction with the correct parameters', async () => { + const expectedLists = [ + buildList({ + name: 'my-list' + }), + buildList({ + name: 'my-list-2' + }) + ] const addListsMock = jest .mocked(formDefinition.addLists) .mockResolvedValueOnce(expectedLists) + callSessionTransactionMock.mockResolvedValueOnce(expectedLists) const result = await addListsToDraftFormDefinition( id, expectedLists, author ) - const [expectedFormId, listToInsert] = addListsMock.mock.calls[0] - expect(expectedFormId).toBe(id) - expect(listToInsert).toEqual(expectedLists) + expect(callSessionTransactionMock).toHaveBeenCalledWith( + id, + expect.any(Function), + author, + { + start: `Adding lists my-list, my-list-2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + end: `Added lists my-list, my-list-2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + fail: `Failed to add lists my-list, my-list-2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214` + } + ) + const [, callback] = callSessionTransactionMock.mock.calls[0] + await callback(mockSession) + expect(addListsMock).toHaveBeenCalledWith(id, expectedLists, mockSession) expect(result).toEqual(expectedLists) - expectMetadataUpdate() - }) - it('should surface errors', async () => { - const boomInternal = Boom.internal('Something went wrong') - jest.mocked(formDefinition.addLists).mockRejectedValueOnce(boomInternal) - await expect( - addListsToDraftFormDefinition(id, [buildList()], author) - ).rejects.toThrow(boomInternal) }) }) describe('updateListOnDraftFormDefinition', () => { - const listToUpdate = buildList() - const listId = '47cfaf57-6cda-44aa-9268-f37c674823d2' - it('should update a list on the form definition', async () => { + const listToUpdate = buildList() + callSessionTransactionMock.mockResolvedValueOnce(listToUpdate) const updateListMock = jest .mocked(formDefinition.updateList) .mockResolvedValueOnce(listToUpdate) @@ -90,41 +81,50 @@ describe('lists', () => { listToUpdate, author ) - const [expectedFormId, expectedListId, expectedListToUpdate] = - updateListMock.mock.calls[0] - expect(expectedFormId).toBe(id) - expect(expectedListId).toBe(listId) - expect(expectedListToUpdate).toEqual(listToUpdate) + + expect(callSessionTransactionMock).toHaveBeenCalledWith( + id, + expect.any(Function), + author, + { + start: `Updating list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + end: `Updated list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + fail: `Failed to update list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214` + } + ) + + const [, callback] = callSessionTransactionMock.mock.calls[0] + await callback(mockSession) + expect(updateListMock).toHaveBeenCalledWith( + id, + listId, + listToUpdate, + mockSession + ) expect(result).toEqual(listToUpdate) - expectMetadataUpdate() - }) - it('should surface errors', async () => { - const boomInternal = Boom.internal('Something went wrong') - jest.mocked(formDefinition.updateList).mockRejectedValueOnce(boomInternal) - await expect( - updateListOnDraftFormDefinition(id, listId, listToUpdate, author) - ).rejects.toThrow(boomInternal) }) }) describe('removeListOnDraftFormDefinition', () => { - const listId = '47cfaf57-6cda-44aa-9268-f37c674823d2' - it('should remove a list on the form definition', async () => { await removeListOnDraftFormDefinition(id, listId, author) - const [expectedFormId, expectedListId] = jest.mocked( - formDefinition.removeList - ).mock.calls[0] - expect(expectedFormId).toBe(id) - expect(expectedListId).toBe(listId) - expectMetadataUpdate() - }) - it('should surface errors', async () => { - const boomInternal = Boom.internal('Something went wrong') - jest.mocked(formDefinition.removeList).mockRejectedValueOnce(boomInternal) - await expect( - removeListOnDraftFormDefinition(id, listId, author) - ).rejects.toThrow(boomInternal) + expect(callSessionTransactionMock).toHaveBeenCalledWith( + id, + expect.any(Function), + author, + { + start: `Removing list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + end: `Removed list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214`, + fail: `Failed to remove list 47cfaf57-6cda-44aa-9268-f37c674823d2 on Form Definition (draft) for form ID 661e4ca5039739ef2902b214` + } + ) + const [, callback] = callSessionTransactionMock.mock.calls[0] + await callback(mockSession) + expect(jest.mocked(formDefinition.removeList)).toHaveBeenCalledWith( + id, + listId, + mockSession + ) }) }) }) diff --git a/src/api/forms/service/shared.js b/src/api/forms/service/shared.js index 6f4fa862..314fda1f 100644 --- a/src/api/forms/service/shared.js +++ b/src/api/forms/service/shared.js @@ -1,8 +1,6 @@ import { FormStatus } from '@defra/forms-model' -import * as formMetadata from '~/src/api/forms/repositories/form-metadata-repository.js' import { createLogger } from '~/src/helpers/logging/logger.js' -import { client } from '~/src/mongo.js' export const logger = createLogger() export const defaultAuthor = { @@ -108,52 +106,6 @@ export function getCreated(document) { } } -/** - * Abstraction of a generic service method - * @template T - * @param {string} formId - * @param {(session: ClientSession) => Promise} asyncHandler - * @param {FormMetadataAuthor} author - * @param {string} startLog - * @param {string} endLog - * @param {string} failLog - * @returns {Promise} - */ -export async function callSessionTransaction( - formId, - asyncHandler, - author, - startLog, - endLog, - failLog -) { - logger.info(startLog) - - const session = client.startSession() - - try { - const sessionReturn = await session.withTransaction(async () => { - const handlerReturn = await asyncHandler(session) - - // Update the form with the new draft state - await formMetadata.update( - formId, - { $set: partialAuditFields(new Date(), author) }, - session - ) - - return handlerReturn - }) - logger.info(endLog) - - return sessionReturn - } catch (err) { - logger.error(err, failLog) - throw err - } finally { - await session.endSession() - } -} /** * @import { FormMetadataDocument, FormMetadata, FormMetadataAuthor } from '@defra/forms-model' * @import { PartialFormMetadataDocument } from '~/src/api/types.js'