Auto plugin detection#1144
Conversation
WalkthroughPolls the async request API after creating an async check to retrieve plugin data, builds a CSV including dynamic plugin and organisationName, and attaches it to Jira asynchronously; organisationName propagated into form payloads; ResultData gains getPlugin(); errorDetail support and templates/tests updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Controller as CheckAnswersController
participant Session
participant AsyncAPI as asyncRequestApi
participant AsyncStore as "Async Request Store"
participant ResultModel as ResultData
participant Jira as JiraService
participant View as Results/View
User->>Controller: submit form
Controller->>Session: read session org / organisationName
Session-->>Controller: organisationName
Controller->>AsyncAPI: postUrlRequest / postFileRequest (includes organisationName)
AsyncAPI->>AsyncStore: enqueue request -> returns requestId
AsyncStore-->>AsyncAPI: requestId
AsyncAPI-->>Controller: response (requestId)
alt background attachment (async, non-blocking)
Controller->>AsyncAPI: getRequestData(requestId) (GET, timeout 15s)
AsyncAPI->>AsyncStore: HTTP GET
AsyncStore-->>AsyncAPI: request data (includes plugin)
AsyncAPI-->>Controller: ResultModel
Controller->>ResultModel: getPlugin()
ResultModel-->>Controller: plugin or null
Controller->>Jira: attachFileToIssue(requestId, CSV-with-plugin, description) (async)
Jira-->>Controller: attach response / errors logged (non-blocking)
end
Controller->>View: render/redirect with organisationName
View-->>User: results (datasetBanner receives organisationName)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/views/check/error-redirect.html (1)
11-19: Fix broken ErrorSummary anchor whenerr.errorDetailis present.The error summary links to
#bad-upload, but that id is only rendered in theelsebranch—so witherr.errorDetail, the summary link target disappears.{% if err.errorDetail %} {% if err.errorDetail is iterable and err.errorDetail is not string %} {% for detail in err.errorDetail %} - <p class="govuk-body">{{ detail }}</p> + <p class="govuk-body"{% if loop.first %} id="bad-upload" tabindex="-1"{% endif %}>{{ detail }}</p> {% endfor %} {% else %} - <p class="govuk-body">{{ err.errorDetail }}</p> + <p class="govuk-body" id="bad-upload" tabindex="-1">{{ err.errorDetail }}</p> {% endif %} {% else %} <p class="govuk-body" id="bad-upload" tabindex="-1"> Error: {{ err.message }} </p> {% endif %}Also applies to: 23-35
src/services/asyncRequestApi.js (1)
61-71: FixgetRequestDataerror status reporting for Axios errors.
error.statusis undefined for Axios errors; useerror.response.statusinstead. Line 70 currently logs an undefined status. Since 404 errors are already handled explicitly, simplify the catch block:export const getRequestData = async (resultId, opts = undefined) => { const url = new URL(`${config.asyncRequestApi.url}/${config.asyncRequestApi.requestsEndpoint}/${resultId}`) try { const response = await axios.get(url) return new ResultData(response.data) } catch (error) { if (error?.response?.status === 404) { throw error } - throw new Error(`HTTP error! status: ${error.status}: ${error.message}`, { cause: error }) + const status = error?.response?.status ?? 'unknown' + throw new Error(`HTTP error! status: ${status}: ${error?.message ?? 'unknown error'}`, { cause: error }) } }
🧹 Nitpick comments (5)
src/controllers/submitUrlController.js (1)
199-209: Consider removing unused validation function.Since the filetype validation has been removed from the validation chain, the
validateAcceptedFileTypefunction is now dead code. If the filetype check is being permanently removed (as indicated by the TODO comment), consider removing this function as well to reduce maintenance burden.Alternatively, if you intend to keep it for future reference or potential re-enablement, add a comment explaining why it's preserved.
test/acceptance/request_check.test.js (1)
312-316: Scope error-message matching to the error region to avoid false positives.
page.getByText(errorMessage)can match incidental content; consider scoping to a known container (e.g..govuk-error-summary/ main content) so “foundError” only trips on the rendered error UI.src/utils/errors.js (1)
17-34: Documentoptions.errorDetailinMiddlewareErrorJSDoc.Helps callers know whether this expects
string,string[], etc.src/controllers/resultsController.js (1)
73-83: Tidy error copy + double-checkerrMsgDetailis safe to show to end-users.
- Minor: “occured” → “occurred” (Line 79).
- Please confirm
errMsgDetaildoesn’t include stack traces/secrets from the async service.- return next(new MiddlewareError('An unknown error occured when processing your endpoint', 500, { template: 'check/error-redirect.html' })) + return next(new MiddlewareError('An unknown error occurred when processing your endpoint', 500, { template: 'check/error-redirect.html' }))test/unit/checkAnswersController.test.js (1)
193-237: Make CSV assertions more specific thantoContain('plugin')/toContain('wfs').To reduce false positives, consider asserting the header row includes a
plugincolumn and that the corresponding row containswfsin that column (e.g. split lines by\nand commas).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/controllers/CheckAnswersController.js(3 hunks)src/controllers/resultsController.js(1 hunks)src/controllers/submitUrlController.js(2 hunks)src/controllers/uploadController.js(1 hunks)src/models/requestData.js(1 hunks)src/services/asyncRequestApi.js(2 hunks)src/utils/errors.js(1 hunks)src/views/check/error-redirect.html(1 hunks)src/views/check/results/results.html(1 hunks)test/acceptance/request_check.test.js(1 hunks)test/unit/checkAnswersController.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T10:13:37.085Z
Learnt from: rosado
Repo: digital-land/submit PR: 831
File: src/controllers/resultsController.js:67-75
Timestamp: 2025-01-30T10:13:37.085Z
Learning: Express.js async middleware functions require explicit error handling using try/catch blocks to properly handle asynchronous errors and prevent unhandled promise rejections.
Applied to files:
src/controllers/resultsController.js
🧬 Code graph analysis (5)
src/controllers/resultsController.js (1)
src/utils/errors.js (1)
MiddlewareError(17-35)
src/models/requestData.js (1)
src/utils/logger.js (1)
logger(21-28)
src/controllers/CheckAnswersController.js (1)
src/services/asyncRequestApi.js (2)
getRequestData(61-72)getRequestData(61-72)
src/services/asyncRequestApi.js (1)
src/utils/logger.js (1)
logger(21-28)
test/unit/checkAnswersController.test.js (1)
src/services/asyncRequestApi.js (6)
getRequestData(61-72)getRequestData(61-72)postUrlRequest(23-34)postUrlRequest(23-34)response(44-44)response(64-64)
⏰ 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 (8)
src/controllers/submitUrlController.js (1)
79-86: LGTM! Sensible handling for servers that don't support HEAD.The 405 status handling is appropriate, as some servers disallow HEAD requests. Skipping post validators in this scenario is reasonable, and the logging provides good observability.
src/views/check/results/results.html (1)
48-48: EnsuredatasetBannerhandles missing/emptyorganisationName.If
options.requestParams.organisationNamecan be undefined (e.g. older deep-links), confirm the macro gracefully falls back without rendering “undefined”.src/controllers/uploadController.js (1)
9-17: Verify session key consistency fororganisationName.This reads
deep-link-session-key.orgName; please confirm all entrypoints that reach upload have that key populated (or you have a deliberate fallback elsewhere).src/services/asyncRequestApi.js (1)
9-34: Confirm async API expectsorganisationName(camelCase) and consider log redaction.Two things to sanity-check:
- If the async API expects snake_case params,
organisationNamemay be silently dropped.logger.debug('postUrlRequest', { url, ... })risks logging secrets in querystrings; consider redacting or logging host-only.test/unit/checkAnswersController.test.js (1)
5-6: Good: test now coversgetRequestDataintegration for plugin retrieval.Nice to see the controller’s new dependency explicitly imported and mockable in unit tests.
src/models/requestData.js (1)
134-140: LGTM! Consistent implementation.The
getPlugin()method follows the same defensive pattern as existing methods in this class (e.g.,getError(),getColumnFieldLog()), with appropriate null checks and logging. The implementation correctly returnsnullwhen the response data is unavailable.src/controllers/CheckAnswersController.js (2)
76-82: LGTM! Organisation name correctly included in API request.The addition of
organisationNametoformDatacorrectly passes the organisation context to the async request API, enabling plugin detection to be associated with the organisation. This is distinct from thedata.organisationNameused for Jira issue creation (line 53).
144-145: Confirm plugin field requirement with Jira integration team.When
requestDataisnullorgetPlugin()returnsnull, the CSV will contain an empty value for the plugin column. This is standard csv-stringify v6.5.2 behaviour (null/undefined values convert to empty fields). Verify whether the downstream Jira CSV import process requires this field to be populated, or confirm that empty plugin values are acceptable.
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
…ssage display content change
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/views/check/error-redirect.html (1)
41-41: Consider "an HTML" for consistency with standard grammar.The phrase "a HTML webpage" could be refined to "an HTML webpage" to align with standard British English grammar conventions.
- <p class="govuk-body" id="bad-upload" tabindex="-1">The URL returns a HTML webpage. This must be a data file.</p> + <p class="govuk-body" id="bad-upload" tabindex="-1">The URL returns an HTML webpage. This must be a data file.</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config/default.yaml(1 hunks)config/development.yaml(0 hunks)config/index.js(1 hunks)config/staging.yaml(0 hunks)readme.md(1 hunks)src/controllers/CheckAnswersController.js(3 hunks)src/views/check/error-redirect.html(1 hunks)test/acceptance/request_check.test.js(3 hunks)test/unit/checkAnswersController.test.js(5 hunks)
💤 Files with no reviewable changes (2)
- config/development.yaml
- config/staging.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/checkAnswersController.test.js
- test/acceptance/request_check.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/controllers/CheckAnswersController.js (2)
src/services/jiraService.js (3)
data(22-30)data(60-60)attachFileToIssue(50-92)src/services/asyncRequestApi.js (4)
response(44-44)response(64-64)getRequestData(61-72)getRequestData(61-72)
⏰ 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 (8)
src/views/check/error-redirect.html (1)
24-53: Excellent improvement to error messaging.The specific error messages for SSL certificate failures and file format issues provide much clearer, actionable guidance to users compared to generic error text. The SSL explanation appropriately balances technical detail with accessibility, and the file format guidance correctly lists the accepted formats including ArcGIS and WFS.
config/default.yaml (1)
106-106: Verify that request type ID 28 is correct for the Jira service desk.The requestTypeId has been changed from 1 to 28 in the default configuration, which is now used consistently across all environments (development, staging, and production). This must correspond to an actual request type configured in your Jira service desk; an incorrect ID would cause all Jira ticket creation to fail. Please confirm with your Jira administrator that this request type ID is valid for the asynchronous attachment workflow.
readme.md (1)
177-179: Helpful clarification for Sandbox setup.The addition of Sandbox URL guidance and the alternative spinup section improves the onboarding experience for local Jira development.
src/controllers/CheckAnswersController.js (5)
76-82: LGTM!The addition of
organisationNameto the form data correctly aligns with the PR objective to store organisation name in parameters rather than relying solely on session data.
128-137: LGTM!The fire-and-forget pattern with
.catch()for error handling is appropriate here. The CSV attachment runs in the background without blocking the user response, and failures are logged but don't affect the main flow.
159-180: Polling logic is sound.The retry mechanism with a maximum of 6 attempts and 5-second intervals provides reasonable tolerance for the async API response delay. The early exit when
isComplete()returns true is correct—no point retrying if the request has finished processing.
184-187: CSV schema update looks good.The dynamic
pluginvalue and the addition of thelicencecolumn align with the PR objectives. Note that if plugin retrieval fails after all retries,csv-stringifywill output an empty cell for the plugin field, which provides graceful degradation.
190-200: LGTM!Error handling for attachment failures is comprehensive with appropriate logging.
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
readme.md (3)
177-178: Fix run-on sentence; clarify Sandbox-vs-alternative guidance.Line 177 contains a comma splice joining two independent clauses. Additionally, the phrase "this only requires the three env parameters for JIRA to be filled in" is unclear about when/whether to use Sandbox versus the alternative spinup below.
Rewrite lines 177–178 to separate the ideas and clarify the choice:
-Ideally use the Jira Sandbox URL, this only requires the three env parameters for JIRA to be filled in, The JIRA_BASIC_AUTH and JIRA_SERVICE_DESK_ID are the same as the production environment, only the URL needs to be changed to use the Sandbox Jira environment. +For local development, use the Jira Sandbox URL if possible—it requires only the three JIRA environment variables. The JIRA_BASIC_AUTH and JIRA_SERVICE_DESK_ID remain the same as production; only the JIRA_URL changes to the Sandbox environment. + +If you prefer to set up your own instance, follow the alternative steps below.
182-182: Standardise quote style in step 2.The phrase uses both double and single quotes inconsistently:
"Create a 'Customer Desk' Project". For readability, use uniform quoting.-2. Click on Try for free and "Create a 'Customer Desk' Project" +2. Click on "Try for free" and create a "Customer Desk" Project
183-185: Clarify vague instructions in steps 3–5.Steps 3–5 reference actions ("Fill in the form", "create a new user", "create a new project") without specifying where these options are located in the Jira UI or what information to enter.
Expand steps 3–5 with clearer UI navigation cues:
-3. Fill in the form and create a test instance -4. In the "Users" section, create a new user -5. In the "Projects" section, create a new project +3. Fill in the form with your details and create your test instance. +4. Navigate to the admin or user management area and create a new user account; note the email and temporary password. +5. In the Projects section, create a new project (or verify that a default project exists).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
readme.md(1 hunks)src/views/check/error-redirect.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/check/error-redirect.html
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/checkAnswersController.test.js (1)
206-249: LGTM! Consider adding edge case coverage.The test correctly validates that plugin data is retrieved and included in the CSV. The CSV content verification on lines 242-246 is thorough.
Optionally, consider adding test cases for edge scenarios:
- Plugin remains null after maximum retries
getRequestDatafails repeatedlyisComplete()returns true but plugin is nullThese would help ensure robustness of the polling mechanism, but the current test coverage is adequate for the happy path.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/CheckAnswersController.js(4 hunks)test/unit/checkAnswersController.test.js(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit 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`.
Applied to files:
test/unit/checkAnswersController.test.js
🧬 Code graph analysis (2)
test/unit/checkAnswersController.test.js (2)
src/services/asyncRequestApi.js (6)
response(44-44)response(64-64)getRequestData(61-72)getRequestData(61-72)postUrlRequest(23-34)postUrlRequest(23-34)src/services/jiraService.js (3)
data(22-30)data(60-60)attachFileToIssue(50-92)
src/controllers/CheckAnswersController.js (2)
src/services/jiraService.js (3)
data(22-30)data(60-60)attachFileToIssue(50-92)src/services/asyncRequestApi.js (4)
response(44-44)response(64-64)getRequestData(61-72)getRequestData(61-72)
⏰ 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/controllers/CheckAnswersController.js (4)
7-7: LGTM!The
getRequestDataimport is correctly added and used in the newattachFileToIssuemethod for polling plugin data.
76-82: LGTM!Adding
organisationNameto the payload aligns with the PR objective to store organisation name in parameters rather than session data.
128-137: LGTM!The fire-and-forget pattern for CSV attachment improves user experience by not blocking the response. Attachment errors are logged appropriately and won't disrupt the main flow.
183-201: LGTM!CSV structure correctly updated to include the plugin column per PR objectives. Error handling for the attachment operation is appropriate.
test/unit/checkAnswersController.test.js (3)
5-5: LGTM!Import correctly updated to include
getRequestDatafor test mocking.
52-52: LGTM!Error message assertions correctly updated to match the source implementation.
Also applies to: 62-62
69-126: LGTM!Tests correctly updated to validate the asynchronous attachment flow and non-blocking behaviour. The spy pattern appropriately verifies that attachment is triggered with the correct parameters, and the test on lines 170-171 correctly reflects that attachment failures don't block Jira request creation.
Also applies to: 150-172
3 participants
Description
This branch implements automatic plugin detection and storage for dataset submissions through the check tool, along with improvements to organisation name handling and error display.
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Go to Doncaster LPA, take one of the submitted URL's for one of the datasets, run this through the check tool, previously this would fail as it was a ArcGIS link, now it should work and show check results with no detectable difference.
Also use a URL that causes a error such as SSL, this should now be nicely displayed.
Finally on JIRA ticket creation, a plugin value should now be automatically created in the IMPORT.csv
NOTE** The JIRA ticket needs a quick live test.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
NOTE** The JIRA ticket creation with plugin needs a quick live test.
QA sign off
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation / Chores
✏️ Tip: You can customize this high-level summary in your review settings.