711 add boundary to map on the check tool playback page#719
Conversation
WalkthroughThe changes in this pull request introduce an additional query parameter, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
||||||||||||||||||||||||||||||||||||||||||||
DilwoarH
force-pushed
the
711-add-boundary-to-map-on-the-check-tool-playback-page
branch
from
ea9d6ab to
123bb8d
Compare
DilwoarH
had a problem deploying
to
development
GitHub Actions
Failure
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Outside diff range and nitpick comments (3)
src/controllers/deepLinkController.js (2)
11-12: Consider stricter validation for orgIdThe
NonEmptyStringvalidation might be too permissive fororgId. Consider implementing a more specific validation pattern that matches your LPA ID format.const OrgIdPattern = v.string([ v.regex(/^[a-zA-Z0-9-_]+$/), // adjust regex to match your LPA ID format v.message('Organisation ID must contain only alphanumeric characters, hyphens, and underscores') ]) const QueryParams = v.object({ dataset: NonEmptyString, orgName: NonEmptyString, orgId: OrgIdPattern })
Line range hint
48-54: Enhance error logging for validation failuresThe current error logging doesn't specify which parameter failed validation. Consider adding more detailed error information.
- logger.info('DeepLinkController.get(): invalid params for deep link, redirecting to start page', - { type: types.App, query: req.query }) + logger.info('DeepLinkController.get(): invalid params for deep link, redirecting to start page', + { + type: types.App, + query: req.query, + validationErrors: validationResult.issues?.map(issue => ({ + path: issue.path?.map(p => p.key).join('.'), + message: issue.message + })) + })test/unit/views/organisations/dataset-overview.test.js (1)
182-182: LGTM! Consider additional test coverageThe test correctly verifies the updated URL format with the orgId parameter.
Consider adding these test cases:
- Verify the URL when orgId is undefined
- Test the URL encoding of special characters in orgId
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/controllers/deepLinkController.js(3 hunks)src/filters/checkToolDeepLink.js(1 hunks)src/views/check/results/errors.html(1 hunks)src/views/check/results/no-errors.html(1 hunks)test/unit/deepLinkController.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🧰 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.
🔇 Additional comments (4)
src/filters/checkToolDeepLink.js (1)
10-10: Verify the organisation object structure
The code assumes organisation.organisation exists for the orgId parameter. Let's verify this assumption across the codebase.
✅ Verification successful
The organisation.organisation field is consistently used across the codebase
The verification shows that the organisation object structure with both name and organisation fields is well-established throughout the codebase:
- It's formally defined in
src/routes/schemas.jsasOrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, ... }) - The structure is consistently used across tests and middleware components
- The field is properly used in database queries and URL parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns where organisation object is constructed or passed
rg -A 2 "organisation\s*[:=]" --type js
Length of output: 15306
src/controllers/deepLinkController.js (1)
59-59: Verify session data structure compatibility
The modified session data structure now includes orgId. Let's verify that all consumers of this session data can handle this addition.
✅ Verification successful
Let me gather more information about how this session data is used in templates and views.
Based on the evidence gathered, I can now provide the final response:
The addition of orgId to the session data structure is safe and compatible
The codebase inspection shows that:
- The test suite already expects and validates the
orgIdfield in the session data - The templates correctly consume the
orgIdfield for boundary URL generation - No direct property access patterns that could break with the addition of
orgId
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of checkToolDeepLinkSessionKey in the codebase
rg "checkToolDeepLinkSessionKey" --type js -A 5
Length of output: 1820
Script:
#!/bin/bash
# Search for usage of deepLink in template files
rg "deepLink" --type html -A 5
# Search for any direct access to session data structure
rg "deepLink\." --type js -A 2
Length of output: 2422
src/views/check/results/errors.html (1)
103-103: LGTM! Verify API endpoint availability
The addition of the boundary URL is well-implemented with proper conditional rendering.
Let's verify the API endpoint exists:
✅ Verification successful
API endpoint is properly implemented and well-tested
The /api/get-lpa-boundary/:boundaryId endpoint is fully implemented with:
- Proper route handling in
src/routes/api.js - Robust boundary service implementation in
src/services/boundaryService.js - Comprehensive test coverage including error cases
- Proper error handling for invalid inputs and failed requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the LPA boundary API endpoint exists
# Expected: Should find the API endpoint implementation
ast-grep --pattern 'app.$_(["\']/api/get-lpa-boundary/:$_["\'], $_)'
Length of output: 116
Script:
#!/bin/bash
# Search for API route definitions with variations of the LPA boundary endpoint
rg -l "app\.(get|post)\s*\(['\"]\/api\/get-lpa-boundary"
# Search for any file containing the endpoint path
rg -l "\/api\/get-lpa-boundary"
# Look for route handlers or controller files
fd -t f "(routes|controller|api)" -x rg -l "get-lpa-boundary"
Length of output: 404
Script:
#!/bin/bash
# Check the API route implementation in the found file
cat src/routes/api.js
# Search for any related boundary endpoint implementations
rg -A 5 "get-lpa-boundary" src/routes/api.js
Length of output: 836
Script:
#!/bin/bash
# Check the boundary service implementation
cat src/services/boundaryService.js
# Look for any tests related to the boundary service
fd -t f "boundary.*test" -x cat {}
Length of output: 3583
src/views/check/results/no-errors.html (1)
107-109: LGTM! Consistent implementation
The boundary URL implementation matches the errors template, ensuring consistent behaviour across both success and error states.
| const query = { dataset: 'conservation-area', orgName: 'Some Org', orgId: 'some-org' } | ||
| const { req, res, next } = mockMiddlewareArgs({ query }) | ||
|
|
||
| deepLinkController.get(req, res, next) | ||
|
|
||
| expect(req.sessionModel.get(deepLinkController.checkToolDeepLinkSessionKey)).toStrictEqual({ | ||
| 'data-subject': 'conservation-area', | ||
| orgName: 'Some Org', | ||
| orgId: 'some-org', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with edge cases
The test suite would benefit from additional test cases:
- Invalid
orgIdformat - Missing
orgId - Special characters in
orgId
Here's a suggested addition:
it('should handle special characters in orgId', async () => {
const query = {
dataset: 'conservation-area',
orgName: 'Some Org',
orgId: 'org/with@special&chars'
}
const { req, res, next } = mockMiddlewareArgs({ query })
deepLinkController.get(req, res, next)
expect(req.sessionModel.get(deepLinkController.checkToolDeepLinkSessionKey))
.toHaveProperty('orgId', 'org/with@special&chars')
})
it('should redirect to check when orgId is missing', async () => {
const query = {
dataset: 'conservation-area',
orgName: 'Some Org'
}
const { req, res, next } = mockMiddlewareArgs({ query })
deepLinkController.get(req, res, next)
expect(res.redirect).toHaveBeenCalledWith('/check')
})
|
Needs #718 merged in first |
DilwoarH
force-pushed
the
699-change-the-way-we-get-boundary-geojson
branch
3 times, most recently
from
e29f9d7 to
a48dab7
Compare
DilwoarH
changed the base branch from
699-change-the-way-we-get-boundary-geojson
to
main
DilwoarH
force-pushed
the
711-add-boundary-to-map-on-the-check-tool-playback-page
branch
from
123bb8d to
46f5d7b
Compare
DilwoarH
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/controllers/deepLinkController.js (1)
48-48: Add JSDoc for the new orgId parameterConsider updating the method's JSDoc to document the new
orgIdquery parameter./** * Handles deep links in the Check Tool. * * It is meant to extract required params from query params * and partially pre-populate the session with them, * then redirect the user to the "next" page in the wizard + * @param {Object} req.query + * @param {string} req.query.dataset - The dataset identifier + * @param {string} req.query.orgName - The organisation name + * @param {string} req.query.orgId - The organisation ID */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/controllers/deepLinkController.js(3 hunks)src/filters/checkToolDeepLink.js(1 hunks)src/views/check/results/errors.html(1 hunks)src/views/check/results/no-errors.html(1 hunks)test/unit/deepLinkController.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/filters/checkToolDeepLink.js
- test/unit/deepLinkController.test.js
- src/views/check/results/errors.html
- src/views/check/results/no-errors.html
- test/unit/views/organisations/dataset-overview.test.js
🔇 Additional comments (2)
src/controllers/deepLinkController.js (2)
59-59: LGTM! Clean implementation of session data update
The changes correctly integrate the new orgId parameter into the session data, maintaining consistency with the existing pattern.
11-12: Verify handling of existing deep links without orgId
The addition of orgId as a required parameter could potentially break existing deep links that don't include this parameter. Whilst the validation will redirect users to '/check', we should verify if there are any existing deep links in production that need updating.
✅ Verification successful
Deep link changes are safely handled with graceful fallback
Based on the test file evidence, the deep link controller already implements proper handling for invalid parameters:
- It redirects to '/check' when parameters are invalid
- The test suite includes verification for both invalid cases and the new required
orgIdparameter - The session update logic is properly tested with the new
orgIdparameter
The implementation ensures a graceful fallback to the check tool start page when deep links are invalid, making this a safe change that won't disrupt user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing deep link URL construction or usage
rg -A 2 "\/check\?"
Length of output: 97
Script:
#!/bin/bash
# Search for deep link URL construction or usage with proper escaping
rg -A 2 "/check\?"
# Also search for potential URL construction patterns
rg -A 2 "createDeepLink|generateDeepLink|buildDeepLink"
# Look for test files that might contain deep link examples
fd -e test.js -e spec.js -X rg -A 2 "check\?"
Length of output: 126
Script:
#!/bin/bash
# Search for URL-related functions in the controller
ast-grep --pattern 'function $FUNC($$$) {
$$$
url $$$
$$$
}'
# Look for any URL generation or redirection logic
rg -l "url|redirect|location" src/controllers/deepLinkController.js
# Check for tests specific to deep linking
fd "deepLink.*test|deepLink.*spec"
# Look for route definitions that might use this controller
rg -A 2 "router.(get|post).*deepLink"
Length of output: 290
Script:
#!/bin/bash
# Check the test file for existing deep link test cases
cat test/unit/deepLinkController.test.js
# Look for validation schema history in git
git log -p -- src/controllers/deepLinkController.js | rg -A 5 -B 5 "NonEmptyString"
# Check for any documentation about deep linking
fd -e md -X rg -l "deep.?link"
Length of output: 1781
DilwoarH
deleted the
711-add-boundary-to-map-on-the-check-tool-playback-page
branch
3 participants
What type of PR is this? (check all applicable)
Description
Add boundary to check tool.
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
orgId, for enhanced deep link handling and validation.orgId, improving the context conveyed in links.boundaryGeoJsonUrlto the context for geographical data retrieval based onorgId.Bug Fixes
Tests
orgIdparameter in relevant functionalities.