771 design and content changes for endpoint submission form#797
Conversation
…gn-and-content-changes-for-endpoint-submission-form
…ement-deep-linking-into-the-submit-form
…71-design-and-content-changes-for-endpoint-submission-form
…gn-and-content-changes-for-endpoint-submission-form
WalkthroughThis pull request introduces significant changes to the endpoint submission form and deep link handling in the application. The modifications include renaming controllers, updating routing configurations, and revising user interface elements across multiple views. The changes streamline the data submission process, update form navigation, and improve the overall user experience for submitting planning data through a more structured and intuitive workflow. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
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: 4
🧹 Nitpick comments (6)
test/unit/checkDeepLinkController.test.js (1)
Line range hint
41-48: Consider adding edge case testsThe test coverage looks good, but consider adding tests for:
- Special characters in organisation names
- Non-existent datasets
- Malformed organisation IDs
test/unit/endpointSubmissionFormLinkController.test.js (1)
4-16: Consider enhancing mock functions with TypeScript interfacesThe mock functions provide a good foundation, but adding TypeScript interfaces would improve type safety and documentation.
interface SessionModel { get: (key: string) => any; set: (key: string, value: any) => void; } interface RequestObject { sessionModel: SessionModel; journeyModel: Map<string, any>; query: Record<string, string>; headers: Record<string, string>; }src/controllers/endpointSubmissionFormDeepLinkController.js (1)
36-54: Consider adding rate limiting for securityThe controller handles deep links but lacks rate limiting, which could lead to potential DoS attacks.
[security]Consider implementing rate limiting middleware:
import rateLimit from 'express-rate-limit' const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }) // Apply to your routesrc/controllers/pageController.js (1)
29-29: Consider using a more specific session key nameThe current session key 'deep-link-session-key' is quite generic. Consider using a more specific name that reflects its purpose.
- sessionKey = 'deep-link-session-key' + sessionKey = 'endpoint-submission-deep-link-session'src/filters/filters.js (1)
12-12: Consider grouping related filtersThe deep linking related filters (checkToolDeepLink and endpointSubmissionFormToolDeepLink) could be grouped together for better organisation.
import { checkToolDeepLink } from './checkToolDeepLink.js' import pluralize from 'pluralize' import { datasetSlugToReadableName } from '../utils/datasetSlugToReadableName.js' import { getDatasetGuidanceUrl } from './getDatasetConfig.js' import { schemaIssues } from './schemaIssues.js' -import { endpointSubmissionFormToolDeepLink } from './endpointSubmissionFormDeepLink.js' +// Deep linking filters +import { + checkToolDeepLink, + endpointSubmissionFormToolDeepLink +} from './deepLinkFilters/index.js'Also applies to: 47-47
src/controllers/checkDeepLinkController.js (1)
Line range hint
32-32: Update logger message to reflect new class name.The log message still references 'DeepLinkController' instead of 'CheckDeepLinkController'.
- logger.info('DeepLinkController.get(): invalid referrer URL, skipping', { + logger.info('CheckDeepLinkController.get(): invalid referrer URL, skipping', {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/controllers/checkDeepLinkController.js(3 hunks)src/controllers/endpointSubmissionFormDeepLinkController.js(1 hunks)src/controllers/pageController.js(2 hunks)src/filters/endpointSubmissionFormDeepLink.js(1 hunks)src/filters/filters.js(2 hunks)src/routes/form-wizard/check/steps.js(2 hunks)src/routes/form-wizard/endpoint-submission-form/steps.js(2 hunks)src/views/organisations/get-started.html(1 hunks)src/views/submit/lpa-details.html(4 hunks)test/unit/checkDeepLinkController.test.js(3 hunks)test/unit/endpointSubmissionFormLinkController.test.js(1 hunks)test/unit/lpa-detailsPage.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (9)
src/routes/form-wizard/endpoint-submission-form/steps.js (1)
46-56: Verify skip and reset behaviour in deep link journeyThe new '/link' route includes both
skipandresetflags. This combination might lead to unexpected behaviour:
skip: truemight bypass necessary validationreset: truealongsideresetJourney: trueseems redundantCould you clarify the intended behaviour and verify that essential validations aren't bypassed?
test/unit/lpa-detailsPage.test.js (1)
15-15: LGTM! Page title update aligns with form changesThe updated page title better reflects the current purpose of collecting contact details.
test/unit/endpointSubmissionFormLinkController.test.js (1)
38-49: Add more test cases for edge scenariosThe current test suite covers basic success and failure cases, but consider adding tests for:
- Special characters in orgName/dataset
- Maximum length validation
- Query parameter encoding
Let's check if there are any existing validation rules:
src/controllers/checkDeepLinkController.js (2)
44-44: LGTM! Class renaming improves clarity.The renaming from
DeepLinkControllertoCheckDeepLinkControllerbetter reflects its specific purpose.Also applies to: 83-83
61-61: LGTM! Session key update maintains consistency.The use of
this.sessionKeyaligns with the changes inpageController.js.src/routes/form-wizard/check/steps.js (1)
11-11: LGTM! Route configuration updated correctly.The import and controller reference have been properly updated to use
checkDeepLinkController, maintaining consistency with the controller renaming.Also applies to: 104-104
src/views/submit/lpa-details.html (2)
8-8: LGTM! Header improvements enhance context and clarity.The new header structure with caption provides better context by showing the LPA and dataset name, whilst maintaining GOV.UK design patterns.
Also applies to: 18-24
50-50: LGTM! Input labels are more specific and user-friendly.The updated labels "Your full name" and "Work email address" are clearer and more specific about the required information.
Also applies to: 66-66
src/views/organisations/get-started.html (1)
185-195: LGTM! Submission process improvements enhance user journey.The changes effectively:
- Streamline the submission process with a direct link
- Clearly outline required information
- Set proper expectations about the confirmation process
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/views/submit/dataset-details.html (1)
64-64: Consider adding a documentation URL field.The form now specifically asks for the webpage where the API/CSV is located, but we might still want to capture documentation URL separately. Consider whether a dedicated field for API/data documentation would be beneficial for users.
Also applies to: 69-69, 71-71, 72-72, 73-73, 74-74, 75-75, 76-76, 77-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/views/check/check-answers.html(4 hunks)src/views/submit/dataset-details.html(2 hunks)
🔇 Additional comments (4)
src/views/check/check-answers.html (2)
35-40: Verify if dataset change functionality is intentionally removedThe dataset row lacks a "Change" action unlike other fields. If this is intentional due to deep linking, please ensure this aligns with the user journey requirements.
133-133: Well-structured content changesThe updated heading, submission text, and button label provide clearer guidance and better align with GDS design patterns. The explicit mention of acceptance in the button text reinforces the user's commitment.
Also applies to: 137-137, 143-143
src/views/submit/dataset-details.html (2)
50-52: Clear and helpful hint text addition.The new hint text effectively clarifies the expected input format by explicitly mentioning both API and CSV file options.
64-64: Verify impact of renaming "Documentation URL" to "Webpage URL".Whilst the new label and hint text are clearer about the webpage's purpose, we should ensure this change doesn't impact existing documentation links or confuse users who previously submitted documentation URLs.
Run this script to check for potential impacts:
Also applies to: 69-69
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/controllers/pageController.js (1)
Line range hint
8-13: Update JSDoc to match implementationThe JSDoc comment doesn't reflect all the properties actually used in the code.
/** * If we arrived at the page via deep from another page, we'll use that page as the back link. * * @param {string} url current page URL - * @param {{ referrer?: string, dataset: string }} deepLinkInfo deep link info from the session + * @param {{ + referrer?: string, + dataset: string, + datasetName: string, + lpa: string, + orgId: string + }} deepLinkInfo deep link info from the session * @returns {string|undefined} back link URL */src/views/submit/check-answers.html (1)
Back link implementation needs improvement for accessibility
The codebase shows a better pattern using proper URLs (e.g., in layout templates with
options.lastPage). Usingjavascript:window.history.back()can be problematic for:
- Users with assistive technologies
- JavaScript-disabled browsers
- Keyboard navigation
Consider:
- Following the established pattern from layout templates using
options.lastPage- Refactoring all JavaScript history implementations in the submit flow for consistency
🔗 Analysis chain
Line range hint
1-150: Verify accessibility of the back link implementation.The back link uses JavaScript history, which might not work as expected for all users, especially those using assistive technologies.
Consider using a proper URL for the back link instead of
javascript:window.history.back(). Run this script to check if there are any established patterns for back links in the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for back link implementations across the codebase rg -B 2 -A 2 'govukBackLink|back-link' --type htmlLength of output: 5161
♻️ Duplicate comments (2)
src/controllers/endpointSubmissionFormDeepLinkController.js (1)
12-26:⚠️ Potential issueStrengthen URL validation in maybeSetReferrer
The current URL validation needs improvement to prevent security issues.
function maybeSetReferrer (req, sessionData) { if (req.headers.referer) { try { - /* eslint-disable-next-line no-new */ - new URL(req.headers.referer) - sessionData.referrer = req.headers.referer + const url = new URL(req.headers.referer) + if (!url.protocol.startsWith('https:')) { + throw new Error('Only HTTPS URLs are allowed') + } + const allowedDomains = ['.gov.uk', '.planning.data.gov.uk'] + if (!allowedDomains.some(domain => url.hostname.endsWith(domain))) { + throw new Error('Domain not allowed') + } + sessionData.referrer = url.toString() } catch (err) {src/controllers/pageController.js (1)
39-44:⚠️ Potential issueAdd type checking for deepLinkInfo properties
The code assumes the presence of properties without validation.
const deepLinkInfo = req?.sessionModel?.get(this.sessionKey) - if (deepLinkInfo) { + if (deepLinkInfo?.datasetName && deepLinkInfo?.lpa && deepLinkInfo?.orgId) { req.form.options.deepLink = deepLinkInfo req.form.options.datasetName = deepLinkInfo.datasetName req.form.options.lpa = deepLinkInfo.lpa req.form.options.orgId = deepLinkInfo.orgId + } else if (deepLinkInfo) { + logger.warn('Incomplete deepLinkInfo object', { + type: types.App, + deepLinkInfo + }) }
🧹 Nitpick comments (2)
src/controllers/endpointSubmissionFormDeepLinkController.js (1)
47-53: Consider using atomic session updatesThe current implementation sets session values individually before setting the combined object. Consider using a single atomic update.
- req.sessionModel.set('dataset', dataset) - req.sessionModel.set('lpa', orgName) - req.sessionModel.set('orgId', orgId) - const sessionData = { lpa: orgName, dataset, orgId } - maybeSetReferrer(req, sessionData) - req.sessionModel.set(this.sessionKey, sessionData) + const sessionData = { + dataset, + lpa: orgName, + orgId + } + maybeSetReferrer(req, sessionData) + req.sessionModel.set({ + dataset, + lpa: orgName, + orgId, + [this.sessionKey]: sessionData + })src/views/submit/check-answers.html (1)
133-133: Enhanced submission section with clearer messaging.The changes to the submission section improve user confidence by:
- Using a more direct heading "Now submit your dataset"
- Providing clearer confirmation text
- Using "Accept and submit" which better indicates the user's commitment
However, consider adding a hint text explaining what happens after submission, such as when users can expect a response or next steps.
Also applies to: 137-137, 143-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/controllers/endpointSubmissionFormDeepLinkController.js(1 hunks)src/controllers/pageController.js(2 hunks)src/routes/form-wizard/endpoint-submission-form/fields.js(1 hunks)src/views/submit/check-answers.html(5 hunks)src/views/submit/check.html(0 hunks)src/views/submit/confirmation.html(1 hunks)src/views/submit/dataset-details.html(3 hunks)src/views/submit/lpa-details.html(4 hunks)
💤 Files with no reviewable changes (1)
- src/views/submit/check.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/submit/dataset-details.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (7)
src/views/submit/lpa-details.html (3)
8-8: LGTM! Clear and well-structured page title.The question format follows GOV.UK Design System guidelines for form pages.
53-53: LGTM! Clear and specific form labels.The updated labels "Your full name" and "Work email address" are more specific and align with GOV.UK content design patterns.
Also applies to: 69-69
18-23: Verify heading structure for accessibility.The header structure looks good, providing clear context with the LPA and dataset name in the caption. However, let's ensure the heading hierarchy is maintained throughout the page for accessibility.
✅ Verification successful
Heading structure verified and follows accessibility standards
The heading hierarchy is properly implemented across the submit flow templates. Each page has a single h1 element, and subsequent heading levels follow the correct order without skipping levels. The sizing pattern is consistent, using larger headings (xl) for key journey pages and l-size headings for detail pages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check heading hierarchy in other template files rg -l "govuk-heading-[lmxs]" --type htmlLength of output: 953
Script:
#!/bin/bash # Check heading structure in lpa-details.html and related submit templates echo "=== lpa-details.html heading structure ===" rg "govuk-heading-[lmxs]" "src/views/submit/lpa-details.html" -B1 -A1 echo -e "\n=== Other submit flow templates heading structure ===" rg "govuk-heading-[lmxs]" "src/views/submit/" -B1 -A1Length of output: 1514
src/views/submit/confirmation.html (1)
15-21: Changes look goodThe link update properly utilises the organisation ID from the options object, and the text changes improve consistency.
src/views/submit/check-answers.html (3)
35-40: LGTM! Good use of the filter for dataset name readability.The addition of the dataset summary row with the
datasetSlugToReadableNamefilter improves the user experience by displaying a human-readable dataset name.
79-79: Improved clarity with better URL labels.The change from "Dataset URL" to "Endpoint URL" and "Documentation URL" to "Webpage URL" makes the purpose of each field clearer to users. This aligns well with the GDS style guide recommendations for clear, simple language.
Also applies to: 96-96
116-116: Good improvement to licence display.Converting the boolean "true"/"false" to "Yes"/"No" is more user-friendly and follows GOV.UK design patterns for displaying boolean values.
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…gn-and-content-changes-for-endpoint-submission-form
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
|
Should we block them from going straight to the service? https://submit-pr-797.herokuapp.com/submit The form breaks |
…gn-and-content-changes-for-endpoint-submission-form
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
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 (2)
src/routes/form-wizard/endpoint-submission-form/fields.js(1 hunks)src/routes/form-wizard/endpoint-submission-form/steps.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/form-wizard/endpoint-submission-form/fields.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/routes/form-wizard/endpoint-submission-form/steps.js (3)
26-30: Update required validation and schema definitionsThe removal of the 'lpa' field requires additional updates to maintain consistency across the codebase.
43-43: Verify implications of disabled journey checkingThe
checkJourney: falseflag is consistently set across routes. This might:
- Allow users to access pages out of sequence
- Bypass necessary validation steps
- Lead to incomplete or invalid form submissions
Consider implementing conditional journey checking based on the entry point.
Also applies to: 48-48
36-37: Review navigation flow for accessibility complianceThe updated back link changes the user journey from '/choose-dataset' to '/lpa-details'. Please ensure:
- This change aligns with GDS accessibility patterns
- Users can navigate backwards through their actual journey
- Screen readers announce the correct navigation context
✅ Verification successful
Back link implementation follows GDS patterns
The navigation changes are properly implemented using the GOV.UK Design System's back link component, ensuring accessibility compliance and maintaining a logical user journey.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for accessibility tests and patterns echo "=== Checking for accessibility tests ===" fd -t f -e test.js -e spec.js . | rg -l 'accessibility|aria|screen.?reader' # Check for navigation patterns echo "=== Checking navigation implementation ===" rg -l 'backLink|previousStep|navigation' src/Length of output: 1138
Script:
#!/bin/bash # Check for GDS back link implementation echo "=== Checking GDS back link usage ===" rg -A 5 'govuk-back-link' src/ # Check form wizard steps implementation echo "=== Checking form wizard steps ===" ast-grep --pattern 'backLink: $_' # Check for journey validation echo "=== Checking journey validation ===" rg -A 10 'checkJourney|validateJourney' src/Length of output: 7954
Script:
#!/bin/bash # Check template implementation echo "=== Checking template implementation ===" rg -A 3 'back.*link' src/views/ # Check for GDS components in layout echo "=== Checking layout components ===" rg -A 5 'govuk-back-link|govukBackLink' src/views/layouts/Length of output: 3158
| import LpaDetailsController from '../../../controllers/lpaDetailsController.js' | ||
| import PageController from '../../../controllers/pageController.js' | ||
| import CheckAnswersController from '../../../controllers/CheckAnswersController.js' | ||
| import EndpointSubmissionFormDeepLinkController from '../../../controllers/endpointSubmissionFormDeepLinkController.js' |
There was a problem hiding this comment.
💡 Codebase verification
Security concerns identified in deep linking implementation
The implementation has several security vulnerabilities that need addressing:
- CSRF protection is explicitly disabled in the form wizard configuration
- No input sanitisation beyond basic presence checks for
datasetandorgName - Authentication middleware is not visible in the request pipeline
- Deep linking with
entryPoint: truecould potentially bypass important workflow validations
Please implement proper security measures before deploying to production.
🔗 Analysis chain
Verify security measures for deep linking implementation
The new deep link route with entryPoint: true and skip: true could potentially bypass important validation steps. Please ensure:
- Input parameters are properly validated and sanitised
- Authentication/authorisation checks are in place
- Rate limiting is implemented to prevent abuse
Also applies to: 12-22
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for validation and security measures in the deep link controller
echo "=== Checking EndpointSubmissionFormDeepLinkController implementation ==="
ast-grep --pattern 'class EndpointSubmissionFormDeepLinkController {
$$$
}'
# Check for security middleware
echo -e "\n=== Checking for security middleware ==="
rg -l 'rate.*limit|sanitise|validate|auth' src/middleware/
Length of output: 777
Script:
#!/bin/bash
# Find the controller file
echo "=== Locating controller file ==="
fd endpointSubmissionFormDeepLinkController
# Check route configuration
echo -e "\n=== Checking route configuration ==="
rg -A 5 -B 5 "'/link'" src/
# Check for auth patterns
echo -e "\n=== Checking auth patterns ==="
rg -l "isAuthenticated|requiresAuth|checkAuth" src/
Length of output: 2200
Script:
#!/bin/bash
# Check controller implementation
echo "=== Controller Implementation ==="
cat src/controllers/endpointSubmissionFormDeepLinkController.js
# Check for global middleware
echo -e "\n=== Global Middleware Configuration ==="
rg -l "app.use|router.use" src/
# Check for parameter validation
echo -e "\n=== Parameter Validation ==="
rg -A 5 "validate|sanitise|params" src/controllers/endpointSubmissionFormDeepLinkController.js
Length of output: 3160
Script:
#!/bin/bash
# Check global middleware setup
echo "=== Global Middleware Setup ==="
cat src/serverSetup/middlewares.js
# Check session configuration
echo -e "\n=== Session Configuration ==="
cat src/serverSetup/session.js
# Check form wizard setup
echo -e "\n=== Form Wizard Setup ==="
cat src/routes/form-wizard/endpoint-submission-form/index.js
Length of output: 3384
DilwoarH
left a comment
DilwoarH
left a comment
There was a problem hiding this comment.
one comment plus tests are failing
There was a problem hiding this comment.
currently nowhere, though it will replace the get-started page when we finally link the submit service
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
DilwoarH
deleted the
771-design-and-content-changes-for-endpoint-submission-form
branch
4 participants
What type of PR is this? (check all applicable)
Description
QA Instructions, Screenshots, Recordings
this flow is currently unlinked. you can access the deep link via: https://submit-pr-797.herokuapp.com/submit/link?dataset=article-4-direction-area&orgName=Newcastle%20City%20Council&orgId=local-authority%3ANET
Flow
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
orgIdin the endpoint submission form.Bug Fixes
Documentation
Refactor
Style