check tool: update 'errors' page design & content #640
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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
🧹 Outside diff range and nitpick comments (4)
src/views/check/results/errors.html (1)
70-79: Clear and well-structured guidance section!The conditional heading and step-by-step instructions are clear. Consider adding a brief explanation of what the specifications contain to help users understand why they're useful.
<ol class="govuk-list govuk-list--number"> - <li>Fix the errors indicated. You should use the <a href="/guidance/specifications/">data specifications</a> to help you do this.</li> + <li>Fix the errors indicated. Use the <a href="/guidance/specifications/">data specifications</a> (which contain detailed rules and examples) to help you do this.</li> <li>Check your updated data to make sure it meets the specifications.</li> </ol>src/models/requestData.js (1)
Line range hint
35-83: Consider refactoring error handling for better maintainabilityThe class currently has multiple methods handling different aspects of errors (
getErrorSummary,hasErrors,getError), each with its own null checks and logging. This could be simplified and made more maintainable.Consider:
- Creating a central error handling utility
- Using TypeScript or JSDoc types more extensively for better type safety
- Implementing a consistent pattern for null checks and logging
Here's a suggested approach:
/** * @typedef {Object} ErrorState * @property {boolean} hasErrors * @property {string[]} summary * @property {Object} details */ /** * Centralised error handling * @returns {ErrorState} */ getErrorState() { if (!this.response?.data) { logger.warn('No response data available', { requestId: this.id }); return { hasErrors: true, summary: [], details: { message: 'An unknown error occurred.' } }; } return { hasErrors: Array.isArray(this.response.data['error-summary']) && this.response.data['error-summary'].length > 0, summary: this.response.data['error-summary'] || [], details: this.response.error || {} }; }src/controllers/resultsController.js (1)
Line range hint
1-95: Consider architectural improvements for error handlingThe error handling flow could benefit from the following improvements:
- Document the different error states and their implications
- Consider extracting the error processing logic into a dedicated service
- Add TypeScript or JSDoc type definitions for error structures
Example type definitions:
/** * @typedef {Object} ErrorSummaryItem * @property {string} text - The error message * @property {string} href - Link to the error location */ /** * @typedef {Object} ErrorResponse * @property {ErrorSummaryItem[]} errorSummary * @property {Object} tableParams * @property {Object} mappings */test/integration/test_recieving_results.playwright.test.js (1)
Line range hint
63-67: Fix incorrect array iteration syntaxThe current implementation has two issues:
- Using
for...inwith arrays is incorrect as it iterates over all enumerable properties- The destructuring syntax with
for...inis invalidApply this fix to correctly iterate over the array with indices:
- for (const [index, issuesForRow] in issues) { - for (const issue in issuesForRow) { - await expect(page.locator('.govuk-table').locator('tr').nth(parseInt(index) + 1)).toContainText(prettifyColumnName(issue)) - } - } + for (const [index, issuesForRow] of issues.entries()) { + for (const issue of issuesForRow) { + await expect(page.locator('.govuk-table').locator('tr').nth(index + 1)).toContainText(prettifyColumnName(issue)) + } + }This change:
- Uses
for...ofwithentries()for proper array iteration- Removes unnecessary
parseIntas index is already a number- Correctly iterates over the issues array
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/controllers/resultsController.js(1 hunks)src/models/requestData.js(1 hunks)src/views/check/results/errors.html(3 hunks)test/PageObjectModels/resultsPage.js(1 hunks)test/integration/test_recieving_results.playwright.test.js(1 hunks)test/unit/errorsPage.test.js(3 hunks)test/unit/resultsController.test.js(1 hunks)
🔇 Additional comments (10)
src/views/check/results/errors.html (2)
7-7: Good addition of the GOV.UK error summary component!
This import enables the use of the standardised error summary component, which improves accessibility and maintains consistency with GOV.UK design patterns.
82-82: Good improvement to the button text!
The new text "Check your data" is more accurate and better describes the next action in the user journey.
src/models/requestData.js (1)
35-38: Review return type documentation for consistency
The JSDoc comment indicates that getErrorSummary returns string[], but according to the AI summary, these error summaries are being transformed into objects with text and href properties in the ResultsController. This suggests a potential disconnect between the documented return type and how the data is actually being used.
Consider either:
- Updating the return type documentation to reflect the actual structure needed by consumers
- Or documenting that the strings returned by this method require transformation before use
Let's verify how this method is used:
src/controllers/resultsController.js (2)
67-72: 🛠️ Refactor suggestion
Consider implementing proper href values for error summary links
The empty href values might affect accessibility and user navigation. Each error summary item should ideally link to the corresponding error location on the page.
Consider this implementation:
req.form.options.errorSummary = requestData.getErrorSummary().map(message => {
return {
text: message,
- href: ''
+ href: `#error-${message.toLowerCase().replace(/\s+/g, '-')}`
}
})Let's verify the error summary structure and its usage:
Line range hint 1-95: Verify integration with LPA name display functionality
The PR objectives mention LPA name display variations. Ensure this controller properly handles both scenarios:
- Default text when LPA name is unavailable
- LPA name display when accessed via deep link
Let's verify the LPA name handling:
✅ Verification successful
LPA name handling is correctly implemented through deep link context
The verification shows that:
- LPA name (organisation name) is properly handled in both error and no-error templates (
src/views/check/results/errors.htmlandsrc/views/check/results/no-errors.html) - The deep link context is correctly passed through
PageControllerwhich setsoptions.deepLinkcontaining the organisation name - The templates appropriately handle both scenarios:
- Show default text when LPA name is unavailable
- Display LPA name when provided via deep link
The implementation in ResultsController inherits this functionality from PageController and doesn't need additional LPA name handling logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how LPA name is processed across the codebase
rg -A 5 'getLpaName|lpaName' src/
# Check for deep link handling
rg -A 5 'isDeepLink|deepLink' src/
Length of output: 4204
Script:
#!/bin/bash
# Check for LPA name handling in the request data model
ast-grep --pattern 'class $_ {
$$$
getLpaName() {
$$$
}
$$$
}'
# Check for LPA name handling in templates
rg -l 'lpa.*name|orgName' src/views/
# Check for deep link data structure in the request model
ast-grep --pattern 'class $_ {
$$$
getDeepLinkData() {
$$$
}
$$$
}'
Length of output: 272
test/unit/errorsPage.test.js (2)
55-55: Well-structured error summary implementation
The transformation of error messages into objects with text and href properties is cleanly implemented. The test properly validates the new structure whilst maintaining existing functionality.
Also applies to: 74-74
40-45: Query: Should href be empty?
The summaryItem function creates objects with an empty href property. Consider whether these error messages should link to specific sections of the form or documentation.
✅ Verification successful
Empty href is correct in error summary items
The empty href in the summaryItem function is the correct implementation. The GOV.UK error summary component is being used across multiple templates (src/views/**/*.html), and these templates import the error summary macro without requiring href values. The error summary is used to display validation messages at the top of the page without needing to link to specific form sections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if hrefs are used in the error templates
rg -g '*.html' -A 5 'govuk-error-summary__list'
Length of output: 47
Script:
#!/bin/bash
# Let's check for error summary implementations in views and templates with different extensions
rg -t html -t njk -t nunjucks -A 5 'govuk-error-summary'
# Also check the controller/page implementation to understand the href usage
ast-grep --pattern 'summaryItem = $_'
Length of output: 124
Script:
#!/bin/bash
# Search for error summary implementations in views directory
rg -g '*.{html,nunjucks}' -A 5 'error-summary'
# Check the controller implementation for error handling
rg -g '*.js' -A 5 'summaryItem'
# Look for any error-related templates
fd . -e html -e nunjucks --exec grep -l "error"
Length of output: 8249
test/integration/test_recieving_results.playwright.test.js (1)
44-44: Good improvement to selector specificity!
The change to use ul.govuk-list instead of a general text locator makes the test more robust and less prone to false positives.
test/unit/resultsController.test.js (2)
Line range hint 1-214: Test coverage looks comprehensive
The test suite thoroughly covers various scenarios:
- Error and no-error states
- Failed file and URL requests
- Request completion status
- Error handling (404, 500)
- Template selection logic
- Data transformation
Well structured and maintainable test cases.
139-139: Verify error summary href implementation
The test shows that href is always set to an empty string. If this is intentional, consider adding a test case that explicitly verifies this behaviour. If not, we might need tests for scenarios where href should contain a value.
✅ Verification successful
Let me search for more context about error summary implementation.
Empty href in error summary is correct implementation
The empty href is intentional and aligns with the implementation in resultsController.js. The controller explicitly sets empty hrefs when mapping error messages:
errorSummary = requestData.getErrorSummary().map(message => {
return {
text: message,
href: ''
}
})This is used with the GOV.UK Design System's error summary component, which allows optional hrefs for cases where error messages don't need to link to specific form fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for href usage in error summaries across the codebase
rg -A 3 'errorSummary.*href'
# Check if there are any error summary components that might expect href values
ast-grep --pattern 'errorSummary.*href'
Length of output: 391
Script:
#!/bin/bash
# Search for error summary usage and structure in the codebase
rg -g '!test/**' 'errorSummary' -A 3
# Look for error summary component or type definitions
fd -e ts -e js -e jsx -e tsx . | xargs rg 'interface.*ErrorSummary|type.*ErrorSummary|class.*ErrorSummary'
# Search for GDS error summary component imports or usage
rg "import.*ErrorSummary|require.*ErrorSummary"
Length of output: 2496
|
The error summary table should use the error messages from this file: https://github.com/digital-land/submit/blob/main/src/content/fieldIssueMessages.csv Can we any numerical or date columns use the |
Those messages come from external service in the form of those strings. We can't look them up in the mentioned table. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/views/check/results/errors.html (1)
81-84: Consider enhancing the guidance contentWhile the current guidance is clear, it could be more specific about common error types and their solutions.
Consider expanding the ordered list to include:
<ol class="govuk-list govuk-list--number"> <li>Fix the errors indicated. You should use the <a href="/guidance/specifications/">data specifications</a> to help you do this.</li> <li>Check your updated data to make sure it meets the specifications.</li> + <li>Common errors include: + <ul class="govuk-list govuk-list--bullet"> + <li>Invalid date formats - ensure dates follow YYYY-MM-DD format</li> + <li>Missing required fields - check all mandatory fields are completed</li> + <li>Incorrect data types - verify numbers and text are in correct fields</li> + </ul> + </li> </ol>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/views/check/results/errors.html(3 hunks)src/views/check/results/no-errors.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/check/results/no-errors.html
🔇 Additional comments (5)
src/views/check/results/errors.html (5)
7-9: LGTM: Import statements are properly structured
The import statements are correctly updated with proper relative paths and the new error summary component import.
39-42: Skip comment as past review is still valid
The previous review comment about href values in the error summary is still applicable.
50-52: LGTM: Accessible map implementation
The map container is properly implemented with a descriptive aria-label and appropriate conditional rendering.
87-87: LGTM: Clear and action-oriented button text
The button text "Check your data" clearly communicates the action to users.
98-105: Skip comment as past review is still valid
The previous review comment about pinning the MapLibre GL version is still applicable.
Thank. And yep, via slack thread. |
What type of PR is this?
Description
Update content & design elements of the check tool's "you've got errors" page.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
For cases where the LPA name is not available (e.g. we have not arrived via deep link) the text at the bottom defaults to "How to improve the data".
But when the accessing the check tool via a deep link we can see the LPA's name:
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests