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
#11895 and #11896 remove every external (frontend, GraphQL, Liquid) dependency on RegistrationPolicyBucket#key as identity. This issue is the remaining internal app-layer cleanup needed before the column itself can be dropped (#11898): the handful of places that still fall back to key-based matching specifically because the frontend didn't supply bucket ids -- which, after #11895, it now reliably does.
RegistrationPolicy#sync_buckets_from_hash!/#match_existing_bucket (app/models/registration_policy.rb): still falls back to existing_by_key when an incoming bucket hash has no id. Once every caller that edits an existing registration policy reliably supplies id for any bucket that already exists, this fallback is no longer reachable in practice and can be removed -- but it needs to be deleted, not just left dead, so a hash with no id is unambiguously "this is a brand new bucket" rather than "maybe this secretly matches something by key."
RegistrationPolicy#equivalent_to?: currently matches self's buckets against other's by key (other.buckets.find { |b| b.key == bucket.key } -- added in Convert remaining registration policy bucket key dependencies (SignupBucketFinder, EventChangeRegistrationPolicyService) to id #11892, with a comment explaining it's structurally needed for now). Once other (typically a detached policy from build_from_hash) reliably carries ids for anything pre-existing, this should match by id instead, with a bucket that has no id always counting as "different" (correct: a brand-new bucket, by definition, changes the policy).
EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_for (app/services/event_change_registration_policy_service.rb): the one remaining commented key-based bridge from Convert remaining registration policy bucket key dependencies (SignupBucketFinder, EventChangeRegistrationPolicyService) to id #11892, translating a real signup's persisted bucket into its detached candidate counterpart during simulation. Same fix: id-based lookup for anything that already existed pre-edit; a genuinely brand-new bucket in the same edit has no id at this point and so has no "current"/"requested" candidate to bridge to at all (correct -- a signup can't already be sitting in, or have already requested, a bucket that didn't exist yet).
Before removing the sync_buckets_from_hash! fallback, audit the other callers of RegistrationPolicy.build_from_hash/#update_from! beyond the updateEvent mutation path that #11895 fixes, to confirm each one either (a) never has pre-existing buckets to match against (a brand-new policy/record, so the fallback was never load-bearing there), or (b) needs its own id-plumbing fix first:
app/graphql/mutations/create_event.rb, create_filler_event.rb, create_event_proposal.rb -- likely (a), since these build a policy for a brand-new Event/EventProposal with no prior buckets to correlate against.
app/services/import_convention_data_service.rb -- confirmed (a): always creates a brand-new Convention, never matches against pre-existing buckets, only cross-references bucket_key within its own import payload. Unaffected by this issue.
Proposed Change
Remove RegistrationPolicy#sync_buckets_from_hash!'s existing_by_key fallback (after confirming the caller audit above).
Switch RegistrationPolicy#equivalent_to? to id-based matching.
Switch EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_for to an id-based lookup.
Update or remove the comments in RegistrationPolicy#build_from_hash/#sync_buckets_from_hash!/RegistrationPolicyBucket#occupies_bucket_as_signup? that describe the key-based fallback, since it will no longer exist.
Background
#11895 and #11896 remove every external (frontend, GraphQL, Liquid) dependency on
RegistrationPolicyBucket#keyas identity. This issue is the remaining internal app-layer cleanup needed before the column itself can be dropped (#11898): the handful of places that still fall back to key-based matching specifically because the frontend didn't supply bucket ids -- which, after #11895, it now reliably does.RegistrationPolicy#sync_buckets_from_hash!/#match_existing_bucket(app/models/registration_policy.rb): still falls back toexisting_by_keywhen an incoming bucket hash has noid. Once every caller that edits an existing registration policy reliably suppliesidfor any bucket that already exists, this fallback is no longer reachable in practice and can be removed -- but it needs to be deleted, not just left dead, so a hash with no id is unambiguously "this is a brand new bucket" rather than "maybe this secretly matches something by key."RegistrationPolicy#equivalent_to?: currently matchesself's buckets againstother's by key (other.buckets.find { |b| b.key == bucket.key }-- added in Convert remaining registration policy bucket key dependencies (SignupBucketFinder, EventChangeRegistrationPolicyService) to id #11892, with a comment explaining it's structurally needed for now). Onceother(typically a detached policy frombuild_from_hash) reliably carries ids for anything pre-existing, this should match by id instead, with a bucket that has no id always counting as "different" (correct: a brand-new bucket, by definition, changes the policy).EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_for(app/services/event_change_registration_policy_service.rb): the one remaining commented key-based bridge from Convert remaining registration policy bucket key dependencies (SignupBucketFinder, EventChangeRegistrationPolicyService) to id #11892, translating a real signup's persisted bucket into its detached candidate counterpart during simulation. Same fix: id-based lookup for anything that already existed pre-edit; a genuinely brand-new bucket in the same edit has no id at this point and so has no "current"/"requested" candidate to bridge to at all (correct -- a signup can't already be sitting in, or have already requested, a bucket that didn't exist yet).Before removing the
sync_buckets_from_hash!fallback, audit the other callers ofRegistrationPolicy.build_from_hash/#update_from!beyond theupdateEventmutation path that #11895 fixes, to confirm each one either (a) never has pre-existing buckets to match against (a brand-new policy/record, so the fallback was never load-bearing there), or (b) needs its own id-plumbing fix first:app/graphql/mutations/create_event.rb,create_filler_event.rb,create_event_proposal.rb-- likely (a), since these build a policy for a brand-newEvent/EventProposalwith no prior buckets to correlate against.app/graphql/mutations/update_event_proposal.rb,app/services/accept_event_proposal_service.rb-- need checking; these operate on anEventProposal's registration policy, which can pre-exist across an edit, similar to the event case Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key) #11895 fixes. May need their own frontend id-plumbing analogous to Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key) #11895 if the event proposal edit form has the sameas_json-omits-id gap.app/services/import_convention_data_service.rb-- confirmed (a): always creates a brand-newConvention, never matches against pre-existing buckets, only cross-referencesbucket_keywithin its own import payload. Unaffected by this issue.Proposed Change
RegistrationPolicy#sync_buckets_from_hash!'sexisting_by_keyfallback (after confirming the caller audit above).RegistrationPolicy#equivalent_to?to id-based matching.EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_forto an id-based lookup.RegistrationPolicy#build_from_hash/#sync_buckets_from_hash!/RegistrationPolicyBucket#occupies_bucket_as_signup?that describe the key-based fallback, since it will no longer exist.Benefits
RegistrationPolicyBucket#keyafter this land are the column's own getter/setter/validation and whatever writes it on creation -- i.e. exactly what Drop the registration_policy_buckets.key column (phase 4 of dropping RegistrationPolicyBucket#key) #11898 needs to be true before dropping the column.Tradeoffs
updateEventstill needs its own frontend id-plumbing fix first, similar to Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key) #11895. Ifupdate_event_proposal.rb/accept_event_proposal_service.rbturn out to have the same gap, that's a prerequisite sub-task, not a blocker to filing this issue now.References
registration_policy_buckets.key; see Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key) #11895, Deprecate and remove the remaining bucket key-based GraphQL/Liquid surface (phase 2 of dropping RegistrationPolicyBucket#key) #11896, Drop the registration_policy_buckets.key column (phase 4 of dropping RegistrationPolicyBucket#key) #11898Issue drafted by Claude