Add issue table view#691
Conversation
…issues-table-component-to-issues-details-page-part-2
WalkthroughThe changes in this pull request involve significant enhancements to middleware and routing related to issue management within the application. A new middleware for handling issue tables has been introduced, alongside modifications to existing middleware to improve pagination and data handling. The routing for issue details has been updated to include new paths, and several new schemas have been defined for better data validation. Additionally, the HTML templates have been adjusted to reflect these changes, ensuring a more structured presentation of issue data. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…issues-table-component-to-issues-details-page-attempt-2
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/unit/middleware/common.middleware.test.js (2)
12-94: Consider adding edge case tests for pagination validationThe test suite provides good coverage but would benefit from additional edge cases:
- Maximum safe integer value for page number
- Floating-point page numbers
- String numbers (e.g., "1", "2")
786-976: Add timeout handling tests for datasette queriesConsider adding test cases for:
- Query timeout scenarios
- Network latency simulation
- Rate limiting responses
Example implementation:
it('handles query timeout', async () => { datasette.runQuery.mockImplementation(() => new Promise(resolve => setTimeout(resolve, 5000) )); // Test implementation });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/unit/middleware/common.middleware.test.js(2 hunks)
🔇 Additional comments (3)
test/unit/middleware/common.middleware.test.js (3)
127-142: Update error handling in pagination tests
The test expects direct error throwing, but middleware should pass errors to the next function instead.
464-469: Update error handling in specification tests
The test expects direct error throwing, but middleware should pass errors to the next function instead.
978-1065: Well-structured test suite with comprehensive coverage
The test cases effectively cover:
- Basic functionality
- Edge cases
- Error scenarios
- Undefined/empty inputs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/middleware/issueDetails.middleware.js(5 hunks)
🔇 Additional comments (3)
src/middleware/issueDetails.middleware.js (3)
84-84: Proper null check implemented
The null check for issueEntitiesCount has been correctly implemented as previously suggested.
172-176: LGTM! URL-safe path construction
The function correctly encodes URL parameters to prevent injection vulnerabilities.
288-291: LGTM! Logical middleware ordering
The new middleware functions are added in the correct order, ensuring each has access to the data it needs from previous steps.
…ttempt-2' of github.com:digital-land/submit into 423-add-issues-table-component-to-issues-details-page-attempt-2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/middleware/issueTable.middleware.js (5)
1-7: Consider enhancing documentation with JSDoc formatThe current documentation clearly outlines the middleware's purpose, but converting it to JSDoc format would provide better IDE support and documentation generation capabilities.
-/* - this middleware should get all entities for the organisation, with all of the most recent issues - then filter out any of the issues that have been fixed - the construct the table params - then construct the template params - then render the template -*/ +/** + * Middleware for handling issue table views. + * + * Flow: + * 1. Fetches all entities for the organisation with most recent issues + * 2. Filters out fixed issues + * 3. Constructs table parameters + * 4. Prepares template parameters + * 5. Renders the template + * + * @middleware + */
28-41: Consider extracting page length as a configurable constantThe page length (50) is currently hardcoded. Consider moving this to a configuration file for better maintainability and flexibility.
+// At the top of the file +const DEFAULT_PAGE_LENGTH = 50 + export const getDataRange = (req, res, next) => { const { issues } = req const { pageNumber } = req.parsedParams - const pageLength = 50 + const pageLength = DEFAULT_PAGE_LENGTH
49-91: Consider optimising data processing for better performanceThe current implementation uses nested mapping and filtering operations which could be optimised for better performance with larger datasets.
- const allRows = entities.map((entity, index) => ({ - columns: Object.fromEntries(uniqueDatasetFields.map((field) => { + const errorsByEntityAndField = new Map( + issues.map(issue => [`${issue.entity}-${issue.field}`, issue.issue_type]) + ) + + 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 + const errorMessage = errorsByEntityAndField.get(`${entity.entity}-${field}`)
157-158: Remove commented-out codeThese commented parameters appear to be unused. If they're no longer needed, they should be removed to maintain code cleanliness.
- // issueEntitiesCount, - // pageNumber
165-178: Consider adding TypeScript-style type definitionsAdding type definitions would improve code maintainability and prevent potential errors when modifying redirect conditions.
+/** @typedef {{ type: string, field: string }} RedirectGroup */ +/** @type {RedirectGroup[]} */ const redirectIssueGroups = [ { type: 'missing value', field: 'reference' }, // ... ]src/middleware/common.middleware.js (1)
Line range hint
143-177: Replace unsafeisNaNand improve error handling
- Replace global
isNaNwithNumber.isNaN- Consider adding a more descriptive error message
- if (isNaN(pageNumber) || pageNumber < 1) { + if (Number.isNaN(pageNumber) || pageNumber < 1) { - const error = new Error('Invalid page number') + const error = new Error(`Invalid page number: ${pageNumber}`) return next(error) }🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 143-143: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
test/unit/middleware/common.middleware.test.js (2)
785-867: Add edge cases for date handling in FilterOutIssuesToMostRecent testsWhile the test coverage for date handling is good, consider adding these edge cases:
- Dates in different timezones
- Future dates
- Dates near the Unix epoch
- ISO 8601 date strings with different formats
871-883: Simplify mock implementation for better maintainabilityThe current mock implementation using array iteration could be simplified using a Map for better performance and readability.
Consider this alternative implementation:
- const mockDatasetteQuery = (moreRecentEntityFieldsFacts) => { - datasette.runQuery.mockImplementation((query, dataset) => { - let formattedData = [] - moreRecentEntityFieldsFacts.forEach(([entity, field, resource]) => { - if (query.includes(entity) && query.includes(field) && query.includes(resource)) { - formattedData = [{ entity, field, resource }] - } - }) - return { - formattedData - } - }) - } + const mockDatasetteQuery = (moreRecentEntityFieldsFacts) => { + const mockData = new Map( + moreRecentEntityFieldsFacts.map(([entity, field, resource]) => [ + `${entity}-${field}-${resource}`, + { entity, field, resource } + ]) + ) + datasette.runQuery.mockImplementation((query) => { + const key = Array.from(mockData.keys()).find(k => query.includes(k)) + return { + formattedData: key ? [mockData.get(key)] : [] + } + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/middleware/common.middleware.js(6 hunks)src/middleware/issueTable.middleware.js(1 hunks)test/unit/middleware/common.middleware.test.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/middleware/common.middleware.js
[error] 122-122: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 143-143: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (12)
src/middleware/issueTable.middleware.js (5)
15-26: LGTM! Comprehensive validation schema with proper type transformations
The validation schema effectively handles all required query parameters with appropriate type checking and transformations.
43-47: LGTM! Proper URL encoding for path parameters
The implementation correctly encodes all path parameters, preventing potential XSS vulnerabilities.
194-212: LGTM! Well-structured middleware chain
The middleware chain is well-organised and follows a logical flow from validation through to rendering.
101-106: 🛠️ Refactor suggestion
Enhance error handling with custom error type
The current error handling could be more specific and informative.
+class IssueTableError extends Error {
+ constructor(message, code) {
+ super(message)
+ this.name = 'IssueTableError'
+ this.code = code
+ }
+}
if (issues.length <= 0) {
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')
+ const error = new IssueTableError('No issues found to display', 'NO_ISSUES_FOUND')
return next(error)
}Likely invalid or redundant comment.
102-102: Investigate potential bug in task list page
The comment suggests there might be a bug in how the task list page handles fixed issues.
src/middleware/common.middleware.js (6)
118-132: Replace unsafe isNaN with Number.isNaN
The use of global isNaN is unsafe as it attempts type coercion. Use Number.isNaN instead for more predictable type checking.
- if (!dataRange || !dataRange.maxPageNumber || isNaN(dataRange.maxPageNumber)) {
+ if (!dataRange || !dataRange.maxPageNumber || Number.isNaN(dataRange.maxPageNumber)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
188-198: Prevent SQL injection by parameterising query inputs
User-provided inputs req.params.lpa and req.params.dataset are directly interpolated into the SQL query. This could lead to SQL injection vulnerabilities.
export const fetchResources = fetchMany({
- query: ({ req }) => `
- select * from resource r
- LEFT JOIN resource_organisation ro ON ro.resource = r.resource
- LEFT JOIN resource_dataset rd ON rd.resource = r.resource
- WHERE ro.organisation = '${req.params.lpa}'
- AND rd.dataset = '${req.params.dataset}'
- AND r.end_date = ''
- ORDER BY start_date desc`,
+ query: ({ req }) => ({
+ query: `
+ SELECT * FROM resource r
+ LEFT JOIN resource_organisation ro ON ro.resource = r.resource
+ LEFT JOIN resource_dataset rd ON rd.resource = r.resource
+ WHERE ro.organisation = :organisation
+ AND rd.dataset = :dataset
+ AND r.end_date = ''
+ ORDER BY start_date DESC`,
+ params: {
+ organisation: req.params.lpa,
+ dataset: req.params.dataset
+ }
+ }),
result: 'resources'
})202-205: Prevent SQL injection by parameterising query inputs
The value req.dataset.collection is directly embedded into the SQL query without sanitisation.
export const fetchSpecification = fetchOne({
- query: ({ req }) => `select * from specification WHERE specification = '${req.dataset.collection}'`,
+ query: ({ req }) => ({
+ query: 'SELECT * FROM specification WHERE specification = :specification',
+ params: {
+ specification: req.dataset.collection
+ }
+ }),
result: 'specification'
})358-372: Prevent SQL injection by parameterising query inputs
Parameters params.issue_type and params.issue_field are directly interpolated into the SQL query.
const fetchEntityIssuesForFieldAndType = fetchMany({
- query: ({ req, params }) => {
- const issueTypeFilter = params.issue_type ? `AND issue_type = '${params.issue_type}'` : ''
- const issueFieldFilter = params.issue_field ? `AND field = '${params.issue_field}'` : ''
-
- return `
- SELECT e.entity, i.* FROM entity e
- INNER JOIN issue i ON e.entity = i.entity
- WHERE e.organisation_entity = ${req.orgInfo.entity}
- ${issueTypeFilter}
- ${issueFieldFilter}`
+ query: ({ req, params }) => {
+ const query = `
+ SELECT e.entity, i.* FROM entity e
+ INNER JOIN issue i ON e.entity = i.entity
+ WHERE e.organisation_entity = :organisationEntity
+ ${params.issue_type ? 'AND issue_type = :issueType' : ''}
+ ${params.issue_field ? 'AND field = :issueField' : ''}`
+
+ const queryParams = {
+ organisationEntity: req.orgInfo.entity
+ }
+ if (params.issue_type) queryParams.issueType = params.issue_type
+ if (params.issue_field) queryParams.issueField = params.issue_field
+
+ return { query, params: queryParams }
+ },
dataset: FetchOptions.fromParams,
result: 'issues'
})421-470: Improve error handling and prevent SQL injection
The function has several areas for improvement:
- SQL injection vulnerability in the query
- Performance impact of multiple database queries
- Basic error handling in Promise chain
export const removeIssuesThatHaveBeenFixed = async (req, res, next) => {
const { issues, resources } = req
if (!resources || resources.length <= 0) {
logger.warn('no resources provided for removeIssueThatHaveBeenFixed')
return next()
}
const promises = issues
.filter(issue => issue.resource !== resources[0].resource)
.map((issue) => {
const resourceIndex = resources.findIndex(resource => resource.resource === issue.resource)
const newerResources = resourceIndex >= 0 ? resources.slice(0, resourceIndex) : resources
- return datasette.runQuery(`
- SELECT * FROM fact f
- LEFT JOIN fact_resource fr ON f.fact = fr.fact
- WHERE entity = ${issue.entity}
- AND field = '${issue.field}'
- AND fr.resource IN ('${newerResources.map(resource => resource.resource).join("','")}')
- ORDER BY fr.start_date desc
- LIMIT 1`,
- issue.dataset
- )
+ return datasette.runQuery({
+ query: `
+ SELECT * FROM fact f
+ LEFT JOIN fact_resource fr ON f.fact = fr.fact
+ WHERE entity = :entity
+ AND field = :field
+ AND fr.resource = ANY(:resources)
+ ORDER BY fr.start_date DESC
+ LIMIT 1`,
+ params: {
+ entity: issue.entity,
+ field: issue.field,
+ resources: newerResources.map(resource => resource.resource)
+ }
+ }, issue.dataset)
})
Promise.allSettled(promises)
.then((results) => {
results.forEach(result => {
if (result.status === 'fulfilled') {
if (result.value.formattedData.length > 0) {
req.issues = req.issues.filter(issue =>
issue.entity !== result.value.formattedData[0].entity ||
issue.field !== result.value.formattedData[0].field
)
}
} else {
- logger.warn('request to datasette failed', {
+ logger.error('request to datasette failed', {
error: result.reason,
stack: result.reason.stack
})
}
})
return next()
})
.catch(error => {
- logger.warn('Error in middleware, could not process promise array', {
+ logger.error('Error in middleware, could not process promise array', {
error,
+ stack: error.stack
})
return next(error)
})
}294-300:
Prevent SQL injection by parameterising query inputs
The value req.orgInfo.entity is directly interpolated into the SQL query.
export const fetchEntities = fetchMany({
- query: ({ req }) => `
- SELECT * FROM entity e
- WHERE e.organisation_entity = ${req.orgInfo.entity}`,
+ query: ({ req }) => ({
+ query: `
+ SELECT * FROM entity e
+ WHERE e.organisation_entity = :organisationEntity`,
+ params: {
+ organisationEntity: req.orgInfo.entity
+ }
+ }),
dataset: FetchOptions.fromParams,
result: 'entities'
})Likely invalid or redundant comment.
test/unit/middleware/common.middleware.test.js (1)
464-469:
Update error handling pattern in middleware test
The test expects the middleware to throw an error directly, which doesn't follow the middleware error handling pattern. Middleware functions should pass errors to the next function.
Apply this diff to correct the test:
- expect(() => getUniqueDatasetFieldsFromSpecification(req, res, next)).toThrowError('specification is required')
+ getUniqueDatasetFieldsFromSpecification(req, res, next)
+ expect(next).toHaveBeenCalledWith(new Error('specification is required'))Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
config/default.yaml (1)
91-91: Consider restructuring the pagination configuration.Whilst the default value of 50 is reasonable, consider the following improvements:
- Move this setting under a dedicated
paginationsection- Add documentation explaining its purpose
- Include additional pagination-related settings that might be needed
Here's a suggested restructure:
-tablePageLength: 50 +# Configuration for pagination across the application +pagination: + # Number of items to display per page in table views + tablePageLength: 50 + # Maximum number of pages to show in pagination controls + maxPageLinks: 5src/middleware/issueTable.middleware.js (4)
1-7: Enhance documentation with structured JSDoc commentsThe current documentation provides a good overview but could be more structured. Consider using JSDoc format for better documentation clarity and IDE support.
-/* - this middleware should get all entities for the organisation, with all of the most recent issues - then filter out any of the issues that have been fixed - the construct the table params - then construct the template params - then render the template -*/ +/** + * Middleware for handling issue table views. + * + * Process flow: + * 1. Fetches all entities for the organisation with their most recent issues + * 2. Filters out fixed issues + * 3. Constructs table parameters + * 4. Prepares template parameters + * 5. Renders the template + * + * @module issueTable.middleware + */
50-92: Consider extracting column preparation logicThe table preparation logic is quite complex. Consider extracting the column preparation logic into separate functions for better maintainability.
+const prepareReferenceColumn = (entity, field, index, lpa, dataset, issueType, issueField, errorMessage) => ({ + html: `<a href='/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/${index + 1}'>${entity[field]}</a>`, + error: errorMessage ? { message: errorMessage } : undefined +}) + +const prepareRegularColumn = (entity, field, errorMessage) => ({ + value: entity[field], + error: errorMessage ? { message: errorMessage } : undefined +}) + export const prepareTableParams = (req, res, next) => { const { entities, issues, uniqueDatasetFields, dataRange } = req const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params 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: `<a href='/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/${index + 1}'>${entity[field]}</a>`, - error: errorMessage - ? { - message: errorMessage - } - : undefined - }] - } else { - return [field, { - value: entity[field], - error: errorMessage - ? { - message: errorMessage - } - : undefined - }] - } + return [ + field, + field === 'reference' + ? prepareReferenceColumn(entity, field, index, lpa, dataset, issueType, issueField, errorMessage) + : prepareRegularColumn(entity, field, errorMessage) + ] })) }))
103-103: Address the TODO comment in the warning messageThe warning message indicates an issue with task list page functionality that needs to be addressed.
Would you like me to help create a GitHub issue to track the task list page issue?
146-162: Remove commented-out codeThe template parameters object contains commented-out properties that should be removed if they're no longer needed.
req.templateParams = { tableParams, organisation: orgInfo, dataset, errorSummary, issueType, pagination, dataRange - // issueEntitiesCount, - // pageNumber }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/default.yaml(1 hunks)src/middleware/issueTable.middleware.js(1 hunks)
🔇 Additional comments (8)
config/default.yaml (1)
91-91: Verify consistent usage of the pagination setting.
Let's ensure this configuration is being used consistently across the codebase and isn't duplicated elsewhere.
✅ Verification successful
Pagination configuration is used consistently across the codebase
The verification shows that:
- The
tablePageLength: 50configuration is correctly used inissueTable.middleware.js - Other instances of
pageLength: 50in test files are consistent with this configuration - Different page lengths (1 and 10) are used intentionally for specific features:
pageLength: 1for issue detailspageLength: 10in common middleware tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded pagination values or similar configuration
# Search for hardcoded page length values
echo "Checking for hardcoded pagination values..."
rg -n '(?i)(pageLength|itemsPerPage|perPage)\s*[=:]\s*\d+'
# Search for usage of tablePageLength
echo "Checking usage of tablePageLength configuration..."
rg -n '(?i)tablePageLength'
# Search for other pagination-related configuration
echo "Checking for other pagination configuration..."
rg -n '(?i)pagination.*config'
Length of output: 1623
src/middleware/issueTable.middleware.js (7)
16-28: LGTM! Robust query parameter validation
The validation schema is well-structured with proper type coercion and validation rules. Good use of the radix parameter in parseInt.
29-42: LGTM! Robust pagination calculation
The data range calculation is well-implemented with proper boundary handling and protection against edge cases.
44-48: LGTM! Secure path handling
Good use of encodeURIComponent for all path segments to prevent path traversal vulnerabilities.
164-187: LGTM! Clear and well-structured redirection logic
The redirection rules are well-organised and the implementation is clean.
189-193: LGTM! Clean template rendering setup
The template rendering is well-implemented using the renderTemplate helper.
195-213: LGTM! Well-structured middleware chain
The middleware chain is logically ordered and clearly composed, following a natural flow of operations.
102-107: 🛠️ Refactor suggestion
Enhance error handling with custom error class
The current error handling could be more specific and informative.
+class IssueTableError extends Error {
+ constructor(message, code) {
+ super(message);
+ this.name = 'IssueTableError';
+ this.code = code;
+ }
+}
if (issues.length <= 0) {
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')
+ const error = new IssueTableError('No issues found to display in the table', 'NO_ISSUES_FOUND')
return next(error)
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/middleware/issueTable.middleware.js (5)
1-7: Convert comment block to JSDoc format for better documentationConsider converting the descriptive comment block to JSDoc format for better IDE integration and documentation generation.
-/* - this middleware should get all entities for the organisation, with all of the most recent issues - then filter out any of the issues that have been fixed - the construct the table params - then construct the template params - then render the template -*/ +/** + * Middleware for handling issue tables. + * + * This middleware: + * 1. Retrieves all entities for the organisation with their most recent issues + * 2. Filters out fixed issues + * 3. Constructs table parameters + * 4. Prepares template parameters + * 5. Renders the template + */
29-42: Consider more descriptive variable names for paginationThe pagination logic is solid, but consider renaming variables for better clarity:
minRow→startIndexmaxRow→endIndexreq.dataRange = { - minRow: (pageNumber - 1) * pageLength, - maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), + startIndex: (pageNumber - 1) * pageLength, + endIndex: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount), totalRows: recordCount, maxPageNumber: Math.max(1, Math.ceil(recordCount / pageLength)), pageLength }
50-92: Consider breaking down complex mapping logic into smaller functionsThe table preparation logic could be more maintainable by extracting the column mapping logic into separate functions.
+const createReferenceColumn = (entity, field, errorMessage, { lpa, dataset, issueType, issueField }, index) => ({ + html: `<a href='/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/${index + 1}'>${entity[field]}</a>`, + error: errorMessage ? { message: errorMessage } : undefined +}) + +const createRegularColumn = (entity, field, errorMessage) => ({ + value: entity[field], + error: errorMessage ? { message: errorMessage } : undefined +}) + export const prepareTableParams = (req, res, next) => { const { entities, issues, uniqueDatasetFields, dataRange } = req const { lpa, dataset, issue_type: issueType, issue_field: issueField } = req.params 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: `<a href='/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/${index + 1}'>${entity[field]}</a>`, - error: errorMessage - ? { - message: errorMessage - } - : undefined - }] - } else { - return [field, { - value: entity[field], - error: errorMessage - ? { - message: errorMessage - } - : undefined - }] - } + return [ + field, + field === 'reference' + ? createReferenceColumn(entity, field, errorMessage, { lpa, dataset, issueType, issueField }, index) + : createRegularColumn(entity, field, errorMessage) + ] })) }))
146-160: Add JSDoc types for template parametersConsider adding JSDoc types to document the expected structure of template parameters.
+/** + * @typedef {Object} TemplateParams + * @property {Object} tableParams - Table configuration and data + * @property {Object} organisation - Organisation information + * @property {Object} dataset - Dataset information + * @property {Object} errorSummary - Error summary information + * @property {string} issueType - Type of issue being displayed + * @property {Object} pagination - Pagination configuration + * @property {Object} dataRange - Data range information + */ +/** + * Prepares parameters for the template rendering + * @param {Request} req - Express request object + * @param {Response} res - Express response object + * @param {Function} next - Express next middleware function + */ export const prepareTemplateParams = (req, res, next) => {
164-177: Move redirection groups to configurationConsider moving the hardcoded redirection groups to the configuration file for better maintainability.
-const redirectIssueGroups = [ +// Move to config/index.js +export const issueRedirectGroups = [ { type: 'missing value', field: 'reference' }, // ... ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/util.js(1 hunks)src/middleware/issueTable.middleware.js(1 hunks)
🔇 Additional comments (5)
config/util.js (1)
95-96: Verify the configuration usage and consistency
Let's ensure this configuration is properly integrated and consistent with other pagination implementations in the codebase.
✅ Verification successful
Configuration is properly integrated and consistent
The tablePageLength configuration is well-integrated and aligns with existing pagination patterns:
- It's properly defined in
config/default.yamlwith a default value of 50 - The implementation in
issueTable.middleware.jscorrectly uses this configuration for pagination - The value matches other pagination implementations (e.g.,
dataview.middleware.jsalso uses 50 as page length) - The pagination logic is consistent across different middleware implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new config property and other pagination implementations
# Check for usage of tablePageLength in the codebase
echo "Checking for tablePageLength usage:"
rg -l "tablePageLength"
# Check for other pagination implementations
echo "\nChecking for other pagination patterns:"
rg -l "pageLength|pageSize|itemsPerPage"
# Check for existing table configurations
echo "\nChecking for other table configurations:"
rg -l "table.*config"
Length of output: 723
Script:
#!/bin/bash
# Let's examine the actual implementation and usage of tablePageLength
# Check the configuration default value
echo "Default configuration:"
cat config/default.yaml
# Check the implementation in issueTable middleware
echo "\nIssue table middleware implementation:"
rg -A 5 -B 5 "tablePageLength" src/middleware/issueTable.middleware.js
# Check the tests to understand the expected behavior
echo "\nRelevant test cases:"
rg -A 5 -B 5 "tablePageLength" test/unit/middleware/issueTable.middleware.test.js
# Check other pagination implementations for consistency
echo "\nOther pagination implementations:"
rg -A 3 "pageLength|pageSize|itemsPerPage" src/middleware/dataview.middleware.js
Length of output: 5034
src/middleware/issueTable.middleware.js (4)
16-23: LGTM! Well-structured schema with proper validation
The schema correctly validates all required fields and includes proper type transformations.
44-48: LGTM! Proper URL encoding for security
The function correctly encodes all URL components to prevent XSS attacks.
193-211: LGTM! Well-structured middleware composition
The middleware chain is well-organized and follows a clear logical flow.
102-107: 🛠️ Refactor suggestion
Enhance error handling with custom error type
Consider using a custom error type for better error tracking and handling.
+class IssueTableError extends Error {
+ constructor(message, code) {
+ super(message);
+ this.name = 'IssueTableError';
+ this.code = code;
+ }
+}
if (issues.length <= 0) {
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')
+ const error = new IssueTableError('No issues found to display in the table', 'NO_ISSUES_FOUND')
return next(error)
}Likely invalid or redundant comment.
| ), | ||
| tablePageLength: v.number() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the table pagination configuration
A few suggestions to improve the robustness and maintainability of the pagination configuration:
- Add validation constraints to ensure reasonable page sizes:
- tablePageLength: v.number()
+ tablePageLength: v.pipe(
+ v.number(),
+ v.minValue(1),
+ v.maxValue(100)
+ )- Consider moving this under a more specific namespace to improve maintainability as the config grows:
- tablePageLength: v.number()
+ issueTable: v.object({
+ pageLength: v.pipe(
+ v.number(),
+ v.minValue(1),
+ v.maxValue(100)
+ )
+ })- Consider a more specific property name to avoid potential conflicts with other table views that might be added in the future:
- tablePageLength: v.number()
+ issueTablePageLength: v.pipe(
+ v.number(),
+ v.minValue(1),
+ v.maxValue(100)
+ )Committable suggestion skipped: line range outside the PR's diff.
4 participants
What type of PR is this? (check all applicable)
Description
Adds the issue table page
moved some code from issue details and dataview to common and refactored for reuse
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Preview Link: https://submit-pr-691.herokuapp.com/organisations/local-authority:WRT/brownfield-land/invalid%20URI/SiteplanURL/entry
Added/updated tests?
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests