Retire RegistrationPolicyBucket#key as an identity mechanism in the app layer - #11903
Merged
Conversation
…pp layer (#11897) RegistrationPolicy#sync_buckets_from_hash!/#match_existing_bucket no longer falls back to key-matching when an incoming bucket hash has no id -- a missing id now unambiguously means "create a new row." Audited every other caller of build_from_hash/#update_from! first: update_event_proposal.rb goes through the same generic, already-id-threaded registration policy editor as update_event.rb, and accept_event_proposal_service.rb/ create_event.rb/create_filler_event.rb/create_event_proposal.rb/ import_convention_data_service.rb never match against a pre-existing policy at all, so none of them needed their own id-plumbing fix first. RegistrationPolicy#equivalent_to? and EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_for both switched from key-based to id-based matching, with a bucket that has no id always counting as "different"/unmatched, per the issue. RegistrationPolicyBucket#occupies_bucket_as_signup? was reviewed too, but needs no change -- it's already id/object-identity based, never key-based. Fixed the test fixtures this surfaced: several tests built a "new_registration_policy" via key-only hashes to describe editing an existing bucket, relying on the now-removed fallback; added a build_edited_registration_policy test helper that supplies the real id for any matching existing bucket, mirroring what the frontend editor sends. Also added assert_registration_policies_have_equivalent_buckets, since RegistrationPolicy#equivalent_to? can no longer verify "these two independently-persisted/detached policies describe the same buckets" (e.g. a clone against its source) now that it matches by id -- several tests needed this key-based comparison instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11897
Summary
RegistrationPolicy#sync_buckets_from_hash!/#match_existing_bucket: removed theexisting_by_keyfallback. A bucket hash with noidis now unambiguously "create a new row" — never "maybe this secretly matches something by key."RegistrationPolicy#equivalent_to?: switched from key-based to id-based bucket matching. A bucket with no id always counts as "different" (correct — a brand-new bucket, by definition, changes the policy).EventChangeRegistrationPolicyService::SignupSimulator#candidate_bucket_for: switched to an id-based lookup, matching the same reasoning.RegistrationPolicyBucket#occupies_bucket_as_signup?: reviewed per the issue, but needs no change — it's already id/object-identity based, never key-based.Caller audit (per the issue's request, before removing the fallback)
update_event_proposal.rb— goes through the exact same generic, already-id-threaded registration policy editor asupdate_event.rb(Give the registration policy editor real bucket ids (phase 1 of dropping RegistrationPolicyBucket#key) #11895), since both Events and EventProposals use the sameregistration_policyform-item type. No separate frontend fix needed.accept_event_proposal_service.rb,create_event.rb,create_filler_event.rb,create_event_proposal.rb— none callupdate_from!/sync_buckets_from_hash!against a pre-existing policy (they either build a policy for a brand-new record, or usebuild_from_hash_as_clone, which never syncs against the source). Unaffected.import_convention_data_service.rb— confirmed always a brand-newConvention, per the issue.Test fallout
This surfaced a real gap in the test suite: several tests built a
new_registration_policyfixture via key-only hashes (no ids) to describe "editing an existing bucket," implicitly relying on the fallback being removed. Fixed by:build_edited_registration_policytotest_helper.rb, which supplies the real id for any bucket spec whose key matches an existing bucket — mirroring what the frontend editor now actually sends.assert_registration_policies_have_equivalent_buckets, sinceequivalent_to?can no longer verify "these two independently-persisted/detached policies describe the same buckets" (e.g. a clone against its source, or a freshly-persisted policy against the detached hash that requested it) now that it matches by id instead of content. Several existing tests needed this key-based comparison instead — genuinely different use cases than whatequivalent_to?is designed for post-Retire RegistrationPolicyBucket#key as an identity mechanism in the app layer (phase 3 of dropping RegistrationPolicyBucket#key) #11897.sync_buckets_from_hash!"preserves row id... falls back to key-matching without an id") to assert the opposite, correct behavior.Test plan
bundle exec rubocopon all changed files (0 offenses)bin/rails test(1169 passed, 1 pre-existing skip)🤖 Generated with Claude Code