From 75625848ece95d8a29d1fe9db06bbbfd77582968 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 11:42:48 +0000 Subject: [PATCH 01/33] update issue details to entity issue details --- src/controllers/OrganisationsController.js | 4 +- .../entityIssueDetails.middleware.js | 210 +++++++++++++ src/middleware/issueDetails.middleware.js | 295 ------------------ src/routes/organisations.js | 4 +- src/routes/schemas.js | 4 +- 5 files changed, 216 insertions(+), 301 deletions(-) create mode 100644 src/middleware/entityIssueDetails.middleware.js delete mode 100644 src/middleware/issueDetails.middleware.js diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index cb7dec68..dd8b5265 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -1,6 +1,6 @@ import datasetTaskListMiddleware from '../middleware/datasetTaskList.middleware.js' import datasetOverviewMiddleware from '../middleware/datasetOverview.middleware.js' -import issueDetailsMiddleware from '../middleware/issueDetails.middleware.js' +import entityIssueDetailsMiddleware from '../middleware/entityIssueDetails.middleware.js' import issueTableMiddleware from '../middleware/issueTable.middleware.js' import organisationsMiddleware from '../middleware/organisations.middleware.js' import getStartedMiddleware from '../middleware/getStarted.middleware.js' @@ -11,7 +11,7 @@ const organisationsController = { organisationsMiddleware, datasetTaskListMiddleware, datasetOverviewMiddleware, - issueDetailsMiddleware, + entityIssueDetailsMiddleware, issueTableMiddleware, getStartedMiddleware, overviewMiddleware, diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js new file mode 100644 index 00000000..2303dbc4 --- /dev/null +++ b/src/middleware/entityIssueDetails.middleware.js @@ -0,0 +1,210 @@ +import performanceDbApi from '../services/performanceDbApi.js' +import logger from '../utils/logger.js' +import { + fetchDatasetInfo, + fetchOrgInfo, + logPageError, + validateQueryParams, + createPaginationTemplateParams, + show404IfPageNumberNotInRange, + fetchResources, + processRelevantIssuesMiddlewares, + processEntitiesMiddlewares, + processSpecificationMiddlewares +} from './common.middleware.js' +import { renderTemplate } from './middleware.builders.js' +import * as v from 'valibot' + +export const IssueDetailsQueryParams = v.object({ + lpa: v.string(), + dataset: v.string(), + issue_type: v.string(), + issue_field: v.string(), + pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), + resourceId: v.optional(v.string()) +}) + +const validateIssueDetailsQueryParams = validateQueryParams({ + schema: IssueDetailsQueryParams +}) + +/** + * + * @param {string} errorMessage + * @param {{value: string}?} issue + * @returns {string} + */ +const issueErrorMessageHtml = (errorMessage, issue) => + `

${errorMessage}

${ + issue ? issue.value ?? '' : '' + }` + +/** + * + * @param {*} text + * @param {*} html + * @param {*} classes + * @returns {{key: {text: string}, value: { html: string}, classes: string}} + */ +const getIssueField = (text, html, classes) => { + return { + key: { + text + }, + value: { + html + }, + classes + } +} + +export const setBaseSubpath = (req, res, next) => { + const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params + req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entity` + next() +} + +export const getDataRange = (req, res, next) => { + const { pageNumber } = req.parsedParams + const { issues } = req + + const pageLength = 1 + const recordCount = issues.length + req.dataRange = { + minRow: (pageNumber - 1) * pageLength, + maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), + totalRows: recordCount, + maxPageNumber: recordCount, + pageLength: 1 + } + next() +} + +export function getErrorSummaryItems (req, res, next) { + const { issue_type: issueType, issue_field: issueField, baseSubpath } = req.params + + const { entities, issues } = req + + let errorHeading = '' + let issueItems + + if (issues.length <= 0) { + // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. + logger.warn(`issueTable was accessed from ${req.headers.referer} but there was no issues`) + const error = new Error('issue count must be larger than 0') + return next(error) + } else if (issues.length < entities.length) { + errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entities.length, field: issueField }, true) + issueItems = issues.map((issue, i) => { + const pageNum = i + 1 + return { + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in entity ${issue.entity}`, + href: `${baseSubpath}/${pageNum}` + } + }) + } else { + issueItems = [{ + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entities.length, field: issueField }, true) + }] + } + + req.errorSummary = { + heading: errorHeading, + items: issueItems + } + + next() +} + +/*** + * Middleware. Updates req with `templateParams` + */ +export function prepareIssueDetailsTemplateParams (req, res, next) { + const { entities, issues, pagination, errorSummary, dataRange } = req + const { issue_type: issueType } = req.params + const { pageNumber } = req.parsedParams + + const entityData = entities[pageNumber - 1] + const entityIssues = issues.filter(issue => issue.entity === entityData.entity) + + const fields = Object.entries(entityData).map(([field, value]) => ({ + key: { + text: field + }, + value: { + html: value ? value.toString() : '' + }, + classes: '' + })) + + entityIssues.forEach(issue => { + const field = fields.find(field => field.key.text === issue.field) + if (field) { + const message = issue.message || issue.type + field.value.html = issueErrorMessageHtml(message, null) + field.value.html + field.classes += 'dl-summary-card-list__row--error' + } + }) + + for (const issue of entityIssues) { + if (!fields.find((field) => field.key.text === issue.field)) { + const errorMessage = issue.message || issueType + // TODO: pull the html out of here and into the template + const valueHtml = issueErrorMessageHtml(errorMessage, issue.value) + const classes = 'dl-summary-card-list__row--error' + + fields.push(getIssueField(issue.field, valueHtml, classes)) + } + } + + const geometries = fields + .filter((row) => row.field === 'geometry') + .map((row) => row.value) + const entry = { + title: entityData.name || `entity: ${entityData.entity}`, + fields, + geometries + } + + // schema: OrgIssueDetails + req.templateParams = { + organisation: req.orgInfo, + dataset: req.dataset, + errorSummary, + entry, + issueType, + pagination, + pageNumber, + dataRange + } + + next() +} + +/** + * Middleware. Renders the issue details page with the list of issues, entry data, + * and organisation and dataset details. + */ +export const getIssueDetails = renderTemplate({ + templateParams: (req) => req.templateParams, + template: 'organisations/issueDetails.html', + handlerName: 'getIssueDetails' +}) + +export default [ + validateIssueDetailsQueryParams, + fetchOrgInfo, + fetchDatasetInfo, + fetchResources, + ...processRelevantIssuesMiddlewares, + ...processEntitiesMiddlewares, + ...processSpecificationMiddlewares, + getDataRange, + show404IfPageNumberNotInRange, + setBaseSubpath, + createPaginationTemplateParams, + getErrorSummaryItems, + prepareIssueDetailsTemplateParams, + getIssueDetails, + logPageError +] diff --git a/src/middleware/issueDetails.middleware.js b/src/middleware/issueDetails.middleware.js deleted file mode 100644 index ce9095f3..00000000 --- a/src/middleware/issueDetails.middleware.js +++ /dev/null @@ -1,295 +0,0 @@ -import performanceDbApi, { issuesQueryLimit } from '../services/performanceDbApi.js' -import { - fetchDatasetInfo, - fetchEntityCount, - fetchLatestResource, - fetchOrgInfo, - isResourceIdInParams, isResourceDataPresent, - logPageError, - takeResourceIdFromParams, - validateQueryParams, - createPaginationTemplateParams, - show404IfPageNumberNotInRange -} from './common.middleware.js' -import { fetchIf, fetchMany, fetchOne, FetchOptions, handleRejections, renderTemplate } from './middleware.builders.js' -import * as v from 'valibot' - -export const IssueDetailsQueryParams = v.object({ - lpa: v.string(), - dataset: v.string(), - issue_type: v.string(), - issue_field: v.string(), - pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), - resourceId: v.optional(v.string()) -}) - -const validateIssueDetailsQueryParams = validateQueryParams({ - schema: IssueDetailsQueryParams -}) - -/** - * Middleware. Updates `req` with `issues`. - * - * Requires resource id under `req.resource.resource`. - */ -const fetchIssues = fetchMany({ - query: ({ req }) => performanceDbApi.getIssuesQuery(req), - result: 'issues', - dataset: FetchOptions.fromParams -}) - -/** - * - * Middleware. Updates `req` with `issuesByEntryNumber` and `entryNumberCount`. - * - * Requires `issues` in request. - * - * @param {*} req - * @param {*} res - * @param {*} 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() -} - -/** - * - * Middleware. Updates `req` with `entryData` - * - * Requires `pageNumber`, `dataset`, `issuesByEntryNumber`, `resource` - * - * @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 } = req.params - const { issues, issueEntitiesCount, issuesByEntryNumber } = req - const { pageNumber } = req.parsedParams - - let entryData - const issuesByEntry = Object.values(issuesByEntryNumber) - if (issues.length > 0) { - if (issueEntitiesCount?.count && pageNumber <= issueEntitiesCount.count) { - 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 = entryData ?? [] - next() -} - -/** - * Middleware. Updates `req` with `issueEntitiesCount` which is the count of entities that have issues. - */ -const fetchIssueEntitiesCount = fetchOne({ - query: ({ req }) => performanceDbApi.getEntitiesWithIssuesCountQuery(req), - result: 'issueEntitiesCount', - dataset: FetchOptions.fromParams -}) - -/** - * - * @param {string} errorMessage - * @param {{value: string}?} issue - * @returns {string} - */ -const issueErrorMessageHtml = (errorMessage, issue) => - `

${errorMessage}

${ - issue ? issue.value ?? '' : '' - }` - -/** - * - * @param {*} text - * @param {*} html - * @param {*} classes - * @returns {{key: {text: string}, value: { html: string}, classes: string}} - */ -const getIssueField = (text, html, classes) => { - return { - key: { - text - }, - value: { - html - }, - classes - } -} - -/** - * - * @param {*} issueType - * @param {*} issuesByEntryNumber - * @param {*} row - * @returns {{key: {text: string}, value: { html: string}, classes: string}} - */ -const processEntryRow = (issueType, issuesByEntryNumber, row) => { - const { entry_number: entryNumber } = row - console.assert(entryNumber, 'precessEntryRow(): entry_number not in row') - let hasError = false - let issueIndex - if (issuesByEntryNumber[entryNumber]) { - issueIndex = issuesByEntryNumber[entryNumber].findIndex( - (issue) => issue.field === row.field - ) - hasError = issueIndex >= 0 - } - - let valueHtml = '' - let classes = '' - if (hasError) { - const message = - issuesByEntryNumber[entryNumber][issueIndex].message || issueType - valueHtml += issueErrorMessageHtml(message, null) - classes += 'dl-summary-card-list__row--error' - } - valueHtml += row.value - - return getIssueField(row.field, valueHtml, classes) -} - -export const setBaseSubpath = (req, res, next) => { - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params - req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry` - next() -} - -export const getDataRange = (req, res, next) => { - const { pageNumber } = req.parsedParams - const { issueEntitiesCount: issueEntitiesCountObj } = req - const { count: issueEntitiesCount } = issueEntitiesCountObj - - const pageLength = 1 - const recordCount = issueEntitiesCount - req.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), - totalRows: recordCount, - maxPageNumber: recordCount, - pageLength: 1 - } - next() -} - -/*** - * Middleware. Updates req with `templateParams` - */ -export function prepareIssueDetailsTemplateParams (req, res, next) { - const { entryData, issueEntitiesCount: issueEntitiesCountObj, issuesByEntryNumber, entryNumberCount, entityCount: entityCountRow, pagination, baseSubpath } = req - const { count: issueEntitiesCount } = issueEntitiesCountObj - const { issue_type: issueType, issue_field: issueField } = req.params - const { pageNumber } = req.parsedParams - const { entity_count: entityCount } = entityCountRow ?? { entity_count: 0 } - - let errorHeading - let issueItems - - 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 - return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in record ${entryNumber}`, - href: `${baseSubpath}/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issueEntitiesCount, entityCount, field: issueField }, true) - }] - } - - const fields = entryData.map((row) => processEntryRow(issueType, issuesByEntryNumber, row)) - const entityIssues = Object.values(issuesByEntryNumber)[pageNumber - 1] || [] - for (const issue of entityIssues) { - if (!fields.find((field) => field.key.text === issue.field)) { - const errorMessage = issue.message || issueType - // TODO: pull the html out of here and into the template - const valueHtml = issueErrorMessageHtml(errorMessage, issue.value) - const classes = 'dl-summary-card-list__row--error' - - fields.push(getIssueField(issue.field, valueHtml, classes)) - } - } - - const geometries = entryData - .filter((row) => row.field === 'geometry') - .map((row) => row.value) - const entry = { - title: `entry: ${entryData.length > 0 ? entryData[0].entry_number : ''}`, - fields, - geometries - } - - // schema: OrgIssueDetails - req.templateParams = { - organisation: req.orgInfo, - dataset: req.dataset, - errorSummary: { - heading: errorHeading, - items: issueItems - }, - entry, - issueType, - pagination, - issueEntitiesCount, - pageNumber - } - - next() -} - -/** - * Middleware. Renders the issue details page with the list of issues, entry data, - * and organisation and dataset details. - */ -export const getIssueDetails = renderTemplate({ - templateParams: (req) => req.templateParams, - template: 'organisations/issueDetails.html', - handlerName: 'getIssueDetails' -}) - -/* eslint-disable no-return-assign */ -const zeroEntityCount = (req) => req.entityCount = { entity_count: 0 } -const zeroIssueEntitiesCount = (req) => req.issueEntitiesCount = { count: 0 } -const emptyIssuesCollection = (req) => req.issues = [] - -export default [ - validateIssueDetailsQueryParams, - fetchOrgInfo, - fetchDatasetInfo, - fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), - fetchIf(isResourceDataPresent, fetchIssues, emptyIssuesCollection), - reformatIssuesToBeByEntryNumber, - fetchIf(isResourceDataPresent, fetchIssueEntitiesCount, zeroIssueEntitiesCount), - handleRejections(fetchEntry), - fetchIf(isResourceDataPresent, fetchEntityCount, zeroEntityCount), - setBaseSubpath, - getDataRange, - show404IfPageNumberNotInRange, - createPaginationTemplateParams, - prepareIssueDetailsTemplateParams, - getIssueDetails, - logPageError -] diff --git a/src/routes/organisations.js b/src/routes/organisations.js index 2995d438..d37371f6 100644 --- a/src/routes/organisations.js +++ b/src/routes/organisations.js @@ -7,8 +7,8 @@ router.get('/:lpa/:dataset/get-started', OrganisationsController.getStartedMiddl router.get('/:lpa/:dataset/overview', OrganisationsController.datasetOverviewMiddleware) router.get('/:lpa/:dataset/data/:pageNumber', OrganisationsController.datasetDataviewMiddleware) router.get('/:lpa/:dataset/data', OrganisationsController.datasetDataviewMiddleware) -router.get('/:lpa/:dataset/:issue_type/:issue_field/entry/:pageNumber', OrganisationsController.issueDetailsMiddleware) -router.get('/:lpa/:dataset/:issue_type/:issue_field/entry', OrganisationsController.issueDetailsMiddleware) +router.get('/:lpa/:dataset/:issue_type/:issue_field/entity/:pageNumber', OrganisationsController.entityIssueDetailsMiddleware) +router.get('/:lpa/:dataset/:issue_type/:issue_field/entity', OrganisationsController.entityIssueDetailsMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field/:pageNumber?', OrganisationsController.issueTableMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field', OrganisationsController.issueTableMiddleware) router.get('/:lpa/:dataset', OrganisationsController.datasetTaskListMiddleware) diff --git a/src/routes/schemas.js b/src/routes/schemas.js index 0ac5554e..77447993 100644 --- a/src/routes/schemas.js +++ b/src/routes/schemas.js @@ -229,8 +229,8 @@ export const OrgIssueDetails = v.strictObject({ geometries: v.optional(v.array(v.string())) }), pagination: PaginationParams, - issueEntitiesCount: v.integer(), - pageNumber: v.integer() + pageNumber: v.integer(), + dataRange: dataRangeParams }) export const CheckAnswers = v.strictObject({ From 1ab09ee062391da7ffa0305da1f24dc36f7e40c8 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 11:44:58 +0000 Subject: [PATCH 02/33] add entryIssueDetailsMiddleware and route --- src/controllers/OrganisationsController.js | 2 ++ src/middleware/entryIssueDetails.middleware.js | 0 src/routes/organisations.js | 2 ++ 3 files changed, 4 insertions(+) create mode 100644 src/middleware/entryIssueDetails.middleware.js diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index dd8b5265..0deeff7a 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -1,6 +1,7 @@ import datasetTaskListMiddleware from '../middleware/datasetTaskList.middleware.js' import datasetOverviewMiddleware from '../middleware/datasetOverview.middleware.js' import entityIssueDetailsMiddleware from '../middleware/entityIssueDetails.middleware.js' +import entryIssueDetailsMiddleware from '../middleware/entryIssueDetails.middleware.js' import issueTableMiddleware from '../middleware/issueTable.middleware.js' import organisationsMiddleware from '../middleware/organisations.middleware.js' import getStartedMiddleware from '../middleware/getStarted.middleware.js' @@ -12,6 +13,7 @@ const organisationsController = { datasetTaskListMiddleware, datasetOverviewMiddleware, entityIssueDetailsMiddleware, + entryIssueDetailsMiddleware, issueTableMiddleware, getStartedMiddleware, overviewMiddleware, diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js new file mode 100644 index 00000000..e69de29b diff --git a/src/routes/organisations.js b/src/routes/organisations.js index d37371f6..62b18553 100644 --- a/src/routes/organisations.js +++ b/src/routes/organisations.js @@ -9,6 +9,8 @@ router.get('/:lpa/:dataset/data/:pageNumber', OrganisationsController.datasetDat router.get('/:lpa/:dataset/data', OrganisationsController.datasetDataviewMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field/entity/:pageNumber', OrganisationsController.entityIssueDetailsMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field/entity', OrganisationsController.entityIssueDetailsMiddleware) +router.get('/:lpa/:dataset/:issue_type/:issue_field/entry/:pageNumber', OrganisationsController.entryIssueDetailsMiddleware) +router.get('/:lpa/:dataset/:issue_type/:issue_field/entry', OrganisationsController.entryIssueDetailsMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field/:pageNumber?', OrganisationsController.issueTableMiddleware) router.get('/:lpa/:dataset/:issue_type/:issue_field', OrganisationsController.issueTableMiddleware) router.get('/:lpa/:dataset', OrganisationsController.datasetTaskListMiddleware) From 56111d39bb9c27bc899460b123d61e17b27112be Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 13:07:58 +0000 Subject: [PATCH 03/33] Updated entity and entry issue details middleware: refactored code, renamed files, and added new functionality to improve performance and correctness --- src/middleware/common.middleware.js | 3 +- .../entityIssueDetails.middleware.js | 33 ++- .../entryIssueDetails.middleware.js | 207 ++++++++++++++++++ src/middleware/issueTable.middleware.js | 5 +- 4 files changed, 232 insertions(+), 16 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 460aceb8..8d563512 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -187,9 +187,10 @@ export const createPaginationTemplateParams = (req, res, next) => { export const fetchResources = fetchMany({ query: ({ req }) => ` - select * from resource r + 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 LEFT JOIN resource_organisation ro ON ro.resource = r.resource LEFT JOIN resource_dataset rd ON rd.resource = r.resource + LEFT JOIN reporting_latest_endpoints rle ON r.resource = rle.resource WHERE ro.organisation = '${req.params.lpa}' AND rd.dataset = '${req.params.dataset}' AND r.end_date = '' diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 2303dbc4..e3701579 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -90,7 +90,7 @@ export function getErrorSummaryItems (req, res, next) { if (issues.length <= 0) { // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`issueTable was accessed from ${req.headers.referer} but there was no issues`) + logger.warn(`entity issue details was accessed from ${req.headers.referer} but there was no issues`) const error = new Error('issue count must be larger than 0') return next(error) } else if (issues.length < entities.length) { @@ -116,13 +116,9 @@ export function getErrorSummaryItems (req, res, next) { next() } -/*** - * Middleware. Updates req with `templateParams` - */ -export function prepareIssueDetailsTemplateParams (req, res, next) { - const { entities, issues, pagination, errorSummary, dataRange } = req - const { issue_type: issueType } = req.params - const { pageNumber } = req.parsedParams +export function prepareEntity (req, res, next) { + const { entities, issues } = req + const { pageNumber, issue_type: issueType } = req.parsedParams const entityData = entities[pageNumber - 1] const entityIssues = issues.filter(issue => issue.entity === entityData.entity) @@ -160,16 +156,28 @@ export function prepareIssueDetailsTemplateParams (req, res, next) { const geometries = fields .filter((row) => row.field === 'geometry') .map((row) => row.value) - const entry = { + + req.entry = { title: entityData.name || `entity: ${entityData.entity}`, fields, geometries } + next() +} + +/*** + * Middleware. Updates req with `templateParams` + */ +export function prepareEntityIssueDetailsTemplateParams (req, res, next) { + const { entry, pagination, errorSummary, dataRange, dataset, orgInfo } = req + const { issue_type: issueType } = req.params + const { pageNumber } = req.parsedParams + // schema: OrgIssueDetails req.templateParams = { - organisation: req.orgInfo, - dataset: req.dataset, + organisation: orgInfo, + dataset, errorSummary, entry, issueType, @@ -204,7 +212,8 @@ export default [ setBaseSubpath, createPaginationTemplateParams, getErrorSummaryItems, - prepareIssueDetailsTemplateParams, + prepareEntity, + prepareEntityIssueDetailsTemplateParams, getIssueDetails, logPageError ] diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index e69de29b..7627906f 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -0,0 +1,207 @@ +import * as v from 'valibot' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' +import logger from '../utils/logger.js' +import performanceDbApi from '../services/performanceDbApi.js' + +export const IssueDetailsQueryParams = v.object({ + lpa: v.string(), + dataset: v.string(), + issue_type: v.string(), + issue_field: v.string(), + pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), + resourceId: v.optional(v.string()) +}) + +const validateIssueDetailsQueryParams = validateQueryParams({ + schema: IssueDetailsQueryParams +}) + +const fetchResourceMetaData = fetchMany({ + query: ({ req }) => ` + select entity_count, entry_count, line_count, mime_type, internal_path, internal_mime_type, resource + FROM dataset_resource + WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') + `, + dataset: FetchOptions.fromParams, + result: 'resourceMetaData' +}) + +const addResourceMetaDataToResources = (req, res, next) => { + const { resources, resourceMetaData } = req + + req.resources = resources.map(resource => { + const metaData = resourceMetaData.find(metaData => metaData.resource === resource.resource) + if (metaData) { + return { ...resource, ...resourceMetaData } + } + return resource + }) + + 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 + WHERE resource = '${req.resources[0].resource}' + AND issue_type = '${params.issue_type}' + AND field = '${params.issue_field}' + AND (entity is null or entity = '') + `, + dataset: FetchOptions.fromParams, + result: 'issues' +}) + +export const getDataRange = (req, res, next) => { + const { pageNumber } = req.parsedParams + const { issues } = req + + const pageLength = 1 + const recordCount = issues.length + req.dataRange = { + minRow: (pageNumber - 1) * pageLength, + maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), + totalRows: recordCount, + maxPageNumber: recordCount, + pageLength: 1 + } + next() +} + +export const setBaseSubpath = (req, res, next) => { + const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params + req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry` + next() +} + +export function getErrorSummaryItems (req, res, next) { + const { issue_type: issueType, issue_field: issueField } = req.params + const { baseSubpath, resources } = req + + const { issues } = req + + const entryCount = resources[0].entry_count + + let errorHeading = '' + let issueItems + + if (issues.length <= 0) { + // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. + logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) + const error = new Error('issue count must be larger than 0') + return next(error) + } else if (issues.length < entryCount) { + errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entryCount, field: issueField }, true) + issueItems = issues.map((issue, i) => { + const pageNum = i + 1 + return { + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in entity ${issue.entity}`, + href: `${baseSubpath}/${pageNum}` + } + }) + } else { + issueItems = [{ + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entryCount, field: issueField }, true) + }] + } + + req.errorSummary = { + heading: errorHeading, + items: issueItems + } + + next() +} + +// what we show in the table +// - resource +// - line number +// - reference + +const prepareEntry = (req, res, next) => { + const { resources, issues } = req + const { pageNumber } = req.parsedParams + + const issue = issues[pageNumber - 1] + + req.entry = { + title: `entry: ${issue.entry_number}`, + fields: [ + { + key: { + text: 'Endpoint' + }, + value: { + html: `${resources[0].endpoint_url}` + }, + classes: '' + }, + { + key: { + text: 'Line number' + }, + value: { + html: issue.line_number.toString() + }, + classes: '' + }, + { + key: { + text: issue.field + }, + value: { + html: `

${issue.message || issue.issue_type}

${issue.value}` + }, + classes: '' + } + ] + } + next() +} + +const prepareEntryIssueDetailsTemplateParams = (req, res, next) => { + const { issue_type: issueType, pageNumber } = req.parsedParams + const { entry, pagination, dataRange, errorSummary, dataset, orgInfo } = req + + // schema: OrgIssueDetails + req.templateParams = { + organisation: orgInfo, + dataset, + errorSummary, + entry, + issueType, + pagination, + pageNumber, + dataRange + } + + next() +} + +export const getIssueDetails = renderTemplate({ + templateParams: (req) => req.templateParams, + template: 'organisations/issueDetails.html', + handlerName: 'getIssueDetails' +}) + +export default [ + validateIssueDetailsQueryParams, + fetchOrgInfo, + fetchDatasetInfo, + fetchResources, + fetchResourceMetaData, + addResourceMetaDataToResources, + fetchEntryIssues, + getDataRange, + show404IfPageNumberNotInRange, + setBaseSubpath, + createPaginationTemplateParams, + getErrorSummaryItems, + prepareEntry, + prepareEntryIssueDetailsTemplateParams, + getIssueDetails + // logPageError +] diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index 65c6c8cd..e80bff1a 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -48,15 +48,14 @@ export const setBaseSubpath = (req, res, next) => { } export const prepareTableParams = (req, res, next) => { - const { entities, issues, uniqueDatasetFields, dataRange } = req - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params + const { entities, issues, uniqueDatasetFields, dataRange, baseSubpath } = req const allRows = entities.map((entity, index) => ({ columns: Object.fromEntries(uniqueDatasetFields.map((field) => { const errorMessage = issues.find(issue => issue.entity === entity.entity && (issue.field === field || issue.replacement_field === field))?.issue_type if (field === 'reference') { return [field, { - html: `${entity[field]}`, + html: `${entity[field]}`, error: errorMessage ? { message: errorMessage From 2b017ddf6565454da3f5a943d4fcae35db83a610 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 14:55:19 +0000 Subject: [PATCH 04/33] dont filter out issues without an entity, and also only get issues of responsibility external --- src/middleware/entryIssueDetails.middleware.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 7627906f..a0489843 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -48,8 +48,9 @@ const fetchEntryIssues = fetchMany({ from issue WHERE resource = '${req.resources[0].resource}' AND issue_type = '${params.issue_type}' + AND it.responsibility = 'external' AND field = '${params.issue_field}' - AND (entity is null or entity = '') + AND issue `, dataset: FetchOptions.fromParams, result: 'issues' From d92501a21d7c3cc81ca54f12c7b6a9a2625ae2a0 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 15:01:08 +0000 Subject: [PATCH 05/33] only output spec fields --- .../entityIssueDetails.middleware.js | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index e3701579..a8d474f3 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -117,21 +117,25 @@ export function getErrorSummaryItems (req, res, next) { } export function prepareEntity (req, res, next) { - const { entities, issues } = req + const { entities, issues, specification } = req const { pageNumber, issue_type: issueType } = req.parsedParams const entityData = entities[pageNumber - 1] const entityIssues = issues.filter(issue => issue.entity === entityData.entity) - const fields = Object.entries(entityData).map(([field, value]) => ({ - key: { - text: field - }, - value: { - html: value ? value.toString() : '' - }, - classes: '' - })) + const fields = specification.fields.map(({ field, datasetField }) => { + const value = entityData[field] || entityData[datasetField] + + return { + key: { + text: field + }, + value: { + html: value ? value.toString() : '' + }, + classes: '' + } + }) entityIssues.forEach(issue => { const field = fields.find(field => field.key.text === issue.field) From 03238bd21a840639baecef26aa02f9766430fd13 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 16:20:40 +0000 Subject: [PATCH 06/33] fix query --- src/middleware/entryIssueDetails.middleware.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index a0489843..25f5bc1d 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -45,12 +45,12 @@ const addResourceMetaDataToResources = (req, res, next) => { const fetchEntryIssues = fetchMany({ query: ({ req, params }) => ` select * - from issue + from issue i + LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' AND issue_type = '${params.issue_type}' AND it.responsibility = 'external' AND field = '${params.issue_field}' - AND issue `, dataset: FetchOptions.fromParams, result: 'issues' From cd403bfe43b144c158ec0c6ec5fdd8f36ffe2f50 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 16:39:42 +0000 Subject: [PATCH 07/33] use already set variable --- src/middleware/entityIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index a8d474f3..eb3ecd4e 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -75,7 +75,7 @@ export const getDataRange = (req, res, next) => { maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), totalRows: recordCount, maxPageNumber: recordCount, - pageLength: 1 + pageLength } next() } From 51f89c927bb390212d80e76a1979f38c457ba178 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Fri, 29 Nov 2024 16:46:20 +0000 Subject: [PATCH 08/33] remove comment --- src/middleware/entryIssueDetails.middleware.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 25f5bc1d..71d152ee 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -204,5 +204,4 @@ export default [ prepareEntry, prepareEntryIssueDetailsTemplateParams, getIssueDetails - // logPageError ] From 3b37099af01060822ee9e3447e2f5e38493e1c25 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 2 Dec 2024 11:55:45 +0000 Subject: [PATCH 09/33] moduleize getBaseSubPath --- src/middleware/common.middleware.js | 16 ++++++++++++++++ src/middleware/dataview.middleware.js | 11 +++-------- src/middleware/entityIssueDetails.middleware.js | 11 +++-------- src/middleware/entryIssueDetails.middleware.js | 13 +++---------- src/middleware/issueTable.middleware.js | 10 ++-------- 5 files changed, 27 insertions(+), 34 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 8d563512..d4c0ec16 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -526,3 +526,19 @@ export const setDefaultParams = (req, res, next) => { next() } + +export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => { + const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params + + const additionalPartsEncoded = additionalParts.map(part => encodeURIComponent(part)) + + let path = '/organisations' + if (lpa) path += `/${lpa}` + if (dataset) path += `/${dataset}` + if (issueType) path += `/${issueType}` + if (issueField) path += `/${issueField}` + if (additionalPartsEncoded.length > 0) path += `/${additionalPartsEncoded.join('/')}` + + req.baseSubpath = path + next() +} diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index 56be8902..0f4ca243 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -1,4 +1,4 @@ -import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange } from './common.middleware.js' +import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath } from './common.middleware.js' import { fetchResourceStatus } from './datasetTaskList.middleware.js' import { fetchIf, fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -26,15 +26,10 @@ export const fetchEntities = fetchMany({ result: 'entities' }) -export const setBaseSubpath = (req, res, next) => { - const { lpa, dataset } = req.params - req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/data` - next() -} - export const getDataRange = (req, res, next) => { const { entityCount } = req const { pageNumber } = req.parsedParams + const pageLength = 50 const recordCount = entityCount.count req.dataRange = { @@ -118,7 +113,7 @@ export default [ validatedataviewQueryParams, setDefaultParams, - setBaseSubpath, + getSetBaseSubPath(['data']), fetchOrgInfo, fetchDatasetInfo, fetchResourceStatus, diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index eb3ecd4e..0182060e 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -10,7 +10,8 @@ import { fetchResources, processRelevantIssuesMiddlewares, processEntitiesMiddlewares, - processSpecificationMiddlewares + processSpecificationMiddlewares, + getSetBaseSubPath } from './common.middleware.js' import { renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -58,12 +59,6 @@ const getIssueField = (text, html, classes) => { } } -export const setBaseSubpath = (req, res, next) => { - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params - req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entity` - next() -} - export const getDataRange = (req, res, next) => { const { pageNumber } = req.parsedParams const { issues } = req @@ -213,7 +208,7 @@ export default [ ...processSpecificationMiddlewares, getDataRange, show404IfPageNumberNotInRange, - setBaseSubpath, + getSetBaseSubPath(['entity']), createPaginationTemplateParams, getErrorSummaryItems, prepareEntity, diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 71d152ee..4ba781d7 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, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' import logger from '../utils/logger.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -48,11 +48,10 @@ const fetchEntryIssues = fetchMany({ from issue i LEFT JOIN issue_type it ON i.issue_type = it.issue_type WHERE resource = '${req.resources[0].resource}' - AND issue_type = '${params.issue_type}' + AND i.issue_type = '${params.issue_type}' AND it.responsibility = 'external' AND field = '${params.issue_field}' `, - dataset: FetchOptions.fromParams, result: 'issues' }) @@ -72,12 +71,6 @@ export const getDataRange = (req, res, next) => { next() } -export const setBaseSubpath = (req, res, next) => { - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params - req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry` - next() -} - export function getErrorSummaryItems (req, res, next) { const { issue_type: issueType, issue_field: issueField } = req.params const { baseSubpath, resources } = req @@ -198,7 +191,7 @@ export default [ fetchEntryIssues, getDataRange, show404IfPageNumberNotInRange, - setBaseSubpath, + getSetBaseSubPath(['entry']), createPaginationTemplateParams, getErrorSummaryItems, prepareEntry, diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index e80bff1a..b2f4b129 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -9,7 +9,7 @@ import config from '../../config/index.js' import performanceDbApi from '../services/performanceDbApi.js' import logger from '../utils/logger.js' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { onlyIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -41,12 +41,6 @@ export const getDataRange = (req, res, next) => { next() } -export const setBaseSubpath = (req, res, next) => { - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params - req.baseSubpath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}` - next() -} - export const prepareTableParams = (req, res, next) => { const { entities, issues, uniqueDatasetFields, dataRange, baseSubpath } = req @@ -203,7 +197,7 @@ export default [ onlyIf(notIssueHasEntity, redirectToEntityView), getDataRange, show404IfPageNumberNotInRange, - setBaseSubpath, + getSetBaseSubPath(), getErrorSummaryItems, createPaginationTemplateParams, prepareTableParams, From 0c05c9bd4c5fe23c23f7fe900542906ebe368133 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 2 Dec 2024 12:12:20 +0000 Subject: [PATCH 10/33] modulized setDataRange --- src/middleware/common.middleware.js | 15 +++++++++++++ src/middleware/dataview.middleware.js | 22 +++++-------------- .../entityIssueDetails.middleware.js | 22 ++++++------------- .../entryIssueDetails.middleware.js | 20 +++++------------ src/middleware/issueTable.middleware.js | 19 +++++----------- 5 files changed, 38 insertions(+), 60 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index d4c0ec16..b80a2020 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -542,3 +542,18 @@ export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => { req.baseSubpath = path next() } + +export const getSetDataRange = (pageLength) => (req, res, next) => { + const { recordCount } = req + const { pageNumber } = req.parsedParams + + req.dataRange = { + minRow: (pageNumber - 1) * pageLength, + maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), + totalRows: recordCount, + maxPageNumber: Math.ceil(recordCount / pageLength), + pageLength, + offset: (pageNumber - 1) * pageLength + } + next() +} diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index 0f4ca243..b2559ecb 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -1,4 +1,5 @@ -import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath } from './common.middleware.js' +import config from '../../config/index.js' +import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange, getSetBaseSubPath, getSetDataRange } from './common.middleware.js' import { fetchResourceStatus } from './datasetTaskList.middleware.js' import { fetchIf, fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -26,20 +27,8 @@ export const fetchEntities = fetchMany({ result: 'entities' }) -export const getDataRange = (req, res, next) => { - const { entityCount } = req - const { pageNumber } = req.parsedParams - - const pageLength = 50 - const recordCount = entityCount.count - req.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), - totalRows: recordCount, - maxPageNumber: Math.ceil(recordCount / pageLength), - pageLength, - offset: (pageNumber - 1) * pageLength - } +export const setRecordCount = (req, res, next) => { + req.recordCount = req.entityCount.count next() } @@ -118,7 +107,8 @@ export default [ fetchDatasetInfo, fetchResourceStatus, fetchEntitiesCount, - getDataRange, + setRecordCount, + getSetDataRange(config.tablePageLength), show404IfPageNumberNotInRange, fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams), diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 0182060e..eceec481 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -11,7 +11,8 @@ import { processRelevantIssuesMiddlewares, processEntitiesMiddlewares, processSpecificationMiddlewares, - getSetBaseSubPath + getSetBaseSubPath, + getSetDataRange } from './common.middleware.js' import { renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -59,19 +60,8 @@ const getIssueField = (text, html, classes) => { } } -export const getDataRange = (req, res, next) => { - const { pageNumber } = req.parsedParams - const { issues } = req - - const pageLength = 1 - const recordCount = issues.length - req.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), - totalRows: recordCount, - maxPageNumber: recordCount, - pageLength - } +export const setRecordCount = (req, res, next) => { + req.recordCount = req.issues.length next() } @@ -206,8 +196,10 @@ export default [ ...processRelevantIssuesMiddlewares, ...processEntitiesMiddlewares, ...processSpecificationMiddlewares, - getDataRange, + setRecordCount, + getSetDataRange(1), show404IfPageNumberNotInRange, + setRecordCount, getSetBaseSubPath(['entity']), createPaginationTemplateParams, getErrorSummaryItems, diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 4ba781d7..b5316505 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, getSetBaseSubPath, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, getSetDataRange, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' import logger from '../utils/logger.js' import performanceDbApi from '../services/performanceDbApi.js' @@ -55,19 +55,8 @@ const fetchEntryIssues = fetchMany({ result: 'issues' }) -export const getDataRange = (req, res, next) => { - const { pageNumber } = req.parsedParams - const { issues } = req - - const pageLength = 1 - const recordCount = issues.length - req.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), - totalRows: recordCount, - maxPageNumber: recordCount, - pageLength: 1 - } +export const setRecordCount = (req, res, next) => { + req.recordCount = req.issues.length next() } @@ -189,7 +178,8 @@ export default [ fetchResourceMetaData, addResourceMetaDataToResources, fetchEntryIssues, - getDataRange, + setRecordCount, + getSetDataRange(1), show404IfPageNumberNotInRange, getSetBaseSubPath(['entry']), createPaginationTemplateParams, diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index b2f4b129..a4e303a5 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -9,7 +9,7 @@ import config from '../../config/index.js' import performanceDbApi from '../services/performanceDbApi.js' import logger from '../utils/logger.js' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { onlyIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -26,18 +26,8 @@ const validateIssueTableQueryParams = validateQueryParams({ schema: IssueTableQueryParams }) -export const getDataRange = (req, res, next) => { - const { issues } = req - const { pageNumber } = req.parsedParams - const pageLength = config.tablePageLength - const recordCount = issues.length - req.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), - totalRows: recordCount, - maxPageNumber: Math.max(1, Math.ceil(recordCount / pageLength)), - pageLength - } +export const setRecordCount = (req, res, next) => { + req.recordCount = req.issues.length next() } @@ -195,7 +185,8 @@ export default [ ...processEntitiesMiddlewares, ...processSpecificationMiddlewares, onlyIf(notIssueHasEntity, redirectToEntityView), - getDataRange, + setRecordCount, + getSetDataRange(config.tablePageLength), show404IfPageNumberNotInRange, getSetBaseSubPath(), getErrorSummaryItems, From d5969d03f0de6c99e4bb456d33ab066494dea70d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 2 Dec 2024 12:34:08 +0000 Subject: [PATCH 11/33] modulize getErrorSummaryItems --- src/middleware/common.middleware.js | 37 ++++++++++++ .../entityIssueDetails.middleware.js | 43 +------------- .../entryIssueDetails.middleware.js | 49 +--------------- src/middleware/issueTable.middleware.js | 56 +------------------ 4 files changed, 41 insertions(+), 144 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index b80a2020..8ea5658c 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -557,3 +557,40 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { } next() } + +export function getErrorSummaryItems (req, res, next) { + const { issue_type: issueType, issue_field: issueField } = req.params + const { baseSubpath, issues, entities, resources } = req + + let errorHeading = '' + let issueItems + + const totalRecordCount = entities ? entities.length : resources[0].entry_count + + if (issues.length <= 0) { + // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. + logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) + 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: issues.length, entityCount: totalRecordCount, field: issueField }, true) + issueItems = issues.map((issue, i) => { + const pageNum = i + 1 + return { + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }), // ToDo: + ` in entity ${issue.entity}`, + href: `${baseSubpath}/${pageNum}` + } + }) + } else { + issueItems = [{ + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: totalRecordCount, field: issueField }, true) + }] + } + + req.errorSummary = { + heading: errorHeading, + items: issueItems + } + + next() +} diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index eceec481..c54b7325 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -1,5 +1,3 @@ -import performanceDbApi from '../services/performanceDbApi.js' -import logger from '../utils/logger.js' import { fetchDatasetInfo, fetchOrgInfo, @@ -12,7 +10,8 @@ import { processEntitiesMiddlewares, processSpecificationMiddlewares, getSetBaseSubPath, - getSetDataRange + getSetDataRange, + getErrorSummaryItems } from './common.middleware.js' import { renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -64,43 +63,6 @@ export const setRecordCount = (req, res, next) => { req.recordCount = req.issues.length next() } - -export function getErrorSummaryItems (req, res, next) { - const { issue_type: issueType, issue_field: issueField, baseSubpath } = req.params - - const { entities, issues } = req - - let errorHeading = '' - let issueItems - - if (issues.length <= 0) { - // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`entity issue details was accessed from ${req.headers.referer} but there was no issues`) - const error = new Error('issue count must be larger than 0') - return next(error) - } else if (issues.length < entities.length) { - errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entities.length, field: issueField }, true) - issueItems = issues.map((issue, i) => { - const pageNum = i + 1 - return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in entity ${issue.entity}`, - href: `${baseSubpath}/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entities.length, field: issueField }, true) - }] - } - - req.errorSummary = { - heading: errorHeading, - items: issueItems - } - - next() -} - export function prepareEntity (req, res, next) { const { entities, issues, specification } = req const { pageNumber, issue_type: issueType } = req.parsedParams @@ -199,7 +161,6 @@ export default [ setRecordCount, getSetDataRange(1), show404IfPageNumberNotInRange, - setRecordCount, getSetBaseSubPath(['entity']), createPaginationTemplateParams, getErrorSummaryItems, diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index b5316505..3b10fbe7 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,8 +1,6 @@ import * as v from 'valibot' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, getSetDataRange, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' -import logger from '../utils/logger.js' -import performanceDbApi from '../services/performanceDbApi.js' export const IssueDetailsQueryParams = v.object({ lpa: v.string(), @@ -59,51 +57,6 @@ export const setRecordCount = (req, res, next) => { req.recordCount = req.issues.length next() } - -export function getErrorSummaryItems (req, res, next) { - const { issue_type: issueType, issue_field: issueField } = req.params - const { baseSubpath, resources } = req - - const { issues } = req - - const entryCount = resources[0].entry_count - - let errorHeading = '' - let issueItems - - if (issues.length <= 0) { - // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) - const error = new Error('issue count must be larger than 0') - return next(error) - } else if (issues.length < entryCount) { - errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entryCount, field: issueField }, true) - issueItems = issues.map((issue, i) => { - const pageNum = i + 1 - return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + ` in entity ${issue.entity}`, - href: `${baseSubpath}/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: entryCount, field: issueField }, true) - }] - } - - req.errorSummary = { - heading: errorHeading, - items: issueItems - } - - next() -} - -// what we show in the table -// - resource -// - line number -// - reference - const prepareEntry = (req, res, next) => { const { resources, issues } = req const { pageNumber } = req.parsedParams diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index a4e303a5..3100eb14 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -7,9 +7,7 @@ */ import config from '../../config/index.js' -import performanceDbApi from '../services/performanceDbApi.js' -import logger from '../utils/logger.js' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { onlyIf, renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -74,58 +72,6 @@ export const prepareTableParams = (req, res, next) => { next() } -export const getErrorSummaryItems = (req, res, next) => { - const { issue_type: issueType, issue_field: issueField, baseSubpath } = req.params - - const { entities, issues } = req - - let errorHeading = '' - let issueItems - - if (issues.length <= 0) { - // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`issueTable was accessed from ${req.headers.referer} but there was no issues`) - const error = new Error('issue count must be larger than 0') - return next(error) - } else if (issues.length < entities.length) { - errorHeading = performanceDbApi.getTaskMessage({ - issue_type: issueType, - num_issues: issues.length, - entityCount: entities.length, - field: issueField - }, - true) - issueItems = issues.map((issue, i) => { - const pageNum = i + 1 - return { - html: performanceDbApi.getTaskMessage({ - issue_type: issueType, - num_issues: 1, - field: issueField - }) + ` in entity ${issue.entity}`, - href: `${baseSubpath}/entity/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ - issue_type: issueType, - num_issues: issues.length, - entityCount: entities.length, - field: - issueField - }, true) - }] - } - - req.errorSummary = { - heading: errorHeading, - items: issueItems - } - - next() -} - export const prepareTemplateParams = (req, res, next) => { const { tableParams, orgInfo, dataset, errorSummary, pagination, dataRange } = req const { issue_type: issueType } = req.params From 8e1409933c5a0676c32f4151122daccd817d0ec5 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Mon, 2 Dec 2024 13:04:17 +0000 Subject: [PATCH 12/33] fixed tests and removed not needed tests --- .../middleware/dataview.middleware.test.js | 45 --- .../issueDetails.middleware.test.js | 319 ------------------ .../middleware/issueTable.middleware.test.js | 212 +----------- .../organisations/issueDetailsPage.test.js | 5 +- 4 files changed, 7 insertions(+), 574 deletions(-) delete mode 100644 test/unit/middleware/issueDetails.middleware.test.js diff --git a/test/unit/middleware/dataview.middleware.test.js b/test/unit/middleware/dataview.middleware.test.js index 5478a5bb..a34aad3b 100644 --- a/test/unit/middleware/dataview.middleware.test.js +++ b/test/unit/middleware/dataview.middleware.test.js @@ -1,7 +1,6 @@ import { describe, it, expect, vi } from 'vitest' import { constructTableParams, - getDataRange, prepareTemplateParams } from '../../../src/middleware/dataview.middleware' @@ -113,50 +112,6 @@ describe('dataview.middleware.test.js', () => { }) }) - describe('getDataRange', () => { - it('should set up correct dataRange properties when pageNumber is 1', () => { - const req = { - entityCount: { count: 10 }, - parsedParams: { pageNumber: 1 } - } - const res = {} - const next = vi.fn() - - getDataRange(req, res, next) - - expect(req.dataRange).toEqual({ - minRow: 0, - maxRow: 10, - totalRows: 10, - pageLength: 50, - offset: 0, - maxPageNumber: Math.ceil(10 / 50) - }) - expect(next).toHaveBeenCalledTimes(1) - }) - - it('should set up correct dataRange properties when pageNumber is greater than 1', () => { - const req = { - entityCount: { count: 80 }, - parsedParams: { pageNumber: 2 } - } - const res = {} - const next = vi.fn() - - getDataRange(req, res, next) - - expect(req.dataRange).toEqual({ - minRow: 50, - maxRow: 80, - totalRows: 80, - pageLength: 50, - offset: 50, - maxPageNumber: Math.ceil(80 / 50) - }) - expect(next).toHaveBeenCalledTimes(1) - }) - }) - describe('prepareTemplateParams', () => { it('prepares template parameters with correct properties', () => { const req = { diff --git a/test/unit/middleware/issueDetails.middleware.test.js b/test/unit/middleware/issueDetails.middleware.test.js deleted file mode 100644 index fb9c7892..00000000 --- a/test/unit/middleware/issueDetails.middleware.test.js +++ /dev/null @@ -1,319 +0,0 @@ -import { describe, it, vi, expect } from 'vitest' -import * as v from 'valibot' - -import performanceDbApi from '../../../src/services/performanceDbApi.js' -import { getIssueDetails, IssueDetailsQueryParams, prepareIssueDetailsTemplateParams } from '../../../src/middleware/issueDetails.middleware.js' - -vi.mock('../../../src/services/performanceDbApi.js') - -describe('issueDetails.middleware.js', () => { - const orgInfo = { name: 'mock lpa', organisation: 'ORG' } - const dataset = { name: 'mock dataset', dataset: 'mock-dataset', collection: 'mock-collection' } - const entryData = [ - { - field: 'start-date', - value: '02-02-2022', - entry_number: 1 - } - ] - const issues = [ - { - entry_number: 0, - field: 'start-date', - value: '02-02-2022' - } - ] - - describe('prepareIssueDetailsTemplateParams', () => { - it('should correctly set the template params', async () => { - const requestParams = { - lpa: 'test-lpa', - dataset: 'test-dataset', - issue_type: 'test-issue-type', - issue_field: 'test-issue-field', - resourceId: 'test-resource-id', - entityNumber: '1' - } - const req = { - params: requestParams, - parsedParams: { pageNumber: 1 }, - // middleware supplies the below - entryNumber: 1, - entityCount: { entity_count: 1 }, - issueEntitiesCount: { count: 1 }, - pageNumber: 1, - pagination: { - items: [ - { - current: true, - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry/1', - number: 1, - type: 'number' - } - ] - }, - baseSubpath: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry', - orgInfo, - dataset, - entryData, - issues, - resource: { resource: requestParams.resourceId }, - entryNumberCount: 1, - issuesByEntryNumber: { - 1: [ - { - field: 'start-date', - value: '02-02-2022', - line_number: 1, - entry_number: 1, - message: 'mock message', - issue_type: 'mock type' - } - ] - } - // errorHeading -- set in prepare* fn - } - v.parse(IssueDetailsQueryParams, req.params) - - issues.forEach(issue => { - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce(`mockMessageFor: ${issue.entry_number}`) - }) - - prepareIssueDetailsTemplateParams(req, {}, () => {}) - - const expectedTempalteParams = { - organisation: { - name: 'mock lpa', - organisation: 'ORG' - }, - dataset: { - name: 'mock dataset', - dataset: 'mock-dataset', - collection: 'mock-collection' - }, - errorSummary: { - heading: undefined, - items: [ - { - html: 'mockMessageFor: 0' - } - ] - }, - entry: { - title: 'entry: 1', - fields: [ - { - key: { text: 'start-date' }, - value: { html: '

mock message

02-02-2022' }, - classes: 'dl-summary-card-list__row--error' - } - ], - geometries: [] - }, - issueType: 'test-issue-type', - pagination: { - items: [{ - current: true, - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry/1', - number: 1, - type: 'number' - }] - }, - issueEntitiesCount: 1, - pageNumber: 1 - } - - expect(req.templateParams).toEqual(expectedTempalteParams) - }) - - it('should correctly set the template params with the correct geometry params', async () => { - const entryData = [ - { - field: 'start-date', - value: '02-02-2022', - entry_number: 1 - }, - { - field: 'geometry', - value: 'POINT(0 0)', - entry_number: 1 - } - ] - const requestParams = { - lpa: 'test-lpa', - dataset: 'test-dataset', - issue_type: 'test-issue-type', - issue_field: 'test-issue-field', - resourceId: 'test-resource-id', - entityNumber: '1' - } - const req = { - params: requestParams, - parsedParams: { pageNumber: 1 }, - pagination: { - items: [ - { - current: true, - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry/1', - number: 1, - type: 'number' - } - ] - }, - baseSubpath: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry', - entryNumber: 1, - entityCount: { entity_count: 3 }, - issueEntitiesCount: { count: 1 }, - pageNumber: 1, - orgInfo, - dataset, - entryData, - issues, - resource: { resource: requestParams.resourceId }, - entryNumberCount: 1, - issuesByEntryNumber: { - 1: [ - { - field: 'start-date', - value: '02-02-2022', - line_number: 1, - entry_number: 1, - message: 'mock message', - issue_type: 'mock type' - } - ] - } - // errorHeading -- set in prepare* fn - } - v.parse(IssueDetailsQueryParams, req.params) - - issues.forEach(issue => { - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce(`mockMessageFor: ${issue.entry_number}`) - }) - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValueOnce('mock task message 1') - - prepareIssueDetailsTemplateParams(req, {}, () => {}) - - const expectedTemplateParams = { - organisation: { - name: 'mock lpa', - organisation: 'ORG' - }, - dataset: { - name: 'mock dataset', - dataset: 'mock-dataset', - collection: 'mock-collection' - }, - errorSummary: { - heading: 'mockMessageFor: 0', - items: [ - { - html: 'mock task message 1 in record 1', - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry/1' - } - ] - }, - entry: { - title: 'entry: 1', - fields: [ - { - key: { text: 'start-date' }, - value: { html: '

mock message

02-02-2022' }, - classes: 'dl-summary-card-list__row--error' - }, - { - classes: '', - key: { - text: 'geometry' - }, - value: { - html: 'POINT(0 0)' - } - } - ], - geometries: ['POINT(0 0)'] - }, - issueType: 'test-issue-type', - pagination: { - items: [{ - current: true, - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/entry/1', - number: 1, - type: 'number' - }] - }, - issueEntitiesCount: 1, - pageNumber: 1 - } - - expect(req.templateParams).toEqual(expectedTemplateParams) - }) - }) - - describe('getIssueDetails', () => { - it('should call render using the provided template params and correct view', () => { - const req = { - templateParams: { - organisation: { - name: 'mock lpa', - organisation: 'ORG' - }, - dataset: { - name: 'mock dataset', - dataset: 'mock-dataset', - collection: 'mock-collection' - }, - errorSummary: { - heading: 'mockMessageFor: 0', - items: [ - { - html: 'mock task message 1 in record 1', - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1' - } - ] - }, - entry: { - title: 'entry: 1', - fields: [ - { - key: { text: 'start-date' }, - value: { html: '

mock message

02-02-2022' }, - classes: 'dl-summary-card-list__row--error' - }, - { - classes: '', - key: { - text: 'geometry' - }, - value: { - html: 'POINT(0 0)' - } - } - ], - geometries: ['POINT(0 0)'] - }, - issueType: 'test-issue-type', - pagination: { - items: [{ - current: true, - href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1', - number: 1, - type: 'number' - }] - }, - issueEntitiesCount: 1, - pageNumber: 1 - } - } - - const res = { - render: vi.fn() - } - - getIssueDetails(req, res, () => {}) - - expect(res.render).toHaveBeenCalledTimes(1) - expect(res.render).toHaveBeenCalledWith('organisations/issueDetails.html', req.templateParams) - }) - }) -}) diff --git a/test/unit/middleware/issueTable.middleware.test.js b/test/unit/middleware/issueTable.middleware.test.js index 5d0c3d50..ce93e013 100644 --- a/test/unit/middleware/issueTable.middleware.test.js +++ b/test/unit/middleware/issueTable.middleware.test.js @@ -1,103 +1,9 @@ import { describe, it, expect, vi } from 'vitest' -import { getDataRange, getErrorSummaryItems, issueTypeAndFieldShouldRedirect, notIssueHasEntity, prepareTableParams, prepareTemplateParams, redirectToEntityView, setBaseSubpath } from '../../../src/middleware/issueTable.middleware' -import performanceDbApi from '../../../src/services/performanceDbApi.js' +import { issueTypeAndFieldShouldRedirect, notIssueHasEntity, prepareTableParams, prepareTemplateParams, redirectToEntityView } from '../../../src/middleware/issueTable.middleware' vi.mock('../../../src/services/performanceDbApi.js') describe('issueTableMiddleware', () => { - describe('setBaseSubpath', () => { - it('sets baseSubpath correctly', () => { - const req = { - params: { - lpa: 'lpa-value', - dataset: 'dataset-value', - issue_type: 'issue-type-value', - issue_field: 'issue-field-value' - } - } - const res = {} - const next = vi.fn() - - setBaseSubpath(req, res, next) - - expect(req.baseSubpath).toBe('/organisations/lpa-value/dataset-value/issue-type-value/issue-field-value') - expect(next).toHaveBeenCalledTimes(1) - }) - }) - - describe('getDataRange', () => { - it('sets dataRange correctly', () => { - const req = { - issues: [ - { entity: 'entity1', field: 'field1', resource: 'resource1' }, - { entity: 'entity2', field: 'field2', resource: 'resource1' }, - { entity: 'entity3', field: 'field3', resource: 'resource1' } - ], - parsedParams: { - pageNumber: 1 - } - } - const next = vi.fn() - - getDataRange(req, {}, next) - - expect(req.dataRange).toEqual({ - minRow: 0, - maxRow: 3, - totalRows: 3, - maxPageNumber: 1, - pageLength: 50 - }) - expect(next).toHaveBeenCalledTimes(1) - }) - - it('sets dataRange correctly for last page', () => { - const req = { - issues: { - length: 160 - }, - parsedParams: { - pageNumber: 2 - } - } - const next = vi.fn() - - getDataRange(req, {}, next) - - expect(req.dataRange).toEqual({ - minRow: 50, - maxRow: 100, - totalRows: 160, - maxPageNumber: 4, - pageLength: 50 - }) - expect(next).toHaveBeenCalledTimes(1) - }) - - it('sets dataRange correctly for first page', () => { - const req = { - issues: { - length: 160 - }, - parsedParams: { - pageNumber: 1 - } - } - const next = vi.fn() - - getDataRange(req, {}, next) - - expect(req.dataRange).toEqual({ - minRow: 0, - maxRow: 50, - totalRows: 160, - maxPageNumber: 4, - pageLength: 50 - }) - expect(next).toHaveBeenCalledTimes(1) - }) - }) - describe('prepareTableParams', () => { const req = { entities: [ @@ -122,7 +28,8 @@ describe('issueTableMiddleware', () => { dataset: 'dataset-value', issue_type: 'issue-type-value', issue_field: 'issue-field-value' - } + }, + baseSubpath: '/organisations/lpa-value/dataset-value/issue-type-value/issue-field-value' } it('correctly sets tableParams.fields', async () => { @@ -165,7 +72,7 @@ describe('issueTableMiddleware', () => { { columns: { reference: { - html: "entity1", + html: "entity1", error: undefined }, name: { @@ -183,7 +90,7 @@ describe('issueTableMiddleware', () => { { columns: { reference: { - html: "entity2", + html: "entity2", error: undefined }, name: { @@ -204,115 +111,6 @@ describe('issueTableMiddleware', () => { }) }) - describe('getErrorSummaryItems', () => { - it('handles no issues are found', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value', - baseSubpath: 'baseSubpath-value' - }, - entities: [], - issues: [], - headers: { - referer: 'referer' - } - } - - const next = vi.fn() - - getErrorSummaryItems(req, null, next) - - expect(next).toHaveBeenCalledWith(new Error('issue count must be larger than 0')) - }) - - it('does not set this header if every entity has the issue', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value', - baseSubpath: 'baseSubpath-value' - }, - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.heading).toBe('') - }) - - it('does sets the header if some entities do not have the issue', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value', - baseSubpath: 'baseSubpath-value' - }, - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.heading).toBe('message') - }) - - it('Correctly sets the issue items', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value', - baseSubpath: 'baseSubpath-value' - }, - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('issue') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.items).toEqual([ - { - html: 'issue in entity entity1', - href: 'baseSubpath-value/entity/1' - }, - { - html: 'issue in entity entity2', - href: 'baseSubpath-value/entity/2' - } - ]) - }) - }) - describe('prepareTemplateParams', () => { it('should set templateParams object with all required props', () => { const req = { diff --git a/test/unit/views/organisations/issueDetailsPage.test.js b/test/unit/views/organisations/issueDetailsPage.test.js index 57aade7a..d527ccf8 100644 --- a/test/unit/views/organisations/issueDetailsPage.test.js +++ b/test/unit/views/organisations/issueDetailsPage.test.js @@ -13,8 +13,6 @@ const seed = new Date().getTime() describe(`issueDetails.html(seed: ${seed})`, () => { const params = mocker(OrgIssueDetails, seed) - params.issueEntitiesCount = undefined - const html = nunjucks.render('organisations/issueDetails.html', params) const dom = new JSDOM(html) const document = dom.window.document @@ -118,7 +116,7 @@ describe(`issueDetails.html(seed: ${seed})`, () => { next, items } - params.issueEntitiesCount = 10 + const multiPageHtml = nunjucks.render('organisations/issueDetails.html', params) // const multiPageDom = new JSDOM(multiPageHtml) // const multiPageDocument = multiPageDom.window.document @@ -174,6 +172,7 @@ describe(`issueDetails.html(seed: ${seed})`, () => { describe('Dataset visualisation: Maps', () => { it('should render a map when geometries are passed in', () => { const paramWithGeometry = { + ...params, organisation: params.organisation, dataset: params.dataset, errorSummary: { From 8fe4abdbfb5480d10e5f5a6fc31b09ca0f0366b9 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 10:23:19 +0000 Subject: [PATCH 13/33] fix existing tests and add tests to common.middleware.test.js --- src/middleware/common.middleware.js | 8 +- .../entityIssueDetails.middleware.js | 1 + .../unit/middleware/common.middleware.test.js | 257 +++++++++++++++++- .../entityIssueDetails.middleware.test.js | 89 ++++++ .../entryIssueDetails.middleware.test.js | 23 ++ 5 files changed, 376 insertions(+), 2 deletions(-) create mode 100644 test/unit/middleware/entityIssueDetails.middleware.test.js create mode 100644 test/unit/middleware/entryIssueDetails.middleware.test.js diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 8ea5658c..f3119511 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -576,8 +576,14 @@ export function getErrorSummaryItems (req, res, next) { errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: totalRecordCount, field: issueField }, true) issueItems = issues.map((issue, i) => { const pageNum = i + 1 + let inString = '' + if (issue.entity) { + inString = ` in entity ${issue.entity}` + } else if (issue.entry_number) { + inString = ` in entry ${issue.entry_number}` + } return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }), // ToDo: + ` in entity ${issue.entity}`, + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + inString, href: `${baseSubpath}/${pageNum}` } }) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index c54b7325..3597585d 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -63,6 +63,7 @@ export const setRecordCount = (req, res, next) => { req.recordCount = req.issues.length next() } + export function prepareEntity (req, res, next) { const { entities, issues, specification } = req const { pageNumber, issue_type: issueType } = req.parsedParams diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 1f35d981..4fd0e8df 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1,7 +1,10 @@ import { describe, it, expect, vi } from 'vitest' -import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue } from '../../../src/middleware/common.middleware' +import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems } 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' + +vi.mock('../../../src/services/performanceDbApi.js') vi.mock('../../../src/services/datasette.js', () => ({ default: { @@ -1148,3 +1151,255 @@ describe('addFieldMappingsToIssue', () => { }) }) }) + +describe('setDataRange', () => { + describe('tableView', () => { + const setTableDataRange = getSetDataRange(50) + + it('should set up correct dataRange properties when pageNumber is 1', () => { + const req = { + recordCount: 10, + parsedParams: { pageNumber: 1 } + } + const res = {} + const next = vi.fn() + + setTableDataRange(req, res, next) + + expect(req.dataRange).toEqual({ + minRow: 0, + maxRow: 10, + totalRows: 10, + pageLength: 50, + offset: 0, + maxPageNumber: Math.ceil(10 / 50) + }) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('should set up correct dataRange properties when pageNumber is greater than 1', () => { + const req = { + recordCount: 80, + parsedParams: { pageNumber: 2 } + } + const res = {} + const next = vi.fn() + + setTableDataRange(req, res, next) + + expect(req.dataRange).toEqual({ + minRow: 50, + maxRow: 80, + totalRows: 80, + pageLength: 50, + offset: 50, + maxPageNumber: Math.ceil(80 / 50) + }) + expect(next).toHaveBeenCalledTimes(1) + }) + }) + + describe('entityView', () => { + const setEntityDataRange = getSetDataRange(1) + + it('should set up correct dataRange properties when pageNumber is 1', () => { + const req = { + recordCount: 10, + parsedParams: { pageNumber: 1 } + } + const res = {} + const next = vi.fn() + + setEntityDataRange(req, res, next) + + expect(req.dataRange).toEqual({ + minRow: 0, + maxRow: 1, + totalRows: 10, + pageLength: 1, + offset: 0, + maxPageNumber: 10 + }) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('should set up correct dataRange properties when pageNumber is greater than 1', () => { + const req = { + recordCount: 10, + parsedParams: { pageNumber: 3 } + } + const res = {} + const next = vi.fn() + + setEntityDataRange(req, res, next) + + expect(req.dataRange).toEqual({ + minRow: 2, + maxRow: 3, + totalRows: 10, + pageLength: 1, + offset: 2, + maxPageNumber: 10 + }) + expect(next).toHaveBeenCalledTimes(1) + }) + }) +}) + +describe('setBaseSubPath', () => { + it('sets baseSubpath correctly when given all url params', () => { + + }) + + it('sets up baseSubpath correctly when given partial params', () => { + + }) + + it('adds additional path parts on at the end', () => { + + }) +}) + +describe('getErrorSummaryItems', () => { + it('handles no issues are found', () => { + const req = { + params: { + issue_type: 'issue-type-value', + issue_field: 'issue-field-value' + }, + baseSubpath: 'baseSubpath-value', + entities: [], + issues: [], + headers: { + referer: 'referer' + } + } + + const next = vi.fn() + + getErrorSummaryItems(req, null, next) + + expect(next).toHaveBeenCalledWith(new Error('issue count must be larger than 0')) + }) + + it('handles if every entity has the issue', () => { + const req = { + params: { + issue_type: 'issue-type-value', + issue_field: 'issue-field-value' + }, + baseSubpath: 'baseSubpath-value', + entities: [ + { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, + { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' } + ], + issues: [ + { entity: 'entity1', error: 'Invalid Amount' }, + { entity: 'entity2', error: ' Invalid Name' } + ] + } + + vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') + + getErrorSummaryItems(req, null, vi.fn()) + + const errorSummary = req.errorSummary + expect(errorSummary.heading).toBe('') + }) + + it('handles some entities not having the issue', () => { + const req = { + params: { + issue_type: 'issue-type-value', + issue_field: 'issue-field-value' + }, + baseSubpath: 'baseSubpath-value', + entities: [ + { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, + { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, + { reference: 'entity3', name: 'Name 3', amount: 300 } + ], + issues: [ + { entity: 'entity1', error: 'Invalid Amount' }, + { entity: 'entity2', error: ' Invalid Name' } + ] + } + + vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') + + getErrorSummaryItems(req, null, vi.fn()) + + const errorSummary = req.errorSummary + expect(errorSummary.heading).toBe('message') + }) + + it('Correctly sets the issue items when some entities dont have issues', () => { + const req = { + params: { + issue_type: 'issue-type-value', + issue_field: 'issue-field-value' + }, + baseSubpath: 'baseSubpath-value/entity', + entities: [ + { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, + { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, + { reference: 'entity3', name: 'Name 3', amount: 300 } + ], + issues: [ + { entity: 'entity1', error: 'Invalid Amount' }, + { entity: 'entity2', error: ' Invalid Name' } + ] + } + + vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('issue') + + getErrorSummaryItems(req, null, vi.fn()) + + const errorSummary = req.errorSummary + expect(errorSummary.items).toEqual([ + { + html: 'issue in entity entity1', + href: 'baseSubpath-value/entity/1' + }, + { + html: 'issue in entity entity2', + href: 'baseSubpath-value/entity/2' + } + ]) + }) + + it('handles no entities provided, but a resource provided with less issues than entries', () => { + const req = { + params: { + issue_type: 'issue-type-value', + issue_field: 'issue-field-value' + }, + baseSubpath: 'baseSubpath-value/entry', + resources: [ + { + entry_count: 3 + } + ], + issues: [ + { entry_number: 1, error: 'Invalid Amount' }, + { entry_number: 2, error: ' Invalid Name' } + ] + } + + vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('issue') + + getErrorSummaryItems(req, null, vi.fn()) + + const errorSummary = req.errorSummary + expect(errorSummary.items).toEqual([ + { + html: 'issue in entry 1', + href: 'baseSubpath-value/entry/1' + }, + { + html: 'issue in entry 2', + href: 'baseSubpath-value/entry/2' + } + ]) + }) +}) diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js new file mode 100644 index 00000000..11aa6f07 --- /dev/null +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -0,0 +1,89 @@ +import { describe, it, vi, expect } from 'vitest' +import { getIssueDetails } from '../../../src/middleware/entityIssueDetails.middleware.js' + +vi.mock('../../../src/services/performanceDbApi.js') + +describe('issueDetails.middleware.js', () => { + describe('prepareEntity', () => { + + }) + + describe('set record count', () => { + + }) + + describe('prepareEntityIssueDetailsTemplateParams', () => { + + }) + + describe('getIssueDetails', () => { + it('should call render using the provided template params and correct view', () => { + const req = { + templateParams: { + organisation: { + name: 'mock lpa', + organisation: 'ORG' + }, + dataset: { + name: 'mock dataset', + dataset: 'mock-dataset', + collection: 'mock-collection' + }, + errorSummary: { + heading: 'mockMessageFor: 0', + items: [ + { + html: 'mock task message 1 in record 1', + href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1' + } + ] + }, + entry: { + title: 'entry: 1', + fields: [ + { + key: { text: 'start-date' }, + value: { html: '

mock message

02-02-2022' }, + classes: 'dl-summary-card-list__row--error' + }, + { + classes: '', + key: { + text: 'geometry' + }, + value: { + html: 'POINT(0 0)' + } + } + ], + geometries: ['POINT(0 0)'] + }, + issueType: 'test-issue-type', + pagination: { + items: [{ + current: true, + href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1', + number: 1, + type: 'number' + }] + }, + pageNumber: 1, + dataRange: { + minRow: 0, + maxRow: 50, + totalRows: 150 + } + } + } + + const res = { + render: vi.fn() + } + + getIssueDetails(req, res, () => {}) + + expect(res.render).toHaveBeenCalledTimes(1) + expect(res.render).toHaveBeenCalledWith('organisations/issueDetails.html', req.templateParams) + }) + }) +}) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js new file mode 100644 index 00000000..739cf954 --- /dev/null +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -0,0 +1,23 @@ +import { describe } from 'vitest' + +describe('entryIssueDetails.middleware.test.js', () => { + describe('addResourceMetaDataToResources', () => { + + }) + + describe('setRecordCount', () => { + + }) + + describe('prepareEntry', () => { + + }) + + describe('prepareEntryIssueDetailsTemplateParams', () => { + + }) + + describe('getIssueDetails', () => { + + }) +}) From 7baf8d071ddc9caf322ffc79042d658f0e287583 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 10:27:46 +0000 Subject: [PATCH 14/33] added baseSubPath tests --- .../unit/middleware/common.middleware.test.js | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 4fd0e8df..639ac881 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 } from 'vitest' -import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems } from '../../../src/middleware/common.middleware' +import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems, getSetBaseSubPath } 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' @@ -1247,16 +1247,52 @@ describe('setDataRange', () => { }) describe('setBaseSubPath', () => { + const lpa = 'lpa' + const dataset = 'dataset' + const issueType = 'issueType' + const issueField = 'issueField' it('sets baseSubpath correctly when given all url params', () => { + const req = { + params: { + lpa, + dataset, + issue_type: issueType, + issue_field: issueField + } + } + const setBaseSubPath = getSetBaseSubPath([]) + setBaseSubPath(req, {}, vi.fn()) + + expect(req.baseSubpath).toEqual('/organisations/lpa/dataset/issueType/issueField') }) it('sets up baseSubpath correctly when given partial params', () => { + const req = { + params: { + lpa, + dataset + } + } + const setBaseSubPath = getSetBaseSubPath([]) + + setBaseSubPath(req, {}, vi.fn()) + expect(req.baseSubpath).toEqual('/organisations/lpa/dataset') }) it('adds additional path parts on at the end', () => { + const req = { + params: { + lpa, + dataset + } + } + const setBaseSubPath = getSetBaseSubPath(['subPath']) + + setBaseSubPath(req, {}, vi.fn()) + expect(req.baseSubpath).toEqual('/organisations/lpa/dataset/subPath') }) }) From 67a5614fbbc6bd328d485757c741698ce62b46db Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 10:36:08 +0000 Subject: [PATCH 15/33] setRecordCount tests --- .../entityIssueDetails.middleware.js | 4 +- .../entityIssueDetails.middleware.test.js | 61 ++++++++++++++++++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 3597585d..09358c0a 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -47,7 +47,7 @@ const issueErrorMessageHtml = (errorMessage, issue) => * @param {*} classes * @returns {{key: {text: string}, value: { html: string}, classes: string}} */ -const getIssueField = (text, html, classes) => { +export const getIssueField = (text, html, classes) => { return { key: { text @@ -60,7 +60,7 @@ const getIssueField = (text, html, classes) => { } export const setRecordCount = (req, res, next) => { - req.recordCount = req.issues.length + req.recordCount = req?.issues?.length next() } diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index 11aa6f07..d110ed0e 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -1,14 +1,71 @@ import { describe, it, vi, expect } from 'vitest' -import { getIssueDetails } from '../../../src/middleware/entityIssueDetails.middleware.js' +import { getIssueDetails, getIssueField, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' vi.mock('../../../src/services/performanceDbApi.js') describe('issueDetails.middleware.js', () => { - describe('prepareEntity', () => { + describe('getIssueField', () => { + it('returns an object with key, value, and classes properties', () => { + const text = 'Issue Text' + const html = '

Issue HTML

' + const classes = ['issue-class'] + const result = getIssueField(text, html, classes) + + expect(result).toEqual({ + key: { text }, + value: { html }, + classes + }) + }) + it('returns an object with default classes if classes are not provided', () => { + const text = 'Issue Text' + const html = '

Issue HTML

' + const result = getIssueField(text, html) + + expect(result).toEqual({ + key: { text }, + value: { html } + }) + }) }) describe('set record count', () => { + it('sets req.recordCount to the length of req.issues', () => { + const req = { issues: [{}, {}, {}] } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(3) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.issues is undefined', () => { + const req = {} + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.issues is null', () => { + const req = { issues: null } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + }) + + describe('prepareEntity', () => { }) From e8c58ce4e37ebbfb556a8d52ad52b364840470f6 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 10:49:04 +0000 Subject: [PATCH 16/33] add tests for prepareEntity --- .../entityIssueDetails.middleware.js | 13 +--- .../entityIssueDetails.middleware.test.js | 60 ++++++++++++++++++- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 09358c0a..a1968c4d 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -48,12 +48,13 @@ const issueErrorMessageHtml = (errorMessage, issue) => * @returns {{key: {text: string}, value: { html: string}, classes: string}} */ export const getIssueField = (text, html, classes) => { + classes = classes || '' return { key: { text }, value: { - html + html: html ? html.toString() : '' }, classes } @@ -74,15 +75,7 @@ export function prepareEntity (req, res, next) { const fields = specification.fields.map(({ field, datasetField }) => { const value = entityData[field] || entityData[datasetField] - return { - key: { - text: field - }, - value: { - html: value ? value.toString() : '' - }, - classes: '' - } + return getIssueField(field, value) }) entityIssues.forEach(issue => { diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index d110ed0e..e4f1331b 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -1,5 +1,5 @@ -import { describe, it, vi, expect } from 'vitest' -import { getIssueDetails, getIssueField, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' +import { describe, it, vi, expect, beforeEach } from 'vitest' +import { getIssueDetails, getIssueField, prepareEntity, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' vi.mock('../../../src/services/performanceDbApi.js') @@ -25,7 +25,8 @@ describe('issueDetails.middleware.js', () => { expect(result).toEqual({ key: { text }, - value: { html } + value: { html }, + classes: '' }) }) }) @@ -66,7 +67,60 @@ describe('issueDetails.middleware.js', () => { }) describe('prepareEntity', () => { + const req = {} + const res = {} + + beforeEach(() => { + req.entities = [ + { entity: 'entity1', name: 'Entity 1', field1: 'value1', field2: 'value2' }, + { entity: 'entity2', name: 'Entity 2', field1: 'value3', field2: 'value4' } + ] + req.issues = [ + { entity: 'entity1', field: 'field1', message: 'Error 1' }, + { entity: 'entity1', field: 'field2', message: 'Error 2' }, + { entity: 'entity2', field: 'field1', message: 'Error 3' } + ] + req.specification = { fields: [{ field: 'field1', datasetField: 'datasetField1' }, { field: 'field2', datasetField: 'datasetField2' }] } + req.parsedParams = { pageNumber: 1, issue_type: 'issueType' } + }) + + it('should set req.entry with correct title and fields', () => { + prepareEntity(req, res, () => {}) + expect(req.entry).toEqual({ + title: 'Entity 1', + fields: [ + { key: { text: 'field1' }, value: { html: '

Error 1

value1' }, classes: 'dl-summary-card-list__row--error' }, + { key: { text: 'field2' }, value: { html: '

Error 2

value2' }, classes: 'dl-summary-card-list__row--error' } + ], + geometries: [] + }) + }) + it('should add error classes to fields with issues', () => { + prepareEntity(req, res, () => {}) + expect(req.entry.fields[0].classes).toContain('dl-summary-card-list__row--error') + expect(req.entry.fields[1].classes).toContain('dl-summary-card-list__row--error') + }) + + it('should add new fields for issues without matching specification fields', () => { + req.issues.push({ entity: 'entity1', field: 'newField', message: 'New Error' }) + prepareEntity(req, res, () => {}) + expect(req.entry.fields).toHaveLength(3) + expect(req.entry.fields[2].key.text).toBe('newField') + expect(req.entry.fields[2].value.html).toBe('

New Error

') + expect(req.entry.fields[2].classes).toContain('dl-summary-card-list__row--error') + }) + + it('should handle no issues or specification fields', () => { + req.issues = [] + req.specification = { fields: [] } + prepareEntity(req, res, () => {}) + expect(req.entry).toEqual({ + title: 'Entity 1', + fields: [], + geometries: [] + }) + }) }) describe('prepareEntityIssueDetailsTemplateParams', () => { From 4963474bf7dd80936e4931c2b0c898018ff85e5c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 12:38:03 +0000 Subject: [PATCH 17/33] add tests for prepareEntityIssueDetailsTemplateParams --- .../entityIssueDetails.middleware.test.js | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index e4f1331b..05c81c0f 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, vi, expect, beforeEach } from 'vitest' -import { getIssueDetails, getIssueField, prepareEntity, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' +import { getIssueDetails, getIssueField, prepareEntity, prepareEntityIssueDetailsTemplateParams, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' vi.mock('../../../src/services/performanceDbApi.js') @@ -124,7 +124,45 @@ describe('issueDetails.middleware.js', () => { }) describe('prepareEntityIssueDetailsTemplateParams', () => { + const req = { + params: { issue_type: 'some-issue-type' }, + parsedParams: { pageNumber: 1 }, + entry: 'some-entry', + pagination: 'some-pagination', + errorSummary: 'some-error-summary', + dataRange: 'some-data-range', + dataset: 'some-dataset', + orgInfo: 'some-org-info' + } + const res = {} + + beforeEach(() => { + req.templateParams = undefined + }) + + it('should set req.templateParams with expected values', () => { + const next = vi.fn() + prepareEntityIssueDetailsTemplateParams(req, res, next) + + expect(req.templateParams).toEqual({ + organisation: 'some-org-info', + dataset: 'some-dataset', + errorSummary: 'some-error-summary', + entry: 'some-entry', + issueType: 'some-issue-type', + pagination: 'some-pagination', + pageNumber: 1, + dataRange: 'some-data-range' + }) + }) + + it('should call next function', () => { + const next = vi.fn() + prepareEntityIssueDetailsTemplateParams(req, res, next) + + expect(next).toHaveBeenCalledTimes(1) + }) }) describe('getIssueDetails', () => { From a52ec8da0cfc159e481bad9040e205d6c314f45c Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 13:46:52 +0000 Subject: [PATCH 18/33] add tests for addResourceMetaDataToResources --- .../entryIssueDetails.middleware.js | 8 +-- .../entryIssueDetails.middleware.test.js | 59 ++++++++++++++++++- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 3b10fbe7..c32bbe71 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -25,13 +25,13 @@ const fetchResourceMetaData = fetchMany({ result: 'resourceMetaData' }) -const addResourceMetaDataToResources = (req, res, next) => { +export const addResourceMetaDataToResources = (req, res, next) => { const { resources, resourceMetaData } = req req.resources = resources.map(resource => { const metaData = resourceMetaData.find(metaData => metaData.resource === resource.resource) if (metaData) { - return { ...resource, ...resourceMetaData } + return { ...resource, ...metaData } } return resource }) @@ -57,7 +57,7 @@ export const setRecordCount = (req, res, next) => { req.recordCount = req.issues.length next() } -const prepareEntry = (req, res, next) => { +export const prepareEntry = (req, res, next) => { const { resources, issues } = req const { pageNumber } = req.parsedParams @@ -98,7 +98,7 @@ const prepareEntry = (req, res, next) => { next() } -const prepareEntryIssueDetailsTemplateParams = (req, res, next) => { +export const prepareEntryIssueDetailsTemplateParams = (req, res, next) => { const { issue_type: issueType, pageNumber } = req.parsedParams const { entry, pagination, dataRange, errorSummary, dataset, orgInfo } = req diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 739cf954..b0050c87 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -1,8 +1,65 @@ -import { describe } from 'vitest' +import { describe, it, expect, vi } from 'vitest' +import { addResourceMetaDataToResources } from '../../../src/middleware/entryIssueDetails.middleware' describe('entryIssueDetails.middleware.test.js', () => { describe('addResourceMetaDataToResources', () => { + it('adds metadata to resources when metadata is found', () => { + const req = { + resources: [ + { resource: 'resource1', foo: 'bar' }, + { resource: 'resource2', baz: 'qux' } + ], + resourceMetaData: [ + { resource: 'resource1', description: 'Resource 1 desc' }, + { resource: 'resource3', description: 'Resource 3 desc' } + ] + } + const res = {} + const next = vi.fn() + addResourceMetaDataToResources(req, res, next) + + expect(req.resources).toEqual([ + { resource: 'resource1', foo: 'bar', description: 'Resource 1 desc' }, + { resource: 'resource2', baz: 'qux' } + ]) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('leaves resources unchanged when metadata is not found', () => { + const req = { + resources: [ + { resource: 'resource1', foo: 'bar' }, + { resource: 'resource2', baz: 'qux' } + ], + resourceMetaData: [ + { resource: 'resource3', metaData: { description: 'Resource 3 desc' } } + ] + } + const res = {} + const next = vi.fn() + + addResourceMetaDataToResources(req, res, next) + + expect(req.resources).toEqual([ + { resource: 'resource1', foo: 'bar' }, + { resource: 'resource2', baz: 'qux' } + ]) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('calls next() when done', () => { + const req = { + resources: [], + resourceMetaData: [] + } + const res = {} + const next = vi.fn() + + addResourceMetaDataToResources(req, res, next) + + expect(next).toHaveBeenCalledTimes(1) + }) }) describe('setRecordCount', () => { From 4886ae97b75d241fadcf7e662ebb66b3ba60bf71 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 14:29:46 +0000 Subject: [PATCH 19/33] move prepare issue details middleware to common --- src/middleware/common.middleware.js | 19 +++++ .../entityIssueDetails.middleware.js | 28 +----- .../entryIssueDetails.middleware.js | 30 ++----- .../unit/middleware/common.middleware.test.js | 45 +++++++++- .../entityIssueDetails.middleware.test.js | 44 +--------- .../entryIssueDetails.middleware.test.js | 85 ++++++++++++++++++- 6 files changed, 159 insertions(+), 92 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index f3119511..0e31fabc 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -600,3 +600,22 @@ export function getErrorSummaryItems (req, res, next) { next() } + +export const prepareIssueDetailsTemplateParams = (req, res, next) => { + const { entry, pagination, dataRange, errorSummary, dataset, orgInfo } = req + const { issue_type: issueType, pageNumber } = req.parsedParams + + // schema: OrgIssueDetails + req.templateParams = { + organisation: orgInfo, + dataset, + errorSummary, + entry, + issueType, + pagination, + pageNumber, + dataRange + } + + next() +} diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index a1968c4d..5f9e5b9b 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -11,7 +11,8 @@ import { processSpecificationMiddlewares, getSetBaseSubPath, getSetDataRange, - getErrorSummaryItems + getErrorSummaryItems, + prepareIssueDetailsTemplateParams } from './common.middleware.js' import { renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -111,29 +112,6 @@ export function prepareEntity (req, res, next) { next() } -/*** - * Middleware. Updates req with `templateParams` - */ -export function prepareEntityIssueDetailsTemplateParams (req, res, next) { - const { entry, pagination, errorSummary, dataRange, dataset, orgInfo } = req - const { issue_type: issueType } = req.params - const { pageNumber } = req.parsedParams - - // schema: OrgIssueDetails - req.templateParams = { - organisation: orgInfo, - dataset, - errorSummary, - entry, - issueType, - pagination, - pageNumber, - dataRange - } - - next() -} - /** * Middleware. Renders the issue details page with the list of issues, entry data, * and organisation and dataset details. @@ -159,7 +137,7 @@ export default [ createPaginationTemplateParams, getErrorSummaryItems, prepareEntity, - prepareEntityIssueDetailsTemplateParams, + prepareIssueDetailsTemplateParams, getIssueDetails, logPageError ] diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index c32bbe71..0c31868e 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, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' export const IssueDetailsQueryParams = v.object({ @@ -57,10 +57,17 @@ export const setRecordCount = (req, res, next) => { req.recordCount = req.issues.length next() } + export const prepareEntry = (req, res, next) => { const { resources, issues } = req const { pageNumber } = req.parsedParams + if (!issues[pageNumber - 1] || !resources) { + const error = new Error('Missing required values on request object') + error.status = 404 + return next(error) + } + const issue = issues[pageNumber - 1] req.entry = { @@ -98,25 +105,6 @@ export const prepareEntry = (req, res, next) => { next() } -export const prepareEntryIssueDetailsTemplateParams = (req, res, next) => { - const { issue_type: issueType, pageNumber } = req.parsedParams - const { entry, pagination, dataRange, errorSummary, dataset, orgInfo } = req - - // schema: OrgIssueDetails - req.templateParams = { - organisation: orgInfo, - dataset, - errorSummary, - entry, - issueType, - pagination, - pageNumber, - dataRange - } - - next() -} - export const getIssueDetails = renderTemplate({ templateParams: (req) => req.templateParams, template: 'organisations/issueDetails.html', @@ -138,6 +126,6 @@ export default [ createPaginationTemplateParams, getErrorSummaryItems, prepareEntry, - prepareEntryIssueDetailsTemplateParams, + prepareIssueDetailsTemplateParams, getIssueDetails ] diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index 639ac881..ee6b4e59 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 } from 'vitest' -import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, addFieldMappingsToIssue, getSetDataRange, getErrorSummaryItems, getSetBaseSubPath } from '../../../src/middleware/common.middleware' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { createPaginationTemplateParams, addDatabaseFieldToSpecification, replaceUnderscoreInSpecification, pullOutDatasetSpecification, extractJsonFieldFromEntities, replaceUnderscoreInEntities, setDefaultParams, getUniqueDatasetFieldsFromSpecification, show404IfPageNumberNotInRange, FilterOutIssuesToMostRecent, 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' @@ -1439,3 +1439,44 @@ describe('getErrorSummaryItems', () => { ]) }) }) + +describe('prepareEntityIssueDetailsTemplateParams', () => { + const req = { + parsedParams: { pageNumber: 1, issue_type: 'some-issue-type' }, + entry: 'some-entry', + pagination: 'some-pagination', + errorSummary: 'some-error-summary', + dataRange: 'some-data-range', + dataset: 'some-dataset', + orgInfo: 'some-org-info' + } + + const res = {} + + beforeEach(() => { + req.templateParams = undefined + }) + + it('should set req.templateParams with expected values', () => { + const next = vi.fn() + prepareIssueDetailsTemplateParams(req, res, next) + + expect(req.templateParams).toEqual({ + organisation: 'some-org-info', + dataset: 'some-dataset', + errorSummary: 'some-error-summary', + entry: 'some-entry', + issueType: 'some-issue-type', + pagination: 'some-pagination', + pageNumber: 1, + dataRange: 'some-data-range' + }) + }) + + it('should call next function', () => { + const next = vi.fn() + prepareIssueDetailsTemplateParams(req, res, next) + + expect(next).toHaveBeenCalledTimes(1) + }) +}) diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index 05c81c0f..dd712fd2 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, vi, expect, beforeEach } from 'vitest' -import { getIssueDetails, getIssueField, prepareEntity, prepareEntityIssueDetailsTemplateParams, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' +import { getIssueDetails, getIssueField, prepareEntity, setRecordCount } from '../../../src/middleware/entityIssueDetails.middleware.js' vi.mock('../../../src/services/performanceDbApi.js') @@ -123,48 +123,6 @@ describe('issueDetails.middleware.js', () => { }) }) - describe('prepareEntityIssueDetailsTemplateParams', () => { - const req = { - params: { issue_type: 'some-issue-type' }, - parsedParams: { pageNumber: 1 }, - entry: 'some-entry', - pagination: 'some-pagination', - errorSummary: 'some-error-summary', - dataRange: 'some-data-range', - dataset: 'some-dataset', - orgInfo: 'some-org-info' - } - - const res = {} - - beforeEach(() => { - req.templateParams = undefined - }) - - it('should set req.templateParams with expected values', () => { - const next = vi.fn() - prepareEntityIssueDetailsTemplateParams(req, res, next) - - expect(req.templateParams).toEqual({ - organisation: 'some-org-info', - dataset: 'some-dataset', - errorSummary: 'some-error-summary', - entry: 'some-entry', - issueType: 'some-issue-type', - pagination: 'some-pagination', - pageNumber: 1, - dataRange: 'some-data-range' - }) - }) - - it('should call next function', () => { - const next = vi.fn() - prepareEntityIssueDetailsTemplateParams(req, res, next) - - expect(next).toHaveBeenCalledTimes(1) - }) - }) - describe('getIssueDetails', () => { it('should call render using the provided template params and correct view', () => { const req = { diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index b0050c87..1d1ce546 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from 'vitest' -import { addResourceMetaDataToResources } from '../../../src/middleware/entryIssueDetails.middleware' +import { addResourceMetaDataToResources, prepareEntry, setRecordCount } from '../../../src/middleware/entryIssueDetails.middleware' describe('entryIssueDetails.middleware.test.js', () => { describe('addResourceMetaDataToResources', () => { @@ -63,11 +63,94 @@ describe('entryIssueDetails.middleware.test.js', () => { }) describe('setRecordCount', () => { + it('should set req.recordCount to req.issues.length', () => { + const req = { issues: [{}, {}, {}] } + const res = {} + const next = vi.fn() + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(3) + expect(next).toHaveBeenCalledTimes(1) + }) }) describe('prepareEntry', () => { + it('should prepare entry with correct fields', () => { + const req = { + resources: [{ endpoint_url: 'https://example.com' }], + issues: [ + { + entry_number: 1, + line_number: 2, + field: 'Field Name', + message: 'Error message', + value: 'Error value' + } + ], + parsedParams: { pageNumber: 1 } + } + const res = {} + const next = vi.fn() + prepareEntry(req, res, next) + + expect(req.entry).toEqual({ + title: 'entry: 1', + fields: [ + { + key: { text: 'Endpoint' }, + value: { html: 'https://example.com' }, + classes: '' + }, + { + key: { text: 'Line number' }, + value: { html: '2' }, + classes: '' + }, + { + key: { text: 'Field Name' }, + value: { html: '

Error message

Error value' }, + classes: '' + } + ] + }) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('should call next with error if issue is missing', () => { + const req = { + resources: [{ endpoint_url: 'https://example.com' }], + issues: [], + parsedParams: { pageNumber: 1 } + } + const res = {} + const next = vi.fn() + prepareEntry(req, res, next) + + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + }) + + it('should throw error if resources is missing', () => { + const req = { + issues: [ + { + entry_number: 1, + line_number: 2, + field: 'Field Name', + message: 'Error message', + value: 'Error value' + } + ], + parsedParams: { pageNumber: 1 } + } + const res = {} + const next = vi.fn() + + prepareEntry(req, res, next) + + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object')) + }) }) describe('prepareEntryIssueDetailsTemplateParams', () => { From b424b4d5184887dfa911494542a60e483d278742 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 14:31:09 +0000 Subject: [PATCH 20/33] add tests for getIssueDetails --- .../entryIssueDetails.middleware.test.js | 71 +++++++++++++++++-- 1 file changed, 67 insertions(+), 4 deletions(-) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 1d1ce546..d725bfa2 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from 'vitest' -import { addResourceMetaDataToResources, prepareEntry, setRecordCount } from '../../../src/middleware/entryIssueDetails.middleware' +import { addResourceMetaDataToResources, getIssueDetails, prepareEntry, setRecordCount } from '../../../src/middleware/entryIssueDetails.middleware' describe('entryIssueDetails.middleware.test.js', () => { describe('addResourceMetaDataToResources', () => { @@ -153,11 +153,74 @@ describe('entryIssueDetails.middleware.test.js', () => { }) }) - describe('prepareEntryIssueDetailsTemplateParams', () => { + describe('getIssueDetails', () => { + it('should call render using the provided template params and correct view', () => { + const req = { + templateParams: { + organisation: { + name: 'mock lpa', + organisation: 'ORG' + }, + dataset: { + name: 'mock dataset', + dataset: 'mock-dataset', + collection: 'mock-collection' + }, + errorSummary: { + heading: 'mockMessageFor: 0', + items: [ + { + html: 'mock task message 1 in record 1', + href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1' + } + ] + }, + entry: { + title: 'entry: 1', + fields: [ + { + key: { text: 'start-date' }, + value: { html: '

mock message

02-02-2022' }, + classes: 'dl-summary-card-list__row--error' + }, + { + classes: '', + key: { + text: 'geometry' + }, + value: { + html: 'POINT(0 0)' + } + } + ], + geometries: ['POINT(0 0)'] + }, + issueType: 'test-issue-type', + pagination: { + items: [{ + current: true, + href: '/organisations/test-lpa/test-dataset/test-issue-type/test-issue-field/1', + number: 1, + type: 'number' + }] + }, + pageNumber: 1, + dataRange: { + minRow: 0, + maxRow: 50, + totalRows: 150 + } + } + } - }) + const res = { + render: vi.fn() + } - describe('getIssueDetails', () => { + getIssueDetails(req, res, () => {}) + expect(res.render).toHaveBeenCalledTimes(1) + expect(res.render).toHaveBeenCalledWith('organisations/issueDetails.html', req.templateParams) + }) }) }) From eab56958c5217da794dd7801940c08dd8e56d29e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 14:36:11 +0000 Subject: [PATCH 21/33] add setRecordCount tests --- src/middleware/dataview.middleware.js | 2 +- .../entryIssueDetails.middleware.js | 2 +- src/middleware/issueTable.middleware.js | 2 +- .../middleware/dataview.middleware.test.js | 38 ++++++++++++++++++- .../middleware/issueTable.middleware.test.js | 37 +++++++++++++++++- 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index b2559ecb..a63978d3 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -28,7 +28,7 @@ export const fetchEntities = fetchMany({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req.entityCount.count + req.recordCount = req?.entityCount?.count next() } diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 0c31868e..9ab8eb94 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -54,7 +54,7 @@ const fetchEntryIssues = fetchMany({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req.issues.length + req.recordCount = req?.issues?.length next() } diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index 483319a2..f44a72fd 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -25,7 +25,7 @@ const validateIssueTableQueryParams = validateQueryParams({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req.issues.length + req.recordCount = req?.issues?.length next() } diff --git a/test/unit/middleware/dataview.middleware.test.js b/test/unit/middleware/dataview.middleware.test.js index a34aad3b..4b5c584e 100644 --- a/test/unit/middleware/dataview.middleware.test.js +++ b/test/unit/middleware/dataview.middleware.test.js @@ -1,10 +1,46 @@ import { describe, it, expect, vi } from 'vitest' import { constructTableParams, - prepareTemplateParams + prepareTemplateParams, + setRecordCount } from '../../../src/middleware/dataview.middleware' describe('dataview.middleware.test.js', () => { + describe('set record count', () => { + it('sets req.recordCount to the length of req.issues', () => { + const req = { entityCount: { count: 3 } } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(3) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.entityCount is undefined', () => { + const req = {} + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.entityCount is null', () => { + const req = { entityCount: null } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + }) + describe('constructTableParams', () => { it('constructs table parameters with correct columns, fields, and rows', () => { const req = { diff --git a/test/unit/middleware/issueTable.middleware.test.js b/test/unit/middleware/issueTable.middleware.test.js index ce93e013..4c46e37d 100644 --- a/test/unit/middleware/issueTable.middleware.test.js +++ b/test/unit/middleware/issueTable.middleware.test.js @@ -1,9 +1,44 @@ import { describe, it, expect, vi } from 'vitest' -import { issueTypeAndFieldShouldRedirect, notIssueHasEntity, prepareTableParams, prepareTemplateParams, redirectToEntityView } from '../../../src/middleware/issueTable.middleware' +import { issueTypeAndFieldShouldRedirect, notIssueHasEntity, prepareTableParams, prepareTemplateParams, redirectToEntityView, setRecordCount } from '../../../src/middleware/issueTable.middleware' vi.mock('../../../src/services/performanceDbApi.js') describe('issueTableMiddleware', () => { + describe('set record count', () => { + it('sets req.recordCount to the length of req.issues', () => { + const req = { issues: [{}, {}, {}] } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(3) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.issues is undefined', () => { + const req = {} + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('does not modify req.recordCount if req.issues is null', () => { + const req = { issues: null } + const res = {} + const next = vi.fn() + + setRecordCount(req, res, next) + + expect(req.recordCount).toBe(undefined) + expect(next).toHaveBeenCalledTimes(1) + }) + }) + describe('prepareTableParams', () => { const req = { entities: [ From 8d078573026ca57e2904fa8881fee36ee5de3fd8 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 15:27:01 +0000 Subject: [PATCH 22/33] set default record count values --- src/middleware/dataview.middleware.js | 2 +- src/middleware/entityIssueDetails.middleware.js | 2 +- src/middleware/entryIssueDetails.middleware.js | 2 +- src/middleware/issueTable.middleware.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/middleware/dataview.middleware.js b/src/middleware/dataview.middleware.js index a63978d3..9cbaeaf8 100644 --- a/src/middleware/dataview.middleware.js +++ b/src/middleware/dataview.middleware.js @@ -28,7 +28,7 @@ export const fetchEntities = fetchMany({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req?.entityCount?.count + req.recordCount = req?.entityCount?.count || 0 next() } diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 5f9e5b9b..8c9694a6 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -62,7 +62,7 @@ export const getIssueField = (text, html, classes) => { } export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issues?.length + req.recordCount = req?.issues?.length || 0 next() } diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 9ab8eb94..e6ace7d4 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -54,7 +54,7 @@ const fetchEntryIssues = fetchMany({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issues?.length + req.recordCount = req?.issues?.length || 0 next() } diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index f44a72fd..1715f061 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -25,7 +25,7 @@ const validateIssueTableQueryParams = validateQueryParams({ }) export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issues?.length + req.recordCount = req?.issues?.length || 0 next() } From 299eeacf29a822f873c1f7f6751e57e92ac6c36e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 3 Dec 2024 15:31:07 +0000 Subject: [PATCH 23/33] fix tests --- test/unit/middleware/dataview.middleware.test.js | 8 ++++---- .../unit/middleware/entityIssueDetails.middleware.test.js | 8 ++++---- test/unit/middleware/issueTable.middleware.test.js | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/unit/middleware/dataview.middleware.test.js b/test/unit/middleware/dataview.middleware.test.js index 4b5c584e..26c52820 100644 --- a/test/unit/middleware/dataview.middleware.test.js +++ b/test/unit/middleware/dataview.middleware.test.js @@ -18,25 +18,25 @@ describe('dataview.middleware.test.js', () => { expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.entityCount is undefined', () => { + it('sets the record count to 0 if req.entityCount is undefined', () => { const req = {} const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.entityCount is null', () => { + it('sets the record count to 0 if req.entityCount is null', () => { const req = { entityCount: null } const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) }) diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index dd712fd2..127de665 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -43,25 +43,25 @@ describe('issueDetails.middleware.js', () => { expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.issues is undefined', () => { + it('sets the record count to 0 if req.issues is undefined', () => { const req = {} const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.issues is null', () => { + it('sets the record count to 0 if req.issues is null', () => { const req = { issues: null } const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) }) diff --git a/test/unit/middleware/issueTable.middleware.test.js b/test/unit/middleware/issueTable.middleware.test.js index 4c46e37d..70e17795 100644 --- a/test/unit/middleware/issueTable.middleware.test.js +++ b/test/unit/middleware/issueTable.middleware.test.js @@ -16,25 +16,25 @@ describe('issueTableMiddleware', () => { expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.issues is undefined', () => { + it('sets the record count to 0 if req.issues is undefined', () => { const req = {} const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) - it('does not modify req.recordCount if req.issues is null', () => { + it('sets the record count to 0 if req.issues is null', () => { const req = { issues: null } const res = {} const next = vi.fn() setRecordCount(req, res, next) - expect(req.recordCount).toBe(undefined) + expect(req.recordCount).toBe(0) expect(next).toHaveBeenCalledTimes(1) }) }) From 990e6d9478dab739433504028105f309196508b4 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 09:59:38 +0000 Subject: [PATCH 24/33] use row number instead of entity number in error message and also get issue count --- src/middleware/common.middleware.js | 34 ++++++++++++++----- .../entityIssueDetails.middleware.js | 2 +- .../entryIssueDetails.middleware.js | 18 ++++++++-- src/middleware/issueTable.middleware.js | 2 +- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 0e31fabc..a7579c71 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -493,6 +493,18 @@ 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 + + return { ...issue, reference } + }) + + next() +} + /** * This middleware chain is responsible for retrieving all entities for the given organisation, their latest issues, * filtering out issues that have been fixed, and constructing the table params. @@ -510,7 +522,8 @@ export const processRelevantIssuesMiddlewares = [ FilterOutIssuesToMostRecent, removeIssuesThatHaveBeenFixed, fetchFieldMappings, - addFieldMappingsToIssue + addFieldMappingsToIssue, + addReferencesToIssues ] // Other @@ -560,12 +573,13 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { export function getErrorSummaryItems (req, res, next) { const { issue_type: issueType, issue_field: issueField } = req.params - const { baseSubpath, issues, entities, resources } = req + const { baseSubpath, issues, issueCount, entities, resources } = req let errorHeading = '' let issueItems const totalRecordCount = entities ? entities.length : resources[0].entry_count + const totalIssues = issueCount?.count || issues.length if (issues.length <= 0) { // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. @@ -573,14 +587,18 @@ 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: issues.length, entityCount: totalRecordCount, field: issueField }, true) + errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) issueItems = issues.map((issue, i) => { const pageNum = i + 1 let inString = '' - if (issue.entity) { - inString = ` in entity ${issue.entity}` - } else if (issue.entry_number) { - inString = ` in entry ${issue.entry_number}` + + // we have to hard code this in because though the entitiy has beeb assigned, it has been assigned incorrectly + const specialIssueTypeCases = ['reference values are not unique'] + + if (issue.reference && !specialIssueTypeCases.includes(issue.issue_type)) { + inString = ` in entity with reference ${issue.reference}` + } else if (issue.line_number) { + inString = ` in row ${issue.line_number}` } return { html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + inString, @@ -589,7 +607,7 @@ export function getErrorSummaryItems (req, res, next) { }) } else { issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: issues.length, entityCount: totalRecordCount, field: issueField }, true) + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) }] } diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index b8857570..1a491849 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -117,8 +117,8 @@ export default [ fetchOrgInfo, fetchDatasetInfo, fetchResources, - ...processRelevantIssuesMiddlewares, ...processEntitiesMiddlewares, + ...processRelevantIssuesMiddlewares, ...processSpecificationMiddlewares, setRecordCount, getSetDataRange(1), diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 84c53808..182bda35 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -1,6 +1,6 @@ import * as v from 'valibot' import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, prepareIssueDetailsTemplateParams, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' -import { fetchMany, FetchOptions, renderTemplate } from './middleware.builders.js' +import { fetchMany, fetchOne, FetchOptions, renderTemplate } from './middleware.builders.js' import { issueErrorMessageHtml } from '../utils/utils.js' export const IssueDetailsQueryParams = v.object({ @@ -54,8 +54,21 @@ const fetchEntryIssues = fetchMany({ result: 'issues' }) +const fetchIssueCount = fetchOne({ + query: ({ req, params }) => ` + select count(*) as count + 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: 'issueCount' +}) + export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issues?.length || 0 + req.recordCount = req?.issueCount?.count || 0 next() } @@ -122,6 +135,7 @@ export default [ fetchResourceMetaData, addResourceMetaDataToResources, fetchEntryIssues, + fetchIssueCount, setRecordCount, getSetDataRange(1), show404IfPageNumberNotInRange, diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index 1715f061..9b0d099b 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -125,8 +125,8 @@ export default [ fetchOrgInfo, fetchDatasetInfo, fetchResources, - ...processRelevantIssuesMiddlewares, ...processEntitiesMiddlewares, + ...processRelevantIssuesMiddlewares, ...processSpecificationMiddlewares, onlyIf(notIssueHasEntity, redirectToEntityView), setRecordCount, From e0105e189e6295ea2c05bf33c0b834bf154cbcd1 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 10:27:15 +0000 Subject: [PATCH 25/33] fix tests --- test/unit/middleware/common.middleware.test.js | 16 ++++++++-------- .../entryIssueDetails.middleware.test.js | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index ee6b4e59..b4167c9f 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1382,8 +1382,8 @@ describe('getErrorSummaryItems', () => { { reference: 'entity3', name: 'Name 3', amount: 300 } ], issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } + { entity: 'entity1', error: 'Invalid Amount', reference: 'entity1' }, + { entity: 'entity2', error: ' Invalid Name', reference: 'entity2' } ] } @@ -1394,11 +1394,11 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.items).toEqual([ { - html: 'issue in entity entity1', + html: 'issue in entity with reference entity1', href: 'baseSubpath-value/entity/1' }, { - html: 'issue in entity entity2', + html: 'issue in entity with reference entity2', href: 'baseSubpath-value/entity/2' } ]) @@ -1417,8 +1417,8 @@ describe('getErrorSummaryItems', () => { } ], issues: [ - { entry_number: 1, error: 'Invalid Amount' }, - { entry_number: 2, error: ' Invalid Name' } + { entry_number: 1, error: 'Invalid Amount', reference: '1' }, + { entry_number: 2, error: ' Invalid Name', reference: '2' } ] } @@ -1429,11 +1429,11 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.items).toEqual([ { - html: 'issue in entry 1', + html: 'issue in entity with reference 1', href: 'baseSubpath-value/entry/1' }, { - html: 'issue in entry 2', + html: 'issue in entity with reference 2', href: 'baseSubpath-value/entry/2' } ]) diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 6234691a..32ead566 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -64,7 +64,7 @@ describe('entryIssueDetails.middleware.test.js', () => { describe('setRecordCount', () => { it('should set req.recordCount to req.issues.length', () => { - const req = { issues: [{}, {}, {}] } + const req = { issueCount: { count: 3 } } const res = {} const next = vi.fn() From 4fffd7d949eebf4211232b66f07eb9a1e2414ef8 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Wed, 4 Dec 2024 10:39:55 +0000 Subject: [PATCH 26/33] Update src/middleware/common.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/common.middleware.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index a7579c71..fe2e0b92 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -560,6 +560,16 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { const { recordCount } = req const { pageNumber } = req.parsedParams + if (typeof recordCount !== 'number' || recordCount < 0) { + return next(new Error('Invalid record count')) + } + if (typeof pageNumber !== 'number' || pageNumber < 1) { + return next(new Error('Invalid page number')) + } + if (typeof pageLength !== 'number' || pageLength < 1) { + return next(new Error('Invalid page length')) + } + req.dataRange = { minRow: (pageNumber - 1) * pageLength, maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), From 9e5cae4b51e69bb36da1ed38de5e5c9d938ebce2 Mon Sep 17 00:00:00 2001 From: GeorgeGoodall-GovUk Date: Wed, 4 Dec 2024 10:40:26 +0000 Subject: [PATCH 27/33] Update src/middleware/common.middleware.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/middleware/common.middleware.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index fe2e0b92..272959af 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -543,13 +543,19 @@ export const setDefaultParams = (req, res, next) => { export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => { const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params + const encodedParams = { + lpa: encodeURIComponent(lpa), + dataset: encodeURIComponent(dataset), + issueType: encodeURIComponent(issueType), + issueField: encodeURIComponent(issueField) + } const additionalPartsEncoded = additionalParts.map(part => encodeURIComponent(part)) let path = '/organisations' - if (lpa) path += `/${lpa}` - if (dataset) path += `/${dataset}` - if (issueType) path += `/${issueType}` - if (issueField) path += `/${issueField}` + if (lpa) path += `/${encodedParams.lpa}` + if (dataset) path += `/${encodedParams.dataset}` + if (issueType) path += `/${encodedParams.issueType}` + if (issueField) path += `/${encodedParams.issueField}` if (additionalPartsEncoded.length > 0) path += `/${additionalPartsEncoded.join('/')}` req.baseSubpath = path From 6df55ba26a7dd7389c4a2e219028eec5f3669e93 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 10:56:51 +0000 Subject: [PATCH 28/33] refactor getSetBaseSubPath --- src/middleware/common.middleware.js | 30 ++++++++++++----------------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 272959af..45ea840d 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -541,24 +541,18 @@ export const setDefaultParams = (req, res, next) => { } export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => { - const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params - - const encodedParams = { - lpa: encodeURIComponent(lpa), - dataset: encodeURIComponent(dataset), - issueType: encodeURIComponent(issueType), - issueField: encodeURIComponent(issueField) - } - const additionalPartsEncoded = additionalParts.map(part => encodeURIComponent(part)) - - let path = '/organisations' - if (lpa) path += `/${encodedParams.lpa}` - if (dataset) path += `/${encodedParams.dataset}` - if (issueType) path += `/${encodedParams.issueType}` - if (issueField) path += `/${encodedParams.issueField}` - if (additionalPartsEncoded.length > 0) path += `/${additionalPartsEncoded.join('/')}` - - req.baseSubpath = path + const params = [ + req.params.lpa, + req.params.dataset, + req.params.issue_type, + req.params.issue_field, + ...additionalParts.map(encodeURIComponent) + ] + + req.baseSubpath = params.reduce( + (path, param) => (param ? `${path}/${param}` : path), + '/organisations' + ) next() } From 06afa6835d4000221bf4b2db7ffd445aa5a6e08e Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 11:33:11 +0000 Subject: [PATCH 29/33] set data range based on entity count --- src/middleware/entityIssueDetails.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 1a491849..86c04ee0 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -52,7 +52,7 @@ export const getIssueField = (text, html, classes) => { } export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issues?.length || 0 + req.recordCount = req?.entities?.length || 0 next() } From fb737e4e1d82a84b74c63a322b3d0c3d1b69d545 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 12:02:10 +0000 Subject: [PATCH 30/33] filter down entities to only those with issues --- src/middleware/common.middleware.js | 10 +++ .../entityIssueDetails.middleware.js | 10 ++- src/middleware/issueTable.middleware.js | 7 +- .../unit/middleware/common.middleware.test.js | 73 ++++++++++++++++++- .../entityIssueDetails.middleware.test.js | 6 +- .../middleware/issueTable.middleware.test.js | 4 + 6 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index 45ea840d..def10d2d 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -354,6 +354,16 @@ export const processEntitiesMiddlewares = [ replaceUnderscoreInEntities ] +export const filterOutEntitiesWithoutIssues = (req, res, next) => { + const { entities, issues } = req + + req.issueEntities = entities.filter(entity => { + return issues.findIndex(issue => issue.entity === entity.entity) >= 0 + }) + + next() +} + // entity issues const fetchEntityIssuesForFieldAndType = fetchMany({ diff --git a/src/middleware/entityIssueDetails.middleware.js b/src/middleware/entityIssueDetails.middleware.js index 86c04ee0..c917c344 100644 --- a/src/middleware/entityIssueDetails.middleware.js +++ b/src/middleware/entityIssueDetails.middleware.js @@ -13,7 +13,8 @@ import { getSetBaseSubPath, getSetDataRange, getErrorSummaryItems, - prepareIssueDetailsTemplateParams + prepareIssueDetailsTemplateParams, + filterOutEntitiesWithoutIssues } from './common.middleware.js' import { renderTemplate } from './middleware.builders.js' import * as v from 'valibot' @@ -52,15 +53,15 @@ export const getIssueField = (text, html, classes) => { } export const setRecordCount = (req, res, next) => { - req.recordCount = req?.entities?.length || 0 + req.recordCount = req?.issueEntities?.length || 0 next() } export function prepareEntity (req, res, next) { - const { entities, issues, specification } = req + const { issueEntities, issues, specification } = req const { pageNumber, issue_type: issueType } = req.parsedParams - const entityData = entities[pageNumber - 1] + const entityData = issueEntities[pageNumber - 1] const entityIssues = issues.filter(issue => issue.entity === entityData.entity) const fields = specification.fields.map(({ field, datasetField }) => { @@ -120,6 +121,7 @@ export default [ ...processEntitiesMiddlewares, ...processRelevantIssuesMiddlewares, ...processSpecificationMiddlewares, + filterOutEntitiesWithoutIssues, setRecordCount, getSetDataRange(1), show404IfPageNumberNotInRange, diff --git a/src/middleware/issueTable.middleware.js b/src/middleware/issueTable.middleware.js index 9b0d099b..bd6a9a87 100644 --- a/src/middleware/issueTable.middleware.js +++ b/src/middleware/issueTable.middleware.js @@ -7,7 +7,7 @@ */ import config from '../../config/index.js' -import { createPaginationTemplateParams, fetchDatasetInfo, fetchOrgInfo, fetchResources, getErrorSummaryItems, getSetBaseSubPath, getSetDataRange, processEntitiesMiddlewares, processRelevantIssuesMiddlewares, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.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' @@ -30,9 +30,9 @@ export const setRecordCount = (req, res, next) => { } export const prepareTableParams = (req, res, next) => { - const { entities, issues, uniqueDatasetFields, dataRange, baseSubpath } = req + const { issueEntities, issues, uniqueDatasetFields, dataRange, baseSubpath } = req - const allRows = entities.map((entity, index) => ({ + const allRows = issueEntities.map((entity, index) => ({ columns: Object.fromEntries(uniqueDatasetFields.map((field) => { const errorMessage = issues.find(issue => issue.entity === entity.entity && (issue.field === field || issue.replacement_field === field))?.issue_type if (field === 'reference') { @@ -129,6 +129,7 @@ export default [ ...processRelevantIssuesMiddlewares, ...processSpecificationMiddlewares, onlyIf(notIssueHasEntity, redirectToEntityView), + filterOutEntitiesWithoutIssues, setRecordCount, getSetDataRange(config.tablePageLength), show404IfPageNumberNotInRange, diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index b4167c9f..bd4b199b 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 { 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, FilterOutIssuesToMostRecent, 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' @@ -1480,3 +1480,74 @@ describe('prepareEntityIssueDetailsTemplateParams', () => { expect(next).toHaveBeenCalledTimes(1) }) }) + +describe('filterOutEntitiesWithoutIssues middleware', () => { + it('should filter out entities without issues', () => { + const entities = [ + { entity: 'entity1' }, + { entity: 'entity2' }, + { entity: 'entity3' } + ] + + const issues = [ + { entity: 'entity1', issue: 'issue1' }, + { entity: 'entity2', issue: 'issue2' } + ] + + const req = { entities, issues } + const res = {} + const next = vi.fn() + + filterOutEntitiesWithoutIssues(req, res, next) + + expect(req.issueEntities).toEqual([ + { entity: 'entity1' }, + { entity: 'entity2' } + ]) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('should return an empty array if no entities have issues', () => { + const entities = [ + { entity: 'entity1' }, + { entity: 'entity2' }, + { entity: 'entity3' } + ] + + const issues = [] + + const req = { entities, issues } + const res = {} + const next = vi.fn() + + filterOutEntitiesWithoutIssues(req, res, next) + + expect(req.issueEntities).toEqual([]) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('should not modify the original entities array', () => { + const entities = [ + { entity: 'entity1' }, + { entity: 'entity2' }, + { entity: 'entity3' } + ] + + const issues = [ + { entity: 'entity1', issue: 'issue1' }, + { entity: 'entity2', issue: 'issue2' } + ] + + const req = { entities, issues } + const res = {} + const next = vi.fn() + + filterOutEntitiesWithoutIssues(req, res, next) + + expect(entities).toEqual([ + { entity: 'entity1' }, + { entity: 'entity2' }, + { entity: 'entity3' } + ]) + }) +}) diff --git a/test/unit/middleware/entityIssueDetails.middleware.test.js b/test/unit/middleware/entityIssueDetails.middleware.test.js index b79ab7aa..8dc73631 100644 --- a/test/unit/middleware/entityIssueDetails.middleware.test.js +++ b/test/unit/middleware/entityIssueDetails.middleware.test.js @@ -33,7 +33,7 @@ describe('issueDetails.middleware.js', () => { describe('set record count', () => { it('sets req.recordCount to the length of req.issues', () => { - const req = { issues: [{}, {}, {}] } + const req = { issueEntities: [{}, {}, {}] } const res = {} const next = vi.fn() @@ -75,6 +75,10 @@ describe('issueDetails.middleware.js', () => { { entity: 'entity1', name: 'Entity 1', field1: 'value1', field2: 'value2' }, { entity: 'entity2', name: 'Entity 2', field1: 'value3', field2: 'value4' } ] + req.issueEntities = [ + { entity: 'entity1', name: 'Entity 1', field1: 'value1', field2: 'value2' }, + { entity: 'entity2', name: 'Entity 2', field1: 'value3', field2: 'value4' } + ] req.issues = [ { entity: 'entity1', field: 'field1', message: 'Error 1' }, { entity: 'entity1', field: 'field2', message: 'Error 2' }, diff --git a/test/unit/middleware/issueTable.middleware.test.js b/test/unit/middleware/issueTable.middleware.test.js index 70e17795..fbb6f2a8 100644 --- a/test/unit/middleware/issueTable.middleware.test.js +++ b/test/unit/middleware/issueTable.middleware.test.js @@ -46,6 +46,10 @@ describe('issueTableMiddleware', () => { { entity: 'entity2', reference: 'entity2', name: 'Name 2', amount: 200 }, { entity: 'entity3', reference: 'entity3', name: 'Name 3', amount: 300 } ], + issueEntities: [ + { entity: 'entity1', reference: 'entity1', name: 'Name 1', amount: 100 }, + { entity: 'entity2', reference: 'entity2', name: 'Name 2', amount: 200 } + ], issues: [ { entity: 'entity1', field: 'amount', issue_type: 'Invalid Amount' }, { entity: 'entity2', field: 'name', issue_type: 'Invalid Name' } From 0883bc1a479a1c4e29c2e5991d63cf30f545c6fc Mon Sep 17 00:00:00 2001 From: George Goodall Date: Wed, 4 Dec 2024 12:05:20 +0000 Subject: [PATCH 31/33] content changes re alex feedback --- src/middleware/common.middleware.js | 2 +- src/middleware/entryIssueDetails.middleware.js | 4 ++-- test/unit/middleware/entryIssueDetails.middleware.test.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index def10d2d..ebc62aed 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -618,7 +618,7 @@ export function getErrorSummaryItems (req, res, next) { if (issue.reference && !specialIssueTypeCases.includes(issue.issue_type)) { inString = ` in entity with reference ${issue.reference}` } else if (issue.line_number) { - inString = ` in row ${issue.line_number}` + inString = ` on row ${issue.line_number}` } return { html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + inString, diff --git a/src/middleware/entryIssueDetails.middleware.js b/src/middleware/entryIssueDetails.middleware.js index 182bda35..8f6cf9fc 100644 --- a/src/middleware/entryIssueDetails.middleware.js +++ b/src/middleware/entryIssueDetails.middleware.js @@ -91,7 +91,7 @@ export const prepareEntry = (req, res, next) => { fields: [ { key: { - text: 'Endpoint' + text: 'Endpoint URL' }, value: { html: `${resources[0].endpoint_url}` @@ -100,7 +100,7 @@ export const prepareEntry = (req, res, next) => { }, { key: { - text: 'Line number' + text: 'Row' }, value: { html: issue.line_number.toString() diff --git a/test/unit/middleware/entryIssueDetails.middleware.test.js b/test/unit/middleware/entryIssueDetails.middleware.test.js index 32ead566..1ac73d14 100644 --- a/test/unit/middleware/entryIssueDetails.middleware.test.js +++ b/test/unit/middleware/entryIssueDetails.middleware.test.js @@ -99,12 +99,12 @@ describe('entryIssueDetails.middleware.test.js', () => { title: 'entry: 1', fields: [ { - key: { text: 'Endpoint' }, + key: { text: 'Endpoint URL' }, value: { html: 'https://example.com' }, classes: '' }, { - key: { text: 'Line number' }, + key: { text: 'Row' }, value: { html: '2' }, classes: '' }, From 773c5444aa7d5ca8633cf47992c3af568821b564 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 5 Dec 2024 13:52:14 +0000 Subject: [PATCH 32/33] fix pagination showing value --- src/views/organisations/dataview.html | 2 +- src/views/organisations/issueTable.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/views/organisations/dataview.html b/src/views/organisations/dataview.html index eff57629..343e7879 100644 --- a/src/views/organisations/dataview.html +++ b/src/views/organisations/dataview.html @@ -61,7 +61,7 @@ {% if pagination.items | length > 1 %}
-

Showing rows {{dataRange.minRow}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

+

Showing rows {{dataRange.minRow + 1}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

{{ govukPagination(pagination) }}
diff --git a/src/views/organisations/issueTable.html b/src/views/organisations/issueTable.html index 30dfef4c..d9e838fe 100644 --- a/src/views/organisations/issueTable.html +++ b/src/views/organisations/issueTable.html @@ -84,7 +84,7 @@ {% if pagination.items | length > 1 %}
-

Showing rows {{dataRange.minRow}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

+

Showing rows {{dataRange.minRow + 1}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

{{ govukPagination(pagination) }}
From b91dc88aca0bb6e03d76aa0d67ca70ec3e8ce7e6 Mon Sep 17 00:00:00 2001 From: George Goodall Date: Thu, 5 Dec 2024 14:06:01 +0000 Subject: [PATCH 33/33] standardise error message summary --- src/middleware/common.middleware.js | 38 ++------ .../unit/middleware/common.middleware.test.js | 86 ++++--------------- 2 files changed, 22 insertions(+), 102 deletions(-) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index ebc62aed..2f1958ae 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -593,43 +593,15 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { export function getErrorSummaryItems (req, res, next) { const { issue_type: issueType, issue_field: issueField } = req.params - const { baseSubpath, issues, issueCount, entities, resources } = req - - let errorHeading = '' - let issueItems + const { issues, issueCount, entities, resources } = req const totalRecordCount = entities ? entities.length : resources[0].entry_count const totalIssues = issueCount?.count || issues.length - if (issues.length <= 0) { - // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) - 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) - issueItems = issues.map((issue, i) => { - const pageNum = i + 1 - let inString = '' - - // we have to hard code this in because though the entitiy has beeb assigned, it has been assigned incorrectly - const specialIssueTypeCases = ['reference values are not unique'] - - if (issue.reference && !specialIssueTypeCases.includes(issue.issue_type)) { - inString = ` in entity with reference ${issue.reference}` - } else if (issue.line_number) { - inString = ` on row ${issue.line_number}` - } - return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + inString, - href: `${baseSubpath}/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) - }] - } + const errorHeading = '' + const issueItems = [{ + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) + }] req.errorSummary = { heading: errorHeading, diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index bd4b199b..7af60dd6 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1315,10 +1315,15 @@ describe('getErrorSummaryItems', () => { getErrorSummaryItems(req, null, next) - expect(next).toHaveBeenCalledWith(new Error('issue count must be larger than 0')) + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type-value', + num_issues: 0, + entityCount: 0, + field: 'issue-field-value' + }, true) }) - it('handles if every entity has the issue', () => { + it('handles if entities are provided', () => { const req = { params: { issue_type: 'issue-type-value', @@ -1341,70 +1346,16 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.heading).toBe('') - }) - it('handles some entities not having the issue', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value' - }, - baseSubpath: 'baseSubpath-value', - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.heading).toBe('message') + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type-value', + num_issues: 2, + entityCount: 2, + field: 'issue-field-value' + }, true) }) - it('Correctly sets the issue items when some entities dont have issues', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value' - }, - baseSubpath: 'baseSubpath-value/entity', - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount', reference: 'entity1' }, - { entity: 'entity2', error: ' Invalid Name', reference: 'entity2' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('issue') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.items).toEqual([ - { - html: 'issue in entity with reference entity1', - href: 'baseSubpath-value/entity/1' - }, - { - html: 'issue in entity with reference entity2', - href: 'baseSubpath-value/entity/2' - } - ]) - }) - - it('handles no entities provided, but a resource provided with less issues than entries', () => { + it('handles no entities provided, but a resource provided', () => { const req = { params: { issue_type: 'issue-type-value', @@ -1429,14 +1380,11 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.items).toEqual([ { - html: 'issue in entity with reference 1', - href: 'baseSubpath-value/entry/1' - }, - { - html: 'issue in entity with reference 2', - href: 'baseSubpath-value/entry/2' + html: 'issue' } ]) + + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ issue_type: 'issue-type-value', num_issues: 2, entityCount: 3, field: 'issue-field-value' }, true) }) })