From e0a934b69fe66ebcebadb68ef10f13d1c95ddba0 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Mon, 13 Apr 2026 17:51:55 +0100 Subject: [PATCH 1/4] fix(DF-922): add version fallback and remove unused versionNumber option Add try-catch fallback to getFormDefinition so versioned endpoint failures fall back to the current definition instead of crashing. Remove unused versionNumber from FormModel constructor call. --- src/lib/manager.js | 28 ++++++++++++----- src/lib/manager.test.js | 66 +++++++++++++++++++++++++++++++++++++++ src/service/sharepoint.js | 3 +- 3 files changed, 88 insertions(+), 9 deletions(-) diff --git a/src/lib/manager.js b/src/lib/manager.js index 1850945..36d581c 100644 --- a/src/lib/manager.js +++ b/src/lib/manager.js @@ -1,11 +1,14 @@ import { FormStatus } from '@defra/forms-model' import { config } from '~/src/config/index.js' +import { createLogger } from '~/src/helpers/logging/logger.js' import { getJson } from '~/src/lib/fetch.js' const managerUrl = config.get('managerUrl') +const logger = createLogger() + /** - * Gets the form definition from the Forms Manager API∂ + * Gets the form definition from the Forms Manager API * @param {string} formId * @param {FormStatus} formStatus * @param {number|undefined} versionNumber - Optional specific version to fetch @@ -16,16 +19,27 @@ export async function getFormDefinition(formId, formStatus, versionNumber) { throw new Error('Missing MANAGER_URL') } - const statusPath = formStatus === FormStatus.Draft ? FormStatus.Draft : '' - const formUrl = - versionNumber || versionNumber === 0 - ? new URL( + if (versionNumber !== undefined) { + try { + const { body } = await getJson( + new URL( `/forms/${formId}/versions/${versionNumber}/definition`, managerUrl ) - : new URL(`/forms/${formId}/definition/${statusPath}`, managerUrl) + ) + return body + } catch (err) { + logger.warn( + err, + `[getFormDefinition] Version ${versionNumber} not found for form ${formId}, falling back to current definition` + ) + } + } - const { body } = await getJson(formUrl) + const statusPath = formStatus === FormStatus.Draft ? FormStatus.Draft : '' + const { body } = await getJson( + new URL(`/forms/${formId}/definition/${statusPath}`, managerUrl) + ) return body } diff --git a/src/lib/manager.test.js b/src/lib/manager.test.js index a6f18ed..89f54dd 100644 --- a/src/lib/manager.test.js +++ b/src/lib/manager.test.js @@ -5,6 +5,14 @@ import { getJson } from '~/src/lib/fetch.js' import { getFormDefinition, getFormMetadata } from '~/src/lib/manager.js' jest.mock('~/src/lib/fetch.js') +jest.mock('~/src/helpers/logging/logger.js', () => ({ + createLogger: () => ({ + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn() + }) +})) jest.mock('~/src/config/index.js', () => ({ config: { get: jest.fn().mockReturnValueOnce('http://forms-manager') @@ -91,6 +99,64 @@ describe('Manager', () => { expect(definition).toEqual(expectedDefinition) }) + it('should fall back to current definition when versioned endpoint fails', async () => { + const expectedDefinition = buildDefinition() + const formId = '68a890909ab460290c289409' + const versionNumber = 5 + jest + .mocked(getJson) + .mockRejectedValueOnce(new Error('Version not found')) + .mockResolvedValueOnce({ response: {}, body: expectedDefinition }) + const definition = await getFormDefinition( + formId, + FormStatus.Live, + versionNumber + ) + expect(getJson).toHaveBeenCalledTimes(2) + expect(getJson).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + href: 'http://forms-manager/forms/68a890909ab460290c289409/versions/5/definition' + }) + ) + expect(getJson).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + href: 'http://forms-manager/forms/68a890909ab460290c289409/definition/' + }) + ) + expect(definition).toEqual(expectedDefinition) + }) + + it('should fall back to draft definition when versioned endpoint fails for draft status', async () => { + const expectedDefinition = buildDefinition() + const formId = '68a890909ab460290c289409' + const versionNumber = 2 + jest + .mocked(getJson) + .mockRejectedValueOnce(new Error('Version not found')) + .mockResolvedValueOnce({ response: {}, body: expectedDefinition }) + const definition = await getFormDefinition( + formId, + FormStatus.Draft, + versionNumber + ) + expect(getJson).toHaveBeenCalledTimes(2) + expect(getJson).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + href: 'http://forms-manager/forms/68a890909ab460290c289409/versions/2/definition' + }) + ) + expect(getJson).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + href: 'http://forms-manager/forms/68a890909ab460290c289409/definition/draft' + }) + ) + expect(definition).toEqual(expectedDefinition) + }) + it('should handle version number 0 correctly', async () => { const expectedDefinition = buildDefinition() const formId = '68a890909ab460290c289409' diff --git a/src/service/sharepoint.js b/src/service/sharepoint.js index ffd240e..b11d8d7 100644 --- a/src/service/sharepoint.js +++ b/src/service/sharepoint.js @@ -312,8 +312,7 @@ export async function saveToSharepointList(message) { const columnProperties = await createMapOfColumnProperties(siteId, listId) const formModel = new FormModel(replaceCustomControllers(definition), { - basePath: '', - versionNumber: versionMetadata?.versionNumber + basePath: '' }) /** @type {Map} */ From 0d976b742402c6d66a53ffe12f291499ed536ce5 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Mon, 13 Apr 2026 18:49:31 +0100 Subject: [PATCH 2/4] fix(DF-922): add logger mock to manager-config test manager.js now imports createLogger at module level, so tests that import manager.js need to mock the logger to avoid pino init errors. --- src/lib/manager-config.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/lib/manager-config.test.js b/src/lib/manager-config.test.js index 47bb19c..5501459 100644 --- a/src/lib/manager-config.test.js +++ b/src/lib/manager-config.test.js @@ -5,6 +5,14 @@ import { getJson } from '~/src/lib/fetch.js' import { getFormDefinition, getFormMetadata } from '~/src/lib/manager.js' jest.mock('~/src/lib/fetch.js') +jest.mock('~/src/helpers/logging/logger.js', () => ({ + createLogger: () => ({ + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn() + }) +})) jest.mock('~/src/config/index.js', () => ({ config: { get: jest.fn().mockReturnValueOnce('') From 244258781df99ce5a3d7ace19703f66a4d3b03b1 Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 14 Apr 2026 09:36:16 +0100 Subject: [PATCH 3/4] fix: mock JWKS endpoint in test setup to prevent flaky network failures Use nock to intercept the OIDC JWKS endpoint during tests instead of hitting the real Microsoft endpoint, matching the pattern used by forms-submission-api. --- jest.setup.js | 23 ++++++++++- package-lock.json | 97 +++++++++++++++++++++++++++++++++++++++++++++++ package.json | 3 +- 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/jest.setup.js b/jest.setup.js index f2caddc..409e8b1 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -1,8 +1,27 @@ +import nock from 'nock' + process.env.MANAGER_URL = 'http://manager' -process.env.OIDC_JWKS_URI = - 'https://login.microsoftonline.com/770a2450-0227-4c62-90c7-4e38537f1102/discovery/v2.0/keys' +process.env.OIDC_JWKS_URI = 'https://oidc.test/.well-known/jwks.json' process.env.SHAREPOINT_TENANT_ID = '6f504113-6b64-43f2-ade9-242e05780007' process.env.SHAREPOINT_CLIENT_ID = 'dummy' process.env.SHAREPOINT_CLIENT_SECRET = 'dummy' process.env.SHAREPOINT_FORM_MAPPINGS = '{"mappings":[{"formId":"my-form-id","siteId":"my-site-id","listId":"my-list-id","status":"draft"}]}' + +const jwks = { + keys: [ + { + alg: 'RS256', + e: 'AQAB', + kid: '9tuAErwpIu41FajLxmC9+8Y7kMXa0kO3sY=', + kty: 'RSA', + n: 'q3DaFfvNA0C8wOaVsx-P68LqF4U5NzQuz9', + use: 'sig' + } + ] +} + +nock('https://oidc.test') + .persist() + .get('/.well-known/jwks.json') + .reply(200, jwks) diff --git a/package-lock.json b/package-lock.json index 3e4c59a..8156e5d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -66,6 +66,7 @@ "lint-staged": "^16.1.2", "luxon": "^3.6.1", "neostandard": "^0.12.2", + "nock": "14.0.12", "nunjucks": "3.2.4", "oidc-client-ts": "^3.2.1", "prettier": "^3.5.3", @@ -6065,6 +6066,24 @@ "sparse-bitfield": "^3.0.3" } }, + "node_modules/@mswjs/interceptors": { + "version": "0.41.3", + "resolved": "https://registry.npmjs.org/@mswjs/interceptors/-/interceptors-0.41.3.tgz", + "integrity": "sha512-cXu86tF4VQVfwz8W1SPbhoRyHJkti6mjH/XJIxp40jhO4j2k1m4KYrEykxqWPkFF3vrK4rgQppBh//AwyGSXPA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@open-draft/deferred-promise": "^2.2.0", + "@open-draft/logger": "^0.3.0", + "@open-draft/until": "^2.0.0", + "is-node-process": "^1.2.0", + "outvariant": "^1.4.3", + "strict-event-emitter": "^0.5.1" + }, + "engines": { + "node": ">=18" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "0.2.12", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-0.2.12.tgz", @@ -6094,6 +6113,31 @@ "node": ">=12.4.0" } }, + "node_modules/@open-draft/deferred-promise": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@open-draft/deferred-promise/-/deferred-promise-2.2.0.tgz", + "integrity": "sha512-CecwLWx3rhxVQF6V4bAgPS5t+So2sTbPgAzafKkVizyi7tlwpcFpdFqq+wqF2OwNBmqFuu6tOyouTuxgpMfzmA==", + "dev": true, + "license": "MIT" + }, + "node_modules/@open-draft/logger": { + "version": "0.3.0", + "resolved": "https://registry.npmjs.org/@open-draft/logger/-/logger-0.3.0.tgz", + "integrity": "sha512-X2g45fzhxH238HKO4xbSr7+wBS8Fvw6ixhTDuvLd5mqh6bJJCFAPwU9mPDxbcrRtfxv4u5IHCEH77BmxvXmmxQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "is-node-process": "^1.2.0", + "outvariant": "^1.4.0" + } + }, + "node_modules/@open-draft/until": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/@open-draft/until/-/until-2.1.0.tgz", + "integrity": "sha512-U69T3ItWHvLwGg5eJ0n3I62nWuE6ilHlmz7zM0npLBRvPRd7e6NYmg54vvRtP5mZG7kZqZCFVdsTWo7BPtBujg==", + "dev": true, + "license": "MIT" + }, "node_modules/@open-wc/dedupe-mixin": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/@open-wc/dedupe-mixin/-/dedupe-mixin-1.4.0.tgz", @@ -13010,6 +13054,13 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/is-node-process": { + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/is-node-process/-/is-node-process-1.2.0.tgz", + "integrity": "sha512-Vg4o6/fqPxIjtxgUH5QLJhwZ7gW5diGCVlXpuUfELC62CuxM1iHcRe51f2W1FDy04Ai4KJkagKjx3XaqyfRKXw==", + "dev": true, + "license": "MIT" + }, "node_modules/is-number": { "version": "7.0.0", "license": "MIT", @@ -16139,6 +16190,13 @@ "integrity": "sha512-3CNZ2DnrpByG9Nqj6Xo8vqbjT4F6N+tb4Gb28ESAZjYZ5yqvmc56J+/kuIwkaAMOyblTQhUW7PxMkUb8Q36N3Q==", "license": "MIT" }, + "node_modules/json-stringify-safe": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz", + "integrity": "sha512-ZClg6AaYvamvYEE82d3Iyd3vSSIjQ+odgjaTzRuO3s7toCdFKczob2i0zCh7JE8kWn17yvAWhUVxvqGwUalsRA==", + "dev": true, + "license": "ISC" + }, "node_modules/json5": { "version": "2.2.3", "dev": true, @@ -17404,6 +17462,21 @@ "path-to-regexp": "^8.1.0" } }, + "node_modules/nock": { + "version": "14.0.12", + "resolved": "https://registry.npmjs.org/nock/-/nock-14.0.12.tgz", + "integrity": "sha512-kZM3bHV0KzhHH6E2eRszHyML/w87AUzLBwupNTHohtYWP9fZYgUPmCbSKq6ITfEEmHqN4/p0MscvUipT4P5Qsg==", + "dev": true, + "license": "MIT", + "dependencies": { + "@mswjs/interceptors": "^0.41.0", + "json-stringify-safe": "^5.0.1", + "propagate": "^2.0.0" + }, + "engines": { + "node": ">=18.20.0 <20 || >=20.12.1" + } + }, "node_modules/node-exports-info": { "version": "1.6.0", "resolved": "https://registry.npmjs.org/node-exports-info/-/node-exports-info-1.6.0.tgz", @@ -17692,6 +17765,13 @@ "integrity": "sha512-KiOAIsdpUTcAXuykya5fnVVT+/5uS0Q1mrkRHcF89tpieSmY33O/tmc54CqwA+bfhbtEfZUNLHaPUiB9X3jt1A==", "license": "MIT" }, + "node_modules/outvariant": { + "version": "1.4.3", + "resolved": "https://registry.npmjs.org/outvariant/-/outvariant-1.4.3.tgz", + "integrity": "sha512-+Sl2UErvtsoajRDKCE5/dBz4DIvHXQQnAxtQTF04OJxY0+DyZXSo5P5Bb7XYWOh81syohlYL24hbDwxedPUJCA==", + "dev": true, + "license": "MIT" + }, "node_modules/own-keys": { "version": "1.0.1", "dev": true, @@ -18344,6 +18424,16 @@ "dev": true, "license": "MIT" }, + "node_modules/propagate": { + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/propagate/-/propagate-2.0.1.tgz", + "integrity": "sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">= 8" + } + }, "node_modules/protocol-buffers-schema": { "version": "3.6.0", "resolved": "https://registry.npmjs.org/protocol-buffers-schema/-/protocol-buffers-schema-3.6.0.tgz", @@ -19211,6 +19301,13 @@ "node": ">= 0.4" } }, + "node_modules/strict-event-emitter": { + "version": "0.5.1", + "resolved": "https://registry.npmjs.org/strict-event-emitter/-/strict-event-emitter-0.5.1.tgz", + "integrity": "sha512-vMgjE/GGEPEFnhFub6pa4FmJBRBVOLpIII2hvCZ8Kzb7K0hlHo7mQv6xYrBvCL2LtAIBwFUK8wvuJgTVSQ5MFQ==", + "dev": true, + "license": "MIT" + }, "node_modules/string-argv": { "version": "0.3.2", "dev": true, diff --git a/package.json b/package.json index 46fbc11..7a3d024 100644 --- a/package.json +++ b/package.json @@ -81,11 +81,12 @@ "eslint-plugin-jest": "^28.14.0", "eslint-plugin-jsdoc": "^51.0.1", "globals": "^17.3.0", - "neostandard": "^0.12.2", "husky": "^9.1.7", "jest": "29.7.0", "lint-staged": "^16.1.2", "luxon": "^3.6.1", + "neostandard": "^0.12.2", + "nock": "14.0.12", "nunjucks": "3.2.4", "oidc-client-ts": "^3.2.1", "prettier": "^3.5.3", From 5a80be6ede9069670b736dc552dbcf48a54b992c Mon Sep 17 00:00:00 2001 From: Mohammed Khalid Date: Tue, 14 Apr 2026 09:38:50 +0100 Subject: [PATCH 4/4] fix: rename shadowed body variable in getFormDefinition --- src/lib/manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/manager.js b/src/lib/manager.js index 36d581c..bde87cc 100644 --- a/src/lib/manager.js +++ b/src/lib/manager.js @@ -21,13 +21,13 @@ export async function getFormDefinition(formId, formStatus, versionNumber) { if (versionNumber !== undefined) { try { - const { body } = await getJson( + const { body: versionedBody } = await getJson( new URL( `/forms/${formId}/versions/${versionNumber}/definition`, managerUrl ) ) - return body + return versionedBody } catch (err) { logger.warn( err,