Skip to content

Give the registration policy editor real bucket ids - #11900

Merged
nbudin merged 4 commits into
mainfrom
11895-registration-policy-editor-bucket-ids
Aug 8, 2026
Merged

Give the registration policy editor real bucket ids#11900
nbudin merged 4 commits into
mainfrom
11895-registration-policy-editor-bucket-ids

Conversation

@nbudin

@nbudin nbudin commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #11895

Summary

  • RegistrationPolicyBucket#as_json now includes "id", so the registration policy editor can round-trip a bucket's real id through form_response_attrs_json. EditingRegistrationBucket/BucketForRegistrationPolicyUtils carry an optional id (absent for a bucket added in the current edit session) so RegistrationPolicy#sync_buckets_from_hash! can correlate edited buckets by id instead of falling back to key.
  • Converted the handful of RegistrationPolicyBucket#key business-logic usages that were directly convertible to id with no backend/schema changes:
    • React list keys in EventCapacityDisplay.tsx/RunCapacityGraph.tsx
    • SignupOption's identity in buildSignupOptions.ts
    • a bucket-to-bucket comparison in SignupAdmin/$id/route.tsx
    • the CreateMySignup/CreateSignupRequest/CreateSignupRankedChoice/CreateUserSignup mutations, switching from the deprecated requested_bucket_key argument to requested_bucket_id
    • a few dead bucket.name ?? bucket.key display fallbacks (name is NOT NULL, so the fallback branch was unreachable)
  • Ran bin/rails graphql:update to regenerate the frontend generated files after the .graphql query/mutation edits (no backend schema changes — verified schema.graphql/schema.json are unchanged).

Scope note

Filed #11899 separately for a much larger, related sweep: nearly every Signup/SignupRequest/SignupRankedChoice/SignupChange/GroupedSignupCount consumer across signup-admin, signup-moderation, and the event page still reads the deprecated bucket_key/requested_bucket_key scalar fields instead of the already-available bucket/requestedBucket relations. That's a ~20-file migration in its own right and deserves its own review rather than being folded into this PR.

Test plan

  • bin/rails test test/models/registration_policy_bucket_test.rb test/models/registration_policy_test.rb
  • yarn run tsc --noEmit
  • yarn vitest run test/javascript (187 passed, 1 skipped)
  • yarn eslint on all changed files (0 errors)

🤖 Generated with Claude Code

RegistrationPolicyBucket#as_json now includes id, and the editor's
EditingRegistrationBucket/BucketForRegistrationPolicyUtils types carry
it through, so RegistrationPolicy#sync_buckets_from_hash! can correlate
edited buckets by id instead of falling back to key.

Also converts the handful of RegistrationPolicyBucket#key usages that
were directly convertible to id with no backend/schema changes: React
list keys, the SignupOption identity, one bucket-to-bucket comparison,
the CreateMySignup/CreateSignupRequest/CreateSignupRankedChoice/
CreateUserSignup mutations (switching off the deprecated
requested_bucket_key argument), and a few dead `name ?? key` display
fallbacks.

The much larger sweep of business logic that reads bucket_key/
requested_bucket_key off Signup/SignupRequest/SignupRankedChoice/
SignupChange/GroupedSignupCount is filed separately as #11899, since
it touches nearly every signup-admin/signup-moderation/event-page
screen and deserves its own review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nbudin nbudin added minor Bumps the minor version number on release refactor labels Aug 7, 2026
The name column is text NOT NULL at the database level, with no
custom resolver or presence validation that could produce nil, so the
existing null: true was stricter than reality. Tightening the schema
here removes the now-unnecessary bucket.name ?? '' fallback this PR
introduced in RunSignupsTable.tsx.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nbudin

nbudin commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up commit in response to a review question about bucket.name ?? '' in RunSignupsTable.tsx: registration_policy_buckets.name is text NOT NULL at the DB level with no custom resolver/validation that could produce nil, so the GraphQL field's null: true was stricter than reality. Tightened it to null: false (a non-breaking direction for an output field) and removed the now-unnecessary fallback.

Comment written by Claude

nbudin and others added 2 commits August 7, 2026 16:40
Now that RegistrationPolicyBucket#name is non-null in the schema, these
?? fallbacks (in buildSignupOptions.ts, RegistrationBucketRow.tsx,
RegistrationPolicyUtils.ts, and RegistrationPolicyPreview.tsx) were
dead code. Left the ones elsewhere that guard against a bucket lookup
itself returning undefined (SignupUtils.ts, UserSignupQueueItem.tsx,
RegistrationPolicyItemChangeDisplay.tsx) -- those are real, unrelated
to name's old nullability.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Since RegistrationPolicyBucket#as_json now includes id,
RegistrationPolicy.build_from_hash(source.as_json) -- used by
create_event_proposal.rb and accept_event_proposal_service.rb to
clone a policy into an unrelated record -- started carrying the
source's real bucket ids into the clone. Persisting that clone then
collided with the still-existing source rows on the
registration_policy_buckets primary key.

Added build_from_hash_as_clone, which strips bucket ids before
delegating to build_from_hash, and switched both cloning call sites
to it. build_from_hash itself keeps its original signature/behavior
(no id stripping) since sync_buckets_from_hash! still needs it for
the edit-in-place flow this PR is about; a keyword argument would
have been simpler but breaks build_from_hash's many bare-hash-literal
callers (Ruby can't parse a bare hash as one positional argument once
the method also declares a keyword parameter).

Also fixed two FormResponseChange tests whose expected new_value was
built from a policy that's never itself persisted (so its buckets
have no id), compared against the real persisted state (which now
has real ids) -- switched to equivalent_to? (content, not identity)
comparisons. Added a regression test for the accept-proposal cloning
path mirroring the existing create-proposal one.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nbudin

nbudin commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the Minitest CI failure. Root cause: since RegistrationPolicyBucket#as_json now includes id, RegistrationPolicy.build_from_hash(source_policy.as_json) -- used by create_event_proposal.rb and accept_event_proposal_service.rb to clone a policy into an unrelated new record -- started carrying the source's real bucket ids into the clone, since build_from_hash intentionally preserves id for the (unrelated) edit-in-place sync flow. Persisting the clone then collided with the still-existing source rows on registration_policy_buckets's primary key (PG::UniqueViolation).

Fix: added RegistrationPolicy.build_from_hash_as_clone, which strips bucket ids before delegating to build_from_hash, and switched both cloning call sites to it. build_from_hash itself is untouched -- a keyword argument would've been simpler, but breaks its many bare-hash-literal callers (method("a" => 1, "b" => 2) can't be parsed as one positional Hash once the method also declares a keyword parameter).

Also fixed two now-fragile FormResponseChange "records the new policy as new_value" tests that compared the real persisted state (now with real ids) against a let-block policy that's never itself persisted (so it never had ids) -- switched those to equivalent_to? (content, not identity). Added a regression test for the accept-proposal cloning path mirroring the existing create-proposal one, and confirmed it reproduces the exact CI failure when the fix is reverted.

Full suite (1169 tests) and CI's Minitest job should now be green.

Comment written by Claude

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
app/graphql/mutations/create_my_signup.rb 🟢 96% 🟢 84% 🔴 -12%
app/javascript/setupI18Next.ts 🟠 72.41% 🟠 65.52% 🔴 -6.89%
app/models/registration_policy.rb 🟢 98.65% 🟢 98.73% 🟢 0.08%
app/services/accept_event_proposal_service.rb 🟢 97.87% 🟢 100% 🟢 2.13%
Overall Coverage 🟢 56.04% 🟢 56.04% ⚪ 0%

Minimum allowed coverage is 0%, this run produced 56.04%

@nbudin
nbudin merged commit d0499e4 into main Aug 8, 2026
25 checks passed
@nbudin
nbudin deleted the 11895-registration-policy-editor-bucket-ids branch August 8, 2026 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Bumps the minor version number on release refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key)

1 participant