From bad6cf49bcfd36ec604b2fb623725f0ed02f9ea7 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 12 Nov 2025 11:09:54 +0000 Subject: [PATCH 01/13] Fixed returnUrl propagation for V2 schema --- src/server/plugins/engine/helpers.ts | 11 ++++++----- src/server/plugins/engine/routes/index.ts | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/server/plugins/engine/helpers.ts b/src/server/plugins/engine/helpers.ts index b57c893cb..516f8874f 100644 --- a/src/server/plugins/engine/helpers.ts +++ b/src/server/plugins/engine/helpers.ts @@ -118,10 +118,11 @@ engine.registerFilter('answer', function (name: string) { export function proceed( request: Pick, h: FormResponseToolkit, - nextUrl: string + nextUrl: string, + queryOverrides: FormQuery = {} ) { const { method, payload, query } = request - const { returnUrl } = query + const redirectQuery = { ...query, ...queryOverrides } const isReturnAllowed = payload && 'action' in payload @@ -131,9 +132,9 @@ export function proceed( // Redirect to return location (optional) const response = - isReturnAllowed && isPathRelative(returnUrl) - ? h.redirect(returnUrl) - : h.redirect(redirectPath(nextUrl)) + isReturnAllowed && isPathRelative(redirectQuery.returnUrl) + ? h.redirect(redirectQuery.returnUrl) + : h.redirect(redirectPath(nextUrl, redirectQuery)) // Redirect POST to GET to avoid resubmission return method === 'post' diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 9cb681c2c..82827dfba 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -1,3 +1,4 @@ +import { SchemaVersion } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, @@ -25,6 +26,7 @@ import { proceed } from '~/src/server/plugins/engine/helpers.js' import { FormModel } from '~/src/server/plugins/engine/models/index.js' +import { TerminalPageController } from '~/src/server/plugins/engine/pageControllers/TerminalPageController.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' import { generateUniqueReference } from '~/src/server/plugins/engine/referenceNumbers.js' import * as defaultServices from '~/src/server/plugins/engine/services/index.js' @@ -38,6 +40,7 @@ import { type PluginOptions } from '~/src/server/plugins/engine/types.js' import { + type FormQuery, type FormRequest, type FormResponseToolkit } from '~/src/server/routes/types.js' @@ -100,12 +103,18 @@ export async function redirectOrMakeHandler( // Redirect back to last relevant page const redirectTo = findPage(model, relevantPath) + const overrideQueryParams: FormQuery = {} + // Set the return URL unless an exit page - if (redirectTo?.next.length) { - request.query.returnUrl = page.getHref(summaryPath) + if ( + (model.schemaVersion === SchemaVersion.V1 && redirectTo?.next.length) || + (model.schemaVersion === SchemaVersion.V2 && + !(redirectTo instanceof TerminalPageController)) + ) { + overrideQueryParams.returnUrl = page.getHref(summaryPath) } - return proceed(request, h, page.getHref(relevantPath)) + return proceed(request, h, page.getHref(relevantPath), overrideQueryParams) } async function importExternalComponentState( From 79925d97a5710d62e57f9b72eb56556f1a020947 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 12 Nov 2025 11:10:53 +0000 Subject: [PATCH 02/13] Unit tests for updated files --- src/server/plugins/engine/helpers.test.ts | 46 +++--- .../plugins/engine/routes/index.test.ts | 134 ++++++++++++------ 2 files changed, 115 insertions(+), 65 deletions(-) diff --git a/src/server/plugins/engine/helpers.test.ts b/src/server/plugins/engine/helpers.test.ts index 6d8abc603..b5761a1b2 100644 --- a/src/server/plugins/engine/helpers.test.ts +++ b/src/server/plugins/engine/helpers.test.ts @@ -165,7 +165,7 @@ describe('Helpers', () => { } ) - it.each([ + const returnUrlTests = [ { href: '/test/full-name', @@ -178,21 +178,6 @@ describe('Helpers', () => { statusCode: StatusCodes.MOVED_TEMPORARILY } satisfies Partial }, - { - href: '/test/full-name', - - request: { - method: 'post', - payload: { - action: FormAction.Validate - }, - query: { returnUrl: 'https://www.gov.uk/help/privacy-notice' } - } satisfies Partial, - - redirect: { - statusCode: StatusCodes.SEE_OTHER - } satisfies Partial - }, { href: '/test/repeater/example', @@ -212,13 +197,36 @@ describe('Helpers', () => { statusCode: StatusCodes.MOVED_TEMPORARILY } satisfies Partial } - ])( - "should not redirect to the 'returnUrl' query param provided (other paths)", + ] + + it.each(returnUrlTests)( + 'should redirect to the path provided and add the returnUrl query param provided in the request', ({ href, ...options }) => { request = { ...request, ...options.request } proceed(request, h, href) - expect(h.redirect).not.toHaveBeenCalledWith(request.query.returnUrl) + expect(h.redirect).toHaveBeenCalledWith(expect.stringContaining(href)) + expect(h.redirect).toHaveBeenCalledWith( + expect.stringContaining( + `returnUrl=${encodeURIComponent(request.query.returnUrl ?? '')}` + ) + ) + } + ) + + it.each(returnUrlTests)( + 'should redirect to the path provided and add the returnUrl provided as an argument', + ({ href, ...options }) => { + const newReturnUrl = '/another-url' + request = { ...request, ...options.request } + + proceed(request, h, href, { returnUrl: newReturnUrl }) + expect(h.redirect).toHaveBeenCalledWith(expect.stringContaining(href)) + expect(h.redirect).toHaveBeenCalledWith( + expect.stringContaining( + `returnUrl=${encodeURIComponent(newReturnUrl)}` + ) + ) } ) }) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 45cb49128..a332faa2b 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -1,3 +1,4 @@ +import { SchemaVersion } from '@defra/forms-model' import Boom from '@hapi/boom' import { type ResponseObject, type ResponseToolkit } from '@hapi/hapi' @@ -8,7 +9,9 @@ import { proceed } from '~/src/server/plugins/engine/helpers.js' import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { TerminalPageController } from '~/src/server/plugins/engine/pageControllers/TerminalPageController.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' +import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/index.js' import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js' import { type AnyFormRequest, @@ -47,7 +50,8 @@ describe('redirectOrMakeHandler', () => { getFormContext: jest.fn().mockReturnValue({ isForceAccess: false, data: {} - }) + }), + schemaVersion: SchemaVersion.V2 } as unknown as FormModel const mockMakeHandler = jest @@ -226,18 +230,9 @@ describe('redirectOrMakeHandler', () => { }) it('should redirect when page is not relevant', async () => { - const testPage = { - getState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - mergeState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - getSummaryPath: jest.fn().mockReturnValue('/summary'), - getHref: jest.fn().mockReturnValue('/test-href'), - getRelevantPath: jest.fn().mockReturnValue('/other-path'), - path: '/test-path' - } as unknown as PageControllerClass + const testPage = Object.assign({}, mockPage, { + getRelevantPath: jest.fn().mockReturnValue('/other-path') + }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) await redirectOrMakeHandler( @@ -247,28 +242,27 @@ describe('redirectOrMakeHandler', () => { mockMakeHandler ) - expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') + expect(proceed).toHaveBeenCalledWith( + mockRequest, + mockH, + '/test-href', + expect.any(Object) + ) expect(mockMakeHandler).not.toHaveBeenCalled() }) - it('should set returnUrl when redirecting and next pages exist', async () => { - const testPage = { - getState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - mergeState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - getSummaryPath: jest.fn().mockReturnValue('/summary'), + it('should set returnUrl when redirecting and next page is not an exit page', async () => { + const testPage = Object.assign({}, mockPage, { getRelevantPath: jest.fn().mockReturnValue('/other-path'), - path: '/test-path', getHref: jest .fn() .mockReturnValueOnce('/summary-href') // First call: for summaryPath (returnUrl) .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) - } as unknown as PageControllerClass + }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) - ;(findPage as jest.Mock).mockReturnValue({ next: ['next-page'] }) + ;(findPage as jest.Mock).mockReturnValue( + Object.create(QuestionPageController.prototype) + ) await redirectOrMakeHandler( mockRequest, @@ -277,31 +271,23 @@ describe('redirectOrMakeHandler', () => { mockMakeHandler ) - expect(mockRequest.query.returnUrl).toBe('/summary-href') expect(proceed).toHaveBeenCalledWith( mockRequest, mockH, - '/relevant-path-href' + '/relevant-path-href', + { returnUrl: '/summary-href' } ) }) - it('should not set returnUrl when redirecting and no next pages exist', async () => { - const testPage = { - getState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - mergeState: jest - .fn() - .mockResolvedValue({ $$__referenceNumber: 'REF-123' }), - getSummaryPath: jest.fn().mockReturnValue('/summary'), + it('should not set returnUrl when redirecting and next page is an exit page', async () => { + const testPage = Object.assign({}, mockPage, { getHref: jest.fn().mockReturnValue('/test-href'), - getRelevantPath: jest.fn().mockReturnValue('/other-path'), - path: '/test-path' - } as unknown as PageControllerClass + getRelevantPath: jest.fn().mockReturnValue('/other-path') + }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) - const returnUrlBefore = mockRequest.query.returnUrl - ;(findPage as jest.Mock).mockReturnValue({ next: [] }) - + ;(findPage as jest.Mock).mockReturnValue( + Object.create(TerminalPageController.prototype) + ) await redirectOrMakeHandler( mockRequest, mockH, @@ -309,9 +295,65 @@ describe('redirectOrMakeHandler', () => { mockMakeHandler ) - // returnUrl should not be set if next pages don't exist - expect(mockRequest.query.returnUrl).toBe(returnUrlBefore) - expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href') + expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href', {}) + }) + + describe('when using v1 schema', () => { + beforeEach(() => { + const mockModelV1: FormModel = Object.assign({}, mockModel, { + schemaVersion: SchemaVersion.V1 + }) as unknown as FormModel + mockRequest.app = { model: mockModelV1 } + }) + + it('should set returnUrl when redirecting and next pages exist', async () => { + const testPage = Object.assign({}, mockPage, { + getRelevantPath: jest.fn().mockReturnValue('/other-path'), + getHref: jest + .fn() + .mockReturnValueOnce('/summary-href') // First call: for summaryPath (returnUrl) + .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) + }) as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) + ;(findPage as jest.Mock).mockReturnValue({ next: ['next-page'] }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(proceed).toHaveBeenCalledWith( + mockRequest, + mockH, + '/relevant-path-href', + { returnUrl: '/summary-href' } + ) + }) + + it('should not set returnUrl when redirecting and no next pages exist', async () => { + const testPage = Object.assign({}, mockPage, { + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/other-path') + }) as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) + ;(findPage as jest.Mock).mockReturnValue({ next: [] }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(proceed).toHaveBeenCalledWith( + mockRequest, + mockH, + '/test-href', + {} + ) + }) }) }) }) From 3c32246ac0d88471464c06593dbd67db9a2e62d6 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 12 Nov 2025 11:11:21 +0000 Subject: [PATCH 03/13] Updated existing journey tests --- test/form/exit-page.test.js | 5 +++- test/form/joined-conditions.test.js | 39 ++++++++++++++++++----------- test/form/journey-basic.test.js | 5 +++- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/test/form/exit-page.test.js b/test/form/exit-page.test.js index f37c76ca7..981db684b 100644 --- a/test/form/exit-page.test.js +++ b/test/form/exit-page.test.js @@ -10,6 +10,7 @@ import { renderResponse } from '~/test/helpers/component-helpers.js' import { getCookie, getCookieHeader } from '~/test/utils/get-cookie.js' const basePath = `${FORM_PREFIX}/demo-cph-number` +const returnUrlQueryString = `?returnUrl=%2Fdemo-cph-number%2Fsummary` jest.mock('~/src/server/utils/notify.ts') jest.mock('~/src/server/plugins/engine/services/formsService.js') @@ -199,7 +200,9 @@ describe('Exit pages', () => { // Redirect back to relevant page (with return URL) expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(response.headers.location).toBe(`${basePath}${paths.next}`) + expect(response.headers.location).toBe( + `${basePath}${paths.next}${returnUrlQueryString}` + ) }) } ) diff --git a/test/form/joined-conditions.test.js b/test/form/joined-conditions.test.js index fe136a4c8..a2d3f4d06 100644 --- a/test/form/joined-conditions.test.js +++ b/test/form/joined-conditions.test.js @@ -13,6 +13,7 @@ jest.mock('~/src/server/plugins/engine/services/formsService.js') describe('Joined conditions functional tests', () => { describe('Simple joined conditions (V2)', () => { const basePath = `${FORM_PREFIX}/joined-conditions-simple-v2` + const returnUrlQueryString = `?returnUrl=%2Fjoined-conditions-simple-v2%2Fsummary` /** @type {Server} */ let server @@ -48,7 +49,9 @@ describe('Joined conditions functional tests', () => { }) expect(res.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res.headers.location).toBe(`${basePath}/name`) + expect(res.headers.location).toBe( + `${basePath}/name${returnUrlQueryString}` + ) }) test('GET /simple-and-page without session redirects to /name', async () => { @@ -57,7 +60,9 @@ describe('Joined conditions functional tests', () => { }) expect(res.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res.headers.location).toBe(`${basePath}/name`) + expect(res.headers.location).toBe( + `${basePath}/name${returnUrlQueryString}` + ) }) }) @@ -98,9 +103,10 @@ describe('Joined conditions functional tests', () => { }) expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([`${basePath}/summary`, `${basePath}/status`]).toContain( - res2.headers.location - ) + expect([ + `${basePath}/summary`, + `${basePath}/status${returnUrlQueryString}` + ]).toContain(res2.headers.location) }) test('POST /name with "bob" (lowercase) skips /age', async () => { @@ -168,9 +174,10 @@ describe('Joined conditions functional tests', () => { }) expect(res3.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([`${basePath}/summary`, `${basePath}/status`]).toContain( - res3.headers.location - ) + expect([ + `${basePath}/summary`, + `${basePath}/status${returnUrlQueryString}` + ]).toContain(res3.headers.location) }) test('Alice (any age) cannot access /simple-and-page', async () => { @@ -188,9 +195,10 @@ describe('Joined conditions functional tests', () => { }) expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([`${basePath}/summary`, `${basePath}/status`]).toContain( - res2.headers.location - ) + expect([ + `${basePath}/summary`, + `${basePath}/status${returnUrlQueryString}` + ]).toContain(res2.headers.location) }) }) @@ -263,6 +271,7 @@ describe('Joined conditions functional tests', () => { describe('Complex joined conditions (V2)', () => { const basePath = `${FORM_PREFIX}/joined-conditions-complex-v2` + const returnUrlQueryString = `?returnUrl=%2Fjoined-conditions-complex-v2%2Fsummary` /** @type {Server} */ let server @@ -465,7 +474,9 @@ describe('Joined conditions functional tests', () => { }) expect(res6.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res6.headers.location).toBe(`${basePath}/are-you-over-18`) + expect(res6.headers.location).toBe( + `${basePath}/are-you-over-18${returnUrlQueryString}` + ) }) test('Alice born on 01/01/2001 who likes blue cannot access /complex-nested-page', async () => { @@ -488,8 +499,8 @@ describe('Joined conditions functional tests', () => { expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) expect([ `${basePath}/summary`, - `${basePath}/status`, - `${basePath}/what-is-your-dob` + `${basePath}/status${returnUrlQueryString}`, + `${basePath}/what-is-your-dob${returnUrlQueryString}` ]).toContain(res2.headers.location) }) }) diff --git a/test/form/journey-basic.test.js b/test/form/journey-basic.test.js index b1801ac3a..d668c757c 100644 --- a/test/form/journey-basic.test.js +++ b/test/form/journey-basic.test.js @@ -13,6 +13,7 @@ import { renderResponse } from '~/test/helpers/component-helpers.js' import { getCookie, getCookieHeader } from '~/test/utils/get-cookie.js' const basePath = `${FORM_PREFIX}/basic` +const returnUrlQueryString = `?returnUrl=%2Fbasic%2Fsummary` jest.mock('~/src/server/utils/notify.ts') jest.mock('~/src/server/plugins/engine/services/formsService.js') @@ -436,7 +437,9 @@ describe('Form journey', () => { // Redirect back to start expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(response.headers.location).toBe(`${basePath}/licence`) + expect(response.headers.location).toBe( + `${basePath}/licence${returnUrlQueryString}` + ) }) }) }) From 93a607a94832c757166420fe1298547fe777f1e9 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 12 Nov 2025 11:11:32 +0000 Subject: [PATCH 04/13] WIP returnUrl journey tests --- test/form/definitions/return-url.js | 152 ++++++++++++++++++++++++++ test/form/return-url.test.js | 161 ++++++++++++++++++++++++++++ 2 files changed, 313 insertions(+) create mode 100644 test/form/definitions/return-url.js create mode 100644 test/form/return-url.test.js diff --git a/test/form/definitions/return-url.js b/test/form/definitions/return-url.js new file mode 100644 index 000000000..466aa9fed --- /dev/null +++ b/test/form/definitions/return-url.js @@ -0,0 +1,152 @@ +import { + ComponentType, + ConditionType, + ControllerType, + Engine, + OperatorName +} from '@defra/forms-model' + +/** + * Pages: + * - /age (Are you over 18?) + * - IF NOT OVER 18 --> /unavailable-service-page (Unavailable service page - terminal) --> EXIT + * - IF OVER 18 --> /pizza (Do you like pizza?) + * - IF LIKES PIZZA --> /favourite-pizza (What is your favourite pizza?) + * - /favourite-food (What is your favourite food?) + * - /summary (Summary) + */ + +export default /** @satisfies {FormDefinition} */ ({ + name: 'Return URL tests', + engine: Engine.V2, + schema: 2, + startPage: '/age', + pages: [ + { + title: 'Are you over 18?', + path: '/age', + components: [ + { + type: ComponentType.YesNoField, + title: 'Are you over 18?', + name: 'isOverEighteen', + id: 'c977e76e-49ab-4443-b93e-e19e8d9c81ac', + options: { required: true } + } + ], + id: '7be18dec-0680-4c41-9981-357aa085429d', + next: [] + }, + { + title: 'Unavailable service page', + controller: ControllerType.Terminal, + path: '/unavailable-service-page', + components: [ + { + title: 'Unavailable service page', + name: 'serviceNotAvailable', + type: ComponentType.Markdown, + content: 'Unavailable service message content.', + options: {} + } + ], + next: [], + id: '53bd4fca-becb-4681-b91a-09132f3500bb', + condition: 'd1f9fcc7-f098-47e7-9d31-4f5ee57ba985' + }, + { + title: 'Do you like pizza?', + path: '/pizza', + components: [ + { + type: ComponentType.YesNoField, + title: 'Do you like pizza?', + name: 'likesPizza', + id: '48287b43-c54f-4084-86ad-00b2a979e78d', + options: { required: true } + } + ], + id: '12345678-90ab-cdef-1234-567890abcdef', + next: [] + }, + { + title: 'What is your favourite pizza?', + path: '/favourite-pizza', + components: [ + { + type: ComponentType.TextField, + title: 'What is your favourite pizza?', + name: 'favouritePizza', + shortDescription: 'favourite pizza', + options: { + required: true + }, + schema: {}, + id: 'dadadada-4838-4c28-a0ef-7cace4a11c0f' + } + ], + next: [], + id: 'efcd83c3-6ad4-4c54-8c39-ef87f79101ef', + condition: 'e53fa1ef-a101-4c8c-81f1-e78b466818d8' + }, + { + title: 'What is your favourite food?', + path: '/favourite-food', + components: [ + { + type: ComponentType.TextField, + title: 'What is your favourite food?', + name: 'favouriteFood', + shortDescription: 'favourite food', + options: { + required: true + }, + schema: {}, + id: 'bbefe85d-4838-4c28-a0ef-7cace4a11c0f' + } + ], + next: [], + id: 'adadadad-6ad4-4c54-8c39-ef87f79101ef' + }, + { + id: '449a45f6-4541-4a46-91bd-8b8931b07b50', + title: 'Summary', + path: '/summary', + controller: ControllerType.Summary + } + ], + conditions: [ + { + items: [ + { + id: 'c833b177-0cba-49de-b670-a297c6db45b8', + componentId: 'c977e76e-49ab-4443-b93e-e19e8d9c81ac', + operator: OperatorName.Is, + value: false, + type: ConditionType.BooleanValue + } + ], + displayName: 'is over 18', + id: 'd1f9fcc7-f098-47e7-9d31-4f5ee57ba985' + }, + { + items: [ + { + id: '48287b43-c54f-4084-86ad-00b2a979e78d', + componentId: '48287b43-c54f-4084-86ad-00b2a979e78d', + operator: OperatorName.Is, + value: true, + type: ConditionType.BooleanValue + } + ], + displayName: 'likes pizza', + id: 'e53fa1ef-a101-4c8c-81f1-e78b466818d8' + } + ], + sections: [], + lists: [] +}) + +/** + * @import { FormDefinition } from '@defra/forms-model' + */ diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js new file mode 100644 index 000000000..c90948c87 --- /dev/null +++ b/test/form/return-url.test.js @@ -0,0 +1,161 @@ +import { join } from 'node:path' + +import { StatusCodes } from 'http-status-codes' + +import { FORM_PREFIX } from '~/src/server/constants.js' +import { createServer } from '~/src/server/index.js' +import { getFormMetadata } from '~/src/server/plugins/engine/services/formsService.js' +import * as fixtures from '~/test/fixtures/index.js' +import { getCookie, getCookieHeader } from '~/test/utils/get-cookie.js' + +const basePath = `${FORM_PREFIX}/return-url` +const returnUrlQueryString = `?returnUrl=${encodeURIComponent('/return-url/summary')}` + +jest.mock('~/src/server/plugins/engine/services/formsService.js') + +describe('Return URL tests', () => { + /** @type {Server} */ + let server + + /** @type {string} */ + let csrfToken + + /** @type {ReturnType} */ + let headers + + // Create server before each test + beforeAll(async () => { + server = await createServer({ + formFileName: 'return-url.js', + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: true + }) + + await server.initialize() + + // Navigate to start + const response = await server.inject({ + url: `${basePath}/age` + }) + + // Extract the session cookie + csrfToken = getCookie(response, 'crumb') + headers = getCookieHeader(response, ['session', 'crumb']) + }) + + beforeEach(() => { + jest.mocked(getFormMetadata).mockResolvedValue(fixtures.form.metadata) + }) + + afterAll(async () => { + await server.stop() + }) + + describe('Return URL testing', () => { + it('should go to first invalid page and include returnUrl query string on mid-flow page load with empty state', async () => { + const response = await server.inject({ + url: `${basePath}/pizza` + }) + + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe( + `${basePath}/age${returnUrlQueryString}` + ) + }) + + // TODO see how to set the context or make POST work + + it('should go to first invalid page and include returnUrl query string on mid-flow page load without full state', async () => { + // SET CONTEXT with age but without pizza answer + const response = await server.inject({ + url: `${basePath}/favourite-pizza` + }) + + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe( + `${basePath}/pizza${returnUrlQueryString}` + ) + }) + + it('should go to first invalid AND relevant page and include returnUrl query string on mid-flow page load without full state', async () => { + // SET CONTEXT with age, with pizza anser as NO + const response = await server.inject({ + url: `${basePath}/favourite-pizza` + }) + + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe( + `${basePath}/favourite-food${returnUrlQueryString}` + ) + }) + + it('should go to page that changes flow and include returnUrl query string on next unanswered page', async () => { + // SET CONTEXT with age as yes, pizza answer as no, favourite food as something + const response = await server.inject({ + url: `${basePath}/pizza${returnUrlQueryString}` + }) + + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe( + `${basePath}/favourite-pizza${returnUrlQueryString}` + ) + }) + + it('should redirect to summary after POST with full state', async () => { + const payload = { + isOverEighteen: 'Some text', + likesPizza: false, + favouriteFood: 'Lasagna' + } + const response = await server.inject({ + url: `${basePath}/favourite-food`, + method: 'POST', + headers, + payload: { ...payload, crumb: csrfToken } + }) + + expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(response.headers.location).toBe(`${basePath}/summary`) + }) + + it('should redirect to exit page (without returnUrl) after POST with age not over 18', async () => { + const payload = { + // action: 'continue', + isOverEighteen: false + } + const response = await server.inject({ + url: `${basePath}/age${returnUrlQueryString}`, + method: 'POST', + headers, + payload: { ...payload, crumb: csrfToken } + }) + + expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) + expect(response.headers.location).toBe( + `${basePath}/unavailable-service-page` + ) + }) + + // it('should not redirect to summary after POST with full state', async () => { + // const payload = { + // isOverEighteen: 'Some text', + // likesPizza: false + // } + // const response = await server.inject({ + // url: `${basePath}/favourite-food${returnUrlQueryString}`, + // method: 'POST', + // headers, + // payload: { ...payload, crumb: csrfToken } + // }) + + // console.log(response.headers.location) + + // expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) + // expect(response.headers.location).not.toBe(`${basePath}/summary`) + // }) + }) +}) + +/** + * @import { Server } from '@hapi/hapi' + */ From 643bd56b6576b6479b128d1bdec4da9553f6d974 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 15:17:42 +0000 Subject: [PATCH 05/13] fixing journey tests WIP --- test/form/return-url.test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js index c90948c87..b71535e03 100644 --- a/test/form/return-url.test.js +++ b/test/form/return-url.test.js @@ -67,6 +67,17 @@ describe('Return URL tests', () => { it('should go to first invalid page and include returnUrl query string on mid-flow page load without full state', async () => { // SET CONTEXT with age but without pizza answer + const payload = { + action: true, + isOverEighteen: true + } + await server.inject({ + url: `${basePath}/age`, + method: 'POST', + headers, + payload: { ...payload, crumb: csrfToken } + }) + const response = await server.inject({ url: `${basePath}/favourite-pizza` }) From a20469868e0946d031c83807fd2e52add4895b5a Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 14 Nov 2025 15:31:49 +0000 Subject: [PATCH 06/13] Journey persist --- test/form/return-url.test.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js index b71535e03..118599050 100644 --- a/test/form/return-url.test.js +++ b/test/form/return-url.test.js @@ -68,24 +68,24 @@ describe('Return URL tests', () => { it('should go to first invalid page and include returnUrl query string on mid-flow page load without full state', async () => { // SET CONTEXT with age but without pizza answer const payload = { - action: true, isOverEighteen: true } - await server.inject({ + const response1 = await server.inject({ url: `${basePath}/age`, method: 'POST', headers, payload: { ...payload, crumb: csrfToken } }) - const response = await server.inject({ - url: `${basePath}/favourite-pizza` + expect(response1.headers.location).toBe(`${basePath}/pizza`) + + const response2 = await server.inject({ + method: 'GET', + url: `${basePath}/pizza`, + headers }) - expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(response.headers.location).toBe( - `${basePath}/pizza${returnUrlQueryString}` - ) + expect(response2.statusCode).toBe(StatusCodes.OK) }) it('should go to first invalid AND relevant page and include returnUrl query string on mid-flow page load without full state', async () => { From 52c4c83a4e4dd7e3df5baa8d403b7f4619dab979 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 15:35:56 +0000 Subject: [PATCH 07/13] fixing journey tests WIP --- test/form/return-url.test.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js index 118599050..90a71c001 100644 --- a/test/form/return-url.test.js +++ b/test/form/return-url.test.js @@ -81,11 +81,14 @@ describe('Return URL tests', () => { const response2 = await server.inject({ method: 'GET', - url: `${basePath}/pizza`, + url: `${basePath}/favourite-pizza`, headers }) - expect(response2.statusCode).toBe(StatusCodes.OK) + expect(response2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response2.headers.location).toBe( + `${basePath}/pizza${returnUrlQueryString}` + ) }) it('should go to first invalid AND relevant page and include returnUrl query string on mid-flow page load without full state', async () => { From a8574b5e75eb54a2e390eeea1b505ed350f458a3 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 17:10:58 +0000 Subject: [PATCH 08/13] more updates --- test/form/return-url.test.js | 40 ++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js index 90a71c001..42ca41618 100644 --- a/test/form/return-url.test.js +++ b/test/form/return-url.test.js @@ -63,38 +63,56 @@ describe('Return URL tests', () => { ) }) - // TODO see how to set the context or make POST work - it('should go to first invalid page and include returnUrl query string on mid-flow page load without full state', async () => { - // SET CONTEXT with age but without pizza answer + // set context with age but without pizza answer const payload = { isOverEighteen: true } - const response1 = await server.inject({ + await server.inject({ url: `${basePath}/age`, method: 'POST', headers, payload: { ...payload, crumb: csrfToken } }) - expect(response1.headers.location).toBe(`${basePath}/pizza`) - - const response2 = await server.inject({ + // trying to load favourite pizza page without pizza answer in context + const response = await server.inject({ method: 'GET', url: `${basePath}/favourite-pizza`, headers }) - expect(response2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(response2.headers.location).toBe( + expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) + expect(response.headers.location).toBe( `${basePath}/pizza${returnUrlQueryString}` ) }) it('should go to first invalid AND relevant page and include returnUrl query string on mid-flow page load without full state', async () => { - // SET CONTEXT with age, with pizza anser as NO + // set context with age, with pizza answer as no + const agePayload = { + isOverEighteen: true + } + await server.inject({ + url: `${basePath}/age`, + method: 'POST', + headers, + payload: { ...agePayload, crumb: csrfToken } + }) + const pizzaPayload = { + likesPizza: false + } + await server.inject({ + url: `${basePath}/pizza`, + method: 'POST', + headers, + payload: { ...pizzaPayload, crumb: csrfToken } + }) + const response = await server.inject({ - url: `${basePath}/favourite-pizza` + method: 'GET', + url: `${basePath}/favourite-pizza`, + headers }) expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) From 8bfb5ec16a246f4498442749d52c73822d72b094 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 18:37:24 +0000 Subject: [PATCH 09/13] Checking that path is summary before adding the returnUrl --- .../plugins/engine/routes/index.test.ts | 55 ++++++++++++++++++- src/server/plugins/engine/routes/index.ts | 7 ++- src/server/plugins/nunjucks/filters/href.js | 2 +- test/form/joined-conditions.test.js | 39 +++++-------- 4 files changed, 71 insertions(+), 32 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index a332faa2b..709f5b4fe 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -251,8 +251,9 @@ describe('redirectOrMakeHandler', () => { expect(mockMakeHandler).not.toHaveBeenCalled() }) - it('should set returnUrl when redirecting and next page is not an exit page', async () => { + it('should set returnUrl when redirecting from summary page and next page is not an exit page', async () => { const testPage = Object.assign({}, mockPage, { + path: '/summary', getRelevantPath: jest.fn().mockReturnValue('/other-path'), getHref: jest .fn() @@ -279,8 +280,9 @@ describe('redirectOrMakeHandler', () => { ) }) - it('should not set returnUrl when redirecting and next page is an exit page', async () => { + it('should not set returnUrl when redirecting from summary and next page is an exit page', async () => { const testPage = Object.assign({}, mockPage, { + path: '/summary', getHref: jest.fn().mockReturnValue('/test-href'), getRelevantPath: jest.fn().mockReturnValue('/other-path') }) as unknown as PageControllerClass @@ -298,6 +300,27 @@ describe('redirectOrMakeHandler', () => { expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href', {}) }) + it('should not set returnUrl when redirecting from a page that is not summary', async () => { + const testPage = Object.assign({}, mockPage, { + path: '/not-summary', + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/other-path') + }) as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) + ;(findPage as jest.Mock).mockReturnValue( + Object.create(QuestionPageController.prototype) + ) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href', {}) + }) + describe('when using v1 schema', () => { beforeEach(() => { const mockModelV1: FormModel = Object.assign({}, mockModel, { @@ -306,8 +329,9 @@ describe('redirectOrMakeHandler', () => { mockRequest.app = { model: mockModelV1 } }) - it('should set returnUrl when redirecting and next pages exist', async () => { + it('should set returnUrl when redirecting from summary and next pages exist', async () => { const testPage = Object.assign({}, mockPage, { + path: '/summary', getRelevantPath: jest.fn().mockReturnValue('/other-path'), getHref: jest .fn() @@ -334,6 +358,7 @@ describe('redirectOrMakeHandler', () => { it('should not set returnUrl when redirecting and no next pages exist', async () => { const testPage = Object.assign({}, mockPage, { + path: '/summary', getHref: jest.fn().mockReturnValue('/test-href'), getRelevantPath: jest.fn().mockReturnValue('/other-path') }) as unknown as PageControllerClass @@ -354,6 +379,30 @@ describe('redirectOrMakeHandler', () => { {} ) }) + + it('should not set returnUrl when not redirecting from summary', async () => { + const testPage = Object.assign({}, mockPage, { + path: '/not-summary', + getHref: jest.fn().mockReturnValue('/test-href'), + getRelevantPath: jest.fn().mockReturnValue('/other-path') + }) as unknown as PageControllerClass + ;(getPage as jest.Mock).mockReturnValue(testPage) + ;(findPage as jest.Mock).mockReturnValue({ next: ['next-page'] }) + + await redirectOrMakeHandler( + mockRequest, + mockH, + undefined, + mockMakeHandler + ) + + expect(proceed).toHaveBeenCalledWith( + mockRequest, + mockH, + '/test-href', + {} + ) + }) }) }) }) diff --git a/src/server/plugins/engine/routes/index.ts b/src/server/plugins/engine/routes/index.ts index 82827dfba..6ecce3347 100644 --- a/src/server/plugins/engine/routes/index.ts +++ b/src/server/plugins/engine/routes/index.ts @@ -107,9 +107,10 @@ export async function redirectOrMakeHandler( // Set the return URL unless an exit page if ( - (model.schemaVersion === SchemaVersion.V1 && redirectTo?.next.length) || - (model.schemaVersion === SchemaVersion.V2 && - !(redirectTo instanceof TerminalPageController)) + page.path === summaryPath && + ((model.schemaVersion === SchemaVersion.V1 && redirectTo?.next.length) || + (model.schemaVersion === SchemaVersion.V2 && + !(redirectTo instanceof TerminalPageController))) ) { overrideQueryParams.returnUrl = page.getHref(summaryPath) } diff --git a/src/server/plugins/nunjucks/filters/href.js b/src/server/plugins/nunjucks/filters/href.js index 61d0ed754..d904d266b 100644 --- a/src/server/plugins/nunjucks/filters/href.js +++ b/src/server/plugins/nunjucks/filters/href.js @@ -1,4 +1,4 @@ -import { getPageHref } from '~/src/server/plugins/engine/index.js' +import { getPageHref } from '~/src/server/plugins/engine/helpers.js' /** * Nunjucks filter to get the answer for a component diff --git a/test/form/joined-conditions.test.js b/test/form/joined-conditions.test.js index a2d3f4d06..fe136a4c8 100644 --- a/test/form/joined-conditions.test.js +++ b/test/form/joined-conditions.test.js @@ -13,7 +13,6 @@ jest.mock('~/src/server/plugins/engine/services/formsService.js') describe('Joined conditions functional tests', () => { describe('Simple joined conditions (V2)', () => { const basePath = `${FORM_PREFIX}/joined-conditions-simple-v2` - const returnUrlQueryString = `?returnUrl=%2Fjoined-conditions-simple-v2%2Fsummary` /** @type {Server} */ let server @@ -49,9 +48,7 @@ describe('Joined conditions functional tests', () => { }) expect(res.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res.headers.location).toBe( - `${basePath}/name${returnUrlQueryString}` - ) + expect(res.headers.location).toBe(`${basePath}/name`) }) test('GET /simple-and-page without session redirects to /name', async () => { @@ -60,9 +57,7 @@ describe('Joined conditions functional tests', () => { }) expect(res.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res.headers.location).toBe( - `${basePath}/name${returnUrlQueryString}` - ) + expect(res.headers.location).toBe(`${basePath}/name`) }) }) @@ -103,10 +98,9 @@ describe('Joined conditions functional tests', () => { }) expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([ - `${basePath}/summary`, - `${basePath}/status${returnUrlQueryString}` - ]).toContain(res2.headers.location) + expect([`${basePath}/summary`, `${basePath}/status`]).toContain( + res2.headers.location + ) }) test('POST /name with "bob" (lowercase) skips /age', async () => { @@ -174,10 +168,9 @@ describe('Joined conditions functional tests', () => { }) expect(res3.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([ - `${basePath}/summary`, - `${basePath}/status${returnUrlQueryString}` - ]).toContain(res3.headers.location) + expect([`${basePath}/summary`, `${basePath}/status`]).toContain( + res3.headers.location + ) }) test('Alice (any age) cannot access /simple-and-page', async () => { @@ -195,10 +188,9 @@ describe('Joined conditions functional tests', () => { }) expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect([ - `${basePath}/summary`, - `${basePath}/status${returnUrlQueryString}` - ]).toContain(res2.headers.location) + expect([`${basePath}/summary`, `${basePath}/status`]).toContain( + res2.headers.location + ) }) }) @@ -271,7 +263,6 @@ describe('Joined conditions functional tests', () => { describe('Complex joined conditions (V2)', () => { const basePath = `${FORM_PREFIX}/joined-conditions-complex-v2` - const returnUrlQueryString = `?returnUrl=%2Fjoined-conditions-complex-v2%2Fsummary` /** @type {Server} */ let server @@ -474,9 +465,7 @@ describe('Joined conditions functional tests', () => { }) expect(res6.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) - expect(res6.headers.location).toBe( - `${basePath}/are-you-over-18${returnUrlQueryString}` - ) + expect(res6.headers.location).toBe(`${basePath}/are-you-over-18`) }) test('Alice born on 01/01/2001 who likes blue cannot access /complex-nested-page', async () => { @@ -499,8 +488,8 @@ describe('Joined conditions functional tests', () => { expect(res2.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) expect([ `${basePath}/summary`, - `${basePath}/status${returnUrlQueryString}`, - `${basePath}/what-is-your-dob${returnUrlQueryString}` + `${basePath}/status`, + `${basePath}/what-is-your-dob` ]).toContain(res2.headers.location) }) }) From da658dc0f97bdfef630385cc97f52aa00d7feed9 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 19:05:51 +0000 Subject: [PATCH 10/13] Return url journey tests --- test/form/return-url.test.js | 89 ++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/test/form/return-url.test.js b/test/form/return-url.test.js index 42ca41618..fdaf54b70 100644 --- a/test/form/return-url.test.js +++ b/test/form/return-url.test.js @@ -52,9 +52,9 @@ describe('Return URL tests', () => { }) describe('Return URL testing', () => { - it('should go to first invalid page and include returnUrl query string on mid-flow page load with empty state', async () => { + it('should go to first invalid page and include returnUrl when loading summary page with empty state', async () => { const response = await server.inject({ - url: `${basePath}/pizza` + url: `${basePath}/summary` }) expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) @@ -63,7 +63,7 @@ describe('Return URL tests', () => { ) }) - it('should go to first invalid page and include returnUrl query string on mid-flow page load without full state', async () => { + it('should go to first invalid page and include returnUrl when loading summary page without full state', async () => { // set context with age but without pizza answer const payload = { isOverEighteen: true @@ -78,7 +78,7 @@ describe('Return URL tests', () => { // trying to load favourite pizza page without pizza answer in context const response = await server.inject({ method: 'GET', - url: `${basePath}/favourite-pizza`, + url: `${basePath}/summary`, headers }) @@ -88,7 +88,7 @@ describe('Return URL tests', () => { ) }) - it('should go to first invalid AND relevant page and include returnUrl query string on mid-flow page load without full state', async () => { + it('should go to first invalid AND relevant page and include returnUrl when loading summary page without full state', async () => { // set context with age, with pizza answer as no const agePayload = { isOverEighteen: true @@ -111,7 +111,7 @@ describe('Return URL tests', () => { const response = await server.inject({ method: 'GET', - url: `${basePath}/favourite-pizza`, + url: `${basePath}/summary`, headers }) @@ -121,70 +121,61 @@ describe('Return URL tests', () => { ) }) - it('should go to page that changes flow and include returnUrl query string on next unanswered page', async () => { - // SET CONTEXT with age as yes, pizza answer as no, favourite food as something + it('should redirect to exit page (without returnUrl) when loading summary page with age not over 18', async () => { + const payload = { + isOverEighteen: false + } + await server.inject({ + url: `${basePath}/age`, + method: 'POST', + headers, + payload: { ...payload, crumb: csrfToken } + }) + const response = await server.inject({ - url: `${basePath}/pizza${returnUrlQueryString}` + method: 'GET', + url: `${basePath}/summary`, + headers }) expect(response.statusCode).toBe(StatusCodes.MOVED_TEMPORARILY) expect(response.headers.location).toBe( - `${basePath}/favourite-pizza${returnUrlQueryString}` + `${basePath}/unavailable-service-page` ) }) it('should redirect to summary after POST with full state', async () => { - const payload = { - isOverEighteen: 'Some text', - likesPizza: false, - favouriteFood: 'Lasagna' + const agePayload = { + isOverEighteen: true } - const response = await server.inject({ - url: `${basePath}/favourite-food`, + await server.inject({ + url: `${basePath}/age`, method: 'POST', headers, - payload: { ...payload, crumb: csrfToken } + payload: { ...agePayload, crumb: csrfToken } }) - - expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) - expect(response.headers.location).toBe(`${basePath}/summary`) - }) - - it('should redirect to exit page (without returnUrl) after POST with age not over 18', async () => { - const payload = { - // action: 'continue', - isOverEighteen: false + const pizzaPayload = { + likesPizza: false + } + await server.inject({ + url: `${basePath}/pizza`, + method: 'POST', + headers, + payload: { ...pizzaPayload, crumb: csrfToken } + }) + const foodPayload = { + favouriteFood: 'Lasagna' } const response = await server.inject({ - url: `${basePath}/age${returnUrlQueryString}`, + url: `${basePath}/favourite-food`, method: 'POST', headers, - payload: { ...payload, crumb: csrfToken } + payload: { ...foodPayload, crumb: csrfToken } }) expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) - expect(response.headers.location).toBe( - `${basePath}/unavailable-service-page` - ) + expect(response.headers.location).toBe(`${basePath}/summary`) }) - - // it('should not redirect to summary after POST with full state', async () => { - // const payload = { - // isOverEighteen: 'Some text', - // likesPizza: false - // } - // const response = await server.inject({ - // url: `${basePath}/favourite-food${returnUrlQueryString}`, - // method: 'POST', - // headers, - // payload: { ...payload, crumb: csrfToken } - // }) - - // console.log(response.headers.location) - - // expect(response.statusCode).toBe(StatusCodes.SEE_OTHER) - // expect(response.headers.location).not.toBe(`${basePath}/summary`) - // }) }) }) From 84cc1f90afb67034130a00fa598db2a9f3154298 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 19:08:48 +0000 Subject: [PATCH 11/13] Undo href change --- src/server/plugins/nunjucks/filters/href.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/plugins/nunjucks/filters/href.js b/src/server/plugins/nunjucks/filters/href.js index d904d266b..61d0ed754 100644 --- a/src/server/plugins/nunjucks/filters/href.js +++ b/src/server/plugins/nunjucks/filters/href.js @@ -1,4 +1,4 @@ -import { getPageHref } from '~/src/server/plugins/engine/helpers.js' +import { getPageHref } from '~/src/server/plugins/engine/index.js' /** * Nunjucks filter to get the answer for a component From 794ae8a4f4a0934b0c32a045eaf6a48f569f8818 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Fri, 14 Nov 2025 20:15:47 +0000 Subject: [PATCH 12/13] Fixed tests --- .../plugins/engine/routes/index.test.ts | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index 709f5b4fe..c4f49073b 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -8,7 +8,7 @@ import { getPage, proceed } from '~/src/server/plugins/engine/helpers.js' -import { type FormModel } from '~/src/server/plugins/engine/models/FormModel.js' +import { FormModel } from '~/src/server/plugins/engine/models/FormModel.js' import { TerminalPageController } from '~/src/server/plugins/engine/pageControllers/TerminalPageController.js' import { type PageControllerClass } from '~/src/server/plugins/engine/pageControllers/helpers/pages.js' import { QuestionPageController } from '~/src/server/plugins/engine/pageControllers/index.js' @@ -18,9 +18,21 @@ import { type OnRequestCallback } from '~/src/server/plugins/engine/types.js' import { type FormResponseToolkit } from '~/src/server/routes/types.js' +import definition from '~/test/form/definitions/basic.js' jest.mock('~/src/server/plugins/engine/helpers') +const page = definition.pages[0] +const model: FormModel = new FormModel(definition, { + basePath: 'test' +}) +const terminalController: TerminalPageController = new TerminalPageController( + model, + page +) +const questionPageController: QuestionPageController = + new QuestionPageController(model, page) + describe('redirectOrMakeHandler', () => { const mockServer = {} as unknown as Parameters< typeof redirectOrMakeHandler @@ -261,9 +273,7 @@ describe('redirectOrMakeHandler', () => { .mockReturnValueOnce('/relevant-path-href') // Second call: for relevantPath (redirect) }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) - ;(findPage as jest.Mock).mockReturnValue( - Object.create(QuestionPageController.prototype) - ) + ;(findPage as jest.Mock).mockReturnValue(questionPageController) await redirectOrMakeHandler( mockRequest, @@ -287,9 +297,7 @@ describe('redirectOrMakeHandler', () => { getRelevantPath: jest.fn().mockReturnValue('/other-path') }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) - ;(findPage as jest.Mock).mockReturnValue( - Object.create(TerminalPageController.prototype) - ) + ;(findPage as jest.Mock).mockReturnValue(terminalController) await redirectOrMakeHandler( mockRequest, mockH, @@ -307,9 +315,7 @@ describe('redirectOrMakeHandler', () => { getRelevantPath: jest.fn().mockReturnValue('/other-path') }) as unknown as PageControllerClass ;(getPage as jest.Mock).mockReturnValue(testPage) - ;(findPage as jest.Mock).mockReturnValue( - Object.create(QuestionPageController.prototype) - ) + ;(findPage as jest.Mock).mockReturnValue(questionPageController) await redirectOrMakeHandler( mockRequest, From 6788a684e471fae60df9c1ecd8f17b23a23139af Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 18 Nov 2025 15:01:21 +0000 Subject: [PATCH 13/13] Updated test --- src/server/plugins/engine/routes/index.test.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/server/plugins/engine/routes/index.test.ts b/src/server/plugins/engine/routes/index.test.ts index c4f49073b..56943a401 100644 --- a/src/server/plugins/engine/routes/index.test.ts +++ b/src/server/plugins/engine/routes/index.test.ts @@ -254,12 +254,7 @@ describe('redirectOrMakeHandler', () => { mockMakeHandler ) - expect(proceed).toHaveBeenCalledWith( - mockRequest, - mockH, - '/test-href', - expect.any(Object) - ) + expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href', {}) expect(mockMakeHandler).not.toHaveBeenCalled() })