feat: Implement endpoint duplication check in CheckConfirmationContro… - #1215
Merged
gibahjoe merged 9 commits intoJul 1, 2026
Merged
Conversation
…ller - Update CheckConfirmationController to check if an endpoint is already being collected for a dataset. - Modify confirmation.html to display appropriate messages based on endpoint collection status. - Add unit tests for CheckConfirmationController and endpointAlreadyCollectedForDataset functionality. - Introduce endpointAlreadyCollectedForDataset utility to query the database for existing endpoints.
6 tasks
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds duplicate-endpoint handling on the confirmation flow and threads unique dataset fields through the status API, controller, view, and polling tests. ChangesDuplicate Endpoint Detection Flow
Status Polling and Field Querying
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant CheckConfirmationController
participant asyncRequestApi
participant endpointAlreadyCollectedForDataset
participant datasette
Browser->>CheckConfirmationController: request confirmation page
CheckConfirmationController->>asyncRequestApi: getRequestData(requestId)
asyncRequestApi-->>CheckConfirmationController: request params
CheckConfirmationController->>endpointAlreadyCollectedForDataset: endpointUrl, dataset, organisation
endpointAlreadyCollectedForDataset->>datasette: runQuery(SQL)
datasette-->>endpointAlreadyCollectedForDataset: formattedData
endpointAlreadyCollectedForDataset-->>CheckConfirmationController: alreadyCollectingEndpoint
CheckConfirmationController->>CheckConfirmationController: set or delete req.session.checkRequestId
CheckConfirmationController-->>Browser: render confirmation page
sequenceDiagram
participant Client
participant getStatus
participant shouldShowColumnMapping
participant StatusController
participant StatusPage
Client->>getStatus: GET /status/:result_id?field=...
getStatus->>shouldShowColumnMapping: request data + uniqueDatasetFields
shouldShowColumnMapping-->>getStatus: showColumnMapping decision
getStatus-->>Client: response body with columnMappingUrl
Client->>StatusController: build pollingEndpoint
StatusController-->>Client: /api/status/:id?field=...
Client->>StatusPage: poll at configured interval
StatusPage-->>Client: update polling state
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/unit/utils/endpointAlreadyCollected.test.js (1)
16-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cross-organisation regression case.
The new behaviour is only meant to short-circuit within the same provision. A negative test for “same URL + same dataset + different organisation” would protect that contract and catch the current over-broad lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/utils/endpointAlreadyCollected.test.js` around lines 16 - 53, Add a regression test in endpointAlreadyCollectedForDataset to cover the same endpointUrl and dataset belonging to a different organisation/provision, and assert it returns false. The current tests in endpointAlreadyCollected.test.js only cover matching, non-matching, escaping, and missing input; extend the Datasette mock setup so the lookup stays scoped to the same provision and does not short-circuit across organisations. Use the existing endpointAlreadyCollectedForDataset helper and datasette.runQuery mock in the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/datasetteQueries/endpointAlreadyCollected.js`:
- Around line 7-20: The duplicate-check query in
endpointAlreadyCollectedForDataset is too broad because it only matches on
endpointUrl and dataset. Update this helper to accept the organisation/provision
key as an additional input, pass it through the call sites, and add it to the
WHERE clause in the SQL so the lookup is scoped to the same
organisation/provision as the submission.
In `@src/views/check/confirmation.html`:
- Around line 17-23: The fallback handling for datasetOverviewHref in the
confirmation template is mismatched with the link copy: when there is no
deepLink or orgId, it currently points to “/” while still using the overview
wording. Update the conditional around datasetOverviewHref and the related link
label so the Home copy is used in that branch, or only keep the overview label
when you can construct a real overview URL from options.deepLink, options.orgId,
and options.dataset.
---
Nitpick comments:
In `@test/unit/utils/endpointAlreadyCollected.test.js`:
- Around line 16-53: Add a regression test in endpointAlreadyCollectedForDataset
to cover the same endpointUrl and dataset belonging to a different
organisation/provision, and assert it returns false. The current tests in
endpointAlreadyCollected.test.js only cover matching, non-matching, escaping,
and missing input; extend the Datasette mock setup so the lookup stays scoped to
the same provision and does not short-circuit across organisations. Use the
existing endpointAlreadyCollectedForDataset helper and datasette.runQuery mock
in the new case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b76550b6-315b-42c6-9a1b-2c19cd2b428c
📒 Files selected for processing (6)
src/controllers/checkConfirmationController.jssrc/utils/datasetteQueries/endpointAlreadyCollected.jssrc/views/check/confirmation.htmltest/unit/check/confirmationPage.test.jstest/unit/checkConfirmationController.test.jstest/unit/utils/endpointAlreadyCollected.test.js
…pdate confirmation view links
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/utils/endpointAlreadyCollected.test.js (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title claims "endpoint URL or dataset is missing" but only exercises the missing-
endpointUrlcase.The
datasetshort-circuit branch isn't separately verified here.♻️ Suggested addition
it('does not query Datasette when endpoint URL or dataset is missing', async () => { await expect(endpointAlreadyCollectedForDataset({ endpointUrl: '', dataset: 'brownfield-land' })).resolves.toBe(false) expect(datasette.runQuery).not.toHaveBeenCalled() }) + + it('does not query Datasette when dataset is missing', async () => { + await expect(endpointAlreadyCollectedForDataset({ + endpointUrl: 'https://example.com/data.csv', + dataset: '' + })).resolves.toBe(false) + + expect(datasette.runQuery).not.toHaveBeenCalled() + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/utils/endpointAlreadyCollected.test.js` around lines 76 - 83, The test name in endpointAlreadyCollectedForDataset is broader than the behavior it covers, since it only checks the missing endpointUrl path. Update the test suite in endpointAlreadyCollected.test.js to also exercise the missing dataset short-circuit branch, ideally as a separate assertion or dedicated test alongside the existing endpointUrl case, and keep the expectation that datasette.runQuery is not called.test/unit/check/confirmationPage.test.js (1)
93-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNegative assertion may be trivially true regardless of correctness.
The check
expect(doc.body.textContent).not.toContain('Return to brownfield-land overview')uses the raw dataset slug, but the template appliesoptions.dataset | datasetSlugToReadableNamebefore rendering the link text. If that filter reformats the slug (e.g. to "Brownfield land"), the literal string being asserted absent would never appear anyway — even if the "already collecting" overview link were incorrectly rendered instead of the home-link fallback. This makes the negative assertion a weak (potentially tautological) guard.Consider asserting on the anchor's
href(e.g. noa[href^="/organisations/"]) or on the filtered text, to make sure the test actually catches a regression where the overview link renders unexpectedly.Suggested strengthening
expect(homeLink).not.toBeNull() expect(homeLink.textContent.trim()).toBe('Return to Home') - expect(doc.body.textContent).not.toContain('Return to brownfield-land overview') + expect(doc.querySelector('a[href^="/organisations/"]')).toBeNull()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/check/confirmationPage.test.js` around lines 93 - 109, The test in confirmationPage.test.js uses a weak negative assertion against the raw dataset slug, which can pass even if the overview link renders incorrectly. Update the assertion in the confirmation page test to verify the rendered anchor target or the transformed link text from check/confirmation.html, using the existing JSDOM query pattern and the alreadyCollectingEndpoint/options.dataset setup, so the test fails if an organisations overview link appears instead of the home fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/check/confirmationPage.test.js`:
- Around line 93-109: The test in confirmationPage.test.js uses a weak negative
assertion against the raw dataset slug, which can pass even if the overview link
renders incorrectly. Update the assertion in the confirmation page test to
verify the rendered anchor target or the transformed link text from
check/confirmation.html, using the existing JSDOM query pattern and the
alreadyCollectingEndpoint/options.dataset setup, so the test fails if an
organisations overview link appears instead of the home fallback.
In `@test/unit/utils/endpointAlreadyCollected.test.js`:
- Around line 76-83: The test name in endpointAlreadyCollectedForDataset is
broader than the behavior it covers, since it only checks the missing
endpointUrl path. Update the test suite in endpointAlreadyCollected.test.js to
also exercise the missing dataset short-circuit branch, ideally as a separate
assertion or dedicated test alongside the existing endpointUrl case, and keep
the expectation that datasette.runQuery is not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6a4f2c6-7051-4eb0-8b1f-74b9667a5623
📒 Files selected for processing (3)
src/views/check/confirmation.htmltest/unit/check/confirmationPage.test.jstest/unit/utils/endpointAlreadyCollected.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/check/confirmation.html
…on and update confirmation view link expectations
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/check/confirmationPage.test.js (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated heading-assertion structure across the two describe blocks.
Both tests assert the same
h1/h2/h3pattern with only the expected text differing. A small shared helper (e.g.expectHeadingStructure(doc, { h1, h3s })) would reduce duplication, though the current form is still clear and easy to follow.Also applies to: 62-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/check/confirmationPage.test.js` around lines 35 - 43, The confirmation page heading assertions are duplicated across the two describe blocks, so extract the shared DOM checks into a small helper used by both tests. Update the test file around the existing heading assertions in the confirmationPage suite, and centralize the repeated `main h1`/`main h2`/`main h3` expectations in a helper like `expectHeadingStructure` that accepts the expected text values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/check/confirmationPage.test.js`:
- Around line 35-43: The confirmation page heading assertions are duplicated
across the two describe blocks, so extract the shared DOM checks into a small
helper used by both tests. Update the test file around the existing heading
assertions in the confirmationPage suite, and centralize the repeated `main
h1`/`main h2`/`main h3` expectations in a helper like `expectHeadingStructure`
that accepts the expected text values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad710878-8c99-4d96-bdfb-7e7f2fbee194
📒 Files selected for processing (4)
src/utils/datasetteQueries/endpointAlreadyCollected.jssrc/views/check/confirmation.htmltest/unit/check/confirmationPage.test.jstest/unit/utils/endpointAlreadyCollected.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/utils/endpointAlreadyCollected.test.js
- src/utils/datasetteQueries/endpointAlreadyCollected.js
- src/views/check/confirmation.html
pooleycodes
self-requested a review
July 1, 2026 12:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a new
/check/confirmationvariant for endpoint URLs that are already being collected for the checked dataset. The page now shows “Data checked” content and returns users to the dataset overview instead of asking them to submit the endpoint URL again.What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Test by checking a URL that is already collected for the same dataset. After continuing from the check results page,
/check/confirmationshould show the “Data checked” panel and explain that the endpoint URL does not need to be submitted again.Also test a URL that is not already collected. The existing “Provide your data” confirmation page and submit button should still appear.
Before
The confirmation page always showed “Provide your data” for URL checks, even when the endpoint URL was already being collected.
After
The confirmation page shows “Data checked” and explains that updates will be collected automatically when the endpoint URL is already collected for the dataset.
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?
None.
[optional] Are there any dependencies on other PRs or Work?
None.
Summary by CodeRabbit