added checkTool link to jira ticket#1078
Conversation
WalkthroughThe update introduces a new processing step in the endpoint submission flow, including a dedicated controller and template for displaying processing status with client-side polling. The CheckAnswersController logic is revised to handle asynchronous URL requests and local validation, returning both issue and request ID. Related tests and route definitions are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CheckAnswersController
participant AsyncRequestApi
participant SubmitUrlController
participant ProcessingController
participant ProcessingTemplate
User->>CheckAnswersController: POST /submit/check-answers
CheckAnswersController->>SubmitUrlController: localUrlValidation(url)
alt Validation fails
CheckAnswersController-->>User: Redirect to /submit/check-answers (error)
else Validation passes
CheckAnswersController->>AsyncRequestApi: postUrlRequest(formData)
AsyncRequestApi-->>CheckAnswersController: requestId
CheckAnswersController->>Jira: createCustomerRequest(description with check tool link)
CheckAnswersController-->>User: Redirect to /submit/processing/{requestId}
end
User->>ProcessingController: GET /submit/processing/{requestId}
ProcessingController->>ProcessingTemplate: Render processing.html with options
loop Polling (JS in Template)
ProcessingTemplate->>API: GET /api/endpoint-request-status/{requestId}
alt Status COMPLETE or FAILED
ProcessingTemplate-->>User: Redirect to confirmation page
else Retry < 20
ProcessingTemplate->>API: Repeat after 3s
else
ProcessingTemplate-->>User: Show "taking longer than expected" message
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/CheckAnswersController.js(3 hunks)test/unit/checkAnswersController.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/controllers/CheckAnswersController.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: #593
File: index.js:20-20
Timestamp: 2024-10-29T14:58:22.843Z
Learning: The function initDatasetSlugToReadableNameFilter() in src/utils/datasetSlugToReadableName.js already includes appropriate error handling and logging.
test/unit/checkAnswersController.test.js (2)
Learnt from: GeorgeGoodall-GovUk
PR: #609
File: test/unit/noErrorsPage.test.js:68-68
Timestamp: 2024-11-12T10:54:09.485Z
Learning: In test/unit/noErrorsPage.test.js, avoid using prettifyColumnName for column headers because the table can contain spec fields, as requested by Alex.
Learnt from: rosado
PR: #657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In test/unit/middleware/issueDetails.middleware.test.js, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for issueEntitiesCount.
🔇 Additional comments (8)
test/unit/checkAnswersController.test.js (5)
5-5: LGTM: Import addition for new async request service.The import for
postUrlRequestis correctly added to support the new check tool integration functionality.
8-8: LGTM: Mock setup for async request API.The mock setup follows existing patterns and is properly placed alongside other service mocks.
28-28: LGTM: Mock reset in beforeEach.Properly resets the mock to ensure test isolation between test cases.
68-68: LGTM: Mock implementation for URL request.The mock correctly resolves with a fixed
'requestId'string that will be used to construct the check tool URL.
87-94: LGTM: Test verification of new integration points.The test properly verifies that:
postUrlRequestis called during execution- The Jira description contains the constructed check tool URL with the expected format
This ensures the new async request integration is working as intended.
src/controllers/CheckAnswersController.js (3)
7-9: LGTM: Import additions for new functionality.The imports for
postUrlRequest,SubmitUrlController, anddatasetsare correctly added to support the check tool integration feature.
65-71: LGTM: Form data construction for async request.The form data object is properly constructed with the required fields:
- URL from request body or session
- Dataset from session
- Collection from dataset metadata
- Geometry type from session
This follows a logical structure for the async request.
94-95: LGTM: Integration of check tool link in Jira description.The check tool link is properly appended to the Jira issue description, providing easy access to the validation results within the ticket.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
DilwoarH
left a comment
DilwoarH
left a comment
There was a problem hiding this comment.
Code generally looks good but I think we'll need a loading spinner or some sort of page that shows users not to kill the request
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/controllers/ProcessingController.js (1)
7-8: Consider making URLs configurable.The polling endpoint and confirmation URL are hardcoded. Consider using configuration values for better maintainability and environment flexibility.
- req.form.options.pollingEndpoint = `/api/status/${requestId}` - req.form.options.confirmationUrl = '/submit/confirmation' + req.form.options.pollingEndpoint = `${config.api.baseUrl}/status/${requestId}` + req.form.options.confirmationUrl = config.routes.confirmation || '/submit/confirmation'src/views/submit/processing.html (1)
30-30: Make polling intervals configurable.Hardcoded polling intervals make it difficult to adjust behaviour for different environments or requirements.
Consider passing polling intervals as template variables:
+ const pollingInterval = {{ options.pollingInterval | default(3000) }}; + const retryInterval = {{ options.retryInterval | default(5000) }}; const finishedStatuses = ['COMPLETE', 'FAILED'];Then use these variables in the setTimeout calls.
src/controllers/CheckAnswersController.js (1)
79-84: Consider adding validation for form data fields.While URL validation is handled, other form data fields like
datasetandgeomTypearen't validated before being sent to the external service.const formData = { url, - dataset: req.sessionModel.get('dataset'), + dataset: req.sessionModel.get('dataset') || 'unknown', collection: datasetMeta.dataSubject, - geomType: req.sessionModel.get('geomType') + geomType: req.sessionModel.get('geomType') || 'unknown' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/CheckAnswersController.js(6 hunks)src/controllers/ProcessingController.js(1 hunks)src/routes/form-wizard/endpoint-submission-form/steps.js(2 hunks)src/views/submit/processing.html(1 hunks)test/unit/checkAnswersController.test.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/checkAnswersController.test.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests / test
🔇 Additional comments (7)
src/routes/form-wizard/endpoint-submission-form/steps.js (2)
5-5: Import statement looks good.The import for ProcessingController follows the existing pattern and naming convention.
46-52: Processing route configuration is well-structured.The new route properly integrates with the existing form wizard pattern and includes appropriate configuration options.
src/controllers/CheckAnswersController.js (5)
24-30: Session management integration looks good.The destructuring of
issueandrequestIdfrom the enhancedcreateJiraServiceRequestmethod and setting of session variables properly supports the new processing flow.
66-77: Error handling for missing/invalid URLs has been properly addressed.The implementation now correctly handles both missing URLs and validation failures by setting appropriate error messages and redirecting back to the check-answers page. This addresses the concerns raised in past reviews.
85-95: Async URL request error handling looks good.The try-catch block properly handles
postUrlRequestfailures and setsrequestIdto null on error, which is then handled gracefully in the check tool link generation. This addresses the past review concern.
96-98: Check tool link generation is well-implemented.The conditional check for
requestIdand fallback message properly handles the case where the async request fails. The use ofconfig.jira.requestTypeIdinstead of hardcoded values is also good practice.
150-150: Return structure properly supports new flow.The enhanced return value with both
issueandrequestIdcorrectly supports the new processing workflow while maintaining backward compatibility.
feba-rajan
force-pushed
the
feat/create_check_url_request
branch
from
95a464a to
e8dbffa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/controllers/processingController.js (1)
5-8: Good error handling for missing request ID.The validation and early return with appropriate HTTP status code is well-implemented. Consider adding validation for malformed request IDs if needed based on your ID format requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/processingController.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/controllers/processingController.js (3)
1-3: LGTM! Clean class structure and appropriate inheritance.The import, class declaration extending PageController, and export are well-structured and follow established patterns.
Also applies to: 26-26
9-20: Well-structured form options configuration.The setup provides comprehensive configuration for the processing page including polling endpoint, confirmation URL, and appropriate user-facing text for different states. The dynamic polling endpoint construction using the requestId is correctly implemented.
22-23: Proper delegation to parent class.The call to
super.get(req, res, next)correctly delegates to the parent PageController after setting up the processing-specific options, maintaining the established controller pattern.
DilwoarH
left a comment
DilwoarH
left a comment
There was a problem hiding this comment.
Looks great, well done 🏅
I tested it with a small Article 4-direction area dataset, and it worked well. I also checked it with a large tree dataset, and it worked well, too.
Some feedback:
- Add tests for the processing Controller
- You should also look at the names of existing controllers using the Pascal case.
- Change the content for the processing page to say something like this:
Please wait while we check your submission. Do not exit this page; large datasets may take a few minutes.
Check with @maddie-broxup
There was a problem hiding this comment.
Do we need add some tests for this?
There was a problem hiding this comment.
sure, will add some
There was a problem hiding this comment.
please can you share a screenshot of what this looks like currently and i can write some content for it? thanks for tagging me!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/processingController.test.js (1)
23-23: Remove redundant nested describe block.The nested
describe('ProcessingController()')block has the same name as the parent block, which creates confusion and serves no organisational purpose.- describe('ProcessingController()', () => { + describe('get method', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/processingController.test.js(1 hunks)
🔇 Additional comments (3)
test/unit/processingController.test.js (3)
1-21: LGTM! Well-structured test setup.The import statements and test setup follow Vitest best practices with proper mocking of Express.js request/response objects and clean beforeEach initialisation.
24-39: LGTM! Comprehensive test coverage for valid request ID scenario.The test thoroughly validates that all expected form options are properly set when a valid request ID is provided, including polling configuration and UI text elements.
40-45: LGTM! Proper error handling validation.The test correctly validates that the controller responds with a 400 status and renders an error page when the request ID is missing.
4 participants
Description
PR updates the submit form flow to trigger the check tool automatically when a endpoint URL is submitted. A link to the check tool results is attached to the Jira ticket created.
What type of PR is this? (check all applicable)
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests