add issue guidance to entity and entry page#950
Conversation
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested labels
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 (3)
✨ Finishing Touches
🪧 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/views/includes/_issue-guidance.html (1)
1-27: Well-structured issue guidance component.The component provides clear, actionable guidance for fixing dataset issues. The conditional rendering based on the presence of
issueSpecificationis appropriate, and the use of the GOV.UK Design System components maintains visual consistency.Some suggestions to enhance accessibility and user experience:
- Consider adding an
idattribute to the issue guidance section for better navigation and potential anchor linking.- The guidance text could benefit from more structured content patterns from the GOV.UK Design System, such as warning text component for critical issues.
src/middleware/entryIssueDetails.middleware.js (1)
2-2: Consider organizing the imports for better readability.This import line has become quite long with the addition of
getIssueSpecificationandprocessSpecificationMiddlewares. Consider organizing these imports for better readability, perhaps by grouping them by functionality or arranging them alphabetically.-import { createPaginationTemplateParams, fetchDatasetInfo, fetchEntryIssues, fetchOrgInfo, fetchResources, getErrorSummaryItems, getIssueSpecification, getSetBaseSubPath, getSetDataRange, logPageError, prepareIssueDetailsTemplateParams, processSpecificationMiddlewares, show404IfPageNumberNotInRange, validateQueryParams } from './common.middleware.js' +import { + createPaginationTemplateParams, + fetchDatasetInfo, + fetchEntryIssues, + fetchOrgInfo, + fetchResources, + getErrorSummaryItems, + getIssueSpecification, + getSetBaseSubPath, + getSetDataRange, + logPageError, + prepareIssueDetailsTemplateParams, + processSpecificationMiddlewares, + show404IfPageNumberNotInRange, + validateQueryParams +} from './common.middleware.js'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/middleware/entityIssueDetails.middleware.js(3 hunks)src/middleware/entryIssueDetails.middleware.js(4 hunks)src/routes/schemas.js(3 hunks)src/views/includes/_issue-guidance.html(1 hunks)src/views/organisations/issueDetails.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)
🔇 Additional comments (12)
src/views/organisations/issueDetails.html (1)
97-97: Good addition of the issue guidance component.Including the issue guidance template before the "How to improve" section provides a logical flow of information. This approach enhances user experience by first explaining the specific issue and then providing general improvement steps.
src/views/organisations/issueTable.html (1)
99-99: Good reuse of the issue guidance component.Including the shared issue guidance template promotes consistency across different views and reduces code duplication. This modular approach will make future maintenance easier.
src/routes/schemas.js (3)
108-114: Good schema definition for issue specification.The
IssueSpecificationschema is well-structured with appropriate field definitions. Making it optional allows for flexibility in different contexts.
223-223: Correct implementation of issue specification schema in OrgIssueTable.Using the centrally defined
IssueSpecificationschema maintains consistency and reduces duplication.
246-247: Appropriate addition of issue specification to OrgIssueDetails.Adding the
issueSpecificationfield to theOrgIssueDetailsschema ensures consistent validation across the application.src/middleware/entityIssueDetails.middleware.js (3)
17-18: Import updated to include the new middleware function.The import statement has been updated to include
getIssueSpecificationfrom the common middleware file, which is consistent with its addition to the middleware pipeline later in this file.
112-115: Template parameters now include issue specification data.The
templateParamsfunction has been updated to include theissueSpecificationproperty from the request object. This is done cleanly using the spread operator to maintain existing template parameters while adding the new data.
128-128: Added issue specification middleware to the processing pipeline.The
getIssueSpecificationmiddleware has been added to the default export array, positioned beforefilterOutEntitiesWithoutIssues. This ensures that the issue specification is available for subsequent middleware functions.src/middleware/entryIssueDetails.middleware.js (4)
22-23: SQL query formatting improved.The SQL query formatting has been adjusted to remove an unnecessary line break, improving consistency in the code.
135-138: Template parameters now include issue specification data.The
templateParamsfunction has been updated to include theissueSpecificationproperty from the request object, consistent with the changes in the entity issue details middleware. This provides the necessary data for displaying issue guidance in the template.
149-150: Added specification middleware to the processing pipeline.Two middleware functions have been added to the default export array:
processSpecificationMiddlewares- Likely processes specification data needed for issue guidancegetIssueSpecification- Retrieves and prepares issue specification dataTheir positioning after data fetching but before further processing is appropriate.
1-165:Details
✅ Verification successful
Verify that the new middleware functions exist and are properly implemented.
The changes in this file and
entityIssueDetails.middleware.jsboth referencegetIssueSpecificationandprocessSpecificationMiddlewares, but these functions aren't visible in the provided code. Ensure they're properly implemented incommon.middleware.js.
🏁 Script executed:
#!/bin/bash # Check for the implementation of the new middleware functions in common.middleware.js grep -n "getIssueSpecification\|processSpecificationMiddlewares" src/middleware/common.middleware.jsLength of output: 216
Middleware Functions Verified
BothprocessSpecificationMiddlewaresandgetIssueSpecificationhave been located insrc/middleware/common.middleware.js(lines 330 and 709, respectively) and appear to be correctly implemented. No further action is required regarding these functions.
DilwoarH
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/views/includes/_issue-guidance.test.js (1)
1-61: Consider adding more boundary cases for robust testing.While the current tests cover the main scenarios well, consider adding tests for additional edge cases:
- When
issueSpecificationis present but empty ({})- When
datasetis missing butissueSpecificationis present- When field names contain special characters that might need escaping in HTML
These additional tests would further strengthen the robustness of your component.
🧰 Tools
🪛 GitHub Actions: Unit test coverage report
[error] 1-1: Failed to load url @jest/globals (resolved id: @jest/globals) in /home/runner/work/submit/submit/test/unit/views/includes/_issue-guidance.test.js. Does the file exist?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/views/includes/_issue-guidance.test.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit test coverage report
test/unit/views/includes/_issue-guidance.test.js
[error] 1-1: Failed to load url @jest/globals (resolved id: @jest/globals) in /home/runner/work/submit/submit/test/unit/views/includes/_issue-guidance.test.js. Does the file exist?
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
test/unit/views/includes/_issue-guidance.test.js (3)
4-8: LGTM! Good test for empty state handling.This first test correctly verifies that the template doesn't render anything when no issue specification is provided, which is an important edge case to cover.
10-37: Good thorough testing of rendered guidance elements.This test provides comprehensive coverage of the component's rendering capabilities when a complete issue specification is provided. The assertions check all key elements: headings, missing column message, guidance content, and field name display.
39-59: LGTM! Good test for partial data handling.This test properly verifies the template's behavior when guidance text is missing, ensuring the UI degrades gracefully without showing empty sections.
DilwoarH
force-pushed
the
add-guidance-sections
branch
from
6d18538 to
a1d5868
Compare
DilwoarH
force-pushed
the
add-guidance-sections
branch
from
a1d5868 to
f8ec991
Compare
github-project-automation
Bot
moved this from Code review & QA
to Ready for release
in Providers team (BACKLOG)
4 participants
Preview Link
https://submit-pr-950.herokuapp.com/organisations/local-authority:LBH/listed-building-outline/missing%20value/reference/entry
What type of PR is this? (check all applicable)
Description
add issue guidance to entity and entry page
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor