diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index aeab97eb..7f6b7b4e 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -39,8 +39,17 @@ export const fetchDatasetInfo = fetchOne({ * @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 + +export const and = (...args) => { + return (req) => args.every(arg => arg(req)) +} +export const or = (...args) => { + return (req) => args.some(arg => arg(req)) +} /** * Middleware. Updates req with `resource`. @@ -72,7 +81,7 @@ export const fetchOrgInfo = fetchOne({ /** * Middleware. Validates query params according to schema. - * Short circuits with 400 error if validation fails + * Short circuits with 400 error if validation fails. Potentially updates req with `parsedParams` * * `this` needs: `{ schema }` * @@ -82,7 +91,7 @@ export const fetchOrgInfo = fetchOne({ */ export function validateQueryParamsFn (req, res, next) { try { - v.parse(this.schema || v.any(), req.params) + req.parsedParams = v.parse(this.schema || v.any(), req.params) next() } catch (error) { res.status(400).render('errorPages/400', {}) diff --git a/src/middleware/datasetOverview.middleware.js b/src/middleware/datasetOverview.middleware.js index cbcf67d9..fc70c4fe 100644 --- a/src/middleware/datasetOverview.middleware.js +++ b/src/middleware/datasetOverview.middleware.js @@ -2,6 +2,8 @@ import { fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgI import { fetchOne, fetchIf, fetchMany, renderTemplate, FetchOptions, onlyIf } from './middleware.builders.js' import { fetchResourceStatus, prepareDatasetTaskListErrorTemplateParams } from './datasetTaskList.middleware.js' import performanceDbApi from '../services/performanceDbApi.js' +import logger from '../utils/logger.js' +import { types } from '../utils/logging.js' const fetchColumnSummary = fetchMany({ query: ({ params }) => ` @@ -36,11 +38,24 @@ const fetchSpecification = fetchOne({ result: 'specification' }) +/** + * Middleware. Updates req with `datasetSpecification` + * + * @param req + * @param res + * @param next + */ export const pullOutDatasetSpecification = (req, res, next) => { const { specification } = req - const collectionSpecifications = JSON.parse(specification.json) - const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset) - req.specification = datasetSpecification + let collectionSpecifications + try { + collectionSpecifications = JSON.parse(specification.json) + } catch (e) { + // we can proceed but we probably should notify the user the displayed data may not be complete + logger.info('failed to parse specification JSON', { type: types.DataValidation, collection: req.dataset.collection }) + collectionSpecifications = [] + } + req.datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset) next() } @@ -105,21 +120,21 @@ const fetchEntityCount = fetchOne({ }) export const prepareDatasetOverviewTemplateParams = (req, res, next) => { - const { orgInfo, specification, columnSummary, entityCount, sources, dataset, issues } = req + const { orgInfo, datasetSpecification, columnSummary, entityCount, sources, dataset, issues } = req const mappingFields = columnSummary[0]?.mapping_field?.split(';') ?? [] const nonMappingFields = columnSummary[0]?.non_mapping_field?.split(';') ?? [] const allFields = [...mappingFields, ...nonMappingFields] - const numberOfFieldsSupplied = specification.fields.map(field => field.field).reduce((acc, current) => { - return allFields.includes(current) ? acc + 1 : acc + const specFields = datasetSpecification ? datasetSpecification.fields : [] + const numberOfFieldsSupplied = specFields.reduce((acc, field) => { + return allFields.includes(field.field) ? acc + 1 : acc }, 0) - - const numberOfFieldsMatched = specification.fields.map(field => field.field).reduce((acc, current) => { - return mappingFields.includes(current) ? acc + 1 : acc + const numberOfFieldsMatched = specFields.reduce((acc, field) => { + return mappingFields.includes(field.field) ? acc + 1 : acc }, 0) - const numberOfExpectedFields = specification.fields.length + 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) => { diff --git a/src/middleware/datasetTaskList.middleware.js b/src/middleware/datasetTaskList.middleware.js index f77db39e..284dd649 100644 --- a/src/middleware/datasetTaskList.middleware.js +++ b/src/middleware/datasetTaskList.middleware.js @@ -1,4 +1,15 @@ -import { fetchDatasetInfo, isResourceAccessible, isResourceNotAccessible, fetchLatestResource, fetchEntityCount, logPageError, fetchLpaDatasetIssues, validateQueryParams, getDatasetTaskListError } from './common.middleware.js' +import { + fetchDatasetInfo, + isResourceAccessible, + isResourceNotAccessible, + fetchLatestResource, + fetchEntityCount, + logPageError, + fetchLpaDatasetIssues, + validateQueryParams, + getDatasetTaskListError, + isResourceIdValid, and +} from './common.middleware.js' import { fetchOne, fetchIf, onlyIf, renderTemplate } from './middleware.builders.js' import performanceDbApi from '../services/performanceDbApi.js' import { statusToTagClass } from '../filters/filters.js' @@ -46,7 +57,6 @@ 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(req.resourceStatus.resource === req.resource.resource, 'mismatch between resourceStatus and resource data') console.assert(typeof entityCount === 'number', 'entityCount should be a number') const taskList = issues.map((issue) => { @@ -113,6 +123,9 @@ const validateParams = validateQueryParams({ }) }) +/* eslint-disable-next-line no-return-assign */ +const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } + export default [ validateParams, fetchResourceStatus, @@ -120,7 +133,7 @@ export default [ fetchDatasetInfo, fetchIf(isResourceAccessible, fetchLatestResource), fetchIf(isResourceAccessible, fetchLpaDatasetIssues), - fetchIf(isResourceAccessible, fetchEntityCount), + fetchIf(and(isResourceAccessible, isResourceIdValid), fetchEntityCount, zeroEntityCount), onlyIf(isResourceAccessible, prepareDatasetTaskListTemplateParams), onlyIf(isResourceAccessible, getDatasetTaskList), onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js index a537a3c9..31e75df9 100644 --- a/src/middleware/issueDetails.middleware.js +++ b/src/middleware/issueDetails.middleware.js @@ -1,17 +1,26 @@ import performanceDbApi from '../services/performanceDbApi.js' -import logger from '../utils/logger.js' -import { types } from '../utils/logging.js' -import { fetchDatasetInfo, fetchEntityCount, fetchLatestResource, fetchOrgInfo, isResourceIdInParams, logPageError, takeResourceIdFromParams, validateQueryParams } from './common.middleware.js' +import { + fetchDatasetInfo, + fetchEntityCount, + fetchLatestResource, + fetchOrgInfo, + isResourceIdInParams, isResourceDataPresent, + logPageError, + takeResourceIdFromParams, + validateQueryParams +} from './common.middleware.js' import { fetchIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' import { pagination } from '../utils/pagination.js' +import logger from '../utils/logger.js' +import { types } from '../utils/logging.js' export const IssueDetailsQueryParams = v.object({ lpa: v.string(), dataset: v.string(), issue_type: v.string(), issue_field: v.string(), - pageNumber: v.optional(v.string()), + pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), resourceId: v.optional(v.string()) }) @@ -19,11 +28,13 @@ const validateIssueDetailsQueryParams = validateQueryParams({ schema: IssueDetailsQueryParams }) +const issuesQueryLimit = 1000 + /** * * Middleware. Updates `req` with `issues`. * - * Requires `resourceId` in request params or request (in that order). + * Requires resource id under `req.resource.resource`. * * @param {*} req * @param {*} res @@ -31,14 +42,12 @@ const validateIssueDetailsQueryParams = validateQueryParams({ */ async function fetchIssues (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params - const { resource: resourceId } = req.resource - if (!resourceId) { - logger.debug('fetchIssues(): missing resourceId', { type: types.App, params: req.params, resource: req.resource }) - throw Error('fetchIssues: missing resourceId') - } - + const { pageNumber } = req.parsedParams + const resourceId = req.resource.resource + const offset = Math.floor((pageNumber - 1) / issuesQueryLimit) * issuesQueryLimit + logger.debug('fetchIssues()', { type: types.App, resourceId, issueType, issueField, offset }) try { - const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField }, datasetId) + const issues = await performanceDbApi.getIssues({ resource: resourceId, issueType, issueField, offset }, datasetId) req.issues = issues next() } catch (error) { @@ -48,7 +57,7 @@ async function fetchIssues (req, res, next) { /** * - * Middleware. Updates `req` with `issues`. + * Middleware. Updates `req` with `issuesByEntryNumber` and `entryNumberCount`. * * Requires `issues` in request. * @@ -58,12 +67,15 @@ async function fetchIssues (req, res, next) { */ async function reformatIssuesToBeByEntryNumber (req, res, next) { const { issues } = req + const count = new Set() const issuesByEntryNumber = issues.reduce((acc, current) => { acc[current.entry_number] = acc[current.entry_number] || [] acc[current.entry_number].push(current) + count.add(current.entry_number) return acc }, {}) req.issuesByEntryNumber = issuesByEntryNumber + req.entryNumberCount = count.size next() } @@ -71,29 +83,35 @@ async function reformatIssuesToBeByEntryNumber (req, res, next) { * * Middleware. Updates `req` with `entryData` * - * Requires `pageNumber`, `dataset` and + * Requires `pageNumber`, `dataset`, `issuesByEntryNumber`, `resource` * - * @param {*} req + * @param {{ issuesByEntryNumber: Object, resource: { resource: string }, params: { dataset: string }, parsedParams: { pageNumber: number }}} req * @param {*} res * @param {*} next * */ async function fetchEntry (req, res, next) { - const { dataset: datasetId, pageNumber } = req.params - const { issuesByEntryNumber } = req - const pageNum = pageNumber ? parseInt(pageNumber) : 1 - req.pageNumber = pageNum + const { dataset: datasetId } = req.params + const { issues, issueEntitiesCount, issuesByEntryNumber } = req + const { pageNumber } = req.parsedParams - // look at issue Entries and get the index of that entry - 1 - - const entityNum = Object.values(issuesByEntryNumber)[pageNum - 1][0].entry_number + let entryData + const issuesByEntry = Object.values(issuesByEntryNumber) + if (issues.length > 0) { + if (pageNumber <= issueEntitiesCount) { + const entryIssues = issuesByEntry[(pageNumber - 1) % issuesQueryLimit] ?? [] + const entryNum = entryIssues.length > 0 ? entryIssues[0].entry_number : undefined + entryData = entryNum + ? await performanceDbApi.getEntry( + req.resource.resource, + entryNum, + datasetId + ) + : [] + } + } - req.entryData = await performanceDbApi.getEntry( - req.resource.resource, - entityNum, - datasetId - ) - req.entryNumber = entityNum + req.entryData = entryData ?? [] next() } @@ -110,9 +128,8 @@ async function fetchEntry (req, res, next) { async function fetchIssueEntitiesCount (req, res, next) { const { dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params const { resource: resourceId } = req.resource - console.assert(resourceId, 'missng resource id') const issueEntitiesCount = await performanceDbApi.getEntitiesWithIssuesCount({ resource: resourceId, issueType, issueField }, datasetId) - req.issueEntitiesCount = parseInt(issueEntitiesCount) + req.issueEntitiesCount = issueEntitiesCount next() } @@ -182,8 +199,9 @@ const processEntryRow = (issueType, issuesByEntryNumber, row) => { * Middleware. Updates req with `templateParams` */ export function prepareIssueDetailsTemplateParams (req, res, next) { - const { entryData, pageNumber, issueEntitiesCount, issuesByEntryNumber, entryNumber, entityCount: entityCountRow } = req + const { entryData, issueEntitiesCount, issuesByEntryNumber, entryNumberCount, entityCount: entityCountRow } = req const { lpa, dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params + const { pageNumber } = req.parsedParams const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } let errorHeading @@ -191,7 +209,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { const BaseSubpath = `/organisations/${lpa}/${datasetId}/${issueType}/${issueField}/` - if (Object.keys(issuesByEntryNumber).length < entityCount) { + if (entryNumberCount < entityCount) { errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issueEntitiesCount, entityCount, field: issueField }, true) issueItems = Object.entries(issuesByEntryNumber).map(([entryNumber, issues], i) => { const pageNum = i + 1 @@ -206,6 +224,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { }] } + const maxPageNumber = issueEntitiesCount const fields = entryData.map((row) => processEntryRow(issueType, issuesByEntryNumber, row)) const entityIssues = Object.values(issuesByEntryNumber)[pageNumber - 1] || [] for (const issue of entityIssues) { @@ -223,7 +242,7 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { .filter((row) => row.field === 'geometry') .map((row) => row.value) const entry = { - title: `entry: ${entryNumber}`, + title: `entry: ${entryData.length > 0 ? entryData[0].entry_number : ''}`, fields, geometries } @@ -235,13 +254,13 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { } } - if (pageNumber < issueEntitiesCount) { + if (pageNumber < maxPageNumber) { paginationObj.next = { href: `${BaseSubpath}${pageNumber + 1}` } } - paginationObj.items = pagination(issueEntitiesCount, pageNumber).map(item => { + paginationObj.items = pagination(maxPageNumber, pageNumber).map(item => { if (item === '...') { return { type: 'ellipsis', @@ -274,6 +293,25 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { next() } +/** + * Middleware. Short-circuits with 404 error if pageNumber is not in range. + * Updates req with `pageNumber` + * + * @param req + * @param res + * @param next + */ +const isPageNumberInRange = (req, res, next) => { + const { issueEntitiesCount } = req + const { pageNumber } = req.parsedParams + + if (pageNumber < 1 || issueEntitiesCount < pageNumber) { + res.status(404).render('errorPages/404', {}) + return + } + next() +} + /** * Middleware. Renders the issue details page with the list of issues, entry data, * and organisation and dataset details. @@ -284,16 +322,22 @@ export const getIssueDetails = renderTemplate({ handlerName: 'getIssueDetails' }) +/* eslint-disable no-return-assign */ +const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } +const zeroIssueEntitiesCount = (req) => req.issueEntitiesCount = 0 +const emptyIssuesCollection = (req) => req.issues = [] + export default [ validateIssueDetailsQueryParams, fetchOrgInfo, fetchDatasetInfo, fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), - fetchIssues, + fetchIf(isResourceDataPresent, fetchIssues, emptyIssuesCollection), reformatIssuesToBeByEntryNumber, + fetchIf(isResourceDataPresent, fetchIssueEntitiesCount, zeroIssueEntitiesCount), + isPageNumberInRange, fetchEntry, - fetchEntityCount, - fetchIssueEntitiesCount, + fetchIf(isResourceDataPresent, fetchEntityCount, zeroEntityCount), prepareIssueDetailsTemplateParams, getIssueDetails, logPageError diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index c5d648b8..1c0d2d58 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -150,7 +150,7 @@ async function fetchIfFn (req, res, next) { * @param {(req) => void} elseFn * @returns */ -export const fetchIf = (condition, fetchFn, elseFn) => { +export const fetchIf = (condition, fetchFn, elseFn = undefined) => { return fetchIfFn.bind({ condition, fetchFn, else: elseFn }) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 10d29d19..79a2862c 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -208,11 +208,8 @@ export default { /** * Returns a task message based on the provided issue type, issue count, and entity count. * - * @param {Object} options - Options object - * @param {string} options.issueType - The type of issue - * @param {number} options.num_issues - The number of issues - * @param {number} options.entityCount - The number of entities - * @param {boolean} [entityLevel=false] - Whether to use entity-level or dataset level messaging + * @param {{issue_type: string, num_issues: number, entityCount: 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 * @@ -367,7 +364,8 @@ export default { async getIssues ({ resource, issueType, - issueField + issueField, + offset = 0 }, database = 'digital-land') { const sql = ` SELECT i.field, i.line_number, entry_number, message, issue_type, value @@ -375,6 +373,8 @@ export default { WHERE resource = '${resource}' AND issue_type = '${issueType}' AND field = '${issueField}' + ORDER BY entry_number ASC + LIMIT 1000 ${offset ? `OFFSET ${offset}` : ''} ` const result = await datasette.runQuery(sql, database) @@ -390,8 +390,9 @@ export default { * @returns {Promise<{field: string, value: string, entry_number: number}[]>} */ async getEntry (resourceId, entryNumber, dataset) { + logger.debug({ message: 'getEntry()', resourceId, entryNumber, dataset, type: types.App }) // TODO: why do we order by rowid? - const sql = ` + const sql = /* sql */ ` select fr.rowid, fr.end_date, diff --git a/test/unit/middleware/datasetOverview.middleware.test.js b/test/unit/middleware/datasetOverview.middleware.test.js index 3060d6df..b0762a02 100644 --- a/test/unit/middleware/datasetOverview.middleware.test.js +++ b/test/unit/middleware/datasetOverview.middleware.test.js @@ -16,7 +16,7 @@ describe('Dataset Overview Middleware', () => { const res = {} describe('pullOutDatasetSpecification', () => { - it('', () => { + it('leaves specification unchanged, and extracts the dataset specification', () => { const reqWithSpecification = { ...req, specification: { @@ -26,7 +26,8 @@ describe('Dataset Overview Middleware', () => { } } pullOutDatasetSpecification(reqWithSpecification, res, () => {}) - expect(reqWithSpecification.specification).toEqual({ dataset: 'mock-dataset', foo: 'bar' }) + expect(reqWithSpecification.specification).toEqual(reqWithSpecification.specification) + expect(reqWithSpecification.datasetSpecification).toEqual({ dataset: 'mock-dataset', foo: 'bar' }) }) }) @@ -35,7 +36,7 @@ describe('Dataset Overview Middleware', () => { const reqWithResults = { ...req, orgInfo: { name: 'mock-org' }, - specification: { fields: [{ field: 'field1' }, { field: 'field2' }] }, + datasetSpecification: { fields: [{ field: 'field1' }, { field: 'field2' }] }, columnSummary: [{ mapping_field: 'field1', non_mapping_field: 'field3' }], entityCount: { entity_count: 10 }, sources: [ diff --git a/test/unit/middleware/issueDetails.middleware.test.js b/test/unit/middleware/issueDetails.middleware.test.js index 143d049c..8fe0ea58 100644 --- a/test/unit/middleware/issueDetails.middleware.test.js +++ b/test/unit/middleware/issueDetails.middleware.test.js @@ -36,9 +36,10 @@ describe('issueDetails.middleware.js', () => { } const req = { params: requestParams, + parsedParams: { pageNumber: 1 }, // middleware supplies the below entryNumber: 1, - entityCount: { entity_count: 3 }, + entityCount: { entity_count: 1 }, issueEntitiesCount: 1, pageNumber: 1, orgInfo, @@ -46,6 +47,7 @@ describe('issueDetails.middleware.js', () => { entryData, issues, resource: { resource: requestParams.resourceId }, + entryNumberCount: 1, issuesByEntryNumber: { 1: [ { @@ -65,7 +67,6 @@ describe('issueDetails.middleware.js', () => { issues.forEach(issue => { vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce(`mockMessageFor: ${issue.entry_number}`) }) - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce('mock task message 1') prepareIssueDetailsTemplateParams(req, {}, () => {}) @@ -79,11 +80,10 @@ describe('issueDetails.middleware.js', () => { dataset: 'mock-dataset', collection: 'mock-collection' }, - errorHeading: 'mockMessageFor: 0', + errorHeading: undefined, issueItems: [ { - html: 'mock task message 1 in record 1', - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1' + html: 'mockMessageFor: 0' } ], entry: { @@ -136,6 +136,7 @@ describe('issueDetails.middleware.js', () => { } const req = { params: requestParams, + parsedParams: { pageNumber: 1 }, // middleware supplies the below entryNumber: 1, entityCount: { entity_count: 3 }, @@ -146,6 +147,7 @@ describe('issueDetails.middleware.js', () => { entryData, issues, resource: { resource: requestParams.resourceId }, + entryNumberCount: 1, issuesByEntryNumber: { 1: [ {