You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#11868 converts bucket_key/requested_bucket_key on signups, signup_requests, and signup_ranked_choices into real foreign keys, but explicitly leaves RegistrationPolicyBucket#key in place. Two places still depend on key as an identity mechanism rather than id:
RegistrationPolicy#bucket_with_key / sync_buckets_from_hash!, which match buckets between the persisted policy and an edited (detached, not-yet-saved) candidate policy by key, so that editing a bucket's other attributes doesn't lose its identity/row.
SignupBucketFinder and RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?, which have to compare real signups against candidate buckets during EventChangeRegistrationPolicyService's registration-policy-change simulation -- and a detached, not-yet-persisted candidate bucket has no id yet, so key is currently the only identity valid on both sides of that comparison.
The eventual goal (per #11868) is to stop depending on bucket keys entirely. Removing key requires replacing this identity mechanism.
Proposed Change
Correlate registration policy buckets by id instead of key when editing/simulating a registration policy change:
RegistrationPolicy.build_from_hash already receives id in the incoming hash (from the GraphQL mutation input) but currently discards it (see IGNORED_HASH_KEYS). Stop discarding it, and use it in sync_buckets_from_hash! to match existing buckets instead of key. An edited-but-existing bucket keeps its real, already-valid id unchanged by the edit, so this correlation works exactly like today's key-based one, just with id instead.
For brand-new buckets being added in the same edit (no id yet, since the row doesn't exist), no occupancy comparison is needed during simulation at all -- nothing can already occupy a bucket that doesn't exist yet. EventChangeRegistrationPolicyService's simulator only needs to defer resolving a placement into a new bucket to a real bucket_id until after RegistrationPolicy#update_from! persists it, which is the same "resolve after persist" pattern already used for the bucket-mapping remap logic in Convert bucket_key/requested_bucket_key to real foreign keys #11868.
Once this lands, SignupBucketFinder, RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?, RegistrationPolicy#bucket_with_key, and the bucket_key/requested_bucket_key computed reader methods added on Signup/SignupRequest/SignupRankedChoice in Convert bucket_key/requested_bucket_key to real foreign keys #11868 (kept specifically to support this key-based comparison) can all be converted to work by id, and RegistrationPolicyBucket#key itself can be considered for removal.
Simplifies SignupBucketFinder/RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket? back down to a single id-based comparison, rather than the key-based version they were kept at specifically to support simulation against detached policies.
Tradeoffs
This changes the correlation identifier the registration-policy-edit UI/GraphQL input has to round-trip from a human-readable string (key) to an opaque database id. That's a bigger contract change for the editing flow than the FK conversion in #11868, and should be scoped as its own piece of work rather than folded in.
Raised during implementation of Convert bucket_key/requested_bucket_key to real foreign keys #11868, while converting SignupBucketFinder/RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket? and finding they still need key-based comparison to support registration-policy-change simulation against a detached, not-yet-persisted policy.
Background
#11868 converts
bucket_key/requested_bucket_keyonsignups,signup_requests, andsignup_ranked_choicesinto real foreign keys, but explicitly leavesRegistrationPolicyBucket#keyin place. Two places still depend onkeyas an identity mechanism rather thanid:RegistrationPolicy#bucket_with_key/sync_buckets_from_hash!, which match buckets between the persisted policy and an edited (detached, not-yet-saved) candidate policy bykey, so that editing a bucket's other attributes doesn't lose its identity/row.SignupBucketFinderandRegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?, which have to compare real signups against candidate buckets duringEventChangeRegistrationPolicyService's registration-policy-change simulation -- and a detached, not-yet-persisted candidate bucket has noidyet, sokeyis currently the only identity valid on both sides of that comparison.The eventual goal (per #11868) is to stop depending on bucket keys entirely. Removing
keyrequires replacing this identity mechanism.Proposed Change
Correlate registration policy buckets by
idinstead ofkeywhen editing/simulating a registration policy change:RegistrationPolicy.build_from_hashalready receivesidin the incoming hash (from the GraphQL mutation input) but currently discards it (seeIGNORED_HASH_KEYS). Stop discarding it, and use it insync_buckets_from_hash!to match existing buckets instead ofkey. An edited-but-existing bucket keeps its real, already-valididunchanged by the edit, so this correlation works exactly like today's key-based one, just withidinstead.idyet, since the row doesn't exist), no occupancy comparison is needed during simulation at all -- nothing can already occupy a bucket that doesn't exist yet.EventChangeRegistrationPolicyService's simulator only needs to defer resolving a placement into a new bucket to a realbucket_iduntil afterRegistrationPolicy#update_from!persists it, which is the same "resolve after persist" pattern already used for the bucket-mapping remap logic in Convert bucket_key/requested_bucket_key to real foreign keys #11868.SignupBucketFinder,RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?,RegistrationPolicy#bucket_with_key, and thebucket_key/requested_bucket_keycomputed reader methods added onSignup/SignupRequest/SignupRankedChoicein Convert bucket_key/requested_bucket_key to real foreign keys #11868 (kept specifically to support this key-based comparison) can all be converted to work byid, andRegistrationPolicyBucket#keyitself can be considered for removal.Benefits
RegistrationPolicyBucket#key(the long-term goal of Convert bucket_key/requested_bucket_key to real foreign keys #11868).SignupBucketFinder/RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?back down to a single id-based comparison, rather than the key-based version they were kept at specifically to support simulation against detached policies.Tradeoffs
This changes the correlation identifier the registration-policy-edit UI/GraphQL input has to round-trip from a human-readable string (
key) to an opaque database id. That's a bigger contract change for the editing flow than the FK conversion in #11868, and should be scoped as its own piece of work rather than folded in.References
SignupBucketFinder/RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket?and finding they still need key-based comparison to support registration-policy-change simulation against a detached, not-yet-persisted policy.Issue drafted by Claude