Skip to content

Fix pagination stats for Guestbook Responses - #12557

Open
stevenwinship wants to merge 1 commit into
developfrom
12523-guestbook-responses-page-stats-fix
Open

Fix pagination stats for Guestbook Responses#12557
stevenwinship wants to merge 1 commit into
developfrom
12523-guestbook-responses-page-stats-fix

Conversation

@stevenwinship

@stevenwinship stevenwinship commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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:

@stevenwinship stevenwinship self-assigned this Jul 24, 2026
@github-actions github-actions Bot added Feature: Guestbook FY27 Sprint 1 FY27 Sprint 1 (2026-07-01 - 2026-07-15) FY27 Sprint 2 FY27 Sprint 2 (2026-07-15 - 2026-07-29) GREI Re-arch Issues related to the GREI Dataverse rearchitecture Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) SPA These changes are required for the Dataverse SPA Type: Bug a defect labels Jul 24, 2026
@stevenwinship stevenwinship moved this to In Progress 💻 in IQSS Dataverse Project Jul 24, 2026
@stevenwinship stevenwinship added this to the 6.12 milestone Jul 24, 2026
@coveralls

coveralls commented Jul 24, 2026

Copy link
Copy Markdown

Coverage Status

Coverage is 24.97%12523-guestbook-responses-page-stats-fix into develop. No base build found for develop.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Test Results

404 tests   389 ✅  33m 22s ⏱️
 55 suites   15 💤
 55 files      0 ❌

Results for commit ac9c11b.

♻️ This comment has been updated with latest results.

@stevenwinship stevenwinship moved this from In Progress 💻 to Ready for Review ⏩ in IQSS Dataverse Project Jul 27, 2026
@stevenwinship stevenwinship removed their assignment Jul 27, 2026
@stevenwinship
stevenwinship force-pushed the 12523-guestbook-responses-page-stats-fix branch from 14343b0 to ac9c11b Compare July 27, 2026 12:54
@github-actions

Copy link
Copy Markdown

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:12523-guestbook-responses-page-stats-fix
ghcr.io/gdcc/configbaker:12523-guestbook-responses-page-stats-fix

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@rtreacy rtreacy self-assigned this Jul 28, 2026
@rtreacy rtreacy moved this from Ready for Review ⏩ to In Review 🔎 in IQSS Dataverse Project Jul 28, 2026

@rtreacy rtreacy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature: Guestbook FY27 Sprint 1 FY27 Sprint 1 (2026-07-01 - 2026-07-15) FY27 Sprint 2 FY27 Sprint 2 (2026-07-15 - 2026-07-29) GREI Re-arch Issues related to the GREI Dataverse rearchitecture Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) SPA These changes are required for the Dataverse SPA Type: Bug a defect

Projects

Status: In Review 🔎

Development

Successfully merging this pull request may close these issues.

/api/guestbooks/{id}/responses returns responses but pagination.totalResponses is 0

3 participants