772 link to endpoint submission form from live service#930
Conversation
WalkthroughThe changes introduce early parameter validation in deep link functions, ensuring that invalid or missing data for organisation or dataset results in a default URL response. Several HTML templates and error pages have been updated to standardise terminology—switching from “data URL” to “endpoint URL”—and URL generation now applies proper encoding. Additionally, a new controller is assigned for the confirmation route, the issue specification schema now includes an optional guidance field, and the endpoint submission controller has been enhanced to incorporate dataset information into session data. 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)
🪧 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/filters/endpointSubmissionFormDeepLink.js (1)
13-13: JSDoc and implementation mismatchThe JSDoc describes dataset as an object with a 'dataset' property, but the implementation uses 'dataset' directly. Additionally, the use of 'organisation.organisation' for the orgId parameter seems unusual.
Consider updating either:
- The JSDoc to match implementation:
/** * Returns the deep link to the endpoint submission form for a given dataset and organisation * - * @param {{name:string}} organisation - * @param {{dataset:string, name:string}} dataset + * @param {{name:string, organisation:string}} organisation + * @param {string} dataset * * @return {string} */
- Or the implementation to match JSDoc:
- return `/submit/link?dataset=${encodeURIComponent(dataset)}&orgName=${encodeURIComponent(organisation)}&orgId=${encodeURIComponent(organisation.organisation)}` + return `/submit/link?dataset=${encodeURIComponent(dataset.dataset)}&orgName=${encodeURIComponent(organisation.name)}&orgId=${encodeURIComponent(organisation.organisation)}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/filters/checkToolDeepLink.js(1 hunks)src/filters/endpointSubmissionFormDeepLink.js(1 hunks)src/routes/form-wizard/check/steps.js(1 hunks)src/views/check/confirmation.html(2 hunks)src/views/organisations/get-started.html(1 hunks)src/views/organisations/http-error.html(3 hunks)test/unit/http-errorPage.test.js(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/views/organisations/http-error.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (8)
src/routes/form-wizard/check/steps.js (1)
99-99: Good addition of the PageController.Adding the controller to this route aligns it with the baseSettings pattern used throughout the file and ensures consistent behavior with other routes. This change supports the PR objective of introducing a PageController to manage the confirmation step effectively.
src/views/check/confirmation.html (1)
29-53: Excellent implementation of feature-flagged submission form option.The conditional rendering based on the
submitEndpointFormfeature flag provides a clean way to gradually roll out the new submission form functionality while maintaining the existing email method as a fallback. This change effectively addresses the PR objective of linking to the endpoint submission form.The instructions are clear and the terminology update from "data URL" to "endpoint URL" is consistently applied.
src/views/organisations/get-started.html (1)
188-188: Correct parameter passing to the deep link function.The function now correctly receives specific properties (
organisation.nameanddataset.dataset) rather than passing entire objects. This addresses the PR objective of fixing parameter passing in theendpointSubmissionFormToolDeepLinkfunction.src/filters/checkToolDeepLink.js (1)
10-12: Good defensive programming with null parameter checks.Adding validation for the
organisationanddatasetparameters prevents potential errors when these values are null or undefined. This improves the robustness of the deep linking functionality and addresses the PR objective of implementing null checks.This pattern should be applied consistently across all deep link functions to ensure comprehensive error handling.
test/unit/http-errorPage.test.js (3)
26-26: Terminology update looks goodThe update from "data URL" to "endpoint URL" aligns with the PR's terminology standardisation objective.
35-35: Terminology consistency maintainedThe consistent update of terminology in the summary list maintains coherent user experience.
56-56:Details
❓ Verification inconclusive
Verify whether 'orgId=undefined' is intentional
The test now correctly uses the updated deep link format with query parameters, but it's expecting 'orgId=undefined' in the URL. Is this the intended behaviour or should the test mock include a valid organisation ID?
🏁 Script executed:
#!/bin/bash # Check if other tests or implementation files expect undefined orgId echo "Searching for references to orgId=undefined in tests or views:" rg "orgId=undefined" --type js --type htmlLength of output: 403
Clarification on Organisation ID in Deep Link
After verifying across the codebase, the only reference to
orgId=undefinedis in this test. Please confirm whether the test intentionally omits a valid organisation ID, meaning the URL should indeed includeorgId=undefined, or if a valid organisation ID is expected and the test mock should be updated accordingly.src/filters/endpointSubmissionFormDeepLink.js (1)
10-12: Good defensive programming with null checksAdding null checks for organisation and dataset parameters improves error handling and prevents potential runtime errors, aligning with the PR objectives.
…-to-endpoint-submission-form-from-live-service
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…-to-endpoint-submission-form-from-live-service
…submission-form-from-live-service
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/filters/checkToolDeepLink.js(1 hunks)src/filters/endpointSubmissionFormDeepLink.js(1 hunks)src/routes/schemas.js(1 hunks)src/views/check/results/issueDetails.html(1 hunks)src/views/organisations/dataset-overview.html(1 hunks)src/views/organisations/datasetTaskList.html(1 hunks)src/views/organisations/get-started.html(3 hunks)src/views/organisations/issueDetails.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/organisations/get-started.html
- src/filters/checkToolDeepLink.js
🧰 Additional context used
🧠 Learnings (1)
test/unit/views/organisations/dataset-overview.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🪛 GitHub Check: test
test/unit/views/organisations/dataset-overview.test.js
[failure] 184-184: test/unit/views/organisations/dataset-overview.test.js > Dataset Overview Page > Renders dataset actions sections
AssertionError: expected '/check/link?dataset=article-4-directi…' to deeply equal '/check/link?dataset=article-4-directi…'
Expected: "/check/link?dataset=article-4-direction-area&orgName=mock-org&orgId=mock-org"
Received: "/check/link?dataset=article-4-direction-area&orgName=Mock%20org&orgId=Mock%20org"
❯ test/unit/views/organisations/dataset-overview.test.js:184:56
🪛 GitHub Actions: Unit test coverage report
test/unit/views/organisations/dataset-overview.test.js
[error] 184-184: AssertionError: expected '/check/link?dataset=article-4-direction-area&orgName=mock-org&orgId=mock-org' to deeply equal '/check/link?dataset=article-4-direction-area&orgName=mock-org&orgId=mock-org'
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (8)
src/views/organisations/datasetTaskList.html (1)
77-77: Changed from passing entire organisation object to just organisation.name.This change supports the PR's goal to improve deep linking by passing the specific name property to the checkToolDeepLink filter instead of the entire object. This is a good practice that makes the parameter usage more explicit.
src/views/organisations/issueDetails.html (1)
105-105: Updated to use organisation.name instead of the whole object.This change correctly updates the parameters passed to the checkToolDeepLink filter, using specific properties instead of whole objects, which aligns with the PR objectives to fix parameter passing in deep link functions.
src/views/organisations/issueTable.html (1)
138-138: Updated to use organisation.name and dataset.dataset explicitly.This change correctly updates the parameters passed to the checkToolDeepLink filter, using specific properties instead of whole objects. This is consistent with the changes in other templates and aligns with the PR objectives.
src/views/check/results/issueDetails.html (1)
82-82: Parameter passing update looks good.The parameter passing to
checkToolDeepLinkhas been updated to passoptions.lpaandoptions.datasetdirectly, which aligns with the updated function signature that now expects strings instead of objects.src/routes/schemas.js (1)
219-220: Good enhancement by adding the guidance field.Adding an optional
guidancefield to theissueSpecificationobject will allow for including helpful guidance information related to the issue, which can improve the user experience.src/views/organisations/dataset-overview.html (1)
212-212: Parameter passing update looks good.The updated parameter passing to
checkToolDeepLinknow usesorganisation.nameinstead of the entire organisation object, anddataset.datasetinstead of the entire dataset object, which aligns with the updated function signature that now expects strings instead of objects.src/filters/endpointSubmissionFormDeepLink.js (2)
4-6: JSDoc updated to match the new parameter types.The JSDoc has been correctly updated to indicate that both parameters are now strings instead of objects.
10-12: Good null check implementation.Adding validation for falsy parameters is a good defensive programming practice to prevent errors when required inputs are missing.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
github-project-automation
Bot
moved this from Code review & QA
to Ready for release
in Providers team (BACKLOG)
rosado
deleted the
772-link-to-endpoint-submission-form-from-live-service
branch
6 participants
Preview Link
https://submit-pr-930.herokuapp.com/
What type of PR is this? (check all applicable)
Description
This PR includes several improvements to deep linking and error handling:
the primary goal of this PR is to link the submit form.
additional changes
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
test out the links on the get-started page, check confirmation page and endpoint error page
Added/updated tests?
QA sign off
Summary by CodeRabbit
New Features
Bug Fixes