From c3d1ad63ce2c6faada4bae9787a2dd624b4a22f8 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 13:43:25 +0000 Subject: [PATCH 01/83] update tasklist to show only relevant issues --- src/middleware/common.middleware.js | 31 +++++- src/middleware/datasetTaskList.middleware.js | 107 +++++++++++-------- src/services/performanceDbApi.js | 6 +- test/unit/performanceDbApi.test.js | 6 +- 4 files changed, 99 insertions(+), 51 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index ebc62aed4..b3db166fc 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -429,6 +429,30 @@ export const FilterOutIssuesToMostRecent = (req, res, next) => { next() } +export const fetchIssueTypes = fetchMany({ + query: () => 'SELECT * FROM issue_type', + result: 'issueTypes' +}) + +export const addIssueSeverityToIssues = (req, res, next) => { + const { issues, issueTypes } = req + + req.issues = issues.map(issue => { + const issueTypeData = issueTypes.find(issueType => issueType.issue_type === issue.issue_type) + return { ...issue, severity: issueTypeData.severity } + }) + + next() +} + +const filterOutNonErrorSeverityIssues = (req, res, next) => { + const { issues } = req + + req.issues = issues.filter(issue => issue.severity === 'error') + + next() +} + export const removeIssuesThatHaveBeenFixed = async (req, res, next) => { const { issues, resources } = req @@ -530,6 +554,9 @@ export const addReferencesToIssues = (req, res, next) => { export const processRelevantIssuesMiddlewares = [ fetchEntityIssuesForFieldAndType, FilterOutIssuesToMostRecent, + fetchIssueTypes, + addIssueSeverityToIssues, + filterOutNonErrorSeverityIssues, removeIssuesThatHaveBeenFixed, fetchFieldMappings, addFieldMappingsToIssue, @@ -607,7 +634,7 @@ export function getErrorSummaryItems (req, res, next) { const error = new Error('issue count must be larger than 0') return next(error) } else if (issues.length < totalRecordCount) { - errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) + errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, rowCount: totalRecordCount, field: issueField }, true) issueItems = issues.map((issue, i) => { const pageNum = i + 1 let inString = '' @@ -627,7 +654,7 @@ export function getErrorSummaryItems (req, res, next) { }) } else { issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, rowCount: totalRecordCount, field: issueField }, true) }] } diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 284dd649d..4f8277aa0 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -1,16 +1,12 @@ import { fetchDatasetInfo, - isResourceAccessible, - isResourceNotAccessible, - fetchLatestResource, - fetchEntityCount, - logPageError, - fetchLpaDatasetIssues, validateQueryParams, - getDatasetTaskListError, - isResourceIdValid, and + fetchResources, + processEntitiesMiddlewares, + processRelevantIssuesMiddlewares, + fetchOrgInfo } from './common.middleware.js' -import { fetchOne, fetchIf, onlyIf, renderTemplate } from './middleware.builders.js' +import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' import { statusToTagClass } from '../filters/filters.js' import * as v from 'valibot' @@ -23,13 +19,6 @@ export const fetchResourceStatus = fetchOne({ result: 'resourceStatus' }) -const fetchOrgInfoWithStatGeo = fetchOne({ - query: ({ params }) => { - return /* sql */ `SELECT name, organisation, statistical_geography FROM organisation WHERE organisation = '${params.lpa}'` - }, - result: 'orgInfo' -}) - /** * Returns a status tag object with a text label and a CSS class based on the status. * @@ -45,6 +34,49 @@ function getStatusTag (status) { } } +export const prepareTasks = (req, res, next) => { + const { lpa, dataset } = req.parsedParams + const { issues, entities, resources } = req + + const entityCount = entities.length + + const specialIssueTypeCases = ['reference values are not unique'] + + const groupedIssues = issues.reduce((acc, issue) => { + const { field, issue_type: type } = issue + const key = `${field}_${type}` + if (acc[key]) { + acc[key].count += 1 + } else { + acc[key] = { + type, + field, + count: 1 + } + } + return acc + }, {}) + + req.taskList = Object.values(groupedIssues).map(({ field, type, count }) => { + // if the issue doesn't have an entity, or is one of the special case issue types then we should use the resource_row_count + + let rowCount = entityCount + if (specialIssueTypeCases.includes(type)) { + rowCount = resources[0].entry_count + } + + return { + title: { + text: performanceDbApi.getTaskMessage({ num_issues: count, rowCount, field, issue_type: type }) + }, + href: `/organisations/${lpa}/${dataset}/${type}/${field}`, + status: getStatusTag('Needs fixing') + } + }) + + next() +} + /** * Middleware. Updates req with `templateParams` * @@ -54,20 +86,7 @@ function getStatusTag (status) { * @returns { { templateParams: object }} */ export const prepareDatasetTaskListTemplateParams = (req, res, next) => { - const { issues, entityCount: entityCountRow, params, dataset, orgInfo: organisation } = req - const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } - const { lpa, dataset: datasetId } = params - console.assert(typeof entityCount === 'number', 'entityCount should be a number') - - const taskList = issues.map((issue) => { - return { - title: { - text: performanceDbApi.getTaskMessage({ ...issue, entityCount, field: issue.field }) - }, - href: `/organisations/${lpa}/${datasetId}/${issue.issue_type}/${issue.field}`, - status: getStatusTag(issue.status) - } - }) + const { taskList, dataset, orgInfo: organisation } = req req.templateParams = { taskList, @@ -123,20 +142,22 @@ const validateParams = validateQueryParams({ }) }) -/* eslint-disable-next-line no-return-assign */ -const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } - export default [ validateParams, - fetchResourceStatus, - fetchOrgInfoWithStatGeo, + fetchOrgInfo, fetchDatasetInfo, - fetchIf(isResourceAccessible, fetchLatestResource), - fetchIf(isResourceAccessible, fetchLpaDatasetIssues), - fetchIf(and(isResourceAccessible, isResourceIdValid), fetchEntityCount, zeroEntityCount), - onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams), - onlyIf(isResourceAccessible, getDatasetTaskList), - onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), - onlyIf(isResourceNotAccessible, getDatasetTaskListError), - logPageError + fetchResources, + ...processEntitiesMiddlewares, + ...processRelevantIssuesMiddlewares, + prepareTasks, + prepareDatasetTaskListTemplateParams, + getDatasetTaskList + // fetchIf(isResourceAccessible, fetchLatestResource), + // fetchIf(isResourceAccessible, fetchLpaDatasetIssues), + // fetchIf(and(isResourceAccessible, isResourceIdValid), fetchEntityCount, zeroEntityCount), + // onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams), + // onlyIf(isResourceAccessible, getDatasetTaskList), + // onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), + // onlyIf(isResourceNotAccessible, getDatasetTaskListError), + // logPageError ] diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 442ee1793..d7088d7ac 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -188,7 +188,7 @@ export default { /** * Returns a task message based on the provided issue type, issue count, and entity count. * - * @param {{issue_type: string, num_issues: number, entityCount: number, field: string }} options + * @param {{issue_type: string, num_issues: number, rowCount: number, field: string }} options * @param {boolean?} entityLevel Whether to use entity-level or dataset level messaging * * @returns {string} The task message with the issue count inserted @@ -198,7 +198,7 @@ export default { getTaskMessage ({ issue_type: issueType, num_issues: numIssues, - entityCount, + rowCount, field }, entityLevel = false) { const messageInfo = messages.get(issueType) @@ -216,7 +216,7 @@ export default { } let message - if (entityCount && numIssues >= entityCount) { + if (rowCount && numIssues >= rowCount) { message = messageInfo.allRows_message } else if (entityLevel) { message = numIssues === 1 diff --git a/test/unit/performanceDbApi.test.js b/test/unit/performanceDbApi.test.js index c1fb07fef..1ed4c1caf 100644 --- a/test/unit/performanceDbApi.test.js +++ b/test/unit/performanceDbApi.test.js @@ -83,12 +83,12 @@ describe('performanceDbApi', () => { }) it('returns an entity-level message when entityLevel is true', () => { - const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 1, entityCount: 2 }, true) + const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 1, rowCount: 2 }, true) expect(message).toContain('singular entities message for value') }) - it('returns an "all rows" message when num_issues >= entityCount', () => { - const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 5, entityCount: 5, field: 'some_field' }) + it('returns an "all rows" message when num_issues >= rowCount', () => { + const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 5, rowCount: 5, field: 'some_field' }) expect(message).toContain('all rows message for some_field') }) From 0a9d508ccd3db016b5adba92e00c2c89ae51f08e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 9 Dec 2024 11:23:54 +0000 Subject: [PATCH 02/83] fix bug for invalid param --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 184fbe697..45539a734 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -627,7 +627,7 @@ export function getErrorSummaryItems (req, res, next) { const errorHeading = '' const issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, rowCount: totalRecordCount, field: issueField }, true) }] req.errorSummary = { From af39dc945a694711a782ff38de67ed4b1d5b4577 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 9 Dec 2024 12:48:43 +0000 Subject: [PATCH 03/83] fix tests --- .../unit/middleware/common.middleware.test.js | 11 +++- .../datasetTaskList.middleware.test.js | 56 +------------------ 2 files changed, 10 insertions(+), 57 deletions(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 7af60dd68..0ff69cc26 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1318,7 +1318,7 @@ describe('getErrorSummaryItems', () => { expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ issue_type: 'issue-type-value', num_issues: 0, - entityCount: 0, + rowCount: 0, field: 'issue-field-value' }, true) }) @@ -1350,7 +1350,7 @@ describe('getErrorSummaryItems', () => { expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ issue_type: 'issue-type-value', num_issues: 2, - entityCount: 2, + rowCount: 2, field: 'issue-field-value' }, true) }) @@ -1384,7 +1384,12 @@ describe('getErrorSummaryItems', () => { } ]) - expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ issue_type: 'issue-type-value', num_issues: 2, entityCount: 3, field: 'issue-field-value' }, true) + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type-value', + num_issues: 2, + rowCount: 3, + field: 'issue-field-value' + }, true) }) }) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index d176617b1..870ca4d8c 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -1,5 +1,4 @@ import { describe, it, vi, expect } from 'vitest' -import performanceDbApi from '../../../src/services/performanceDbApi.js' import { prepareDatasetTaskListTemplateParams, prepareDatasetTaskListErrorTemplateParams } from '../../../src/middleware/datasetTaskList.middleware.js' vi.mock('../../../src/services/performanceDbApi.js') @@ -8,68 +7,17 @@ describe('datasetTaskList.middleware.js', () => { describe('prepareDatasetTaskListParams', () => { it('sets the correct template params on the request object', async () => { const req = { - params: { lpa: 'example-lpa', dataset: 'example-dataset' }, - resourceStatus: { - resource: 'mock-resource', - endpoint_url: 'http://example.com/resource', - status: '200', - latest_log_entry_date: '', - days_since_200: 0 - }, orgInfo: { name: 'Example Organisation', organisation: 'ORG' }, dataset: { name: 'Example Dataset' }, - resource: { resource: 'mock-resource' }, - issues: [ - { - issue: 'Example issue 1', - issue_type: 'Example issue type 1', - field: 'Example issue field 1', - num_issues: 1, - status: 'Error' - }, - { - issue: 'Example issue 2', - issue_type: 'Example issue type 2', - field: 'Example issue field 2', - num_issues: 1, - status: 'Needs fixing' - } - ] + taskList: 'taskList' } const res = { render: vi.fn() } const next = vi.fn() - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce('task message 1').mockReturnValueOnce('task message 2') - prepareDatasetTaskListTemplateParams(req, res, next) const templateParams = { - taskList: [ - { - title: { - text: 'task message 1' - }, - href: '/organisations/example-lpa/example-dataset/Example issue type 1/Example issue field 1', - status: { - tag: { - classes: 'govuk-tag--red', - text: 'Error' - } - } - }, - { - title: { - text: 'task message 2' - }, - href: '/organisations/example-lpa/example-dataset/Example issue type 2/Example issue field 2', - status: { - tag: { - classes: 'govuk-tag--yellow', - text: 'Needs fixing' - } - } - } - ], + taskList: 'taskList', organisation: { name: 'Example Organisation', organisation: 'ORG' }, dataset: { name: 'Example Dataset' } } From 8c89de9770a1a8112f8c9ca1bb031679cb4be060 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 9 Dec 2024 13:33:50 +0000 Subject: [PATCH 04/83] remove comments --- src/middleware/datasetTaskList.middleware.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 4f8277aa0..770c8471a 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -152,12 +152,4 @@ export default [ prepareTasks, prepareDatasetTaskListTemplateParams, getDatasetTaskList - // fetchIf(isResourceAccessible, fetchLatestResource), - // fetchIf(isResourceAccessible, fetchLpaDatasetIssues), - // fetchIf(and(isResourceAccessible, isResourceIdValid), fetchEntityCount, zeroEntityCount), - // onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams), - // onlyIf(isResourceAccessible, getDatasetTaskList), - // onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), - // onlyIf(isResourceNotAccessible, getDatasetTaskListError), - // logPageError ] From 381095acdbf5f9a8e382be5e90815c71d7a00d6d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 11 Dec 2024 12:06:39 +0000 Subject: [PATCH 05/83] remove unneeded middleware for getting entity issues got the tasklist page working with all active endpoints --- src/middleware/common.middleware.js | 162 +++++++----------- src/middleware/datasetTaskList.middleware.js | 13 +- .../entryIssueDetails.middleware.js | 25 +-- src/middleware/issueTable.middleware.js | 17 +- src/utils/utils.js | 15 ++ 5 files changed, 95 insertions(+), 137 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 45539a734..6826bef54 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -1,5 +1,6 @@ import logger from '../utils/logger.js' import { types } from '../utils/logging.js' +import { entryIssueGroups } from '../utils/utils.js' import performanceDbApi from '../services/performanceDbApi.js' import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -187,7 +188,7 @@ export const createPaginationTemplateParams = (req, res, next) => { export const fetchResources = fetchMany({ query: ({ req }) => ` - SELECT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r + SELECT DISTINCT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r LEFT JOIN resource_organisation ro ON ro.resource = r.resource LEFT JOIN resource_dataset rd ON rd.resource = r.resource LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource @@ -368,91 +369,24 @@ export const filterOutEntitiesWithoutIssues = (req, res, next) => { const fetchEntityIssuesForFieldAndType = fetchMany({ query: ({ req, params }) => { - const issueTypeFilter = params.issue_type ? `AND issue_type = '${params.issue_type}'` : '' - const issueFieldFilter = params.issue_field ? `AND field = '${params.issue_field}'` : '' - + const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' + const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' return ` - SELECT e.entity, i.* FROM entity e - INNER JOIN issue i ON e.entity = i.entity - WHERE e.organisation_entity = ${req.orgInfo.entity} - ${issueTypeFilter} - ${issueFieldFilter}` + select * + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource = '${req.resources[0].resource}' + ${issueTypeClause} + AND it.responsibility = 'external' + AND it.severity = 'error' + ${issueFieldClause} + AND entity != '' + ` + // LIMIT ${req.dataRange.pageLength} OFFSET ${req.dataRange.offset} }, - dataset: FetchOptions.fromParams, result: 'issues' }) -export const FilterOutIssuesToMostRecent = (req, res, next) => { - const { resources, issues } = req - - const issuesWithResources = issues.filter(issue => { - if (!issue.resource || !resources.find(resource => resource.resource === issue.resource)) { - logger.warn(`Missing resource on issue: ${JSON.stringify(issue)}`) - return false - } - return true - }) - - const groupedIssues = issuesWithResources.reduce((acc, current) => { - current.start_date = new Date(resources.find(resource => resource.resource === current.resource)?.start_date) - const { entity, field } = current - if (!acc[entity]) { - acc[entity] = {} - } - if (!acc[entity][field]) { - acc[entity][field] = [] - } - acc[entity][field].push(current) - - return acc - }, {}) - - const recentIssues = Object.fromEntries(Object.entries(groupedIssues).map(([entityName, issuesByEntity]) => - [ - entityName, - Object.fromEntries(Object.entries(issuesByEntity).map(([field, issues]) => [ - field, - issues.sort((a, b) => b.start_date.getTime() - a.start_date.getTime())[0] - ])) - ] - )) - - const issuesFlattened = [] - - Object.values(recentIssues).forEach(issueByEntry => { - Object.values(issueByEntry).forEach(issueByField => { - issuesFlattened.push(issueByField) - }) - }) - - req.issues = issuesFlattened - next() -} - -export const fetchIssueTypes = fetchMany({ - query: () => 'SELECT * FROM issue_type', - result: 'issueTypes' -}) - -export const addIssueSeverityToIssues = (req, res, next) => { - const { issues, issueTypes } = req - - req.issues = issues.map(issue => { - const issueTypeData = issueTypes.find(issueType => issueType.issue_type === issue.issue_type) - return { ...issue, severity: issueTypeData.severity } - }) - - next() -} - -const filterOutNonErrorSeverityIssues = (req, res, next) => { - const { issues } = req - - req.issues = issues.filter(issue => issue.severity === 'error') - - next() -} - export const removeIssuesThatHaveBeenFixed = async (req, res, next) => { const { issues, resources } = req @@ -527,17 +461,50 @@ export const addFieldMappingsToIssue = (req, res, next) => { next() } -export const addReferencesToIssues = (req, res, next) => { - const { issues, entities } = req - - req.issues = issues.map(issue => { - const reference = entities.find(entity => entity.entity === issue.entity)?.reference +// We can only get the issues without entity from the latest resource as we have no way of knowing if those in previous resources have been fixed? +export const fetchEntryIssues = fetchMany({ + query: ({ req, params }) => { + const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' + const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' + return ` + select * + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource = '${req.resources[0].resource}' + ${issueTypeClause} + AND it.responsibility = 'external' + AND it.severity = 'error' + ${issueFieldClause} + AND (entity = '' OR i.issue_type in ('${entryIssueGroups.map(issue => issue.type).join("', '")}')) + LIMIT ${req.dataRange.pageLength} OFFSET ${req.dataRange.offset} + ` + }, + result: 'entryIssues' +}) - return { ...issue, reference } - }) +export const fetchEntityIssueCounts = fetchMany({ + query: ({ req }) => ` + select field, i.issue_type, COUNT(resource+line_number) as count + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') + AND entity != '' + GROUP BY field, i.issue_type + `, + result: 'entityIssueCounts' +}) - next() -} +export const fetchEntryIssueCounts = fetchMany({ + query: ({ req }) => ` + select field, i.issue_type, COUNT(resource+line_number) as count + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource = '${req.resources[0].resource}' + AND entity = '' + GROUP BY field, i.issue_type + `, + result: 'entryIssueCounts' +}) /** * This middleware chain is responsible for retrieving all entities for the given organisation, their latest issues, @@ -553,14 +520,11 @@ export const addReferencesToIssues = (req, res, next) => { */ export const processRelevantIssuesMiddlewares = [ fetchEntityIssuesForFieldAndType, - FilterOutIssuesToMostRecent, - fetchIssueTypes, - addIssueSeverityToIssues, - filterOutNonErrorSeverityIssues, - removeIssuesThatHaveBeenFixed, + // arguably removeIssuesThatHaveBeenFixed should be s step however we have only currently found one organisation, + // however this step is very time consuming, so in order to progress im commenting it out for now + // removeIssuesThatHaveBeenFixed, fetchFieldMappings, - addFieldMappingsToIssue, - addReferencesToIssues + addFieldMappingsToIssue ] // Other @@ -620,7 +584,9 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { export function getErrorSummaryItems (req, res, next) { const { issue_type: issueType, issue_field: issueField } = req.params - const { issues, issueCount, entities, resources } = req + const { entryIssues, issues: entityIssues, issueCount, entities, resources } = req + + const issues = entityIssues || entryIssues const totalRecordCount = entities ? entities.length : resources[0].entry_count const totalIssues = issueCount?.count || issues.length diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 770c8471a..fa736949d 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -3,8 +3,9 @@ import { validateQueryParams, fetchResources, processEntitiesMiddlewares, - processRelevantIssuesMiddlewares, - fetchOrgInfo + fetchOrgInfo, + fetchEntityIssueCounts, + fetchEntryIssueCounts } from './common.middleware.js' import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -36,12 +37,15 @@ function getStatusTag (status) { export const prepareTasks = (req, res, next) => { const { lpa, dataset } = req.parsedParams - const { issues, entities, resources } = req + const { entities, resources } = req + const { entryIssueCounts, entityIssueCounts } = req const entityCount = entities.length const specialIssueTypeCases = ['reference values are not unique'] + const issues = [...entryIssueCounts, ...entityIssueCounts] + const groupedIssues = issues.reduce((acc, issue) => { const { field, issue_type: type } = issue const key = `${field}_${type}` @@ -148,7 +152,8 @@ export default [ fetchDatasetInfo, fetchResources, ...processEntitiesMiddlewares, - ...processRelevantIssuesMiddlewares, + fetchEntityIssueCounts, + fetchEntryIssueCounts, prepareTasks, prepareDatasetTaskListTemplateParams, getDatasetTaskList diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 8f6cf9fcd..b7d44343c 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,5 +1,5 @@ import * as v from 'valibot' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchEntryIssues, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import { issueErrorMessageHtml } from '../utils/utils.js' @@ -40,20 +40,6 @@ export const addResourceMetaDataToResources = (req, res, next) => { next() } -// We can only get the issues without entity from the latest resource as we have no way of knowing if those in previous resources have been fixed? -const fetchEntryIssues = fetchMany({ - query: ({ req, params }) => ` - select * - from issue i - LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource = '${req.resources[0].resource}' - AND i.issue_type = '${params.issue_type}' - AND it.responsibility = 'external' - AND field = '${params.issue_field}' - `, - result: 'issues' -}) - const fetchIssueCount = fetchOne({ query: ({ req, params }) => ` select count(*) as count @@ -73,16 +59,15 @@ export const setRecordCount = (req, res, next) => { } export const prepareEntry = (req, res, next) => { - const { resources, issues } = req - const { pageNumber } = req.parsedParams + const { resources, entryIssues } = req - if (!issues[pageNumber - 1] || !resources) { + if (!entryIssues[0] || !resources) { const error = new Error('Missing required values on request object') error.status = 404 return next(error) } - const issue = issues[pageNumber - 1] + const issue = entryIssues[0] const errorMessage = issue.message || issue.issue_type @@ -134,10 +119,10 @@ export default [ fetchResources, fetchResourceMetaData, addResourceMetaDataToResources, - fetchEntryIssues, fetchIssueCount, setRecordCount, getSetDataRange(1), + fetchEntryIssues, show404IfPageNumberNotInRange, getSetBaseSubPath(['entry']), createPaginationTemplateParams, diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index bd6a9a873..8e8a7f1c1 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -10,6 +10,7 @@ import config from '../../config/index.js' import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, filterOutEntitiesWithoutIssues, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { onlyIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' +import { entryIssueGroups } from '../utils/utils.js' export const IssueTableQueryParams = v.object({ lpa: v.string(), @@ -90,22 +91,8 @@ export const prepareTemplateParams = (req, res, next) => { export const notIssueHasEntity = (req, res, next) => req.issues.length <= 0 -const redirectIssueGroups = [ - { - type: 'missing value', - field: 'reference' - }, - { - type: 'reference values are not unique', - field: 'reference' - }, - { - type: 'unknown entity - missing reference', - field: 'entity' - } -] export const issueTypeAndFieldShouldRedirect = (req, res, next) => - redirectIssueGroups.findIndex(({ type, field }) => (type === req.params.issue_type && field === req.params.issue_field)) >= 0 + entryIssueGroups.findIndex(({ type, field }) => (type === req.params.issue_type && field === req.params.issue_field)) >= 0 export const redirectToEntityView = (req, res, next) => { const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params diff --git a/src/utils/utils.js b/src/utils/utils.js index cc2b7b625..0a6676802 100644 --- a/src/utils/utils.js +++ b/src/utils/utils.js @@ -94,6 +94,21 @@ export const dataSubjects = { } } +export const entryIssueGroups = [ + { + type: 'missing value', + field: 'reference' + }, + { + type: 'reference values are not unique', + field: 'reference' + }, + { + type: 'unknown entity - missing reference', + field: 'entity' + } +] + export function makeDatasetsLookup (dataSubjects) { const lookup = new Map() for (const [key, dataSubject] of Object.entries(dataSubjects)) { From e79497a0cd7c9c19b07007ab1e895915bc6e1b99 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 11 Dec 2024 14:01:04 +0000 Subject: [PATCH 06/83] remove and fix tests --- .../unit/middleware/common.middleware.test.js | 166 +----------------- .../entryIssueDetails.middleware.test.js | 6 +- 2 files changed, 4 insertions(+), 168 deletions(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 0ff69cc26..864f21f8f 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -import { filterOutEntitiesWithoutIssues, createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems, getSetBaseSubPath, prepareIssueDetailsTemplateParams } from '../../../src/middleware/common.middleware' +import { filterOutEntitiesWithoutIssues, createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems, getSetBaseSubPath, prepareIssueDetailsTemplateParams } from '../../../src/middleware/common.middleware' import logger from '../../../src/utils/logger' import datasette from '../../../src/services/datasette.js' import performanceDbApi from '../../../src/services/performanceDbApi.js' @@ -706,170 +706,6 @@ describe('setDefaultParams', () => { }) }) -describe('FilterOutIssuesToMostRecent', () => { - it('removes issues of the same type and field and entity to only get the most recent', () => { - const req = { - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource2', start_date: '2022-01-02' }, - { resource: 'resource3', start_date: '2022-01-03' } - ], - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1' }, - { entity: 'entity1', field: 'field1', resource: 'resource2' }, - { entity: 'entity1', field: 'field2', resource: 'resource1' }, - { entity: 'entity1', field: 'field2', resource: 'resource2' }, - { entity: 'entity1', field: 'field2', resource: 'resource3' }, - { entity: 'entity2', field: 'field2', resource: 'resource1' }, - { entity: 'entity2', field: 'field2', resource: 'resource2' }, - { entity: 'entity2', field: 'field2', resource: 'resource3' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: new Date('2022-01-02') }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') } - ]) - }) - - it('leaves issues with different resources', () => { - const req = { - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource2', start_date: '2022-01-02' }, - { resource: 'resource3', start_date: '2022-01-03' } - ], - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1' }, - { entity: 'entity2', field: 'field2', resource: 'resource2' }, - { entity: 'entity3', field: 'field3', resource: 'resource3' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', resource: 'resource1', start_date: new Date('2022-01-01') }, - { entity: 'entity2', field: 'field2', resource: 'resource2', start_date: new Date('2022-01-02') }, - { entity: 'entity3', field: 'field3', resource: 'resource3', start_date: new Date('2022-01-03') } - ]) - }) - - it('handles issues with no corresponding resource', () => { - const req = { - issues: [ - { entity: 'entity1', field: 'field1', start_date: '2022-01-01', resource: 'resource1' }, - { entity: 'entity2', field: 'field2', start_date: '2022-01-01', resource: 'invalid-resource' }, - { entity: 'entity3', field: 'field3', start_date: '2022-01-02', resource: 'resource3' } - ], - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource3', start_date: '2022-01-02' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', start_date: new Date('2022-01-01'), resource: 'resource1' }, - { entity: 'entity3', field: 'field3', start_date: new Date('2022-01-02'), resource: 'resource3' } - ]) - }) - - it('handles correctly with invalid date strings in start_date', () => { - const req = { - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource2', start_date: '2022-01-02' }, - { resource: 'resource3', start_date: '2022-01-03' } - ], - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1', start_date: 'not-a-date' }, - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: '2022-01-03' }, - { entity: 'entity2', field: 'field2', resource: 'resource1', start_date: '2022-01-01' }, - { entity: 'entity2', field: 'field2', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: '2022-01-03' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: new Date('2022-01-02') }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') } - ]) - }) - - it('handles correctly with missing start_date field', () => { - const req = { - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource2', start_date: '2022-01-02' }, - { resource: 'resource3', start_date: '2022-01-03' } - ], - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1', start_date: undefined }, - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: '2022-01-03' }, - { entity: 'entity2', field: 'field2', resource: 'resource1', start_date: '2022-01-01' }, - { entity: 'entity2', field: 'field2', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: '2022-01-03' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: new Date('2022-01-02') }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') } - ]) - }) - - it('handles correctly with malformed date objects', () => { - const req = { - resources: [ - { resource: 'resource1', start_date: '2022-01-01' }, - { resource: 'resource2', start_date: '2022-01-02' }, - { resource: 'resource3', start_date: '2022-01-03' } - ], - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1', start_date: new Date(' invalid-date') }, - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: '2022-01-03' }, - { entity: 'entity2', field: 'field2', resource: 'resource1', start_date: '2022-01-01' }, - { entity: 'entity2', field: 'field2', resource: 'resource2', start_date: '2022-01-02' }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: '2022-01-03' } - ] - } - const res = {} - const next = vi.fn() - - FilterOutIssuesToMostRecent(req, res, next) - - expect(req.issues).toEqual([ - { entity: 'entity1', field: 'field1', resource: 'resource2', start_date: new Date('2022-01-02') }, - { entity: 'entity1', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') }, - { entity: 'entity2', field: 'field2', resource: 'resource3', start_date: new Date('2022-01-03') } - ]) - }) -}) - describe('removeIssuesThatHaveBeenFixed', () => { const mockDatasetteQuery = (moreRecentEntityFieldsFacts) => { datasette.runQuery.mockImplementation((query, dataset) => { diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 1ac73d14f..fae486ce6 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -79,7 +79,7 @@ describe('entryIssueDetails.middleware.test.js', () => { it('should prepare entry with correct fields', () => { const req = { resources: [{ endpoint_url: 'https://example.com' }], - issues: [ + entryIssues: [ { entry_number: 1, line_number: 2, @@ -121,7 +121,7 @@ describe('entryIssueDetails.middleware.test.js', () => { it('should call next with error if issue is missing', () => { const req = { resources: [{ endpoint_url: 'https://example.com' }], - issues: [], + entryIssues: [], parsedParams: { pageNumber: 1 } } const res = {} @@ -133,7 +133,7 @@ describe('entryIssueDetails.middleware.test.js', () => { it('should throw error if resources is missing', () => { const req = { - issues: [ + entryIssues: [ { entry_number: 1, line_number: 2, From e6743198329f72147f71e3678c83e36bf27f0c1c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 11:04:43 +0000 Subject: [PATCH 07/83] dashboard now gets data from all resources and also refactored code --- src/middleware/common.middleware.js | 41 ++-- src/middleware/datasetOverview.middleware.js | 14 +- src/middleware/datasetTaskList.middleware.js | 6 +- src/middleware/dataview.middleware.js | 6 +- .../entryIssueDetails.middleware.js | 5 +- src/middleware/issueTable.middleware.js | 5 +- src/middleware/middleware.builders.js | 33 ++++ src/middleware/overview.middleware.js | 186 ++++++++---------- src/routes/schemas.js | 7 +- src/services/performanceDbApi.js | 14 -- src/views/organisations/overview.html | 2 +- 11 files changed, 159 insertions(+), 160 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 6826bef54..fd6e27b5e 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -68,13 +68,6 @@ export const takeResourceIdFromParams = (req) => { req.resource = { resource: req.params.resourceId } } -export const fetchEntityCount = fetchOne({ - query: ({ req }) => performanceDbApi.entityCountQuery(req.resource.resource), - result: 'entityCount', - dataset: FetchOptions.fromParams, - fallbackPolicy: FetchOneFallbackPolicy.continue -}) - export const fetchOrgInfo = fetchOne({ query: ({ params }) => { return `SELECT name, organisation, entity, statistical_geography FROM organisation WHERE organisation = '${params.lpa}'` @@ -187,15 +180,19 @@ export const createPaginationTemplateParams = (req, res, next) => { // Resources export const fetchResources = fetchMany({ - query: ({ req }) => ` - SELECT DISTINCT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r - LEFT JOIN resource_organisation ro ON ro.resource = r.resource - LEFT JOIN resource_dataset rd ON rd.resource = r.resource - LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource - WHERE ro.organisation = '${req.params.lpa}' - AND rd.dataset = '${req.params.dataset}' - AND r.end_date = '' - ORDER BY start_date desc`, + query: ({ req }) => { + const lpaClause = req.params.lpa ? `AND ro.organisation = '${req.params.lpa}'` : '' + const datasetClause = req.params.dataset ? `AND rd.dataset = '${req.params.dataset}'` : '' + return ` + SELECT DISTINCT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rd.dataset, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r + LEFT JOIN resource_organisation ro ON ro.resource = r.resource + LEFT JOIN resource_dataset rd ON rd.resource = r.resource + LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource + WHERE r.end_date = '' + ${lpaClause} + ${datasetClause} + ORDER BY start_date desc` + }, result: 'resources' }) @@ -375,7 +372,7 @@ const fetchEntityIssuesForFieldAndType = fetchMany({ select * from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource = '${req.resources[0].resource}' + WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') ${issueTypeClause} AND it.responsibility = 'external' AND it.severity = 'error' @@ -484,24 +481,26 @@ export const fetchEntryIssues = fetchMany({ export const fetchEntityIssueCounts = fetchMany({ query: ({ req }) => ` - select field, i.issue_type, COUNT(resource+line_number) as count + select dataset, field, i.issue_type, COUNT(resource+line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') AND entity != '' - GROUP BY field, i.issue_type + AND it.responsibility = 'external' + AND it.severity = 'error' + GROUP BY field, i.issue_type, dataset `, result: 'entityIssueCounts' }) export const fetchEntryIssueCounts = fetchMany({ query: ({ req }) => ` - select field, i.issue_type, COUNT(resource+line_number) as count + select dataset, field, i.issue_type, COUNT(resource+line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' AND entity = '' - GROUP BY field, i.issue_type + GROUP BY field, i.issue_type, dataset `, result: 'entryIssueCounts' }) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 373b97853..4104f2289 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -1,7 +1,6 @@ import { fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, getDatasetTaskListError, isResourceAccessible, isResourceIdInParams, isResourceNotAccessible, logPageError, pullOutDatasetSpecification, takeResourceIdFromParams } from './common.middleware.js' -import { fetchOne, fetchIf, fetchMany, renderTemplate, FetchOptions, onlyIf } from './middleware.builders.js' +import { fetchOne, fetchIf, fetchMany, renderTemplate, FetchOptions, onlyIf, FetchOneFallbackPolicy } from './middleware.builders.js' import { fetchResourceStatus, prepareDatasetTaskListErrorTemplateParams } from './datasetTaskList.middleware.js' -import performanceDbApi from '../services/performanceDbApi.js' import { getDeadlineHistory, requiredDatasets } from '../utils/utils.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' @@ -169,10 +168,15 @@ export const setNoticesFromSourceKey = (sourceKey) => (req, res, next) => { next() } -const fetchEntityCount = fetchOne({ - query: ({ req }) => performanceDbApi.entityCountQuery(req.orgInfo.entity), +export const fetchEntityCount = fetchOne({ + query: ({ req }) => ` + select count(entity) as entity_count + from entity + WHERE organisation_entity = '${req.orgInfo.entity}' + `, result: 'entityCount', - dataset: FetchOptions.fromParams + dataset: FetchOptions.fromParams, + fallbackPolicy: FetchOneFallbackPolicy.continue }) export const prepareDatasetOverviewTemplateParams = (req, res, next) => { diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index fa736949d..82a989fe0 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -5,7 +5,8 @@ import { processEntitiesMiddlewares, fetchOrgInfo, fetchEntityIssueCounts, - fetchEntryIssueCounts + fetchEntryIssueCounts, + logPageError } from './common.middleware.js' import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -156,5 +157,6 @@ export default [ fetchEntryIssueCounts, prepareTasks, prepareDatasetTaskListTemplateParams, - getDatasetTaskList + getDatasetTaskList, + logPageError ] diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index 9cbaeaf83..75ba7a5f6 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -1,5 +1,5 @@ import config from '../../config/index.js' -import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath, getSetDataRange } from './common.middleware.js' +import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath, getSetDataRange, logPageError } from './common.middleware.js' import { fetchResourceStatus } from './datasetTaskList.middleware.js' import { fetchIf, fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -125,5 +125,7 @@ export default [ createPaginationTemplateParams, prepareTemplateParams, - getGetDataview + getGetDataview, + + logPageError ] diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index b7d44343c..1a3e448c4 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,5 +1,5 @@ import * as v from 'valibot' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchEntryIssues, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchEntryIssues, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, logPageError, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import { issueErrorMessageHtml } from '../utils/utils.js' @@ -129,5 +129,6 @@ export default [ getErrorSummaryItems, prepareEntry, prepareIssueDetailsTemplateParams, - getIssueDetails + getIssueDetails, + logPageError ] diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index 8e8a7f1c1..37c7df8e8 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -7,7 +7,7 @@ */ import config from '../../config/index.js' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, filterOutEntitiesWithoutIssues, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, filterOutEntitiesWithoutIssues, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, logPageError, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { onlyIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' import { entryIssueGroups } from '../utils/utils.js' @@ -125,5 +125,6 @@ export default [ createPaginationTemplateParams, prepareTableParams, prepareTemplateParams, - getIssueTable + getIssueTable, + logPageError ] diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 518fea9be..184e28d0d 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,6 +13,13 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' +import { dataSubjects } from '../utils/utils.js' + +const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => + dataSubject.dataSets + .filter((dataset) => dataset.available) + .map((dataset) => dataset.value) +) export const FetchOptions = { /** @@ -114,6 +121,23 @@ export async function fetchManyFn (req, res, next) { } } +export async function fetchOneFromAllDatasetsFn (req, res, next) { + try { + const query = this.query({ req, params: req.params }) + const promises = availableDatasets.map((dataset) => { + return datasette.runQuery(query, dataset) + }) + const result = await Promise.all(promises) + req[this.result] = Object.fromEntries(result.map(({ formattedData }, i) => [availableDatasets[i], formattedData[0]])) + logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) + next() + } catch (error) { + logger.debug('fetchMany: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + req.handlerName = `fetching '${this.result}'` + next(error) + } +} + /** * Middleware. Does a conditional fetch. Optionally invokes `else` if condition is false. * @@ -178,6 +202,15 @@ export function fetchMany (context) { return fetchManyFn.bind(context) } +/** + * Fetches a collection of records from all dataset databases and stores them in `req` under key specified by `result` entry. + * + * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context + */ +export function fetchOneFromAllDatasets (context) { + return fetchOneFromAllDatasetsFn.bind(context) +} + /** * Looks up schema for name in @{link templateSchema} (defaults to any()), validates and renders the template. * diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index 13ed46537..d2d0d8c90 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -1,6 +1,5 @@ -import performanceDbApi, { lpaOverviewQuery } from '../services/performanceDbApi.js' -import { fetchOrgInfo, logPageError } from './common.middleware.js' -import { fetchMany, FetchOptions, handleRejections, renderTemplate } from './middleware.builders.js' +import { fetchEntityIssueCounts, fetchEntryIssueCounts, fetchOrgInfo, fetchResources, logPageError } from './common.middleware.js' +import { fetchMany, fetchOneFromAllDatasets, renderTemplate } from './middleware.builders.js' import { dataSubjects, getDeadlineHistory, requiredDatasets } from '../utils/utils.js' import config from '../../config/index.js' import _ from 'lodash' @@ -13,29 +12,6 @@ const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => .map((dataset) => dataset.value) ) -/** - * Middleware. Updates req with 'lpaOverview' - * - * Relies on {@link config}. - * - * @param {{ params: { lpa: string }, entityCounts: { dataset: string, resource: string, entityCount?: number }[]}} req - */ -const fetchLpaOverview = fetchMany({ - query: ({ req, params }) => { - return lpaOverviewQuery(params.lpa, { datasetsFilter: Object.keys(config.datasetsConfig), entityCounts: req.entityCounts }) - }, - dataset: FetchOptions.performanceDb, - result: 'lpaOverview' -}) - -const fetchLatestResources = fetchMany({ - query: ({ params }) => { - return performanceDbApi.latestResourcesQuery(params.lpa, { datasetsFilter: Object.keys(config.datasetsConfig) }) - }, - result: 'resourceLookup', - dataset: FetchOptions.performanceDb -}) - const fetchProvisions = fetchMany({ query: ({ params }) => { const excludeDatasets = Object.keys(config.datasetsConfig).map(dataset => `'${dataset}'`).join(',') @@ -45,25 +21,13 @@ const fetchProvisions = fetchMany({ result: 'provisions' }) -/** - * Updates req with `entityCounts` (of shape `{ resource, dataset}|{ resource, dataset, entityCount }`) - * - * @param {{ resourceLookup: {resource: string, dataset: string}[] }} req - * @param {*} res - * @param {*} next - */ -const fetchEntityCounts = async (req, res, next) => { - const { resourceLookup } = req - - req.entityCounts = await performanceDbApi.getEntityCounts(resourceLookup) - next() -} - -/** - * For the purpose of displaying single status label on (possibly) many issues, - * we want issues with 'worse' status to be weighted higher. - */ -const statusOrdering = new Map(['Live', 'Needs fixing', 'Error', 'Not submitted'].map((status, i) => [status, i])) +const fetchEntityCounts = fetchOneFromAllDatasets({ + query: ({ req }) => ` + select count(entity) as entity_count + from entity + WHERE organisation_entity = '${req.orgInfo.entity}'`, + result: 'entityCounts' +}) /** * Calculates overall "health" of the datasets (not)provided by an organisation. @@ -73,7 +37,7 @@ const statusOrdering = new Map(['Live', 'Needs fixing', 'Error', 'Not submitted' * @returns */ const orgStatsReducer = (accumulator, dataset) => { - if (dataset.endpoint) accumulator[0]++ + if (dataset.endpointCount > 0) accumulator[0]++ if (dataset.status === 'Needs fixing') accumulator[1]++ if (dataset.status === 'Error') accumulator[2]++ return accumulator @@ -91,11 +55,14 @@ const orgStatsReducer = (accumulator, dataset) => { * and sets flags for due and overdue notices accordingly. */ export const datasetSubmissionDeadlineCheck = (req, res, next) => { - const { resourceLookup } = req + const { resources } = req const currentDate = new Date() req.noticeFlags = requiredDatasets.map(dataset => { - let resource = resourceLookup.find(resource => resource.dataset === dataset.dataset) + let resource + if (resources[dataset]) { + resource = resources[dataset][0] + } let datasetSuppliedForCurrentYear = false let datasetSuppliedForLastYear = false @@ -189,54 +156,64 @@ export const addNoticesToDatasets = (req, res, next) => { next() } -/** - * The overview data can contain multiple rows per dataset, - * and we want a collection of one item per dataset, - * because that's how we display it on the page. - * - * @param {object[]} lpaOverview - * @returns {object[]} - */ -export function aggregateOverviewData (req, res, next) { - const { lpaOverview } = req - if (!Array.isArray(lpaOverview)) { - throw new Error('lpaOverview should be an array') - } - const grouped = _.groupBy(lpaOverview, 'dataset') - const datasets = [] - for (const [dataset, rows] of Object.entries(grouped)) { - let numIssues = 0 - for (const row of rows) { - if (row.status !== 'Needs fixing') { - continue - } - if (row.issue_count) { - const numFields = (row.fields ?? '').split(',').length - if (row.issue_count >= row.entity_count) numIssues += numFields - else numIssues += row.issue_count - } +export function groupResourcesByDataset (req, res, next) { + const { resources } = req + + req.resources = resources.reduce((acc, current) => { + if (!acc[current.dataset]) { + acc[current.dataset] = [] } - const info = { - dataset, - issue_count: numIssues, - endpoint: rows[0].endpoint, - error: rows[0].error, - status: _.maxBy(rows, row => statusOrdering.get(row.status)).status + acc[current.dataset].push(current) + return acc + }, {}) + + next() +} + +export function groupIssuesCountsByDataset (req, res, next) { + const { entityIssueCounts, entryIssuesCounts } = req + + // merge arrays and handle undefined + const issueCounts = [...(entityIssueCounts || []), ...(entryIssuesCounts || [])] + req.issues = issueCounts.reduce((acc, current) => { + if (!acc[current.dataset]) { + acc[current.dataset] = [] + } + acc[current.dataset].push(current) + return acc + }, {}) + + next() +} + +export function prepareDatasetObjects (req, res, next) { + const { resources, issues } = req + + req.datasets = availableDatasets.map((dataset) => { + const datasetResources = resources[dataset] + const datasetIssues = issues[dataset] + + if (!datasetResources) { + return { status: 'Not submitted', endpointCount: 0, dataset } } - datasets.push(info) - } - requiredDatasets.forEach(requiredDataset => { - const hasDataset = datasets.findIndex(dataset => dataset.dataset === requiredDataset.dataset) >= 0 - if (!hasDataset) { - datasets.push({ - dataset: requiredDataset.dataset, - status: 'Not submitted' - }) + const endpointCount = datasetResources.length + const httpStatus = datasetResources.find(resource => resource.status !== '200')?.latest_status + const error = httpStatus ? `There was a ${httpStatus} error accessing the data URL` : undefined + const issueCount = datasetIssues?.length || 0 + + let status + if (error) { + status = 'Error' + } else if (issueCount > 0) { + status = 'Needs fixing' + } else { + status = 'Live' } + + return { dataset, error, issueCount, status, endpointCount } }) - req.datasets = datasets next() } @@ -249,19 +226,6 @@ export function aggregateOverviewData (req, res, next) { export function prepareOverviewTemplateParams (req, res, next) { const { orgInfo: organisation, provisions, datasets } = req // add in any of the missing key 8 datasets - const keys = new Set(datasets.map(d => d.dataset)) - availableDatasets.forEach((dataset) => { - if (!keys.has(dataset)) { - const row = { - dataset, - endpoint: null, - status: 'Not submitted', - issue_count: 0, - entity_count: undefined - } - datasets.push(row) - } - }) const totalDatasets = datasets.length const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = @@ -313,10 +277,18 @@ export const getOverview = renderTemplate({ export default [ fetchOrgInfo, - fetchLatestResources, - handleRejections(fetchEntityCounts), - fetchLpaOverview, - aggregateOverviewData, + fetchResources, + fetchEntityIssueCounts, + fetchEntryIssueCounts, + fetchEntityCounts, + groupResourcesByDataset, + groupIssuesCountsByDataset, + + prepareDatasetObjects, + // prepareOverviewData, + + // fetchLpaOverview, + // aggregateOverviewData, datasetSubmissionDeadlineCheck, addNoticesToDatasets, fetchProvisions, diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 774479939..8c761e97d 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -106,14 +106,13 @@ export const DeadlineNoticeField = v.strictObject({ const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()) }) const DatasetNameField = v.strictObject({ name: NonEmptyString, dataset: NonEmptyString, collection: NonEmptyString }) const DatasetItem = v.strictObject({ - endpoint: v.optional(v.url()), + endpointCount: v.optional(v.number()), status: v.enum(datasetStatusEnum), dataset: NonEmptyString, - issue_count: v.optional(v.number()), + issueCount: v.optional(v.number()), error: v.optional(v.nullable(NonEmptyString)), - http_error: v.optional(NonEmptyString), issue: v.optional(NonEmptyString), - entity_count: v.optional(v.number()), + entityCount: v.optional(v.number()), project: v.optional(v.string()), // synthetic entry, represents a user friendly count (e.g. count missing value in a column as 1 issue) numIssues: v.optional(v.number()), diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index d7088d7ac..d0fbb9581 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -363,19 +363,5 @@ export default { const result = await datasette.runQuery(sql, dataset) return result.formattedData - }, - - /** - * Query for the entity count for a given organisation and dataset. - * - * @param orgEntity - * @returns {string} - */ - entityCountQuery (orgEntity) { - return /* sql */ ` - select count(entity) as entity_count - from entity - WHERE organisation_entity = '${orgEntity}' - ` } } diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 11e18ed22..9fc5ab406 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -33,7 +33,7 @@

{% elif dataset.status == 'Error' %}

{{dataset.error}}

{% elif dataset.status == 'Needs fixing' %} -

There {{ "is" | pluralise(dataset.issue_count) }} {{ dataset.issue_count }} {{ "issue" | pluralise(dataset.issue_count) }} in this dataset

+

There {{ "is" | pluralise(dataset.issueCount) }} {{ dataset.issueCount }} {{ "issue" | pluralise(dataset.issueCount) }} in this dataset

{% else %}

Data URL submitted

{% endif %} From 7e7c2cfc47193f172de5981a6d4d1fa27fa0451a Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 11:37:47 +0000 Subject: [PATCH 08/83] add js doc --- src/middleware/datasetTaskList.middleware.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index fa736949d..f39426ba0 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -35,6 +35,18 @@ function getStatusTag (status) { } } +/** + * Prepares the task list for the dataset task list page + * + * This function takes the request, response, and next middleware function as arguments + * and uses the parsed request parameters, entities, resources, and entry/ entity issue counts + * to generate a list of tasks based on the issues found in the dataset + * + * @param {Object} req - The request object + * @param {Object} res - The response object + * @param {Function} next - The next middleware function + * @return {undefined} + */ export const prepareTasks = (req, res, next) => { const { lpa, dataset } = req.parsedParams const { entities, resources } = req From 0dba092ef2469cd56ae6db447c202faccd37a3b5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 11:37:54 +0000 Subject: [PATCH 09/83] add tests --- .../datasetTaskList.middleware.test.js | 114 +++++++++++++++++- 1 file changed, 113 insertions(+), 1 deletion(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index 870ca4d8c..3fa74ce75 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -1,5 +1,6 @@ import { describe, it, vi, expect } from 'vitest' -import { prepareDatasetTaskListTemplateParams, prepareDatasetTaskListErrorTemplateParams } from '../../../src/middleware/datasetTaskList.middleware.js' +import { prepareDatasetTaskListTemplateParams, prepareDatasetTaskListErrorTemplateParams, prepareTasks } from '../../../src/middleware/datasetTaskList.middleware.js' +import performanceDbApi from '../../../src/services/performanceDbApi.js' vi.mock('../../../src/services/performanceDbApi.js') @@ -54,4 +55,115 @@ describe('datasetTaskList.middleware.js', () => { expect(templateParams.errorData.latest_200_date).toMatch(dataTimeRegex) }) }) + + describe('prepareTasks', () => { + it('prepares the task list with issues', async () => { + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: ['entity1', 'entity2'], + resources: [{ entry_count: 10 }], + entryIssueCounts: [{ field: 'field1', issue_type: 'issue-type1' }], + entityIssueCounts: [{ field: 'field2', issue_type: 'issue-type2' }] + } + + const res = { + status: vi.fn() + } + + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type1', + num_issues: 1, + rowCount: 2, + field: 'field1' + }) + + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type2', + num_issues: 1, + rowCount: 2, + field: 'field2' + }) + + expect(req.taskList).toEqual([ + { + title: { + text: undefined + }, + href: '/organisations/some-lpa/some-dataset/issue-type1/field1', + status: { + tag: { + classes: 'govuk-tag--yellow', + text: 'Needs fixing' + } + } + }, + { + title: { + text: undefined + }, + href: '/organisations/some-lpa/some-dataset/issue-type2/field2', + status: { + tag: { + classes: 'govuk-tag--yellow', + text: 'Needs fixing' + } + } + } + ]) + + expect(next).toHaveBeenCalledTimes(1) + }) + + it('uses resource row count for special issue types', async () => { + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: [], + resources: [{ entry_count: 10 }], + entryIssueCounts: [{ field: 'field1', issue_type: 'reference values are not unique' }], + entityIssueCounts: [] + } + + const res = { + status: vi.fn() + } + + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'reference values are not unique', + num_issues: 1, + rowCount: 10, + field: 'field1' + }) + + expect(req.taskList).toEqual([ + { + title: { + text: undefined + }, + href: '/organisations/some-lpa/some-dataset/reference values are not unique/field1', + status: { + tag: { + classes: 'govuk-tag--yellow', + text: 'Needs fixing' + } + } + } + ]) + + expect(next).toHaveBeenCalledTimes(1) + }) + }) }) From 595658d61234c1edc32ac2b05d1f08916ed2979e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 11:40:01 +0000 Subject: [PATCH 10/83] update test name --- test/unit/middleware/datasetTaskList.middleware.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index 3fa74ce75..370e8f7b2 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -5,7 +5,7 @@ import performanceDbApi from '../../../src/services/performanceDbApi.js' vi.mock('../../../src/services/performanceDbApi.js') describe('datasetTaskList.middleware.js', () => { - describe('prepareDatasetTaskListParams', () => { + describe('prepareDatasetTaskListTemplateParams', () => { it('sets the correct template params on the request object', async () => { const req = { orgInfo: { name: 'Example Organisation', organisation: 'ORG' }, From 1aba7d3de2d09a97e05aa378237e01255059e44c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 12:03:16 +0000 Subject: [PATCH 11/83] update queries to ensure we are getting the correct issues --- src/middleware/common.middleware.js | 44 ++++++++++++++++++----------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 6826bef54..6a6d7dedf 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -380,6 +380,7 @@ const fetchEntityIssuesForFieldAndType = fetchMany({ AND it.responsibility = 'external' AND it.severity = 'error' ${issueFieldClause} + AND i.dataset = '${req.params.dataset}' AND entity != '' ` // LIMIT ${req.dataRange.pageLength} OFFSET ${req.dataRange.offset} @@ -474,6 +475,7 @@ export const fetchEntryIssues = fetchMany({ ${issueTypeClause} AND it.responsibility = 'external' AND it.severity = 'error' + AND i.dataset = '${req.params.dataset}' ${issueFieldClause} AND (entity = '' OR i.issue_type in ('${entryIssueGroups.map(issue => issue.type).join("', '")}')) LIMIT ${req.dataRange.pageLength} OFFSET ${req.dataRange.offset} @@ -483,26 +485,36 @@ export const fetchEntryIssues = fetchMany({ }) export const fetchEntityIssueCounts = fetchMany({ - query: ({ req }) => ` - select field, i.issue_type, COUNT(resource+line_number) as count - from issue i - LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') - AND entity != '' - GROUP BY field, i.issue_type - `, + query: ({ req }) => { + const datasetClause = req.params.dataset ? `AND i.dataset = '${req.params.dataset}'` : '' + return ` + select dataset, field, i.issue_type, COUNT(resource+line_number) as count + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') + AND entity != '' + AND it.responsibility = 'external' + AND it.severity = 'error' + ${datasetClause} + GROUP BY field, i.issue_type, dataset + ` + }, result: 'entityIssueCounts' }) export const fetchEntryIssueCounts = fetchMany({ - query: ({ req }) => ` - select field, i.issue_type, COUNT(resource+line_number) as count - from issue i - LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource = '${req.resources[0].resource}' - AND entity = '' - GROUP BY field, i.issue_type - `, + query: ({ req }) => { + const datasetClause = req.params.dataset ? `AND i.dataset = '${req.params.dataset}'` : '' + return ` + select dataset, field, i.issue_type, COUNT(resource+line_number) as count + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type + WHERE resource = '${req.resources[0].resource}' + AND entity = '' + ${datasetClause} + GROUP BY field, i.issue_type, dataset + ` + }, result: 'entryIssueCounts' }) From 19190c79fe919aaeeb51287ac68e2d2c15bde1f2 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 14:11:50 +0000 Subject: [PATCH 12/83] get entity counts for resources --- src/middleware/common.middleware.js | 25 ++++++++- src/middleware/datasetTaskList.middleware.js | 10 +++- src/middleware/middleware.builders.js | 53 ++++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 6a6d7dedf..0aae94467 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -2,7 +2,7 @@ import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { entryIssueGroups } from '../utils/utils.js' import performanceDbApi from '../services/performanceDbApi.js' -import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTemplate } from './middleware.builders.js' +import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTemplate, fetchManyFromAllDatasets } from './middleware.builders.js' import * as v from 'valibot' import { pagination } from '../utils/pagination.js' import datasette from '../services/datasette.js' @@ -199,6 +199,29 @@ export const fetchResources = fetchMany({ result: 'resources' }) +export const fetchDatasetResources = fetchManyFromAllDatasets({ + query: ({ req }) => ` + SELECT * FROM dataset_resource WHERE end_date = '' + `, + dataset: FetchOptions.performanceDb, + result: 'datasetResources' +}) + +export const addLineCountsToResources = (req, res, next) => { + const { resources, datasetResources } = req + + req.resources = resources.map(resource => { + const thisDatasetResource = datasetResources[resource.dataset] + const datasetResource = thisDatasetResource.find( + _datasetResource => + _datasetResource.resource === resource.resource + ) + return { ...resource, entry_count: datasetResource.entry_count } + }) + + next() +} + // Specification export const fetchSpecification = fetchOne({ diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index f39426ba0..7ae94c93e 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -5,7 +5,10 @@ import { processEntitiesMiddlewares, fetchOrgInfo, fetchEntityIssueCounts, - fetchEntryIssueCounts + fetchEntryIssueCounts, + logPageError, + fetchDatasetResources, + addLineCountsToResources } from './common.middleware.js' import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -163,10 +166,13 @@ export default [ fetchOrgInfo, fetchDatasetInfo, fetchResources, + fetchDatasetResources, + addLineCountsToResources, ...processEntitiesMiddlewares, fetchEntityIssueCounts, fetchEntryIssueCounts, prepareTasks, prepareDatasetTaskListTemplateParams, - getDatasetTaskList + getDatasetTaskList, + logPageError ] diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 518fea9be..9e9b5c814 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,6 +13,7 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' +import { availableDatasets } from '../utils/utils.js' export const FetchOptions = { /** @@ -114,6 +115,40 @@ export async function fetchManyFn (req, res, next) { } } +export async function fetchOneFromAllDatasetsFn (req, res, next) { + try { + const query = this.query({ req, params: req.params }) + const promises = availableDatasets.map((dataset) => { + return datasette.runQuery(query, dataset) + }) + const result = await Promise.all(promises) + req[this.result] = Object.fromEntries(result.map(({ formattedData }, i) => [availableDatasets[i], formattedData[0]])) + logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) + next() + } catch (error) { + logger.debug('fetchMany: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + req.handlerName = `fetching '${this.result}'` + next(error) + } +} + +export async function fetchManyFromAllDatasetsFn (req, res, next) { + try { + const query = this.query({ req, params: req.params }) + const promises = availableDatasets.map((dataset) => { + return datasette.runQuery(query, dataset) + }) + const result = await Promise.all(promises) + req[this.result] = Object.fromEntries(result.map(({ formattedData }, i) => [availableDatasets[i], formattedData])) + logger.debug({ type: types.DataFetch, message: 'fetchManyFromAllDatasets', resultKey: this.result }) + next() + } catch (error) { + logger.debug('fetchMany: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + req.handlerName = `fetching '${this.result}'` + next(error) + } +} + /** * Middleware. Does a conditional fetch. Optionally invokes `else` if condition is false. * @@ -178,6 +213,24 @@ export function fetchMany (context) { return fetchManyFn.bind(context) } +/** + * Fetches a single record from each dataset databases and stores them in `req` under key specified by `result` entry. + * + * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context + */ +export function fetchOneFromAllDatasets (context) { + return fetchOneFromAllDatasetsFn.bind(context) +} + +/** + * Fetches a collection of records from all dataset databases and stores them in `req` under key specified by `result` entry. + * + * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context + */ +export function fetchManyFromAllDatasets (context) { + return fetchManyFromAllDatasetsFn.bind(context) +} + /** * Looks up schema for name in @{link templateSchema} (defaults to any()), validates and renders the template. * From b4d46741205545f88eda04cb3514d5de6c8991d0 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 14:13:11 +0000 Subject: [PATCH 13/83] fix available datasets --- src/middleware/middleware.builders.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 9e9b5c814..e637aa260 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,7 +13,13 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' -import { availableDatasets } from '../utils/utils.js' +import { dataSubjects } from '../utils/utils.js' + +const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => + dataSubject.dataSets + .filter((dataset) => dataset.available) + .map((dataset) => dataset.value) +) export const FetchOptions = { /** From 0ffd7844b0a669290bfe228206fc2788c732e5a7 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 15:38:07 +0000 Subject: [PATCH 14/83] get entity_count on resource --- src/middleware/common.middleware.js | 39 ++++++++++---------- src/middleware/datasetTaskList.middleware.js | 6 +-- src/services/datasette.js | 1 + 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 0aae94467..f5e958a28 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -2,7 +2,7 @@ import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { entryIssueGroups } from '../utils/utils.js' import performanceDbApi from '../services/performanceDbApi.js' -import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTemplate, fetchManyFromAllDatasets } from './middleware.builders.js' +import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' import { pagination } from '../utils/pagination.js' import datasette from '../services/datasette.js' @@ -199,27 +199,28 @@ export const fetchResources = fetchMany({ result: 'resources' }) -export const fetchDatasetResources = fetchManyFromAllDatasets({ - query: ({ req }) => ` - SELECT * FROM dataset_resource WHERE end_date = '' - `, - dataset: FetchOptions.performanceDb, - result: 'datasetResources' -}) - -export const addLineCountsToResources = (req, res, next) => { - const { resources, datasetResources } = req +export const addEntityCountsToResources = async (req, res, next) => { + const { resources } = req - req.resources = resources.map(resource => { - const thisDatasetResource = datasetResources[resource.dataset] - const datasetResource = thisDatasetResource.find( - _datasetResource => - _datasetResource.resource === resource.resource - ) - return { ...resource, entry_count: datasetResource.entry_count } + const promises = resources.map(resource => { + const query = `SELECT entry_count FROM dataset_resource WHERE resource = '${resource.resource}'` + return datasette.runQuery(query, resource.dataset) }) - next() + try { + const datasetResources = await Promise.all(promises).catch(error => { + console.log(error) + }) + + req.resources = resources.map((resource, i) => { + return { ...resource, entry_count: datasetResources[i]?.formattedData[0]?.entry_count } + }) + + next() + } catch (error) { + // Any other error handling code goes here + console.log(error) + } } // Specification diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 7ae94c93e..19df14071 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -7,8 +7,7 @@ import { fetchEntityIssueCounts, fetchEntryIssueCounts, logPageError, - fetchDatasetResources, - addLineCountsToResources + addEntityCountsToResources } from './common.middleware.js' import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -166,8 +165,7 @@ export default [ fetchOrgInfo, fetchDatasetInfo, fetchResources, - fetchDatasetResources, - addLineCountsToResources, + addEntityCountsToResources, ...processEntitiesMiddlewares, fetchEntityIssueCounts, fetchEntryIssueCounts, diff --git a/src/services/datasette.js b/src/services/datasette.js index b931c05c4..f90a9639a 100644 --- a/src/services/datasette.js +++ b/src/services/datasette.js @@ -17,6 +17,7 @@ export default { runQuery: async (query, database = 'digital-land') => { const encodedQuery = encodeURIComponent(query) const url = `${datasetteUrl}/${database}.json?sql=${encodedQuery}` + try { const response = await axios.get(url) return { From 82eb4bf3ca03ef17d38a1178bac3115d99ad9d2c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 15:52:54 +0000 Subject: [PATCH 15/83] look for entity null too --- src/middleware/common.middleware.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index f5e958a28..3dc657b02 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -188,7 +188,7 @@ export const createPaginationTemplateParams = (req, res, next) => { export const fetchResources = fetchMany({ query: ({ req }) => ` - SELECT DISTINCT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r + SELECT DISTINCT rd.dataset, r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r LEFT JOIN resource_organisation ro ON ro.resource = r.resource LEFT JOIN resource_dataset rd ON rd.resource = r.resource LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource @@ -203,7 +203,7 @@ export const addEntityCountsToResources = async (req, res, next) => { const { resources } = req const promises = resources.map(resource => { - const query = `SELECT entry_count FROM dataset_resource WHERE resource = '${resource.resource}'` + const query = `SELECT entry_count FROM dataset_resource WHERE resource = "${resource.resource}"` return datasette.runQuery(query, resource.dataset) }) @@ -501,7 +501,7 @@ export const fetchEntryIssues = fetchMany({ AND it.severity = 'error' AND i.dataset = '${req.params.dataset}' ${issueFieldClause} - AND (entity = '' OR i.issue_type in ('${entryIssueGroups.map(issue => issue.type).join("', '")}')) + AND (entity = '' OR entity is NULL OR i.issue_type in ('${entryIssueGroups.map(issue => issue.type).join("', '")}')) LIMIT ${req.dataRange.pageLength} OFFSET ${req.dataRange.offset} ` }, @@ -534,7 +534,7 @@ export const fetchEntryIssueCounts = fetchMany({ from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' - AND entity = '' + AND (entity = '' OR entity is NULL) ${datasetClause} GROUP BY field, i.issue_type, dataset ` From 99235f0e8ffd3848e8eaa6f289146cbbaad6c01c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 16:02:08 +0000 Subject: [PATCH 16/83] dont group issues as doing it in the query --- src/middleware/datasetTaskList.middleware.js | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 19df14071..868f84d39 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -60,22 +60,7 @@ export const prepareTasks = (req, res, next) => { const issues = [...entryIssueCounts, ...entityIssueCounts] - const groupedIssues = issues.reduce((acc, issue) => { - const { field, issue_type: type } = issue - const key = `${field}_${type}` - if (acc[key]) { - acc[key].count += 1 - } else { - acc[key] = { - type, - field, - count: 1 - } - } - return acc - }, {}) - - req.taskList = Object.values(groupedIssues).map(({ field, type, count }) => { + req.taskList = Object.values(issues).map(({ field, issue_type: type, count }) => { // if the issue doesn't have an entity, or is one of the special case issue types then we should use the resource_row_count let rowCount = entityCount From 00939afbc2ca5816a4e7c8993e662254dc97ee06 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 13:40:25 +0000 Subject: [PATCH 17/83] bug fixes --- src/middleware/common.middleware.js | 18 ++++++++++++++++-- src/middleware/overview.middleware.js | 4 ++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 84f59f681..ad20f19e5 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -513,7 +513,7 @@ export const fetchEntityIssueCounts = fetchMany({ from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') - AND entity != '' + AND (entity != '' AND entity IS NOT NULL) AND it.responsibility = 'external' AND it.severity = 'error' ${datasetClause} @@ -526,12 +526,26 @@ export const fetchEntityIssueCounts = fetchMany({ export const fetchEntryIssueCounts = fetchMany({ query: ({ req }) => { const datasetClause = req.params.dataset ? `AND i.dataset = '${req.params.dataset}'` : '' + + const mostRecentResourcesForEachDataset = {} + + req.resources.forEach(resource => { + const currentRecentResource = mostRecentResourcesForEachDataset[resource.dataset] + if (!currentRecentResource || new Date(currentRecentResource.start_date) < resource.start_date) { + mostRecentResourcesForEachDataset[resource.dataset] = resource + } + }) + + const mostRecentResources = Object.values(mostRecentResourcesForEachDataset).map(resource => resource.resource) + return ` select dataset, field, i.issue_type, COUNT(resource+line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource = '${req.resources[0].resource}' + WHERE resource in ('${mostRecentResources.join("', '")}') AND (entity = '' OR entity is NULL) + AND it.responsibility = 'external' + AND it.severity = 'error' ${datasetClause} GROUP BY field, i.issue_type, dataset ` diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index d2d0d8c90..764a5e2aa 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -171,10 +171,10 @@ export function groupResourcesByDataset (req, res, next) { } export function groupIssuesCountsByDataset (req, res, next) { - const { entityIssueCounts, entryIssuesCounts } = req + const { entityIssueCounts, entryIssueCounts } = req // merge arrays and handle undefined - const issueCounts = [...(entityIssueCounts || []), ...(entryIssuesCounts || [])] + const issueCounts = [...(entityIssueCounts || []), ...(entryIssueCounts || [])] req.issues = issueCounts.reduce((acc, current) => { if (!acc[current.dataset]) { acc[current.dataset] = [] From 099f009aa2e17b5a29cc0494a8392cba0edecc73 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 13:40:25 +0000 Subject: [PATCH 18/83] bug fixes --- src/middleware/common.middleware.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 3dc657b02..ba960ae3a 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -516,7 +516,7 @@ export const fetchEntityIssueCounts = fetchMany({ from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') - AND entity != '' + AND (entity != '' AND entity IS NOT NULL) AND it.responsibility = 'external' AND it.severity = 'error' ${datasetClause} @@ -529,12 +529,26 @@ export const fetchEntityIssueCounts = fetchMany({ export const fetchEntryIssueCounts = fetchMany({ query: ({ req }) => { const datasetClause = req.params.dataset ? `AND i.dataset = '${req.params.dataset}'` : '' + + const mostRecentResourcesForEachDataset = {} + + req.resources.forEach(resource => { + const currentRecentResource = mostRecentResourcesForEachDataset[resource.dataset] + if (!currentRecentResource || new Date(currentRecentResource.start_date) < resource.start_date) { + mostRecentResourcesForEachDataset[resource.dataset] = resource + } + }) + + const mostRecentResources = Object.values(mostRecentResourcesForEachDataset).map(resource => resource.resource) + return ` select dataset, field, i.issue_type, COUNT(resource+line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource = '${req.resources[0].resource}' + WHERE resource in ('${mostRecentResources.join("', '")}') AND (entity = '' OR entity is NULL) + AND it.responsibility = 'external' + AND it.severity = 'error' ${datasetClause} GROUP BY field, i.issue_type, dataset ` From e789a72a28940617d4b3b204f005f33c4bb5b461 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 14:29:02 +0000 Subject: [PATCH 19/83] fix tests --- test/unit/middleware/datasetTaskList.middleware.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index 370e8f7b2..60347ad49 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -65,8 +65,8 @@ describe('datasetTaskList.middleware.js', () => { }, entities: ['entity1', 'entity2'], resources: [{ entry_count: 10 }], - entryIssueCounts: [{ field: 'field1', issue_type: 'issue-type1' }], - entityIssueCounts: [{ field: 'field2', issue_type: 'issue-type2' }] + entryIssueCounts: [{ field: 'field1', issue_type: 'issue-type1', count: 1 }], + entityIssueCounts: [{ field: 'field2', issue_type: 'issue-type2', count: 1 }] } const res = { @@ -129,7 +129,7 @@ describe('datasetTaskList.middleware.js', () => { }, entities: [], resources: [{ entry_count: 10 }], - entryIssueCounts: [{ field: 'field1', issue_type: 'reference values are not unique' }], + entryIssueCounts: [{ field: 'field1', issue_type: 'reference values are not unique', count: 1 }], entityIssueCounts: [] } From d74b089d6c29d2b360ba135961b7e2ba63492074 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 14:37:54 +0000 Subject: [PATCH 20/83] Update src/middleware/common.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/common.middleware.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index ba960ae3a..a325e2216 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -209,7 +209,8 @@ export const addEntityCountsToResources = async (req, res, next) => { try { const datasetResources = await Promise.all(promises).catch(error => { - console.log(error) + logger.error('Failed to fetch dataset resources', { error, type: types.App }) + throw error }) req.resources = resources.map((resource, i) => { @@ -218,8 +219,8 @@ export const addEntityCountsToResources = async (req, res, next) => { next() } catch (error) { - // Any other error handling code goes here - console.log(error) + logger.error('Error in addEntityCountsToResources', { error, type: types.App }) + next(error) } } From 33c0938218f2530ca0ccee0d5e0a352ad7b3effb Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 14:59:27 +0000 Subject: [PATCH 21/83] Update src/middleware/datasetTaskList.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/datasetTaskList.middleware.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 868f84d39..70e0cd33e 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -65,7 +65,11 @@ export const prepareTasks = (req, res, next) => { let rowCount = entityCount if (specialIssueTypeCases.includes(type)) { - rowCount = resources[0].entry_count + if (resources.length > 0) { + rowCount = resources[0].entry_count + } else { + rowCount = 0 + } } return { From 87d819ae910233c2c40a191e5f0ff56e9a096252 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 15:02:45 +0000 Subject: [PATCH 22/83] Update src/middleware/middleware.builders.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/middleware.builders.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index e637aa260..62c587eaa 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -16,7 +16,7 @@ import * as v from 'valibot' import { dataSubjects } from '../utils/utils.js' const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => - dataSubject.dataSets + (dataSubject.dataSets || []) .filter((dataset) => dataset.available) .map((dataset) => dataset.value) ) From d65bb8369d458a2dc3c5c60afe2f307573efaf38 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 15:04:01 +0000 Subject: [PATCH 23/83] Update src/middleware/middleware.builders.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/middleware.builders.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 62c587eaa..96989aa25 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -128,7 +128,14 @@ export async function fetchOneFromAllDatasetsFn (req, res, next) { return datasette.runQuery(query, dataset) }) const result = await Promise.all(promises) - req[this.result] = Object.fromEntries(result.map(({ formattedData }, i) => [availableDatasets[i], formattedData[0]])) + req[this.result] = Object.fromEntries( + result.reduce((acc, { formattedData }, i) => { + if (formattedData.length > 0) { + acc.push([availableDatasets[i], formattedData[0]]) + } + return acc + }, []) + ) logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) next() } catch (error) { From 16f21b97e0c0e4fad27a739b6615f9f51ae13cfc Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 15:21:01 +0000 Subject: [PATCH 24/83] improved fetchEntitiesIssueCount --- src/middleware/common.middleware.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index a325e2216..ea9914740 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -527,26 +527,30 @@ export const fetchEntityIssueCounts = fetchMany({ result: 'entityIssueCounts' }) +export const getMostRecentResources = (resources) => { + const mostRecentResourcesMap = {} + resources.forEach(resource => { + const currentRecent = mostRecentResourcesMap[resource.dataset] + if (!currentRecent || new Date(currentRecent.start_date) < resource.start_date) { + mostRecentResourcesMap[resource.dataset] = resource + } + }) + return Object.values(mostRecentResourcesMap) +} + export const fetchEntryIssueCounts = fetchMany({ query: ({ req }) => { const datasetClause = req.params.dataset ? `AND i.dataset = '${req.params.dataset}'` : '' - const mostRecentResourcesForEachDataset = {} + const mostRecentResources = getMostRecentResources(req.resources) - req.resources.forEach(resource => { - const currentRecentResource = mostRecentResourcesForEachDataset[resource.dataset] - if (!currentRecentResource || new Date(currentRecentResource.start_date) < resource.start_date) { - mostRecentResourcesForEachDataset[resource.dataset] = resource - } - }) - - const mostRecentResources = Object.values(mostRecentResourcesForEachDataset).map(resource => resource.resource) + const resourceIds = Object.values(mostRecentResources).map(resource => resource.resource) return ` - select dataset, field, i.issue_type, COUNT(resource+line_number) as count + select dataset, field, i.issue_type, COUNT(CONCAT(resource, line_number)) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type - WHERE resource in ('${mostRecentResources.join("', '")}') + WHERE resource in ('${resourceIds.join("', '")}') AND (entity = '' OR entity is NULL) AND it.responsibility = 'external' AND it.severity = 'error' From eacadf1a5a495d8fe241d40fcb4a33f77504bd59 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 15:47:24 +0000 Subject: [PATCH 25/83] Update src/middleware/middleware.builders.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/middleware.builders.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 96989aa25..4d0cc0a7f 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -149,10 +149,16 @@ export async function fetchManyFromAllDatasetsFn (req, res, next) { try { const query = this.query({ req, params: req.params }) const promises = availableDatasets.map((dataset) => { - return datasette.runQuery(query, dataset) + return datasette.runQuery(query, dataset).catch(error => { + logger.error('Query failed for dataset', { dataset, error, type: types.DataFetch }) + return { formattedData: [] } + }) }) const result = await Promise.all(promises) - req[this.result] = Object.fromEntries(result.map(({ formattedData }, i) => [availableDatasets[i], formattedData])) + req[this.result] = Object.fromEntries( + result.filter(({ formattedData }) => formattedData.length > 0) + .map(({ formattedData }, i) => [availableDatasets[i], formattedData]) + ) logger.debug({ type: types.DataFetch, message: 'fetchManyFromAllDatasets', resultKey: this.result }) next() } catch (error) { From f6ec4623782f348490e31ce819e675fe8fb98305 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 15:48:38 +0000 Subject: [PATCH 26/83] improved error handling --- src/middleware/datasetTaskList.middleware.js | 20 +++- .../datasetTaskList.middleware.test.js | 99 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 70e0cd33e..fe8779df7 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -13,6 +13,7 @@ import { fetchOne, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' import { statusToTagClass } from '../filters/filters.js' import * as v from 'valibot' +import logger from '../utils/logger.js' /** * Fetches the resource status @@ -58,7 +59,14 @@ export const prepareTasks = (req, res, next) => { const specialIssueTypeCases = ['reference values are not unique'] - const issues = [...entryIssueCounts, ...entityIssueCounts] + let issues = [...entryIssueCounts, ...entityIssueCounts] + + issues = issues.filter( + issue => issue.issue_type !== '' && + issue.issue_type !== undefined && + issue.field !== '' && + issue.field !== undefined + ) req.taskList = Object.values(issues).map(({ field, issue_type: type, count }) => { // if the issue doesn't have an entity, or is one of the special case issue types then we should use the resource_row_count @@ -72,9 +80,17 @@ export const prepareTasks = (req, res, next) => { } } + let title + try { + title = performanceDbApi.getTaskMessage({ num_issues: count, rowCount, field, issue_type: type }) + } catch (e) { + logger.warn('datasetTaskList::prepareTasks could not get task title so setting to default', { error: e, params: { num_issues: count, rowCount, field, issue_type: type } }) + title = `${count} issue${count > 1 ? 's' : ''} of type ${type}` + } + return { title: { - text: performanceDbApi.getTaskMessage({ num_issues: count, rowCount, field, issue_type: type }) + text: title }, href: `/organisations/${lpa}/${dataset}/${type}/${field}`, status: getStatusTag('Needs fixing') diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index 60347ad49..c1340f839 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -165,5 +165,104 @@ describe('datasetTaskList.middleware.js', () => { expect(next).toHaveBeenCalledTimes(1) }) + + it('handles empty issue counts gracefully', async () => { + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: ['entity1'], + resources: [{ entry_count: 10 }], + entryIssueCounts: [], + entityIssueCounts: [] + } + + const res = { status: vi.fn() } + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(req.taskList).toEqual([]) // No tasks should be created + + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles errors from performanceDbApi gracefully', async () => { + // Simulating error in performanceDbApi.getTaskMessage + vi.spyOn(performanceDbApi, 'getTaskMessage').mockImplementation(() => { + throw new Error('Error generating task message') + }) + + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: ['entity1'], + resources: [{ entry_count: 10 }], + entryIssueCounts: [{ field: 'field1', issue_type: 'issue-type1', count: 1 }], + entityIssueCounts: [] + } + + const res = { status: vi.fn() } + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(req.taskList).toEqual([ + { + title: { text: '1 issue of type issue-type1' }, // Or some default text if error is handled that way + href: '/organisations/some-lpa/some-dataset/issue-type1/field1', + status: { tag: { classes: 'govuk-tag--yellow', text: 'Needs fixing' } } + } + ]) + + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles invalid issue types', async () => { + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: ['entity1'], + resources: [{ entry_count: 10 }], + entryIssueCounts: [{ field: 'field1', issue_type: '', count: 1 }], // Invalid issue type (empty string) + entityIssueCounts: [] + } + + const res = { status: vi.fn() } + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(req.taskList).toEqual([]) + + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles missing field param in issues', async () => { + const req = { + parsedParams: { + lpa: 'some-lpa', + dataset: 'some-dataset' + }, + entities: ['entity1'], + resources: [{ entry_count: 10 }], + entryIssueCounts: [{ issue_type: 'issue-type1', count: 1 }], // Missing field + entityIssueCounts: [] + } + + const res = { status: vi.fn() } + const next = vi.fn() + + prepareTasks(req, res, next) + + expect(req.taskList).toEqual([]) // No task created due to missing field + + expect(next).toHaveBeenCalledTimes(1) + }) }) }) From 5b034c49dc9d7525c06d0d9d834ef2ca40339746 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 16:22:12 +0000 Subject: [PATCH 27/83] Update src/middleware/datasetTaskList.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/datasetTaskList.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index fe8779df7..448d623c5 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -92,7 +92,7 @@ export const prepareTasks = (req, res, next) => { title: { text: title }, - href: `/organisations/${lpa}/${dataset}/${type}/${field}`, + href: `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(type)}/${encodeURIComponent(field)}`, status: getStatusTag('Needs fixing') } }) From 5ffa55c893e04a423e304e07db8717cbf572c4f5 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Fri, 13 Dec 2024 16:22:46 +0000 Subject: [PATCH 28/83] Update src/middleware/datasetTaskList.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/datasetTaskList.middleware.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 448d623c5..962104e4a 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -57,8 +57,14 @@ export const prepareTasks = (req, res, next) => { const entityCount = entities.length - const specialIssueTypeCases = ['reference values are not unique'] +const SPECIAL_ISSUE_TYPES = ['reference values are not unique'] +export const prepareTasks = (req, res, next) => { + const { lpa, dataset } = req.parsedParams + const { entities, resources } = req + const { entryIssueCounts, entityIssueCounts } = req + + const entityCount = entities.length let issues = [...entryIssueCounts, ...entityIssueCounts] issues = issues.filter( From 8d2183bd99be084c5a6434b7a4bb8244b83d1fa0 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 16:25:28 +0000 Subject: [PATCH 29/83] fix special issue types --- src/middleware/datasetTaskList.middleware.js | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 962104e4a..89d0a103d 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -38,6 +38,8 @@ function getStatusTag (status) { } } +const SPECIAL_ISSUE_TYPES = ['reference values are not unique'] + /** * Prepares the task list for the dataset task list page * @@ -50,15 +52,6 @@ function getStatusTag (status) { * @param {Function} next - The next middleware function * @return {undefined} */ -export const prepareTasks = (req, res, next) => { - const { lpa, dataset } = req.parsedParams - const { entities, resources } = req - const { entryIssueCounts, entityIssueCounts } = req - - const entityCount = entities.length - -const SPECIAL_ISSUE_TYPES = ['reference values are not unique'] - export const prepareTasks = (req, res, next) => { const { lpa, dataset } = req.parsedParams const { entities, resources } = req @@ -78,7 +71,7 @@ export const prepareTasks = (req, res, next) => { // if the issue doesn't have an entity, or is one of the special case issue types then we should use the resource_row_count let rowCount = entityCount - if (specialIssueTypeCases.includes(type)) { + if (SPECIAL_ISSUE_TYPES.includes(type)) { if (resources.length > 0) { rowCount = resources[0].entry_count } else { From 061a38c34d8adb3d28c9fbbeef3f38d32d56c094 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 16:29:46 +0000 Subject: [PATCH 30/83] fix test --- test/unit/middleware/datasetTaskList.middleware.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index c1340f839..9413772da 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -153,7 +153,7 @@ describe('datasetTaskList.middleware.js', () => { title: { text: undefined }, - href: '/organisations/some-lpa/some-dataset/reference values are not unique/field1', + href: encodeURI('/organisations/some-lpa/some-dataset/reference values are not unique/field1'), status: { tag: { classes: 'govuk-tag--yellow', From b96547a7ab97f7cb37cf80a17a13ccf8ccece019 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 16 Dec 2024 10:11:28 +0000 Subject: [PATCH 31/83] fix errors --- src/middleware/overview.middleware.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index 39de8d8b5..d245ad7f5 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -224,7 +224,7 @@ export function prepareDatasetObjects (req, res, next) { * @param next */ export function prepareOverviewTemplateParams (req, res, next) { - const { orgInfo: organisation, provisions, datasets, datasetErrorStatus } = req + const { orgInfo: organisation, provisions, datasets, resources } = req // add in any of the missing key 8 datasets const provisionData = new Map() @@ -233,10 +233,10 @@ export function prepareOverviewTemplateParams (req, res, next) { } // we patch the datasets' project (based on provision data) and status (based on its endpoints error status) - const datasetsWithEndpointErrors = new Set(datasetErrorStatus.map(item => item.dataset)) for (const dataset of datasets) { + const datasetResources = resources[dataset.dataset] dataset.project = provisionData.get(dataset.dataset)?.project - if (dataset.status !== 'Error' && datasetsWithEndpointErrors.has(dataset.dataset)) { + if (dataset.status !== 'Error' && (datasetResources === undefined || datasetResources.findIndex(resource => resource.status !== '200') > 0)) { dataset.status = 'Error' } } From 652355c396200d08f734b8cf16669374fa3f1f51 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 16 Dec 2024 17:11:53 +0000 Subject: [PATCH 32/83] make sure overview page gets Error status's from endpoints and not resources --- src/middleware/common.middleware.js | 25 ++- src/middleware/datasetOverview.middleware.js | 141 +++++++-------- src/middleware/dataview.middleware.js | 19 +- src/middleware/overview.middleware.js | 180 +++++++++++++------ src/routes/schemas.js | 2 +- 5 files changed, 229 insertions(+), 138 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 9790247b5..9c802b6d0 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -41,9 +41,7 @@ export const fetchDatasetInfo = fetchOne({ * @param {*} req * @returns {boolean} */ -export const isResourceAccessible = (req) => req.resourceStatus.status === '200' export const isResourceIdValid = (req) => req.resourceStatus.resource.trim() !== '' -export const isResourceNotAccessible = (req) => !isResourceAccessible(req) export const isResourceIdInParams = ({ params }) => !('resourceId' in params) export const isResourceDataPresent = (req) => 'resource' in req @@ -184,14 +182,15 @@ export const fetchResources = fetchMany({ const lpaClause = req.params.lpa ? `AND ro.organisation = '${req.params.lpa}'` : '' const datasetClause = req.params.dataset ? `AND rd.dataset = '${req.params.dataset}'` : '' return ` - SELECT DISTINCT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rd.dataset, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r + SELECT DISTINCT s.documentation_url, r.start_date as resource_start_date, r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rd.dataset, rhe.endpoint_url, rhe.licence, rhe.status, rhe.latest_log_entry_date, rhe.endpoint_entry_date from resource r LEFT JOIN resource_organisation ro ON ro.resource = r.resource LEFT JOIN resource_dataset rd ON rd.resource = r.resource - LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource + LEFT JOIN reporting_historic_endpoints rhe ON r.resource = rhe.resource + LEFT JOIN source s ON s.endpoint = rhe.endpoint_url WHERE r.end_date = '' ${lpaClause} ${datasetClause} - ORDER BY start_date desc` + ORDER BY r.start_date desc` }, result: 'resources' }) @@ -544,7 +543,7 @@ export const fetchEntryIssueCounts = fetchMany({ const resourceIds = Object.values(mostRecentResources).map(resource => resource.resource) return ` - select dataset, field, i.issue_type, COUNT(CONCAT(resource, line_number)) as count + select dataset, field, i.issue_type, COUNT(resource + line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource in ('${resourceIds.join("', '")}') @@ -674,3 +673,17 @@ export const prepareIssueDetailsTemplateParams = (req, res, next) => { next() } + +export const fetchEndpointSummary = fetchMany({ + query: ({ params }) => { + const datasetClause = params.dataset ? `AND dataset = '${params.dataset}'` : '' + + return ` + SELECT * FROM endpoint_dataset_summary + WHERE organisation = '${params.lpa}' + ${datasetClause} + ` + }, + result: 'endpoints', + dataset: FetchOptions.performanceDb +}) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 4104f2289..48d7155c9 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -1,9 +1,9 @@ -import { fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, getDatasetTaskListError, isResourceAccessible, isResourceIdInParams, isResourceNotAccessible, logPageError, pullOutDatasetSpecification, takeResourceIdFromParams } from './common.middleware.js' -import { fetchOne, fetchIf, fetchMany, renderTemplate, FetchOptions, onlyIf, FetchOneFallbackPolicy } from './middleware.builders.js' -import { fetchResourceStatus, prepareDatasetTaskListErrorTemplateParams } from './datasetTaskList.middleware.js' +import { fetchDatasetInfo, fetchEntityIssueCounts, fetchEntryIssueCounts, fetchOrgInfo, fetchResources, getDatasetTaskListError, logPageError, pullOutDatasetSpecification } from './common.middleware.js' +import { fetchOne, fetchMany, renderTemplate, FetchOptions, FetchOneFallbackPolicy, onlyIf } from './middleware.builders.js' import { getDeadlineHistory, requiredDatasets } from '../utils/utils.js' import logger from '../utils/logger.js' import { types } from '../utils/logging.js' +import { prepareDatasetTaskListErrorTemplateParams } from './datasetTaskList.middleware.js' const fetchColumnSummary = fetchMany({ query: ({ params }) => ` @@ -38,64 +38,64 @@ const fetchSpecification = fetchOne({ result: 'specification' }) -const fetchSources = fetchMany({ - query: ({ params }) => ` - WITH RankedEndpoints AS ( - SELECT - rhe.endpoint, - rhe.endpoint_url, - rhe.status, - rhe.exception, - rhe.resource, - rhe.latest_log_entry_date, - rhe.endpoint_entry_date, - rhe.endpoint_end_date, - rhe.resource_start_date as resource_start_date, - rhe.resource_end_date, - s.documentation_url, - ROW_NUMBER() OVER ( - PARTITION BY rhe.endpoint_url - ORDER BY - rhe.latest_log_entry_date DESC - ) AS row_num - FROM - reporting_historic_endpoints rhe - LEFT JOIN source s ON rhe.endpoint = s.endpoint - WHERE - REPLACE(rhe.organisation, '-eng', '') = '${params.lpa}' - AND rhe.pipeline = '${params.dataset}' - AND ( - rhe.resource_end_date >= current_timestamp - OR rhe.resource_end_date IS NULL - OR rhe.resource_end_date = '' - ) - AND ( - rhe.endpoint_end_date >= current_timestamp - OR rhe.endpoint_end_date IS NULL - OR rhe.endpoint_end_date = '' - ) - ) - SELECT - endpoint, - endpoint_url, - status, - exception, - resource, - latest_log_entry_date, - endpoint_entry_date, - endpoint_end_date, - resource_start_date, - resource_end_date, - documentation_url - FROM - RankedEndpoints - WHERE - row_num = 1 - ORDER BY - latest_log_entry_date DESC; - `, - result: 'sources' -}) +// const fetchSources = fetchMany({ +// query: ({ params }) => ` +// WITH RankedEndpoints AS ( +// SELECT +// rhe.endpoint, +// rhe.endpoint_url, +// rhe.status, +// rhe.exception, +// rhe.resource, +// rhe.latest_log_entry_date, +// rhe.endpoint_entry_date, +// rhe.endpoint_end_date, +// rhe.resource_start_date as resource_start_date, +// rhe.resource_end_date, +// s.documentation_url, +// ROW_NUMBER() OVER ( +// PARTITION BY rhe.endpoint_url +// ORDER BY +// rhe.latest_log_entry_date DESC +// ) AS row_num +// FROM +// reporting_historic_endpoints rhe +// LEFT JOIN source s ON rhe.endpoint = s.endpoint +// WHERE +// REPLACE(rhe.organisation, '-eng', '') = '${params.lpa}' +// AND rhe.pipeline = '${params.dataset}' +// AND ( +// rhe.resource_end_date >= current_timestamp +// OR rhe.resource_end_date IS NULL +// OR rhe.resource_end_date = '' +// ) +// AND ( +// rhe.endpoint_end_date >= current_timestamp +// OR rhe.endpoint_end_date IS NULL +// OR rhe.endpoint_end_date = '' +// ) +// ) +// SELECT +// endpoint, +// endpoint_url, +// status, +// exception, +// resource, +// latest_log_entry_date, +// endpoint_entry_date, +// endpoint_end_date, +// resource_start_date, +// resource_end_date, +// documentation_url +// FROM +// RankedEndpoints +// WHERE +// row_num = 1 +// ORDER BY +// latest_log_entry_date DESC; +// `, +// result: 'sources' +// }) /** * Sets notices from a source key in the request object. @@ -180,7 +180,7 @@ export const fetchEntityCount = fetchOne({ }) export const prepareDatasetOverviewTemplateParams = (req, res, next) => { - const { orgInfo, datasetSpecification, columnSummary, entityCount, sources, dataset, issues, notice } = req + const { orgInfo, datasetSpecification, columnSummary, entityCount, resources, dataset, entryIssueCounts, entityIssueCounts, notice } = req const mappingFields = columnSummary[0]?.mapping_field?.split(';') ?? [] const nonMappingFields = columnSummary[0]?.non_mapping_field?.split(';') ?? [] @@ -197,7 +197,7 @@ export const prepareDatasetOverviewTemplateParams = (req, res, next) => { const numberOfExpectedFields = specFields.length // I'm pretty sure every endpoint has a separate documentation-url, but this isn't currently represented in the performance db. need to double check this and update if so - const endpoints = sources.sort((a, b) => { + const endpoints = resources.sort((a, b) => { if (a.status >= 200 && a.status < 300) return -1 if (b.status >= 200 && b.status < 300) return 1 return 0 @@ -224,7 +224,7 @@ export const prepareDatasetOverviewTemplateParams = (req, res, next) => { req.templateParams = { organisation: orgInfo, dataset, - taskCount: issues.length ?? 0, + taskCount: entryIssueCounts.length + entityIssueCounts.length, stats: { numberOfFieldsSupplied: numberOfFieldsSupplied ?? 0, numberOfFieldsMatched: numberOfFieldsMatched ?? 0, @@ -246,18 +246,19 @@ const getDatasetOverview = renderTemplate( } ) +const noResourceAccessible = (req, res, next) => req.resources.length === 0 + export default [ fetchOrgInfo, fetchDatasetInfo, fetchColumnSummary, - fetchResourceStatus, - fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), - fetchIf(isResourceAccessible, fetchLpaDatasetIssues), - onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), - onlyIf(isResourceNotAccessible, getDatasetTaskListError), + fetchResources, + fetchEntityIssueCounts, + fetchEntryIssueCounts, + onlyIf(noResourceAccessible, prepareDatasetTaskListErrorTemplateParams), + onlyIf(noResourceAccessible, getDatasetTaskListError), fetchSpecification, pullOutDatasetSpecification, - fetchSources, setNoticesFromSourceKey('resource'), fetchEntityCount, prepareDatasetOverviewTemplateParams, diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index 75ba7a5f6..179b436bc 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -1,7 +1,6 @@ import config from '../../config/index.js' -import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath, getSetDataRange, logPageError } from './common.middleware.js' -import { fetchResourceStatus } from './datasetTaskList.middleware.js' -import { fetchIf, fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' +import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchOrgInfo, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath, getSetDataRange, logPageError, fetchResources, fetchEntityIssueCounts, fetchEntryIssueCounts } from './common.middleware.js' +import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' export const dataviewQueryParams = v.object({ @@ -79,12 +78,12 @@ export const constructTableParams = (req, res, next) => { } export const prepareTemplateParams = (req, res, next) => { - const { orgInfo, dataset, tableParams, issues, pagination, dataRange } = req + const { orgInfo, dataset, tableParams, pagination, dataRange, entityIssueCounts, entryIssueCounts } = req req.templateParams = { organisation: orgInfo, dataset, - taskCount: (issues?.length) ?? 0, + taskCount: entityIssueCounts.length + entryIssueCounts.length, tableParams, pagination, dataRange @@ -105,14 +104,18 @@ export default [ getSetBaseSubPath(['data']), fetchOrgInfo, fetchDatasetInfo, - fetchResourceStatus, + + // fetch sources + fetchResources, + fetchEntityIssueCounts, + fetchEntryIssueCounts, + fetchEntitiesCount, setRecordCount, getSetDataRange(config.tablePageLength), show404IfPageNumberNotInRange, - fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), - fetchIf(isResourceAccessible, fetchLpaDatasetIssues), + // fetchTaskCount fetchEntities, extractJsonFieldFromEntities, diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index d245ad7f5..28c6fe432 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -1,5 +1,6 @@ -import { fetchEntityIssueCounts, fetchEntryIssueCounts, fetchOrgInfo, fetchResources, logPageError } from './common.middleware.js' -import { fetchMany, fetchOneFromAllDatasets, renderTemplate } from './middleware.builders.js' +import performanceDbApi from '../services/performanceDbApi.js' +import { fetchOrgInfo, logPageError, fetchEndpointSummary, fetchEntityIssueCounts, fetchEntryIssueCounts, fetchResources } from './common.middleware.js' +import { fetchMany, FetchOptions, renderTemplate, fetchOneFromAllDatasets } from './middleware.builders.js' import { dataSubjects, getDeadlineHistory, requiredDatasets } from '../utils/utils.js' import config from '../../config/index.js' import _ from 'lodash' @@ -12,6 +13,19 @@ const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => .map((dataset) => dataset.value) ) +/** + * Middleware. Updates req with 'datasetErrorStatus'. + * + * Fetches datasets which have active endpoints in error state. + */ +const fetchDatasetErrorStatus = fetchMany({ + query: ({ params }) => { + return performanceDbApi.datasetErrorStatusQuery(params.lpa, { datasetsFilter: Object.keys(config.datasetsConfig) }) + }, + result: 'datasetErrorStatus', + dataset: FetchOptions.performanceDb +}) + const fetchProvisions = fetchMany({ query: ({ params }) => { const excludeDatasets = Object.keys(config.datasetsConfig).map(dataset => `'${dataset}'`).join(',') @@ -29,6 +43,12 @@ const fetchEntityCounts = fetchOneFromAllDatasets({ result: 'entityCounts' }) +/** + * For the purpose of displaying single status label on (possibly) many issues, + * we want issues with 'worse' status to be weighted higher. + */ +const statusOrdering = new Map(['Live', 'Needs fixing', 'Error', 'Not submitted'].map((status, i) => [status, i])) + /** * Calculates overall "health" of the datasets (not)provided by an organisation. * @@ -59,10 +79,8 @@ export const datasetSubmissionDeadlineCheck = (req, res, next) => { const currentDate = new Date() req.noticeFlags = requiredDatasets.map(dataset => { - let resource - if (resources[dataset]) { - resource = resources[dataset][0] - } + const datasetResources = resources[dataset.dataset] + let resource = datasetResources?.find(resource => resource.dataset === dataset.dataset) let datasetSuppliedForCurrentYear = false let datasetSuppliedForLastYear = false @@ -100,6 +118,20 @@ export const datasetSubmissionDeadlineCheck = (req, res, next) => { next() } +export function groupResourcesByDataset (req, res, next) { + const { resources } = req + + req.resources = resources.reduce((acc, current) => { + if (!acc[current.dataset]) { + acc[current.dataset] = [] + } + acc[current.dataset].push(current) + return acc + }, {}) + + next() +} + /** * Adds notices to datasets based on notice flags * @@ -156,50 +188,58 @@ export const addNoticesToDatasets = (req, res, next) => { next() } -export function groupResourcesByDataset (req, res, next) { - const { resources } = req - - req.resources = resources.reduce((acc, current) => { - if (!acc[current.dataset]) { - acc[current.dataset] = [] +/** + * The overview data can contain multiple rows per dataset, + * and we want a collection of one item per dataset, + * because that's how we display it on the page. + * + * @param {object[]} lpaOverview + * @returns {object[]} + */ +export function aggregateOverviewData (req, res, next) { + const { lpaOverview } = req + if (!Array.isArray(lpaOverview)) { + throw new Error('lpaOverview should be an array') + } + const grouped = _.groupBy(lpaOverview, 'dataset') + const datasets = [] + for (const [dataset, rows] of Object.entries(grouped)) { + let numIssues = 0 + for (const row of rows) { + if (row.status !== 'Needs fixing') { + continue + } + if (row.issue_count) { + const numFields = (row.fields ?? '').split(',').length + if (row.issue_count >= row.entity_count) numIssues += numFields + else numIssues += row.issue_count + } } - acc[current.dataset].push(current) - return acc - }, {}) - - next() -} - -export function groupIssuesCountsByDataset (req, res, next) { - const { entityIssueCounts, entryIssueCounts } = req - - // merge arrays and handle undefined - const issueCounts = [...(entityIssueCounts || []), ...(entryIssueCounts || [])] - req.issues = issueCounts.reduce((acc, current) => { - if (!acc[current.dataset]) { - acc[current.dataset] = [] + const info = { + dataset, + issue_count: numIssues, + endpoint: rows[0].endpoint, + error: rows[0].error, + status: _.maxBy(rows, row => statusOrdering.get(row.status)).status } - acc[current.dataset].push(current) - return acc - }, {}) - - next() + datasets.push(info) + } } export function prepareDatasetObjects (req, res, next) { - const { resources, issues } = req + const { issues, endpoints } = req req.datasets = availableDatasets.map((dataset) => { - const datasetResources = resources[dataset] + const datasetEndpoints = endpoints[dataset] const datasetIssues = issues[dataset] - if (!datasetResources) { + if (!datasetEndpoints) { return { status: 'Not submitted', endpointCount: 0, dataset } } - const endpointCount = datasetResources.length - const httpStatus = datasetResources.find(resource => resource.status !== '200')?.latest_status - const error = httpStatus ? `There was a ${httpStatus} error accessing the data URL` : undefined + const endpointCount = datasetEndpoints.length + const httpStatus = datasetEndpoints.find(endpoint => endpoint.latest_status !== '200')?.latest_status + const error = httpStatus !== undefined ? `There was a ${httpStatus} error accessing the data URL` : undefined const issueCount = datasetIssues?.length || 0 let status @@ -224,26 +264,29 @@ export function prepareDatasetObjects (req, res, next) { * @param next */ export function prepareOverviewTemplateParams (req, res, next) { - const { orgInfo: organisation, provisions, datasets, resources } = req + const { orgInfo: organisation, provisions, datasets } = req // add in any of the missing key 8 datasets + const keys = new Set(datasets.map(d => d.dataset)) + availableDatasets.forEach((dataset) => { + if (!keys.has(dataset)) { + const row = { + dataset, + endpoint: null, + status: 'Not submitted', + issue_count: 0, + entity_count: undefined + } + datasets.push(row) + } + }) const provisionData = new Map() for (const provision of provisions ?? []) { provisionData.set(provision.dataset, provision) } - // we patch the datasets' project (based on provision data) and status (based on its endpoints error status) - for (const dataset of datasets) { - const datasetResources = resources[dataset.dataset] - dataset.project = provisionData.get(dataset.dataset)?.project - if (dataset.status !== 'Error' && (datasetResources === undefined || datasetResources.findIndex(resource => resource.status !== '200') > 0)) { - dataset.status = 'Error' - } - } - const totalDatasets = datasets.length - const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = - datasets.reduce(orgStatsReducer, [0, 0, 0]) + const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = datasets.reduce(orgStatsReducer, [0, 0, 0]) const datasetsByReason = _.groupBy(datasets, (ds) => { const reason = provisionData.get(ds.dataset)?.provision_reason @@ -280,20 +323,51 @@ export const getOverview = renderTemplate({ handlerName: 'getOverview' }) +export function groupIssuesCountsByDataset (req, res, next) { + const { entityIssueCounts, entryIssueCounts } = req + + // merge arrays and handle undefined + const issueCounts = [...(entityIssueCounts || []), ...(entryIssueCounts || [])] + req.issues = issueCounts.reduce((acc, current) => { + if (!acc[current.dataset]) { + acc[current.dataset] = [] + } + acc[current.dataset].push(current) + return acc + }, {}) + + next() +} + +export function groupEndpointsByDataset (req, res, next) { + const { endpoints } = req + + // merge arrays and handle undefined + req.endpoints = endpoints.reduce((acc, current) => { + if (!acc[current.dataset]) { + acc[current.dataset] = [] + } + acc[current.dataset].push(current) + return acc + }, {}) + + next() +} + export default [ fetchOrgInfo, fetchResources, + fetchDatasetErrorStatus, + fetchEndpointSummary, fetchEntityIssueCounts, fetchEntryIssueCounts, fetchEntityCounts, groupResourcesByDataset, groupIssuesCountsByDataset, + groupEndpointsByDataset, prepareDatasetObjects, - // prepareOverviewData, - // fetchLpaOverview, - // aggregateOverviewData, datasetSubmissionDeadlineCheck, addNoticesToDatasets, fetchProvisions, diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 8c761e97d..64cf04944 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -151,7 +151,7 @@ export const OrgDatasetOverview = v.strictObject({ numberOfExpectedFields: v.integer(), endpoints: v.array(v.strictObject({ name: v.string(), - documentation_url: v.optional(v.string()), + documentation_url: v.nullable(v.optional(v.string())), endpoint: v.string(), lastAccessed: v.string(), lastUpdated: v.nullable(v.string()), From 5dbb046f50f840207616de76e26d52d1174f4260 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 10:17:10 +0000 Subject: [PATCH 33/83] fix tests --- src/middleware/overview.middleware.js | 49 +---- .../datasetOverview.middleware.test.js | 5 +- .../middleware/dataview.middleware.test.js | 3 +- .../middleware/overview.middleware.test.js | 178 +++++------------- 4 files changed, 54 insertions(+), 181 deletions(-) diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index 28c6fe432..8c740a6d5 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -43,12 +43,6 @@ const fetchEntityCounts = fetchOneFromAllDatasets({ result: 'entityCounts' }) -/** - * For the purpose of displaying single status label on (possibly) many issues, - * we want issues with 'worse' status to be weighted higher. - */ -const statusOrdering = new Map(['Live', 'Needs fixing', 'Error', 'Not submitted'].map((status, i) => [status, i])) - /** * Calculates overall "health" of the datasets (not)provided by an organisation. * @@ -78,6 +72,11 @@ export const datasetSubmissionDeadlineCheck = (req, res, next) => { const { resources } = req const currentDate = new Date() + if (!resources) { + const error = new Error('datasetSubmissionDeadlineCheck requires resources') + next(error) + } + req.noticeFlags = requiredDatasets.map(dataset => { const datasetResources = resources[dataset.dataset] let resource = datasetResources?.find(resource => resource.dataset === dataset.dataset) @@ -188,44 +187,6 @@ export const addNoticesToDatasets = (req, res, next) => { next() } -/** - * The overview data can contain multiple rows per dataset, - * and we want a collection of one item per dataset, - * because that's how we display it on the page. - * - * @param {object[]} lpaOverview - * @returns {object[]} - */ -export function aggregateOverviewData (req, res, next) { - const { lpaOverview } = req - if (!Array.isArray(lpaOverview)) { - throw new Error('lpaOverview should be an array') - } - const grouped = _.groupBy(lpaOverview, 'dataset') - const datasets = [] - for (const [dataset, rows] of Object.entries(grouped)) { - let numIssues = 0 - for (const row of rows) { - if (row.status !== 'Needs fixing') { - continue - } - if (row.issue_count) { - const numFields = (row.fields ?? '').split(',').length - if (row.issue_count >= row.entity_count) numIssues += numFields - else numIssues += row.issue_count - } - } - const info = { - dataset, - issue_count: numIssues, - endpoint: rows[0].endpoint, - error: rows[0].error, - status: _.maxBy(rows, row => statusOrdering.get(row.status)).status - } - datasets.push(info) - } -} - export function prepareDatasetObjects (req, res, next) { const { issues, endpoints } = req diff --git a/test/unit/middleware/datasetOverview.middleware.test.js b/test/unit/middleware/datasetOverview.middleware.test.js index 50c57d4d9..abd429063 100644 --- a/test/unit/middleware/datasetOverview.middleware.test.js +++ b/test/unit/middleware/datasetOverview.middleware.test.js @@ -25,11 +25,11 @@ describe('Dataset Overview Middleware', () => { datasetSpecification: { fields: [{ field: 'field1' }, { field: 'field2' }] }, columnSummary: [{ mapping_field: 'field1', non_mapping_field: 'field3' }], entityCount: { entity_count: 10 }, - sources: [ + resources: [ { endpoint_url: 'endpoint1', documentation_url: 'doc-url1', status: '200', endpoint_entry_date: 'LU1', latest_log_entry_date: 'LA1', resource_start_date: '2023-01-01' }, { endpoint_url: 'endpoint2', documentation_url: 'doc-url2', status: '404', exception: 'exception', endpoint_entry_date: 'LU2', latest_log_entry_date: 'LA2', resource_start_date: '2023-01-02' } ], - issues: [ + entryIssueCounts: [ { issue: 'Example issue 1', issue_type: 'Example issue type 1', @@ -38,6 +38,7 @@ describe('Dataset Overview Middleware', () => { status: 'Error' } ], + entityIssueCounts: [], notice: undefined } prepareDatasetOverviewTemplateParams(reqWithResults, res, () => {}) diff --git a/test/unit/middleware/dataview.middleware.test.js b/test/unit/middleware/dataview.middleware.test.js index 26c52820e..31702e389 100644 --- a/test/unit/middleware/dataview.middleware.test.js +++ b/test/unit/middleware/dataview.middleware.test.js @@ -154,7 +154,8 @@ describe('dataview.middleware.test.js', () => { orgInfo: { name: 'Mock Org' }, dataset: { name: 'Mock Dataset' }, tableParams: { columns: ['foo'], fields: ['foo'] }, - issues: [], + entityIssueCounts: [], + entryIssueCounts: [], pagination: {}, entityCount: { count: 1 }, offset: 0, diff --git a/test/unit/middleware/overview.middleware.test.js b/test/unit/middleware/overview.middleware.test.js index 2bcf178a0..9e4f0b12b 100644 --- a/test/unit/middleware/overview.middleware.test.js +++ b/test/unit/middleware/overview.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, vi, expect, beforeEach, afterEach } from 'vitest' -import { addNoticesToDatasets, aggregateOverviewData, datasetSubmissionDeadlineCheck, getOverview, prepareOverviewTemplateParams } from '../../../src/middleware/overview.middleware' +import { addNoticesToDatasets, datasetSubmissionDeadlineCheck, getOverview, prepareOverviewTemplateParams } from '../../../src/middleware/overview.middleware' import { setupNunjucks } from '../../../src/serverSetup/nunjucks.js' import jsdom from 'jsdom' @@ -26,29 +26,29 @@ const reqTemplate = { datasets: [ { dataset: 'dataset1', - issue_count: 0, - endpoint: 'https://example.com', + issueCount: 0, + endpointCount: 1, error: undefined, status: 'Live' }, { dataset: 'dataset2', - issue_count: 0, - endpoint: null, + issueCount: 0, + endpointCount: 0, error: undefined, status: 'Needs fixing' }, { dataset: 'dataset3', - issue_count: 0, - endpoint: 'https://example.com', + issueCount: 0, + endpointCount: 1, error: undefined, status: 'Error' }, { dataset: 'dataset4', - issue_count: 0, - endpoint: null, + issueCount: 0, + endpointCount: 0, error: 'There was a 404 error', status: 'Error' } @@ -82,12 +82,12 @@ describe('overview.middleware', () => { organisation: { name: 'Example LPA', organisation: 'LPA' }, datasets: { statutory: expect.arrayContaining([ - { endpoint: 'https://example.com', status: 'Live', dataset: 'dataset1', error: undefined, issue_count: 0, project: 'open-digital-planning' }, - { endpoint: 'https://example.com', status: 'Error', dataset: 'dataset3', error: undefined, issue_count: 0, project: 'open-digital-planning' } + { endpointCount: 1, status: 'Live', dataset: 'dataset1', error: undefined, issueCount: 0 }, + { endpointCount: 1, status: 'Error', dataset: 'dataset3', error: undefined, issueCount: 0 } ]), other: expect.arrayContaining([ - { endpoint: null, status: 'Needs fixing', dataset: 'dataset2', error: undefined, issue_count: 0, project: 'open-digital-planning' }, - { endpoint: null, status: 'Error', dataset: 'dataset4', error: 'There was a 404 error', issue_count: 0, project: 'open-digital-planning' } + { endpointCount: 0, status: 'Needs fixing', dataset: 'dataset2', error: undefined, issueCount: 0 }, + { endpointCount: 0, status: 'Error', dataset: 'dataset4', error: 'There was a 404 error', issueCount: 0 } ]) }, totalDatasets: 4, @@ -102,101 +102,6 @@ describe('overview.middleware', () => { expect(errorCardNodes[0].querySelector('.govuk-task-list__hint').textContent.trim()).toBe('There was an error accessing the data URL') expect(errorCardNodes[1].querySelector('.govuk-task-list__hint').textContent.trim()).toBe('There was a 404 error') }) - - it('should patch dataset status based on the provision_summary info', () => { - const req = structuredClone(reqTemplate) - console.assert(req.datasets[0].status === 'Live') - req.datasetErrorStatus = [{ dataset: 'dataset1' }] - const res = { render: vi.fn() } - - prepareOverviewTemplateParams(req, res, () => {}) - - const ds1 = req.templateParams.datasets.statutory[0] - expect(ds1.status).toBe('Error') - expect(ds1.error).toBeUndefined() - - const ds4 = req.templateParams.datasets.other[1] - expect(ds4.status).toBe('Error') - expect(ds4.error).toBe(req.datasets[3].error) // Error message should be left untouched - }) - }) - - describe('aggregateOverviewData middleware', () => { - const req = { lpaOverview: [] } - const res = {} - const next = vi.fn() - - beforeEach(() => { - vi.resetAllMocks() - }) - - it('should set req.datasets to just the required datasets when input is empty', async () => { - aggregateOverviewData(req, res, next) - expect(req.datasets).toEqual([ - { status: 'Not submitted', dataset: 'brownfield-land' } - ]) - }) - - it('should count issues correctly', async () => { - const exampleData = [ - { endpoint: 'https://example.com', status: 'Live', dataset: 'dataset1', entity_count: 11 }, - { endpoint: null, status: 'Error', dataset: 'dataset2', entity_count: 12, issue_count: 12 }, - { endpoint: 'https://example.com/3', status: 'Needs fixing', dataset: 'dataset3', entity_count: 5, issue_count: 5, fields: 'foo' }, - { endpoint: 'https://example.com/3', status: 'Needs fixing', dataset: 'dataset3', entity_count: 5, issue_count: 2, fields: 'bar' } - ] - - req.lpaOverview = exampleData - - aggregateOverviewData(req, res, next) - - expect(req.datasets).toEqual([ - { endpoint: 'https://example.com', status: 'Live', dataset: 'dataset1', error: undefined, issue_count: 0 }, - { endpoint: null, status: 'Error', dataset: 'dataset2', error: undefined, issue_count: 0 }, - { endpoint: 'https://example.com/3', status: 'Needs fixing', dataset: 'dataset3', error: undefined, issue_count: 3 }, - { dataset: 'brownfield-land', status: 'Not submitted' } - ]) - }) - - it('should ensure dataset issues get to the surface', async () => { - const exampleData = [ - { endpoint: 'https://example.com', status: 'Live', dataset: 'dataset1', entity_count: 11 }, - { endpoint: null, status: 'Error', dataset: 'dataset1', entity_count: 12, issue_count: 12 }, - { endpoint: 'https://example.com/2', status: 'Live', dataset: 'dataset2', entity_count: 5, issue_count: 5, fields: 'foo' }, - { endpoint: 'https://example.com/2', status: 'Needs fixing', dataset: 'dataset2', entity_count: 5, issue_count: 2, fields: 'bar' } - ] - - req.lpaOverview = exampleData - - aggregateOverviewData(req, res, next) - - expect(req.datasets[0].status).toBe('Error') - expect(req.datasets[1].status).toBe('Needs fixing') - }) - - it('should handle multiple fields', async () => { - const exampleData = [ - { endpoint: 'https://example.com/2', status: 'Needs fixing', dataset: 'dataset1', entity_count: 5, issue_count: 5, fields: 'foo,bar' }, - { endpoint: 'https://example.com/2', status: 'Needs fixing', dataset: 'dataset2', entity_count: 5, issue_count: 2, fields: 'baz,qux' } - ] - - req.lpaOverview = exampleData - - aggregateOverviewData(req, res, next) - - expect(req.datasets[0].status).toBe('Needs fixing') - expect(req.datasets[0].issue_count).toBe(2) // 2 columns affected - expect(req.datasets[1].status).toBe('Needs fixing') - expect(req.datasets[1].issue_count).toBe(2) // 2 rows affected (in the same two fields) - }) - - it('should\'t add a required dataset if it is already present', async () => { - const exampleData = [ - { endpoint: 'https://example.com/2', status: 'Needs fixing', dataset: 'brownfield-land', entity_count: 5, issue_count: 5, fields: 'foo,bar' } - ] - req.lpaOverview = exampleData - aggregateOverviewData(req, res, next) - expect(req.datasets.length).toEqual(1) - }) }) describe('getOverview', () => { @@ -209,12 +114,12 @@ describe('overview.middleware', () => { organisation: { name: 'Example LPA', organisation: 'LPA' }, datasets: { statutory: [ - { endpoint: 'https://example.com', status: 'Live', dataset: 'statutory1' } + { endpointCount: 1, status: 'Live', dataset: 'statutory1' } ], other: [ - { endpoint: 'https://example.com', status: 'Live', dataset: 'dataset1' }, - { endpoint: null, status: 'Needs fixing', dataset: 'dataset2' }, - { endpoint: 'https://example.com', status: 'Error', dataset: 'dataset3' } + { endpointCount: 1, status: 'Live', dataset: 'dataset1' }, + { endpointCount: 1, status: 'Needs fixing', dataset: 'dataset2' }, + { endpointCount: 1, status: 'Error', dataset: 'dataset3' } ] }, totalDatasets: 3, @@ -232,7 +137,7 @@ describe('overview.middleware', () => { describe('datasetSubmissionDeadlineCheck', () => { const req = { - resourceLookup: [] + resources: [] } const res = {} const next = vi.fn() @@ -246,12 +151,14 @@ describe('overview.middleware', () => { }) it('sets dueNotice flag if they are in the notice period and they haven\'t submitted this year', async () => { - req.resourceLookup = [ - { - dataset: 'brownfield-land', - startDate: '1995-03-17T10:00:00.000z' - } - ] + req.resources = { + 'brownfield-land': [ + { + dataset: 'brownfield-land', + startDate: '1995-03-17T10:00:00.000z' + } + ] + } vi.setSystemTime(new Date('1996-03-03T00:00:00.000Z')) @@ -262,13 +169,14 @@ describe('overview.middleware', () => { }) it('sets overdueNotice flag if they haven\'t submitted for last year', async () => { - req.resourceLookup = [ - { - dataset: 'brownfield-land', - startDate: '1994-03-17T10:00:00.000z' - } - ] - + req.resources = { + 'brownfield-land': [ + { + dataset: 'brownfield-land', + startDate: '1994-03-17T10:00:00.000z' + } + ] + } vi.setSystemTime(new Date('1995-12-03T00:00:00.000Z')) await datasetSubmissionDeadlineCheck(req, res, next) @@ -278,12 +186,14 @@ describe('overview.middleware', () => { }) it('doesn\'t set any flag if they have submitted this year and we are in the notice period', async () => { - req.resourceLookup = [ - { - dataset: 'brownfield-land', - startDate: '1996-03-17T10:00:00.000z' - } - ] + req.resources = { + 'brownfield-land': [ + { + dataset: 'brownfield-land', + startDate: '1996-03-17T10:00:00.000z' + } + ] + } vi.setSystemTime(new Date('1996-03-03T00:00:00.000Z')) @@ -294,7 +204,7 @@ describe('overview.middleware', () => { }) it('sets dueNotice flag if they haven\'t ever submitted and we are in the notice period', async () => { - req.resourceLookup = [] + req.resources = [] vi.setSystemTime(new Date('1996-03-03T00:00:00.000Z')) @@ -305,7 +215,7 @@ describe('overview.middleware', () => { }) it('sets overdueNotice flag if they haven\'t ever submitted and we are not in the notice period', async () => { - req.resourceLookup = [] + req.resources = [] vi.setSystemTime(new Date('1995-11-03T00:00:00.000Z')) From 7de56d11ee4ec2c7ecfb404a779b8c742f308689 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 10:33:05 +0000 Subject: [PATCH 34/83] use + instead of concat in sql query --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index ea9914740..9b77bfa50 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -547,7 +547,7 @@ export const fetchEntryIssueCounts = fetchMany({ const resourceIds = Object.values(mostRecentResources).map(resource => resource.resource) return ` - select dataset, field, i.issue_type, COUNT(CONCAT(resource, line_number)) as count + select dataset, field, i.issue_type, COUNT(resource + line_number) as count from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource in ('${resourceIds.join("', '")}') From afe65f64df6dc888cd8ef25739d800575de82e35 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 10:59:20 +0000 Subject: [PATCH 35/83] fix banners --- src/middleware/datasetOverview.middleware.js | 8 +++++--- src/middleware/overview.middleware.js | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 48d7155c9..f593a6138 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -105,7 +105,9 @@ const fetchSpecification = fetchOne({ */ export const setNoticesFromSourceKey = (sourceKey) => (req, res, next) => { const { dataset } = req.params - const source = req[sourceKey] + const resources = req[sourceKey] + + const source = resources[0] const deadlineObj = requiredDatasets.find(deadline => deadline.dataset === dataset) @@ -125,7 +127,7 @@ export const setNoticesFromSourceKey = (sourceKey) => (req, res, next) => { const { deadlineDate, lastYearDeadline, twoYearsAgoDeadline } = getDeadlineHistory(deadlineObj.deadline) - const startDate = new Date(source.startDate) + const startDate = new Date(source.start_date) if (startDate.toString() === 'Invalid Date') { logger.warn('Invalid start date encountered', { @@ -259,7 +261,7 @@ export default [ onlyIf(noResourceAccessible, getDatasetTaskListError), fetchSpecification, pullOutDatasetSpecification, - setNoticesFromSourceKey('resource'), + setNoticesFromSourceKey('resources'), fetchEntityCount, prepareDatasetOverviewTemplateParams, getDatasetOverview, diff --git a/src/middleware/overview.middleware.js b/src/middleware/overview.middleware.js index 8c740a6d5..197cebb6a 100644 --- a/src/middleware/overview.middleware.js +++ b/src/middleware/overview.middleware.js @@ -92,9 +92,9 @@ export const datasetSubmissionDeadlineCheck = (req, res, next) => { } if (resource) { - const startDate = new Date(resource.startDate) + const startDate = new Date(resource.start_date) - if (!resource.startDate || Number.isNaN(startDate.getTime())) { + if (!startDate || Number.isNaN(startDate.getTime())) { logger.error(`Invalid start date for resource: ${dataset.dataset}`) return { dataset: dataset.dataset, dueNotice: false, overdueNotice: false, deadline: undefined } } From 7a153ebc5c6e33da7597bd1194d88671928151c5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 11:15:06 +0000 Subject: [PATCH 36/83] fix banner tests --- .../datasetOverview.middleware.test.js | 60 +++++++++++-------- .../middleware/overview.middleware.test.js | 6 +- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/test/unit/middleware/datasetOverview.middleware.test.js b/test/unit/middleware/datasetOverview.middleware.test.js index abd429063..913c0ad47 100644 --- a/test/unit/middleware/datasetOverview.middleware.test.js +++ b/test/unit/middleware/datasetOverview.middleware.test.js @@ -85,10 +85,12 @@ describe('Dataset Overview Middleware', () => { it('should set notice for due dataset', () => { const reqWithDataset = { ...req, - dataset: { - dataset: 'mock-dataset', - startDate: '2020-11-01' - } + dataset: [ + { + dataset: 'mock-dataset', + start_date: '2020-11-01' + } + ] } vi.setSystemTime(new Date('2022-02-01T00:00:00Z')) @@ -104,10 +106,12 @@ describe('Dataset Overview Middleware', () => { it('should set notice for overdue dataset', () => { const reqWithDataset = { ...req, - dataset: { - dataset: 'mock-dataset', - startDate: '2020-01-01' - } + dataset: [ + { + dataset: 'mock-dataset', + start_date: '2020-01-01' + } + ] } vi.setSystemTime(new Date('2021-11-01T00:00:00Z')) @@ -125,10 +129,12 @@ describe('Dataset Overview Middleware', () => { lpa: 'mock-lpa', dataset: 'unknown-dataset' }, - dataset: { - dataset: 'unknown-dataset', - startDate: '2022-01-01' - } + dataset: [ + { + dataset: 'unknown-dataset', + start_date: '2022-01-01' + } + ] } setNoticesFromSourceKey('dataset')(reqWithDataset, res, () => {}) @@ -138,10 +144,12 @@ describe('Dataset Overview Middleware', () => { it('should set notice for due dataset at notice period boundary', () => { const reqWithDataset = { ...req, - dataset: { - dataset: 'mock-dataset', - startDate: '2020-11-01' - } + dataset: [ + { + dataset: 'mock-dataset', + start_date: '2020-11-01' + } + ] } vi.setSystemTime(new Date('2022-02-01T15:30:00Z')) @@ -157,10 +165,12 @@ describe('Dataset Overview Middleware', () => { it('should set notice for due dataset with notice period 1', () => { const reqWithDataset = { ...req, - dataset: { - dataset: 'mock-dataset-2', - startDate: '2020-11-01' - } + dataset: [ + { + dataset: 'mock-dataset-2', + start_date: '2020-11-01' + } + ] } utils.requiredDatasets = [ @@ -184,10 +194,12 @@ describe('Dataset Overview Middleware', () => { it('should not set notice for due dataset with notice period 0', () => { const reqWithDataset = { ...req, - dataset: { - dataset: 'mock-dataset-3', - startDate: '2020-11-01' - } + dataset: [ + { + dataset: 'mock-dataset-3', + start_date: '2020-11-01' + } + ] } utils.requiredDatasets = [ diff --git a/test/unit/middleware/overview.middleware.test.js b/test/unit/middleware/overview.middleware.test.js index 9e4f0b12b..61f2af7d6 100644 --- a/test/unit/middleware/overview.middleware.test.js +++ b/test/unit/middleware/overview.middleware.test.js @@ -155,7 +155,7 @@ describe('overview.middleware', () => { 'brownfield-land': [ { dataset: 'brownfield-land', - startDate: '1995-03-17T10:00:00.000z' + start_date: '1995-03-17T10:00:00.000z' } ] } @@ -173,7 +173,7 @@ describe('overview.middleware', () => { 'brownfield-land': [ { dataset: 'brownfield-land', - startDate: '1994-03-17T10:00:00.000z' + start_date: '1994-03-17T10:00:00.000z' } ] } @@ -190,7 +190,7 @@ describe('overview.middleware', () => { 'brownfield-land': [ { dataset: 'brownfield-land', - startDate: '1996-03-17T10:00:00.000z' + start_date: '1996-03-17T10:00:00.000z' } ] } From f84569f8e384ea1f20c18660e5615d24ef6d38c5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 11:22:06 +0000 Subject: [PATCH 37/83] improve prepareEntry param validation --- src/middleware/entryIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index b7d44343c..b46397b25 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -61,7 +61,7 @@ export const setRecordCount = (req, res, next) => { export const prepareEntry = (req, res, next) => { const { resources, entryIssues } = req - if (!entryIssues[0] || !resources) { + if (!entryIssues || entryIssues.length === 0 || !resources || resources.length === 0) { const error = new Error('Missing required values on request object') error.status = 404 return next(error) From a28c1614d5bb81611c6bc1724f09bcd599b3850d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 11:43:37 +0000 Subject: [PATCH 38/83] add custom field mapping for geoX,geoY to point --- src/middleware/common.middleware.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 9c802b6d0..7b0b64f3e 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -460,6 +460,20 @@ export const removeIssuesThatHaveBeenFixed = async (req, res, next) => { }) } +// some field mappings aren't in our database, so we should add them here +const customFieldMappings = [ + { + field: 'GeoX,GeoY', + replacement_field: 'point' + } +] +export const addCustomFieldMappings = (req, res, next) => { + const { fieldMappings } = req + + req.fieldMappings = [...fieldMappings, ...customFieldMappings] + next() +} + export const addFieldMappingsToIssue = (req, res, next) => { const { issues, fieldMappings } = req @@ -575,6 +589,7 @@ export const processRelevantIssuesMiddlewares = [ // however this step is very time consuming, so in order to progress im commenting it out for now // removeIssuesThatHaveBeenFixed, fetchFieldMappings, + addCustomFieldMappings, addFieldMappingsToIssue ] From 0ac44c4a06e3e0d75478ef0a384b580fecde7eab Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 11:53:18 +0000 Subject: [PATCH 39/83] add additional validation to prepare entity --- src/middleware/entryIssueDetails.middleware.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index b46397b25..331718d98 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -69,6 +69,12 @@ export const prepareEntry = (req, res, next) => { const issue = entryIssues[0] + if (!issue.entry_number || !issue.issue_type || typeof issue.line_number !== 'number') { + const error = new Error('Invalid entry issue structure') + error.status = 500 + return next(error) + } + const errorMessage = issue.message || issue.issue_type req.entry = { From 98bb3753f951f9fba664d37122eeaf49891e3b87 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:12:33 +0000 Subject: [PATCH 40/83] error pages: fold error templates into one, show stack when not in prod --- config/util.js | 7 +- src/assets/scss/index.scss | 8 ++ src/filters/filters.js | 2 + src/filters/schemaIssues.js | 21 +++++ src/middleware/common.middleware.js | 8 +- .../entryIssueDetails.middleware.js | 4 +- src/middleware/middleware.builders.js | 7 +- src/routes/guidance.js | 81 ------------------- src/routes/schemas.js | 16 ++-- src/serverSetup/errorHandlers.js | 17 ++-- src/serverSetup/middlewares.js | 4 +- src/services/datasette.js | 1 + src/utils/errors.js | 26 ++++++ src/views/check/results/failedUrlRequest.html | 2 +- src/views/errorPages/400.html | 19 ----- src/views/errorPages/404.html | 19 ----- src/views/errorPages/500.html | 15 ---- src/views/errorPages/503.html | 17 ---- src/views/errorPages/504.html | 15 ---- src/views/errorPages/error.njk | 76 +++++++++++++++++ test/unit/default-http-errors.test.js | 17 +++- .../unit/middleware/common.middleware.test.js | 5 +- test/unit/publishRequestAPI.test.js | 6 +- 23 files changed, 194 insertions(+), 199 deletions(-) create mode 100644 src/filters/schemaIssues.js create mode 100644 src/utils/errors.js delete mode 100644 src/views/errorPages/400.html delete mode 100644 src/views/errorPages/404.html delete mode 100644 src/views/errorPages/500.html delete mode 100644 src/views/errorPages/503.html delete mode 100644 src/views/errorPages/504.html create mode 100644 src/views/errorPages/error.njk diff --git a/config/util.js b/config/util.js index a1463f664..bb8b57649 100644 --- a/config/util.js +++ b/config/util.js @@ -94,7 +94,12 @@ export const ConfigSchema = v.object({ measurementId: v.string() }) ), - tablePageLength: v.number() + tablePageLength: v.number(), + contact: v.object({ + issues: v.object({ + email: v.string() + }) + }) }) const readConfig = (config) => { diff --git a/src/assets/scss/index.scss b/src/assets/scss/index.scss index cb4e0840b..c9e16d1a6 100644 --- a/src/assets/scss/index.scss +++ b/src/assets/scss/index.scss @@ -214,3 +214,11 @@ code * { .app-content__markdown img { max-width: 100%; } + +.schema-issue { + display: flex; + flex-direction: column; + padding-bottom: 1em; +} + +.schema-issue p { padding: 0; margin: 0} diff --git a/src/filters/filters.js b/src/filters/filters.js index 9bd633ef9..02bd16f5a 100644 --- a/src/filters/filters.js +++ b/src/filters/filters.js @@ -8,6 +8,7 @@ import { checkToolDeepLink } from './checkToolDeepLink.js' import pluralize from 'pluralize' import { datasetSlugToReadableName } from '../utils/datasetSlugToReadableName.js' import { getDatasetGuidanceUrl } from './getDatasetConfig.js' +import { schemaIssues } from './schemaIssues.js' /** maps dataset status (as returned by `fetchLpaOverview` middleware to a * CSS class used by the govuk-tag component @@ -43,6 +44,7 @@ const addFilters = (nunjucksEnv) => { nunjucksEnv.addFilter('pluralise', pluralize) nunjucksEnv.addFilter('checkToolDeepLink', checkToolDeepLink) nunjucksEnv.addFilter('getDatasetGuidanceUrl', getDatasetGuidanceUrl) + nunjucksEnv.addFilter('schemaIssues', schemaIssues) } export default addFilters diff --git a/src/filters/schemaIssues.js b/src/filters/schemaIssues.js new file mode 100644 index 000000000..31d923d48 --- /dev/null +++ b/src/filters/schemaIssues.js @@ -0,0 +1,21 @@ +import * as v from 'valibot' + +/** + * Takes an Error and if it's a ValiError, returns an array of issue summaries (with paths). Empty array otherwise. + * + * Useful to show the schema issues in a consise way. + * + * @param {Error} error + * @returns { { path: string[], message: string}[] } + */ +export function schemaIssues (error) { + const issues = [] + if (v.isValiError(error)) { + for (const issue of error.issues) { + if (issue.path) { + issues.push({ path: issue.path.map(elem => elem.key), message: issue.message }) + } + } + } + return issues +} diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 145172c89..91cd5ba96 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -6,6 +6,7 @@ import { fetchOne, FetchOptions, FetchOneFallbackPolicy, fetchMany, renderTempla import * as v from 'valibot' import { pagination } from '../utils/pagination.js' import datasette from '../services/datasette.js' +import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' /** * Middleware. Set `req.handlerName` to a string that will identify @@ -97,7 +98,8 @@ export function validateQueryParamsFn (req, res, next) { req.parsedParams = v.parse(this.schema || v.any(), req.params) next() } catch (error) { - res.status(400).render('errorPages/400', {}) + const err = new MiddlewareError('Query params validation error', 400, { cause: error }) + res.status(400).render(err.template, { ...errorTemplateContext(), err }) } } @@ -126,9 +128,7 @@ export const show404IfPageNumberNotInRange = (req, res, next) => { } if (pageNumber > dataRange.maxPageNumber || pageNumber < 1) { - const error = new Error('page number not in range') - // @ts-ignore - error.status = 404 + const error = new MiddlewareError('page number not in range', 404) return next(error) } next() diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 8f6cf9fcd..62d4881f5 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,5 +1,6 @@ import * as v from 'valibot' import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { MiddlewareError } from '../utils/errors.js' import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import { issueErrorMessageHtml } from '../utils/utils.js' @@ -77,8 +78,7 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const error = new Error('Missing required values on request object') - error.status = 404 + const error = new MiddlewareError('Missing required values on request object', 404) return next(error) } diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 518fea9be..fe0d7639b 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,6 +13,7 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' +import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' export const FetchOptions = { /** @@ -30,7 +31,8 @@ const datasetOverride = (val, req) => { return 'digital-land' } if (val === FetchOptions.fromParams) { - console.assert(req.params.dataset, 'no "dataset" in request params') + logger.warn('no "dataset" in request params', + { types: types.App, endpoint: req.originalUrl, params: req.params }) return req.params.dataset } else if (val === FetchOptions.performanceDb) { return 'performance' @@ -40,7 +42,8 @@ const datasetOverride = (val, req) => { } const fetchOneFallbackPolicy = (req, res, next) => { - res.status(404).render('errorPages/404', {}) + const err = new MiddlewareError('Not found', 404) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err }) } /** diff --git a/src/routes/guidance.js b/src/routes/guidance.js index 5a3e79157..b0f9edeb2 100644 --- a/src/routes/guidance.js +++ b/src/routes/guidance.js @@ -1,7 +1,4 @@ import express from 'express' -/* import nunjucks from 'nunjucks' -import config from '../../config/index.js' -import logger from '../utils/logger.js' */ const router = express.Router() @@ -11,82 +8,4 @@ router.get('/*', (req, res) => { return res.redirect(302, `https://www.planning.data.gov.uk/guidance/${path}`) }) -/* function getNavigationStructure () { - if (!config.guidanceNavigation) { - logger.error('Guidance navigation configuration is missing') - return { title: 'Guidance', items: [] } - } - - return config.guidanceNavigation -} */ - -/* function extractUrlsFromItems (items) { - let urls = [] - items.forEach(item => { - if (item.url) { - urls.push(item.url) - } - if (item.items) { - urls = urls.concat(extractUrlsFromItems(item.items)) - } - }) - return urls -} - -function checkPathExists (items, path) { - const fullPath = `/guidance${path}`.replace(/\/$/, '') - const urls = extractUrlsFromItems(items) - - return urls.includes(fullPath) -} - -router.get('/*', (req, res) => { - const path = req.params[0].replace(/[^a-zA-Z0-9/-]/g, '') - return res.redirect(302, `https://www.planning.data.gov.uk/guidance${path}`) - - try { - const navigationStructure = getNavigationStructure() - const path = req.params[0].replace(/[^a-zA-Z0-9/-]/g, '') - - if (path.includes('..') || !checkPathExists(navigationStructure.items, req.path)) { - throw new Error('Invalid path') - } - - let templatePath = '' - - switch (path) { - case '': - case '/': - templatePath = 'guidance/index' - break - case 'specifications': - case 'specifications/': - templatePath = 'guidance/specifications/index' - break - default: - templatePath = `guidance/${path}` - } - - const guidancePage = nunjucks.render(`${templatePath}.md`, { - navigation: navigationStructure - }) - - res.send(guidancePage) - } catch (error) { - if (error?.message === 'template not found' || error?.message === 'Invalid path') { - logger.info('Guidance page not found', { type: 'App', path: req.path }) - - res.status(404).render('errorPages/404', {}) - } else { - logger.error('Error rendering guidance page', { - type: 'App', - path: req.path, - error: error.message - }) - - res.status(500).render('errorPages/500', {}) - } - } -}) */ - export default router diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 774479939..063d525e5 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -4,14 +4,16 @@ */ import * as v from 'valibot' +import { MiddlewareError } from '../utils/errors.js' export const EmptyParams = v.object({}) -export const UptimeParams = v.object({ - upTime: v.string() -}) -export const ErrorParams = v.strictObject({ - err: v.object({}) +export const ErrorPageParams = v.object({ + err: v.instance(MiddlewareError), + env: v.string(), + supportEmail: v.pipe(v.string(), v.email()), + uptime: v.optional(v.string()), + downtime: v.optional(v.string()) }) export const NonEmptyString = v.pipe(v.string(), v.nonEmpty()) @@ -300,9 +302,7 @@ export const templateSchema = new Map([ ['organisations/issueTable.html', OrgIssueTable], ['organisations/issueDetails.html', OrgIssueDetails], - ['errorPages/503', UptimeParams], - ['errorPages/500', ErrorParams], - ['errorPages/404', EmptyParams], + ['errorPages/error.njk', ErrorPageParams], ['privacy-notice.html', EmptyParams], ['landing.html', EmptyParams], ['cookies.html', EmptyParams], diff --git a/src/serverSetup/errorHandlers.js b/src/serverSetup/errorHandlers.js index a8a6f692e..81322a806 100644 --- a/src/serverSetup/errorHandlers.js +++ b/src/serverSetup/errorHandlers.js @@ -1,7 +1,9 @@ import logger from '../utils/logger.js' import { types } from '../utils/logging.js' +import { MiddlewareError, errorTemplateContext } from '../utils/errors.js' export function setupErrorHandlers (app) { + const errContext = errorTemplateContext() app.use((err, req, res, next) => { logger.error({ type: types.Response, @@ -17,17 +19,19 @@ export function setupErrorHandlers (app) { return next(err) } - err.template = err.template || (err.status && `errorPages/${err.status}`) || 'errorPages/500' - if (err.redirect) { return res.redirect(err.redirect) } - err.status = err.status || 500 + const middlewareError = err instanceof MiddlewareError ? err : new MiddlewareError('Internal server error', 500, { cause: err }) try { - res.status(err.status).render(err.template, { err }) + res.status(middlewareError.statusCode).render(err.template, { ...errContext, err: middlewareError }) } catch (e) { - res.status(err.status).render('errorPages/500', { err }) + logger.error('Failed to render error page.', { + type: types.Response, errorMessage: e.message, errorStack: e.stack, originalError: err.message, endpoint: req.originalUrl + }) + const renderError = new MiddlewareError('Failed to render error page', 500, { cause: e }) + res.status(500).render('errorPages/error.njk', { ...errContext, err: renderError }) } }) @@ -41,6 +45,7 @@ export function setupErrorHandlers (app) { endpoint: req.originalUrl, message: 'not found' }) - res.status(404).render('errorPages/404') + const err = new MiddlewareError('Not found', 404) + res.status(err.statusCode).render(err.template, { ...errContext, err }) }) } diff --git a/src/serverSetup/middlewares.js b/src/serverSetup/middlewares.js index bf2220b67..0ad424640 100644 --- a/src/serverSetup/middlewares.js +++ b/src/serverSetup/middlewares.js @@ -6,6 +6,7 @@ import { types } from '../utils/logging.js' import hash from '../utils/hasher.js' import config from '../../config/index.js' import { preventIndexing } from '../middleware/common.middleware.js' +import { MiddlewareError, errorTemplateContext } from '../utils/errors.js' export function setupMiddlewares (app) { app.use((req, res, next) => { @@ -34,7 +35,8 @@ export function setupMiddlewares (app) { app.use((req, res, next) => { const serviceDown = config.maintenance.serviceUnavailable || false if (serviceDown) { - res.status(503).render('errorPages/503', { upTime: config.maintenance.upTime }) + const err = new MiddlewareError('Service unavailable', 503) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err, uptime: config.maintenance.upTime }) } else { next() } diff --git a/src/services/datasette.js b/src/services/datasette.js index b931c05c4..683d3b03d 100644 --- a/src/services/datasette.js +++ b/src/services/datasette.js @@ -9,6 +9,7 @@ export default { * Executes a SQL query on the Datasette instance and returns the results. * * @param {string} query - The SQL query to execute. + * @param {string} database - The name of the database to query. Defaults to 'digital-land'. * @returns {Promise<{data: object, formattedData: object[]}>} - A promise that resolves to an object with the following properties: * - `data`: The raw data returned by Datasette. * - `formattedData`: The formatted data, with columns and rows parsed into a usable format. diff --git a/src/utils/errors.js b/src/utils/errors.js new file mode 100644 index 000000000..6d49e5254 --- /dev/null +++ b/src/utils/errors.js @@ -0,0 +1,26 @@ +import config from '../../config/index.js' + +const context = { + env: process.env.NODE_ENV || process.env.ENVIRONMENT || 'local', + supportEmail: config.contact.issues.email +} + +export function errorTemplateContext () { + return { ...context } +} + +export class MiddlewareError extends Error { + /** + * @param {string} message + * @param {number} statusCode status to be returned to the client + * @param {{template?: string, cause?: Error}} options template path + */ + constructor (message, statusCode, options = {}) { + super(message, options) + if (typeof statusCode !== 'number') { + throw new Error(`statusCode should be a number: ${statusCode}`) + } + this.statusCode = statusCode + this.template = options?.template ?? 'errorPages/error.njk' + } +} diff --git a/src/views/check/results/failedUrlRequest.html b/src/views/check/results/failedUrlRequest.html index ea93f1ec8..7e91af4a3 100644 --- a/src/views/check/results/failedUrlRequest.html +++ b/src/views/check/results/failedUrlRequest.html @@ -22,7 +22,7 @@

  • Error code: {{ error.errCode}}
  • Error Message: {{ error.errMsg }}
    • - Try again + Try again {% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/400.html b/src/views/errorPages/400.html deleted file mode 100644 index 883d3738f..000000000 --- a/src/views/errorPages/400.html +++ /dev/null @@ -1,19 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Invalid Parameters' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - -

    If you typed the web address, check it is correct.

    - -

    If you pasted the web address, check you copied the entire address.

    - -

    If the web address is correct or you selected a link or button send an email - to digitalland@communities.gov.uk.

    - -
    -
    -{% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/404.html b/src/views/errorPages/404.html deleted file mode 100644 index dc1580562..000000000 --- a/src/views/errorPages/404.html +++ /dev/null @@ -1,19 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Page not found' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - -

    If you typed the web address, check it is correct.

    - -

    If you pasted the web address, check you copied the entire address.

    - -

    If the web address is correct or you selected a link or button send an email - to digitalland@communities.gov.uk.

    - -
    -
    -{% endblock %} \ No newline at end of file diff --git a/src/views/errorPages/500.html b/src/views/errorPages/500.html deleted file mode 100644 index 2182ac7f9..000000000 --- a/src/views/errorPages/500.html +++ /dev/null @@ -1,15 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, there’s a problem with the service' %} - -{% block content %} -
    -
    -

    {{pageName}}

    -

    Try again later.

    -

    - You'll need to start from the beginning of the service. -

    -
    -
    -{% endblock %} diff --git a/src/views/errorPages/503.html b/src/views/errorPages/503.html deleted file mode 100644 index ae47938b7..000000000 --- a/src/views/errorPages/503.html +++ /dev/null @@ -1,17 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, the service is unavailable' %} - -{% block content %} -
    -
    -

    {{pageName}}

    - {% if downtime %} -

    The service will be unavailable from {{downtime}}.

    - {% endif %} - {% if upTime %} -

    You’ll be able to use the service from {{upTime}}.

    - {% endif %} -
    -
    -{% endblock %} diff --git a/src/views/errorPages/504.html b/src/views/errorPages/504.html deleted file mode 100644 index 2182ac7f9..000000000 --- a/src/views/errorPages/504.html +++ /dev/null @@ -1,15 +0,0 @@ -{% extends "layouts/main.html" %} - -{% set pageName = 'Sorry, there’s a problem with the service' %} - -{% block content %} -
    -
    -

    {{pageName}}

    -

    Try again later.

    -

    - You'll need to start from the beginning of the service. -

    -
    -
    -{% endblock %} diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk new file mode 100644 index 000000000..8fc8adc82 --- /dev/null +++ b/src/views/errorPages/error.njk @@ -0,0 +1,76 @@ +{% extends "layouts/main.html" %} + +{% set pageNames = { + '400': 'Invalid parameters', + '404': 'Page not found', + '412': 'Yo momma too fat', + '500': 'Sorry, there’s a problem with the service', + '503': 'Sorry, the service is unavailable', + '504': 'Sorry, there’s a problem with the service'} %} + +{% set pageName = pageNames[err.statusCode] or 'Sorry, there’s a problem with the service' %} + +{% block content %} +
    +
    +

    {{ pageName }}

    + + {% if err.statusCode === 400 %} +

    If you typed the web address, check it is correct.

    +

    If you pasted the web address, check you copied the entire address.

    +

    If the web address is correct or you selected a link or button send an email to {{supportEmail}}.

    + + {% elif err.statusCode === 404 %} +

    If you typed the address, check it is correct.

    +

    If you copied the web address, check you copied the entire address.

    +

    If the web address is correct or you selected a link or button, send an email to {{supportEmail}} for help.

    + +

    If you used an old address, it's possible it has expired. You can start the process from the beginning

    + + {% elif err.statusCode === 503 %} + {% if downtime %} +

    The service will be unavailable from {{downtime}}.

    + {% endif %} + {% if uptime %} +

    You’ll be able to use the service from {{uptime}}.

    + {% endif %} + + {% else %} +

    Try again later.

    +

    You'll need to start from the beginning of the service.

    + {% endif %} + +
    +
    + +
    + {% if env !== 'production' and err.statusCode !== 404 %} +
    + Error details + 
    +            {{ err.stack | safe }}
+
    + {% if err.cause %} +

    Cause:

    + 
    +            {{ err.cause.stack | safe }}
+
    + {% endif %} +
    + + {% set issues = (err.cause | schemaIssues) %} + {% if issues.length > 0 %} +
    + Schema Issues + {% for issue in issues %} +
    +

    Path: [{{ issue.path.join(', ') }}]

    +

    Message:: {{ issue.message }}

    +
    + {% endfor %} +
    + + {% endif %} + {% endif %} +
    +{% endblock %} diff --git a/test/unit/default-http-errors.test.js b/test/unit/default-http-errors.test.js index 7743bb375..a80de225a 100644 --- a/test/unit/default-http-errors.test.js +++ b/test/unit/default-http-errors.test.js @@ -1,15 +1,28 @@ import { describe, expect, it } from 'vitest' import { setupNunjucks } from '../../src/serverSetup/nunjucks.js' +import { MiddlewareError } from '../../src/utils/errors.js' const nunjucks = setupNunjucks({ datasetNameMapping: new Map() }) describe('render default HTTP error pages', () => { - const statuses = ['400', '404', '500', '503', '504'] + const statuses = [400, 404, 500, 503, 504] + const templateContext = { supportEmail: 'foo@example.com' } for (const status of statuses) { it(`status ${status}`, () => { - const page = nunjucks.render(`errorPages/${status}.html`, {}) + const err = new MiddlewareError(`Failed with ${status}`, status) + const page = nunjucks.render(err.template, { err, env: 'local', ...templateContext }) expect(page).toBeDefined() + if (status !== 404) { + expect(page).to.contain('Error details') + } }) } + + it('does not include error detaiils in production', () => { + const err = new MiddlewareError('Failed in prod', 500) + const page = nunjucks.render(err.template, { err, env: 'production', ...templateContext }) + expect(page).toBeDefined() + expect(page).not.to.contain('Error details') + }) }) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 23a146ea8..2daf74cb9 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -19,6 +19,7 @@ import { prepareIssueDetailsTemplateParams, preventIndexing } from '../../../src/middleware/common.middleware' +import { MiddlewareError } from '../../../src/utils/errors.js' import logger from '../../../src/utils/logger' import datasette from '../../../src/services/datasette.js' import performanceDbApi from '../../../src/services/performanceDbApi.js' @@ -55,7 +56,7 @@ describe('show404IfPageNumberNotInRange middleware', () => { const res = {} const next = vi.fn((err) => { expect(err instanceof Error).toBe(true) - expect(err.status).toBe(404) + expect(err.statusCode).toBe(404) expect(err.message).toBe('page number not in range') }) show404IfPageNumberNotInRange(req, res, next) @@ -69,7 +70,7 @@ describe('show404IfPageNumberNotInRange middleware', () => { const res = {} const next = vi.fn((err) => { expect(err instanceof Error).toBe(true) - expect(err.status).toBe(404) + expect(err.statusCode).toBe(404) expect(err.message).toBe('page number not in range') }) show404IfPageNumberNotInRange(req, res, next) diff --git a/test/unit/publishRequestAPI.test.js b/test/unit/publishRequestAPI.test.js index b744c8ff5..0c212970f 100644 --- a/test/unit/publishRequestAPI.test.js +++ b/test/unit/publishRequestAPI.test.js @@ -3,6 +3,7 @@ import { postFileRequest, postUrlRequest, getRequestData } from '../../src/servi import axios from 'axios' import RequestData from '../../src/models/requestData.js' import config from '../../config/index.js' +import { MiddlewareError } from '../../src/utils/errors.js' vi.mock('axios') @@ -123,10 +124,7 @@ describe('asyncRequestApi', () => { it('should throw an error if the GET request fails with status 500', async () => { const resultId = '123' - const expectedError = new Error('HTTP error! status: 500') - expectedError.message = 'HTTP error! status: 500' - expectedError.status = 500 - + const expectedError = new MiddlewareError('HTTP error! status: 500', 500) const expectedResponse = { status: 500 } axios.get.mockRejectedValue(expectedResponse) From c61a92af5d7d54c6be96f0ff8b90ba76ed9b3fef Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:14:49 +0000 Subject: [PATCH 41/83] remove unused import --- test/unit/middleware/common.middleware.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 2daf74cb9..b4d4122b9 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -19,7 +19,6 @@ import { prepareIssueDetailsTemplateParams, preventIndexing } from '../../../src/middleware/common.middleware' -import { MiddlewareError } from '../../../src/utils/errors.js' import logger from '../../../src/utils/logger' import datasette from '../../../src/services/datasette.js' import performanceDbApi from '../../../src/services/performanceDbApi.js' From 9d62ab3a2749b433d0fa6f9f938d599538347f3f Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:28:44 +0000 Subject: [PATCH 42/83] fix: use the middleware error --- src/serverSetup/errorHandlers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/serverSetup/errorHandlers.js b/src/serverSetup/errorHandlers.js index 81322a806..e9fc00a5d 100644 --- a/src/serverSetup/errorHandlers.js +++ b/src/serverSetup/errorHandlers.js @@ -25,7 +25,7 @@ export function setupErrorHandlers (app) { const middlewareError = err instanceof MiddlewareError ? err : new MiddlewareError('Internal server error', 500, { cause: err }) try { - res.status(middlewareError.statusCode).render(err.template, { ...errContext, err: middlewareError }) + res.status(middlewareError.statusCode).render(middlewareError.template, { ...errContext, err: middlewareError }) } catch (e) { logger.error('Failed to render error page.', { type: types.Response, errorMessage: e.message, errorStack: e.stack, originalError: err.message, endpoint: req.originalUrl From 3714dba45d78d76a7030fb5419b256a8311ad19c Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 12:45:04 +0000 Subject: [PATCH 43/83] update default config with contact email --- config/default.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/default.yaml b/config/default.yaml index 2224d7b98..61dcb21ea 100644 --- a/config/default.yaml +++ b/config/default.yaml @@ -42,6 +42,10 @@ email: { validations: { maxFileSize: 100000000 } +contact: + issues: + # description: should be used for general issues, e.g. if user lands on 404 doesn't know how to proceed + email: digitalland@communities.gov.uk datasetsConfig: article-4-direction: guidanceUrl: /guidance/specifications/article-4-direction From f91d6b98ad756df636b1c6747b3b974408056c1c Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:09:32 +0000 Subject: [PATCH 44/83] In case of check tool errors, prompt user to start over. --- src/views/check/results/failedFileRequest.html | 2 +- src/views/check/results/failedUrlRequest.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/views/check/results/failedFileRequest.html b/src/views/check/results/failedFileRequest.html index bd1888177..279e8aa88 100644 --- a/src/views/check/results/failedFileRequest.html +++ b/src/views/check/results/failedFileRequest.html @@ -19,7 +19,7 @@

  • the file is in the right format
  • there is no errors in the file formatting (for example, a missing comma)
    • - Try again + Try again {% endblock %} \ No newline at end of file diff --git a/src/views/check/results/failedUrlRequest.html b/src/views/check/results/failedUrlRequest.html index 7e91af4a3..68ee40470 100644 --- a/src/views/check/results/failedUrlRequest.html +++ b/src/views/check/results/failedUrlRequest.html @@ -22,7 +22,7 @@

  • Error code: {{ error.errCode}}
  • Error Message: {{ error.errMsg }}
    • - Try again + Try again {% endblock %} \ No newline at end of file From d6f7ced841b15e2df3b4dfc9df2f7bf27faef238 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:09:52 +0000 Subject: [PATCH 45/83] minor edits --- src/services/asyncRequestApi.js | 2 +- src/views/errorPages/error.njk | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/asyncRequestApi.js b/src/services/asyncRequestApi.js index e96f7ea59..5e4d938de 100644 --- a/src/services/asyncRequestApi.js +++ b/src/services/asyncRequestApi.js @@ -60,6 +60,6 @@ export const getRequestData = async (resultId) => { return new ResultData(response.data) } catch (error) { - throw new Error(`HTTP error! status: ${error.status}`) + throw new Error(`HTTP error! status: ${error.status}: ${error.message}`, { cause: error }) } } diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index 8fc8adc82..9f17ee972 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -25,7 +25,7 @@

    If you copied the web address, check you copied the entire address.

    If the web address is correct or you selected a link or button, send an email to {{supportEmail}} for help.

    -

    If you used an old address, it's possible it has expired. You can start the process from the beginning

    +

    If you used an old address, it's possible it has expired. You can start the process from the beginning.

    {% elif err.statusCode === 503 %} {% if downtime %} From 603605aee202aba652da6ceb6617abacce5f2595 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Wed, 8 Jan 2025 16:40:53 +0000 Subject: [PATCH 46/83] fix tests --- test/unit/publishRequestAPI.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/unit/publishRequestAPI.test.js b/test/unit/publishRequestAPI.test.js index 0c212970f..898663e03 100644 --- a/test/unit/publishRequestAPI.test.js +++ b/test/unit/publishRequestAPI.test.js @@ -3,7 +3,6 @@ import { postFileRequest, postUrlRequest, getRequestData } from '../../src/servi import axios from 'axios' import RequestData from '../../src/models/requestData.js' import config from '../../config/index.js' -import { MiddlewareError } from '../../src/utils/errors.js' vi.mock('axios') @@ -112,10 +111,10 @@ describe('asyncRequestApi', () => { const resultId = '123' const expectedError = new Error('HTTP error! status: 404') - expectedError.message = 'HTTP error! status: 404' + expectedError.message = 'HTTP error! status: 404: Failed to do the thing' expectedError.status = 404 - const expectedResponse = { status: 404 } + const expectedResponse = { status: 404, message: 'Failed to do the thing' } axios.get.mockRejectedValue(expectedResponse) await expect(getRequestData(resultId)).rejects.toThrow(expectedError) @@ -124,8 +123,8 @@ describe('asyncRequestApi', () => { it('should throw an error if the GET request fails with status 500', async () => { const resultId = '123' - const expectedError = new MiddlewareError('HTTP error! status: 500', 500) - const expectedResponse = { status: 500 } + const expectedError = new Error('HTTP error! status: 500: Failed to do the thing') + const expectedResponse = { status: 500, message: 'Failed to do the thing' } axios.get.mockRejectedValue(expectedResponse) await expect(getRequestData(resultId)).rejects.toThrow(expectedError) From ceb8819b3b86d71e7e4340681469d13188e42c82 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 10:50:36 +0000 Subject: [PATCH 47/83] minor edit --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 91cd5ba96..8197ba6aa 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -99,7 +99,7 @@ export function validateQueryParamsFn (req, res, next) { next() } catch (error) { const err = new MiddlewareError('Query params validation error', 400, { cause: error }) - res.status(400).render(err.template, { ...errorTemplateContext(), err }) + res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err }) } } From 73d58b1ed964ac5a16365046d8a3bd85c36df894 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 10:57:04 +0000 Subject: [PATCH 48/83] add JSDoc string to MiddlewareError --- src/utils/errors.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/utils/errors.js b/src/utils/errors.js index 6d49e5254..edddd0248 100644 --- a/src/utils/errors.js +++ b/src/utils/errors.js @@ -9,6 +9,11 @@ export function errorTemplateContext () { return { ...context } } +/** + * Use this class if you want to display specific HTTP error page. + * + * Uses the `errorPages/error.njk` template, but it can be overriden via options. + */ export class MiddlewareError extends Error { /** * @param {string} message From 44ffec6b795889ed4c2ec2c8f08f52e726c3645a Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 11:00:27 +0000 Subject: [PATCH 49/83] fix linting --- src/utils/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/errors.js b/src/utils/errors.js index edddd0248..13d1ff84c 100644 --- a/src/utils/errors.js +++ b/src/utils/errors.js @@ -11,7 +11,7 @@ export function errorTemplateContext () { /** * Use this class if you want to display specific HTTP error page. - * + * * Uses the `errorPages/error.njk` template, but it can be overriden via options. */ export class MiddlewareError extends Error { From a476ad428a41854c515a04f869155b1342a0b260 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 11:09:53 +0000 Subject: [PATCH 50/83] format the error page template --- src/views/errorPages/error.njk | 54 ++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index 9f17ee972..a1048953c 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -43,34 +43,36 @@ -
    - {% if env !== 'production' and err.statusCode !== 404 %} + {% if env !== 'production' and err.statusCode !== 404 %} +
    + +
    + Error details + 
    +          {{ err.stack | safe }}
+
    + {% if err.cause %} +

    Cause:

    + 
    +          {{ err.cause.stack | safe }}
+
    + {% endif %} +
    + + {% set issues = (err.cause | schemaIssues) %} + {% if issues.length > 0 %}
    - Error details - 
    -            {{ err.stack | safe }}
-
    - {% if err.cause %} -

    Cause:

    - 
    -            {{ err.cause.stack | safe }}
-
    - {% endif %} + Schema Issues + {% for issue in issues %} +
    +

    Path: [{{ issue.path.join(', ') }}]

    +

    Message:: {{ issue.message }}

    +
    + {% endfor %}
    - {% set issues = (err.cause | schemaIssues) %} - {% if issues.length > 0 %} -
    - Schema Issues - {% for issue in issues %} -
    -

    Path: [{{ issue.path.join(', ') }}]

    -

    Message:: {{ issue.message }}

    -
    - {% endfor %} -
    - - {% endif %} {% endif %} -
    +
    + {% endif %} + {% endblock %} From b079e172b319a7f67150a5590bfbf15cd7ab3bea Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 13:45:40 +0000 Subject: [PATCH 51/83] remove debug code --- src/views/errorPages/error.njk | 1 - 1 file changed, 1 deletion(-) diff --git a/src/views/errorPages/error.njk b/src/views/errorPages/error.njk index a1048953c..984dce96d 100644 --- a/src/views/errorPages/error.njk +++ b/src/views/errorPages/error.njk @@ -3,7 +3,6 @@ {% set pageNames = { '400': 'Invalid parameters', '404': 'Page not found', - '412': 'Yo momma too fat', '500': 'Sorry, there’s a problem with the service', '503': 'Sorry, the service is unavailable', '504': 'Sorry, there’s a problem with the service'} %} From 9760e44f540617398f8d54e4ac2c2fa1397ebb37 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Thu, 9 Jan 2025 17:14:04 +0000 Subject: [PATCH 52/83] better schema Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- config/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/util.js b/config/util.js index bb8b57649..2f004de7b 100644 --- a/config/util.js +++ b/config/util.js @@ -97,7 +97,7 @@ export const ConfigSchema = v.object({ tablePageLength: v.number(), contact: v.object({ issues: v.object({ - email: v.string() + email: v.pipe(v.string(), v.email()) }) }) }) From f1077a6f00398baa2ce2753b784e818b3b7cc333 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 10:59:18 +0000 Subject: [PATCH 53/83] more details in error message --- src/middleware/entryIssueDetails.middleware.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 62d4881f5..1645bb2eb 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -78,7 +78,8 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const error = new MiddlewareError('Missing required values on request object', 404) + const details = `issues[pageNumber-1]=${issues[pageNumber-1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` + const error = new MiddlewareError(`Missing required values on request object: ${details}`, 404) return next(error) } From 8c63fd232430ad239895b5f640c01957a89cb8b1 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:01:22 +0000 Subject: [PATCH 54/83] linting --- src/middleware/entryIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 1645bb2eb..8301111a6 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -78,7 +78,7 @@ export const prepareEntry = (req, res, next) => { const { pageNumber } = req.parsedParams if (!issues[pageNumber - 1] || !resources) { - const details = `issues[pageNumber-1]=${issues[pageNumber-1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` + const details = `issues[pageNumber-1]=${issues[pageNumber - 1] ? 'present' : 'missing'} resources=${resources ? 'present' : 'missing'}` const error = new MiddlewareError(`Missing required values on request object: ${details}`, 404) return next(error) } From eba05e84abfc0c2133e65ec552737c73a78dafc0 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:09:30 +0000 Subject: [PATCH 55/83] update unit tests --- test/unit/middleware/entryIssueDetails.middleware.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 1ac73d14f..fa1aeaec6 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -128,7 +128,7 @@ describe('entryIssueDetails.middleware.test.js', () => { const next = vi.fn() prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=missing resources=present')) }) it('should throw error if resources is missing', () => { @@ -149,7 +149,7 @@ describe('entryIssueDetails.middleware.test.js', () => { prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=present resources=missing')) }) }) From 6866391f852bfd154ae9d5700ea76ed9eb60ced4 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 10 Jan 2025 11:17:19 +0000 Subject: [PATCH 56/83] fix flaky unit tests --- test/unit/views/organisations/lpaOverviewPage.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/views/organisations/lpaOverviewPage.test.js b/test/unit/views/organisations/lpaOverviewPage.test.js index e4b3157bf..b6552990d 100644 --- a/test/unit/views/organisations/lpaOverviewPage.test.js +++ b/test/unit/views/organisations/lpaOverviewPage.test.js @@ -67,13 +67,13 @@ describe(`LPA Overview Page (seed: ${seed})`, () => { }) it('The correct number of dataset cards are rendered with the correct titles in group "statutory"', () => { - if (params.datasets.statutory) { + if (params.datasets.statutory && params.datasets.statutory.length > 0) { datasetGroup({ expect }, 'statutory', params.datasets.statutory, document) } }) it('The correct number of dataset cards are rendered with the correct titles in group "other"', () => { - if (params.datasets.other) { + if (params.datasets.other && params.datasets.other.length > 0) { datasetGroup({ expect }, 'other', params.datasets.other, document) const infoText = document.querySelector('.org-membership-info').textContent From 3b39eaa1de57a3ea2f0da793a6ba777b79405e5d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 13 Jan 2025 16:36:22 +0000 Subject: [PATCH 57/83] improve error logging --- src/middleware/common.middleware.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index df2a7af9c..53395fb5f 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -217,7 +217,7 @@ export const addEntityCountsToResources = async (req, res, next) => { try { const datasetResources = await Promise.all(promises).catch(error => { - logger.error('Failed to fetch dataset resources', { error, type: types.App }) + logger.error('Failed to fetch dataset resources', { type: types.DataFetch, errorMessage: error.message, errorStack: error.stack }) throw error }) @@ -227,7 +227,7 @@ export const addEntityCountsToResources = async (req, res, next) => { next() } catch (error) { - logger.error('Error in addEntityCountsToResources', { error, type: types.App }) + logger.error('Error in addEntityCountsToResources', { type: types.App, errorMessage: error.message, errorStack: error.stack }) next(error) } } From c3ddbe1c7d1d932b82a8697389404d80af14e91e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 13 Jan 2025 16:38:29 +0000 Subject: [PATCH 58/83] compare timestamps instead of potenitally incorect dates --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 53395fb5f..84dcc61ba 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -546,7 +546,7 @@ export const getMostRecentResources = (resources) => { const mostRecentResourcesMap = {} resources.forEach(resource => { const currentRecent = mostRecentResourcesMap[resource.dataset] - if (!currentRecent || new Date(currentRecent.start_date) < resource.start_date) { + if (!currentRecent || new Date(currentRecent.start_date).getTime() < new Date(resource.start_date).getTime()) { mostRecentResourcesMap[resource.dataset] = resource } }) From 6b8d964ec4fd997eea5d7ae44d0fedbb716d34a0 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 13 Jan 2025 16:44:20 +0000 Subject: [PATCH 59/83] make sure to explictly test that rowcount is an integer --- src/services/performanceDbApi.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 039ad593d..c27069d1c 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -216,7 +216,7 @@ export default { } let message - if (rowCount && numIssues >= rowCount) { + if (Number.isInteger(rowCount) && numIssues >= rowCount) { message = messageInfo.allRows_message } else if (entityLevel) { message = numIssues === 1 From 3a7a22f5fb8e1dd28765e2e14ea7546126692c3f Mon Sep 17 00:00:00 2001 From: Dilwoar Hussain Date: Mon, 13 Jan 2025 13:33:18 +0000 Subject: [PATCH 60/83] Add back button to issues pages ## Context: - The issues details pages ([table view](https://submit.planning.data.gov.uk/organisations/local-authority:LBH/article-4-direction/missing%20value/document-url) and [entity view](https://submit.planning.data.gov.uk/organisations/local-authority:LBH/article-4-direction/missing%20value/document-url/entity/1)) are orphaned from the wider navigation of the dataset details pages. - These pages should have a 'Back' button to help the user get back to the previous page. ## Acceptance criteria: - As a data provider, when I'm viewing the issues table page, I see a back button to get me back to the task list page, with the content: `Back to task list`. - As a data provider, when I'm viewing the issues entity level page, I see a back button to get me back to the issues table page, with the content: `Back to dataset table`. https://github.com/digital-land/submit/issues/736 --- src/middleware/common.middleware.js | 5 +++-- src/routes/schemas.js | 1 + src/views/organisations/issueDetails.html | 15 ++++++++++++++- src/views/organisations/issueTable.html | 8 ++++++++ .../entityIssueDetails.middleware.test.js | 1 + .../entryIssueDetails.middleware.test.js | 1 + 6 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 69c28114f..ef5f7be18 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -628,7 +628,7 @@ export function getErrorSummaryItems (req, res, next) { export const prepareIssueDetailsTemplateParams = (req, res, next) => { const { entry, pagination, dataRange, errorSummary, dataset, orgInfo } = req - const { issue_type: issueType, pageNumber } = req.parsedParams + const { issue_type: issueType, issue_field: issueField, pageNumber } = req.parsedParams // schema: OrgIssueDetails req.templateParams = { @@ -637,6 +637,7 @@ export const prepareIssueDetailsTemplateParams = (req, res, next) => { errorSummary, entry, issueType, + issueField, pagination, pageNumber, dataRange @@ -658,7 +659,7 @@ export const fetchSources = fetchMany({ SELECT rhe.endpoint, rhe.endpoint_url, - case + case when rhe.status = '' or rhe.status is null then null else cast(rhe.status as int) end as status, diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 5a5117bfb..1ee7e351c 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -220,6 +220,7 @@ export const OrgIssueDetails = v.strictObject({ dataset: DatasetNameField, errorSummary: errorSummaryParams, issueType: NonEmptyString, + issueField: NonEmptyString, entry: v.strictObject({ title: NonEmptyString, fields: v.array(v.strictObject({ diff --git a/src/views/organisations/issueDetails.html b/src/views/organisations/issueDetails.html index ed5614173..b44a2140c 100644 --- a/src/views/organisations/issueDetails.html +++ b/src/views/organisations/issueDetails.html @@ -36,7 +36,11 @@ href: '/organisations/' + organisation.organisation + '/' + dataset.dataset }, { - text: issueType | capitalize + text: issueType | capitalize, + href: '/organisations/' + organisation.organisation + '/' + dataset.dataset + '/' + issueType + '/' + issueField | urlencode + }, + { + text: entry.title } ] }) }} @@ -45,6 +49,15 @@ {% block content %} +
    +
    + {{ govukBackLink({ + text: "Back to dataset table", + href: '/organisations/' + organisation.organisation + '/' + dataset.dataset + '/' + issueType + '/' + issueField | urlencode + }) }} +
    +
    +
    {% include "includes/_dataset-page-header.html" %}
    diff --git a/src/views/organisations/issueTable.html b/src/views/organisations/issueTable.html index 49d9b1133..a3c44340e 100644 --- a/src/views/organisations/issueTable.html +++ b/src/views/organisations/issueTable.html @@ -47,6 +47,14 @@ {% block content %} +
    +
    + {{ govukBackLink({ + text: "Back to task list", + href: '/organisations/' + organisation.organisation | urlencode + '/' + dataset.dataset | urlencode + }) }} +
    +
    {% include "includes/_dataset-page-header.html" %} diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index 8dc736318..9d0e4ee55 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -170,6 +170,7 @@ describe('issueDetails.middleware.js', () => { geometries: ['POINT(0 0)'] }, issueType: 'test-issue-type', + issueField: 'test-issue-field', pagination: { items: [{ current: true, diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 1ac73d14f..5fbd33eb7 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -196,6 +196,7 @@ describe('entryIssueDetails.middleware.test.js', () => { geometries: ['POINT(0 0)'] }, issueType: 'test-issue-type', + issueField: 'test-issue-field', pagination: { items: [{ current: true, From a01444057ef01e6b76df5d705e03e0a6e4b64d5c Mon Sep 17 00:00:00 2001 From: Dilwoar Hussain Date: Mon, 13 Jan 2025 13:37:44 +0000 Subject: [PATCH 61/83] Change `docker-compose` to `docker compose` `docker-compose` is deprecated in the new version of docker --- package.json | 6 +++--- readme.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 5d5015bd2..091d5a05a 100644 --- a/package.json +++ b/package.json @@ -6,15 +6,15 @@ "type": "module", "scripts": { "prepare": "husky", - "dev": "docker-compose -f docker-compose-real-backend-minus-frontend.yml up -d && npm run start:local", + "dev": "docker compose -f docker-compose-real-backend-minus-frontend.yml up -d && npm run start:local", "start": "node index.js", "start:watch": "concurrently \"nodemon --exec 'npm run build && npm run start'\" \"npm run scss:watch\"", "start:test": "NODE_ENV=test node index.js", "start:local": "NODE_ENV=local node index.js", - "start:wiremock": "docker-compose up -d --force-recreate --build && NODE_ENV=wiremock node index.js", + "start:wiremock": "docker compose up -d --force-recreate --build && NODE_ENV=wiremock node index.js", "start:development": "NODE_ENV=development node index.js", "start:local:watch": "NODE_ENV=test npm run start:watch", - "docker-security-scan": "docker-compose -f docker-compose.security.yml run --rm zap", + "docker-security-scan": "docker compose -f docker-compose.security.yml run --rm zap", "static-security-scan": "npm audit --json > npm-audit-report.json", "scan:zap": "npm run docker-security-scan && npm run static-security-scan", "mock:api": "node ./test/mock-api/index.js", diff --git a/readme.md b/readme.md index 1ff605f96..f07d48249 100644 --- a/readme.md +++ b/readme.md @@ -102,11 +102,11 @@ of [package.json](package.json)) for more examples. ``` - Run the application using docker ``` - docker-compose -f docker-compose-real-backend.yml up + docker compose -f docker-compose-real-backend.yml up ``` - Run the application (without the frontend) using docker ``` - docker-compose -f docker-compose-real-backend-minus-frontend.yml up + docker compose -f docker-compose-real-backend-minus-frontend.yml up ``` - Run external services in containers and start application ``` From 8445b485b107814ed14989653869333dbe7d1f60 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:02:19 +0000 Subject: [PATCH 62/83] improve jsdoc --- src/middleware/datasetTaskList.middleware.js | 22 ++++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index c5dbf389a..8ce322481 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -40,16 +40,20 @@ function getStatusTag (status) { const SPECIAL_ISSUE_TYPES = ['reference values are not unique'] /** - * Prepares the task list for the dataset task list page + * Generates a list of tasks based on the issues found in the dataset. * - * This function takes the request, response, and next middleware function as arguments - * and uses the parsed request parameters, entities, resources, and entry/ entity issue counts - * to generate a list of tasks based on the issues found in the dataset - * - * @param {Object} req - The request object - * @param {Object} res - The response object - * @param {Function} next - The next middleware function - * @return {undefined} + * @param {Object} req - The request object. It should contain the following properties: + * - {Object} parsedParams: An object containing the parameters of the request. It should have the following properties: + * - {string} lpa: The LPA (Local Planning Authority) associated with the request. + * - {string} dataset: The name of the dataset associated with the request. + * - {Array} entities: An array of entity objects. + * - {Array} resources: An array of resource objects. + * - {Array} sources: An array of source objects. + * - {Object} entryIssueCounts: An object containing the issue counts for the entries in the dataset. + * - {Object} entityIssueCounts: An object containing the issue counts for the entities in the dataset. + * @param {Object} res - The response object. + * @param {Function} next - The next middleware function. + * @returns {undefined} */ export const prepareTasks = (req, res, next) => { const { lpa, dataset } = req.parsedParams From 03fc2accafab7da9e9d3d72155e69d562faa640a Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:26:28 +0000 Subject: [PATCH 63/83] explicitly state collumns we are fetching --- src/middleware/common.middleware.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 84dcc61ba..7f0a997eb 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -412,7 +412,7 @@ const fetchEntityIssuesForFieldAndType = fetchMany({ const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' return ` - select * + select i.issue_type, field, entity, message, severity, value from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' @@ -508,7 +508,7 @@ export const fetchEntryIssues = fetchMany({ const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' return ` - select * + select i.issue_type, field, entity, message, severity, value, line_number from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' From 6737606aeaab0de39bd58cc9c466eda619fe6139 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:33:18 +0000 Subject: [PATCH 64/83] improve error handling --- src/middleware/middleware.builders.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 4d0cc0a7f..10f124ca1 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -150,7 +150,7 @@ export async function fetchManyFromAllDatasetsFn (req, res, next) { const query = this.query({ req, params: req.params }) const promises = availableDatasets.map((dataset) => { return datasette.runQuery(query, dataset).catch(error => { - logger.error('Query failed for dataset', { dataset, error, type: types.DataFetch }) + logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) return { formattedData: [] } }) }) From ef9697ad7db82fc76c097f5f31dfff9a95707a06 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:35:38 +0000 Subject: [PATCH 65/83] improve error handling --- src/middleware/datasetTaskList.middleware.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index 8ce322481..a564950e9 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -13,6 +13,7 @@ import performanceDbApi from '../services/performanceDbApi.js' import { statusToTagClass } from '../filters/filters.js' import '../types/datasette.js' import logger from '../utils/logger.js' +import { types } from 'util' /** * Fetches the resource status @@ -86,7 +87,12 @@ export const prepareTasks = (req, res, next) => { try { title = performanceDbApi.getTaskMessage({ num_issues: count, rowCount, field, issue_type: type }) } catch (e) { - logger.warn('datasetTaskList::prepareTasks could not get task title so setting to default', { error: e, params: { num_issues: count, rowCount, field, issue_type: type } }) + logger.warn('Failed to generate task title', { + type: types.App, + errorMessage: e.message, + errorStack: e.stack, + params: { num_issues: count, rowCount, field, issue_type: type } + }) title = `${count} issue${count > 1 ? 's' : ''} of type ${type}` } From 7eb1eb8158fb9de2155adfa056e88dc7cd533c82 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:38:55 +0000 Subject: [PATCH 66/83] fix: Validate issue structure in entry issue details middleware --- src/middleware/entryIssueDetails.middleware.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index b46397b25..331718d98 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -69,6 +69,12 @@ export const prepareEntry = (req, res, next) => { const issue = entryIssues[0] + if (!issue.entry_number || !issue.issue_type || typeof issue.line_number !== 'number') { + const error = new Error('Invalid entry issue structure') + error.status = 500 + return next(error) + } + const errorMessage = issue.message || issue.issue_type req.entry = { From 90d39ad864f72e8940f9a9575a50fec5102fa164 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 11:39:23 +0000 Subject: [PATCH 67/83] Update task title in datasetTaskList.middleware.test.js --- test/unit/middleware/datasetTaskList.middleware.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index dd59b3ac5..1cddee257 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -66,7 +66,7 @@ describe('datasetTaskList.middleware.js', () => { expect(req.taskList).toEqual([ { title: { - text: undefined + text: 'Mocked task message' }, href: '/organisations/some-lpa/some-dataset/issue-type1/field1', status: { From 7d1cf92dc57fbcf3fe1777b87813ffb06511aacc Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 12:22:11 +0000 Subject: [PATCH 68/83] Revert "Update task title in datasetTaskList.middleware.test.js" This reverts commit 90d39ad864f72e8940f9a9575a50fec5102fa164. --- test/unit/middleware/datasetTaskList.middleware.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/middleware/datasetTaskList.middleware.test.js b/test/unit/middleware/datasetTaskList.middleware.test.js index 1cddee257..dd59b3ac5 100644 --- a/test/unit/middleware/datasetTaskList.middleware.test.js +++ b/test/unit/middleware/datasetTaskList.middleware.test.js @@ -66,7 +66,7 @@ describe('datasetTaskList.middleware.js', () => { expect(req.taskList).toEqual([ { title: { - text: 'Mocked task message' + text: undefined }, href: '/organisations/some-lpa/some-dataset/issue-type1/field1', status: { From 3e7221c621555471255dc0669f6480dff64a3839 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 15:49:00 +0000 Subject: [PATCH 69/83] fix tests --- src/middleware/datasetOverview.middleware.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 8e90399fd..014bc20b3 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -106,6 +106,13 @@ export const setNoticesFromSourceKey = (sourceKey) => (req, res, next) => { const { dataset } = req.params const resources = req[sourceKey] + if (!resources) { + logger.warn('No resources provided to set notices.', { + type: types.DataValidation + }) + return next() + } + const source = resources[0] const deadlineObj = requiredDatasets.find(deadline => deadline.dataset === dataset) @@ -126,7 +133,7 @@ export const setNoticesFromSourceKey = (sourceKey) => (req, res, next) => { const { deadlineDate, lastYearDeadline, twoYearsAgoDeadline } = getDeadlineHistory(deadlineObj.deadline) - const startDate = source ? new Date(source.startDate) : undefined + const startDate = source ? new Date(source.start_date) : undefined if (!startDate || startDate.toString() === 'Invalid Date') { logger.warn('Invalid start date encountered', { From f0b7c7ae6b1c6ecef8e0e2d9056c327c5fc47b38 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 14 Jan 2025 16:06:48 +0000 Subject: [PATCH 70/83] fix test --- test/unit/middleware/entryIssueDetails.middleware.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index fae486ce6..90c7848f0 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -84,6 +84,7 @@ describe('entryIssueDetails.middleware.test.js', () => { entry_number: 1, line_number: 2, field: 'Field Name', + issue_type: 'Type', message: 'Error message', value: 'Error value' } From b325713057473b1ec7ab06a2a6137ccc776220b4 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 09:41:14 +0000 Subject: [PATCH 71/83] ensure entry count is always a number --- src/middleware/common.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 7f0a997eb..e3ce13cdb 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -222,7 +222,7 @@ export const addEntityCountsToResources = async (req, res, next) => { }) req.resources = resources.map((resource, i) => { - return { ...resource, entry_count: datasetResources[i]?.formattedData[0]?.entry_count } + return { ...resource, entry_count: datasetResources[i]?.formattedData[0]?.entry_count ?? 0 } }) next() From 6149abe4efd5469aba6e738a532367eb8f234ef7 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 09:41:48 +0000 Subject: [PATCH 72/83] update logging --- src/middleware/middleware.builders.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 10f124ca1..ef5f44435 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -139,7 +139,7 @@ export async function fetchOneFromAllDatasetsFn (req, res, next) { logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) next() } catch (error) { - logger.debug('fetchMany: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + logger.debug('fetchOneFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) req.handlerName = `fetching '${this.result}'` next(error) } @@ -162,7 +162,7 @@ export async function fetchManyFromAllDatasetsFn (req, res, next) { logger.debug({ type: types.DataFetch, message: 'fetchManyFromAllDatasets', resultKey: this.result }) next() } catch (error) { - logger.debug('fetchMany: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + logger.debug('fetchManyFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) req.handlerName = `fetching '${this.result}'` next(error) } From c2a85cd8593b7706651ee14e228a51b6b54e3ee1 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 09:45:40 +0000 Subject: [PATCH 73/83] fail all for fetch from all datasets --- src/middleware/middleware.builders.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index ef5f44435..8b33f560f 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -125,7 +125,10 @@ export async function fetchOneFromAllDatasetsFn (req, res, next) { try { const query = this.query({ req, params: req.params }) const promises = availableDatasets.map((dataset) => { - return datasette.runQuery(query, dataset) + return datasette.runQuery(query, dataset).catch(error => { + logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) + throw error + }) }) const result = await Promise.all(promises) req[this.result] = Object.fromEntries( @@ -151,7 +154,7 @@ export async function fetchManyFromAllDatasetsFn (req, res, next) { const promises = availableDatasets.map((dataset) => { return datasette.runQuery(query, dataset).catch(error => { logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) - return { formattedData: [] } + throw error }) }) const result = await Promise.all(promises) From 309f105c676a3581b9565f7e9443f5d6481a088c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 10:14:26 +0000 Subject: [PATCH 74/83] remove unused middleware functions --- src/middleware/middleware.builders.js | 75 --------------------------- 1 file changed, 75 deletions(-) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 8b33f560f..518fea9be 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -13,13 +13,6 @@ import { templateSchema } from '../routes/schemas.js' import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import * as v from 'valibot' -import { dataSubjects } from '../utils/utils.js' - -const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => - (dataSubject.dataSets || []) - .filter((dataset) => dataset.available) - .map((dataset) => dataset.value) -) export const FetchOptions = { /** @@ -121,56 +114,6 @@ export async function fetchManyFn (req, res, next) { } } -export async function fetchOneFromAllDatasetsFn (req, res, next) { - try { - const query = this.query({ req, params: req.params }) - const promises = availableDatasets.map((dataset) => { - return datasette.runQuery(query, dataset).catch(error => { - logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) - throw error - }) - }) - const result = await Promise.all(promises) - req[this.result] = Object.fromEntries( - result.reduce((acc, { formattedData }, i) => { - if (formattedData.length > 0) { - acc.push([availableDatasets[i], formattedData[0]]) - } - return acc - }, []) - ) - logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) - next() - } catch (error) { - logger.debug('fetchOneFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) - req.handlerName = `fetching '${this.result}'` - next(error) - } -} - -export async function fetchManyFromAllDatasetsFn (req, res, next) { - try { - const query = this.query({ req, params: req.params }) - const promises = availableDatasets.map((dataset) => { - return datasette.runQuery(query, dataset).catch(error => { - logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) - throw error - }) - }) - const result = await Promise.all(promises) - req[this.result] = Object.fromEntries( - result.filter(({ formattedData }) => formattedData.length > 0) - .map(({ formattedData }, i) => [availableDatasets[i], formattedData]) - ) - logger.debug({ type: types.DataFetch, message: 'fetchManyFromAllDatasets', resultKey: this.result }) - next() - } catch (error) { - logger.debug('fetchManyFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) - req.handlerName = `fetching '${this.result}'` - next(error) - } -} - /** * Middleware. Does a conditional fetch. Optionally invokes `else` if condition is false. * @@ -235,24 +178,6 @@ export function fetchMany (context) { return fetchManyFn.bind(context) } -/** - * Fetches a single record from each dataset databases and stores them in `req` under key specified by `result` entry. - * - * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context - */ -export function fetchOneFromAllDatasets (context) { - return fetchOneFromAllDatasetsFn.bind(context) -} - -/** - * Fetches a collection of records from all dataset databases and stores them in `req` under key specified by `result` entry. - * - * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context - */ -export function fetchManyFromAllDatasets (context) { - return fetchManyFromAllDatasetsFn.bind(context) -} - /** * Looks up schema for name in @{link templateSchema} (defaults to any()), validates and renders the template. * From fb3becebe321b37731f28bec62693c065fed3129 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 10:15:31 +0000 Subject: [PATCH 75/83] fix duplicate key --- test/unit/middleware/entryIssueDetails.middleware.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 2ab93520c..90c7848f0 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -83,7 +83,6 @@ describe('entryIssueDetails.middleware.test.js', () => { { entry_number: 1, line_number: 2, - issue_type: 'issue type', field: 'Field Name', issue_type: 'Type', message: 'Error message', From 17256ce599174b9a65f0e541fb715b750a8813ca Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 15 Jan 2025 12:45:03 +0000 Subject: [PATCH 76/83] fixing some entity issues that have entry numbers (due to duplicate reference issue) --- src/middleware/entryIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 331718d98..cf632cf9b 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -69,7 +69,7 @@ export const prepareEntry = (req, res, next) => { const issue = entryIssues[0] - if (!issue.entry_number || !issue.issue_type || typeof issue.line_number !== 'number') { + if (!(issue.entry_number || issue.entity) || !issue.issue_type || typeof issue.line_number !== 'number') { const error = new Error('Invalid entry issue structure') error.status = 500 return next(error) From edb4eea396a3c6df702d7701e1ca76eb8831d0c6 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 09:49:41 +0000 Subject: [PATCH 77/83] fix unit tests --- test/unit/middleware/entryIssueDetails.middleware.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index f742ad67e..2a2c68a03 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -129,7 +129,7 @@ describe('entryIssueDetails.middleware.test.js', () => { const next = vi.fn() prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=missing resources=present')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: entryIssues: present, entryIssues[0]: missing, resources: present')) }) it('should throw error if resources is missing', () => { @@ -150,7 +150,7 @@ describe('entryIssueDetails.middleware.test.js', () => { prepareEntry(req, res, next) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=present resources=missing')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: entryIssues: present, entryIssues[0]: present, resources: missing')) }) }) From 84c2bc9090f297b9ef00b7b0828a62f062534555 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 10:15:31 +0000 Subject: [PATCH 78/83] add back in fetchFromAllDatasets middlewares --- src/middleware/middleware.builders.js | 109 ++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index fe0d7639b..8dfc2db57 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -15,6 +15,14 @@ import datasette from '../services/datasette.js' import * as v from 'valibot' import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' +import { dataSubjects } from '../utils/utils.js' + +const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => + (dataSubject.dataSets || []) + .filter((dataset) => dataset.available) + .map((dataset) => dataset.value) +) + export const FetchOptions = { /** * Use 'dataset' from requets params. @@ -117,6 +125,89 @@ export async function fetchManyFn (req, res, next) { } } +/** + * Fetches one result from all available datasets. + * + * This function runs a query on each available dataset, catches any errors that may occur, + * and then compiles the results into a single object. The result object is then attached to + * the request object. + * + * @async + * @function fetchOneFromAllDatasetsFn + * @param {Object} req - The request object. + * @param {Object} req.params - Route parameters. + * @param {string} req.originalUrl - The original URL of the request. + * @param {Object} req[this.result] - A property to store the result of the query. + * @param {string} req.handlerName - A property to store the name of the handler. + * @param {function} req.query - A function to construct a query based on the request parameters. + * @param {Object} res - The response object. + * @param {Function} next - The next middleware function in the stack. + * @throws {Error} If any of the queries fail. + * @memberof middleware + */ +export async function fetchOneFromAllDatasetsFn (req, res, next) { + try { + const query = this.query({ req, params: req.params }) + const promises = availableDatasets.map((dataset) => { + return datasette.runQuery(query, dataset).catch(error => { + logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) + throw error + }) + }) + const result = await Promise.all(promises) + req[this.result] = Object.fromEntries( + result.reduce((acc, { formattedData }, i) => { + if (formattedData.length > 0) { + acc.push([availableDatasets[i], formattedData[0]]) + } + return acc + }, []) + ) + logger.debug({ type: types.DataFetch, message: 'fetchOneFromAllDatasets', resultKey: this.result }) + next() + } catch (error) { + logger.debug('fetchOneFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + req.handlerName = `fetching '${this.result}'` + next(error) + } +} + +/** + * Fetches data from all available datasets and stores the result in the request object. + * + * @async + * @function fetchManyFromAllDatasetsFn + * @param {Object} req - The request object. + * @param {string} req.params - The URL parameters for the request. + * @param {Object} req[this.result] - Property of the request object where the result will be stored. + * @param {Object} res - The response object. + * @param {Function} next - The next middleware function in the chain. + * + * @throws {Error} If an error occurs while fetching data from any of the datasets. + */ +export async function fetchManyFromAllDatasetsFn (req, res, next) { + try { + const query = this.query({ req, params: req.params }) + const promises = availableDatasets.map((dataset) => { + return datasette.runQuery(query, dataset).catch(error => { + logger.error('Query failed for dataset', { dataset, errorMessage: error.message, errorStack: error.stack, type: types.DataFetch }) + throw error + }) + }) + const result = await Promise.all(promises) + req[this.result] = Object.fromEntries( + result.filter(({ formattedData }) => formattedData.length > 0) + .map(({ formattedData }, i) => [availableDatasets[i], formattedData]) + ) + logger.debug({ type: types.DataFetch, message: 'fetchManyFromAllDatasets', resultKey: this.result }) + next() + } catch (error) { + logger.debug('fetchManyFromAllDatasetsFn: failed', { type: types.DataFetch, errorMessage: error.message, endpoint: req.originalUrl, resultKey: this.result }) + req.handlerName = `fetching '${this.result}'` + next(error) + } +} + /** * Middleware. Does a conditional fetch. Optionally invokes `else` if condition is false. * @@ -181,6 +272,24 @@ export function fetchMany (context) { return fetchManyFn.bind(context) } +/** + * Fetches a single record from each dataset databases and stores them in `req` under key specified by `result` entry. + * + * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context + */ +export function fetchOneFromAllDatasets (context) { + return fetchOneFromAllDatasetsFn.bind(context) +} + +/** + * Fetches a collection of records from all dataset databases and stores them in `req` under key specified by `result` entry. + * + * @param {{query: ({req, params}) => object, result: string, dataset?: FetchParams | (req) => string}} context + */ +export function fetchManyFromAllDatasets (context) { + return fetchManyFromAllDatasetsFn.bind(context) +} + /** * Looks up schema for name in @{link templateSchema} (defaults to any()), validates and renders the template. * From c248ada4433c129c5f0464c1d661ce63988dce88 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 10:21:58 +0000 Subject: [PATCH 79/83] catch undefined issue counts --- src/middleware/datasetOverview.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 014bc20b3..54f730b6a 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -240,7 +240,7 @@ export const prepareDatasetOverviewTemplateParams = (req, res, next) => { req.templateParams = { organisation: orgInfo, dataset, - taskCount: entryIssueCounts.length + entityIssueCounts.length + endpointErrorIssues, + taskCount: (entryIssueCounts ? entryIssueCounts.length : 0) + (entityIssueCounts ? entityIssueCounts.length : 0) + endpointErrorIssues, stats: { numberOfFieldsSupplied: numberOfFieldsSupplied ?? 0, numberOfFieldsMatched: numberOfFieldsMatched ?? 0, From 8022dce7e167b4ca5a86fb3be37814064b09e0d4 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 10:25:41 +0000 Subject: [PATCH 80/83] remove code comment --- src/middleware/datasetOverview.middleware.js | 59 -------------------- 1 file changed, 59 deletions(-) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index 54f730b6a..eef0f02d2 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -37,65 +37,6 @@ const fetchSpecification = fetchOne({ result: 'specification' }) -// const fetchSources = fetchMany({ -// query: ({ params }) => ` -// WITH RankedEndpoints AS ( -// SELECT -// rhe.endpoint, -// rhe.endpoint_url, -// rhe.status, -// rhe.exception, -// rhe.resource, -// rhe.latest_log_entry_date, -// rhe.endpoint_entry_date, -// rhe.endpoint_end_date, -// rhe.resource_start_date as resource_start_date, -// rhe.resource_end_date, -// s.documentation_url, -// ROW_NUMBER() OVER ( -// PARTITION BY rhe.endpoint_url -// ORDER BY -// rhe.latest_log_entry_date DESC -// ) AS row_num -// FROM -// reporting_historic_endpoints rhe -// LEFT JOIN source s ON rhe.endpoint = s.endpoint -// WHERE -// REPLACE(rhe.organisation, '-eng', '') = '${params.lpa}' -// AND rhe.pipeline = '${params.dataset}' -// AND ( -// rhe.resource_end_date >= current_timestamp -// OR rhe.resource_end_date IS NULL -// OR rhe.resource_end_date = '' -// ) -// AND ( -// rhe.endpoint_end_date >= current_timestamp -// OR rhe.endpoint_end_date IS NULL -// OR rhe.endpoint_end_date = '' -// ) -// ) -// SELECT -// endpoint, -// endpoint_url, -// status, -// exception, -// resource, -// latest_log_entry_date, -// endpoint_entry_date, -// endpoint_end_date, -// resource_start_date, -// resource_end_date, -// documentation_url -// FROM -// RankedEndpoints -// WHERE -// row_num = 1 -// ORDER BY -// latest_log_entry_date DESC; -// `, -// result: 'sources' -// }) - /** * Sets notices from a source key in the request object. * From 677990457f39fae850320328ccf95dbdae6bdc08 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 10:31:40 +0000 Subject: [PATCH 81/83] remove comments --- src/middleware/dataview.middleware.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index 0a6aa04ba..d01de6784 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -94,7 +94,6 @@ export default [ fetchOrgInfo, fetchDatasetInfo, - // fetch sources fetchResources, fetchEntityIssueCounts, fetchEntryIssueCounts, @@ -104,8 +103,6 @@ export default [ getSetDataRange(config.tablePageLength), show404IfPageNumberNotInRange, - // fetchTaskCount - fetchEntities, extractJsonFieldFromEntities, replaceUnderscoreInEntities, From a8a49187dddd6b207e750cc41adfa152592fc462 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 10:34:56 +0000 Subject: [PATCH 82/83] add back in test and fix --- .../unit/middleware/overview.middleware.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/unit/middleware/overview.middleware.test.js b/test/unit/middleware/overview.middleware.test.js index 61f2af7d6..ab724f91f 100644 --- a/test/unit/middleware/overview.middleware.test.js +++ b/test/unit/middleware/overview.middleware.test.js @@ -102,6 +102,23 @@ describe('overview.middleware', () => { expect(errorCardNodes[0].querySelector('.govuk-task-list__hint').textContent.trim()).toBe('There was an error accessing the data URL') expect(errorCardNodes[1].querySelector('.govuk-task-list__hint').textContent.trim()).toBe('There was a 404 error') }) + + it('should patch dataset status based on the provision_summary info', () => { + const req = structuredClone(reqTemplate) + console.assert(req.datasets[0].status === 'Live') + req.datasetErrorStatus = [{ dataset: 'dataset1' }] + const res = { render: vi.fn() } + + prepareOverviewTemplateParams(req, res, () => {}) + + const ds1 = req.templateParams.datasets.statutory[0] + expect(ds1.status).toBe('Live') + expect(ds1.error).toBeUndefined() + + const ds4 = req.templateParams.datasets.other[1] + expect(ds4.status).toBe('Error') + expect(ds4.error).toBe(req.datasets[3].error) // Error message should be left untouched + }) }) describe('getOverview', () => { From bc87bf37f1f062a78582d4908b2bc13f4d4b44d5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 16 Jan 2025 11:03:10 +0000 Subject: [PATCH 83/83] fix for issue structure check --- src/middleware/entryIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 02882f8bf..1f222beca 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -74,7 +74,7 @@ export const prepareEntry = (req, res, next) => { const issue = entryIssues[0] - if (!(issue.entry_number || issue.entity) || !issue.issue_type || typeof issue.line_number !== 'number') { + if ((!issue.entry_number && !issue.entity && !issue.line_number) || !issue.issue_type || typeof issue.line_number !== 'number') { const error = new Error('Invalid entry issue structure') error.status = 500 return next(error)