Dsfaap 2016 propagate return url

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<ResponseObject>
       },
-      {
-        href: '/test/full-name',
-
-        request: {
-          method: 'post',
-          payload: {
-            action: FormAction.Validate
-          },
-          query: { returnUrl: 'https://www.gov.uk/help/privacy-notice' }
-        } satisfies Partial<FormContextRequest>,
-
-        redirect: {
-          statusCode: StatusCodes.SEE_OTHER
-        } satisfies Partial<ResponseObject>
-      },
       {
         href: '/test/repeater/example',
 
@@ -212,13 +197,36 @@ describe('Helpers', () => {
           statusCode: StatusCodes.MOVED_TEMPORARILY
         } satisfies Partial<ResponseObject>
       }
-    ])(
-      "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)}`
+          )
+        )
       }
     )
   })

src/server/plugins/engine/helpers.ts

@@ -118,10 +118,11 @@ engine.registerFilter('answer', function (name: string) {
 export function proceed(
   request: Pick<FormContextRequest, 'method' | 'payload' | 'query'>,
   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'

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'
 
@@ -7,17 +8,31 @@ 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'
 import { redirectOrMakeHandler } from '~/src/server/plugins/engine/routes/index.js'
 import {
   type AnyFormRequest,
   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
@@ -47,7 +62,8 @@ describe('redirectOrMakeHandler', () => {
     getFormContext: jest.fn().mockReturnValue({
       isForceAccess: false,
       data: {}
-    })
+    }),
+    schemaVersion: SchemaVersion.V2
   } as unknown as FormModel
 
   const mockMakeHandler = jest
@@ -226,18 +242,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 +254,21 @@ describe('redirectOrMakeHandler', () => {
         mockMakeHandler
       )
 
-      expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href')
+      expect(proceed).toHaveBeenCalledWith(mockRequest, mockH, '/test-href', {})
       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 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'),
-        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(questionPageController)
 
       await redirectOrMakeHandler(
         mockRequest,
@@ -277,30 +277,40 @@ 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 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'),
-        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(terminalController)
+      await redirectOrMakeHandler(
+        mockRequest,
+        mockH,
+        undefined,
+        mockMakeHandler
+      )
+
+      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(questionPageController)
 
       await redirectOrMakeHandler(
         mockRequest,
@@ -309,9 +319,91 @@ 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 from summary and next pages exist', async () => {
+        const testPage = Object.assign({}, mockPage, {
+          path: '/summary',
+          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, {
+          path: '/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: [] })
+
+        await redirectOrMakeHandler(
+          mockRequest,
+          mockH,
+          undefined,
+          mockMakeHandler
+        )
+
+        expect(proceed).toHaveBeenCalledWith(
+          mockRequest,
+          mockH,
+          '/test-href',
+          {}
+        )
+      })
+
+      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',
+          {}
+        )
+      })
     })
   })
 })

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,19 @@ 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 (
+    page.path === summaryPath &&
+    ((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(