check tool: issue details page#892
Conversation
WalkthroughThis pull request introduces a new feature flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant IssueDetailsController
participant performanceDbApi
Client->>IssueDetailsController: HTTP Request for issue details page
IssueDetailsController->>Middleware: Execute validateParams (conditional)
Middleware-->>IssueDetailsController: Return validated request or error
IssueDetailsController->>Middleware: Call prepareTask middleware
Middleware->>IssueDetailsController: Extract issueType and field from request
IssueDetailsController->>IssueDetailsController: Retrieve task from req.aggregatedTasks
alt Task not found
IssueDetailsController->>Client: Respond with 404 MiddlewareError
else Task found
IssueDetailsController->>performanceDbApi: Call getTaskMessage(task details)
performanceDbApi-->>IssueDetailsController: Return generated message
IssueDetailsController->>Middleware: Set req.locals.task with task and message
IssueDetailsController->>Client: Proceed with request processing
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/controllers/issueDetailsController.js (3)
1-7: Consider correcting the name of 'aggreatedTasks'
The property namereq.aggreatedTasksappears to have a typo. Consider renaming it toreq.aggregatedTasksfor clarity.
15-21: Typographical error in the error message
The error message has 'isssue' spelt with three 's'. Consider correcting it to 'issue' to maintain consistency and clarity.- return next(new MiddlewareError(`No isssue of type '${issueType}' for field ${field}`, 404)) + return next(new MiddlewareError(`No issue of type '${issueType}' for field ${field}`, 404))
53-69: Potential collision when merging options
UsingObject.assign(req.form.options, req.locals)might overwrite existing properties inreq.form.options. Consider ensuring that collisions are handled or documented if that is the intended behaviour.src/controllers/resultsController.js (3)
205-220: Confusing function naming causes potential misunderstanding
The function namefilterOutTasksByQualityCriterialLevelsuggests discarding tasks, but it actually returnstruefor tasks that should be retained. Clarify or rename the function to align with its inclusion logic.
250-273: Check for potential parameter encoding
makeTaskParamconstructs an href using rawissueTypeandfield. Consider encoding potentially unsafe characters in the URL to avoid unpredictable behaviours if these values contain special characters.
324-342: Check aggregator map usage
While preserving missing column tasks inreq.aggreatedTasks, ensure no duplication occurs ifgetMissingColumnTasksis invoked multiple times. If each field can only be reported once, guard against repeated entries.config/production.yaml (1)
26-30: Fix indentation and trailing spaces.The feature flag configuration has inconsistent indentation and trailing spaces.
Apply this diff to fix the formatting:
checkIssueDetailsPage: # https://github.com/digital-land/submit/issues/786 - # enabling this feature will cause the results page to present + # enabling this feature will cause the results page to present # links to individual issue pages enabled: false🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 28-28: trailing spaces
(trailing-spaces)
[warning] 30-30: wrong indentation: expected 6 but found 4
(indentation)
src/views/check/results/issueDetails.html (2)
33-37: Add aria-label to error summary for better accessibility.The error summary should have an aria-label to improve screen reader experience.
Apply this diff:
{{ govukErrorSummary({ titleText: criteriaLabels[options.task.qualityCriteriaLevel] or 'There is a problem', + ariaLabel: "Issue details", errorList: [ { html: options.task.message, href: ""}] }) }}
81-85: Add descriptive text to the list items.The ordered list items could benefit from more descriptive text to better guide users.
Apply this diff:
- <li>Fix the errors indicated</li> - <li>Use the <a href="/check">check service</a> to make sure the data meets - the standard</li> - <li>Publish the updated data on the data URL</li> + <li>Fix the errors indicated above in your dataset</li> + <li>Use the <a href="/check">check service</a> to validate that your data meets + all required standards</li> + <li>Publish the updated and validated data on your data URL</li>config/default.yaml (1)
105-109: Fix indentation and trailing spaces.The feature flag configuration has inconsistent indentation and trailing spaces.
Apply this diff to fix the formatting:
checkIssueDetailsPage: # https://github.com/digital-land/submit/issues/786 - # enabling this feature will cause the results page to present + # enabling this feature will cause the results page to present # links to individual issue pages enabled: true🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 107-107: trailing spaces
(trailing-spaces)
[warning] 109-109: wrong indentation: expected 6 but found 4
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/default.yaml(1 hunks)config/production.yaml(1 hunks)src/assets/scss/index.scss(1 hunks)src/controllers/issueDetailsController.js(1 hunks)src/controllers/resultsController.js(6 hunks)src/models/responseDetails.js(1 hunks)src/routes/form-wizard/check/steps.js(2 hunks)src/services/performanceDbApi.js(3 hunks)src/views/check/results/issueDetails.html(1 hunks)src/views/includes/_dataset-issue-page-header.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)test/acceptance/request_check.test.js(4 hunks)test/unit/performanceDbApi.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/views/organisations/issueTable.html
- src/models/responseDetails.js
🧰 Additional context used
🪛 YAMLlint (1.35.1)
config/production.yaml
[error] 28-28: trailing spaces
(trailing-spaces)
[warning] 30-30: wrong indentation: expected 6 but found 4
(indentation)
config/default.yaml
[error] 107-107: trailing spaces
(trailing-spaces)
[warning] 109-109: wrong indentation: expected 6 but found 4
(indentation)
🔇 Additional comments (11)
src/controllers/issueDetailsController.js (2)
8-14: Documentation looks good
The introductory doc comment clearly explains the purpose of the middleware function.
35-51: Verify the repeated usage of getRequestDataMiddleware
The array conditionally includesresults.getRequestDataMiddlewarein line 37, but then also unconditionally calls the same middleware in line 39. Confirm if this duplication is intentional or if one usage can be removed.src/controllers/resultsController.js (2)
191-200: Doc comment is clear
The high-level explanation of how tasks are aggregated is clear and helpful for future maintainers.
235-237: Verify intended use of filtering logic
Confirm that[2, 3].includes(issue.quality_criteria_level)is aligned with your definition of blocking or non-blocking issues. If there's a possibility of new levels in future, consider making this more flexible.src/routes/form-wizard/check/steps.js (1)
12-12: Good integration of the new controller
ImportingissueDetailsControlleris consistent with the overall design and helps maintain a clear separation of concerns.test/unit/performanceDbApi.test.js (1)
76-79: LGTM!The test case correctly verifies the HTML formatting functionality of the
getTaskMessagemethod.test/acceptance/request_check.test.js (2)
24-66: LGTM! Well-structured refactoring.The function effectively encapsulates common test logic, reducing code duplication while maintaining readability and proper error handling.
68-121: LGTM! Comprehensive test coverage.The function effectively encapsulates common test logic and includes thorough verification of the new issue details page functionality.
src/services/performanceDbApi.js (1)
191-235: LGTM! Well-implemented feature.The HTML formatting support is implemented in a clean, backwards-compatible way with proper documentation.
src/views/includes/_dataset-issue-page-header.html (1)
1-10: LGTM! Well-structured template.The template follows GOV.UK Design System patterns and implements proper conditional rendering for different data scenarios.
src/assets/scss/index.scss (1)
226-228: LGTM!The CSS rule for emphasising column names in error summaries is well-scoped and follows GOV.UK design patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/controllers/issueDetailsController.js (2)
7-13: Enhance JSDoc documentation.The JSDoc could be more descriptive about the middleware's purpose and parameters.
/** - * Middleware. Updates req with `task` + * Middleware that prepares task data for the issue details page. + * Extracts issue type and field from request parameters and retrieves + * the corresponding task from aggregated tasks. * - * @param {*} req request - * @param {*} res response - * @param {*} next next function + * @param {Object} req - Express request object + * @param {Object} req.params - Request parameters + * @param {string} req.params.issueType - Type of the issue + * @param {string} req.params.field - Field name + * @param {Map} req.aggreatedTasks - Map of aggregated tasks + * @param {Object} res - Express response object + * @param {Function} next - Express next middleware function + * @throws {MiddlewareError} When task is not found */
19-19: Fix typo in error message.There's a typo in the word "isssue" (three 's').
- return next(new MiddlewareError(`No isssue of type '${issueType}' for field ${field}`, 404)) + return next(new MiddlewareError(`No issue of type '${issueType}' for field ${field}`, 404))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/issueDetailsController.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
src/controllers/issueDetailsController.js (1)
34-34: Verify feature flag configuration.Ensure that the
checkIssueDetailsPagefeature flag is properly configured in both default and production configurations.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check feature flag configuration in config files echo "Checking feature flag configuration..." rg -A 2 "checkIssueDetailsPage" config/Length of output: 556
Feature flag configuration verified.
The configuration for
checkIssueDetailsPageis present and correctly set up in bothconfig/default.yamlandconfig/production.yaml.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/controllers/issueDetailsController.js (1)
24-35: 🛠️ Refactor suggestionImprove error handling and format validation.
The code needs better format validation and error handling:
- The format parameter should be validated
- The fallback message could be more descriptive
- let message = issueType // fallback + const ALLOWED_FORMATS = ['html', 'text'] + const format = 'html' + + if (!ALLOWED_FORMATS.includes(format)) { + return next(new MiddlewareError(`Invalid format: ${format}`, 400)) + } + + let message = `Issue type: ${issueType}, Field: ${field}` // more descriptive fallback try { message = performanceDbApi.getTaskMessage({ issue_type: task.issueType, num_issues: task.count, rowCount: req.totalRows, field: task.field, - format: 'html' + format }) } catch (error) { logger.warn('prepareTask/getTaskMessage failure', { type: types.App, errorMessage: error.message, errorStack: error.stack }) + return next(new MiddlewareError(`Failed to generate task message: ${error.message}`, 500)) }
🧹 Nitpick comments (3)
src/controllers/issueDetailsController.js (3)
21-21: Fix typo in error message.There's a typo in the word "isssue" (three 's' characters).
- return next(new MiddlewareError(`No isssue of type '${issueType}' for field ${field}`, 404)) + return next(new MiddlewareError(`No issue of type '${issueType}' for field ${field}`, 404))
51-51: Complete the inline comment.The comment appears to be incomplete: "we get this to ensure 'missing column issues".
- results.getBlockingTasks, // we get this to ensure 'missing column issues + results.getBlockingTasks, // we get this to ensure 'missing column' issues are handled
65-72: Improve error handling in locals method.The current implementation might swallow errors from
super.locals. Consider moving the try-catch block to only wrap theObject.assignoperation.async locals (req, res, next) { try { Object.assign(req.form.options, req.locals) - super.locals(req, res, next) } catch (error) { next(error) + return } + super.locals(req, res, next) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/issueDetailsController.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
src/controllers/issueDetailsController.js (1)
1-8: LGTM! All necessary dependencies are properly imported.The imports are well-organised and cover all required functionality for the controller.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
test/unit/issueDetailsController.test.js (2)
5-11: Consider enhancing test data setup.The request template could be more comprehensive to test edge cases:
- Empty aggregatedTasks map
- Invalid issue types
- Missing fields
const reqTemplate = { locals: {}, aggregatedTasks: new Map([ - ['missing column|name', { field: 'name' }]]), + ['missing column|name', { field: 'name' }], + ['invalid flag|status', { field: 'status' }], + ['unknown_issue|field', { field: 'field' }]]), params: { issueType: 'missing column', field: 'name' }, totalRows: 10 }
13-18: Add test coverage for error scenarios.The test only covers the happy path. Consider adding tests for:
- Non-existent issue types
- Missing fields
- Invalid HTML formatting
src/controllers/issueDetailsController.js (2)
29-29: Fix typo in error message.There's a typo in the error message: "isssue" should be "issue".
- return next(new MiddlewareError(`No isssue of type '${issueType}' for field ${field}`, 404)) + return next(new MiddlewareError(`No issue of type '${issueType}' for field ${field}`, 404))
68-85: Consider middleware order optimisation.The middleware chain could be optimised:
validateParamsshould be first to fail fast- Feature flag check could be combined with validation
- Error handling middleware should be last
const middlewares = [ + validateParams, isFeatureEnabled('checkIssueDetailsPage') - ? validateParams + ? (req, res, next) => next() : (req, res, next) => { return next(new MiddlewareError('Not found', 404)) }, results.getRequestDataMiddleware, results.fetchResponseDetails, - results.checkForErroredResponse, results.setupTableParams, setPagination, results.getIssueTypesWithQualityCriteriaLevels, results.extractIssuesFromResults, results.addQualityCriteriaLevelsToIssues, results.aggregateIssues, results.getBlockingTasks, results.getTotalRows, prepareTask, + results.checkForErroredResponse, results.setupError ]src/controllers/resultsController.js (1)
196-228: Improve task aggregation performance.The task aggregation could be optimised using
reduceinstead of a for-of loop.- const taskMap = new Map() - for (const issue of issues) { - if (filterOutTasksByQualityCriterialLevel(issue)) { - const key = `${issue['issue-type']}|${issue.field}` - const task = taskMap.get(key) - if (!task) { - taskMap.set(key, { - issueType: issue['issue-type'], - field: issue.field, - qualityCriteriaLevel: issue.quality_criteria_level, - count: 1 - }) - } else { - task.count++ - } - } - } + const taskMap = issues.reduce((map, issue) => { + if (filterOutTasksByQualityCriterialLevel(issue)) { + const key = `${issue['issue-type']}|${issue.field}` + const task = map.get(key) + if (!task) { + map.set(key, { + issueType: issue['issue-type'], + field: issue.field, + qualityCriteriaLevel: issue.quality_criteria_level, + count: 1 + }) + } else { + task.count++ + } + } + return map + }, new Map())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/issueDetailsController.js(1 hunks)src/controllers/resultsController.js(9 hunks)src/models/responseDetails.js(4 hunks)src/services/performanceDbApi.js(4 hunks)test/unit/issueDetailsController.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/models/responseDetails.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
test/unit/issueDetailsController.test.js (1)
20-29: Verify next() function calls.The test doesn't verify if
next()was called. Add assertions to ensure proper middleware flow.const next = vi.fn() prepareTask(req, {}, next) expect(req.locals.task.message.trim()).toBe('2 entries have <span class="column-name">some flag</span> fields that must have valid YES or NO values') +expect(next).toHaveBeenCalledTimes(1) +expect(next).toHaveBeenCalledWith()
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/controllers/issueDetailsController.js (2)
36-47:⚠️ Potential issueImprove error handling in message generation.
The catch block only logs the error but continues execution, which could lead to undefined behaviour.
try { message = performanceDbApi.getTaskMessage({ issue_type: task.issueType, num_issues: task.count, rowCount: req.totalRows, field: task.field, format: 'html' }) } catch (error) { logger.warn('prepareTask/getTaskMessage failure', { type: types.App, errorMessage: error.message, errorStack: error.stack }) + return next(new MiddlewareError(`Failed to generate task message: ${error.message}`, 500)) }
61-63:⚠️ Potential issuePrevent XSS vulnerability in pagination URL construction.
The pagination URL construction doesn't validate or sanitise inputs.
+ const sanitizedId = encodeURIComponent(id) + const sanitizedIssueType = encodeURIComponent(issueType) + const sanitizedField = encodeURIComponent(field) const pagination = responseDetails.getPagination(req.parsedParams.pageNumber, { - href: (item) => `/check/results/${id}/issue/${issueType}/${field}/${item}` + href: (item) => `/check/results/${sanitizedId}/issue/${sanitizedIssueType}/${sanitizedField}/${encodeURIComponent(item)}` })
🧹 Nitpick comments (1)
config/default.yaml (1)
106-110: Fix YAML formatting issues.Please address the following formatting issues:
- Remove trailing spaces in the comments on line 108
- Fix indentation on line 110 to match the 6-space indentation used throughout the file
Apply these changes:
checkIssueDetailsPage: # https://github.com/digital-land/submit/issues/786 - # enabling this feature will cause the results page to present - # links to individual issue pages - enabled: true + # enabling this feature will cause the results page to present + # links to individual issue pages + enabled: true🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 108-108: trailing spaces
(trailing-spaces)
[warning] 110-110: wrong indentation: expected 6 but found 4
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config/default.yaml(1 hunks)src/controllers/issueDetailsController.js(1 hunks)src/controllers/resultsController.js(9 hunks)src/models/requestData.js(2 hunks)src/serverSetup/routes.js(2 hunks)src/services/asyncRequestApi.js(1 hunks)src/views/organisations/issueTable.html(1 hunks)test/acceptance/request_check.test.js(4 hunks)test/unit/requestData.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/organisations/issueTable.html
🧰 Additional context used
🧠 Learnings (1)
src/controllers/resultsController.js (1)
Learnt from: rosado
PR: digital-land/submit#892
File: src/controllers/resultsController.js:91-98
Timestamp: 2025-02-20T15:38:57.104Z
Learning: In the results controller, page number validation is handled by the `validateParams` middleware using valibot schema with `minValue(1)`, making additional validation in the controller methods unnecessary.
🪛 GitHub Actions: Code change pipeline to development by @rosado
src/controllers/issueDetailsController.js
[error] 27-49: 'issueType' is not defined. (no-undef)
🪛 YAMLlint (1.35.1)
config/default.yaml
[error] 108-108: trailing spaces
(trailing-spaces)
[warning] 110-110: wrong indentation: expected 6 but found 4
(indentation)
🔇 Additional comments (10)
src/services/asyncRequestApi.js (1)
57-64: Well-structured changes to support issue details page.The refactoring improves URL handling by using the URL constructor and adds flexibility with the optional opts parameter.
src/models/requestData.js (1)
7-47: Well-structured implementation with proper validation.The changes demonstrate good practices:
- Clear parameter validation using valibot
- Comprehensive JSDoc documentation
- Proper handling of special cases (e.g., 'missing column' issue type)
test/unit/requestData.test.js (1)
72-72: LGTM! Test updated to match new method signature.The test case has been correctly updated to use the new object-based parameter structure for the
fetchResponseDetailsmethod.test/acceptance/request_check.test.js (2)
24-66: LGTM! Well-structured refactoring of test logic.The
checkDataFilefunction effectively encapsulates common test logic, reducing code duplication and improving maintainability. The function handles both JavaScript enabled and disabled scenarios appropriately.
68-121: LGTM! Comprehensive test coverage for error scenarios.The
checkErrorDataFilefunction provides thorough testing of error scenarios, including verification of error messages and 404 routing. The implementation aligns well with the new issue details page feature.src/controllers/resultsController.js (5)
1-10: LGTM! Clean feature flag implementation.The feature flag is properly imported and initialized, allowing for controlled rollout of the issue details page feature.
91-113: Verify error handling for detailsOptions.The
detailsOptionsparameter is destructured with optional chaining, but there's no explicit error handling for malformed options.Consider adding validation for the
detailsOptionsobject structure:- const detailsOpts = req.locals.detailsOptions ?? {} + const detailsOpts = req.locals.detailsOptions ?? {} + if (detailsOpts.issue && (!detailsOpts.issue.issueType || !detailsOpts.issue.field)) { + return next(new Error('Invalid issue details options')) + }
213-249: LGTM! Improved task aggregation logic.The refactored aggregation logic is more maintainable and properly handles the quality criteria level filtering.
251-284: Add input validation for task parameters.The
makeTaskParamfunction doesn't validate all required fields before use.
312-402: LGTM! Well-structured task handling functions.The new task handling functions are well-organized and properly support the issue details page feature. The parameter validation schema is correctly implemented using valibot.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/controllers/issueDetailsController.js (3)
36-47:⚠️ Potential issueImprove error handling in message generation.
The catch block only logs the error but continues execution. This could lead to undefined behaviour.
try { message = performanceDbApi.getTaskMessage({ issue_type: task.issueType, num_issues: task.count, rowCount: req.totalRows, field: task.field, format: 'html' }) } catch (error) { logger.warn('prepareTask/getTaskMessage failure', { type: types.App, errorMessage: error.message, errorStack: error.stack }) + return next(new MiddlewareError(`Failed to generate task message: ${error.message}`, 500)) }
37-43: 🛠️ Refactor suggestionValidate format parameter and handle potential errors.
The code doesn't validate the 'format' parameter or handle potential errors from
getTaskMessage.+ const ALLOWED_FORMATS = ['html', 'text'] + const format = 'html' + + if (!ALLOWED_FORMATS.includes(format)) { + return next(new MiddlewareError(`Invalid format: ${format}`, 400)) + } + message = performanceDbApi.getTaskMessage({ issue_type: task.issueType, num_issues: task.count, rowCount: req.totalRows, field: task.field, - format: 'html' + format })
61-63:⚠️ Potential issueValidate pagination parameters.
The pagination URL construction doesn't validate or sanitise inputs, which could lead to XSS vulnerabilities.
+ const sanitizedId = encodeURIComponent(id) + const sanitizedIssueType = encodeURIComponent(issueType) + const sanitizedField = encodeURIComponent(field) const pagination = responseDetails.getPagination(req.parsedParams.pageNumber, { - href: (item) => `/check/results/${id}/issue/${issueType}/${field}/${item}` + href: (item) => `/check/results/${sanitizedId}/issue/${sanitizedIssueType}/${sanitizedField}/${encodeURIComponent(item)}` })
🧹 Nitpick comments (5)
src/controllers/issueDetailsController.js (5)
11-15: Consider enhancing the pageNumber validation.The current validation could be more robust by adding an upper limit and handling invalid number strings.
const validateParams = validateQueryParams({ schema: v.object({ - pageNumber: v.optional(v.pipe(v.string(), v.transform(parseInt), v.minValue(1)), '1') + pageNumber: v.optional( + v.pipe( + v.string(), + v.transform((value) => { + const parsed = parseInt(value, 10) + if (isNaN(parsed)) throw new Error('Invalid number') + return parsed + }), + v.number([ + v.minValue(1), + v.maxValue(1000) // Prevent excessive pagination + ]) + ), + '1' + ) }) })
29-29: Fix typo in error message.There's a typo in the word 'isssue' (three 's').
- return next(new MiddlewareError(`No isssue of type '${issueTypeSlug}' for field ${field}`, 404)) + return next(new MiddlewareError(`No issue of type '${issueTypeSlug}' for field ${field}`, 404))
68-71: Consider validating params before assignment.The direct assignment of request parameters could be unsafe. Consider validating and sanitising the inputs.
async function setDetailsOptions (req, res, next) { + const sanitizedParams = Object.fromEntries( + Object.entries(req.params).map(([key, value]) => [key, encodeURIComponent(value)]) + ) - req.locals.detailsOptions = { issue: { ...req.params } } + req.locals.detailsOptions = { issue: sanitizedParams } next() }
73-91: Add documentation for the middleware pipeline.The middleware pipeline would benefit from documentation explaining the purpose and dependencies of each middleware in the chain.
+/** + * Middleware pipeline for processing issue details: + * 1. Feature flag check and parameter validation + * 2. Request data setup and error handling + * 3. Response details and pagination + * 4. Issue processing and quality criteria + * 5. Task preparation and error setup + */ const middlewares = [ isFeatureEnabled('checkIssueDetailsPage') ? validateParams : (req, res, next) => { return next(new MiddlewareError('Not found', 404)) }, // ... rest of the middleware chain ]
101-108: Enhance error handling in locals method.Consider adding more context to the error and logging before passing it to next.
async locals (req, res, next) { try { Object.assign(req.form.options, req.locals) super.locals(req, res, next) } catch (error) { + logger.error('Failed to process locals', { + type: types.App, + errorMessage: error.message, + errorStack: error.stack + }) + error.message = `Failed to process locals: ${error.message}` next(error) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/issueDetailsController.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/controllers/resultsController.js (2)
213-249: Consider using Map for issue type lookup.The current implementation iterates through all issues. Consider creating a Map for issue types to improve performance when dealing with large datasets.
export function aggregateIssues (req, res, next) { const { issues } = req + const issueTypeMap = new Map(issues.map(issue => [ + `${issue['issue-type']}|${issue.field}`, + issue + ])) const taskMap = new Map() - for (const issue of issues) { + for (const [key, issue] of issueTypeMap.entries()) { if (filterOutTasksByQualityCriterialLevel(issue)) { - const key = `${issue['issue-type']}|${issue.field}` const task = taskMap.get(key)
398-402: Enhance validation error messages.Consider adding custom error messages to the valibot schema to improve user experience when validation fails.
const validateParams = validateQueryParams({ schema: v.object({ - pageNumber: v.optional(v.pipe(v.string(), v.transform(parseInt), v.minValue(1)), '1') + pageNumber: v.optional(v.pipe( + v.string([v.message('Page number must be a string')]), + v.transform(parseInt), + v.minValue(1, [v.message('Page number must be greater than 0')]) + ), '1') }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/assets/scss/index.scss(1 hunks)src/controllers/resultsController.js(9 hunks)src/routes/form-wizard/check/steps.js(3 hunks)test/unit/publishRequestAPI.test.js(1 hunks)test/unit/requestData.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/assets/scss/index.scss
- test/unit/requestData.test.js
🧰 Additional context used
🧠 Learnings (1)
src/controllers/resultsController.js (1)
Learnt from: rosado
PR: digital-land/submit#892
File: src/controllers/resultsController.js:91-98
Timestamp: 2025-02-20T15:38:57.104Z
Learning: In the results controller, page number validation is handled by the `validateParams` middleware using valibot schema with `minValue(1)`, making additional validation in the controller methods unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (7)
src/routes/form-wizard/check/steps.js (3)
13-13: LGTM! Import statement follows consistent pattern.The import of
issueDetailsControlleris correctly placed and follows the established pattern of controller imports.
97-105: Parameter validation needed for issueType and field.The route accepts dynamic parameters without explicit validation. If
issueTypeorfieldare unrecognised or maliciously crafted, it could lead to unexpected UI or logs. Consider adding validation logic or sanitisation before processing them.
97-105: LGTM! Route configuration follows established patterns.The new route configuration is well-structured and consistent with other routes in the file. It correctly:
- Extends
baseSettings- Sets appropriate flags (
noPost,checkJourney,entryPoint,forwardQuery)- Specifies the correct template and controller
src/controllers/resultsController.js (4)
1-10: Clean imports and feature flag setup!The imports are well-organized, and the feature flag implementation follows best practices.
97-106: Fix undefined parsedParams error.The static analysis indicates that
req.parsedParamsis undefined in tests. EnsurevalidateParamsmiddleware is executed beforefetchResponseDetails.
267-283: Add input validation for task parameters.The
makeTaskParamfunction doesn't validate all required fields before use.
16-49:Details
❓ Verification inconclusive
Verify middleware execution order.
The middleware stack looks good, but ensure that
validateParamsexecutes before any middleware that depends onreq.parsedParams.
🏁 Script executed:
#!/bin/bash # Search for middleware that uses req.parsedParams rg -A 3 'req\.parsedParams' src/Length of output: 3787
Action: Confirm Middleware Execution Order
The middleware stack in
src/controllers/resultsController.jsappears to be in the correct order withvalidateParamsinvoked first. This ensures that any downstream middleware reliant onreq.parsedParamsreceives it correctly. However, as several other parts of the code (e.g. insrc/middleware/common.middleware.jsand controllers likeissueDetailsController.js) also depend onreq.parsedParams, please double-check that any future reordering or modifications preserve this critical execution order.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/controllers/ShareResultsController.js (1)
25-27: LGTM! Consider making the page number configurable.The path update is consistent with the new routing structure. However, since this number appears to be significant for navigation, consider making it a configurable constant to avoid magic numbers.
+ const DEFAULT_RESULTS_PAGE = '1' + generateResultsLink (id) { - return `${config.url}check/results/${id}/1` + return `${config.url}check/results/${id}/${DEFAULT_RESULTS_PAGE}` }test/unit/shareResultsController.test.js (1)
35-37: LGTM! Consider using a shared constant for the page number.The test updates are thorough and consistent with the new routing structure. To improve maintainability, consider using a shared constant for the page number, which would align with the suggested enhancement in ShareResultsController.
+ const DEFAULT_RESULTS_PAGE = '1' + it('should set shareLink in req.locals and req.form.options', async () => { await shareResultsController.locals(req, res, next) - const expectedLink = `${config.url}check/results/123/1` + const expectedLink = `${config.url}check/results/123/${DEFAULT_RESULTS_PAGE}` expect(req.locals.shareLink).toBe(expectedLink) expect(req.form.options.shareLink).toBe(expectedLink) })Also applies to: 43-44, 71-71, 79-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
performance-test/locustfile.py(1 hunks)src/controllers/ShareResultsController.js(1 hunks)test/unit/middleware/common.middleware.test.js(1 hunks)test/unit/shareResultsController.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/unit/middleware/common.middleware.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
performance-test/locustfile.py (1)
62-62: LGTM! Path update aligns with new routing structure.The change from
/0to/1in the results path is consistent with the broader routing updates across the application.
5 participants
Preview Link
https://submit-pr-892.herokuapp.com/
What type of PR is this? (check all applicable)
Description
Adds issue details page to the check tool results.
As discussed, the table, to be actually useful, needs changes on the API side - currently it shows all data (as opposed, just the data relevant to the viewed issue).
Also:
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Go to preview link, submit an URL (with some data issues) and click through to an issue from the results page.
Note: the column names and order in the table are fixed in #897
Added/updated tests?
QA sign off
Summary by CodeRabbit
New Features
Style
Bug Fixes