Fix pagination stats for Guestbook Responses - #12557
Conversation
This comment has been minimized.
This comment has been minimized.
Test Results404 tests 389 ✅ 33m 22s ⏱️ Results for commit ac9c11b. ♻️ This comment has been updated with latest results. |
14343b0 to
ac9c11b
Compare
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
rtreacy
left a comment
There was a problem hiding this comment.
Claude
Overview
Fixes a bug (#12523) where GET /api/guestbooks/{id}/responses returned pagination totals (totalResponses, usage count) that undercounted responses/usages from datasets in child dataverses when a guestbook defined on a parent dataverse is applied to a dataset further down the hierarchy. Core fix is a 2-line change in Guestbooks.java, plus a new integration test and a small test-utility fix to avoid guestbook-name collisions.
Root cause analysis (traced through the code)
- Guestbooks.getResponses() sets dataverse = guestbook.getDataverse() — always the guestbook's owning dataverse, never a descendant.
- The old code called findCountUsages(guestbookId, dataverse.getId()) / findCountByGuestbookId(guestbookId, dataverse.getId()), whose non-null-dataverseId branches filter with obj.owner_id = dataverseId — i.e., only datasets directly owned by the guestbook's home dataverse. Datasets in child/grandchild dataverses using the same (inherited) guestbook were silently excluded from the totals.
- Meanwhile, the actual list of returned responses (findAllByGuestbookId) was already filtered by guestbook.id alone, with no dataverse scoping — so the list already included the full hierarchy while the pagination totals didn't. That's exactly the reported symptom (list/total mismatch).
- Passing null for dataverseId routes both count methods into their already-existing "guestbookId only, no dataverse restriction" branches, bringing the totals back in line with the list query. This is a minimal, correct, surgical fix.
- Confirmed this change is scoped only to this one call site (Guestbooks.java:190-193); other callers of the same two methods (ManageGuestbooksPage.java, GuestbookResponsesPage.java, Guestbooks.java:93-94) are untouched and keep their existing dataverse-scoped semantics, so no unintended behavior change elsewhere.
Test coverage
- New FilesIT.testDownloadFileWithParentGuestbookResponse correctly reproduces the bug: creates a parent dataverse + child dataverse, creates a dataset in the child, creates a guestbook in the parent and explicitly assigns it to the child's dataset (updateDatasetGuestbook), then asserts responseListSize == totalCountFromJson after a download. This is exactly the right regression test for the fix.
- Minor: the test creates a second "user with no permission" (apiToken/username at line ~64-68) that's never referenced afterward — looks like leftover copy-paste from the neighboring testDownloadFileWithGuestbookResponse test. Harmless but dead code; worth trimming.
- The guestbook-test.json name placeholder ({@} → UUID via createRandomGuestbook) is a reasonable supporting fix, presumably needed to avoid a guestbook-name collision now that tests create more guestbooks per run. Checked the one other direct consumer of that JSON file's literal name (GuestbooksIT.testGuestbook's string-replace of "my test guestbook" → "my modified test guestbook") — still works correctly since it's a substring replace, not exact-match, so no regression there.
Other notes
- No API/security concerns: the endpoint's permission check (EditDataverse on the guestbook's owning dataverse) is unchanged; removing the dataverse scoping only affects which rows are counted, not who can call the endpoint. Since a guestbook can only be assigned to datasets in its own dataverse or descendants by design, an unscoped count is exactly the right scope for "this guestbook, wherever it's used."
- Release note included and accurately describes the fix.
Summary
This is a well-targeted, minimal bug fix with a solid regression test. Only nit is the unused second user in the new test — not blocking.
What this PR does / why we need it: Pagination totals reported in API /api/guestbooks/{id}/responses were not including all stats from the full hierarchy of parent collections
Which issue(s) this PR closes:#12523
Special notes for your reviewer:
Suggestions on how to test this: See Issue. Also see FilesIT testDownloadFileWithParentGuestbookResponse
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: included
Additional documentation: