Convert bucket_key/requested_bucket_key to real foreign keys - #11871
Conversation
Adds bucket_id/requested_bucket_id FK columns (backfilled from the old string columns, then drops them) to signups, signup_requests, and signup_ranked_choices, referencing registration_policy_buckets.id directly instead of matching by key. signup_changes (an audit table) gets FK columns that nullify on bucket delete, plus bucket_name/ requested_bucket_name snapshot columns so historical audit rows keep showing a bucket's name even after it's deleted. All application code (models, services, GraphQL, presenters, Liquid drops, notifiers) is converted to use bucket_id as the source of truth. SignupBucketFinder and RegistrationPolicyBucket#signup_ definitely_occupies_slot_in_bucket? deliberately stay key-based internally, since they're also used to simulate a registration policy change against a detached, not-yet-persisted candidate policy whose buckets have no id yet. EventChangeRegistrationPolicyService (the #11229 remap-on-policy- change logic) is reordered: FK references to a bucket about to be destroyed are nulled out before the destroy (avoiding a foreign key violation) and re-resolved to their final value once the surviving or newly-created bucket is persisted. GraphQL mutations/types keep bucket_key/requested_bucket_key functional but deprecated, and add bucket/requestedBucket object fields (and bucket_id/to_bucket_id/from_bucket_id args) so existing clients don't break. Added descriptions to the pre-existing GraphQL fields/arguments this touched that didn't already have one. Extended .rubocop_todo.yml for pre-existing style debt (missing super calls, parameter list length, Time.zone usage, etc.) in files this migration happens to touch but doesn't otherwise need to change. Note: a small number of historical requested_bucket_key values (a fraction of a percent) referenced buckets already removed from their event's current policy before this migration and could not be backfilled to a real id -- this data was already unusable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review: PR #11871 — Convert
|
Addresses findings from a review of #11871: a policy-change simulation bug where clearing a requested bucket on a removed bucket leaked the old key back in, a bucket_key/bucket_id mismatch between the admin signups filter and its presenter, two mutations that could 500 on a nil/invalid bucket instead of raising a clean GraphQL error, a stale bucket key in the notifier preview factory, and several N+1s introduced by bucket_key/requested_bucket_key becoming association reads with nothing preloading the association. Also splits the bucket_id backfill migration from the legacy column drop so they can ship in separate releases (drop filed as #11872), changes the admin signups table's Bucket column to sort by case-insensitive bucket name instead of bucket row id, and fixes pre-existing rubocop/eslint debt (a shadowed Lint/MissingSuper entry in .rubocop_todo.yml, missing `id` selections on several buckets{} queries) uncovered while touching these files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes for review findingsAddressed all four correctness bugs and the N+1s from the review, plus the deploy-risk and smaller-notes items, in Correctness
N+1s
Added query-count regression tests ( Deploy riskSplit the single migration so this release only adds/backfills Smaller notesDocumented the withdrawn-signup preference-clearing behavior change in Incidental cleanupFixing the bucket filter surfaced a shadowed All 353 relevant Ruby tests pass, Comment written by Claude |
Manual QA planThis PR converts Suggested test convention: an event with a limited counted bucket (e.g. "Dogs"), an unlimited/flex bucket (e.g. "Anything"), and a run with a handful of signups split across them plus a couple of waitlisted signups. 1. Registration policy editing — bucket removal + remapping (highest risk)This is the scenario behind the policy-change simulation bug: clearing a signup's bucket preference on a removed bucket, when the new policy has no flex bucket to fall back to.
2. Admin Signups table — Bucket filter and sortRoute: Event → Run → Signups tab.
3. Force-confirm and change-bucket (Edit Signup page)Route: Signups table → click a row → Edit signup.
4. Self-service signup (attendee-facing)
5. Ranked-choice / lottery conventions (if applicable to your test convention)
6. Signup change historyRoute: Signups tab → Change history tab.
7. Notification previewRoute: Notifications admin (
8. Freeze bucket assignmentsRoute: Signups tab → Freeze bucket assignments button.
9. GraphQL API / backward compatibility
10. General regression pass
Notes for whoever picks this up
QA plan written by Claude |
BucketKeyRemappingModal is mounted once, up front, with removedBuckets: [] before an admin ever clicks Save; removedBuckets only becomes non-empty later, as a prop update on the already-mounted component. Its mappings state was seeded via a useState lazy initializer, which only ever saw that initial empty array, so a removed bucket's default "No preference" selection (already shown as selected) never got an entry in state unless the admin explicitly touched its dropdown. Since handleConfirm builds bucketKeyMappings from Object.entries(mappings) rather than from removedBuckets, accepting the default sent an empty mapping list, and EventChangeRegistrationPolicyService correctly rejected the change with "no mapping was provided" -- caught while manually QAing the registration-policy-editing flow from PR #11871's QA plan. Pre-existing bug (predates #11868/#11871), not something the bucket FK conversion introduced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bug found in QA Section 1, fixed in
|
The remap modal offered "No preference" as a destination for a removed bucket even when the new registration policy has prevent_no_preference_signups set, which would leave affected signups/requests/ranked choices with a null requested_bucket_id in a state new signups aren't allowed to be created in. Caught via manual QA on #11871. Frontend: BucketKeyRemappingModal hides the "No preference" option and disables Apply until every removed bucket has an explicit destination when the new policy disallows it. Backend: EventChangeRegistrationPolicyService now rejects a to_key: nil mapping outright when the new policy's prevent_no_preference_signups is set, as defense in depth for any caller that bypasses the modal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second bug found in QA Section 1, fixed in
|
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
…#signups Sentry showed a new registration_policy_buckets-by-id N+1 (INTERCODE-186/187/189) after #11880 shipped -- it had been masked by the much larger registration_policy/buckets N+1 that #11880 fixed, and became the dominant N+1 once that one was gone. Traced it to the "My Schedule" widget (the user_signups/signup_bucket_description CMS partials, present by default on most conventions' sites), which calls signup.bucket.name / signup.requested_bucket.name per signup. UserConProfileDrop#signups already preloads several associations but never picked up :bucket/:requested_bucket -- these only became real belongs_to associations in #11871 (they used to be plain string columns needing no extra query at all), so this gap predates #11880 but was previously hidden under the louder N+1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Converts
bucket_key/requested_bucket_keystring columns onsignups,signup_requests, andsignup_ranked_choicesinto realbucket_id/requested_bucket_idforeign keys referencingregistration_policy_buckets.id, so bucket references are enforced at the database level instead of matched by a free-text string.bucket_id/requested_bucket_idFK columns, backfills them from the old string columns (matching by key, scoped to the correct registration policy), then drops the old columns entirely.signup_changes(an audit-log table) gets FK columns that nullify on bucket delete (rather than the restrictive default), plus newbucket_name/requested_bucket_namesnapshot columns, so historical audit rows keep showing a bucket's name even after that bucket is later deleted.bucket_idas the source of truth.SignupBucketFinderandRegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?deliberately stay key-based internally — they're shared withEventChangeRegistrationPolicyService's simulation of a registration policy change against a detached, not-yet-persisted candidate policy, whose buckets have noidyet. Key is the only identity valid on both a real signup and a hypothetical candidate bucket.EventChangeRegistrationPolicyService(the Changes to bucket keys can invalidate signups, signup requests, and queue items #11229 remap-on-policy-change logic) is reordered: any FK reference to a bucket about to be destroyed is nulled out before the destroy (the FKs onsignups/signup_requests/signup_ranked_choicesdon't nullify on delete, so leaving a reference in place would raise a foreign key violation), then re-resolved to its final value once the surviving or newly-created bucket is persisted with a real id.bucket_key/requested_bucket_keyfunctional but deprecated, and addbucket/requestedBucketobject fields (plusbucketId/toBucketId/fromBucketIdinput args) so existing clients keep working unchanged. Added descriptions to the pre-existing GraphQL fields/arguments this PR touched that were missing one..rubocop_todo.ymlfor pre-existing style debt (missingsupercalls, long parameter lists,Time.zoneusage, etc.) in files this migration happens to touch but doesn't otherwise need to change.Known limitation
A small number of historical
requested_bucket_keyvalues (well under 1%) referenced buckets already removed from their event's current registration policy before this migration ran, and couldn't be backfilled to a real id. That preference data was already unusable (the bucket it pointed at doesn't exist anymore), so this is a no-op in practice.Follow-up
Filed #11870 to track converting the remaining internal key-based identity (used for registration-policy-edit correlation and simulation) to id-based once bucket
keyitself is no longer needed anywhere — out of scope here since it changes the correlation identifier the registration-policy-edit UI has to round-trip.Test plan
yarn run tsc --noEmitpassesbin/rails graphql:update)Fixes #11868
🤖 Generated with Claude Code