Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

684 issue details fetch data from all active resources #707

Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
7562584
update issue details to entity issue details
GeorgeGoodall Nov 29, 2024
1ab09ee
add entryIssueDetailsMiddleware and route
GeorgeGoodall Nov 29, 2024
56111d3
Updated entity and entry issue details middleware: refactored code, r…
GeorgeGoodall Nov 29, 2024
2b017dd
dont filter out issues without an entity, and also only get issues of…
GeorgeGoodall Nov 29, 2024
d92501a
only output spec fields
GeorgeGoodall Nov 29, 2024
03238bd
fix query
GeorgeGoodall Nov 29, 2024
cd403bf
use already set variable
GeorgeGoodall Nov 29, 2024
51f89c9
remove comment
GeorgeGoodall Nov 29, 2024
3b37099
moduleize getBaseSubPath
GeorgeGoodall Dec 2, 2024
0c05c9b
modulized setDataRange
GeorgeGoodall Dec 2, 2024
d5969d0
modulize getErrorSummaryItems
GeorgeGoodall Dec 2, 2024
e26aa37
Merge commit '9952399edf04b9de3e24e31750ab1204638376df' into 684-issu…
GeorgeGoodall Dec 2, 2024
8e14099
fixed tests and removed not needed tests
GeorgeGoodall Dec 2, 2024
8fe4abd
fix existing tests and add tests to common.middleware.test.js
GeorgeGoodall Dec 3, 2024
7baf8d0
added baseSubPath tests
GeorgeGoodall Dec 3, 2024
67a5614
setRecordCount tests
GeorgeGoodall Dec 3, 2024
e8c58ce
add tests for prepareEntity
GeorgeGoodall Dec 3, 2024
4963474
add tests for prepareEntityIssueDetailsTemplateParams
GeorgeGoodall Dec 3, 2024
a52ec8d
add tests for addResourceMetaDataToResources
GeorgeGoodall Dec 3, 2024
4886ae9
move prepare issue details middleware to common
GeorgeGoodall Dec 3, 2024
b424b4d
add tests for getIssueDetails
GeorgeGoodall Dec 3, 2024
eab5695
add setRecordCount tests
GeorgeGoodall Dec 3, 2024
1f442ff
Merge commit '281a2bdc523825310fa56b1cc8aeb02b29a6cb0f' into 684-issu…
GeorgeGoodall Dec 3, 2024
dd6b4fd
Merge commit 'de9d791250893f75897cc666cee20ba6ff043da9' into 684-issu…
GeorgeGoodall Dec 3, 2024
36f6f95
Merge branch 'main' into 684-issue-details-fetch-data-from-all-active…
GeorgeGoodall-GovUk Dec 3, 2024
4200d2f
Merge branch 'main' into 684-issue-details-fetch-data-from-all-active…
GeorgeGoodall-GovUk Dec 3, 2024
8d07857
set default record count values
GeorgeGoodall Dec 3, 2024
299eeac
fix tests
GeorgeGoodall Dec 3, 2024
b049f62
Merge remote-tracking branch 'origin/507-rows-with-issues-should-play…
GeorgeGoodall Dec 3, 2024
7f9b98e
Merge commit '600bb319ac2f4921a2e34be1ca70ce186e211479' into 684-issu…
GeorgeGoodall Dec 3, 2024
990e6d9
use row number instead of entity number in error message and also get…
GeorgeGoodall Dec 4, 2024
e0105e1
fix tests
GeorgeGoodall Dec 4, 2024
4fffd7d
Update src/middleware/common.middleware.js
GeorgeGoodall-GovUk Dec 4, 2024
9e5cae4
Update src/middleware/common.middleware.js
GeorgeGoodall-GovUk Dec 4, 2024
6df55ba
refactor getSetBaseSubPath
GeorgeGoodall Dec 4, 2024
06afa68
set data range based on entity count
GeorgeGoodall Dec 4, 2024
fb737e4
filter down entities to only those with issues
GeorgeGoodall Dec 4, 2024
0883bc1
content changes re alex feedback
GeorgeGoodall Dec 4, 2024
bb78185
Merge commit '62071268364fc9b770280436f806cae3ea44a471' into 684-issu…
GeorgeGoodall Dec 5, 2024
773c544
fix pagination showing value
GeorgeGoodall Dec 5, 2024
b91dc88
standardise error message summary
GeorgeGoodall Dec 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/controllers/OrganisationsController.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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 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'
Expand All @@ -11,7 +12,8 @@ const organisationsController = {
organisationsMiddleware,
datasetTaskListMiddleware,
datasetOverviewMiddleware,
issueDetailsMiddleware,
entityIssueDetailsMiddleware,
entryIssueDetailsMiddleware,
issueTableMiddleware,
getStartedMiddleware,
overviewMiddleware,
Expand Down
136 changes: 134 additions & 2 deletions src/middleware/common.middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
GeorgeGoodall-GovUk marked this conversation as resolved.
Show resolved Hide resolved
WHERE ro.organisation = '${req.params.lpa}'
AND rd.dataset = '${req.params.dataset}'
AND r.end_date = ''
Expand Down Expand Up @@ -353,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({
Expand Down Expand Up @@ -492,6 +503,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()
}
Comment on lines +506 to +516
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and error handling

The function should validate its inputs and handle edge cases:

  1. Check if issues and entities arrays exist and are arrays
  2. Handle cases where entity is not found
 export const addReferencesToIssues = (req, res, next) => {
   const { issues, entities } = req
+  
+  if (!Array.isArray(issues)) {
+    logger.warn('no issues array provided to addReferencesToIssues')
+    return next()
+  }
+
+  if (!Array.isArray(entities)) {
+    logger.warn('no entities array provided to addReferencesToIssues')
+    return next()
+  }

   req.issues = issues.map(issue => {
     const reference = entities.find(entity => entity.entity === issue.entity)?.reference
+    if (!reference) {
+      logger.warn(`No matching entity found for issue entity ${issue.entity}`)
+    }

     return { ...issue, reference }
   })

   next()
 }

Committable suggestion skipped: line range outside the PR's diff.


/**
* 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.
Expand All @@ -509,7 +532,8 @@ export const processRelevantIssuesMiddlewares = [
FilterOutIssuesToMostRecent,
removeIssuesThatHaveBeenFixed,
fetchFieldMappings,
addFieldMappingsToIssue
addFieldMappingsToIssue,
addReferencesToIssues
]

// Other
Expand All @@ -525,3 +549,111 @@ export const setDefaultParams = (req, res, next) => {

next()
}

export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => {
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()
}

export const getSetDataRange = (pageLength) => (req, res, next) => {
const { recordCount } = req
const { pageNumber } = req.parsedParams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another middleware to ensure pageNumber is a number and defined?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the reason why I ask this, is because I've seen a lot of errors in sentry related to the pageNumber being undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, valibot takes care of that now and sets a default of 0 (as long as your getting it from the 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),
totalRows: recordCount,
maxPageNumber: Math.ceil(recordCount / pageLength),
pageLength,
offset: (pageNumber - 1) * pageLength
}
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 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)
}]
}

req.errorSummary = {
heading: errorHeading,
items: issueItems
}

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()
}
29 changes: 7 additions & 22 deletions src/middleware/dataview.middleware.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createPaginationTemplateParams, extractJsonFieldFromEntities, fetchDatasetInfo, fetchLatestResource, fetchLpaDatasetIssues, fetchOrgInfo, isResourceAccessible, isResourceIdInParams, processSpecificationMiddlewares, replaceUnderscoreInEntities, setDefaultParams, takeResourceIdFromParams, validateQueryParams, show404IfPageNumberNotInRange } 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'
Expand Down Expand Up @@ -26,25 +27,8 @@ 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 = {
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 || 0
next()
}

Expand Down Expand Up @@ -118,12 +102,13 @@ export default [
validatedataviewQueryParams,
setDefaultParams,

setBaseSubpath,
getSetBaseSubPath(['data']),
fetchOrgInfo,
fetchDatasetInfo,
fetchResourceStatus,
fetchEntitiesCount,
getDataRange,
setRecordCount,
getSetDataRange(config.tablePageLength),
show404IfPageNumberNotInRange,

fetchIf(isResourceIdInParams, fetchLatestResource, takeResourceIdFromParams),
Expand Down
135 changes: 135 additions & 0 deletions src/middleware/entityIssueDetails.middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import { issueErrorMessageHtml } from '../utils/utils.js'
import {
fetchDatasetInfo,
fetchOrgInfo,
logPageError,
validateQueryParams,
createPaginationTemplateParams,
show404IfPageNumberNotInRange,
fetchResources,
processRelevantIssuesMiddlewares,
processEntitiesMiddlewares,
processSpecificationMiddlewares,
getSetBaseSubPath,
getSetDataRange,
getErrorSummaryItems,
prepareIssueDetailsTemplateParams,
filterOutEntitiesWithoutIssues
} 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 {*} text
* @param {*} html
* @param {*} classes
* @returns {{key: {text: string}, value: { html: string}, classes: string}}
*/
export const getIssueField = (text, html, classes) => {
classes = classes || ''
return {
key: {
text
},
value: {
html: html ? html.toString() : ''
},
classes
}
}

export const setRecordCount = (req, res, next) => {
req.recordCount = req?.issueEntities?.length || 0
next()
}

export function prepareEntity (req, res, next) {
const { issueEntities, issues, specification } = req
const { pageNumber, issue_type: issueType } = req.parsedParams

const entityData = issueEntities[pageNumber - 1]
const entityIssues = issues.filter(issue => issue.entity === entityData.entity)

const fields = specification.fields.map(({ field, datasetField }) => {
const value = entityData[field] || entityData[datasetField]

return getIssueField(field, value)
})

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 govuk-form-group--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 govuk-form-group--error'

fields.push(getIssueField(issue.field, valueHtml, classes))
}
}

const geometries = fields
.filter((row) => row.field === 'geometry')
.map((row) => row.value)

req.entry = {
title: entityData.name || `entity: ${entityData.entity}`,
fields,
geometries
}

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,
...processEntitiesMiddlewares,
...processRelevantIssuesMiddlewares,
...processSpecificationMiddlewares,
filterOutEntitiesWithoutIssues,
setRecordCount,
getSetDataRange(1),
show404IfPageNumberNotInRange,
getSetBaseSubPath(['entity']),
createPaginationTemplateParams,
getErrorSummaryItems,
prepareEntity,
prepareIssueDetailsTemplateParams,
getIssueDetails,
logPageError
]
Loading
Loading