Redirects /check and /submit to landing page#987
Conversation
This is to ensure users always go through our main user journey with proper deep linking set up.
WalkthroughThis pull request modifies the redirection behaviour in multiple controllers handling deep link requests. In the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CDC as CheckDeepLinkController
participant Logger as Logger
User->>CDC: GET /?invalid_params
CDC->>Logger: Log "redirecting to landing page"
CDC->>User: res.redirect("/")
sequenceDiagram
participant User
participant EDFC as EndpointSubmissionFormDeepLinkController
participant Logger as Logger
User->>EDFC: GET with invalid query parameters
EDFC->>Logger: Log invalid parameters
EDFC->>User: res.redirect("/")
sequenceDiagram
participant User
participant Router as Router
participant CSC as checkStartController
User->>Router: GET /
Router->>CSC: Invoke get() method
CSC->>User: res.redirect("/")
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (3)
src/controllers/checkStartController.js (2)
1-1: Remove redundant 'use strict' directiveES modules run in strict mode by default, so this directive is unnecessary.
-'use strict' import PageController from './pageController.js'🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
5-5: Follow PascalCase naming convention for controller classesOther controllers in this codebase follow PascalCase naming convention (as seen in references like
PageController). Maintaining consistent naming conventions improves code readability and maintainability.-class checkStartController extends PageController { +class CheckStartController extends PageController {Don't forget to update the export statement as well:
-export default checkStartController +export default CheckStartControllertest/unit/endpointSubmissionFormLinkController.test.js (1)
28-33: Update test name to match the new expected behaviourThe test name "should throw a 400 error when params invalid" no longer accurately reflects the actual behaviour being tested. The implementation has changed from throwing an error to redirecting to the landing page. Consider updating the test name to something like "should redirect to landing page when params invalid" to better align with the actual assertion.
- it('should throw a 400 error when params invalid', async () => { + it('should redirect to landing page when params invalid', async () => { const { req, res, next } = mockMiddlewareArgs({ query: {} }) endpointSubmissionFormDeepLinkController.get(req, res, next) expect(res.redirect).toHaveBeenCalledWith('/') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/controllers/checkDeepLinkController.js(1 hunks)src/controllers/checkStartController.js(1 hunks)src/controllers/endpointSubmissionFormDeepLinkController.js(1 hunks)src/routes/form-wizard/check/steps.js(2 hunks)test/PageObjectModels/startPage.js(1 hunks)test/unit/checkDeepLinkController.test.js(1 hunks)test/unit/endpointSubmissionFormLinkController.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/controllers/checkStartController.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (7)
src/controllers/checkStartController.js (1)
6-8: Redirect implementation looks correctThe implementation successfully fulfills the PR objective of redirecting to the landing page.
test/unit/checkDeepLinkController.test.js (1)
32-32: Redirect path updated correctlyThe test now correctly expects a redirect to the landing page ('/'). This aligns with the PR objective of redirecting the
/checkendpoint to the landing page.test/PageObjectModels/startPage.js (1)
6-6: Updated URL path with appropriate query parametersThe StartPage constructor now initializes with a specific deep link URL including dataset and organisation parameters. This change ensures the test properly accesses the check functionality via the deep link mechanism rather than directly accessing '/check/'.
src/routes/form-wizard/check/steps.js (2)
14-14: New controller import looks goodThe checkStartController is correctly imported to be used in the route definition.
24-24: Root route controller correctly updatedThe root route now correctly uses the checkStartController, which will redirect to the landing page. This implements the PR objective of redirecting routes to ensure users navigate through the main user journey.
src/controllers/checkDeepLinkController.js (1)
51-53: Consistent redirection implemented correctlyThe redirection to the landing page has been correctly implemented, which aligns with the PR objectives to ensure users consistently navigate through the main journey.
src/controllers/endpointSubmissionFormDeepLinkController.js (1)
42-46: Improved validation and redirection logicThis change correctly implements the redirection to the landing page and adds an additional validation check to ensure the dataset exists in the datasets collection. The error handling has been appropriately replaced with a redirect mechanism, ensuring a better user experience for invalid deep links.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/acceptance/request_check.test.js (1)
107-108:Details
❓ Verification inconclusive
Verify that error handling still provides adequate user feedback.
The test has been simplified to check for a single error message rather than detailed validations that were likely removed. This aligns with the PR objective of redirecting users to the landing page, but ensure this simplified approach still provides adequate error feedback to users.
While the new message clearly states that issues must be fixed before submission, consider whether additional contextual information (like specific error details) might still be necessary for users to understand what needs fixing.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase where similar error message checks were updated # This will help verify consistency in the error handling approach # Search for similar error message expectations in test files rg -A 2 -B 2 "You cannot submit your data" --glob "test/**/*.js" # Check if there are any other error message checks that might need updating rg -A 2 -B 2 "description column is missing" --glob "test/**/*.js" # Look at the removed error checks from git history git log -p -- test/acceptance/request_check.test.js | grep -B 5 -A 5 "errorSummary" | head -n 20Length of output: 939
Ensure Simplified Error Feedback Remains Clear for Users
The test now exclusively validates that the error message "You cannot submit your data to the platform until you fix these issues." is visible after clicking the "description column is missing" indication. This approach aligns with the PR’s objective of redirecting users to the landing page. However, please verify that this simplified error handling still delivers adequate and clear feedback, as users might benefit from retaining some specific context (e.g. delineating which input is problematic) to efficiently rectify the issues.
- File:
test/acceptance/request_check.test.js(Lines 107-108)- Confirm that no additional error details are necessary, or if supplementary contextual messages would enhance user understanding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/acceptance/request_check.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
Changes the flow due to deep linking
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
test/PageObjectModels/startPage.js (1)
4-6: Updated URL path is now tied to specific test data.The constructor now uses a very specific URL with hardcoded query parameters (dataset, organisation name, and organisation ID). This might make the StartPage less reusable for tests with different contexts or datasets.
Consider making the URL parameters configurable through constructor arguments to increase reusability:
- constructor (page) { - super(page, '/check/link?dataset=article-4-direction-area&orgName=Newcastle%20City%20Council&orgId=local-authority%3ANET') + constructor (page, dataset = 'article-4-direction-area', orgName = 'Newcastle%20City%20Council', orgId = 'local-authority%3ANET') { + super(page, `/check/link?dataset=${dataset}&orgName=${orgName}&orgId=${orgId}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/PageObjectModels/startPage.js(1 hunks)test/integration/back_buttons.playwright.test.js(3 hunks)test/integration/pages_load_ok.playwright.test.js(6 hunks)test/integration/validation_errors.playwright.test.js(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
test/integration/back_buttons.playwright.test.js (2)
test/integration/validation_errors.playwright.test.js (14)
startPage(7-7)startPage(29-29)startPage(51-51)startPage(76-76)datasetPage(13-13)datasetPage(35-35)datasetPage(57-57)datasetPage(82-82)geometryTypePage(16-16)uploadMethodPage(10-10)uploadMethodPage(32-32)uploadMethodPage(54-54)uploadMethodPage(79-79)uploadFilePage(63-63)test/PageObjectModels/startPage.js (1)
StartPage(3-7)
test/integration/validation_errors.playwright.test.js (1)
test/PageObjectModels/startPage.js (1)
StartPage(3-7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (21)
test/integration/back_buttons.playwright.test.js (7)
4-8: Imports now explicitly include page objects used in tests.The imports have been updated to include the DatasetPage, UploadMethodPage, and LandingPage, making it clearer which page objects are being used in the tests.
17-25: Navigation flow updated to match new application behaviour.The test now verifies that users can navigate from UploadMethodPage back to DatasetPage, and then finally to LandingPage, which aligns with the PR objective of redirecting users to the landing page.
32-46: Navigation testing for geometry type page now follows consistent pattern.This change maintains the consistent pattern of verifying the UploadMethodPage first, then going back to DatasetPage before proceeding with the specific test flow.
53-61: Upload method page navigation test follows the same pattern as other tests.The consistent approach of starting at UploadMethodPage and navigating back to DatasetPage improves code readability and maintenance.
68-88: Geometry type navigation flow maintains consistency with other tests.The navigation pattern is consistent with other tests, ensuring thorough testing of back button functionality from various pages in the application flow.
95-117: Upload file page navigation test updated to match new flow.This test follows the same pattern as others, providing consistent coverage of navigation paths through the application.
124-146: Upload URL page navigation test updated to match new flow.The test maintains consistency with other tests and ensures proper coverage of back button functionality in the URL upload flow.
test/integration/validation_errors.playwright.test.js (5)
3-4: Import statements updated to include page object classes.The imports now explicitly include DatasetPage and UploadMethodPage classes in addition to their exported constants, which improves code clarity.
10-13: Geometry type test now follows the new navigation pattern.The test has been updated to start from UploadMethodPage and navigate back to DatasetPage before continuing with the test logic, which aligns with the new application flow.
32-35: Upload method test navigation updated for consistency.The test now follows the same pattern of starting at UploadMethodPage and navigating back to DatasetPage before proceeding with specific test logic.
54-57: File upload test navigation updated to match new flow.This test maintains consistency with the new navigation pattern implemented across all tests.
79-82: URL upload test navigation updated to match new flow.The test ensures consistent navigation through the application, starting from UploadMethodPage and navigating back to DatasetPage.
test/integration/pages_load_ok.playwright.test.js (9)
7-9: Updated imports to include page object classes.The imports now explicitly include DatasetPage and UploadMethodPage classes along with their exported constants, improving code clarity.
49-55: Updated test to reflect new routing path.The test case was renamed from "/check/dataset" to "/check" and updated to verify that the UploadMethodPage is accessible, which aligns with the PR objective of redirecting "/check" to the landing page.
61-64: Geometry-type test navigation updated to match new flow.The test now follows the consistent pattern of starting at UploadMethodPage and navigating back to DatasetPage before proceeding with specific test logic.
76-79: Upload-method test navigation updated for consistency.The test maintains the consistent navigation pattern implemented across all tests in the application.
95-109: Upload test navigation and variable naming updated.The test follows the new navigation pattern and renames the second instance of uploadMethodPage to uploadMethodPage2 to avoid confusion, which is a good practice.
118-132: URL test navigation and variable naming updated.The test follows the new navigation pattern and uses consistent variable naming for page objects, improving code clarity and maintainability.
144-144: URL path updated in assertion to include "/check" prefix.The assertion has been updated to check for '/check/status/processing' instead of '/status/processing', which aligns with the new URL structure.
156-156: URL path updated in assertion to include "/check" prefix.The assertion has been updated to check for '/check/results/processing' in the URL, reflecting the new URL structure.
163-163: URL path updated in assertion to include "/check" prefix.The assertion now checks for '/check/results/completed/1' in the URL, aligning with the new URL structure.
DilwoarH
force-pushed
the
985-bug-check-tool-confirmation-page-send-invalid-values-to-endpoint-submission-form
branch
from
fc54953 to
cbafbe9
Compare
DilwoarH
deleted the
985-bug-check-tool-confirmation-page-send-invalid-values-to-endpoint-submission-form
branch
3 participants
Preview Link
https://submit-pr-987.herokuapp.com/
What type of PR is this? (check all applicable)
Description
Redirects
/checkand/submitto landing page. This ensures users always go through our main user journey with proper deep linking set up.Related Tickets & Documents
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
New Features
Bug Fixes
Refactor