From c3d1ad63ce2c6faada4bae9787a2dd624b4a22f8 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 13:43:25 +0000 Subject: [PATCH 01/30] 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 ebc62aed..b3db166f 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 284dd649..4f8277aa 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 442ee179..d7088d7a 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 c1fb07fe..1ed4c1ca 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/30] 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 184fbe69..45539a73 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/30] 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 7af60dd6..0ff69cc2 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 d176617b..870ca4d8 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/30] 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 4f8277aa..770c8471 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/30] 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 45539a73..6826bef5 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 770c8471..fa736949 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 8f6cf9fc..b7d44343 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 bd6a9a87..8e8a7f1c 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 cc2b7b62..0a667680 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/30] 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 0ff69cc2..864f21f8 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 1ac73d14..fae486ce 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 7e7c2cfc47193f172de5981a6d4d1fa27fa0451a Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 12 Dec 2024 11:37:47 +0000 Subject: [PATCH 07/30] 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 fa736949..f39426ba 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 08/30] 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 870ca4d8..3fa74ce7 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 09/30] 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 3fa74ce7..370e8f7b 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 10/30] 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 6826bef5..6a6d7ded 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 11/30] 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 6a6d7ded..0aae9446 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 f39426ba..7ae94c93 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 518fea9b..9e9b5c81 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 12/30] 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 9e9b5c81..e637aa26 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 13/30] 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 0aae9446..f5e958a2 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 7ae94c93..19df1407 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 b931c05c..f90a9639 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 14/30] 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 f5e958a2..3dc657b0 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 15/30] 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 19df1407..868f84d3 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 099f009aa2e17b5a29cc0494a8392cba0edecc73 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 13 Dec 2024 13:40:25 +0000 Subject: [PATCH 16/30] 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 3dc657b0..ba960ae3 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 17/30] 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 370e8f7b..60347ad4 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 18/30] 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 ba960ae3..a325e221 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 19/30] 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 868f84d3..70e0cd33 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 20/30] 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 e637aa26..62c587ea 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 21/30] 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 62c587ea..96989aa2 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 22/30] 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 a325e221..ea991474 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 23/30] 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 96989aa2..4d0cc0a7 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 24/30] 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 70e0cd33..fe8779df 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 60347ad4..c1340f83 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 25/30] 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 fe8779df..448d623c 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 26/30] 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 448d623c..962104e4 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 27/30] 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 962104e4..89d0a103 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 28/30] 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 c1340f83..9413772d 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 7de56d11ee4ec2c7ecfb404a779b8c742f308689 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 10:33:05 +0000 Subject: [PATCH 29/30] 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 ea991474..9b77bfa5 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 f84569f8e384ea1f20c18660e5615d24ef6d38c5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 17 Dec 2024 11:22:06 +0000 Subject: [PATCH 30/30] 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 b7d44343..b46397b2 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)