684 issue details fetch data from all active resources #707
Conversation
…enamed files, and added new functionality to improve performance and correctness
WalkthroughThe changes in this pull request involve significant modifications to middleware and routing within the application. Key updates include the introduction of new middleware functions for handling issue details, data range, and error summaries, as well as the removal of obsolete middleware. The routing structure has been adjusted to reflect these changes, particularly in the Changes
Possibly related 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 (
|
GeorgeGoodall-GovUk
changed the base branch from
main
to
423-add-issues-table-component-to-issues-details-page-attempt-2
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
… responsibility external
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
…e-details-fetch-data-from-all-active-resources
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
|
|
||
| export const getSetDataRange = (pageLength) => (req, res, next) => { | ||
| const { recordCount } = req | ||
| const { pageNumber } = req.parsedParams |
There was a problem hiding this comment.
Is there another middleware to ensure pageNumber is a number and defined?
There was a problem hiding this comment.
(the reason why I ask this, is because I've seen a lot of errors in sentry related to the pageNumber being undefined)
There was a problem hiding this comment.
yep, valibot takes care of that now and sets a default of 0 (as long as your getting it from the parsedParams)
src/middleware/common.middleware.js
Outdated
| 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('/')}` |
There was a problem hiding this comment.
@GeorgeGoodall-GovUk this might be cleaner:
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();
};
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
![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)
src/middleware/entityIssueDetails.middleware.js (2)
34-52: Improve JSDoc documentation for better type clarityThe JSDoc comments should specify proper types instead of using
*. Consider updating to:/** - * @param {*} text - * @param {*} html - * @param {*} classes + * @param {string} text - The field text label + * @param {string|null} html - The HTML content to display + * @param {string} [classes] - Optional CSS classes to apply * @returns {{key: {text: string}, value: { html: string}, classes: string}} */
84-84: Address TODO comment regarding HTML template extractionThe TODO comment indicates that HTML construction should be moved to templates. This aligns with best practices for separation of concerns and would help address the XSS vulnerability.
Would you like me to help create a template for this HTML construction?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/middleware/common.middleware.js(4 hunks)src/middleware/entityIssueDetails.middleware.js(1 hunks)
🔇 Additional comments (9)
src/middleware/common.middleware.js (4)
559-582: Well-implemented pagination logic with proper validation
The function includes:
- Comprehensive input validation
- Clear error messages
- Correct pagination calculations
190-193:
Fix SQL injection vulnerability
The query still directly interpolates user input, making it vulnerable to SQL injection attacks.
Use parameterised queries to fix this security issue.
543-557: 🛠️ Refactor suggestion
Add URL encoding for all path segments
While additional parts are encoded, the main path segments (lpa, dataset, etc.) are not, which could lead to URL encoding issues.
Ensure consistent URL encoding for all path segments.
496-506: 🛠️ Refactor suggestion
Add input validation and error handling
The function assumes issues and entities arrays exist and are valid.
Add validation:
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()
}Likely invalid or redundant comment.
src/middleware/entityIssueDetails.middleware.js (5)
1-32: LGTM: Well-structured schema definition and imports
The validation schema is well-defined with appropriate types and optional parameters. The use of valibot for schema validation is a good choice for type safety.
54-57: LGTM: Clean and concise record count middleware
The middleware effectively handles the record count with proper null checking using optional chaining.
105-113: LGTM: Clean template rendering implementation
The template rendering middleware is well-documented and follows a consistent pattern.
115-133: LGTM: Well-structured middleware composition
The middleware chain is logically ordered, covering all necessary steps from validation through to error handling.
76-76:
Critical: Potential XSS vulnerability in HTML construction
The direct concatenation of user input into HTML strings via issueErrorMessageHtml could lead to XSS attacks.
Consider:
- Moving HTML construction to templates
- Using proper HTML escaping functions
- Implementing Content Security Policy (CSP) headers
Also applies to: 85-85
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
src/middleware/entityIssueDetails.middleware.js (5)
27-28: Add validation for pageNumber transformationConsider adding error handling for the pageNumber transformation to handle non-numeric strings gracefully.
- pageNumber: v.optional(v.pipe(v.string(), v.transform(s => parseInt(s, 10)), v.minValue(1)), '1'), + pageNumber: v.optional(v.pipe( + v.string(), + v.transform((s) => { + const num = parseInt(s, 10); + if (isNaN(num)) throw new Error('Invalid page number'); + return num; + }), + v.minValue(1) + ), '1'),
55-58: Add defensive programming to record countConsider adding type checking to ensure the count is always a valid number.
export const setRecordCount = (req, res, next) => { - req.recordCount = req?.issueEntities?.length || 0 + const count = req?.issueEntities?.length; + req.recordCount = typeof count === 'number' ? count : 0; next() }
85-91: Address TODO comment and improve error handlingThe TODO comment suggests missing implementation. Additionally, consider extracting HTML construction to a template.
Would you like me to help create a template for the HTML construction or open a GitHub issue to track this task?
73-81: Simplify issue filtering logicConsider extracting the issue filtering logic into a separate utility function for better maintainability.
+const findMatchingIssue = (field, issues) => + issues.find((issue) => issue.field === field); entityIssues.forEach(issue => { - const field = fields.find(field => field.key.text === issue.field) + const field = findMatchingIssue(issue.field, fields) if (field) { // ... rest of the code } })
110-114: Add error handling to template renderingConsider adding error handling for cases where templateParams or template is undefined.
export const getIssueDetails = renderTemplate({ - templateParams: (req) => req.templateParams, + templateParams: (req) => { + if (!req.templateParams) { + throw new Error('Template params are required'); + } + return req.templateParams; + }, template: 'organisations/issueDetails.html', handlerName: 'getIssueDetails' })src/middleware/entryIssueDetails.middleware.js (1)
79-83: Improve error handling with detailed messagesConsider providing more specific error messages to aid debugging.
if (!issues[pageNumber - 1] || !resources) { - const error = new Error('Missing required values on request object') + const error = new Error( + `Missing required values: ${!issues[pageNumber - 1] ? 'issues' : ''} ${!resources ? 'resources' : ''}`.trim() + ) error.status = 404 return next(error) }src/middleware/common.middleware.js (2)
605-606: Fix typo in warning messageThe warning message contains a grammatical error.
- logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) + logger.warn(`entry issue details were accessed from ${req.headers.referer} but there were no issues`)
616-616: Consider moving special case types to a constantThe special issue types should be defined as a constant at the module level for better maintainability.
+ const SPECIAL_ISSUE_TYPES = Object.freeze(['reference values are not unique']) - const specialIssueTypeCases = ['reference values are not unique'] + const specialIssueTypeCases = SPECIAL_ISSUE_TYPES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/middleware/common.middleware.js(5 hunks)src/middleware/entityIssueDetails.middleware.js(1 hunks)src/middleware/entryIssueDetails.middleware.js(1 hunks)src/middleware/issueTable.middleware.js(3 hunks)test/unit/middleware/common.middleware.test.js(2 hunks)test/unit/middleware/entityIssueDetails.middleware.test.js(1 hunks)test/unit/middleware/entryIssueDetails.middleware.test.js(1 hunks)test/unit/middleware/issueTable.middleware.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/middleware/entryIssueDetails.middleware.test.js
- test/unit/middleware/entityIssueDetails.middleware.test.js
🔇 Additional comments (14)
src/middleware/entityIssueDetails.middleware.js (2)
116-135: LGTM! Well-structured middleware chain
The middleware chain follows a logical order and includes all necessary steps for processing entity issue details.
42-53:
Security: Potential XSS vulnerability in HTML construction
The function directly concatenates potentially unsafe values into HTML strings. Consider using HTML escaping utilities.
+import { escapeHtml } from '../utils/security.js';
export const getIssueField = (text, html, classes) => {
classes = classes || ''
return {
key: {
- text
+ text: escapeHtml(text)
},
value: {
- html: html ? html.toString() : ''
+ html: html ? escapeHtml(html.toString()) : ''
},
classes
}
}Likely invalid or redundant comment.
src/middleware/issueTable.middleware.js (3)
27-30: Add defensive programming to record count
Consider adding type checking to ensure the count is always a valid number.
export const setRecordCount = (req, res, next) => {
- req.recordCount = req?.issues?.length || 0
+ const count = req?.issues?.length;
+ req.recordCount = typeof count === 'number' ? count : 0;
next()
}132-136: LGTM! Well-structured middleware chain updates
The additions to the middleware chain are logical and maintain the correct order of operations.
40-40:
Security: Potential XSS vulnerability in link generation
The code directly interpolates values into HTML without proper escaping.
+import { escapeHtml } from '../utils/security.js';
- html: `<a href='${baseSubpath}/entity/${index + 1}'>${entity[field]}</a>`,
+ html: `<a href='${escapeHtml(baseSubpath)}/entity/${index + 1}'>${escapeHtml(entity[field])}</a>`,Likely invalid or redundant comment.
src/middleware/entryIssueDetails.middleware.js (3)
130-148: LGTM! Well-structured middleware chain
The middleware chain follows a logical order and includes all necessary steps for processing entry issue details.
20-24:
Critical: SQL Injection vulnerability in query
The query uses string interpolation for resource values, making it vulnerable to SQL injection attacks.
- 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("', '")}')
- `,
+ query: ({ req }) => {
+ const resources = req.resources.map(resource => resource.resource);
+ const placeholders = resources.map(() => '?').join(', ');
+ return {
+ text: `
+ SELECT entity_count, entry_count, line_count, mime_type, internal_path, internal_mime_type, resource
+ FROM dataset_resource
+ WHERE resource IN (${placeholders})
+ `,
+ values: resources
+ };
+ },Likely invalid or redundant comment.
96-98:
Security: Potential XSS vulnerability in URL rendering
The code directly interpolates the URL into HTML without proper escaping.
+import { escapeHtml } from '../utils/security.js';
- html: `<a href='${resources[0].endpoint_url}'>${resources[0].endpoint_url}</a>`
+ html: `<a href='${escapeHtml(resources[0].endpoint_url)}'>${escapeHtml(resources[0].endpoint_url)}</a>`Likely invalid or redundant comment.
src/middleware/common.middleware.js (5)
569-592: LGTM! Well-structured pagination logic
The implementation includes proper input validation and correct pagination calculations.
642-659: LGTM! Clean template parameter preparation
The function follows a clear schema and is well-structured.
190-193:
Critical: SQL Injection Vulnerability in Resource Query
The query is vulnerable to SQL injection attacks as it directly interpolates user input (req.params.lpa and req.params.dataset).
Replace string interpolation with parameterised queries:
- query: ({ req }) => `
- SELECT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r
- 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 = ''
- ORDER BY start_date desc`,
+ query: ({ req }) => ({
+ text: `
+ 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 = $1
+ AND rd.dataset = $2
+ AND r.end_date = ''
+ ORDER BY start_date DESC`,
+ values: [req.params.lpa, req.params.dataset]
+ }),Likely invalid or redundant comment.
553-567: 🛠️ Refactor suggestion
Ensure URL parameters are properly encoded
The path parameters should be URL encoded to prevent XSS and ensure proper URL formatting.
export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => {
const params = [
- req.params.lpa,
- req.params.dataset,
- req.params.issue_type,
- req.params.issue_field,
+ encodeURIComponent(req.params.lpa),
+ encodeURIComponent(req.params.dataset),
+ encodeURIComponent(req.params.issue_type),
+ encodeURIComponent(req.params.issue_field),
...additionalParts.map(encodeURIComponent)
]
req.baseSubpath = params.reduce(
(path, param) => (param ? `${path}/${param}` : path),
'/organisations'
)
next()
}Likely invalid or redundant comment.
506-516: 🛠️ Refactor suggestion
Add input validation and error handling
The function should validate its inputs and handle edge cases:
- Check if issues and entities arrays exist and are arrays
- 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()
}Likely invalid or redundant comment.
test/unit/middleware/common.middleware.test.js (1)
1154-1553: LGTM! Comprehensive test coverage
The test suite includes thorough coverage of edge cases, error conditions, and happy paths for all middleware functions.
| @@ -165,7 +111,7 @@ describe('issueTableMiddleware', () => { | |||
| { | |||
| columns: { | |||
| reference: { | |||
| html: "<a href='/organisations/lpa-value/dataset-value/issue-type-value/issue-field-value/entry/1'>entity1</a>", | |||
| html: "<a href='/organisations/lpa-value/dataset-value/issue-type-value/issue-field-value/entity/1'>entity1</a>", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security tests for HTML link generation
The tests should verify that the HTML output is properly escaped when the input contains special characters that could lead to XSS.
Add this test case:
it('properly escapes special characters in entity references', async () => {
const reqCopy = {
...req,
entities: [{
entity: 'entity1',
reference: '"><script>alert("xss")</script>',
name: 'Name 1',
amount: 100
}],
issues: [{
entity: 'entity1',
field: 'amount',
issue_type: 'Invalid Amount'
}]
}
const next = vi.fn()
await prepareTableParams(reqCopy, {}, next)
expect(reqCopy.tableParams.rows[0].columns.reference.html).not.toContain('<script>')
})Also applies to: 132-132
…e-details-fetch-data-from-all-active-resources
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/views/organisations/issueTable.html (2)
87-87: LGTM! Consistent pagination display across templates.The change maintains consistency with the dataview template, ensuring a uniform user experience across the application.
Consider adding a comment in both templates explaining that row numbering starts from 1 for user-friendly display:
+ {# Row numbering starts from 1 for user-friendly display #} <p>Showing rows {{dataRange.minRow + 1}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}</p>
Line range hint
44-53: Consider enhancing error summary accessibility.The error summary implementation could benefit from additional ARIA attributes to improve screen reader experience.
Consider adding descriptive ARIA labels:
{{ govukErrorSummary({ titleText: errorSummary.heading if errorSummary.heading else 'There is a problem', + titleId: "error-summary-title", + errorList: errorSummary.items | map('setAttribute', 'data-cy', 'error-item'), + attributes: { + 'aria-labelledby': "error-summary-title" + }, errorList: errorSummary.items }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/views/organisations/dataview.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)
🔇 Additional comments (1)
src/views/organisations/dataview.html (1)
64-64: LGTM! The change improves user experience.
The adjustment to display row numbers starting from 1 instead of 0 is more intuitive for users. This change aligns with common UI patterns where pagination typically starts counting from 1.
Let's verify the middleware handling of dataRange:
✅ Verification successful
The middleware code shows that dataRange is properly handled. In common.middleware.js, the dataRange object is set with the correct zero-based indexing:
req.dataRange = {
minRow: (pageNumber - 1) * pageLength,
maxRow: Math.min((pageNumber - 1) * pageLength + pageLength, recordCount),
totalRows: recordCount,
// ...
}The minRow calculation starts from 0 (when pageNumber is 1), which aligns perfectly with the template's adjustment to display minRow + 1. This ensures consistent behaviour between the backend pagination logic and the frontend display.
The pagination implementation is correctly handling zero-based indexing
The middleware properly calculates zero-based indices for data slicing, while the template now displays user-friendly one-based numbering. This is a robust implementation that maintains technical correctness whilst improving user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dataRange middleware implementation
# Expected: Find middleware setting dataRange object with minRow property
# Search for dataRange-related middleware
rg -A 5 "dataRange" src/middleware/
Length of output: 6441
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
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)
1155-1247: Add tests for invalid inputs insetDataRangeTo ensure the robustness of the
setDataRangefunction, consider adding test cases that verify its behaviour when provided with invalid inputs, such as negativerecordCount,pageNumberless than 1, or non-numeric values. This will confirm that the error handling in the function works as expected.For example:
it('should return an error when recordCount is negative', () => { const req = { recordCount: -10, parsedParams: { pageNumber: 1 } } const res = {} const next = vi.fn() setTableDataRange(req, res, next) expect(next).toHaveBeenCalledWith(new Error('Invalid record count')) })
1249-1297: Add test cases for URL encoding insetBaseSubPathTo ensure that
setBaseSubPathcorrectly handles URL encoding for all path segments, add test cases where parameters contain special characters that need to be encoded. This will verify that the function behaves as expected when parameters include spaces, slashes, or other special characters.You can add a test like:
it('correctly URL encodes parameters with special characters', () => { const req = { params: { lpa: 'lpa name', dataset: 'data/set', issue_type: 'type#1', issue_field: 'field?' } } const setBaseSubPath = getSetBaseSubPath([]) setBaseSubPath(req, {}, vi.fn()) expect(req.baseSubpath).toEqual('/organisations/lpa%20name/data%2Fset/type%231/field%3F') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/middleware/common.middleware.js(5 hunks)test/unit/middleware/common.middleware.test.js(2 hunks)
🔇 Additional comments (7)
src/middleware/common.middleware.js (6)
357-365: Function filterOutEntitiesWithoutIssues is correctly implemented
The function efficiently filters entities that have corresponding issues associated with them.
570-592: Function getSetDataRange handles pagination correctly
The function correctly calculates the data range for pagination and includes proper error handling for invalid inputs.
614-631: Function prepareIssueDetailsTemplateParams correctly constructs template parameters
The function appropriately prepares req.templateParams with the necessary properties for rendering.
190-193:
Critical Security Issue: SQL Injection Vulnerability in Unparameterised SQL Query
The SQL query in fetchResources directly interpolates req.params.lpa and req.params.dataset, which poses a SQL injection risk. Please refactor the query to use parameterised inputs.
Apply this diff to fix the issue:
- query: ({ req }) => `
- SELECT r.end_date, r.entry_date, r.mime_type, r.resource, r.start_date, rle.endpoint_url, rle.licence, rle.status, rle.latest_log_entry_date, rle.endpoint_entry_date from resource r
- 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 = ''
- ORDER BY start_date desc`,
+ query: ({ req }) => {
+ const { lpa, dataset } = req.params
+ return {
+ text: `
+ 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 = $1
+ AND rd.dataset = $2
+ AND r.end_date = ''
+ ORDER BY start_date DESC`,
+ values: [lpa, dataset]
+ }
+ },506-516: 🛠️ Refactor suggestion
Add input validation and error handling in addReferencesToIssues
The function should validate its inputs and handle edge cases:
- Check if
issuesandentitiesarrays exist and are arrays. - Handle cases where an entity is not found.
Apply this diff to improve the function:
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()
}553-567: 🛠️ Refactor suggestion
Ensure consistent URL encoding for all path segments in getSetBaseSubPath
While additional parts are URL encoded, the main path segments (lpa, dataset, issue_type, issue_field) are not, which could lead to URL encoding issues if these parameters contain special characters.
Apply this diff to encode all path segments:
export const getSetBaseSubPath = (additionalParts = []) => (req, res, next) => {
+ const params = [
+ encodeURIComponent(req.params.lpa),
+ encodeURIComponent(req.params.dataset),
+ encodeURIComponent(req.params.issue_type),
+ encodeURIComponent(req.params.issue_field),
+ ...additionalParts.map(encodeURIComponent)
+ ]
-
- 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()
}test/unit/middleware/common.middleware.test.js (1)
1300-1389: Tests for getErrorSummaryItems cover key scenarios
The test cases effectively validate getErrorSummaryItems for various scenarios, ensuring it handles situations with no issues, with entities provided, and with resource-based entries.
What type of PR is this? (check all applicable)
Description
Update the issue details page to two other files: entity issue details and entry issue details.
getting the correct relevent issues from the entity issue links
also refactored alot of code from the middlewares into common middleware
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
I recommend looking through the datasets on lambeth and local-authority:WRT
Before
Before screenshot here
After
After screenshot here
Added/updated tests?
QA sign off
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests