From dd8fc70c608b576a5f0a2e060bdd135a7b4bd3fa Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Wed, 27 May 2026 16:00:02 +0100 Subject: [PATCH 01/21] feat: add column mapping functionality to C&P --- config/default.yaml | 98 ++++- src/assets/scss/index.scss | 16 + src/controllers/columnMappingController.js | 362 ++++++++++++++++ src/controllers/resultsController.js | 25 ++ src/controllers/statusController.js | 88 ++++ src/routes/form-wizard/check/steps.js | 9 + src/services/asyncRequestApi.js | 4 + src/utils/redisLoader.js | 95 ++++- src/views/check/column-mapping.html | 54 +++ .../check/components/column-mapping.html | 100 +++++ src/views/check/results/results.html | 25 ++ test/unit/columnMappingController.test.js | 390 ++++++++++++++++++ test/unit/redisLoader.test.js | 155 ++++++- test/unit/statusController.test.js | 124 +++++- test/unit/views/check/columnMapping.test.js | 169 ++++++++ 15 files changed, 1708 insertions(+), 6 deletions(-) create mode 100644 src/controllers/columnMappingController.js create mode 100644 src/views/check/column-mapping.html create mode 100644 src/views/check/components/column-mapping.html create mode 100644 test/unit/columnMappingController.test.js create mode 100644 test/unit/views/check/columnMapping.test.js diff --git a/config/default.yaml b/config/default.yaml index 2b89001bc..e553e842f 100644 --- a/config/default.yaml +++ b/config/default.yaml @@ -56,85 +56,181 @@ datasetsConfig: entityDisplayName: base: article 4 variable: direction + requiredFields: + - reference + - name + - document-url + - documentation-url article-4-direction-area: guidanceUrl: /guidance/specifications/article-4-direction entityDisplayName: base: article 4 direction variable: area + requiredFields: + - reference + - geometry + - name + - permitted-development-rights + - article-4-direction brownfield-land: guidanceUrl: https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data entityDisplayName: base: brownfield land variable: site + requiredFields: + - OrganisationURI + - SiteReference + - SiteNameAddress + - GeoX + - GeoY conservation-area: guidanceUrl: /guidance/specifications/conservation-area entityDisplayName: base: conservation variable: area + requiredFields: + - reference + - geometry + - name conservation-area-document: guidanceUrl: /guidance/specifications/conservation-area entityDisplayName: base: conservation variable: area + requiredFields: + - reference + - name + - conservation-area + - document-url + - documentation-url + - document-type tree-preservation-order: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: base: tree preservation variable: order + requiredFields: + - reference + - document-url + - documentation-url tree-preservation-zone: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: base: tree preservation variable: zone + requiredFields: + - reference + - geometry + - tree-preservation-order tree: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: variable: tree + requiredFields: + - reference + - tree-preservation-order + - point + - geometry listed-building-outline: guidanceUrl: /guidance/specifications/listed-building entityDisplayName: base: listed building variable: outline + requiredFields: + - reference + - geometry + - name + - listed-building developer-agreement-transaction: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer agreement variable: transaction + requiredFields: + - reference developer-agreement: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer variable: agreement + requiredFields: + - reference developer-agreement-contribution: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer agreement variable: contribution + requiredFields: + - reference infrastructure-funding-statement: guidanceUrl: https://digital-land.github.io/specification/specification/infrastructure-funding-statement/ entityDisplayName: base: infrastructure funding variable: statement + requiredFields: + - reference plan-timetable: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: plan variable: timetable + requiredFields: + - reference + - plan + - plan-event + - event-date + - entry-date local-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: local - variable: plan + variable: plan + requiredFields: + - reference + - name + - description + - dataset + - period-start-date + - period-end-date + - local-planning-authorities + - documentation-url + - document-url + - required-housing + - entry-date minerals-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: minerals variable: plan + requiredFields: + - reference + - name + - description + - dataset + - period-start-date + - period-end-date + - minerals-and-waste-planning-authorities + - documentation-url + - document-url + - document-count + - entry-date waste-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: waste variable: plan + requiredFields: + - reference + - name + - description + - dataset + - period-start-date + - period-end-date + - minerals-and-waste-planning-authorities + - documentation-url + - document-url + - document-count + - entry-date supplementary-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: diff --git a/src/assets/scss/index.scss b/src/assets/scss/index.scss index 3b706c57e..2783428a9 100644 --- a/src/assets/scss/index.scss +++ b/src/assets/scss/index.scss @@ -249,6 +249,22 @@ code * { border-left: govuk-spacing(1) solid $govuk-error-colour; } +.column-mapping__row--error { + padding-left: 0; + + .govuk-table__cell:first-child { + border-left: govuk-spacing(1) solid $govuk-error-colour; + } +} + +.column-mapping-table { + th, + td { + padding-top: govuk-spacing(2); + padding-bottom: govuk-spacing(2); + } +} + .app-content__markdown img { max-width: 100%; } diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js new file mode 100644 index 000000000..3efc41c84 --- /dev/null +++ b/src/controllers/columnMappingController.js @@ -0,0 +1,362 @@ +import PageController from './pageController.js' +import config from '../../config/index.js' +import { postCheckRequest } from '../services/asyncRequestApi.js' +import { getDatasetFields, isStatutoryDataset } from '../utils/redisLoader.js' +import { getRequestDataMiddleware, updateSessionFromRequestData } from './resultsController.js' + +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(updateSessionFromRequestData) + } + + async locals (req, res, next) { + try { + const { requestData } = req.locals + if (!requestData) return + + if (requestData.isFailed()) { + res.redirect(`/check/results/${req.params.id}/1`) + return + } + + Object.assign(req.form.options, await buildColumnMappingOptions({ + requestData, + requestId: req.params.id + })) + super.locals(req, res, next) + } catch (error) { + next(error) + } + } + + async post (req, res, next) { + try { + const requestData = await getCompletedRequestData(req, res) + if (!requestData) return + + const options = await buildColumnMappingOptions({ + requestData, + requestId: req.params.id, + body: req.body + }) + const validationErrors = validateColumnMapping(req.body, options.mappingRows) + if (Object.keys(validationErrors).length > 0) { + options.columnMappingErrors = validationErrors + Object.assign(req.form.options, options) + Object.assign(res.locals, { + options: req.form.options, + errors: {} + }) + res.status(400) + res.render('check/column-mapping.html') + return + } + + const params = requestData.getParams() ?? {} + const { columnMappingRows } = await prepareColumnMappingContext(requestData) + const spareUploadedColumns = buildSelectableColumns(columnMappingRows) + const columnMapping = buildSubmittedColumnMapping({ + existingMapping: params.column_mapping, + body: req.body, + spareUploadedColumns + }) + + const newRequestId = await postCheckRequest({ + ...params, + column_mapping: Object.keys(columnMapping).length > 0 ? columnMapping : null + }) + + req.sessionModel.set('request_id', newRequestId) + res.redirect(`/check/status/${newRequestId}`) + } catch (error) { + next(error) + } + } +} + +/** + * Build the options object passed to the column-mapping template. + * Returns UI-friendly data including expected mapping rows, selectable uploaded + * columns and any validation errors so the view can render the mapping form. + */ +async function buildColumnMappingOptions ({ requestData, requestId, body = {}, validationErrors = {} }) { + const { + columnMappingRows, + specFields, + requiredFields + } = await prepareColumnMappingContext(requestData) + const mappingRows = buildExpectedFieldRows({ + columnMappingRows, + specFields, + requiredFields + }) + + applySubmittedFieldSelections(mappingRows, body) + + return { + id: requestId, + requestParams: requestData.getParams(), + mappingRows, + uploadedColumns: buildSelectableColumns(columnMappingRows), + columnMappingErrors: validationErrors, + lastPage: `/check/status/${requestId}` + } +} + +/** + * Prepare the low-level data required to build the column mapping UI. + * + * - Fetches response details and the column-field detection log from `requestData`. + * - Builds `columnMappingRows` (combined auto-detected + user overrides). + * - Resolves `specFields` from the dataset and `requiredFields` from config. + */ +async function prepareColumnMappingContext (requestData) { + const responseDetails = await requestData.fetchResponseDetails(0, 50) + const columnFieldLog = requestData.getColumnFieldLog() + const params = requestData.getParams() ?? {} + const userColumnMapping = params.column_mapping ?? {} + const columnMappingRows = buildColumnMappingRows({ + columnFieldLog, + responseRows: responseDetails.getRows(), + userColumnMapping + }) + const specFields = buildSpecFields(await getDatasetFields(params.dataset)) + const requiredFields = config.datasetsConfig?.[params.dataset]?.requiredFields ?? [] + return { + columnMappingRows, + specFields, + requiredFields + } +} + +async function getCompletedRequestData (req, res) { + const { requestData } = req.locals + if (!requestData) { + return null + } + if (!requestData.isComplete()) { + res.redirect(`/check/status/${req.params.id}`) + return null + } + return requestData +} + +export function validateColumnMapping (body = {}, mappingRows = []) { + const requiredFields = new Set(mappingRows.filter(row => row.isRequired).map(row => row.field)) + + return Object.fromEntries( + Object.entries(getBracketFields(body, 'fieldMap')) + .filter(([field, value]) => value === '' || (value === 'na' && requiredFields.has(field))) + .map(([field]) => [field, { + text: `Select the ${field} field` + }]) + ) +} + +export function applySubmittedFieldSelections (mappingRows = [], body = {}) { + const fieldMap = getBracketFields(body, 'fieldMap') + mappingRows.forEach(row => { + if (Object.hasOwn(fieldMap, row.field)) { + row.column = fieldMap[row.field] + } + }) +} + +export function buildSubmittedColumnMapping ({ existingMapping = {}, body = {}, spareUploadedColumns = [] }) { + const columnMapping = { ...existingMapping } + const columns = Array.isArray(body.columns) ? body.columns : [body.columns].filter(Boolean) + const selectedUploadedColumns = new Set() + + columns.forEach((column, index) => { + const field = body[`field-${index}`]?.trim() + const status = body[`status-${index}`] + + if (status === 'ignore') { + columnMapping[column] = 'IGNORE' + } else if (status === 'unmap') { + delete columnMapping[column] + } else if (field) { + columnMapping[column] = field + } + }) + + for (const [column, fieldValue] of Object.entries(getBracketFields(body, 'map'))) { + const field = fieldValue?.trim() + if (field === 'IGNORE') { + columnMapping[column] = 'IGNORE' + } else if (field) { + columnMapping[column] = field + } + } + + for (const [column, value] of Object.entries(getBracketFields(body, 'unmap'))) { + if (value === 'yes') { + delete columnMapping[column] + } + } + + for (const [field, columnValue] of Object.entries(getBracketFields(body, 'fieldMap'))) { + const column = columnValue?.trim() + if (!column) continue + + for (const [mappedColumn, mappedField] of Object.entries(columnMapping)) { + if (mappedField === field) delete columnMapping[mappedColumn] + } + if (column !== 'na') { + columnMapping[column] = field + selectedUploadedColumns.add(column) + } + } + + spareUploadedColumns.forEach(column => { + if (!selectedUploadedColumns.has(column) && !columnMapping[column]) { + columnMapping[column] = 'IGNORE' + } + }) + + return Object.fromEntries( + Object.entries(columnMapping).filter(([, field]) => field) + ) +} + +export function getBracketFields (body = {}, fieldName) { + const values = { ...(body[fieldName] ?? {}) } + const prefix = `${fieldName}[` + + for (const [key, value] of Object.entries(body)) { + if (key.startsWith(prefix) && key.endsWith(']')) { + values[key.slice(prefix.length, -1)] = value + } + } + + return values +} + +export function buildSpecFields (datasetFields = []) { + const fields = new Set(datasetFields) + return [...fields].sort() +} + +/** + * Build the canonical list of column mapping rows. + * + * Each row describes an uploaded column and how it relates to a spec field: + * - `column`: uploaded column name (empty string for missing auto-detected fields) + * - `field`: mapped spec field (if any) + * - `isMapped`, `isAutoMapped`, `isMissing`, `userDefined`, `userIgnored` + * + * @param {Object} options + * @param {Array} options.columnFieldLog - auto-detected column->field log entries + * @param {Array} options.responseRows - sample response rows with `converted_row` keys + * @param {Object} options.userColumnMapping - existing user mapping overrides + * @returns {Array} rows suitable for UI consumption + */ +export function buildColumnMappingRows ({ columnFieldLog = [], responseRows = [], userColumnMapping = {} }) { + const mappedColumns = new Set() + const rows = [] + + columnFieldLog.forEach(entry => { + const column = entry?.column + if (!column) { + if (entry?.field && entry?.missing) { + rows.push({ + column: '', + field: entry.field, + isMapped: false, + isMissing: true, + userDefined: false, + userIgnored: false + }) + } + return + } + + mappedColumns.add(column) + const userMappedField = userColumnMapping[column] + const userIgnored = userMappedField === 'IGNORE' + const field = userIgnored ? '' : userMappedField || entry.field || '' + + rows.push({ + column, + field, + isMapped: Boolean(field) && !entry.missing, + isAutoMapped: Boolean(entry.field) && !userMappedField && !entry.missing, + isMissing: Boolean(entry.missing), + userDefined: Boolean(userMappedField) && !userIgnored, + userIgnored + }) + }) + + const unmappedColumns = new Set() + responseRows.forEach(row => { + Object.keys(row?.converted_row ?? {}).forEach(column => { + if (!mappedColumns.has(column)) unmappedColumns.add(column) + }) + }) + + const sortedUnmappedColumns = [...unmappedColumns].sort() + sortedUnmappedColumns.forEach(column => { + rows.push({ + column, + field: userColumnMapping[column] === 'IGNORE' ? '' : userColumnMapping[column] || '', + isMapped: Boolean(userColumnMapping[column]) && userColumnMapping[column] !== 'IGNORE', + isAutoMapped: false, + isMissing: false, + userDefined: Boolean(userColumnMapping[column]) && userColumnMapping[column] !== 'IGNORE', + userIgnored: userColumnMapping[column] === 'IGNORE' + }) + }) + + return rows +} + +export function buildSelectableColumns (columnMappingRows = []) { + return [...new Set( + columnMappingRows + .filter(row => row.userDefined || !row.isMapped) + .map(row => row.column) + .filter(Boolean) + )].sort() +} + +export function buildExpectedFieldRows ({ columnMappingRows = [], specFields = [], requiredFields = [] }) { + const requiredFieldSet = new Set(requiredFields) + + return specFields.map(field => { + const row = columnMappingRows.find(row => row.field === field && row.isMapped && !row.userIgnored) + const isAutoMapped = Boolean(row?.isMapped) && !row?.userDefined + return { + field, + column: row?.column ?? '', + isMapped: Boolean(row?.column), + isAutoMapped, + userDefined: Boolean(row?.userDefined), // if the user has explicitly mapped this field + isEditable: !isAutoMapped, // shows as dropdown in the UI and also shows as an Unmapped badge + isRequired: requiredFieldSet.has(field) + } + }).sort((a, b) => { + const rank = (row) => { + if (row.isAutoMapped && row.isRequired) return 0 + if (row.isAutoMapped && !row.isRequired) return 1 + if (row.userDefined && row.isRequired) return 2 + if (!row.isMapped && row.isRequired) return 3 + if (row.userDefined && !row.isRequired) return 4 + return 5 + } + + if (rank(a) !== rank(b)) return rank(a) - rank(b) + return a.field.localeCompare(b.field) + }) +} + +export default ColumnMappingController diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 7a508d4a3..f2b2569cb 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -223,6 +223,31 @@ export function setupTableParams (req, res, next) { columnNameProcessing: 'none', mapping: columnToField } + + // Show column mapping if the requestParams contains a non-empty mapping object + const columnMapping = req.locals.requestParams?.column_mapping + const hasMappingObject = columnMapping && typeof columnMapping === 'object' && Object.keys(columnMapping).length > 0 + const columnMappingEnabled = columnMapping === true || hasMappingObject + req.locals.columnMappingEnabled = columnMappingEnabled + if (columnMappingEnabled) { + const columnMappingRows = orderedFields + .map(col => { + const mappedField = columnToField.get(col) || '' + return { mappedField, col } + }) + .filter(({ mappedField, col }) => { + // skip empty values and explicit IGNORE mappings + if (!mappedField || !col) return false + if (typeof mappedField === 'string' && mappedField.toUpperCase() === 'IGNORE') return false + return true + }) + .map(({ mappedField, col }) => ({ + key: { text: mappedField }, + value: { html: col } + })) + + req.locals.columnMappingRows = columnMappingRows + } req.locals.geometries = req.locals.datasetTypology === 'geography' ? responseDetails.getGeometries() diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index 3b902b93a..3eb84e365 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -2,6 +2,7 @@ import PageController from './pageController.js' import { getRequestData } from '../services/asyncRequestApi.js' import { finishedProcessingStatuses } from '../utils/utils.js' import { headingTexts, messageTexts, buttonTexts, buttonAriaLabels } from '../content/statusPage.js' +import { getDatasetFields, isStatutoryDataset } from '../utils/redisLoader.js' /** * Attempts to infer how we ended up on this page. @@ -20,6 +21,19 @@ function getLastPage (req) { } class StatusController extends PageController { + async post (req, res, next) { + try { + const requestData = await getRequestData(req.params.id) + const nextStep = await shouldShowColumnMapping(requestData) + ? `/check/column-mapping/${req.params.id}` + : `/check/results/${req.params.id}/1` + + res.redirect(nextStep) + } catch (error) { + next(error) + } + } + async locals (req, res, next) { try { req.form.options.data = await getRequestData(req.params.id) @@ -44,4 +58,78 @@ class StatusController extends PageController { } } +export async function shouldShowColumnMapping (requestData) { + if (!requestData?.isComplete?.() || requestData?.isFailed?.()) { + return false + } + + const params = requestData.getParams?.() ?? {} + + try { + if (await isStatutoryDataset({ + organisation: params.organisationName, + dataset: params.dataset + })) { + return false + } + + const expectedFields = await getDatasetFields(params.dataset) + if (expectedFields.length === 0) return false + + const columnFieldLog = requestData.getColumnFieldLog?.() ?? [] + const userColumnMapping = params.column_mapping ?? {} + const responseDetails = await requestData.fetchResponseDetails(0, 50) + const rows = responseDetails.getRows?.() ?? [] + + const mappedFields = buildMappedFields(columnFieldLog, userColumnMapping) + const unmappedExpectedFields = new Set(expectedFields.filter(field => !mappedFields.has(field))) + const hasUnmappedExpectedFields = unmappedExpectedFields.size > 0 + if (!hasUnmappedExpectedFields) return false + + // If there are other blocking external errors, users should resolve those in + // results instead of being redirected to column-mapping. + const hasOtherBlockingExternalErrors = rows.some(row => + (row?.issue_logs ?? []).some(issue => + issue?.severity === 'error' && + issue?.responsibility === 'external' && + !unmappedExpectedFields.has(issue?.field) + ) + ) + if (hasOtherBlockingExternalErrors) return false + + const spareUploadedColumns = buildSpareUploadedColumns(columnFieldLog, rows, userColumnMapping) + return spareUploadedColumns.length > 0 + } catch { + return false + } +} + +function buildMappedFields (columnFieldLog = [], userColumnMapping = {}) { + const fields = new Set() + + columnFieldLog.forEach(column => { + if (column?.field && column?.column && !column?.missing) fields.add(column.field) + }) + + Object.values(userColumnMapping).forEach(field => { + if (field && field !== 'IGNORE') fields.add(field) + }) + + return fields +} + +function buildSpareUploadedColumns (columnFieldLog = [], rows = [], userColumnMapping = {}) { + const mappedColumns = new Set(columnFieldLog.map(column => column?.column).filter(Boolean)) + Object.entries(userColumnMapping).forEach(([column, field]) => { + if (field) mappedColumns.add(column) + }) + + const uploadedColumns = new Set() + rows.forEach(row => { + Object.keys(row?.converted_row ?? {}).forEach(column => uploadedColumns.add(column)) + }) + + return [...uploadedColumns].filter(column => !mappedColumns.has(column)) +} + export default StatusController diff --git a/src/routes/form-wizard/check/steps.js b/src/routes/form-wizard/check/steps.js index 9396ee3ec..fbf4c90fc 100644 --- a/src/routes/form-wizard/check/steps.js +++ b/src/routes/form-wizard/check/steps.js @@ -10,6 +10,7 @@ import shareResultsController from '../../../controllers/ShareResultsController. import issueDetailsController from '../../../controllers/issueDetailsController.js' import checkStartController from '../../../controllers/checkStartController.js' import checkConfirmationController from '../../../controllers/checkConfirmationController.js' +import columnMappingController from '../../../controllers/columnMappingController.js' const baseSettings = { controller: PageController, @@ -82,6 +83,14 @@ export default { entryPoint: true, next: (req, res) => `results/${req.params.id}/1` }, + '/column-mapping/:id': { + ...baseSettings, + template: 'check/column-mapping.html', + controller: columnMappingController, + checkJourney: false, + entryPoint: true, + next: (req, res) => `results/${req.params.id}/1` + }, '/results/:id/share': { ...baseSettings, template: 'check/results/shareResults.html', diff --git a/src/services/asyncRequestApi.js b/src/services/asyncRequestApi.js index 306347f60..21da16286 100644 --- a/src/services/asyncRequestApi.js +++ b/src/services/asyncRequestApi.js @@ -33,6 +33,10 @@ export const postUrlRequest = async (formData) => { }) } +export const postCheckRequest = async (params) => { + return await postRequest(params) +} + /** * POSTs a requeset to the 'publish' API. * diff --git a/src/utils/redisLoader.js b/src/utils/redisLoader.js index a848a474c..b851be3eb 100644 --- a/src/utils/redisLoader.js +++ b/src/utils/redisLoader.js @@ -2,6 +2,7 @@ import config from '../../config/index.js' import { createClient } from 'redis' import logger from '../utils/logger.js' +import datasette from '../services/datasette.js' let redisClient @@ -36,7 +37,99 @@ export async function getRedisClient () { return redisClient } -const CACHE_TTL = 300 // 5min +const CACHE_TTL = 60 * 60 * 6 // 6 hours +const SYSTEM_FIELDS = new Set([ + 'entity', + 'prefix', + 'entry-number', + 'organisation-entity', + 'organisation', + 'IGNORE', + '' +]) + +function escapeSqlString (value) { + return String(value).replaceAll("'", "''") +} + +async function getCachedJson (key, logPrefix) { + const client = await getRedisClient() + if (!client) return undefined + + try { + const cached = await client.get(cacheKey(key)) + if (cached) return JSON.parse(cached) + } catch (err) { + logger.warn(`${logPrefix}/redis get error: ${err.message}`) + } + + return undefined +} + +async function setCachedJson (key, value, logPrefix, ttl = CACHE_TTL) { + const client = await getRedisClient() + if (!client) return + + try { + await client.setEx(cacheKey(key), ttl, JSON.stringify(value)) + } catch (err) { + logger.warn(`${logPrefix}/redis set error: ${err.message}`) + } +} + +export function normaliseDatasetFields (rows = [], dataset) { + return [...new Set( + rows + .map(row => row?.field) + .filter(field => field && !SYSTEM_FIELDS.has(field) && dataset !== field) + )].sort() +} + +export async function getDatasetFields (dataset) { + if (!dataset) return [] + + const key = `dataset-fields:${dataset}` + const cached = await getCachedJson(key, 'getDatasetFields') + if (cached) return cached + + const query = `select field from dataset_field where dataset = '${escapeSqlString(dataset)}'` + const response = await datasette.runQuery(query) + const fields = normaliseDatasetFields(response.formattedData) + + await setCachedJson(key, fields, 'getDatasetFields') + + return fields +} + +export async function getProvisionReasonsForDataset ({ organisation, dataset }) { + if (!organisation || !dataset) return [] + + const key = `provision-reasons:${organisation}:${dataset}` + const cached = await getCachedJson(key, 'getProvisionReasonsForDataset') + if (cached) return cached + + const query = ` + select provision_reason from provision + where organisation = '${escapeSqlString(organisation)}' + and dataset = '${escapeSqlString(dataset)}' + and ( + end_date is null + or end_date = '') + ` + const response = await datasette.runQuery(query) + const provisionReasons = response.formattedData + .map(row => row?.provision_reason) + .filter(Boolean) + + await setCachedJson(key, provisionReasons, 'getProvisionReasonsForDataset') + + return provisionReasons +} + +export async function isStatutoryDataset ({ organisation, dataset }) { + const provisionReasons = await getProvisionReasonsForDataset({ organisation, dataset }) + return provisionReasons.includes('statutory') +} // TODO: future removal of this function in favour of using datasetNameSlug and datasetSubjectLoaded instead. export async function fetchDatasetNames (datasetKeys) { diff --git a/src/views/check/column-mapping.html b/src/views/check/column-mapping.html new file mode 100644 index 000000000..f9e43a6bb --- /dev/null +++ b/src/views/check/column-mapping.html @@ -0,0 +1,54 @@ +{% extends "layouts/main.html" %} + +{% from 'govuk/components/back-link/macro.njk' import govukBackLink %} +{% from 'govuk/components/error-summary/macro.njk' import govukErrorSummary %} +{% from '../components/dataset-banner.html' import datasetBanner %} +{% from './components/column-mapping.html' import columnMapping %} + +{% set serviceType = 'Check' %} +{% set pageName = 'Map your fields' %} + +{% block beforeContent %} + {{ govukBackLink({ + text: "Back", + href: options.lastPage + }) }} +{% endblock %} + +{% block content %} +
+
+ {{ datasetBanner(options.requestParams.organisationName | orgIdToName, options.requestParams.dataset) }} + + {% set columnMappingErrorKeys = options.columnMappingErrors | getkeys %} + {% if columnMappingErrorKeys | length > 0 %} + {% set errorList = [] %} + {% for row in options.mappingRows %} + {% if options.columnMappingErrors[row.field] %} + {% set _ = errorList.push({ + text: options.columnMappingErrors[row.field].text, + href: "#fieldMap-" + loop.index0 + }) %} + {% endif %} + {% endfor %} + {{ govukErrorSummary({ + titleText: "The following fields need to be mapped before you can check your data", + errorList: errorList + }) }} + {% endif %} + +

{{ pageName }}

+

Review how columns from your data match planning data fields. If a column is mapped incorrectly, choose a different field and re-check the data.

+ +
+ + {{ columnMapping(options.mappingRows, options.uploadedColumns, options.columnMappingErrors,false) }} +
+ + +
+ +
+
+
+{% endblock %} diff --git a/src/views/check/components/column-mapping.html b/src/views/check/components/column-mapping.html new file mode 100644 index 000000000..19195cb3e --- /dev/null +++ b/src/views/check/components/column-mapping.html @@ -0,0 +1,100 @@ +{% from 'govuk/components/button/macro.njk' import govukButton %} +{% from "govuk/components/error-message/macro.njk" import govukErrorMessage %} +{% from "govuk/components/select/macro.njk" import govukSelect %} + + +{% macro columnMapping(mappingRows, uploadedColumns=[], errors={}, allowUnmapping=true) %} +
+
+ + + + + + + + + + + + + + + + + {% for row in mappingRows %} + {% set isEditable = row.isEditable%} + + + + + + + {% endfor %} + +
Expected fieldsFields we found in your dataStatus
+ {{ row.field }} + + {% if not isEditable %} + {{ row.column }} + {% else %} + {% set selectItems = [ + { + value: "", + text: "-- Select a field --", + selected: not row.column + } + ] %} + {% if not row.isRequired %} + {% set _ = selectItems.push({ + value: "na", + text: "Not provided" + }) %} + {% endif %} + {% for column in uploadedColumns %} + {% set _ = selectItems.push({ + value: column, + text: column, + selected: column == row.column + }) %} + {% endfor %} + {% if errors[row.field] %} + {{ govukErrorMessage({ + id: "fieldMap-" + loop.index0 + "-error", + text: errors[row.field].text + }) }} + {% endif %} + {{ govukSelect({ + id: "fieldMap-" + loop.index0, + name: "fieldMap[" + row.field + "]", + classes: "govuk-!-width-full govuk-select--error" if errors[row.field] else "govuk-!-width-full", + describedBy: "fieldMap-" + loop.index0 + "-error" if errors[row.field] else "", + label: { + text: "fields we found in your data", + classes: "govuk-visually-hidden" + }, + formGroup: { + classes: "govuk-!-margin-bottom-0" + }, + items: selectItems + }) }} + {% endif %} + + {% if not isEditable %} + {% if allowUnmapping %} + + {% else %} + Mapped + {% endif %} + {% else %} + Unmapped + {% endif %} +
+
+
+{% endmacro %} diff --git a/src/views/check/results/results.html b/src/views/check/results/results.html index 35838b0be..997064730 100644 --- a/src/views/check/results/results.html +++ b/src/views/check/results/results.html @@ -119,6 +119,31 @@

{{ govukSummaryList({ rows: rows }) }} + + {% if options.columnMappingEnabled and options.columnMappingRows and options.columnMappingRows | length > 0 %} +
+
+
+

Field mapping

+
+
+ Map again +
+
+ + {% set headerRows = [ + { + key: { text: 'Expected field' }, + value: { html: 'Field in your data' } + } + ] %} + + {% set summaryRows = headerRows.concat(options.columnMappingRows) %} + + {{ govukSummaryList({ rows: summaryRows }) }} +
+ + {% endif %} {% if options.passedChecks.length > 0 %}
diff --git a/test/unit/columnMappingController.test.js b/test/unit/columnMappingController.test.js new file mode 100644 index 000000000..a6f6355fc --- /dev/null +++ b/test/unit/columnMappingController.test.js @@ -0,0 +1,390 @@ +import { describe, expect, it } from 'vitest' +import { + applySubmittedFieldSelections, + buildExpectedFieldRows, + buildColumnMappingRows, + buildSpecFields, + buildSubmittedColumnMapping, + buildSelectableColumns, + getBracketFields, + validateColumnMapping +} from '../../src/controllers/columnMappingController.js' + +describe('columnMappingController helpers', () => { + it('builds rows from mapped, missing and unmapped columns', () => { + const rows = buildColumnMappingRows({ + columnFieldLog: [ + { column: 'Reference', field: 'reference' }, + { column: 'Start', field: 'start-date', missing: true } + ], + responseRows: [ + { converted_row: { Reference: 'abc', Name: 'Test name' } } + ], + userColumnMapping: { + Name: 'name' + } + }) + + expect(rows).toEqual([ + { + column: 'Reference', + field: 'reference', + isMapped: true, + isAutoMapped: true, + isMissing: false, + userDefined: false, + userIgnored: false + }, + { + column: 'Start', + field: 'start-date', + isMapped: false, + isAutoMapped: false, + isMissing: true, + userDefined: false, + userIgnored: false + }, + { + column: 'Name', + field: 'name', + isMapped: true, + isAutoMapped: false, + isMissing: false, + userDefined: true, + userIgnored: false + } + ]) + }) + + it('keeps missing required fields that have no uploaded column', () => { + const rows = buildColumnMappingRows({ + columnFieldLog: [ + { field: 'reference', missing: true } + ], + responseRows: [] + }) + + expect(rows).toEqual([ + { + column: '', + field: 'reference', + isMapped: false, + isMissing: true, + userDefined: false, + userIgnored: false + } + ]) + }) + + it('merges submitted mappings with existing mappings', () => { + const mapping = buildSubmittedColumnMapping({ + existingMapping: { + Old: 'notes', + Removed: 'name', + Reference: 'reference' + }, + body: { + map: { + Reference: 'IGNORE' + }, + fieldMap: { + reference: 'New reference', + notes: 'na' + } + } + }) + + expect(mapping).toEqual({ + Removed: 'name', + Reference: 'IGNORE', + 'New reference': 'reference' + }) + }) + + it('supports literal bracketed form keys', () => { + const mapping = buildSubmittedColumnMapping({ + existingMapping: { + Old: 'notes' + }, + body: { + 'map[Old]': 'IGNORE', + 'fieldMap[reference]': 'Reference', + 'unmap[Removed]': 'yes' + } + }) + + expect(mapping).toEqual({ + Old: 'IGNORE', + Reference: 'reference' + }) + }) + + it('leaves existing mappings unchanged when fieldMap values are blank', () => { + const mapping = buildSubmittedColumnMapping({ + existingMapping: { + Description: 'description', + Geometry: 'geometry' + }, + body: { + fieldMap: { + description: '', + geometry: '', + notes: 'Notes' + }, + map: { + Description: '', + Geometry: '' + } + } + }) + + expect(mapping).toEqual({ + Description: 'description', + Geometry: 'geometry', + Notes: 'notes' + }) + }) + + it('builds spec fields from dataset fields without hardcoded defaults', () => { + expect(buildSpecFields(['notes', 'reference', 'name'])).toEqual(['name', 'notes', 'reference']) + expect(buildSpecFields([])).toEqual([]) + }) + + it('builds expected field rows and uploaded column options', () => { + const columnMappingRows = [ + { + column: 'Reference', + field: 'reference', + isMapped: true, + userDefined: false, + userIgnored: false + }, + { + column: 'Name', + field: 'name', + isMapped: true, + userDefined: true, + userIgnored: false + } + ] + + expect(buildSelectableColumns(columnMappingRows)).toEqual(['Name']) + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['notes', 'reference', 'name'], + requiredFields: ['reference'] + })).toEqual([ + { + field: 'reference', + column: 'Reference', + isMapped: true, + isAutoMapped: true, + isEditable: false, + userDefined: false, + isRequired: true + }, + { + field: 'name', + column: 'Name', + isMapped: true, + isAutoMapped: false, + isEditable: true, + userDefined: true, + isRequired: false + }, + { + field: 'notes', + column: '', + isMapped: false, + isAutoMapped: false, + isEditable: true, + userDefined: false, + isRequired: false + } + ]) + }) + + it('sorts expected field rows by automapped, required, then user-defined priority', () => { + const columnMappingRows = [ + { + column: 'Alpha column', + field: 'alpha', + isMapped: true, + isAutoMapped: true, + userDefined: false, + userIgnored: false + }, + { + column: 'Beta column', + field: 'beta', + isMapped: true, + isAutoMapped: true, + userDefined: false, + userIgnored: false + }, + { + column: '', + field: 'gamma', + isMapped: false, + userDefined: false, + userIgnored: false + }, + { + column: '', + field: 'delta', + isMapped: false, + userDefined: false, + userIgnored: false + }, + { + column: 'Epsilon column', + field: 'epsilon', + isMapped: true, + userDefined: true, + userIgnored: false + } + ] + + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['delta', 'beta', 'gamma', 'epsilon', 'alpha'], + requiredFields: ['alpha', 'gamma'] + }).map(row => row.field)).toEqual([ + 'alpha', + 'beta', + 'gamma', + 'epsilon', + 'delta' + ]) + }) + + it('marks missing expected field rows as required', () => { + const columnMappingRows = [ + { + column: '', + field: 'reference', + isMapped: false, + isMissing: true, + userDefined: false, + userIgnored: false + } + ] + + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['notes', 'reference'], + requiredFields: ['reference'] + })).toEqual([ + { + field: 'reference', + column: '', + isMapped: false, + isAutoMapped: false, + isEditable: true, + userDefined: false, + isRequired: true + }, + { + field: 'notes', + column: '', + isMapped: false, + isAutoMapped: false, + isEditable: true, + userDefined: false, + isRequired: false + } + ]) + }) + + it('includes ignored and spare uploaded columns in dropdown options', () => { + const rows = buildColumnMappingRows({ + columnFieldLog: [ + { column: 'Reference', field: 'reference' } + ], + responseRows: [ + { converted_row: { Reference: 'abc', Notes: 'note', Extra: 'extra' } } + ], + userColumnMapping: { + Extra: 'IGNORE' + } + }) + + expect(buildSelectableColumns(rows)).toEqual(['Extra', 'Notes']) + }) + + it('extracts nested and literal bracketed fields', () => { + expect(getBracketFields({ + map: { Column: 'field' }, + 'map[Other column]': 'other-field' + }, 'map')).toEqual({ + Column: 'field', + 'Other column': 'other-field' + }) + }) + + it('validates blank field mapping selections', () => { + expect(validateColumnMapping({ + fieldMap: { + notes: '', + reference: 'Reference', + geometry: 'na' + } + })).toEqual({ + notes: { + text: 'Select the notes field' + } + }) + }) + + it('validates not provided selections for required fields', () => { + expect(validateColumnMapping({ + fieldMap: { + reference: 'na', + notes: 'na' + } + }, [ + { field: 'reference', isRequired: true }, + { field: 'notes', isRequired: false } + ])).toEqual({ + reference: { + text: 'Select the reference field' + } + }) + }) + + it('applies submitted selections to mapping rows for redisplay', () => { + const rows = [ + { field: 'notes', column: '' }, + { field: 'reference', column: 'Reference' } + ] + + applySubmittedFieldSelections(rows, { + fieldMap: { + notes: 'Notes' + } + }) + + expect(rows).toEqual([ + { field: 'notes', column: 'Notes' }, + { field: 'reference', column: 'Reference' } + ]) + }) + + it('marks unselected spare uploaded columns as ignored on submit', () => { + const mapping = buildSubmittedColumnMapping({ + body: { + fieldMap: { + reference: 'Reference', + notes: 'na' + } + }, + spareUploadedColumns: ['Reference', 'Notes', 'Extra'] + }) + + expect(mapping).toEqual({ + Reference: 'reference', + Notes: 'IGNORE', + Extra: 'IGNORE' + }) + }) +}) diff --git a/test/unit/redisLoader.test.js b/test/unit/redisLoader.test.js index 0cfc6045e..683556874 100644 --- a/test/unit/redisLoader.test.js +++ b/test/unit/redisLoader.test.js @@ -1,5 +1,5 @@ import { vi, it, describe, expect, beforeEach, afterEach } from 'vitest' -import { getDatasetNameMap } from '../../src/utils/redisLoader' +import { getDatasetNameMap, normaliseDatasetFields } from '../../src/utils/redisLoader' import config from '../../config' describe('getDatasetNameMap', () => { @@ -103,4 +103,157 @@ describe('getDatasetNameMap', () => { vi.resetModules() } }) + + it('should fetch dataset fields and provision reasons with namespaced cache keys', async () => { + const originalDeployTime = process.env.DEPLOY_TIME + const mockRedisClient = { + isOpen: false, + connect: vi.fn().mockImplementation(async () => { + mockRedisClient.isOpen = true + }), + get: vi.fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(JSON.stringify(['statutory'])), + setEx: vi.fn().mockResolvedValue() + } + const mockDatasette = { + default: { + runQuery: vi.fn() + .mockResolvedValueOnce({ + formattedData: [ + { field: 'reference' }, + { field: 'organisation' }, + { field: 'notes' }, + { field: 'entity' }, + { field: 'name' } + ] + }) + .mockResolvedValueOnce({ + formattedData: [{ provision_reason: 'statutory' }] + }) + } + } + + try { + process.env.DEPLOY_TIME = 'deploy-456' + + vi.resetModules() + vi.doMock('redis', () => ({ + createClient: vi.fn(() => mockRedisClient) + })) + vi.doMock('../../config/index.js', () => ({ + default: { + redis: { + secure: false, + host: 'localhost', + port: 6379 + }, + mainWebsiteUrl: config.mainWebsiteUrl + } + })) + vi.doMock('../../src/services/datasette.js', () => mockDatasette) + + const { + getDatasetFields, + getProvisionReasonsForDataset, + isStatutoryDataset + } = await import('../../src/utils/redisLoader.js') + + await expect(getDatasetFields('conservation-area')).resolves.toEqual(['name', 'notes', 'reference']) + await expect(getProvisionReasonsForDataset({ + organisation: 'local-authority:TST', + dataset: 'conservation-area' + })).resolves.toEqual(['statutory']) + await expect(isStatutoryDataset({ + organisation: 'local-authority:TST', + dataset: 'conservation-area' + })).resolves.toBe(true) + + expect(mockRedisClient.get).toHaveBeenNthCalledWith(1, 'deploy-456:dataset-fields:conservation-area') + expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( + 1, + 'deploy-456:dataset-fields:conservation-area', + 300, + JSON.stringify(['name', 'notes', 'reference']) + ) + expect(mockRedisClient.get).toHaveBeenNthCalledWith(2, 'deploy-456:provision-reasons:local-authority:TST:conservation-area') + expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( + 2, + 'deploy-456:provision-reasons:local-authority:TST:conservation-area', + 300, + JSON.stringify(['statutory']) + ) + expect(mockDatasette.default.runQuery).toHaveBeenCalledTimes(2) + } finally { + process.env.DEPLOY_TIME = originalDeployTime + vi.unmock('redis') + vi.unmock('../../config/index.js') + vi.unmock('../../src/services/datasette.js') + vi.resetModules() + } + }) + + it('should return cached dataset fields and provision reasons without querying Datasette', async () => { + const mockRedisClient = { + isOpen: false, + connect: vi.fn().mockImplementation(async () => { + mockRedisClient.isOpen = true + }), + get: vi.fn() + .mockResolvedValueOnce(JSON.stringify(['cached-field'])) + .mockResolvedValueOnce(JSON.stringify(['expected'])), + setEx: vi.fn().mockResolvedValue() + } + const mockDatasette = { + default: { + runQuery: vi.fn() + } + } + + try { + vi.resetModules() + vi.doMock('redis', () => ({ + createClient: vi.fn(() => mockRedisClient) + })) + vi.doMock('../../config/index.js', () => ({ + default: { + redis: { + secure: false, + host: 'localhost', + port: 6379 + }, + mainWebsiteUrl: config.mainWebsiteUrl + } + })) + vi.doMock('../../src/services/datasette.js', () => mockDatasette) + + const { + getDatasetFields, + getProvisionReasonsForDataset + } = await import('../../src/utils/redisLoader.js') + + await expect(getDatasetFields('conservation-area')).resolves.toEqual(['cached-field']) + await expect(getProvisionReasonsForDataset({ + organisation: 'local-authority:TST', + dataset: 'conservation-area' + })).resolves.toEqual(['expected']) + expect(mockDatasette.default.runQuery).not.toHaveBeenCalled() + } finally { + vi.unmock('redis') + vi.unmock('../../config/index.js') + vi.unmock('../../src/services/datasette.js') + vi.resetModules() + } + }) + + it('should normalise duplicate, empty and system dataset fields', () => { + expect(normaliseDatasetFields([ + { field: 'reference' }, + { field: 'reference' }, + { field: 'organisation' }, + { field: 'entity' }, + { field: '' }, + {} + ])).toEqual(['reference']) + }) }) diff --git a/test/unit/statusController.test.js b/test/unit/statusController.test.js index d5ceaea50..e0197b4a2 100644 --- a/test/unit/statusController.test.js +++ b/test/unit/statusController.test.js @@ -1,14 +1,18 @@ -import StatusController from '../../src/controllers/statusController.js' import { describe, it, vi, expect, beforeEach } from 'vitest' +import StatusController, { shouldShowColumnMapping } from '../../src/controllers/statusController.js' +import { getDatasetFields, isStatutoryDataset } from '../../src/utils/redisLoader.js' -describe('StatusController', () => { - vi.mock('@/services/asyncRequestApi.js') +vi.mock('@/services/asyncRequestApi.js') +vi.mock('../../src/utils/redisLoader.js') +describe('StatusController', () => { let asyncRequestApi let statusController beforeEach(async () => { asyncRequestApi = await import('@/services/asyncRequestApi') + vi.mocked(getDatasetFields).mockResolvedValue(['name', 'reference']) + vi.mocked(isStatutoryDataset).mockResolvedValue(false) statusController = new StatusController({ route: '/status' @@ -40,4 +44,118 @@ describe('StatusController', () => { expect(req.form.options.data).toBe(mockResult) }) }) + + describe('post', () => { + it('redirects to column mapping when columns need mapping', async () => { + asyncRequestApi.getRequestData = vi.fn().mockResolvedValue({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ field: 'reference', missing: true }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Ref: 'abc' } }] + }) + }) + + const req = { params: { id: '123' } } + const res = { redirect: vi.fn() } + const next = vi.fn() + + await statusController.post(req, res, next) + + expect(res.redirect).toHaveBeenCalledWith('/check/column-mapping/123') + }) + + it('redirects to results when all columns are mapped', async () => { + asyncRequestApi.getRequestData = vi.fn().mockResolvedValue({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Reference', field: 'reference' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Reference: 'abc' } }] + }) + }) + + const req = { params: { id: '123' } } + const res = { redirect: vi.fn() } + const next = vi.fn() + + await statusController.post(req, res, next) + + expect(res.redirect).toHaveBeenCalledWith('/check/results/123/1') + }) + }) + + describe('shouldShowColumnMapping', () => { + it('returns true when expected fields are unmapped and unmapped headings are available', async () => { + await expect(shouldShowColumnMapping({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Name: 'Name', Ref: 'abc' } }] + }) + })).resolves.toBe(true) + }) + + it('returns false for statutory datasets', async () => { + vi.mocked(isStatutoryDataset).mockResolvedValue(true) + + await expect(shouldShowColumnMapping({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Name: 'Name', Ref: 'abc' } }] + }) + })).resolves.toBe(false) + }) + + it('returns false when expected fields are unmapped but no unmapped headings are available', async () => { + await expect(shouldShowColumnMapping({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Name: 'abc' } }] + }) + })).resolves.toBe(false) + }) + + it('returns false when all expected fields are mapped', async () => { + await expect(shouldShowColumnMapping({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Reference', field: 'reference' }, { column: 'Name', field: 'name' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ converted_row: { Reference: 'abc', Name: 'Name', Extra: 'extra' } }] + }) + })).resolves.toBe(false) + }) + + it('returns false when there are other blocking external errors', async () => { + await expect(shouldShowColumnMapping({ + isComplete: () => true, + isFailed: () => false, + getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), + getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => [{ + converted_row: { Name: 'Name', Ref: 'abc' }, + issue_logs: [{ + severity: 'error', + responsibility: 'external', + field: 'geometry', + 'issue-type': 'invalid geometry' + }] + }] + }) + })).resolves.toBe(false) + }) + }) }) diff --git a/test/unit/views/check/columnMapping.test.js b/test/unit/views/check/columnMapping.test.js new file mode 100644 index 000000000..35418a81f --- /dev/null +++ b/test/unit/views/check/columnMapping.test.js @@ -0,0 +1,169 @@ +import { describe, expect, it } from 'vitest' +import { JSDOM } from 'jsdom' +import { setupNunjucks } from '../../../../src/serverSetup/nunjucks.js' + +const nunjucks = setupNunjucks({ datasetNameMapping: new Map() }) + +describe('check/column-mapping.html', () => { + it('renders mapping rows and submit actions', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'reference', + column: 'Reference', + isMapped: true, + isEditable: false, + userDefined: false + }, + { + field: 'notes', + column: '', + isMapped: false, + isEditable: true, + userDefined: false + } + ], + uploadedColumns: ['Name', 'Reference'], + columnMappingErrors: { + notes: { + text: 'Select the notes field' + } + } + } + }) + + const document = new JSDOM(html, { url: 'http://localhost' }).window.document + + expect(document.querySelector('th').textContent).toContain('Expected fields') + expect(document.querySelector('td strong').textContent).toBe('reference') + expect(document.querySelectorAll('td')[1].textContent).toContain('Reference') + expect(document.querySelector('td').getAttribute('style')).toContain('vertical-align: middle') + expect(document.querySelector('.govuk-error-summary').textContent).toContain('Select the notes field') + expect(document.querySelector('.govuk-error-summary a').getAttribute('href')).toBe('#fieldMap-1') + expect(document.querySelector('.govuk-error-message').textContent).toContain('Select the notes field') + expect(document.querySelector('.govuk-form-group').className).toContain('govuk-!-margin-bottom-0') + expect(document.querySelector('tr.govuk-form-group--error').textContent).toContain('notes') + expect(document.querySelector('.govuk-form-group').className).not.toContain('govuk-form-group--error') + expect(document.querySelector('button[form="columnMappingForm"]').textContent).toContain('Check your data') + }) + + it('does not render an error summary when there are no mapping errors', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'notes', + column: '', + isMapped: false, + userDefined: false + } + ], + uploadedColumns: ['Name', 'Reference'], + columnMappingErrors: {} + } + }) + + const document = new JSDOM(html, { url: 'http://localhost' }).window.document + + expect(document.querySelector('.govuk-error-summary')).toBeNull() + expect(document.querySelector('.govuk-error-message')).toBeNull() + }) + + it('does not show not provided for required fields', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'reference', + column: '', + isMapped: false, + isEditable: true, + userDefined: false, + isRequired: true + } + ], + uploadedColumns: ['Reference'], + columnMappingErrors: {} + } + }) + + const document = new JSDOM(html, { url: 'https://example.test/' }).window.document + + expect(document.querySelector('option[value="na"]')).toBeNull() + }) + + it('preselects user-defined mapped fields in the selector', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'notes', + column: 'Shape__Area', + isMapped: true, + userDefined: true, + isEditable: true, + isRequired: false + } + ], + uploadedColumns: ['Shape__Area', 'Reference'], + columnMappingErrors: {} + } + }) + + const document = new JSDOM(html, { url: 'https://example.test/' }).window.document + const fieldSelect = document.querySelector('select[name="fieldMap[notes]"]') + + expect(fieldSelect).not.toBeNull() + expect(fieldSelect.value).toBe('Shape__Area') + expect(document.querySelectorAll('td')[2].textContent).toContain('Unmapped') + }) + + it('shows not provided for optional fields', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'notes', + column: '', + isMapped: false, + userDefined: false, + isEditable: true, + isRequired: false + } + ], + uploadedColumns: ['Notes'], + columnMappingErrors: {} + } + }) + + const document = new JSDOM(html).window.document + + expect(document.querySelector('option[value="na"]').textContent).toContain('Not provided') + }) +}) From e3c54f18c8380accd83a75ae5d6de23b0690e93a Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Wed, 27 May 2026 16:31:03 +0100 Subject: [PATCH 02/21] fixed nitpix --- src/assets/scss/index.scss | 17 +++++++++++++++++ src/controllers/columnMappingController.js | 6 +++++- src/controllers/resultsController.js | 6 ++---- src/utils/redisLoader.js | 2 +- src/views/check/column-mapping.html | 2 +- src/views/check/components/column-mapping.html | 2 +- test/unit/redisLoader.test.js | 6 +++--- test/unit/views/check/columnMapping.test.js | 7 +++++-- 8 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/assets/scss/index.scss b/src/assets/scss/index.scss index 2783428a9..95cbd7998 100644 --- a/src/assets/scss/index.scss +++ b/src/assets/scss/index.scss @@ -265,6 +265,23 @@ code * { } } +.column-mapping-panel { + background-color: govuk-colour("light-grey"); + padding: 15px 20px; +} + +.column-mapping__unmap-select { + background-color: govuk-colour("green"); + color: govuk-colour("white"); + font-weight: bold; + font-size: 0.875rem; + padding: 2px 8px; + border: none; + border-radius: 0; + cursor: pointer; + appearance: auto; +} + .app-content__markdown img { max-width: 100%; } diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index 3efc41c84..f5904bb49 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -174,7 +174,11 @@ export function applySubmittedFieldSelections (mappingRows = [], body = {}) { export function buildSubmittedColumnMapping ({ existingMapping = {}, body = {}, spareUploadedColumns = [] }) { const columnMapping = { ...existingMapping } - const columns = Array.isArray(body.columns) ? body.columns : [body.columns].filter(Boolean) + const columns = Array.isArray(body.columns) + ? body.columns + : body.columns + ? [body.columns] + : [] const selectedUploadedColumns = new Set() columns.forEach((column, index) => { diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index f2b2569cb..97c1de8fb 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -236,10 +236,8 @@ export function setupTableParams (req, res, next) { return { mappedField, col } }) .filter(({ mappedField, col }) => { - // skip empty values and explicit IGNORE mappings - if (!mappedField || !col) return false - if (typeof mappedField === 'string' && mappedField.toUpperCase() === 'IGNORE') return false - return true + const normalizedMappedField = typeof mappedField === 'string' ? mappedField.toUpperCase() : mappedField + return Boolean(mappedField && col && normalizedMappedField !== 'IGNORE') }) .map(({ mappedField, col }) => ({ key: { text: mappedField }, diff --git a/src/utils/redisLoader.js b/src/utils/redisLoader.js index b851be3eb..7c208cdc4 100644 --- a/src/utils/redisLoader.js +++ b/src/utils/redisLoader.js @@ -94,7 +94,7 @@ export async function getDatasetFields (dataset) { const query = `select field from dataset_field where dataset = '${escapeSqlString(dataset)}'` const response = await datasette.runQuery(query) - const fields = normaliseDatasetFields(response.formattedData) + const fields = normaliseDatasetFields(response.formattedData, dataset) await setCachedJson(key, fields, 'getDatasetFields') diff --git a/src/views/check/column-mapping.html b/src/views/check/column-mapping.html index f9e43a6bb..cbad43b06 100644 --- a/src/views/check/column-mapping.html +++ b/src/views/check/column-mapping.html @@ -40,7 +40,7 @@

{{ pageName }}

Review how columns from your data match planning data fields. If a column is mapped incorrectly, choose a different field and re-check the data.

-
+
{{ columnMapping(options.mappingRows, options.uploadedColumns, options.columnMappingErrors,false) }}
diff --git a/src/views/check/components/column-mapping.html b/src/views/check/components/column-mapping.html index 19195cb3e..da5422721 100644 --- a/src/views/check/components/column-mapping.html +++ b/src/views/check/components/column-mapping.html @@ -77,7 +77,7 @@ {% if not isEditable %} {% if allowUnmapping %} - diff --git a/test/unit/redisLoader.test.js b/test/unit/redisLoader.test.js index 683556874..4f831af3f 100644 --- a/test/unit/redisLoader.test.js +++ b/test/unit/redisLoader.test.js @@ -92,7 +92,7 @@ describe('getDatasetNameMap', () => { expect(mockRedisClient.get).toHaveBeenCalledWith('deploy-123:dataset:example-dataset') expect(mockRedisClient.setEx).toHaveBeenCalledWith( 'deploy-123:dataset:example-dataset', - 300, + 21600, JSON.stringify({ 'example-dataset': 'Example dataset' }) ) } finally { @@ -173,14 +173,14 @@ describe('getDatasetNameMap', () => { expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( 1, 'deploy-456:dataset-fields:conservation-area', - 300, + 21600, JSON.stringify(['name', 'notes', 'reference']) ) expect(mockRedisClient.get).toHaveBeenNthCalledWith(2, 'deploy-456:provision-reasons:local-authority:TST:conservation-area') expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( 2, 'deploy-456:provision-reasons:local-authority:TST:conservation-area', - 300, + 21600, JSON.stringify(['statutory']) ) expect(mockDatasette.default.runQuery).toHaveBeenCalledTimes(2) diff --git a/test/unit/views/check/columnMapping.test.js b/test/unit/views/check/columnMapping.test.js index 35418a81f..fc9588b71 100644 --- a/test/unit/views/check/columnMapping.test.js +++ b/test/unit/views/check/columnMapping.test.js @@ -39,16 +39,19 @@ describe('check/column-mapping.html', () => { }) const document = new JSDOM(html, { url: 'http://localhost' }).window.document + const notesSelect = document.querySelector('select[name="fieldMap[notes]"]') + const notesRow = notesSelect.closest('tr') expect(document.querySelector('th').textContent).toContain('Expected fields') expect(document.querySelector('td strong').textContent).toBe('reference') expect(document.querySelectorAll('td')[1].textContent).toContain('Reference') - expect(document.querySelector('td').getAttribute('style')).toContain('vertical-align: middle') + expect(document.defaultView.getComputedStyle(document.querySelector('td')).verticalAlign).toBe('middle') expect(document.querySelector('.govuk-error-summary').textContent).toContain('Select the notes field') expect(document.querySelector('.govuk-error-summary a').getAttribute('href')).toBe('#fieldMap-1') expect(document.querySelector('.govuk-error-message').textContent).toContain('Select the notes field') expect(document.querySelector('.govuk-form-group').className).toContain('govuk-!-margin-bottom-0') - expect(document.querySelector('tr.govuk-form-group--error').textContent).toContain('notes') + expect(notesRow.className).toContain('govuk-form-group--error') + expect(notesRow.textContent).toContain('notes') expect(document.querySelector('.govuk-form-group').className).not.toContain('govuk-form-group--error') expect(document.querySelector('button[form="columnMappingForm"]').textContent).toContain('Check your data') }) From 6573aa53d08b071955be2909cad210406be85cd4 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Thu, 28 May 2026 11:10:43 +0100 Subject: [PATCH 03/21] fixed issue with missing column --- src/controllers/statusController.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index 3eb84e365..b01e4bfbf 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -77,15 +77,25 @@ export async function shouldShowColumnMapping (requestData) { if (expectedFields.length === 0) return false const columnFieldLog = requestData.getColumnFieldLog?.() ?? [] + + // the column mapping the user has done const userColumnMapping = params.column_mapping ?? {} const responseDetails = await requestData.fetchResponseDetails(0, 50) const rows = responseDetails.getRows?.() ?? [] - + // all the columns the user has mapped. const mappedFields = buildMappedFields(columnFieldLog, userColumnMapping) + + // all the fields in the dataset that has not been mapped const unmappedExpectedFields = new Set(expectedFields.filter(field => !mappedFields.has(field))) const hasUnmappedExpectedFields = unmappedExpectedFields.size > 0 if (!hasUnmappedExpectedFields) return false + const spareUploadedColumns = buildSpareUploadedColumns(columnFieldLog, rows, userColumnMapping) + + if (spareUploadedColumns.length === 0) return false + + if (columnFieldLog.some(column => column?.missing)) return true + // If there are other blocking external errors, users should resolve those in // results instead of being redirected to column-mapping. const hasOtherBlockingExternalErrors = rows.some(row => @@ -95,10 +105,7 @@ export async function shouldShowColumnMapping (requestData) { !unmappedExpectedFields.has(issue?.field) ) ) - if (hasOtherBlockingExternalErrors) return false - - const spareUploadedColumns = buildSpareUploadedColumns(columnFieldLog, rows, userColumnMapping) - return spareUploadedColumns.length > 0 + return !hasOtherBlockingExternalErrors } catch { return false } From d57d7bc99962c3087ee47bae94eaff22b9dc2838 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Fri, 29 May 2026 03:27:11 +0100 Subject: [PATCH 04/21] feat: enhance column mapping functionality and improve dataset handling - Refactored StatusController to include middleware for fetching dataset information from platform API. - Updated shouldShowColumnMapping function to consider unique dataset fields and handle geometry/point field logic. - Removed unused getDatasetFields function and related normalisation logic from redisLoader. - Modified requestData model to include mandatory field information in column mapping. - Improved column mapping HTML to provide clearer user instructions and handle user ignored fields. - Enhanced unit tests for StatusController and column mapping to cover new functionality and edge cases. --- config/default.yaml | 98 +-------- src/controllers/columnMappingController.js | 208 +++++++++++------- src/controllers/statusController.js | 101 +++++++-- src/models/requestData.js | 9 +- src/utils/redisLoader.js | 33 --- src/views/check/column-mapping.html | 7 +- .../check/components/column-mapping.html | 9 +- test/unit/columnMappingController.test.js | 101 ++++++--- test/unit/redisLoader.test.js | 50 +---- test/unit/requestData.test.js | 14 +- test/unit/statusController.test.js | 118 +++++----- test/unit/views/check/columnMapping.test.js | 35 ++- 12 files changed, 417 insertions(+), 366 deletions(-) diff --git a/config/default.yaml b/config/default.yaml index e553e842f..180d4afa3 100644 --- a/config/default.yaml +++ b/config/default.yaml @@ -56,181 +56,85 @@ datasetsConfig: entityDisplayName: base: article 4 variable: direction - requiredFields: - - reference - - name - - document-url - - documentation-url article-4-direction-area: guidanceUrl: /guidance/specifications/article-4-direction entityDisplayName: base: article 4 direction variable: area - requiredFields: - - reference - - geometry - - name - - permitted-development-rights - - article-4-direction brownfield-land: guidanceUrl: https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data entityDisplayName: base: brownfield land variable: site - requiredFields: - - OrganisationURI - - SiteReference - - SiteNameAddress - - GeoX - - GeoY conservation-area: guidanceUrl: /guidance/specifications/conservation-area entityDisplayName: base: conservation variable: area - requiredFields: - - reference - - geometry - - name conservation-area-document: guidanceUrl: /guidance/specifications/conservation-area entityDisplayName: base: conservation variable: area - requiredFields: - - reference - - name - - conservation-area - - document-url - - documentation-url - - document-type tree-preservation-order: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: base: tree preservation variable: order - requiredFields: - - reference - - document-url - - documentation-url tree-preservation-zone: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: base: tree preservation variable: zone - requiredFields: - - reference - - geometry - - tree-preservation-order tree: guidanceUrl: /guidance/specifications/tree-preservation-order entityDisplayName: variable: tree - requiredFields: - - reference - - tree-preservation-order - - point - - geometry listed-building-outline: guidanceUrl: /guidance/specifications/listed-building entityDisplayName: base: listed building variable: outline - requiredFields: - - reference - - geometry - - name - - listed-building developer-agreement-transaction: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer agreement variable: transaction - requiredFields: - - reference developer-agreement: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer variable: agreement - requiredFields: - - reference developer-agreement-contribution: guidanceUrl: https://www.gov.uk/guidance/publish-your-developer-contributions-data entityDisplayName: base: developer agreement variable: contribution - requiredFields: - - reference infrastructure-funding-statement: guidanceUrl: https://digital-land.github.io/specification/specification/infrastructure-funding-statement/ entityDisplayName: base: infrastructure funding variable: statement - requiredFields: - - reference plan-timetable: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: plan variable: timetable - requiredFields: - - reference - - plan - - plan-event - - event-date - - entry-date local-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: local variable: plan - requiredFields: - - reference - - name - - description - - dataset - - period-start-date - - period-end-date - - local-planning-authorities - - documentation-url - - document-url - - required-housing - - entry-date minerals-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: minerals variable: plan - requiredFields: - - reference - - name - - description - - dataset - - period-start-date - - period-end-date - - minerals-and-waste-planning-authorities - - documentation-url - - document-url - - document-count - - entry-date waste-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: waste variable: plan - requiredFields: - - reference - - name - - description - - dataset - - period-start-date - - period-end-date - - minerals-and-waste-planning-authorities - - documentation-url - - document-url - - document-count - - entry-date supplementary-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: @@ -270,4 +174,4 @@ features: nonAuthPages: # This feature enables pages to show non-authoritative datasets # If off, it will still check for local-plan datasets. - enabled: true + enabled: true \ No newline at end of file diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index f5904bb49..f74393b96 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -1,8 +1,11 @@ import PageController from './pageController.js' -import config from '../../config/index.js' import { postCheckRequest } from '../services/asyncRequestApi.js' -import { getDatasetFields, isStatutoryDataset } from '../utils/redisLoader.js' +import { isStatutoryDataset } from '../utils/redisLoader.js' import { getRequestDataMiddleware, updateSessionFromRequestData } from './resultsController.js' +import { processSpecificationMiddlewares } from '../middleware/common.middleware.js' +import platformApi from '../services/platformApi.js' +import { types } from '../utils/logging.js' +import logger from '../utils/logger.js' class ColumnMappingController extends PageController { middlewareSetup () { @@ -17,6 +20,33 @@ class ColumnMappingController extends PageController { next() }) this.use(updateSessionFromRequestData) + // Populate req.params and dataset, then run specification processing middlewares + this.use(async (req, res, next) => { + const { requestData } = req.locals + const params = requestData?.getParams() ?? {} + try { + const { formattedData } = await platformApi.fetchDatasets({ dataset: params.dataset }) + // Bounds check TODO move to external bounds handling as in fetchOne + if (!formattedData || formattedData.length === 0) { + const error = new Error(`Dataset not found: ${req.params.dataset}`) + logger.warn('fetchDatasetPlatformInfo: no dataset returned', { type: types.App, dataset: req.params.dataset }) + return next(error) + } + const datasetInfo = formattedData[0] + req.dataset = { + collection: datasetInfo.collection, + name: datasetInfo.name, + dataset: datasetInfo.dataset, + typology: datasetInfo.typology + } + } catch (error) { + logger.warn('fetchDatasetPlatformInfo failed', { type: types.App, errorMessage: error.message, errorStack: error.stack }) + return next(error) + } + return next() + }) + // attach the standard specification processing middleware chain + processSpecificationMiddlewares.forEach(mw => this.use(mw)) } async locals (req, res, next) { @@ -28,10 +58,11 @@ class ColumnMappingController extends PageController { res.redirect(`/check/results/${req.params.id}/1`) return } - + const uniqueDatasetFields = req.uniqueDatasetFields || [] Object.assign(req.form.options, await buildColumnMappingOptions({ requestData, - requestId: req.params.id + requestId: req.params.id, + uniqueDatasetFields })) super.locals(req, res, next) } catch (error) { @@ -47,7 +78,8 @@ class ColumnMappingController extends PageController { const options = await buildColumnMappingOptions({ requestData, requestId: req.params.id, - body: req.body + body: req.body, + uniqueDatasetFields: req.uniqueDatasetFields || [] }) const validationErrors = validateColumnMapping(req.body, options.mappingRows) if (Object.keys(validationErrors).length > 0) { @@ -63,7 +95,8 @@ class ColumnMappingController extends PageController { } const params = requestData.getParams() ?? {} - const { columnMappingRows } = await prepareColumnMappingContext(requestData) + const uniqueDatasetFields = req.uniqueDatasetFields || [] + const { columnMappingRows } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) const spareUploadedColumns = buildSelectableColumns(columnMappingRows) const columnMapping = buildSubmittedColumnMapping({ existingMapping: params.column_mapping, @@ -89,12 +122,12 @@ class ColumnMappingController extends PageController { * Returns UI-friendly data including expected mapping rows, selectable uploaded * columns and any validation errors so the view can render the mapping form. */ -async function buildColumnMappingOptions ({ requestData, requestId, body = {}, validationErrors = {} }) { +async function buildColumnMappingOptions ({ requestData, requestId, body = {}, validationErrors = {}, uniqueDatasetFields = [] }) { const { columnMappingRows, specFields, requiredFields - } = await prepareColumnMappingContext(requestData) + } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) const mappingRows = buildExpectedFieldRows({ columnMappingRows, specFields, @@ -103,11 +136,12 @@ async function buildColumnMappingOptions ({ requestData, requestId, body = {}, v applySubmittedFieldSelections(mappingRows, body) + const responseDetails = await requestData.fetchResponseDetails(0, 50) return { id: requestId, requestParams: requestData.getParams(), mappingRows, - uploadedColumns: buildSelectableColumns(columnMappingRows), + uploadedColumns: buildSelectableColumns(columnMappingRows, responseDetails.getRows()), columnMappingErrors: validationErrors, lastPage: `/check/status/${requestId}` } @@ -120,18 +154,20 @@ async function buildColumnMappingOptions ({ requestData, requestId, body = {}, v * - Builds `columnMappingRows` (combined auto-detected + user overrides). * - Resolves `specFields` from the dataset and `requiredFields` from config. */ -async function prepareColumnMappingContext (requestData) { - const responseDetails = await requestData.fetchResponseDetails(0, 50) - const columnFieldLog = requestData.getColumnFieldLog() +async function prepareColumnMappingContext (requestData, uniqueDatasetFields = []) { + let columnFieldLog = requestData.getColumnFieldLog() + if (uniqueDatasetFields.length > 0) { + columnFieldLog = columnFieldLog.filter(entry => uniqueDatasetFields.includes(entry?.field)) + } const params = requestData.getParams() ?? {} const userColumnMapping = params.column_mapping ?? {} const columnMappingRows = buildColumnMappingRows({ columnFieldLog, - responseRows: responseDetails.getRows(), userColumnMapping }) - const specFields = buildSpecFields(await getDatasetFields(params.dataset)) - const requiredFields = config.datasetsConfig?.[params.dataset]?.requiredFields ?? [] + + const specFields = buildSpecFields(columnFieldLog.map(entry => entry?.field).filter(Boolean)) + const requiredFields = columnFieldLog.filter(entry => entry?.mandatory).map(entry => entry.field) return { columnMappingRows, specFields, @@ -154,20 +190,43 @@ async function getCompletedRequestData (req, res) { export function validateColumnMapping (body = {}, mappingRows = []) { const requiredFields = new Set(mappingRows.filter(row => row.isRequired).map(row => row.field)) - return Object.fromEntries( - Object.entries(getBracketFields(body, 'fieldMap')) + const fieldMap = getBracketFields(body, 'fieldMap') + + // base errors: missing or explicit 'na' for required fields + const errors = Object.fromEntries( + Object.entries(fieldMap) .filter(([field, value]) => value === '' || (value === 'na' && requiredFields.has(field))) .map(([field]) => [field, { text: `Select the ${field} field` }]) ) + + // check for duplicate selections (same column selected for multiple fields) + const selections = Object.entries(fieldMap) + .map(([field, value]) => [field, (value ?? '').trim()]) + .filter(([, col]) => col && col !== 'na') + + const counts = selections.reduce((acc, [, col]) => { + acc[col] = (acc[col] || 0) + 1 + return acc + }, {}) + + for (const [field, col] of selections) { + if (counts[col] > 1 && !errors[field]) { + errors[field] = { text: `${col} has been selected more than once` } + } + } + + return errors } export function applySubmittedFieldSelections (mappingRows = [], body = {}) { const fieldMap = getBracketFields(body, 'fieldMap') mappingRows.forEach(row => { if (Object.hasOwn(fieldMap, row.field)) { - row.column = fieldMap[row.field] + const selectedColumn = (fieldMap[row.field] ?? '').trim() + row.userIgnored = selectedColumn === 'na' + row.column = row.userIgnored ? '' : selectedColumn } }) } @@ -251,33 +310,19 @@ export function buildSpecFields (datasetFields = []) { return [...fields].sort() } -/** - * Build the canonical list of column mapping rows. - * - * Each row describes an uploaded column and how it relates to a spec field: - * - `column`: uploaded column name (empty string for missing auto-detected fields) - * - `field`: mapped spec field (if any) - * - `isMapped`, `isAutoMapped`, `isMissing`, `userDefined`, `userIgnored` - * - * @param {Object} options - * @param {Array} options.columnFieldLog - auto-detected column->field log entries - * @param {Array} options.responseRows - sample response rows with `converted_row` keys - * @param {Object} options.userColumnMapping - existing user mapping overrides - * @returns {Array} rows suitable for UI consumption - */ -export function buildColumnMappingRows ({ columnFieldLog = [], responseRows = [], userColumnMapping = {} }) { - const mappedColumns = new Set() +export function buildColumnMappingRows ({ columnFieldLog = [], userColumnMapping = {} }) { + const entries = columnFieldLog const rows = [] - columnFieldLog.forEach(entry => { + entries.forEach(entry => { const column = entry?.column if (!column) { - if (entry?.field && entry?.missing) { + if (entry?.field) { rows.push({ column: '', field: entry.field, isMapped: false, - isMissing: true, + isMissing: entry.missing, userDefined: false, userIgnored: false }) @@ -285,70 +330,77 @@ export function buildColumnMappingRows ({ columnFieldLog = [], responseRows = [] return } - mappedColumns.add(column) const userMappedField = userColumnMapping[column] - const userIgnored = userMappedField === 'IGNORE' - const field = userIgnored ? '' : userMappedField || entry.field || '' - + const field = entry.field + const isMapped = Boolean(field) && Boolean(column) rows.push({ column, field, - isMapped: Boolean(field) && !entry.missing, - isAutoMapped: Boolean(entry.field) && !userMappedField && !entry.missing, - isMissing: Boolean(entry.missing), - userDefined: Boolean(userMappedField) && !userIgnored, - userIgnored - }) - }) - - const unmappedColumns = new Set() - responseRows.forEach(row => { - Object.keys(row?.converted_row ?? {}).forEach(column => { - if (!mappedColumns.has(column)) unmappedColumns.add(column) + isMapped, + isAutoMapped: isMapped && !userMappedField, + isMissing: entry.missing, + userDefined: Boolean(userMappedField) }) }) - const sortedUnmappedColumns = [...unmappedColumns].sort() - sortedUnmappedColumns.forEach(column => { - rows.push({ - column, - field: userColumnMapping[column] === 'IGNORE' ? '' : userColumnMapping[column] || '', - isMapped: Boolean(userColumnMapping[column]) && userColumnMapping[column] !== 'IGNORE', - isAutoMapped: false, - isMissing: false, - userDefined: Boolean(userColumnMapping[column]) && userColumnMapping[column] !== 'IGNORE', - userIgnored: userColumnMapping[column] === 'IGNORE' + if (Object.keys(userColumnMapping).length > 0) { + rows.forEach(row => { + if (!row.column) { + row.userIgnored = true + } }) - }) + } return rows } -export function buildSelectableColumns (columnMappingRows = []) { - return [...new Set( - columnMappingRows - .filter(row => row.userDefined || !row.isMapped) - .map(row => row.column) - .filter(Boolean) - )].sort() +// selectable columns are converted rows that have not been auto-mapped by the system +export function buildSelectableColumns (columnMappingRows = [], responseRows = []) { + const autoMappedColumns = new Set(columnMappingRows.filter(row => row.isAutoMapped).map(row => row.column).filter(Boolean)) + const unmappedColumns = new Set() + responseRows.forEach(row => { + Object.keys(row?.converted_row ?? {}).forEach(column => { + if (!autoMappedColumns.has(column)) unmappedColumns.add(column) + }) + }) + return [...unmappedColumns].sort() } export function buildExpectedFieldRows ({ columnMappingRows = [], specFields = [], requiredFields = [] }) { const requiredFieldSet = new Set(requiredFields) - return specFields.map(field => { - const row = columnMappingRows.find(row => row.field === field && row.isMapped && !row.userIgnored) - const isAutoMapped = Boolean(row?.isMapped) && !row?.userDefined + let rows = specFields.map(field => { + const mappedRow = columnMappingRows.find(row => row.field === field && row.isMapped && !row.userIgnored) + const ignoredRow = columnMappingRows.find(row => row.field === field && row.userIgnored) + + const isAutoMapped = Boolean(mappedRow?.isMapped) && !mappedRow?.userDefined return { field, - column: row?.column ?? '', - isMapped: Boolean(row?.column), + column: mappedRow?.column ?? '', + isMapped: Boolean(mappedRow?.column), isAutoMapped, - userDefined: Boolean(row?.userDefined), // if the user has explicitly mapped this field + userDefined: Boolean(mappedRow?.userDefined), // if the user has explicitly mapped this field + userIgnored: Boolean(ignoredRow), isEditable: !isAutoMapped, // shows as dropdown in the UI and also shows as an Unmapped badge isRequired: requiredFieldSet.has(field) } - }).sort((a, b) => { + }) + + // Business rule: geometry and point are mutually exclusive when one is already mapped. + // - If `geometry` is mapped and `point` is not, hide the `point` option. + // - If `point` is mapped and `geometry` is not, hide the `geometry` option. + // - If both are mapped or both are unmapped, leave both rows as-is. + const geometryRow = rows.find(r => r.field === 'geometry') + const pointRow = rows.find(r => r.field === 'point') + const geometryMapped = Boolean(geometryRow && geometryRow.isAutoMapped) + const pointMapped = Boolean(pointRow && pointRow.isAutoMapped) + if (geometryMapped && !pointMapped) { + rows = rows.filter(r => r.field !== 'point') + } else if (pointMapped && !geometryMapped) { + rows = rows.filter(r => r.field !== 'geometry') + } + + return rows.sort((a, b) => { const rank = (row) => { if (row.isAutoMapped && row.isRequired) return 0 if (row.isAutoMapped && !row.isRequired) return 1 diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index b01e4bfbf..f06e73b25 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -2,7 +2,11 @@ import PageController from './pageController.js' import { getRequestData } from '../services/asyncRequestApi.js' import { finishedProcessingStatuses } from '../utils/utils.js' import { headingTexts, messageTexts, buttonTexts, buttonAriaLabels } from '../content/statusPage.js' -import { getDatasetFields, isStatutoryDataset } from '../utils/redisLoader.js' +import { isStatutoryDataset } from '../utils/redisLoader.js' +import platformApi from '../services/platformApi.js' +import logger from '../utils/logger.js' +import { types } from '../utils/logging.js' +import { processSpecificationMiddlewares } from '../middleware/common.middleware.js' /** * Attempts to infer how we ended up on this page. @@ -21,10 +25,42 @@ function getLastPage (req) { } class StatusController extends PageController { + middlewareSetup () { + super.middlewareSetup() + // Populate req.params and dataset, then run specification processing middlewares + this.use(async (req, res, next) => { + const requestData = await getRequestData(req.params.id) + const params = requestData?.getParams() ?? {} + try { + const { formattedData } = await platformApi.fetchDatasets({ dataset: params.dataset }) + // Bounds check TODO move to external bounds handling as in fetchOne + if (!formattedData || formattedData.length === 0) { + const error = new Error(`Dataset not found: ${req.params.dataset}`) + logger.warn('fetchDatasetPlatformInfo: no dataset returned', { type: types.App, dataset: req.params.dataset }) + return next(error) + } + const datasetInfo = formattedData[0] + req.dataset = { + collection: datasetInfo.collection, + name: datasetInfo.name, + dataset: datasetInfo.dataset, + typology: datasetInfo.typology + } + } catch (error) { + logger.warn('fetchDatasetPlatformInfo failed', { type: types.App, errorMessage: error.message, errorStack: error.stack }) + return next(error) + } + return next() + }) + // attach the standard specification processing middleware chain + processSpecificationMiddlewares.forEach(mw => this.use(mw)) + } + async post (req, res, next) { try { const requestData = await getRequestData(req.params.id) - const nextStep = await shouldShowColumnMapping(requestData) + const uniqueDatasetFields = req.uniqueDatasetFields || [] + const nextStep = await shouldShowColumnMapping(requestData, uniqueDatasetFields) ? `/check/column-mapping/${req.params.id}` : `/check/results/${req.params.id}/1` @@ -58,7 +94,7 @@ class StatusController extends PageController { } } -export async function shouldShowColumnMapping (requestData) { +export async function shouldShowColumnMapping (requestData, uniqueDatasetFields = []) { if (!requestData?.isComplete?.() || requestData?.isFailed?.()) { return false } @@ -73,37 +109,55 @@ export async function shouldShowColumnMapping (requestData) { return false } - const expectedFields = await getDatasetFields(params.dataset) - if (expectedFields.length === 0) return false + let columnMapping = requestData.getColumnFieldLog?.() ?? [] - const columnFieldLog = requestData.getColumnFieldLog?.() ?? [] + if (uniqueDatasetFields.length > 0) { + columnMapping = columnMapping.filter(entry => uniqueDatasetFields.includes(entry?.field)) + } + let allFieldsInDataset = columnMapping.map(column => column?.field).filter(Boolean) + // filter out point or geometry field depending on which is present. + // If geometry is mapped, do not show point. If point is mapped, do not show geometry. + // If both are mapped, no action needed. If neither are mapped, show both options + const geometryItem = columnMapping.find(column => column?.field?.toLowerCase() === 'geometry') ?? null + const pointFieldItem = columnMapping.find(column => column?.field?.toLowerCase() === 'point') ?? null + const geometryMapped = Boolean(geometryItem?.column) + const pointMapped = Boolean(pointFieldItem?.column) + if (geometryMapped && !pointMapped) { + allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'point') + } else if (pointMapped && !geometryMapped) { + allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'geometry') + } + if (allFieldsInDataset.length === 0) return false // the column mapping the user has done const userColumnMapping = params.column_mapping ?? {} const responseDetails = await requestData.fetchResponseDetails(0, 50) const rows = responseDetails.getRows?.() ?? [] // all the columns the user has mapped. - const mappedFields = buildMappedFields(columnFieldLog, userColumnMapping) + const fieldsMappedByUser = buildMappedFields(columnMapping, userColumnMapping) // all the fields in the dataset that has not been mapped - const unmappedExpectedFields = new Set(expectedFields.filter(field => !mappedFields.has(field))) - const hasUnmappedExpectedFields = unmappedExpectedFields.size > 0 + const unmappedDatasetFields = new Set(allFieldsInDataset.filter(field => !fieldsMappedByUser.has(field))) + + const hasUnmappedExpectedFields = unmappedDatasetFields.size > 0 + // there are no dataset fields that are unmapped, so no need to show column mapping page if (!hasUnmappedExpectedFields) return false - const spareUploadedColumns = buildSpareUploadedColumns(columnFieldLog, rows, userColumnMapping) + // the uploaded columns that have not been mapped either automatically or by the user (they are available for mapping) + const spareUploadedColumns = buildSpareUploadedColumns(columnMapping, rows, userColumnMapping) + // if there are no uploaded columns that can be mapped, then there is no point showing the column mapping page if (spareUploadedColumns.length === 0) return false - if (columnFieldLog.some(column => column?.missing)) return true + // if there are missing mandatory fields that are not mapped by the user, then we should show the column mapping page + const hasMissingMandatoryFields = columnMapping.some(column => column?.mandatory && !column?.column && !fieldsMappedByUser.has(column.field)) + if (hasMissingMandatoryFields) return true - // If there are other blocking external errors, users should resolve those in - // results instead of being redirected to column-mapping. - const hasOtherBlockingExternalErrors = rows.some(row => - (row?.issue_logs ?? []).some(issue => - issue?.severity === 'error' && - issue?.responsibility === 'external' && - !unmappedExpectedFields.has(issue?.field) - ) + // if has unmapped required fields + const hasOtherBlockingExternalErrors = requestData.getIssueTasks().some(issue => + issue.severity === 'error' && + issue.responsibility === 'external' && + issue['issue-type'] !== 'missing-field' ) return !hasOtherBlockingExternalErrors } catch { @@ -111,10 +165,10 @@ export async function shouldShowColumnMapping (requestData) { } } -function buildMappedFields (columnFieldLog = [], userColumnMapping = {}) { +function buildMappedFields (columnMapping = [], userColumnMapping = {}) { const fields = new Set() - columnFieldLog.forEach(column => { + columnMapping.forEach(column => { if (column?.field && column?.column && !column?.missing) fields.add(column.field) }) @@ -125,8 +179,9 @@ function buildMappedFields (columnFieldLog = [], userColumnMapping = {}) { return fields } -function buildSpareUploadedColumns (columnFieldLog = [], rows = [], userColumnMapping = {}) { - const mappedColumns = new Set(columnFieldLog.map(column => column?.column).filter(Boolean)) +// uploaded columns that have not been mapped either automatically or by the user (they are available for mapping) +function buildSpareUploadedColumns (columnMapping = [], rows = [], userColumnMapping = {}) { + const mappedColumns = new Set(columnMapping.map(column => column?.column).filter(Boolean)) Object.entries(userColumnMapping).forEach(([column, field]) => { if (field) mappedColumns.add(column) }) diff --git a/src/models/requestData.js b/src/models/requestData.js index ee9afed82..9e233960c 100644 --- a/src/models/requestData.js +++ b/src/models/requestData.js @@ -125,7 +125,7 @@ export default class ResultData { const columnMapping = this.response.data['column-mapping'] ?? [] const taskLog = this.response.data['task-log'] ?? [] - const log = columnMapping.map(({ field, column }) => ({ field, column, missing: false })) + const log = columnMapping.map(({ field, column, mandatory }) => ({ field, column, missing: false, mandatory })) for (const task of taskLog) { if (task['task-source'] === 'column-field') { @@ -137,7 +137,12 @@ export default class ResultData { continue } if (details.field) { - log.push({ field: details.field, missing: true }) + const logEntry = log.find(entry => entry.field === details.field) + if (logEntry) { + logEntry.missing = true + } else { + log.push({ field: details.field, column: null, missing: true, mandatory: true }) + } } } } diff --git a/src/utils/redisLoader.js b/src/utils/redisLoader.js index 7c208cdc4..f83479273 100644 --- a/src/utils/redisLoader.js +++ b/src/utils/redisLoader.js @@ -38,15 +38,6 @@ export async function getRedisClient () { } const CACHE_TTL = 60 * 60 * 6 // 6 hours -const SYSTEM_FIELDS = new Set([ - 'entity', - 'prefix', - 'entry-number', - 'organisation-entity', - 'organisation', - 'IGNORE', - '' -]) function escapeSqlString (value) { return String(value).replaceAll("'", "''") @@ -77,30 +68,6 @@ async function setCachedJson (key, value, logPrefix, ttl = CACHE_TTL) { } } -export function normaliseDatasetFields (rows = [], dataset) { - return [...new Set( - rows - .map(row => row?.field) - .filter(field => field && !SYSTEM_FIELDS.has(field) && dataset !== field) - )].sort() -} - -export async function getDatasetFields (dataset) { - if (!dataset) return [] - - const key = `dataset-fields:${dataset}` - const cached = await getCachedJson(key, 'getDatasetFields') - if (cached) return cached - - const query = `select field from dataset_field where dataset = '${escapeSqlString(dataset)}'` - const response = await datasette.runQuery(query) - const fields = normaliseDatasetFields(response.formattedData, dataset) - - await setCachedJson(key, fields, 'getDatasetFields') - - return fields -} - export async function getProvisionReasonsForDataset ({ organisation, dataset }) { if (!organisation || !dataset) return [] diff --git a/src/views/check/column-mapping.html b/src/views/check/column-mapping.html index cbad43b06..7aab82858 100644 --- a/src/views/check/column-mapping.html +++ b/src/views/check/column-mapping.html @@ -37,8 +37,13 @@ }) }} {% endif %} + + + +

{{ pageName }}

-

Review how columns from your data match planning data fields. If a column is mapped incorrectly, choose a different field and re-check the data.

+

We could not map all of the fields in your data to what is in the data standard.

+

You need to select a field for all expected fields.

diff --git a/src/views/check/components/column-mapping.html b/src/views/check/components/column-mapping.html index da5422721..f7a15d2de 100644 --- a/src/views/check/components/column-mapping.html +++ b/src/views/check/components/column-mapping.html @@ -5,7 +5,7 @@ {% macro columnMapping(mappingRows, uploadedColumns=[], errors={}, allowUnmapping=true) %}
-
+
@@ -36,15 +36,14 @@ { value: "", text: "-- Select a field --", - selected: not row.column + selected: not row.column and not row.userIgnored } ] %} - {% if not row.isRequired %} {% set _ = selectItems.push({ value: "na", - text: "Not provided" + text: "Not provided", + selected: row.userIgnored }) %} - {% endif %} {% for column in uploadedColumns %} {% set _ = selectItems.push({ value: column, diff --git a/test/unit/columnMappingController.test.js b/test/unit/columnMappingController.test.js index a6f6355fc..687dacb91 100644 --- a/test/unit/columnMappingController.test.js +++ b/test/unit/columnMappingController.test.js @@ -14,11 +14,9 @@ describe('columnMappingController helpers', () => { it('builds rows from mapped, missing and unmapped columns', () => { const rows = buildColumnMappingRows({ columnFieldLog: [ - { column: 'Reference', field: 'reference' }, - { column: 'Start', field: 'start-date', missing: true } - ], - responseRows: [ - { converted_row: { Reference: 'abc', Name: 'Test name' } } + { column: 'Reference', field: 'reference', mandatory: true, missing: false }, + { column: 'Name', field: 'name', mandatory: false, missing: false }, + { field: 'start-date', mandatory: false, missing: true } ], userColumnMapping: { Name: 'name' @@ -32,17 +30,7 @@ describe('columnMappingController helpers', () => { isMapped: true, isAutoMapped: true, isMissing: false, - userDefined: false, - userIgnored: false - }, - { - column: 'Start', - field: 'start-date', - isMapped: false, - isAutoMapped: false, - isMissing: true, - userDefined: false, - userIgnored: false + userDefined: false }, { column: 'Name', @@ -50,8 +38,15 @@ describe('columnMappingController helpers', () => { isMapped: true, isAutoMapped: false, isMissing: false, - userDefined: true, - userIgnored: false + userDefined: true + }, + { + column: '', + field: 'start-date', + isMapped: false, + isMissing: true, + userDefined: false, + userIgnored: true } ]) }) @@ -59,9 +54,9 @@ describe('columnMappingController helpers', () => { it('keeps missing required fields that have no uploaded column', () => { const rows = buildColumnMappingRows({ columnFieldLog: [ - { field: 'reference', missing: true } + { field: 'reference', mandatory: true, missing: true } ], - responseRows: [] + userColumnMapping: {} }) expect(rows).toEqual([ @@ -156,6 +151,7 @@ describe('columnMappingController helpers', () => { column: 'Reference', field: 'reference', isMapped: true, + isAutoMapped: true, userDefined: false, userIgnored: false }, @@ -168,7 +164,7 @@ describe('columnMappingController helpers', () => { } ] - expect(buildSelectableColumns(columnMappingRows)).toEqual(['Name']) + expect(buildSelectableColumns(columnMappingRows, [{ converted_row: { Reference: 'abc', Name: 'Test name' } }])).toEqual(['Name']) expect(buildExpectedFieldRows({ columnMappingRows, specFields: ['notes', 'reference', 'name'], @@ -181,6 +177,7 @@ describe('columnMappingController helpers', () => { isAutoMapped: true, isEditable: false, userDefined: false, + userIgnored: false, isRequired: true }, { @@ -190,6 +187,7 @@ describe('columnMappingController helpers', () => { isAutoMapped: false, isEditable: true, userDefined: true, + userIgnored: false, isRequired: false }, { @@ -199,6 +197,7 @@ describe('columnMappingController helpers', () => { isAutoMapped: false, isEditable: true, userDefined: false, + userIgnored: false, isRequired: false } ]) @@ -282,6 +281,7 @@ describe('columnMappingController helpers', () => { isAutoMapped: false, isEditable: true, userDefined: false, + userIgnored: false, isRequired: true }, { @@ -291,6 +291,36 @@ describe('columnMappingController helpers', () => { isAutoMapped: false, isEditable: true, userDefined: false, + userIgnored: false, + isRequired: false + } + ]) + }) + + it('preserves ignored fields when building expected rows', () => { + const columnMappingRows = [ + { + column: '', + field: 'notes', + isMapped: false, + userDefined: false, + userIgnored: true + } + ] + + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['notes'], + requiredFields: [] + })).toEqual([ + { + field: 'notes', + column: '', + isMapped: false, + isAutoMapped: false, + isEditable: true, + userDefined: false, + userIgnored: true, isRequired: false } ]) @@ -301,15 +331,12 @@ describe('columnMappingController helpers', () => { columnFieldLog: [ { column: 'Reference', field: 'reference' } ], - responseRows: [ - { converted_row: { Reference: 'abc', Notes: 'note', Extra: 'extra' } } - ], userColumnMapping: { Extra: 'IGNORE' } }) - expect(buildSelectableColumns(rows)).toEqual(['Extra', 'Notes']) + expect(buildSelectableColumns(rows, [{ converted_row: { Reference: 'abc', Notes: 'note', Extra: 'extra' } }])).toEqual(['Extra', 'Notes']) }) it('extracts nested and literal bracketed fields', () => { @@ -354,8 +381,8 @@ describe('columnMappingController helpers', () => { it('applies submitted selections to mapping rows for redisplay', () => { const rows = [ - { field: 'notes', column: '' }, - { field: 'reference', column: 'Reference' } + { field: 'notes', column: '', userIgnored: false }, + { field: 'reference', column: 'Reference', userIgnored: false } ] applySubmittedFieldSelections(rows, { @@ -365,8 +392,24 @@ describe('columnMappingController helpers', () => { }) expect(rows).toEqual([ - { field: 'notes', column: 'Notes' }, - { field: 'reference', column: 'Reference' } + { field: 'notes', column: 'Notes', userIgnored: false }, + { field: 'reference', column: 'Reference', userIgnored: false } + ]) + }) + + it('marks na submitted selections as user ignored for redisplay', () => { + const rows = [ + { field: 'notes', column: 'Notes', userIgnored: false } + ] + + applySubmittedFieldSelections(rows, { + fieldMap: { + notes: 'na' + } + }) + + expect(rows).toEqual([ + { field: 'notes', column: '', userIgnored: true } ]) }) diff --git a/test/unit/redisLoader.test.js b/test/unit/redisLoader.test.js index 4f831af3f..ca04522e7 100644 --- a/test/unit/redisLoader.test.js +++ b/test/unit/redisLoader.test.js @@ -1,5 +1,5 @@ import { vi, it, describe, expect, beforeEach, afterEach } from 'vitest' -import { getDatasetNameMap, normaliseDatasetFields } from '../../src/utils/redisLoader' +import { getDatasetNameMap } from '../../src/utils/redisLoader' import config from '../../config' describe('getDatasetNameMap', () => { @@ -104,7 +104,7 @@ describe('getDatasetNameMap', () => { } }) - it('should fetch dataset fields and provision reasons with namespaced cache keys', async () => { + it('should fetch provision reasons with namespaced cache keys', async () => { const originalDeployTime = process.env.DEPLOY_TIME const mockRedisClient = { isOpen: false, @@ -119,15 +119,6 @@ describe('getDatasetNameMap', () => { const mockDatasette = { default: { runQuery: vi.fn() - .mockResolvedValueOnce({ - formattedData: [ - { field: 'reference' }, - { field: 'organisation' }, - { field: 'notes' }, - { field: 'entity' }, - { field: 'name' } - ] - }) .mockResolvedValueOnce({ formattedData: [{ provision_reason: 'statutory' }] }) @@ -154,12 +145,10 @@ describe('getDatasetNameMap', () => { vi.doMock('../../src/services/datasette.js', () => mockDatasette) const { - getDatasetFields, getProvisionReasonsForDataset, isStatutoryDataset } = await import('../../src/utils/redisLoader.js') - await expect(getDatasetFields('conservation-area')).resolves.toEqual(['name', 'notes', 'reference']) await expect(getProvisionReasonsForDataset({ organisation: 'local-authority:TST', dataset: 'conservation-area' @@ -169,21 +158,14 @@ describe('getDatasetNameMap', () => { dataset: 'conservation-area' })).resolves.toBe(true) - expect(mockRedisClient.get).toHaveBeenNthCalledWith(1, 'deploy-456:dataset-fields:conservation-area') + expect(mockRedisClient.get).toHaveBeenNthCalledWith(1, 'deploy-456:provision-reasons:local-authority:TST:conservation-area') expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( 1, - 'deploy-456:dataset-fields:conservation-area', - 21600, - JSON.stringify(['name', 'notes', 'reference']) - ) - expect(mockRedisClient.get).toHaveBeenNthCalledWith(2, 'deploy-456:provision-reasons:local-authority:TST:conservation-area') - expect(mockRedisClient.setEx).toHaveBeenNthCalledWith( - 2, 'deploy-456:provision-reasons:local-authority:TST:conservation-area', 21600, JSON.stringify(['statutory']) ) - expect(mockDatasette.default.runQuery).toHaveBeenCalledTimes(2) + expect(mockDatasette.default.runQuery).toHaveBeenCalledTimes(1) } finally { process.env.DEPLOY_TIME = originalDeployTime vi.unmock('redis') @@ -193,7 +175,7 @@ describe('getDatasetNameMap', () => { } }) - it('should return cached dataset fields and provision reasons without querying Datasette', async () => { + it('should return cached provision reasons without querying Datasette', async () => { const mockRedisClient = { isOpen: false, connect: vi.fn().mockImplementation(async () => { @@ -228,15 +210,18 @@ describe('getDatasetNameMap', () => { vi.doMock('../../src/services/datasette.js', () => mockDatasette) const { - getDatasetFields, - getProvisionReasonsForDataset + getProvisionReasonsForDataset, + isStatutoryDataset } = await import('../../src/utils/redisLoader.js') - await expect(getDatasetFields('conservation-area')).resolves.toEqual(['cached-field']) await expect(getProvisionReasonsForDataset({ organisation: 'local-authority:TST', dataset: 'conservation-area' - })).resolves.toEqual(['expected']) + })).resolves.toEqual(['cached-field']) + await expect(isStatutoryDataset({ + organisation: 'local-authority:TST', + dataset: 'conservation-area' + })).resolves.toBe(false) expect(mockDatasette.default.runQuery).not.toHaveBeenCalled() } finally { vi.unmock('redis') @@ -245,15 +230,4 @@ describe('getDatasetNameMap', () => { vi.resetModules() } }) - - it('should normalise duplicate, empty and system dataset fields', () => { - expect(normaliseDatasetFields([ - { field: 'reference' }, - { field: 'reference' }, - { field: 'organisation' }, - { field: 'entity' }, - { field: '' }, - {} - ])).toEqual(['reference']) - }) }) diff --git a/test/unit/requestData.test.js b/test/unit/requestData.test.js index 9f4482639..0c50f5ad5 100644 --- a/test/unit/requestData.test.js +++ b/test/unit/requestData.test.js @@ -39,7 +39,7 @@ describe('RequestData', () => { const response = { id: 1, - getColumnFieldLog: () => [] + getColumnMapping: () => [] } const requestData = new RequestData(response) @@ -68,7 +68,7 @@ describe('RequestData', () => { const response = { id: 1, - getColumnFieldLog: () => [] + getColumnMapping: () => [] } const requestData = new RequestData(response) @@ -97,7 +97,7 @@ describe('RequestData', () => { const response = { id: 1, - getColumnFieldLog: () => [] + getColumnMapping: () => [] } const requestData = new RequestData(response) @@ -268,8 +268,8 @@ describe('RequestData', () => { const requestData = new RequestData({ response }) expect(requestData.getColumnFieldLog()).toStrictEqual([ - { field: 'name', column: 'name', missing: false }, - { field: 'geometry', column: 'geom', missing: false } + { field: 'name', column: 'name', missing: false, mandatory: undefined }, + { field: 'geometry', column: 'geom', missing: false, mandatory: undefined } ]) }) @@ -296,8 +296,8 @@ describe('RequestData', () => { const requestData = new RequestData({ response }) expect(requestData.getColumnFieldLog()).toStrictEqual([ - { field: 'name', column: 'name', missing: false }, - { field: 'geometry', missing: true } + { field: 'name', column: 'name', missing: false, mandatory: undefined }, + { field: 'geometry', column: null, missing: true, mandatory: true } ]) }) diff --git a/test/unit/statusController.test.js b/test/unit/statusController.test.js index e0197b4a2..d8c4d6234 100644 --- a/test/unit/statusController.test.js +++ b/test/unit/statusController.test.js @@ -1,17 +1,34 @@ import { describe, it, vi, expect, beforeEach } from 'vitest' import StatusController, { shouldShowColumnMapping } from '../../src/controllers/statusController.js' -import { getDatasetFields, isStatutoryDataset } from '../../src/utils/redisLoader.js' +import { isStatutoryDataset } from '../../src/utils/redisLoader.js' vi.mock('@/services/asyncRequestApi.js') vi.mock('../../src/utils/redisLoader.js') +const makeRequestData = ({ + params = { organisationName: 'local-authority:TST', dataset: 'test-dataset' }, + columnFieldLog = [], + rows = [], + issueTasks = [], + complete = true, + failed = false +} = {}) => ({ + isComplete: () => complete, + isFailed: () => failed, + getParams: () => params, + getColumnFieldLog: () => columnFieldLog, + fetchResponseDetails: vi.fn().mockResolvedValue({ + getRows: () => rows + }), + getIssueTasks: () => issueTasks +}) + describe('StatusController', () => { let asyncRequestApi let statusController beforeEach(async () => { asyncRequestApi = await import('@/services/asyncRequestApi') - vi.mocked(getDatasetFields).mockResolvedValue(['name', 'reference']) vi.mocked(isStatutoryDataset).mockResolvedValue(false) statusController = new StatusController({ @@ -28,7 +45,7 @@ describe('StatusController', () => { form: { options: {} }, - params: 'fake_id' + params: { id: 'fake_id' } } const res = {} @@ -48,12 +65,12 @@ describe('StatusController', () => { describe('post', () => { it('redirects to column mapping when columns need mapping', async () => { asyncRequestApi.getRequestData = vi.fn().mockResolvedValue({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ field: 'reference', missing: true }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Ref: 'abc' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Ref: 'abc' } }] }) }) @@ -68,12 +85,12 @@ describe('StatusController', () => { it('redirects to results when all columns are mapped', async () => { asyncRequestApi.getRequestData = vi.fn().mockResolvedValue({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Reference', field: 'reference' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Reference: 'abc' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'name', column: 'Name', missing: false, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Name: 'Test' } }] }) }) @@ -90,12 +107,12 @@ describe('StatusController', () => { describe('shouldShowColumnMapping', () => { it('returns true when expected fields are unmapped and unmapped headings are available', async () => { await expect(shouldShowColumnMapping({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Name: 'Name', Ref: 'abc' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Ref: 'abc' } }] }) })).resolves.toBe(true) }) @@ -104,55 +121,52 @@ describe('StatusController', () => { vi.mocked(isStatutoryDataset).mockResolvedValue(true) await expect(shouldShowColumnMapping({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Name: 'Name', Ref: 'abc' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc' } }] }) })).resolves.toBe(false) }) it('returns false when expected fields are unmapped but no unmapped headings are available', async () => { await expect(shouldShowColumnMapping({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Name: 'abc' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'name', column: 'Name', missing: false, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Name: 'abc', Extra: 'extra' } }] }) })).resolves.toBe(false) }) it('returns false when all expected fields are mapped', async () => { await expect(shouldShowColumnMapping({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Reference', field: 'reference' }, { column: 'Name', field: 'name' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ converted_row: { Reference: 'abc', Name: 'Name', Extra: 'extra' } }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'name', column: 'Name', missing: false, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Name: 'Name', Extra: 'extra' } }] }) })).resolves.toBe(false) }) it('returns false when there are other blocking external errors', async () => { await expect(shouldShowColumnMapping({ - isComplete: () => true, - isFailed: () => false, - getParams: () => ({ organisationName: 'local-authority:TST', dataset: 'test-dataset' }), - getColumnFieldLog: () => [{ column: 'Name', field: 'name' }], - fetchResponseDetails: vi.fn().mockResolvedValue({ - getRows: () => [{ - converted_row: { Name: 'Name', Ref: 'abc' }, - issue_logs: [{ - severity: 'error', - responsibility: 'external', - field: 'geometry', - 'issue-type': 'invalid geometry' - }] + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'description', column: null, missing: true, mandatory: false } + ], + rows: [{ converted_row: { Reference: 'abc', Notes: 'note', Extra: 'extra' } }], + issueTasks: [{ + severity: 'error', + responsibility: 'external', + 'issue-type': 'invalid geometry' }] }) })).resolves.toBe(false) diff --git a/test/unit/views/check/columnMapping.test.js b/test/unit/views/check/columnMapping.test.js index fc9588b71..a97f5f8ec 100644 --- a/test/unit/views/check/columnMapping.test.js +++ b/test/unit/views/check/columnMapping.test.js @@ -108,7 +108,8 @@ describe('check/column-mapping.html', () => { const document = new JSDOM(html, { url: 'https://example.test/' }).window.document - expect(document.querySelector('option[value="na"]')).toBeNull() + expect(document.querySelector('option[value="na"]')).not.toBeNull() + expect(document.querySelector('option[value="na"]').textContent).toContain('Not provided') }) it('preselects user-defined mapped fields in the selector', () => { @@ -156,6 +157,7 @@ describe('check/column-mapping.html', () => { column: '', isMapped: false, userDefined: false, + userIgnored: false, isEditable: true, isRequired: false } @@ -169,4 +171,35 @@ describe('check/column-mapping.html', () => { expect(document.querySelector('option[value="na"]').textContent).toContain('Not provided') }) + + it('preselects not provided when field is user ignored', () => { + const html = nunjucks.render('check/column-mapping.html', { + options: { + lastPage: '/check/status/123', + requestParams: { + organisationName: 'local-authority:ABC', + dataset: 'conservation-area' + }, + mappingRows: [ + { + field: 'notes', + column: '', + isMapped: false, + userDefined: false, + userIgnored: true, + isEditable: true, + isRequired: false + } + ], + uploadedColumns: ['Notes'], + columnMappingErrors: {} + } + }) + + const document = new JSDOM(html).window.document + const fieldSelect = document.querySelector('select[name="fieldMap[notes]"]') + + expect(fieldSelect).not.toBeNull() + expect(fieldSelect.value).toBe('na') + }) }) From b31639850c8cf232650d4f251a6a06d26ffa0769 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Fri, 29 May 2026 03:48:35 +0100 Subject: [PATCH 05/21] fix: update dataset error handling and improve test descriptions --- config/default.yaml | 2 +- src/controllers/statusController.js | 4 ++-- test/unit/requestData.test.js | 8 ++++---- test/unit/views/check/columnMapping.test.js | 2 +- vite.config.js | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/config/default.yaml b/config/default.yaml index 180d4afa3..278cd98f6 100644 --- a/config/default.yaml +++ b/config/default.yaml @@ -124,7 +124,7 @@ datasetsConfig: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: base: local - variable: plan + variable: plan minerals-plan: guidanceUrl: https://www.gov.uk/government/publications/publish-your-plan-data/publish-your-plan-data entityDisplayName: diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index f06e73b25..05f5f5089 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -35,8 +35,8 @@ class StatusController extends PageController { const { formattedData } = await platformApi.fetchDatasets({ dataset: params.dataset }) // Bounds check TODO move to external bounds handling as in fetchOne if (!formattedData || formattedData.length === 0) { - const error = new Error(`Dataset not found: ${req.params.dataset}`) - logger.warn('fetchDatasetPlatformInfo: no dataset returned', { type: types.App, dataset: req.params.dataset }) + const error = new Error(`Dataset not found: ${params.dataset}`) + logger.warn('fetchDatasetPlatformInfo: no dataset returned', { type: types.App, dataset: params.dataset }) return next(error) } const datasetInfo = formattedData[0] diff --git a/test/unit/requestData.test.js b/test/unit/requestData.test.js index 0c50f5ad5..45594c2d2 100644 --- a/test/unit/requestData.test.js +++ b/test/unit/requestData.test.js @@ -2,20 +2,20 @@ import RequestData, { fetchPaginated } from '../../src/models/requestData.js' import ResponseDetails from '../../src/models/responseDetails.js' import { describe, it, expect, vi } from 'vitest' import axios from 'axios' -import logger from '../../src/utils/logger.js' vi.mock('axios') -vi.mock('../utils/logger.js', () => { +vi.mock('../../src/utils/logger.js', () => { return { default: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), error: vi.fn() } } }) -vi.spyOn(logger, 'error') - // Tech Debt: we should write some more tests around the requestData.js file describe('RequestData', () => { describe('fetchResponseDetails', () => { diff --git a/test/unit/views/check/columnMapping.test.js b/test/unit/views/check/columnMapping.test.js index a97f5f8ec..b8fbc3b40 100644 --- a/test/unit/views/check/columnMapping.test.js +++ b/test/unit/views/check/columnMapping.test.js @@ -83,7 +83,7 @@ describe('check/column-mapping.html', () => { expect(document.querySelector('.govuk-error-message')).toBeNull() }) - it('does not show not provided for required fields', () => { + it('shows not provided for required fields', () => { const html = nunjucks.render('check/column-mapping.html', { options: { lastPage: '/check/status/123', diff --git a/vite.config.js b/vite.config.js index 3a9f37bde..a73c1ed1c 100644 --- a/vite.config.js +++ b/vite.config.js @@ -16,7 +16,7 @@ export default defineConfig({ // If you want a coverage reports even if your tests are failing, include the reportOnFailure option reportOnFailure: true, // ignore these files - exclude: ['**/node_modules/**', '**/test/**', '**/public/**', '**/coverage/**', '**/vite.config.js'] + exclude: ['**/node_modules/**', '**/test/**', '**/public/**', '**/coverage/**', '**/vite.config.js', '**/.history/**'] }, environmentOptions: { jsdom: { From 849d42ed7d3c19c295e82028471c7c626907e7c8 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Fri, 29 May 2026 05:44:30 +0100 Subject: [PATCH 06/21] clean up --- src/controllers/columnMappingController.js | 11 ++++------- src/controllers/resultsController.js | 23 +++++++++++----------- src/controllers/statusController.js | 16 ++++++++++++--- test/unit/columnMappingController.test.js | 12 +++++------ test/unit/statusController.test.js | 19 ++++++++++++++++++ 5 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index f74393b96..2f9dbbe3d 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -188,14 +188,12 @@ async function getCompletedRequestData (req, res) { } export function validateColumnMapping (body = {}, mappingRows = []) { - const requiredFields = new Set(mappingRows.filter(row => row.isRequired).map(row => row.field)) - const fieldMap = getBracketFields(body, 'fieldMap') // base errors: missing or explicit 'na' for required fields const errors = Object.fromEntries( Object.entries(fieldMap) - .filter(([field, value]) => value === '' || (value === 'na' && requiredFields.has(field))) + .filter(([field, value]) => value === '') .map(([field]) => [field, { text: `Select the ${field} field` }]) @@ -275,10 +273,9 @@ export function buildSubmittedColumnMapping ({ existingMapping = {}, body = {}, for (const [mappedColumn, mappedField] of Object.entries(columnMapping)) { if (mappedField === field) delete columnMapping[mappedColumn] } - if (column !== 'na') { - columnMapping[column] = field - selectedUploadedColumns.add(column) - } + + columnMapping[column] = column === 'na' ? 'IGNORE' : field + selectedUploadedColumns.add(column) } spareUploadedColumns.forEach(column => { diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 079484206..2ffbdc03f 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -226,18 +226,17 @@ export function setupTableParams (req, res, next) { const columnMappingEnabled = columnMapping === true || hasMappingObject req.locals.columnMappingEnabled = columnMappingEnabled if (columnMappingEnabled) { - const columnMappingRows = orderedFields - .map(col => { - const mappedField = columnToField.get(col) || '' - return { mappedField, col } - }) - .filter(({ mappedField, col }) => { - const normalizedMappedField = typeof mappedField === 'string' ? mappedField.toUpperCase() : mappedField - return Boolean(mappedField && col && normalizedMappedField !== 'IGNORE') - }) - .map(({ mappedField, col }) => ({ - key: { text: mappedField }, - value: { html: col } + const requestData = req.locals.requestData + const columnMappingRows = requestData.getColumnFieldLog() + .filter(({ field, column, missing, mandatory }) => Boolean(field) && Boolean(column)) + .map(({ field, column, missing, mandatory }) => { + return { + field, + column: column || '' + } + }).map(({ field, column }) => ({ + key: { text: field }, + value: { html: column } })) req.locals.columnMappingRows = columnMappingRows diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index 05f5f5089..538c05308 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -131,6 +131,9 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields // the column mapping the user has done const userColumnMapping = params.column_mapping ?? {} + const hasUserColumnMapping = Object.values(userColumnMapping).some(value => Boolean(value)) + if (hasUserColumnMapping) return false + const responseDetails = await requestData.fetchResponseDetails(0, 50) const rows = responseDetails.getRows?.() ?? [] // all the columns the user has mapped. @@ -151,15 +154,22 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields // if there are missing mandatory fields that are not mapped by the user, then we should show the column mapping page const hasMissingMandatoryFields = columnMapping.some(column => column?.mandatory && !column?.column && !fieldsMappedByUser.has(column.field)) + // Note: at this point we've already ensured there are spare uploaded columns (earlier check), + // so a missing mandatory field with spare columns means we should show the mapping page. if (hasMissingMandatoryFields) return true - // if has unmapped required fields - const hasOtherBlockingExternalErrors = requestData.getIssueTasks().some(issue => + // Do not show column mapping if there are any external, error-level issues + // whose issue-type is not 'missing-field' (these are blocking external errors). + const issueTasks = requestData.getIssueTasks?.() ?? [] + const hasOtherBlockingExternalErrors = issueTasks.some(issue => issue.severity === 'error' && issue.responsibility === 'external' && issue['issue-type'] !== 'missing-field' ) - return !hasOtherBlockingExternalErrors + if (hasOtherBlockingExternalErrors) return false + + // Show column mapping if there are unmapped expected fields (and spareUploadedColumns > 0 ensured earlier) + return hasUnmappedExpectedFields } catch { return false } diff --git a/test/unit/columnMappingController.test.js b/test/unit/columnMappingController.test.js index 687dacb91..433bc4cf3 100644 --- a/test/unit/columnMappingController.test.js +++ b/test/unit/columnMappingController.test.js @@ -92,7 +92,8 @@ describe('columnMappingController helpers', () => { expect(mapping).toEqual({ Removed: 'name', Reference: 'IGNORE', - 'New reference': 'reference' + 'New reference': 'reference', + na: 'IGNORE' }) }) @@ -372,11 +373,7 @@ describe('columnMappingController helpers', () => { }, [ { field: 'reference', isRequired: true }, { field: 'notes', isRequired: false } - ])).toEqual({ - reference: { - text: 'Select the reference field' - } - }) + ])).toEqual({}) }) it('applies submitted selections to mapping rows for redisplay', () => { @@ -427,7 +424,8 @@ describe('columnMappingController helpers', () => { expect(mapping).toEqual({ Reference: 'reference', Notes: 'IGNORE', - Extra: 'IGNORE' + Extra: 'IGNORE', + na: 'IGNORE' }) }) }) diff --git a/test/unit/statusController.test.js b/test/unit/statusController.test.js index d8c4d6234..84550ff6b 100644 --- a/test/unit/statusController.test.js +++ b/test/unit/statusController.test.js @@ -155,6 +155,25 @@ describe('StatusController', () => { })).resolves.toBe(false) }) + it('returns false when the user has already started mapping', async () => { + await expect(shouldShowColumnMapping({ + ...makeRequestData({ + params: { + organisationName: 'local-authority:TST', + dataset: 'test-dataset', + column_mapping: { + na: 'IGNORE' + } + }, + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Ref: 'abc' } }] + }) + })).resolves.toBe(false) + }) + it('returns false when there are other blocking external errors', async () => { await expect(shouldShowColumnMapping({ ...makeRequestData({ From 1b4584602a418a717bae39bcc5d88c3265b37832 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 1 Jun 2026 10:02:56 +0100 Subject: [PATCH 07/21] fix: prevent showing column mapping for blocking external errors --- src/controllers/statusController.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index 538c05308..a2d343504 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -108,6 +108,15 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields })) { return false } + // Do not show column mapping if there are any external, error-level issues + // whose issue-type is not 'missing-field' (these are blocking external errors). + const issueTasks = requestData.getIssueTasks?.() ?? [] + const hasOtherBlockingExternalErrors = issueTasks.some(issue => + issue.severity === 'error' && + issue.responsibility === 'external' && + issue['issue-type'] !== 'missing-field' + ) + if (hasOtherBlockingExternalErrors) return false let columnMapping = requestData.getColumnFieldLog?.() ?? [] @@ -158,16 +167,6 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields // so a missing mandatory field with spare columns means we should show the mapping page. if (hasMissingMandatoryFields) return true - // Do not show column mapping if there are any external, error-level issues - // whose issue-type is not 'missing-field' (these are blocking external errors). - const issueTasks = requestData.getIssueTasks?.() ?? [] - const hasOtherBlockingExternalErrors = issueTasks.some(issue => - issue.severity === 'error' && - issue.responsibility === 'external' && - issue['issue-type'] !== 'missing-field' - ) - if (hasOtherBlockingExternalErrors) return false - // Show column mapping if there are unmapped expected fields (and spareUploadedColumns > 0 ensured earlier) return hasUnmappedExpectedFields } catch { From 5736de249dab988d517453455760d11ec8f8bd91 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 1 Jun 2026 10:57:41 +0100 Subject: [PATCH 08/21] test: add cases for handling missing-field and internal issues in column mapping --- test/unit/statusController.test.js | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/unit/statusController.test.js b/test/unit/statusController.test.js index 84550ff6b..2efd090d7 100644 --- a/test/unit/statusController.test.js +++ b/test/unit/statusController.test.js @@ -190,5 +190,39 @@ describe('StatusController', () => { }) })).resolves.toBe(false) }) + + it('does not block when issue-type is missing-field', async () => { + await expect(shouldShowColumnMapping({ + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Ref: 'abc' } }], + issueTasks: [{ + severity: 'error', + responsibility: 'external', + 'issue-type': 'missing-field' + }] + }) + })).resolves.toBe(true) + }) + + it('does not block when issues are internal', async () => { + await expect(shouldShowColumnMapping({ + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ], + rows: [{ converted_row: { Reference: 'abc', Ref: 'abc' } }], + issueTasks: [{ + severity: 'error', + responsibility: 'internal', + 'issue-type': 'invalid geometry' + }] + }) + })).resolves.toBe(true) + }) }) }) From 45a953e5ed1b73d4da89f613c49c211bcb051940 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 12:46:58 +0100 Subject: [PATCH 09/21] feat: add manageServiceUrl to configuration files and implement internal note functionality in JIRA service --- config/development.yaml | 1 + config/local.yaml | 1 + config/production.yaml | 1 + config/staging.yaml | 1 + src/controllers/CheckAnswersController.js | 14 ++++- src/controllers/resultsController.js | 10 +++- src/controllers/statusController.js | 28 +++++++--- src/services/jiraService.js | 29 ++++++++++ test/unit/checkAnswersController.test.js | 3 +- test/unit/services/jiraService.test.js | 68 ++++++++++++++++++++++- test/unit/statusController.test.js | 32 ++++++++++- 11 files changed, 172 insertions(+), 16 deletions(-) diff --git a/config/development.yaml b/config/development.yaml index 6701ad3e8..34d59afcd 100644 --- a/config/development.yaml +++ b/config/development.yaml @@ -5,6 +5,7 @@ aws: { region: eu-west-2, bucket: 'development-pub-async-request-files', } +manageServiceUrl: 'https://manage.development.planning.data.gov.uk' datasetteUrl: https://datasette.development.planning.data.gov.uk downloadUrl: 'https://download.development.planning.data.gov.uk' redis: { diff --git a/config/local.yaml b/config/local.yaml index 273fa47bd..a7e0cbae9 100644 --- a/config/local.yaml +++ b/config/local.yaml @@ -7,6 +7,7 @@ aws: { endpoint: 'http://localhost:4566', s3ForcePathStyle: true, } +manageServiceUrl: 'http://localhost:5000' organisationTypes: - local-authority - national-park-authority diff --git a/config/production.yaml b/config/production.yaml index 5d00f2d01..0035a4a79 100644 --- a/config/production.yaml +++ b/config/production.yaml @@ -10,6 +10,7 @@ aws: { region: eu-west-2, bucket: 'production-pub-async-request-files', } +manageServiceUrl: 'https://manage.planning.data.gov.uk' redis: { secure: true, host: 'production-pub-async-redis-eihmmv.serverless.euw2.cache.amazonaws.com', diff --git a/config/staging.yaml b/config/staging.yaml index 59baa932f..29469261d 100644 --- a/config/staging.yaml +++ b/config/staging.yaml @@ -10,6 +10,7 @@ aws: { region: eu-west-2, bucket: 'staging-pub-async-request-files', } +manageServiceUrl: 'https://manage.staging.planning.data.gov.uk/' redis: { secure: true, host: 'staging-pub-async-redis-kh1elu.serverless.euw2.cache.amazonaws.com', diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index 2749fe033..6b3baa9d4 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -1,6 +1,6 @@ import PageController from './pageController.js' import config from '../../config/index.js' -import { attachFileToIssue, createCustomerRequest } from '../services/jiraService.js' +import { addInternalNoteToIssue, attachFileToIssue, createCustomerRequest } from '../services/jiraService.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { stringify } from 'csv-stringify/sync' @@ -119,6 +119,18 @@ class CheckAnswersController extends PageController { type: types.External }) }) + const urlSearchParams = new URLSearchParams({ requestId, dataset: data.dataset, organisationId: data.organisationId, endpointUrl: data.endpoint, documentationUrl: data.documentationUrl, jiraIssueId: response.data.issueKey }) + if (data.dataset === 'tree') { + urlSearchParams.append('geometryType', data.geomType) + } + const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` + addInternalNoteToIssue(response.data.issueKey, `Click here to add this data \n\n ${manageServiceLink} \n\n\n [Click here to add this data] (${manageServiceLink})`).catch((error) => { + logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { + errorMessage: error.message, + errorStack: error, + type: types.External + }) + }) return response.data } diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 2ffbdc03f..17b934e38 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -296,9 +296,15 @@ export function filterOutInternalIssues (req, res, next) { export function addQualityCriteriaLevelsToIssues (req, res, next) { const { issues, issueTypes } = req + req.issues = addQualityCriteriaLevels(issues, issueTypes) + + next() +} + +export function addQualityCriteriaLevels (issues = [], issueTypes = []) { const issueTypeMap = new Map(issueTypes.map(it => [it.issue_type, it])) - req.issues = issues.map(issue => { + return issues.map(issue => { const issueType = issueTypeMap.get(issue['issue-type']) let qualityLevel = issueType ? issueType.quality_criteria_level : null @@ -312,8 +318,6 @@ export function addQualityCriteriaLevelsToIssues (req, res, next) { quality_criteria_level: qualityLevel } }) - - next() } /** diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index a2d343504..a1ec9bc0f 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -7,6 +7,8 @@ import platformApi from '../services/platformApi.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { processSpecificationMiddlewares } from '../middleware/common.middleware.js' +import datasette from '../services/datasette.js' +import { addQualityCriteriaLevels } from './resultsController.js' /** * Attempts to infer how we ended up on this page. @@ -108,15 +110,7 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields })) { return false } - // Do not show column mapping if there are any external, error-level issues - // whose issue-type is not 'missing-field' (these are blocking external errors). - const issueTasks = requestData.getIssueTasks?.() ?? [] - const hasOtherBlockingExternalErrors = issueTasks.some(issue => - issue.severity === 'error' && - issue.responsibility === 'external' && - issue['issue-type'] !== 'missing-field' - ) - if (hasOtherBlockingExternalErrors) return false + if (await hasBlockingNonColumnMappingTasks(requestData)) return false let columnMapping = requestData.getColumnFieldLog?.() ?? [] @@ -174,6 +168,22 @@ export async function shouldShowColumnMapping (requestData, uniqueDatasetFields } } +export async function hasBlockingNonColumnMappingTasks (requestData) { + const issueTasks = requestData.getIssueTasks?.() ?? [] + if (issueTasks.length === 0) return false + + const { formattedData: issueTypes } = await datasette.runQuery(` + select issue_type, quality_criteria_level + from issue_type + `) + + return addQualityCriteriaLevels(issueTasks, issueTypes).some(issue => + issue.responsibility !== 'internal' && + issue.quality_criteria_level === 2 && + !['missing column', 'missing-field'].includes(issue['issue-type']) + ) +} + function buildMappedFields (columnMapping = [], userColumnMapping = {}) { const fields = new Set() diff --git a/src/services/jiraService.js b/src/services/jiraService.js index f9c40373e..619a376dc 100644 --- a/src/services/jiraService.js +++ b/src/services/jiraService.js @@ -90,3 +90,32 @@ export async function attachFileToIssue (issueKey, file, additionalComment = 'Pl return attachment } + +/** + * Adds an internal note (non-public comment) to an existing JIRA Service Desk issue. + * + * @doc https://developer.atlassian.com/cloud/jira/service-desk/rest/api-group-request/#api-rest-servicedeskapi-request-issueidorkey-comment-post + * @param {string} issueKey - The key of the issue to which the note will be added. + * @param {string} note - The internal note body text. + * @returns {Promise} - A promise that resolves to the response of the JIRA API. + * @throws {Error} - Throws an error if JIRA_URL, JIRA_API_KEY, or JIRA_SERVICE_DESK_ID are not set. + */ +export async function addInternalNoteToIssue (issueKey, note) { + const JIRA_URL = process.env.JIRA_URL + const JIRA_API_KEY = process.env.JIRA_API_KEY + const JIRA_SERVICE_DESK_ID = process.env.JIRA_SERVICE_DESK_ID + + if (!JIRA_URL || !JIRA_API_KEY || !JIRA_SERVICE_DESK_ID) { + throw new Error('JIRA_URL, JIRA_API_KEY and JIRA_SERVICE_DESK_ID must be set') + } + + return await axios.post(`${JIRA_URL}/rest/servicedeskapi/request/${issueKey}/comment`, { + body: note, + public: false + }, { + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${JIRA_API_KEY}` + } + }) +} diff --git a/test/unit/checkAnswersController.test.js b/test/unit/checkAnswersController.test.js index a22734bf0..595405c23 100644 --- a/test/unit/checkAnswersController.test.js +++ b/test/unit/checkAnswersController.test.js @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -import { createCustomerRequest, attachFileToIssue } from '../../src/services/jiraService.js' +import { addInternalNoteToIssue, createCustomerRequest, attachFileToIssue } from '../../src/services/jiraService.js' import config from '../../config/index.js' import CheckAnswersController from '../../src/controllers/CheckAnswersController.js' import { getRequestData } from '../../src/services/asyncRequestApi.js' @@ -35,6 +35,7 @@ describe('CheckAnswersController', () => { next = vi.fn() controller = new CheckAnswersController({ route: '/check-answers/:requestId' }) vi.clearAllMocks() + addInternalNoteToIssue.mockResolvedValue({ data: {} }) }) describe('POST to CheckAnswersController', () => { diff --git a/test/unit/services/jiraService.test.js b/test/unit/services/jiraService.test.js index 391a58c70..3bcd985d2 100644 --- a/test/unit/services/jiraService.test.js +++ b/test/unit/services/jiraService.test.js @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import axios from 'axios' -import { attachFileToIssue, createCustomerRequest } from '../../../src/services/jiraService' +import { addInternalNoteToIssue, attachFileToIssue, createCustomerRequest } from '../../../src/services/jiraService' vi.mock('axios') @@ -252,4 +252,70 @@ describe('jiraService', () => { .toThrowError('Request failed with status code 500') }) }) + + describe('addInternalNoteToIssue', () => { + const issueKey = 'SUP-1' + const note = 'Internal triage note' + + beforeEach(() => { + process.env.JIRA_URL = 'https://jira.example.com' + process.env.JIRA_API_KEY = 'apiToken' + process.env.JIRA_SERVICE_DESK_ID = 'serviceDeskId' + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + it('should throw an error if JIRA_URL, JIRA_API_KEY, or JIRA_SERVICE_DESK_ID are not set', async () => { + delete process.env.JIRA_URL + + await expect(addInternalNoteToIssue(issueKey, note)) + .rejects + .toThrowError('JIRA_URL, JIRA_API_KEY and JIRA_SERVICE_DESK_ID must be set') + }) + + it('should call axios.post with correct parameters', async () => { + const mockResponse = { + data: { + id: '10000', + body: note, + public: false + } + } + axios.post.mockResolvedValue(mockResponse) + + const response = await addInternalNoteToIssue(issueKey, note) + + expect(axios.post).toHaveBeenCalledWith( + 'https://jira.example.com/rest/servicedeskapi/request/SUP-1/comment', + { + body: 'Internal triage note', + public: false + }, + { + headers: { + 'Content-Type': 'application/json', + Authorization: 'Bearer apiToken' + } + } + ) + expect(response).toBe(mockResponse) + }) + + it('should handle network errors gracefully', async () => { + axios.post.mockRejectedValue({ + response: { + status: 500, + statusText: 'Internal Server Error', + data: { error: 'Internal Server Error' } + }, + message: 'Request failed with status code 500' + }) + + await expect(addInternalNoteToIssue(issueKey, note)) + .rejects + .toThrowError('Request failed with status code 500') + }) + }) }) diff --git a/test/unit/statusController.test.js b/test/unit/statusController.test.js index 2efd090d7..029259868 100644 --- a/test/unit/statusController.test.js +++ b/test/unit/statusController.test.js @@ -1,9 +1,15 @@ import { describe, it, vi, expect, beforeEach } from 'vitest' import StatusController, { shouldShowColumnMapping } from '../../src/controllers/statusController.js' import { isStatutoryDataset } from '../../src/utils/redisLoader.js' +import datasette from '../../src/services/datasette.js' vi.mock('@/services/asyncRequestApi.js') vi.mock('../../src/utils/redisLoader.js') +vi.mock('../../src/services/datasette.js', () => ({ + default: { + runQuery: vi.fn() + } +})) const makeRequestData = ({ params = { organisationName: 'local-authority:TST', dataset: 'test-dataset' }, @@ -30,6 +36,13 @@ describe('StatusController', () => { beforeEach(async () => { asyncRequestApi = await import('@/services/asyncRequestApi') vi.mocked(isStatutoryDataset).mockResolvedValue(false) + vi.mocked(datasette.runQuery).mockResolvedValue({ + formattedData: [ + { issue_type: 'invalid geometry', quality_criteria_level: 2 }, + { issue_type: 'missing-field', quality_criteria_level: 2 }, + { issue_type: 'minor formatting issue', quality_criteria_level: 3 } + ] + }) statusController = new StatusController({ route: '/status' @@ -174,7 +187,7 @@ describe('StatusController', () => { })).resolves.toBe(false) }) - it('returns false when there are other blocking external errors', async () => { + it('returns false when there are level 2 external issue tasks', async () => { await expect(shouldShowColumnMapping({ ...makeRequestData({ columnFieldLog: [ @@ -191,6 +204,23 @@ describe('StatusController', () => { })).resolves.toBe(false) }) + it('does not block when external issue tasks are level 3', async () => { + await expect(shouldShowColumnMapping({ + ...makeRequestData({ + columnFieldLog: [ + { field: 'reference', column: 'Reference', missing: false, mandatory: true }, + { field: 'description', column: null, missing: true, mandatory: false } + ], + rows: [{ converted_row: { Reference: 'abc', Notes: 'note', Extra: 'extra' } }], + issueTasks: [{ + severity: 'error', + responsibility: 'external', + 'issue-type': 'minor formatting issue' + }] + }) + })).resolves.toBe(true) + }) + it('does not block when issue-type is missing-field', async () => { await expect(shouldShowColumnMapping({ ...makeRequestData({ From bc982961d52d3a856ff0e191649ac2e3c7def8b0 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 14:44:49 +0100 Subject: [PATCH 10/21] refactor notes to use --- src/controllers/CheckAnswersController.js | 47 ++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index 6b3baa9d4..de9b06d5d 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -124,7 +124,52 @@ class CheckAnswersController extends PageController { urlSearchParams.append('geometryType', data.geomType) } const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` - addInternalNoteToIssue(response.data.issueKey, `Click here to add this data \n\n ${manageServiceLink} \n\n\n [Click here to add this data] (${manageServiceLink})`).catch((error) => { + const note = { + type: 'doc', + version: 1, + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'Click here to add this data', + marks: [ + { + type: 'link', + attrs: { + href: manageServiceLink + } + } + ] + } + ] + }, + { + type: 'paragraph', + content: [] + }, + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'If the link above is not clickable, copy and paste the link below instead:' + } + ] + }, + { + type: 'paragraph', + content: [ + { + type: 'text', + text: manageServiceLink + } + ] + } + ] + } + addInternalNoteToIssue(response.data.issueKey, note).catch((error) => { logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { errorMessage: error.message, errorStack: error, From 405c60975b4dfd8c46c89eb2c14f590437e35eac Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 15:19:05 +0100 Subject: [PATCH 11/21] refactor: simplify internal note creation for Jira issues --- src/controllers/CheckAnswersController.js | 49 +---------------------- 1 file changed, 2 insertions(+), 47 deletions(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index de9b06d5d..b53cd87f5 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -124,59 +124,14 @@ class CheckAnswersController extends PageController { urlSearchParams.append('geometryType', data.geomType) } const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` - const note = { - type: 'doc', - version: 1, - content: [ - { - type: 'paragraph', - content: [ - { - type: 'text', - text: 'Click here to add this data', - marks: [ - { - type: 'link', - attrs: { - href: manageServiceLink - } - } - ] - } - ] - }, - { - type: 'paragraph', - content: [] - }, - { - type: 'paragraph', - content: [ - { - type: 'text', - text: 'If the link above is not clickable, copy and paste the link below instead:' - } - ] - }, - { - type: 'paragraph', - content: [ - { - type: 'text', - text: manageServiceLink - } - ] - } - ] - } - addInternalNoteToIssue(response.data.issueKey, note).catch((error) => { + + addInternalNoteToIssue(response.data.issueKey, `[Click here to add this data|${manageServiceLink}]`).catch((error) => { logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { errorMessage: error.message, errorStack: error, type: types.External }) }) - return response.data } From 7092401e1a10490a4ab3e360ee618f2d3cf79782 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 15:39:27 +0100 Subject: [PATCH 12/21] fix: await asynchronous calls for attaching files and adding internal notes in Jira issue --- src/controllers/CheckAnswersController.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index b53cd87f5..1108fef05 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -112,26 +112,27 @@ class CheckAnswersController extends PageController { return null } - this.attachFileToIssue(requestId, data, description, response).catch((error) => { - logger.error('CheckAnswersController.attachFileToIssue(): Failed to attach CSV to Jira issue', { - errorMessage: error.message, - errorStack: error, - type: types.External - }) - }) const urlSearchParams = new URLSearchParams({ requestId, dataset: data.dataset, organisationId: data.organisationId, endpointUrl: data.endpoint, documentationUrl: data.documentationUrl, jiraIssueId: response.data.issueKey }) if (data.dataset === 'tree') { urlSearchParams.append('geometryType', data.geomType) } const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` - addInternalNoteToIssue(response.data.issueKey, `[Click here to add this data|${manageServiceLink}]`).catch((error) => { + await addInternalNoteToIssue(response.data.issueKey, `[Click here to add this data|${manageServiceLink}]`).catch((error) => { logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { errorMessage: error.message, errorStack: error, type: types.External }) }) + + await this.attachFileToIssue(requestId, data, description, response).catch((error) => { + logger.error('CheckAnswersController.attachFileToIssue(): Failed to attach CSV to Jira issue', { + errorMessage: error.message, + errorStack: error, + type: types.External + }) + }) return response.data } From f5f0ccf8b74f82ad89a04f5e1911133eaa6a1397 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 15:48:17 +0100 Subject: [PATCH 13/21] test: mock attachFileToIssue in createJiraServiceRequest for improved testing --- test/unit/checkAnswersController.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/checkAnswersController.test.js b/test/unit/checkAnswersController.test.js index 595405c23..8004f4369 100644 --- a/test/unit/checkAnswersController.test.js +++ b/test/unit/checkAnswersController.test.js @@ -140,6 +140,7 @@ describe('CheckAnswersController', () => { const response = { data: { issueKey: 'TEST-123' } } createCustomerRequest.mockResolvedValue(response) attachFileToIssue.mockResolvedValue({ data: {} }) + vi.spyOn(controller, 'attachFileToIssue').mockResolvedValue() const result = await controller.createJiraServiceRequest(req, res, next) From 5cb6aca008d65e6fc3ee56973bad10ff17b4b1a6 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 16:03:12 +0100 Subject: [PATCH 14/21] fix: enhance internal note message with alternative link instructions for Jira issues --- src/controllers/CheckAnswersController.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index 1108fef05..29c310717 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -118,7 +118,8 @@ class CheckAnswersController extends PageController { } const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` - await addInternalNoteToIssue(response.data.issueKey, `[Click here to add this data|${manageServiceLink}]`).catch((error) => { + const internalNote = `This request is ready to be added in the Manage Service.\n\n[Open this request in Manage Service|${manageServiceLink}]\n\nIf the link does not open, use this URL:\n${manageServiceLink}` + await addInternalNoteToIssue(response.data.issueKey, internalNote).catch((error) => { logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { errorMessage: error.message, errorStack: error, From 0f4cf80762a3b6b71062c8412a2e2a96e53a2a92 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 19:49:48 +0100 Subject: [PATCH 15/21] chore: implement local fallback for Jira configuration in CheckAnswersController and shortened manage service url --- src/controllers/CheckAnswersController.js | 35 ++++++++++++++++++----- test/unit/checkAnswersController.test.js | 27 ++++++++++++++++- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/controllers/CheckAnswersController.js b/src/controllers/CheckAnswersController.js index 29c310717..f15839b48 100644 --- a/src/controllers/CheckAnswersController.js +++ b/src/controllers/CheckAnswersController.js @@ -41,6 +41,12 @@ class CheckAnswersController extends PageController { async post (req, res, next) { try { const issue = await this.createJiraServiceRequest(req, res, next) + if (issue?.localJiraFallback) { + return res.json({ + message: issue.message, + manageServiceLink: issue.manageServiceLink + }) + } if (issue) { req.sessionModel.set('reference', issue.issueKey) req.sessionModel.set('errors', []) @@ -80,6 +86,18 @@ class CheckAnswersController extends PageController { const checkTool = requestId ? `${config.url}check/results/${requestId}/${config.jira.requestTypeId}` : 'Check tool link unavailable' + const manageServiceLink = buildManageServiceLink(requestId, data) + + // Theres no point in attempting to create a Jira issue if we're going to fail, so we check + // first and return early with a message about using the Manage Service for local development + // if Jira isn't configured + if (config.environment === 'local' && !isJiraConfigured()) { + return { + localJiraFallback: true, + message: 'Jira is not configured for local development. Use this Manage Service link to add the data.', + manageServiceLink + } + } const isNonProd = ['local', 'development', 'staging'].includes(config.environment) const summary = `${isNonProd ? '[TEST] ' : ''}Dataset URL request: ${data.organisationName} for ${data.dataset}` @@ -112,13 +130,7 @@ class CheckAnswersController extends PageController { return null } - const urlSearchParams = new URLSearchParams({ requestId, dataset: data.dataset, organisationId: data.organisationId, endpointUrl: data.endpoint, documentationUrl: data.documentationUrl, jiraIssueId: response.data.issueKey }) - if (data.dataset === 'tree') { - urlSearchParams.append('geometryType', data.geomType) - } - const manageServiceLink = `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` - - const internalNote = `This request is ready to be added in the Manage Service.\n\n[Open this request in Manage Service|${manageServiceLink}]\n\nIf the link does not open, use this URL:\n${manageServiceLink}` + const internalNote = `This request is ready to be added in the Manage Service.\n\n[Open this request in Manage Service|${manageServiceLink}]\n\nIf the link does not open, copy and paste this URL in your browser:\n${manageServiceLink}` await addInternalNoteToIssue(response.data.issueKey, internalNote).catch((error) => { logger.error('CheckAnswersController.addInternalNoteToIssue(): Failed to add internal note to Jira issue', { errorMessage: error.message, @@ -202,4 +214,13 @@ class CheckAnswersController extends PageController { } } +function buildManageServiceLink (requestId, data) { + const urlSearchParams = new URLSearchParams({ requestId, documentationUrl: data.documentationUrl }) + return `${config.manageServiceUrl}/datamanager${urlSearchParams.toString() ? `?${urlSearchParams.toString()}` : ''}` +} + +function isJiraConfigured () { + return Boolean(process.env.JIRA_URL && process.env.JIRA_API_KEY && process.env.JIRA_SERVICE_DESK_ID) +} + export default CheckAnswersController diff --git a/test/unit/checkAnswersController.test.js b/test/unit/checkAnswersController.test.js index 8004f4369..f4cc4a846 100644 --- a/test/unit/checkAnswersController.test.js +++ b/test/unit/checkAnswersController.test.js @@ -31,7 +31,7 @@ describe('CheckAnswersController', () => { form: { options: {} }, body: {} } - res = { redirect: vi.fn() } + res = { redirect: vi.fn(), json: vi.fn() } next = vi.fn() controller = new CheckAnswersController({ route: '/check-answers/:requestId' }) vi.clearAllMocks() @@ -39,6 +39,31 @@ describe('CheckAnswersController', () => { }) describe('POST to CheckAnswersController', () => { + it('should return the Manage Service link as JSON locally when Jira is not configured', async () => { + const originalEnvironment = config.environment + config.environment = 'local' + vi.stubEnv('JIRA_URL', '') + vi.stubEnv('JIRA_API_KEY', '') + vi.stubEnv('JIRA_SERVICE_DESK_ID', '') + req.sessionModel.get.mockImplementation((key) => sessionData[key]) + + try { + await controller.post(req, res, next) + + expect(createCustomerRequest).not.toHaveBeenCalled() + expect(res.json).toHaveBeenCalledWith({ + message: 'Jira is not configured for local development. Use this Manage Service link to add the data.', + manageServiceLink: expect.stringContaining(`${config.manageServiceUrl}/datamanager`) + }) + expect(res.json.mock.calls[0][0].manageServiceLink).toContain('requestId=existing-request-id') + expect(res.json.mock.calls[0][0].manageServiceLink).toContain('documentationUrl=http%3A%2F%2Fexample.com%2Fdoc') + expect(next).not.toHaveBeenCalled() + } finally { + config.environment = originalEnvironment + vi.unstubAllEnvs() + } + }) + it('should create a Jira issue and set session data on success', async () => { const issue = { issueKey: 'TEST-123' } vi.spyOn(controller, 'createJiraServiceRequest').mockResolvedValue(issue) From 80d3f64e4dedfc1042181fa1b5703687c78c0ee9 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 2 Jun 2026 20:06:31 +0100 Subject: [PATCH 16/21] fix: update column mapping to ensure proper text representation in setupTableParams --- src/controllers/resultsController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 17b934e38..6c24f081a 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -236,7 +236,7 @@ export function setupTableParams (req, res, next) { } }).map(({ field, column }) => ({ key: { text: field }, - value: { html: column } + value: { text: String(column || '') } })) req.locals.columnMappingRows = columnMappingRows From 20335c6688cb99da779bb6b3a401aa2856a6f0a0 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Thu, 11 Jun 2026 09:20:54 +0100 Subject: [PATCH 17/21] Implement geometry column mapping detection and integration in column mapping workflow --- src/controllers/columnMappingController.js | 93 +++++++++++++++-- src/controllers/resultsController.js | 21 ++++ test/unit/columnMappingController.test.js | 111 +++++++++++++++++++++ 3 files changed, 215 insertions(+), 10 deletions(-) diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index 2f9dbbe3d..05a084a4c 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -96,10 +96,13 @@ class ColumnMappingController extends PageController { const params = requestData.getParams() ?? {} const uniqueDatasetFields = req.uniqueDatasetFields || [] - const { columnMappingRows } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) + const { columnMappingRows, detectedGeometryMapping } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) const spareUploadedColumns = buildSelectableColumns(columnMappingRows) const columnMapping = buildSubmittedColumnMapping({ - existingMapping: params.column_mapping, + existingMapping: { + ...(params.column_mapping ?? {}), + ...detectedGeometryMapping + }, body: req.body, spareUploadedColumns }) @@ -126,7 +129,8 @@ async function buildColumnMappingOptions ({ requestData, requestId, body = {}, v const { columnMappingRows, specFields, - requiredFields + requiredFields, + responseRows } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) const mappingRows = buildExpectedFieldRows({ columnMappingRows, @@ -136,12 +140,11 @@ async function buildColumnMappingOptions ({ requestData, requestId, body = {}, v applySubmittedFieldSelections(mappingRows, body) - const responseDetails = await requestData.fetchResponseDetails(0, 50) return { id: requestId, requestParams: requestData.getParams(), mappingRows, - uploadedColumns: buildSelectableColumns(columnMappingRows, responseDetails.getRows()), + uploadedColumns: buildSelectableColumns(columnMappingRows, responseRows), columnMappingErrors: validationErrors, lastPage: `/check/status/${requestId}` } @@ -159,6 +162,11 @@ async function prepareColumnMappingContext (requestData, uniqueDatasetFields = [ if (uniqueDatasetFields.length > 0) { columnFieldLog = columnFieldLog.filter(entry => uniqueDatasetFields.includes(entry?.field)) } + const responseDetails = await requestData.fetchResponseDetails(0, 50) + const responseRows = responseDetails.getRows() + const detectedGeometryMapping = detectGeometryColumnMapping(columnFieldLog, responseRows) + columnFieldLog = applyDetectedGeometryColumnMapping(columnFieldLog, responseRows) + const params = requestData.getParams() ?? {} const userColumnMapping = params.column_mapping ?? {} const columnMappingRows = buildColumnMappingRows({ @@ -171,7 +179,9 @@ async function prepareColumnMappingContext (requestData, uniqueDatasetFields = [ return { columnMappingRows, specFields, - requiredFields + requiredFields, + responseRows, + detectedGeometryMapping } } @@ -328,15 +338,16 @@ export function buildColumnMappingRows ({ columnFieldLog = [], userColumnMapping } const userMappedField = userColumnMapping[column] + const isDetectedGeometryMapping = entry?.detectedGeometryMapping === true const field = entry.field const isMapped = Boolean(field) && Boolean(column) rows.push({ column, field, isMapped, - isAutoMapped: isMapped && !userMappedField, + isAutoMapped: isMapped && !userMappedField && !isDetectedGeometryMapping, isMissing: entry.missing, - userDefined: Boolean(userMappedField) + userDefined: Boolean(userMappedField || isDetectedGeometryMapping) }) }) @@ -351,6 +362,68 @@ export function buildColumnMappingRows ({ columnFieldLog = [], userColumnMapping return rows } +export function applyDetectedGeometryColumnMapping (columnFieldLog = [], responseRows = []) { + const detectedMapping = detectGeometryColumnMapping(columnFieldLog, responseRows) + if (Object.keys(detectedMapping).length === 0) return columnFieldLog + + const [[detectedColumn, detectedField]] = Object.entries(detectedMapping) + return columnFieldLog.map(entry => { + if (entry?.field !== detectedField) return entry + + return { + ...entry, + column: detectedColumn, + detectedGeometryMapping: true, + missing: false + } + }) +} + +export function detectGeometryColumnMapping (columnFieldLog = [], responseRows = []) { + const geometryFields = ['geometry', 'point'] + const expectedGeometryFields = new Set( + columnFieldLog + .map(entry => entry?.field) + .filter(field => geometryFields.includes(field)) + ) + + if (expectedGeometryFields.size === 0) return {} + + const alreadyMapped = columnFieldLog.some(entry => + geometryFields.includes(entry?.field) && Boolean(entry?.column) + ) + if (alreadyMapped) return {} + + const detectedMapping = detectGeometryColumnFromFirstRow(responseRows, expectedGeometryFields) + if (!detectedMapping) return {} + + return { + [detectedMapping.column]: detectedMapping.field + } +} + +export function detectGeometryColumnFromFirstRow (responseRows = [], expectedFields = new Set(['geometry', 'point'])) { + const firstRow = responseRows[0]?.converted_row ?? {} + + for (const [column, value] of Object.entries(firstRow)) { + const field = getGeometryFieldForValue(value) + if (field && expectedFields.has(field)) { + return { column, field } + } + } + + return null +} + +function getGeometryFieldForValue (value) { + if (typeof value !== 'string') return null + + const normalisedValue = value.trimStart().toUpperCase() + if (normalisedValue.startsWith('POINT')) return 'point' + if (normalisedValue.startsWith('POLYGON') || normalisedValue.startsWith('MULTIPOLYGON')) return 'geometry' + return null +} + // selectable columns are converted rows that have not been auto-mapped by the system export function buildSelectableColumns (columnMappingRows = [], responseRows = []) { const autoMappedColumns = new Set(columnMappingRows.filter(row => row.isAutoMapped).map(row => row.column).filter(Boolean)) @@ -389,8 +462,8 @@ export function buildExpectedFieldRows ({ columnMappingRows = [], specFields = [ // - If both are mapped or both are unmapped, leave both rows as-is. const geometryRow = rows.find(r => r.field === 'geometry') const pointRow = rows.find(r => r.field === 'point') - const geometryMapped = Boolean(geometryRow && geometryRow.isAutoMapped) - const pointMapped = Boolean(pointRow && pointRow.isAutoMapped) + const geometryMapped = Boolean(geometryRow && geometryRow.isMapped && !geometryRow.userIgnored) + const pointMapped = Boolean(pointRow && pointRow.isMapped && !pointRow.userIgnored) if (geometryMapped && !pointMapped) { rows = rows.filter(r => r.field !== 'point') } else if (pointMapped && !geometryMapped) { diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index 9d893703f..1775e0158 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -224,6 +224,27 @@ export async function setupTableParams (req, res, next) { req.locals.datasetTypology === 'geography' ? await responseDetails.getGeometries() : null + // Show column mapping if the requestParams contains a non-empty mapping object + const columnMapping = req.locals.requestParams?.column_mapping + const hasMappingObject = columnMapping && typeof columnMapping === 'object' && Object.keys(columnMapping).length > 0 + const columnMappingEnabled = columnMapping === true || hasMappingObject + req.locals.columnMappingEnabled = columnMappingEnabled + if (columnMappingEnabled) { + const requestData = req.locals.requestData + const columnMappingRows = requestData.getColumnFieldLog() + .filter(({ field, column, missing, mandatory }) => Boolean(field) && Boolean(column)) + .map(({ field, column, missing, mandatory }) => { + return { + field, + column: column || '' + } + }).map(({ field, column }) => ({ + key: { text: field }, + value: { text: String(column || '') } + })) + + req.locals.columnMappingRows = columnMappingRows + } // pagination is on the 'table' tab, so we want to ensure clicking those // links takes us to a page with the table tab *selected* const { pageNumber } = req.parsedParams diff --git a/test/unit/columnMappingController.test.js b/test/unit/columnMappingController.test.js index 433bc4cf3..c4b734097 100644 --- a/test/unit/columnMappingController.test.js +++ b/test/unit/columnMappingController.test.js @@ -1,11 +1,13 @@ import { describe, expect, it } from 'vitest' import { applySubmittedFieldSelections, + applyDetectedGeometryColumnMapping, buildExpectedFieldRows, buildColumnMappingRows, buildSpecFields, buildSubmittedColumnMapping, buildSelectableColumns, + detectGeometryColumnMapping, getBracketFields, validateColumnMapping } from '../../src/controllers/columnMappingController.js' @@ -204,6 +206,115 @@ describe('columnMappingController helpers', () => { ]) }) + it('detects point geometry from the first data row', () => { + const columnFieldLog = [ + { field: 'point', column: null, missing: true, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ] + const rows = [ + { converted_row: { Reference: 'abc', WKT: 'POINT (-0.1 51.5)' } } + ] + + expect(detectGeometryColumnMapping(columnFieldLog, rows)).toEqual({ + WKT: 'point' + }) + expect(applyDetectedGeometryColumnMapping(columnFieldLog, rows)).toEqual([ + { field: 'point', column: 'WKT', detectedGeometryMapping: true, missing: false, mandatory: true }, + { field: 'geometry', column: null, missing: true, mandatory: true } + ]) + }) + + it('detects polygon geometry from the first data row', () => { + const columnFieldLog = [ + { field: 'geometry', column: null, missing: true, mandatory: true } + ] + const rows = [ + { converted_row: { Reference: 'abc', Shape: ' MULTIPOLYGON (((-0.1 51.5,-0.2 51.6)))' } } + ] + + expect(detectGeometryColumnMapping(columnFieldLog, rows)).toEqual({ + Shape: 'geometry' + }) + expect(applyDetectedGeometryColumnMapping(columnFieldLog, rows)).toEqual([ + { field: 'geometry', column: 'Shape', detectedGeometryMapping: true, missing: false, mandatory: true } + ]) + expect(detectGeometryColumnMapping(columnFieldLog, [ + { converted_row: { Boundary: 'POLYGON ((-0.1 51.5,-0.2 51.6))' } } + ])).toEqual({ + Boundary: 'geometry' + }) + }) + + it('does not detect geometry when point or geometry are not expected fields', () => { + expect(detectGeometryColumnMapping([ + { field: 'reference', column: null, missing: true, mandatory: true } + ], [ + { converted_row: { WKT: 'POINT (-0.1 51.5)' } } + ])).toEqual({}) + }) + + it('does not override an existing point or geometry mapping', () => { + expect(detectGeometryColumnMapping([ + { field: 'geometry', column: 'Geometry', missing: false, mandatory: true }, + { field: 'point', column: null, missing: true, mandatory: true } + ], [ + { converted_row: { WKT: 'POINT (-0.1 51.5)' } } + ])).toEqual({}) + }) + + it('hides the other geometry field when one is mapped by column mapping', () => { + expect(buildExpectedFieldRows({ + columnMappingRows: [ + { + column: 'WKT', + field: 'geometry', + isMapped: true, + isAutoMapped: false, + userDefined: true, + userIgnored: false + }, + { + column: '', + field: 'point', + isMapped: false, + userDefined: false, + userIgnored: false + } + ], + specFields: ['geometry', 'point'], + requiredFields: ['geometry', 'point'] + }).map(row => row.field)).toEqual(['geometry']) + }) + + it('preselects detected geometry mappings as editable field rows', () => { + const columnMappingRows = buildColumnMappingRows({ + columnFieldLog: applyDetectedGeometryColumnMapping([ + { field: 'geometry', column: null, missing: true, mandatory: true }, + { field: 'point', column: null, missing: true, mandatory: true } + ], [ + { converted_row: { WKT: 'POLYGON ((-0.1 51.5,-0.2 51.6))' } } + ]), + userColumnMapping: {} + }) + + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['geometry', 'point'], + requiredFields: ['geometry', 'point'] + })).toEqual([ + { + field: 'geometry', + column: 'WKT', + isMapped: true, + isAutoMapped: false, + isEditable: true, + userDefined: true, + userIgnored: false, + isRequired: true + } + ]) + }) + it('sorts expected field rows by automapped, required, then user-defined priority', () => { const columnMappingRows = [ { From 575f670ebd41f77db9ae6f88e5774f841ac78831 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 15 Jun 2026 09:12:03 +0100 Subject: [PATCH 18/21] Add column mapping functionality to status page and related components --- src/assets/js/statusPage.js | 36 ++++- src/content/statusPage.js | 15 ++- src/controllers/statusController.js | 124 +----------------- src/routes/api.js | 24 +++- src/services/columnMappingDecider.js | 110 ++++++++++++++++ src/views/check/statusPage/status.html | 6 +- .../check/statusPage/statusContentMacro.html | 5 +- test/unit/statusPage.test.js | 23 ++++ 8 files changed, 211 insertions(+), 132 deletions(-) create mode 100644 src/services/columnMappingDecider.js diff --git a/src/assets/js/statusPage.js b/src/assets/js/statusPage.js index 2664058f1..9cc290b86 100644 --- a/src/assets/js/statusPage.js +++ b/src/assets/js/statusPage.js @@ -16,6 +16,7 @@ export default class StatusPage { this.interval = null this.heading = document.querySelector('#js-async-processing-heading') this.processingMessage = document.querySelector('#js-async-processing-message') + this.secondaryProcessingMessage = document.querySelector('#js-async-processing-secondary-message') this.continueButton = document.querySelector('#js-async-continue-button') } @@ -29,7 +30,11 @@ export default class StatusPage { console.info('StatusPage: polled request and got a status of: ' + data.status) // ToDo: handle other status' here if (finishedProcessingStatuses.includes(data.status)) { - this.updatePageToComplete() + if (data.showColumnMapping) { + this.updatePageToColumnMapping(data) + } else { + this.updatePageToComplete() + } clearInterval(interval) } }).catch((reason) => { @@ -49,7 +54,9 @@ export default class StatusPage { updatePageToChecking () { // update the page this.heading.textContent = headingTexts.checking + this.processingMessage.style.display = 'block' this.processingMessage.textContent = messageTexts.checking + this.hideSecondaryMessage() this.continueButton.style.display = 'none' } @@ -57,20 +64,47 @@ export default class StatusPage { // update the page this.heading.textContent = headingTexts.checked this.processingMessage.style.display = 'none' + this.hideSecondaryMessage() this.continueButton.textContent = buttonTexts.checked this.continueButton.ariaLabel = buttonAriaLabels.checked this.continueButton.style.display = 'block' } + updatePageToColumnMapping (data) { + // update the page to show column mapping action + this.heading.textContent = headingTexts.columnMapping + this.processingMessage.style.display = 'block' + this.processingMessage.textContent = messageTexts.columnMapping.primary + this.showSecondaryMessage(messageTexts.columnMapping.secondary) + this.continueButton.textContent = buttonTexts.columnMapping + this.continueButton.ariaLabel = buttonAriaLabels.columnMapping + this.continueButton.style.display = 'block' + } + updatePageForPollingTimeout () { // update the page this.heading.textContent = headingTexts.checking this.processingMessage.style.display = 'block' this.processingMessage.textContent = messageTexts.checking + this.hideSecondaryMessage() this.continueButton.textContent = buttonTexts.checking this.continueButton.ariaLabel = buttonAriaLabels.checking this.continueButton.style.display = 'block' } + + hideSecondaryMessage () { + if (!this.secondaryProcessingMessage) return + + this.secondaryProcessingMessage.textContent = '' + this.secondaryProcessingMessage.style.display = 'none' + } + + showSecondaryMessage (message) { + if (!this.secondaryProcessingMessage) return + + this.secondaryProcessingMessage.textContent = message + this.secondaryProcessingMessage.style.display = 'block' + } } window.addEventListener('load', () => { diff --git a/src/content/statusPage.js b/src/content/statusPage.js index 64e3c2f30..95ac7fcd1 100644 --- a/src/content/statusPage.js +++ b/src/content/statusPage.js @@ -1,19 +1,26 @@ export const headingTexts = { checking: 'Checking your data', - checked: 'Data checked' + checked: 'Data checked', + columnMapping: 'You need to map your fields' } export const messageTexts = { checking: 'Do not close this page. Larger files may take longer.', - checked: 'You can continue' + checked: 'You can continue', + columnMapping: { + primary: 'We could not map all of the fields in your data to what is in the standard.', + secondary: 'For all expected fields, you need to select a field found in your data or "not provided".' + } } export const buttonTexts = { checking: 'Check latest status', - checked: 'Continue' + checked: 'Continue', + columnMapping: 'Map your fields' } export const buttonAriaLabels = { checking: 'Check latest status of data check', - checked: 'Continue to next step' + checked: 'Continue to next step', + columnMapping: 'Map your fields' } diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index a1ec9bc0f..d768251cd 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -2,13 +2,11 @@ import PageController from './pageController.js' import { getRequestData } from '../services/asyncRequestApi.js' import { finishedProcessingStatuses } from '../utils/utils.js' import { headingTexts, messageTexts, buttonTexts, buttonAriaLabels } from '../content/statusPage.js' -import { isStatutoryDataset } from '../utils/redisLoader.js' import platformApi from '../services/platformApi.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { processSpecificationMiddlewares } from '../middleware/common.middleware.js' -import datasette from '../services/datasette.js' -import { addQualityCriteriaLevels } from './resultsController.js' +import { shouldShowColumnMapping } from '../services/columnMappingDecider.js' /** * Attempts to infer how we ended up on this page. @@ -76,6 +74,9 @@ class StatusController extends PageController { try { req.form.options.data = await getRequestData(req.params.id) req.form.options.processingComplete = finishedProcessingStatuses.includes(req.form.options.data.status) + req.form.options.showColumnMapping = req.form.options.processingComplete + ? await shouldShowColumnMapping(req.form.options.data, req.uniqueDatasetFields || []) + : false req.form.options.headingTexts = headingTexts req.form.options.messageTexts = messageTexts req.form.options.buttonTexts = buttonTexts @@ -96,121 +97,6 @@ class StatusController extends PageController { } } -export async function shouldShowColumnMapping (requestData, uniqueDatasetFields = []) { - if (!requestData?.isComplete?.() || requestData?.isFailed?.()) { - return false - } - - const params = requestData.getParams?.() ?? {} - - try { - if (await isStatutoryDataset({ - organisation: params.organisationName, - dataset: params.dataset - })) { - return false - } - if (await hasBlockingNonColumnMappingTasks(requestData)) return false - - let columnMapping = requestData.getColumnFieldLog?.() ?? [] - - if (uniqueDatasetFields.length > 0) { - columnMapping = columnMapping.filter(entry => uniqueDatasetFields.includes(entry?.field)) - } - let allFieldsInDataset = columnMapping.map(column => column?.field).filter(Boolean) - // filter out point or geometry field depending on which is present. - // If geometry is mapped, do not show point. If point is mapped, do not show geometry. - // If both are mapped, no action needed. If neither are mapped, show both options - const geometryItem = columnMapping.find(column => column?.field?.toLowerCase() === 'geometry') ?? null - const pointFieldItem = columnMapping.find(column => column?.field?.toLowerCase() === 'point') ?? null - const geometryMapped = Boolean(geometryItem?.column) - const pointMapped = Boolean(pointFieldItem?.column) - if (geometryMapped && !pointMapped) { - allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'point') - } else if (pointMapped && !geometryMapped) { - allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'geometry') - } - if (allFieldsInDataset.length === 0) return false - - // the column mapping the user has done - const userColumnMapping = params.column_mapping ?? {} - const hasUserColumnMapping = Object.values(userColumnMapping).some(value => Boolean(value)) - if (hasUserColumnMapping) return false - - const responseDetails = await requestData.fetchResponseDetails(0, 50) - const rows = responseDetails.getRows?.() ?? [] - // all the columns the user has mapped. - const fieldsMappedByUser = buildMappedFields(columnMapping, userColumnMapping) - - // all the fields in the dataset that has not been mapped - const unmappedDatasetFields = new Set(allFieldsInDataset.filter(field => !fieldsMappedByUser.has(field))) - - const hasUnmappedExpectedFields = unmappedDatasetFields.size > 0 - // there are no dataset fields that are unmapped, so no need to show column mapping page - if (!hasUnmappedExpectedFields) return false - - // the uploaded columns that have not been mapped either automatically or by the user (they are available for mapping) - const spareUploadedColumns = buildSpareUploadedColumns(columnMapping, rows, userColumnMapping) - - // if there are no uploaded columns that can be mapped, then there is no point showing the column mapping page - if (spareUploadedColumns.length === 0) return false - - // if there are missing mandatory fields that are not mapped by the user, then we should show the column mapping page - const hasMissingMandatoryFields = columnMapping.some(column => column?.mandatory && !column?.column && !fieldsMappedByUser.has(column.field)) - // Note: at this point we've already ensured there are spare uploaded columns (earlier check), - // so a missing mandatory field with spare columns means we should show the mapping page. - if (hasMissingMandatoryFields) return true - - // Show column mapping if there are unmapped expected fields (and spareUploadedColumns > 0 ensured earlier) - return hasUnmappedExpectedFields - } catch { - return false - } -} - -export async function hasBlockingNonColumnMappingTasks (requestData) { - const issueTasks = requestData.getIssueTasks?.() ?? [] - if (issueTasks.length === 0) return false - - const { formattedData: issueTypes } = await datasette.runQuery(` - select issue_type, quality_criteria_level - from issue_type - `) - - return addQualityCriteriaLevels(issueTasks, issueTypes).some(issue => - issue.responsibility !== 'internal' && - issue.quality_criteria_level === 2 && - !['missing column', 'missing-field'].includes(issue['issue-type']) - ) -} - -function buildMappedFields (columnMapping = [], userColumnMapping = {}) { - const fields = new Set() - - columnMapping.forEach(column => { - if (column?.field && column?.column && !column?.missing) fields.add(column.field) - }) - - Object.values(userColumnMapping).forEach(field => { - if (field && field !== 'IGNORE') fields.add(field) - }) - - return fields -} - -// uploaded columns that have not been mapped either automatically or by the user (they are available for mapping) -function buildSpareUploadedColumns (columnMapping = [], rows = [], userColumnMapping = {}) { - const mappedColumns = new Set(columnMapping.map(column => column?.column).filter(Boolean)) - Object.entries(userColumnMapping).forEach(([column, field]) => { - if (field) mappedColumns.add(column) - }) - - const uploadedColumns = new Set() - rows.forEach(row => { - Object.keys(row?.converted_row ?? {}).forEach(column => uploadedColumns.add(column)) - }) - - return [...uploadedColumns].filter(column => !mappedColumns.has(column)) -} +export { shouldShowColumnMapping } export default StatusController diff --git a/src/routes/api.js b/src/routes/api.js index 143d77037..546830ed6 100644 --- a/src/routes/api.js +++ b/src/routes/api.js @@ -1,5 +1,6 @@ import express from 'express' import { getRequestData } from '../services/asyncRequestApi.js' +import { shouldShowColumnMapping } from '../services/columnMappingDecider.js' import { getBoundaryForLpa } from '../services/boundaryService.js' import { getOsMapAccessToken } from '../services/osMapService.js' @@ -16,11 +17,24 @@ const router = express.Router() */ router.get('/status/:result_id', async (req, res) => { res.set('Cache-Control', 'no-store') - const response = getRequestData(req.params.result_id) - .then(data => res.json(data)) - .catch(error => res.status(500).json({ error })) - - return response + try { + const resultData = await getRequestData(req.params.result_id) + // serialize the result data (plain properties) + const payload = { ...resultData } + // compute whether we should show column mapping when the request finished + if (typeof resultData.isComplete === 'function' && resultData.isComplete() && !resultData.isFailed?.()) { + try { + const show = await shouldShowColumnMapping(resultData, []) + payload.showColumnMapping = show + if (show) payload.columnMappingUrl = `/check/column-mapping/${resultData.id}` + } catch (e) { + // swallow and continue without the flag + } + } + return res.json(payload) + } catch (error) { + return res.status(500).json({ error }) + } }) /** diff --git a/src/services/columnMappingDecider.js b/src/services/columnMappingDecider.js new file mode 100644 index 000000000..9231d6d03 --- /dev/null +++ b/src/services/columnMappingDecider.js @@ -0,0 +1,110 @@ +import datasette from '../services/datasette.js' +import { isStatutoryDataset } from '../utils/redisLoader.js' +import { addQualityCriteriaLevels } from '../controllers/resultsController.js' + +export async function shouldShowColumnMapping (requestData, uniqueDatasetFields = []) { + if (!requestData?.isComplete?.() || requestData?.isFailed?.()) { + return false + } + + const params = requestData.getParams?.() ?? {} + + try { + if (await isStatutoryDataset({ + organisation: params.organisationName, + dataset: params.dataset + })) { + return false + } + if (await hasBlockingNonColumnMappingTasks(requestData)) return false + + let columnMapping = requestData.getColumnFieldLog?.() ?? [] + + if (uniqueDatasetFields.length > 0) { + columnMapping = columnMapping.filter(entry => uniqueDatasetFields.includes(entry?.field)) + } + let allFieldsInDataset = columnMapping.map(column => column?.field).filter(Boolean) + const geometryItem = columnMapping.find(column => column?.field?.toLowerCase() === 'geometry') ?? null + const pointFieldItem = columnMapping.find(column => column?.field?.toLowerCase() === 'point') ?? null + const geometryMapped = Boolean(geometryItem?.column) + const pointMapped = Boolean(pointFieldItem?.column) + if (geometryMapped && !pointMapped) { + allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'point') + } else if (pointMapped && !geometryMapped) { + allFieldsInDataset = allFieldsInDataset.filter(field => field.toLowerCase() !== 'geometry') + } + if (allFieldsInDataset.length === 0) return false + + const userColumnMapping = params.column_mapping ?? {} + const hasUserColumnMapping = Object.values(userColumnMapping).some(value => Boolean(value)) + if (hasUserColumnMapping) return false + + const responseDetails = await requestData.fetchResponseDetails(0, 50) + const rows = responseDetails.getRows?.() ?? [] + const fieldsMappedByUser = buildMappedFields(columnMapping, userColumnMapping) + + const unmappedDatasetFields = new Set(allFieldsInDataset.filter(field => !fieldsMappedByUser.has(field))) + + const hasUnmappedExpectedFields = unmappedDatasetFields.size > 0 + if (!hasUnmappedExpectedFields) return false + + const spareUploadedColumns = buildSpareUploadedColumns(columnMapping, rows, userColumnMapping) + if (spareUploadedColumns.length === 0) return false + + const hasMissingMandatoryFields = columnMapping.some(column => column?.mandatory && !column?.column && !fieldsMappedByUser.has(column.field)) + if (hasMissingMandatoryFields) return true + + return hasUnmappedExpectedFields + } catch { + return false + } +} + +export async function hasBlockingNonColumnMappingTasks (requestData) { + const issueTasks = requestData.getIssueTasks?.() ?? [] + if (issueTasks.length === 0) return false + + const { formattedData: issueTypes } = await datasette.runQuery(` + select issue_type, quality_criteria_level + from issue_type + `) + + return addQualityCriteriaLevels(issueTasks, issueTypes).some(issue => + issue.responsibility !== 'internal' && + issue.quality_criteria_level === 2 && + !['missing column', 'missing-field'].includes(issue['issue-type']) + ) +} + +function buildMappedFields (columnMapping = [], userColumnMapping = {}) { + const fields = new Set() + + columnMapping.forEach(column => { + if (column?.field && column?.column && !column?.missing) fields.add(column.field) + }) + + Object.values(userColumnMapping).forEach(field => { + if (field && field !== 'IGNORE') fields.add(field) + }) + + return fields +} + +function buildSpareUploadedColumns (columnMapping = [], rows = [], userColumnMapping = {}) { + const mappedColumns = new Set(columnMapping.map(column => column?.column).filter(Boolean)) + Object.entries(userColumnMapping).forEach(([column, field]) => { + if (field) mappedColumns.add(column) + }) + + const uploadedColumns = new Set() + rows.forEach(row => { + Object.keys(row?.converted_row ?? {}).forEach(column => uploadedColumns.add(column)) + }) + + return [...uploadedColumns].filter(column => !mappedColumns.has(column)) +} + +export default { + shouldShowColumnMapping, + hasBlockingNonColumnMappingTasks +} diff --git a/src/views/check/statusPage/status.html b/src/views/check/statusPage/status.html index 8a3f28a4f..a429bca41 100644 --- a/src/views/check/statusPage/status.html +++ b/src/views/check/statusPage/status.html @@ -8,7 +8,11 @@ {% set serviceType = 'Check' %} {% set pageName = 'Status' %} -{% if options.processingComplete %} +{% if options.showColumnMapping %} + {% set pageContent = statusContent(options.headingTexts.columnMapping, options.messageTexts.columnMapping.primary, options.messageTexts.columnMapping.secondary) %} + {% set buttonText = buttonTexts.columnMapping %} + {% set buttonClasses = "" %} +{% elif options.processingComplete %} {% set pageContent = statusContent(options.headingTexts.checked, options.messageTexts.checked) %} {% set buttonText = buttonTexts.checked %} {% set buttonClasses = "" %} diff --git a/src/views/check/statusPage/statusContentMacro.html b/src/views/check/statusPage/statusContentMacro.html index 6b8441b36..e21da8c04 100644 --- a/src/views/check/statusPage/statusContentMacro.html +++ b/src/views/check/statusPage/statusContentMacro.html @@ -1,4 +1,5 @@ -{% macro statusContent(heading, message) %} +{% macro statusContent(heading, message, secondaryMessage = null) %}

{{heading}}

{{message}}

-{% endmacro %} \ No newline at end of file +

{{secondaryMessage}}

+{% endmacro %} diff --git a/test/unit/statusPage.test.js b/test/unit/statusPage.test.js index 422a9e052..287e1431d 100644 --- a/test/unit/statusPage.test.js +++ b/test/unit/statusPage.test.js @@ -6,6 +6,7 @@ describe('StatusPage', () => { let statusPage let mockHeading let mockMessage + let mockSecondaryMessage let mockButton beforeEach(async () => { @@ -34,6 +35,7 @@ describe('StatusPage', () => { mockHeading = { textContent: 'Checking File' } mockButton = { textContent: 'Check latest status', style: { display: 'block' } } mockMessage = { textContent: 'Please wait', style: { display: 'block' } } + mockSecondaryMessage = { textContent: '', style: { display: 'none' } } global.window = { addEventListener: vi.fn() } @@ -46,6 +48,8 @@ describe('StatusPage', () => { return mockButton case '#js-async-processing-message': return mockMessage + case '#js-async-processing-secondary-message': + return mockSecondaryMessage default: return null } @@ -77,6 +81,25 @@ describe('StatusPage', () => { expect(statusPage.heading.textContent).toBe(headingTexts.checked) expect(statusPage.continueButton.style.display).toBe('block') expect(statusPage.processingMessage.style.display).toBe('none') + expect(statusPage.secondaryProcessingMessage.style.display).toBe('none') + }) + + it('should update the page when column mapping is required', async () => { + const mockResponse = { status: 'COMPLETE', showColumnMapping: true } + global.fetch.mockResolvedValueOnce({ + json: () => Promise.resolve(mockResponse) + }) + + statusPage.beginPolling('http://test.com', '123') + await vi.advanceTimersByTimeAsync(1000) + await Promise.resolve() + + expect(statusPage.heading.textContent).toBe(headingTexts.columnMapping) + expect(statusPage.processingMessage.textContent).toBe(messageTexts.columnMapping.primary) + expect(statusPage.secondaryProcessingMessage.textContent).toBe(messageTexts.columnMapping.secondary) + expect(statusPage.secondaryProcessingMessage.style.display).toBe('block') + expect(statusPage.continueButton.textContent).toBe('Map your fields') + expect(statusPage.continueButton.style.display).toBe('block') }) it('should begin polling and update the page when the status is FAILED', async () => { From d8e94dcca519d2337e975304196770dcfd8b29de Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 15 Jun 2026 11:41:21 +0100 Subject: [PATCH 19/21] Update column mapping messages and add guidance link in column mapping view --- src/content/statusPage.js | 4 ++-- src/views/check/column-mapping.html | 9 +++++++-- test/unit/views/check/columnMapping.test.js | 9 ++++++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/content/statusPage.js b/src/content/statusPage.js index 95ac7fcd1..0e15c2d90 100644 --- a/src/content/statusPage.js +++ b/src/content/statusPage.js @@ -8,8 +8,8 @@ export const messageTexts = { checking: 'Do not close this page. Larger files may take longer.', checked: 'You can continue', columnMapping: { - primary: 'We could not map all of the fields in your data to what is in the standard.', - secondary: 'For all expected fields, you need to select a field found in your data or "not provided".' + primary: 'We could not map all of the fields in your data to what is in the data standard.', + secondary: "For all expected fields, you need to select a field found in your data or 'not provided'." } } diff --git a/src/views/check/column-mapping.html b/src/views/check/column-mapping.html index 7aab82858..bc217741e 100644 --- a/src/views/check/column-mapping.html +++ b/src/views/check/column-mapping.html @@ -43,15 +43,20 @@

{{ pageName }}

We could not map all of the fields in your data to what is in the data standard.

-

You need to select a field for all expected fields.

+

For all expected fields, you need to select a field found in your data or 'not provided'.

{{ columnMapping(options.mappingRows, options.uploadedColumns, options.columnMappingErrors,false) }}
+
+ {% set datasetGuidanceUrl = options.requestParams.dataset | getDatasetGuidanceUrl %} + {% if datasetGuidanceUrl %} + Find out what we expect in each field (opens in new tab) + {% endif %} -
+
diff --git a/test/unit/views/check/columnMapping.test.js b/test/unit/views/check/columnMapping.test.js index b8fbc3b40..8a635a576 100644 --- a/test/unit/views/check/columnMapping.test.js +++ b/test/unit/views/check/columnMapping.test.js @@ -11,7 +11,7 @@ describe('check/column-mapping.html', () => { lastPage: '/check/status/123', requestParams: { organisationName: 'local-authority:ABC', - dataset: 'conservation-area' + dataset: 'article-4-direction' }, mappingRows: [ { @@ -53,6 +53,13 @@ describe('check/column-mapping.html', () => { expect(notesRow.className).toContain('govuk-form-group--error') expect(notesRow.textContent).toContain('notes') expect(document.querySelector('.govuk-form-group').className).not.toContain('govuk-form-group--error') + const guidanceLink = [...document.querySelectorAll('a')].find(link => + link.textContent.includes('Find out what we expect in each field') + ) + expect(guidanceLink).not.toBeUndefined() + expect(guidanceLink.getAttribute('href')).toBe('/guidance/specifications/article-4-direction') + expect(guidanceLink.target).toBe('_blank') + expect(guidanceLink.rel).toBe('noopener noreferrer') expect(document.querySelector('button[form="columnMappingForm"]').textContent).toContain('Check your data') }) From b20bf191229515be9dbc3effae5c9ee5b5db8dc7 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 15 Jun 2026 13:57:49 +0100 Subject: [PATCH 20/21] Refactor column mapping logic to remove unused variable and streamline data preparation --- src/controllers/columnMappingController.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index 05a084a4c..18c713c75 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -96,15 +96,13 @@ class ColumnMappingController extends PageController { const params = requestData.getParams() ?? {} const uniqueDatasetFields = req.uniqueDatasetFields || [] - const { columnMappingRows, detectedGeometryMapping } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) - const spareUploadedColumns = buildSelectableColumns(columnMappingRows) + const { detectedGeometryMapping } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) const columnMapping = buildSubmittedColumnMapping({ existingMapping: { ...(params.column_mapping ?? {}), ...detectedGeometryMapping }, - body: req.body, - spareUploadedColumns + body: req.body }) const newRequestId = await postCheckRequest({ From 386bf1b9694c3b924ffd552a71cd69164217ed66 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Tue, 16 Jun 2026 09:21:38 +0100 Subject: [PATCH 21/21] Lock detected geometry mappings and update related logic in column mapping --- src/controllers/columnMappingController.js | 34 +++++++++++++++--- src/views/check/column-mapping.html | 2 ++ test/unit/columnMappingController.test.js | 42 +++++++++++++++++++--- 3 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/controllers/columnMappingController.js b/src/controllers/columnMappingController.js index 18c713c75..2c84d5984 100644 --- a/src/controllers/columnMappingController.js +++ b/src/controllers/columnMappingController.js @@ -7,6 +7,9 @@ import platformApi from '../services/platformApi.js' import { types } from '../utils/logging.js' import logger from '../utils/logger.js' +const LOCK_DETECTED_GEOMETRY_MAPPINGS = true +const GEOMETRY_FIELDS = ['geometry', 'point'] + class ColumnMappingController extends PageController { middlewareSetup () { super.middlewareSetup() @@ -167,6 +170,7 @@ async function prepareColumnMappingContext (requestData, uniqueDatasetFields = [ const params = requestData.getParams() ?? {} const userColumnMapping = params.column_mapping ?? {} + columnFieldLog = applyPersistedDetectedGeometryColumnMapping(columnFieldLog, responseRows, userColumnMapping) const columnMappingRows = buildColumnMappingRows({ columnFieldLog, userColumnMapping @@ -337,15 +341,16 @@ export function buildColumnMappingRows ({ columnFieldLog = [], userColumnMapping const userMappedField = userColumnMapping[column] const isDetectedGeometryMapping = entry?.detectedGeometryMapping === true + const isLockedDetectedGeometryMapping = LOCK_DETECTED_GEOMETRY_MAPPINGS && isDetectedGeometryMapping const field = entry.field const isMapped = Boolean(field) && Boolean(column) rows.push({ column, field, isMapped, - isAutoMapped: isMapped && !userMappedField && !isDetectedGeometryMapping, + isAutoMapped: isMapped && (isLockedDetectedGeometryMapping || (!userMappedField && !isDetectedGeometryMapping)), isMissing: entry.missing, - userDefined: Boolean(userMappedField || isDetectedGeometryMapping) + userDefined: Boolean((userMappedField || isDetectedGeometryMapping) && !isLockedDetectedGeometryMapping) }) }) @@ -378,17 +383,16 @@ export function applyDetectedGeometryColumnMapping (columnFieldLog = [], respons } export function detectGeometryColumnMapping (columnFieldLog = [], responseRows = []) { - const geometryFields = ['geometry', 'point'] const expectedGeometryFields = new Set( columnFieldLog .map(entry => entry?.field) - .filter(field => geometryFields.includes(field)) + .filter(field => GEOMETRY_FIELDS.includes(field)) ) if (expectedGeometryFields.size === 0) return {} const alreadyMapped = columnFieldLog.some(entry => - geometryFields.includes(entry?.field) && Boolean(entry?.column) + GEOMETRY_FIELDS.includes(entry?.field) && Boolean(entry?.column) ) if (alreadyMapped) return {} @@ -400,6 +404,26 @@ export function detectGeometryColumnMapping (columnFieldLog = [], responseRows = } } +function applyPersistedDetectedGeometryColumnMapping (columnFieldLog = [], responseRows = [], userColumnMapping = {}) { + if (!LOCK_DETECTED_GEOMETRY_MAPPINGS) return columnFieldLog + + const firstRow = responseRows[0]?.converted_row ?? {} + const detectedUserMappings = Object.entries(userColumnMapping) + .filter(([column, field]) => GEOMETRY_FIELDS.includes(field) && getGeometryFieldForValue(firstRow[column]) === field) + + if (detectedUserMappings.length === 0) return columnFieldLog + + return columnFieldLog.map(entry => { + const isPersistedDetectedMapping = detectedUserMappings.some(([column, field]) => + entry?.column === column && entry?.field === field + ) + + return isPersistedDetectedMapping + ? { ...entry, detectedGeometryMapping: true } + : entry + }) +} + export function detectGeometryColumnFromFirstRow (responseRows = [], expectedFields = new Set(['geometry', 'point'])) { const firstRow = responseRows[0]?.converted_row ?? {} diff --git a/src/views/check/column-mapping.html b/src/views/check/column-mapping.html index bc217741e..d8629bc39 100644 --- a/src/views/check/column-mapping.html +++ b/src/views/check/column-mapping.html @@ -52,7 +52,9 @@

{{ pageName }}


{% set datasetGuidanceUrl = options.requestParams.dataset | getDatasetGuidanceUrl %} {% if datasetGuidanceUrl %} +

Find out what we expect in each field (opens in new tab) +

{% endif %} diff --git a/test/unit/columnMappingController.test.js b/test/unit/columnMappingController.test.js index c4b734097..20c40249a 100644 --- a/test/unit/columnMappingController.test.js +++ b/test/unit/columnMappingController.test.js @@ -286,7 +286,7 @@ describe('columnMappingController helpers', () => { }).map(row => row.field)).toEqual(['geometry']) }) - it('preselects detected geometry mappings as editable field rows', () => { + it('locks detected geometry mappings as mapped field rows', () => { const columnMappingRows = buildColumnMappingRows({ columnFieldLog: applyDetectedGeometryColumnMapping([ { field: 'geometry', column: null, missing: true, mandatory: true }, @@ -306,9 +306,43 @@ describe('columnMappingController helpers', () => { field: 'geometry', column: 'WKT', isMapped: true, - isAutoMapped: false, - isEditable: true, - userDefined: true, + isAutoMapped: true, + isEditable: false, + userDefined: false, + userIgnored: false, + isRequired: true + } + ]) + }) + + it('keeps detected geometry mappings locked after they are persisted as column mappings', () => { + const columnMappingRows = buildColumnMappingRows({ + columnFieldLog: [ + { + field: 'geometry', + column: 'WKT', + detectedGeometryMapping: true, + missing: false, + mandatory: true + } + ], + userColumnMapping: { + WKT: 'geometry' + } + }) + + expect(buildExpectedFieldRows({ + columnMappingRows, + specFields: ['geometry'], + requiredFields: ['geometry'] + })).toEqual([ + { + field: 'geometry', + column: 'WKT', + isMapped: true, + isAutoMapped: true, + isEditable: false, + userDefined: false, userIgnored: false, isRequired: true }