Add issue specification guidance to issueTable page#818
Conversation
WalkthroughThis pull request introduces a new function Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/views/organisations/issueTable.html (1)
100-128: Improve accessibility of the guidance link.The implementation follows GOV.UK Design System patterns well. However, the guidance link could be more accessible.
Apply this diff to improve the link's accessibility:
- <p class="govuk-body">The <a href="{{ dataset.dataset | getDatasetGuidanceUrl }}" target="_blank">{{ dataset.name }}</a> guidance explains how to fix the issue:</p> + <p class="govuk-body">The <a href="{{ dataset.dataset | getDatasetGuidanceUrl }}" target="_blank" rel="noopener noreferrer">{{ dataset.name }} guidance (opens in new tab)</a> explains how to fix the issue:</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/middleware/common.middleware.js(1 hunks)src/middleware/issueTable.middleware.js(4 hunks)src/routes/schemas.js(1 hunks)src/views/organisations/issueTable.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/middleware/issueTable.middleware.js (1)
Line range hint
77-89: LGTM! Template parameters are correctly updated.The
issueSpecificationparameter is properly destructured from the request object and added to the template parameters.src/routes/schemas.js (1)
214-219: LGTM! Schema definition is well-structured.The
issueSpecificationproperty is properly defined with:
- Appropriate optional wrapper
- Strict object validation
- Required and optional fields with proper types
src/views/organisations/issueTable.html (1)
7-7: LGTM! Macro import is correctly placed.The
govukInsetTextmacro is properly imported from the GOV.UK Design System.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
.github/PULL_REQUEST_TEMPLATE.md
17-17: null
Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
.github/PULL_REQUEST_TEMPLATE.md (1)
Line range hint
1-14: The PR template structure looks good!The template follows GitHub's best practices and includes comprehensive sections for documenting PRs effectively.
Also applies to: 19-89
🧰 Tools
🪛 Markdownlint (0.37.0)
17-17: null
Bare URL used(MD034, no-bare-urls)
DilwoarH
force-pushed
the
782-technical-spike-how-will-we-surface-guidance-content-snippets
branch
from
3cc9a34 to
fbb4c93
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/PULL_REQUEST_TEMPLATE.md (1)
15-24:⚠️ Potential issueFormat the preview link section properly.
The preview link section needs proper formatting as previously suggested.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Bare URL used
null(MD034, no-bare-urls)
🧹 Nitpick comments (2)
test/unit/middleware/common.middleware.test.js (1)
1388-1455: Consider adding more test cases for better coverage.The test suite is well-structured, but could benefit from additional test cases:
- Test when
issue_fieldparameter is missing inreq.params- Test when field specification has empty
descriptionorguidancepropertiesdescribe('getIssueSpecification', () => { + it('handles missing issue_field parameter', () => { + const req = { + params: {}, + specification: { + fields: [ + { field: 'field1', description: 'string' } + ] + } + } + const res = {} + const next = vi.fn() + + getIssueSpecification(req, res, next) + + expect(req.issueSpecification).toBeUndefined() + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles field with empty description and guidance', () => { + const req = { + params: { issue_field: 'field1' }, + specification: { + fields: [ + { field: 'field1', description: '', guidance: '' } + ] + } + } + const res = {} + const next = vi.fn() + + getIssueSpecification(req, res, next) + + expect(req.issueSpecification).toEqual({ field: 'field1', description: '', guidance: '' }) + expect(next).toHaveBeenCalledTimes(1) + })src/views/organisations/issueTable.html (1)
117-119: Consider using a more specific heading.The current heading "How to fix this issue" is generic. Consider making it more specific to the context, such as "How to fix the missing column issue".
- <h2 class="govuk-heading-l"> - How to fix this issue - </h2> + <h2 class="govuk-heading-l"> + How to fix the missing column issue + </h2>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)src/middleware/common.middleware.js(1 hunks)src/middleware/issueTable.middleware.js(4 hunks)src/routes/schemas.js(1 hunks)src/views/organisations/issueTable.html(2 hunks)test/unit/middleware/common.middleware.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/middleware/issueTable.middleware.js
- src/middleware/common.middleware.js
- src/routes/schemas.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.github/PULL_REQUEST_TEMPLATE.md
23-23: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
DilwoarH
force-pushed
the
782-technical-spike-how-will-we-surface-guidance-content-snippets
branch
from
afcac26 to
de70d9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/middleware/common.middleware.test.js (1)
1393-1460: Add test cases for edge cases in getIssueSpecification.The test suite would benefit from additional test cases to ensure robust error handling:
- Test with undefined
issue_fieldparameter- Test with
nullfields array- Test with malformed field specification
Add these test cases:
it('handles missing specification in req', () => { const req = { params: { issue_field: 'field1' } } const res = {} const next = vi.fn() getIssueSpecification(req, res, next) expect(req.issueSpecification).toBeUndefined() expect(next).toHaveBeenCalledTimes(1) }) + + it('handles undefined issue_field parameter', () => { + const req = { + params: {}, + specification: { + fields: [ + { field: 'field1', description: 'string' } + ] + } + } + const res = {} + const next = vi.fn() + + getIssueSpecification(req, res, next) + + expect(req.issueSpecification).toBeUndefined() + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles null fields array in specification', () => { + const req = { + params: { issue_field: 'field1' }, + specification: { + fields: null + } + } + const res = {} + const next = vi.fn() + + getIssueSpecification(req, res, next) + + expect(req.issueSpecification).toBeUndefined() + expect(next).toHaveBeenCalledTimes(1) + }) + + it('handles malformed field specification', () => { + const req = { + params: { issue_field: 'field1' }, + specification: { + fields: [ + { invalidKey: 'field1' } + ] + } + } + const res = {} + const next = vi.fn() + + getIssueSpecification(req, res, next) + + expect(req.issueSpecification).toBeUndefined() + expect(next).toHaveBeenCalledTimes(1) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)src/middleware/common.middleware.js(1 hunks)src/middleware/issueTable.middleware.js(4 hunks)src/routes/schemas.js(1 hunks)src/views/organisations/issueTable.html(2 hunks)test/unit/middleware/common.middleware.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/schemas.js
- src/middleware/issueTable.middleware.js
- src/middleware/common.middleware.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.github/PULL_REQUEST_TEMPLATE.md
23-23: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (2)
.github/PULL_REQUEST_TEMPLATE.md (1)
23-23: Format the preview URL as a Markdown link.The bare URL should be formatted as a Markdown link for better readability and to follow Markdown best practices.
Apply this diff to format the URL:
-https://submit-pr-[PR_NUMBER].herokuapp.com/ +[Preview deployment](https://submit-pr-[PR_NUMBER].herokuapp.com/)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Bare URL used
null(MD034, no-bare-urls)
src/views/organisations/issueTable.html (1)
122-122: Add security attributes to the external link.When using
target="_blank", it's recommended to addrel="noopener noreferrer"to prevent potential security vulnerabilities.Apply this diff:
- <p class="govuk-body">The <a href="{{ dataset.dataset | getDatasetGuidanceUrl }}" target="_blank">{{ dataset.name }}</a> guidance explains how to fix the issue:</p> + <p class="govuk-body">The <a href="{{ dataset.dataset | getDatasetGuidanceUrl }}" target="_blank" rel="noopener noreferrer">{{ dataset.name }}</a> guidance explains how to fix the issue:</p>
DilwoarH
dismissed stale reviews from rosado and GeorgeGoodall-GovUk
via
cf755f3
DilwoarH
had a problem deploying
to
development
GitHub Actions
Failure
DilwoarH
force-pushed
the
782-technical-spike-how-will-we-surface-guidance-content-snippets
branch
from
cf755f3 to
a0f3893
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/staging.yaml (1)
14-16: Consider adding feature description.While the implementation is correct, consider adding a comment describing the purpose and impact of this feature flag for better maintainability.
features: + # Controls the display of issue specification guidance in the UI issueSpecificationGuidance: enabled: trueconfig/production.yaml (1)
21-23: Good practice for controlled rollout.Excellent approach to disable the feature in production initially. This allows for controlled rollout and easy rollback if needed.
Consider documenting the rollout strategy and criteria for enabling this feature in production.
config/default.yaml (1)
97-99: LGTM! Consider adding documentation for the feature flag.The feature flag implementation follows good practices by being disabled by default. However, it would be helpful to add a comment explaining its purpose and impact.
Add documentation above the feature flag:
features: + # Controls the display of specification guidance on the issueTable page + # When enabled, shows additional context about issue specifications issueSpecificationGuidance: enabled: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/default.yaml(1 hunks)config/development.yaml(1 hunks)config/local.yaml(1 hunks)config/production.yaml(1 hunks)config/staging.yaml(1 hunks)config/test.yaml(1 hunks)src/middleware/common.middleware.js(2 hunks)src/views/organisations/issueTable.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/organisations/issueTable.html
- src/middleware/common.middleware.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (4)
config/test.yaml (1)
14-17: Well-structured feature flag implementation.The feature flag follows best practices with a clear hierarchical structure. Good choice to enable it in the test environment for proper testing.
config/local.yaml (1)
17-19: Consistent with other environments.The implementation maintains consistency with other configuration files, which is crucial for maintainability.
config/development.yaml (1)
14-16: Consistent implementation.The feature flag implementation aligns with other environments, maintaining a uniform configuration structure.
config/default.yaml (1)
Line range hint
13-14: Update or remove the TODO comment.There's an unrelated TODO comment about URL and name updates that should be addressed or removed if no longer relevant.
Let's check if this TODO is still relevant:
✅ Verification successful
Remove outdated TODO comment
The TODO comment about updating the URL and name appears to be outdated. Both the URL (submit.planning.data.gov.uk) and the service names are actively used throughout the system and appear to be current.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the URL mentioned in the TODO is still in use # Search for references to the URL rg -i 'submit.planning.data.gov.uk' --type yaml --type js --type htmlLength of output: 369
Script:
#!/bin/bash # Find the TODO comment in the configuration file cat config/default.yamlLength of output: 3828
4 participants
What type of PR is this? (check all applicable)
Description
Add issue specification guidance to issue table 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
Release Notes
New Features
Improvements
Chores
The update provides a more informative user experience by displaying contextual information about potential dataset issues and their recommended resolutions.