From 79385adc40384f06d3c9db27fe3308125383b313 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Wed, 3 Jun 2026 10:06:45 +0100 Subject: [PATCH 1/2] fix: Enhance validation in controllers and filters to handle missing dataset gracefully --- src/controllers/CheckAnswersController.js | 3 ++- src/controllers/datasetDetailsController.js | 3 ++- src/controllers/lpaDetailsController.js | 8 +++---- src/controllers/pageController.js | 10 ++++---- .../makeDatasetSlugToReadableNameFilter.js | 5 ++++ test/unit/PageController.test.js | 4 +++- test/unit/checkAnswersController.test.js | 14 +++++++++++ test/unit/lpaDetailsController.test.js | 24 +++++++++++++++++++ ...akeDatasetSlugToReadableNameFilter.test.js | 7 ++++++ 9 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index 2749fe033..4034f8068 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -10,7 +10,8 @@ import { getDatasets } from '../utils/utils.js' class CheckAnswersController extends PageController { async locals (req, res, next) { const requestId = req.sessionModel.get('requestId') - if (!requestId) { + const dataset = req.sessionModel.get('dataset') + if (!requestId || !dataset) { return res.redirect('/check/url') } try { diff --git a/src/controllers/datasetDetailsController.js b/src/controllers/datasetDetailsController.js index 7a39983ed..808ee3b75 100644 --- a/src/controllers/datasetDetailsController.js +++ b/src/controllers/datasetDetailsController.js @@ -3,7 +3,8 @@ import PageController from './pageController.js' class DatasetDetailsController extends PageController { locals (req, res, next) { const requestId = req.sessionModel.get('requestId') - if (!requestId) { + const dataset = req.sessionModel.get('dataset') + if (!requestId || !dataset) { return res.redirect('/check/url') } super.locals(req, res, next) diff --git a/src/controllers/lpaDetailsController.js b/src/controllers/lpaDetailsController.js index 85c4b6a31..e9453abfe 100644 --- a/src/controllers/lpaDetailsController.js +++ b/src/controllers/lpaDetailsController.js @@ -5,15 +5,15 @@ import { orgIdToName } from '../utils/orgIdToName.js' class LpaDetailsController extends PageController { async locals (req, res, next) { - const requestId = req.session?.checkRequestId + const requestId = req.session?.checkRequestId ?? req.sessionModel.get('request_id') try { const requestData = await getRequestData(requestId) - if (requestData?.getParams()?.type !== 'check_url') { + const params = requestData?.getParams() + if (params?.type !== 'check_url' || !params.dataset) { return res.redirect('/check/url') } // Populate submit wizard session from the check request params - const params = requestData.getParams() const orgId = params.organisationName req.sessionModel.set('requestId', requestId) req.sessionModel.set('lpa', orgIdToName(orgId)) @@ -33,7 +33,7 @@ class LpaDetailsController extends PageController { value: name })) - req.form.options.lastPage = `/check/results/${req.session.checkRequestId}/1` + req.form.options.lastPage = `/check/results/${requestId}/1` super.locals(req, res, next) } diff --git a/src/controllers/pageController.js b/src/controllers/pageController.js index 30ba1d39d..3f08c868d 100644 --- a/src/controllers/pageController.js +++ b/src/controllers/pageController.js @@ -79,10 +79,12 @@ class PageController extends Controller { } const dataset = req?.sessionModel?.get('dataset') - try { - req.form.options.datasetName = datasetSlugToReadableName(dataset) - } catch (e) { - logger.warn(`Failed to get readable dataset name from slug: ${dataset}`) + if (dataset) { + try { + req.form.options.datasetName = datasetSlugToReadableName(dataset) + } catch (e) { + logger.warn(`Failed to get readable dataset name from slug: ${dataset}`) + } } const errors = req?.sessionModel?.get('errors') diff --git a/src/filters/makeDatasetSlugToReadableNameFilter.js b/src/filters/makeDatasetSlugToReadableNameFilter.js index d99b3dcde..49def78b4 100644 --- a/src/filters/makeDatasetSlugToReadableNameFilter.js +++ b/src/filters/makeDatasetSlugToReadableNameFilter.js @@ -20,6 +20,11 @@ export const makeDatasetSlugToReadableNameFilter = (datasetNameMapping) => { const lowercaseFirst = (str) => str.charAt(0).toLowerCase() + str.slice(1) return (slug, capitalize = false) => { + if (!slug) { + logger.debug(`can't find a dataset name for ${slug}`) + return slug + } + const name = datasetNameMapping.get(slug) if (!name) { // ToDo: work out what to do here? potentially update it with data from datasette diff --git a/test/unit/PageController.test.js b/test/unit/PageController.test.js index 9a767ecba..b6076cc10 100644 --- a/test/unit/PageController.test.js +++ b/test/unit/PageController.test.js @@ -128,9 +128,11 @@ describe('PageController', () => { }) it('handles missing dataset gracefully', () => { - req.sessionModel.get.mockReturnValue({}) + vi.clearAllMocks() + req.sessionModel.get.mockReturnValue(undefined) pageController.locals(req, {}, vi.fn()) expect(req.form.options.datasetName).toBeUndefined() + expect(datasetSlugToReadableName.datasetSlugToReadableName).not.toHaveBeenCalled() }) it('handles datasetSlugToReadableName errors gracefully', () => { diff --git a/test/unit/checkAnswersController.test.js b/test/unit/checkAnswersController.test.js index a22734bf0..a0fc7f323 100644 --- a/test/unit/checkAnswersController.test.js +++ b/test/unit/checkAnswersController.test.js @@ -37,6 +37,20 @@ describe('CheckAnswersController', () => { vi.clearAllMocks() }) + describe('locals', () => { + it('should redirect to /check/url when dataset is missing', async () => { + req.sessionModel.get.mockImplementation(key => ({ + requestId: 'existing-request-id' + }[key])) + + await controller.locals(req, res, next) + + expect(res.redirect).toHaveBeenCalledWith('/check/url') + expect(getRequestData).not.toHaveBeenCalled() + expect(next).not.toHaveBeenCalled() + }) + }) + describe('POST to CheckAnswersController', () => { it('should create a Jira issue and set session data on success', async () => { const issue = { issueKey: 'TEST-123' } diff --git a/test/unit/lpaDetailsController.test.js b/test/unit/lpaDetailsController.test.js index ab8ad6a55..0485c2fbf 100644 --- a/test/unit/lpaDetailsController.test.js +++ b/test/unit/lpaDetailsController.test.js @@ -56,6 +56,19 @@ describe('lpaDetailsController', async () => { expect(req.sessionModel.set).toHaveBeenCalledWith('dataset', 'mock-dataset') }) + it('should use request_id from the session model when checkRequestId is not set', async () => { + req.session = {} + req.sessionModel.get.mockImplementation(key => ({ + request_id: 'session-model-request-id' + }[key])) + + await controller.locals(req, res, next) + + expect(getRequestData).toHaveBeenCalledWith('session-model-request-id') + expect(req.sessionModel.set).toHaveBeenCalledWith('requestId', 'session-model-request-id') + expect(req.form.options.lastPage).toEqual('/check/results/session-model-request-id/1') + }) + it('should set localAuthorities options in the form', async () => { fetchLocalAuthorities.fetchLocalAuthorities = vi.fn().mockResolvedValue(['Authority 1', 'Authority 2']) @@ -88,6 +101,17 @@ describe('lpaDetailsController', async () => { expect(next).not.toHaveBeenCalled() }) + it('should redirect to /check/url when request params do not include a dataset', async () => { + getRequestData.mockResolvedValue({ + getParams: () => ({ type: 'check_url', organisationName: 'Mock LPA' }) + }) + + await controller.locals(req, res, next) + + expect(res.redirect).toHaveBeenCalledWith('/check/url') + expect(next).not.toHaveBeenCalled() + }) + it('should redirect to /check/url when getRequestData throws', async () => { getRequestData.mockRejectedValue(new Error('API error')) diff --git a/test/unit/makeDatasetSlugToReadableNameFilter.test.js b/test/unit/makeDatasetSlugToReadableNameFilter.test.js index b97f164f7..11d4afac4 100644 --- a/test/unit/makeDatasetSlugToReadableNameFilter.test.js +++ b/test/unit/makeDatasetSlugToReadableNameFilter.test.js @@ -24,4 +24,11 @@ describe('makeDatasetSlugToReadableNameFilter', () => { expect(filter('Unknown-slug')).toBe('unknown-slug') expect(filter('Unknown-slug', true)).toBe('Unknown-slug') }) + + it('returns a missing slug without trying to capitalize it', () => { + expect(filter(undefined)).toBeUndefined() + expect(filter(undefined, true)).toBeUndefined() + expect(filter(null)).toBeNull() + expect(filter(null, true)).toBeNull() + }) }) From 729de0684f774ba4e55d4c40fc2801c6c7d5c253 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Wed, 3 Jun 2026 10:19:52 +0100 Subject: [PATCH 2/2] fix: Remove fallback to session model for requestId in LpaDetailsController --- src/controllers/lpaDetailsController.js | 2 +- test/unit/lpaDetailsController.test.js | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/controllers/lpaDetailsController.js b/src/controllers/lpaDetailsController.js index e9453abfe..cb488428f 100644 --- a/src/controllers/lpaDetailsController.js +++ b/src/controllers/lpaDetailsController.js @@ -5,7 +5,7 @@ import { orgIdToName } from '../utils/orgIdToName.js' class LpaDetailsController extends PageController { async locals (req, res, next) { - const requestId = req.session?.checkRequestId ?? req.sessionModel.get('request_id') + const requestId = req.session?.checkRequestId try { const requestData = await getRequestData(requestId) diff --git a/test/unit/lpaDetailsController.test.js b/test/unit/lpaDetailsController.test.js index 0485c2fbf..ea24903ac 100644 --- a/test/unit/lpaDetailsController.test.js +++ b/test/unit/lpaDetailsController.test.js @@ -56,19 +56,6 @@ describe('lpaDetailsController', async () => { expect(req.sessionModel.set).toHaveBeenCalledWith('dataset', 'mock-dataset') }) - it('should use request_id from the session model when checkRequestId is not set', async () => { - req.session = {} - req.sessionModel.get.mockImplementation(key => ({ - request_id: 'session-model-request-id' - }[key])) - - await controller.locals(req, res, next) - - expect(getRequestData).toHaveBeenCalledWith('session-model-request-id') - expect(req.sessionModel.set).toHaveBeenCalledWith('requestId', 'session-model-request-id') - expect(req.form.options.lastPage).toEqual('/check/results/session-model-request-id/1') - }) - it('should set localAuthorities options in the form', async () => { fetchLocalAuthorities.fetchLocalAuthorities = vi.fn().mockResolvedValue(['Authority 1', 'Authority 2'])