refactor: improve column mapping request handling and add tests for m…

src/controllers/columnMappingController.js

@@ -6,6 +6,7 @@ import { processSpecificationMiddlewares } from '../middleware/common.middleware
 import platformApi from '../services/platformApi.js'
 import { types } from '../utils/logging.js'
 import logger from '../utils/logger.js'
+import { MiddlewareError } from '../utils/errors.js'
 
 const LOCK_DETECTED_GEOMETRY_MAPPINGS = true
 const GEOMETRY_FIELDS = ['geometry', 'point']
@@ -14,14 +15,7 @@ class ColumnMappingController extends PageController {
   middlewareSetup () {
     super.middlewareSetup()
     this.use(getRequestDataMiddleware)
-    this.use(async (req, res, next) => {
-      const { requestData } = req.locals
-      const params = requestData.getParams() ?? {}
-      if (await isStatutoryDataset(params.organisationName, params.dataset)) {
-        return res.status(404).render('errors/404.html')
-      }
-      next()
-    })
+    this.use(handleUnavailableColumnMappingRequest)
     this.use(updateSessionFromRequestData)
     // Populate req.params and dataset, then run specification processing middlewares
     this.use(async (req, res, next) => {
@@ -121,6 +115,25 @@ class ColumnMappingController extends PageController {
   }
 }
 
+export async function handleUnavailableColumnMappingRequest (req, res, next) {
+  const { requestData } = req.locals
+  const params = requestData?.getParams?.() ?? {}
+  const { organisationName, dataset } = params
+
+  if (!organisationName || !dataset) {
+    return next(new MiddlewareError('Column mapping request params not found', 404))
+  }
+
+  if (await isStatutoryDataset({
+    organisation: organisationName,
+    dataset
+  })) {
+    return next(new MiddlewareError('Column mapping not found', 404))
+  }
+
+  next()
+}
+
 /**
  * Build the options object passed to the column-mapping template.
  * Returns UI-friendly data including expected mapping rows, selectable uploaded

src/utils/redisLoader.js

@@ -93,7 +93,8 @@ export async function getProvisionReasonsForDataset ({ organisation, dataset })
   return provisionReasons
 }
 
-export async function isStatutoryDataset ({ organisation, dataset }) {
+export async function isStatutoryDataset (requestParams = {}) {
+  const { organisation, dataset } = requestParams ?? {}
   const provisionReasons = await getProvisionReasonsForDataset({ organisation, dataset })
   return provisionReasons.includes('statutory')
 }

test/unit/columnMappingController.test.js

@@ -1,4 +1,4 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, it, vi } from 'vitest'
 import {
   applySubmittedFieldSelections,
   applyDetectedGeometryColumnMapping,
@@ -9,10 +9,50 @@ import {
   buildSelectableColumns,
   detectGeometryColumnMapping,
   getBracketFields,
+  handleUnavailableColumnMappingRequest,
   validateColumnMapping
 } from '../../src/controllers/columnMappingController.js'
+import { MiddlewareError } from '../../src/utils/errors.js'
 
 describe('columnMappingController helpers', () => {
+  it('returns a 404 when column mapping request params are missing', async () => {
+    const res = {}
+    const next = vi.fn()
+
+    await handleUnavailableColumnMappingRequest({
+      locals: {
+        requestData: {
+          getParams: () => ({ dataset: 'tree' })
+        }
+      }
+    }, res, next)
+
+    expect(next).toHaveBeenCalledWith(expect.any(MiddlewareError))
+    expect(next.mock.calls[0][0].statusCode).toBe(404)
+
+    vi.clearAllMocks()
+
+    await handleUnavailableColumnMappingRequest({
+      locals: {
+        requestData: {
+          getParams: () => ({ organisationName: 'local-authority:TST' })
+        }
+      }
+    }, res, next)
+
+    expect(next).toHaveBeenCalledWith(expect.any(MiddlewareError))
+    expect(next.mock.calls[0][0].statusCode).toBe(404)
+
+    vi.clearAllMocks()
+
+    await handleUnavailableColumnMappingRequest({
+      locals: {}
+    }, res, next)
+
+    expect(next).toHaveBeenCalledWith(expect.any(MiddlewareError))
+    expect(next.mock.calls[0][0].statusCode).toBe(404)
+  })
+
   it('builds rows from mapped, missing and unmapped columns', () => {
     const rows = buildColumnMappingRows({
       columnFieldLog: [

test/unit/redisLoader.test.js

@@ -230,4 +230,37 @@ describe('getDatasetNameMap', () => {
       vi.resetModules()
     }
   })
+
+  it('should return false for missing statutory dataset params without querying Datasette', async () => {
+    const mockDatasette = {
+      default: {
+        runQuery: vi.fn()
+      }
+    }
+
+    try {
+      vi.resetModules()
+      vi.doMock('../../config/index.js', () => ({
+        default: {
+          redis: false,
+          mainWebsiteUrl: config.mainWebsiteUrl
+        }
+      }))
+      vi.doMock('../../src/services/datasette.js', () => mockDatasette)
+
+      const { isStatutoryDataset } = await import('../../src/utils/redisLoader.js')
+
+      await expect(isStatutoryDataset(null)).resolves.toBe(false)
+      await expect(isStatutoryDataset()).resolves.toBe(false)
+      await expect(isStatutoryDataset({
+        organisation: 'local-authority:TST'
+      })).resolves.toBe(false)
+
+      expect(mockDatasette.default.runQuery).not.toHaveBeenCalled()
+    } finally {
+      vi.unmock('../../config/index.js')
+      vi.unmock('../../src/services/datasette.js')
+      vi.resetModules()
+    }
+  })
 })