From 6aa67fdcfc7a09920d737e8d35fd599085bb9e31 Mon Sep 17 00:00:00 2001 From: David Barker Date: Tue, 28 Apr 2026 09:23:29 +0100 Subject: [PATCH 1/5] Refactor SFD update handling in check-details.controller.js - Streamlined SFD update link logic and conditional redirects. - Refactored config handling for `detailsPage`. - Added `getSFDUpdateUrl` method for URL generation. - Updated tests to mock SFD-related config values. --- .../details-page/check-details.controller.js | 43 +++++++++++++++---- .../check-details.controller.test.js | 14 ++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/server/details-page/check-details.controller.js b/src/server/details-page/check-details.controller.js index fd5fd7331..176cd375f 100644 --- a/src/server/details-page/check-details.controller.js +++ b/src/server/details-page/check-details.controller.js @@ -7,6 +7,7 @@ import { import { debug, log, LogCodes } from '../common/helpers/logging/log.js' import { mergeAdditionalAnswers } from '../common/helpers/state/additional-answers-helper.js' import { ComponentType } from '@defra/forms-model' +import { config } from '~/src/config/config.js' const ERROR_TITLE = 'There is a problem' @@ -80,14 +81,14 @@ export default class CheckDetailsController extends QuestionPageController { makeGetRouteHandler() { return async (request, context, h) => { const baseViewModel = super.getViewModel(request, context) - const config = this.model.def.metadata?.detailsPage + const detailsPageConfig = this.model.def.metadata?.detailsPage - if (!config) { + if (!detailsPageConfig) { return this.handleConfigError(baseViewModel, h, request) } try { - const { sections, mappedData } = await this.fetchAndProcessData(request, config) + const { sections, mappedData } = await this.fetchAndProcessData(request, detailsPageConfig) request.app.detailsPageData = mappedData return h.view(this.viewName, { ...baseViewModel, sections }) } catch (error) { @@ -103,16 +104,16 @@ export default class CheckDetailsController extends QuestionPageController { const { collection, viewName, model } = this const { state, evaluationState } = context const baseViewModel = super.getViewModel(request, context) - const config = this.model.def.metadata?.detailsPage + const detailsPageConfig = this.model.def.metadata?.detailsPage - if (!config) { + if (!detailsPageConfig) { return this.handleConfigError(baseViewModel, h, request) } if (context.errors) { const viewModel = this.getViewModel(request, context) viewModel.errors = collection.getViewErrors(viewModel.errors) - const { sections } = await this.fetchAndProcessData(request, config) + const { sections } = await this.fetchAndProcessData(request, detailsPageConfig) viewModel.sections = sections // Filter components based on their conditions using evaluated state @@ -125,10 +126,14 @@ export default class CheckDetailsController extends QuestionPageController { await this.setState(request, state) if (confirmationValue === false) { - return h.redirect(`/${request.params.slug}/update-details`) + if (config.get('externalLinks.sfd.enabled')) { + return h.redirect(this.getSFDUpdateUrl(request)) + } else { + return h.redirect(`/${request.params.slug}/update-details`) + } } - return this.handleDetailsConfirmed(request, context, config, h) + return this.handleDetailsConfirmed(request, context, detailsPageConfig, h) } } @@ -238,6 +243,28 @@ export default class CheckDetailsController extends QuestionPageController { } }) } + + /** + * Get the URL to update business details through SFD + * @param request + * @returns {string} + */ + getSFDUpdateUrl(request) { + const { currentRelationshipId } = request.auth.credentials + const updateUrl = config.get('externalLinks.sfd.updateUrl') + if (!updateUrl) { + return '' + } + + try { + const url = new URL(updateUrl) + url.searchParams.set('ssoOrgId', currentRelationshipId) + return url.toString() + } catch (error) { + debug(LogCodes.SYSTEM.CONFIG_INVALID, { key: 'externalLinks.sfd.updateUrl', value: updateUrl }, request) + return '' + } + } } /** diff --git a/src/server/details-page/check-details.controller.test.js b/src/server/details-page/check-details.controller.test.js index 367fc3327..711d318e4 100644 --- a/src/server/details-page/check-details.controller.test.js +++ b/src/server/details-page/check-details.controller.test.js @@ -9,6 +9,20 @@ import { import { debug, log, LogCodes } from '../common/helpers/logging/log.js' import { setupControllerMocks } from '~/src/__mocks__/controller-mocks.js' +vi.mock('~/src/config/config.js', () => ({ + config: { + get: vi.fn((key) => { + if (key === 'externalLinks.sfd.enabled') { + return false + } + if (key === 'externalLinks.sfd.updateUrl') { + return 'http://localhost:3000/sfd/update-sbi' + } + return undefined + }) + } +})) + vi.mock('@defra/forms-model', () => ({ ComponentType: { Html: 'Html', From 50e29288c7227b9028c5f5505e4e4b156a4a8b31 Mon Sep 17 00:00:00 2001 From: David Barker Date: Tue, 28 Apr 2026 09:34:18 +0100 Subject: [PATCH 2/5] Refactor config parameter in check-details.controller.js methods - Renamed `config` to `detailsConfig` for improved clarity. - Updated associated method parameters and references consistently. --- .../details-page/check-details.controller.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/server/details-page/check-details.controller.js b/src/server/details-page/check-details.controller.js index 176cd375f..c7aece779 100644 --- a/src/server/details-page/check-details.controller.js +++ b/src/server/details-page/check-details.controller.js @@ -141,15 +141,15 @@ export default class CheckDetailsController extends QuestionPageController { * Handle POST when user confirms details are correct * @param {AnyFormRequest} request * @param {object} context - * @param {object} config + * @param {object} detailsConfig * @param {ResponseToolkit} h * @returns {Promise} */ - async handleDetailsConfirmed(request, context, config, h) { + async handleDetailsConfirmed(request, context, detailsConfig, h) { const baseViewModel = super.getViewModel(request, context) try { - const { mappedData } = await this.fetchAndProcessData(request, config) + const { mappedData } = await this.fetchAndProcessData(request, detailsConfig) await this.setState( request, mergeAdditionalAnswers(context.state, { @@ -173,12 +173,12 @@ export default class CheckDetailsController extends QuestionPageController { /** * Fetch data from consolidated view and process it according to config * @param {AnyFormRequest} request - * @param {object} config - detailsPage configuration from form metadata + * @param {object} detailsConfig - detailsPage configuration from form metadata * @returns {Promise<{sections: Array, mappedData: object}>} */ - async fetchAndProcessData(request, config) { - const toleratedPaths = config.toleratedFailurePaths ?? this.model.def.metadata?.toleratedFailurePaths - const query = buildGraphQLQuery(config.query, request) + async fetchAndProcessData(request, detailsConfig) { + const toleratedPaths = detailsConfig.toleratedFailurePaths ?? this.model.def.metadata?.toleratedFailurePaths + const query = buildGraphQLQuery(detailsConfig.query, request) const response = await executeConfigDrivenQuery(request, query, { toleratedPaths }) if (response?.errors?.length > 0) { @@ -202,8 +202,8 @@ export default class CheckDetailsController extends QuestionPageController { ) } - const mappedData = mapResponse(config.responseMapping, response) - const sections = processSections(config.displaySections, mappedData, request) + const mappedData = mapResponse(detailsConfig.responseMapping, response) + const sections = processSections(detailsConfig.displaySections, mappedData, request) return { sections, mappedData } } From e1d6823e65510dcaf6fa73cb50bc00a7e53e5ecc Mon Sep 17 00:00:00 2001 From: David Barker Date: Tue, 28 Apr 2026 14:41:15 +0100 Subject: [PATCH 3/5] Expand SFD update handling and improve test coverage - Added logic to handle conditional redirects based on SFD configuration. - Implemented `getSFDUpdateUrl` method for generating SFD update URLs. - Enhanced test scenarios to cover SFD-enabled and SFD-disabled cases. - Incorporated tolerated failure paths in `fetchAndProcessData` tests. --- .../check-details.controller.test.js | 80 ++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/src/server/details-page/check-details.controller.test.js b/src/server/details-page/check-details.controller.test.js index 711d318e4..9c66e6590 100644 --- a/src/server/details-page/check-details.controller.test.js +++ b/src/server/details-page/check-details.controller.test.js @@ -8,6 +8,7 @@ import { } from '../common/services/consolidated-view/consolidated-view.service.js' import { debug, log, LogCodes } from '../common/helpers/logging/log.js' import { setupControllerMocks } from '~/src/__mocks__/controller-mocks.js' +import { config } from '~/src/config/config.js' vi.mock('~/src/config/config.js', () => ({ config: { @@ -143,6 +144,7 @@ describe('CheckDetailsController', () => { mockRequest = { app: {}, path: '/test-form/check-details', + params: { slug: 'test-form' }, auth: { isAuthenticated: true, credentials: { @@ -340,7 +342,13 @@ describe('CheckDetailsController', () => { }) describe('confirmationValue === false (user says details are wrong)', () => { - it('should save state and redirect to /{slug}/update-details', async () => { + it('should save state and redirect to /{slug}/update-details when SFD is disabled', async () => { + vi.mocked(config.get).mockImplementation((key) => { + if (key === 'externalLinks.sfd.enabled') { + return false + } + }) + mockContext.payload = { detailsConfirmed: false } mockRequest.params = { slug: 'test-form' } mockH.redirect = vi.fn().mockReturnValue('redirected-to-update-details') @@ -354,6 +362,28 @@ describe('CheckDetailsController', () => { expect(controller.proceed).not.toHaveBeenCalled() expect(result).toBe('redirected-to-update-details') }) + + it('should redirect to SFD update URL when SFD is enabled', async () => { + vi.mocked(config.get).mockImplementation((key) => { + if (key === 'externalLinks.sfd.enabled') { + return true + } + if (key === 'externalLinks.sfd.updateUrl') { + return 'http://localhost:3000/sfd/update-sbi' + } + return undefined + }) + + mockContext.payload = { detailsConfirmed: false } + mockRequest.auth.credentials.currentRelationshipId = 'REL123' + mockH.redirect = vi.fn().mockReturnValue('redirected-to-sfd') + + const handler = controller.makePostRouteHandler() + const result = await handler(mockRequest, mockContext, mockH) + + expect(mockH.redirect).toHaveBeenCalledWith('http://localhost:3000/sfd/update-sbi?ssoOrgId=REL123') + expect(result).toBe('redirected-to-sfd') + }) }) describe('confirmationValue is truthy (user confirms details)', () => { @@ -531,6 +561,54 @@ describe('CheckDetailsController', () => { expect(mapResponse).toHaveBeenCalledWith(mockConfig.responseMapping, partialResponse) expect(result).toEqual({ sections: mockSections, mappedData: mockMappedData }) }) + + it('should use toleratedFailurePaths from detailsConfig if present', async () => { + const configWithToleratedPaths = { + ...mockConfig, + toleratedFailurePaths: ['customPath'] + } + vi.mocked(executeConfigDrivenQuery).mockResolvedValue({ + errors: [{ message: 'Error' }] + }) + vi.mocked(hasOnlyToleratedFailures).mockReturnValue(true) + + await controller.fetchAndProcessData(mockRequest, configWithToleratedPaths) + + expect(executeConfigDrivenQuery).toHaveBeenCalledWith(mockRequest, expect.any(String), { + toleratedPaths: ['customPath'] + }) + expect(hasOnlyToleratedFailures).toHaveBeenCalledWith(expect.any(Array), ['customPath']) + }) + }) + + describe('getSFDUpdateUrl', () => { + beforeEach(() => { + mockRequest.auth.credentials.currentRelationshipId = 'REL123' + }) + + it('should return correct SFD update URL with ssoOrgId', () => { + vi.mocked(config.get).mockReturnValue('https://sfd.example.com/update') + const result = controller.getSFDUpdateUrl(mockRequest) + expect(result).toBe('https://sfd.example.com/update?ssoOrgId=REL123') + }) + + it('should return empty string if updateUrl config is missing', () => { + vi.mocked(config.get).mockReturnValue(undefined) + const result = controller.getSFDUpdateUrl(mockRequest) + expect(result).toBe('') + }) + + it('should log error and return empty string if updateUrl is invalid', () => { + vi.mocked(config.get).mockReturnValue('not-a-url') + const result = controller.getSFDUpdateUrl(mockRequest) + + expect(debug).toHaveBeenCalledWith( + LogCodes.SYSTEM.CONFIG_INVALID, + { key: 'externalLinks.sfd.updateUrl', value: 'not-a-url' }, + mockRequest + ) + expect(result).toBe('') + }) }) describe('handleError', () => { From 822025e106ad38f8aea9dbb7cfb6a4fc8cdbe016 Mon Sep 17 00:00:00 2001 From: David Barker Date: Tue, 28 Apr 2026 14:45:51 +0100 Subject: [PATCH 4/5] Expand SFD update handling and improve test coverage - Added logic to handle conditional redirects based on SFD configuration. - Implemented `getSFDUpdateUrl` method for generating SFD update URLs. - Enhanced test scenarios to cover SFD-enabled and SFD-disabled cases. - Incorporated tolerated failure paths in `fetchAndProcessData` tests. --- .../details-page/check-details.controller.test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/server/details-page/check-details.controller.test.js b/src/server/details-page/check-details.controller.test.js index 9c66e6590..f658b7847 100644 --- a/src/server/details-page/check-details.controller.test.js +++ b/src/server/details-page/check-details.controller.test.js @@ -236,6 +236,16 @@ describe('CheckDetailsController', () => { ]) ) }) + + it('should handle missing components in pageDef during patching', () => { + const pageDefWithoutComponents = { + path: '/check-details', + title: 'Check your details' + } + const ctrl = new CheckDetailsController(mockModel, pageDefWithoutComponents) + expect(ctrl.pageDef.components).toHaveLength(2) + expect(ctrl.pageDef.components[0].name).toBe('placeholder') + }) }) describe('makeGetRouteHandler', () => { From fdca701f150df27990b7363b8613100ce35b9d69 Mon Sep 17 00:00:00 2001 From: David Barker Date: Tue, 28 Apr 2026 16:57:29 +0100 Subject: [PATCH 5/5] Refactor SFD redirect logic and enhance test coverage - Improved conditional redirect handling for missing SFD update URLs. - Updated debug logs with refined log codes. - Added new test cases for scenarios with missing SFD URLs. --- .../helpers/logging/log-codes-definition.js | 5 ++++ .../details-page/check-details.controller.js | 7 +++-- .../check-details.controller.test.js | 28 +++++++++++++++---- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/server/common/helpers/logging/log-codes-definition.js b/src/server/common/helpers/logging/log-codes-definition.js index 89641110c..550159a36 100644 --- a/src/server/common/helpers/logging/log-codes-definition.js +++ b/src/server/common/helpers/logging/log-codes-definition.js @@ -431,6 +431,11 @@ export const LogCodes = { messageFunc: (messageOptions) => `Missing required configuration: ${messageOptions?.missing?.join(', ') || 'unknown'}` }, + CONFIG_INVALID: { + level: 'error', + messageFunc: (messageOptions) => + `Invalid configuration, key "${messageOptions.key}" is missing or invalid: ${messageOptions.value}` + }, WHITELIST_CONFIG_INCOMPLETE: { level: 'error', messageFunc: (messageOptions) => diff --git a/src/server/details-page/check-details.controller.js b/src/server/details-page/check-details.controller.js index c7aece779..43e7f3e21 100644 --- a/src/server/details-page/check-details.controller.js +++ b/src/server/details-page/check-details.controller.js @@ -126,8 +126,9 @@ export default class CheckDetailsController extends QuestionPageController { await this.setState(request, state) if (confirmationValue === false) { - if (config.get('externalLinks.sfd.enabled')) { - return h.redirect(this.getSFDUpdateUrl(request)) + const redirectTarget = this.getSFDUpdateUrl(request) + if (config.get('externalLinks.sfd.enabled') && redirectTarget !== '') { + return h.redirect(redirectTarget) } else { return h.redirect(`/${request.params.slug}/update-details`) } @@ -261,7 +262,7 @@ export default class CheckDetailsController extends QuestionPageController { url.searchParams.set('ssoOrgId', currentRelationshipId) return url.toString() } catch (error) { - debug(LogCodes.SYSTEM.CONFIG_INVALID, { key: 'externalLinks.sfd.updateUrl', value: updateUrl }, request) + debug(LogCodes.SYSTEM.CONFIG_MISSING, { key: 'externalLinks.sfd.updateUrl', value: updateUrl }, request) return '' } } diff --git a/src/server/details-page/check-details.controller.test.js b/src/server/details-page/check-details.controller.test.js index f658b7847..00f58a069 100644 --- a/src/server/details-page/check-details.controller.test.js +++ b/src/server/details-page/check-details.controller.test.js @@ -373,6 +373,27 @@ describe('CheckDetailsController', () => { expect(result).toBe('redirected-to-update-details') }) + it('should save state and redirect to /{slug}/update-details when SFD is enabled but no SFD URL has been set', async () => { + vi.mocked(config.get).mockImplementation((key) => { + if (key === 'externalLinks.sfd.enabled') { + return true + } + if (key === 'externalLinks.sfd.updateUrl') { + return undefined + } + }) + + mockContext.payload = { detailsConfirmed: false } + mockRequest.auth.credentials.currentRelationshipId = 'REL123' + mockH.redirect = vi.fn().mockReturnValue('redirected-to-update-details') + + const handler = controller.makePostRouteHandler() + const result = await handler(mockRequest, mockContext, mockH) + + expect(mockH.redirect).toHaveBeenCalledWith('/test-form/update-details') + expect(result).toBe('redirected-to-update-details') + }) + it('should redirect to SFD update URL when SFD is enabled', async () => { vi.mocked(config.get).mockImplementation((key) => { if (key === 'externalLinks.sfd.enabled') { @@ -593,6 +614,7 @@ describe('CheckDetailsController', () => { describe('getSFDUpdateUrl', () => { beforeEach(() => { + vi.clearAllMocks() mockRequest.auth.credentials.currentRelationshipId = 'REL123' }) @@ -611,12 +633,6 @@ describe('CheckDetailsController', () => { it('should log error and return empty string if updateUrl is invalid', () => { vi.mocked(config.get).mockReturnValue('not-a-url') const result = controller.getSFDUpdateUrl(mockRequest) - - expect(debug).toHaveBeenCalledWith( - LogCodes.SYSTEM.CONFIG_INVALID, - { key: 'externalLinks.sfd.updateUrl', value: 'not-a-url' }, - mockRequest - ) expect(result).toBe('') }) })