From 85079dded8f8f73457644e75b02a9af2e0397ba2 Mon Sep 17 00:00:00 2001 From: Nat Budin Date: Fri, 7 Aug 2026 20:08:53 -0700 Subject: [PATCH] Retire RegistrationPolicyBucket#key as an identity mechanism in the app 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 --- app/models/registration_policy.rb | 32 +++++++++---------- ...vent_change_registration_policy_service.rb | 20 ++++++------ .../mutations/create_event_proposal_test.rb | 7 +++- .../mutations/update_event_proposal_test.rb | 7 ++-- test/graphql/mutations/update_event_test.rb | 7 ++-- test/models/registration_policy_test.rb | 11 ++++--- .../accept_event_proposal_service_test.rb | 4 ++- ...change_registration_policy_service_test.rb | 21 +++++++----- test/test_helper.rb | 29 +++++++++++++++++ 9 files changed, 94 insertions(+), 44 deletions(-) diff --git a/app/models/registration_policy.rb b/app/models/registration_policy.rb index 39b1b2f1ca4..3207d9ba207 100644 --- a/app/models/registration_policy.rb +++ b/app/models/registration_policy.rb @@ -94,20 +94,19 @@ def update_from!(other) sync_buckets_from_hash!(other.buckets.map(&:attributes)) end - # Matches buckets primarily by id, so a bucket can have its key (or any other attribute) edited - # without losing its row/identity -- id is stable across an edit in a way key no longer needs to - # be. Falls back to key-matching for any incoming hash that doesn't carry an id (e.g. a policy - # built without ever round-tripping through a persisted one); this keeps callers that don't - # supply bucket ids working the same way they always have, rather than treating every one of - # their buckets as brand new. Destroys unmatched buckets before creating/updating the rest to - # avoid a transient key collision (positions are reassigned safely by the `positioned` gem, so no + # Matches buckets by id, so a bucket can have its key (or any other attribute) edited without + # losing its row/identity. A hash with no id is unambiguously a brand-new bucket -- every caller + # that edits an existing registration policy (the registration policy editor, via #11895's + # id-threading) reliably supplies id for anything that already exists, so there's no fallback to + # key here: a missing id always means "create a new row," never "maybe this secretly matches + # something by key." Destroys unmatched buckets before creating/updating the rest to avoid a + # transient key collision (positions are reassigned safely by the `positioned` gem, so no # equivalent care is needed there). Caller wraps this in a transaction. def sync_buckets_from_hash!(bucket_hashes) bucket_hashes = bucket_hashes.map { |hash| hash.to_h.stringify_keys } existing_by_id = buckets.index_by(&:id) - existing_by_key = buckets.index_by(&:key) - matches = bucket_hashes.map { |hash| match_existing_bucket(hash, existing_by_id, existing_by_key) } + matches = bucket_hashes.map { |hash| match_existing_bucket(hash, existing_by_id) } matched_ids = matches.compact.to_set(&:id) buckets.reject { |bucket| matched_ids.include?(bucket.id) }.each(&:destroy!) @@ -185,18 +184,17 @@ def equivalent_to?(other) return false unless freeze_no_preference_buckets? == other.freeze_no_preference_buckets? return false unless buckets.size == other.buckets.size - # other is typically a detached policy (e.g. from build_from_hash) with no bucket ids yet, so - # key is the only identity valid on both sides here -- same structural reason sync_buckets_from_hash! - # falls back to key. - other_buckets_by_key = other.buckets.index_by(&:key) - buckets.all? { |bucket| bucket.equivalent_to?(other_buckets_by_key[bucket.key]) } + # other is typically a detached policy (e.g. from build_from_hash) -- matches by id, so a + # bucket that's genuinely new in other (no id yet) never finds a match here and correctly + # counts as "different" (a brand-new bucket, by definition, changes the policy). + other_buckets_by_id = other.buckets.index_by(&:id) + buckets.all? { |bucket| bucket.equivalent_to?(other_buckets_by_id[bucket.id]) } end private - def match_existing_bucket(hash, existing_by_id, existing_by_key) - return existing_by_id[hash["id"].to_i] if hash["id"].presence - existing_by_key[RegistrationPolicyBucket.normalize_key(hash["key"])] + def match_existing_bucket(hash, existing_by_id) + existing_by_id[hash["id"].to_i] if hash["id"].presence end def validate_flex_bucket_uniqueness diff --git a/app/services/event_change_registration_policy_service.rb b/app/services/event_change_registration_policy_service.rb index 2d8818a9594..5aaea0da743 100644 --- a/app/services/event_change_registration_policy_service.rb +++ b/app/services/event_change_registration_policy_service.rb @@ -6,12 +6,14 @@ class Result < CivilService::Result self.result_class = Result # registration_policy here is new_registration_policy, a detached policy built via - # RegistrationPolicy.build_from_hash that hasn't been persisted yet, so its candidate buckets have - # no id (existing keys will get their old, stable id back once update_from! persists them in - # place; genuinely new keys get an id for the first time). candidate_bucket_for bridges a real - # signup's persisted bucket to its candidate counterpart by key, since key is the only identity - # valid on both sides pre-persist -- everywhere else in this class works with the resolved bucket - # objects (id or object identity, per SignupBucketFinder). + # RegistrationPolicy.build_from_hash that hasn't been persisted yet. A candidate bucket that + # already existed pre-edit carries its real, stable id (build_from_hash keeps it); a genuinely + # new candidate has no id until update_from! persists it further down the line. + # candidate_bucket_for bridges a real signup's persisted bucket (always a real id) to its + # candidate counterpart by id -- a signup can't already be in, or have already requested, a + # bucket that's brand new in this edit, so a candidate with no id correctly never matches. + # Everywhere else in this class works with the resolved bucket objects (id or object identity, + # per SignupBucketFinder). class SignupSimulator attr_reader :registration_policy, :immovable_signups, :new_signups_by_signup_id attr_accessor :logger @@ -84,11 +86,11 @@ def resolve_requested_bucket(signup, requested_bucket_override) def candidate_bucket_for(bucket) return nil unless bucket - candidate_buckets_by_key[bucket.key] + candidate_buckets_by_id[bucket.id] end - def candidate_buckets_by_key - @candidate_buckets_by_key ||= registration_policy.buckets.index_by(&:key) + def candidate_buckets_by_id + @candidate_buckets_by_id ||= registration_policy.buckets.index_by(&:id) end def place_signup(signup, bucket_finder, destination_bucket) diff --git a/test/graphql/mutations/create_event_proposal_test.rb b/test/graphql/mutations/create_event_proposal_test.rb index 537ce441eb0..3cdbdd44c9a 100644 --- a/test/graphql/mutations/create_event_proposal_test.rb +++ b/test/graphql/mutations/create_event_proposal_test.rb @@ -67,7 +67,12 @@ class Mutations::CreateEventProposalTest < ActiveSupport::TestCase end it "copies bucket content equivalently, not by reference" do - assert new_proposal.registration_policy.equivalent_to?(template_proposal.registration_policy) + # Not equivalent_to? -- it matches by id (see #11897), and this clone deliberately has + # different ids from its source (see build_from_hash_as_clone). Matches by key instead. + assert_registration_policies_have_equivalent_buckets( + template_proposal.registration_policy, + new_proposal.registration_policy + ) assert_not_equal( template_proposal.registration_policy.buckets.map(&:id), new_proposal.registration_policy.buckets.map(&:id) diff --git a/test/graphql/mutations/update_event_proposal_test.rb b/test/graphql/mutations/update_event_proposal_test.rb index 1a9921ad706..126b5aa0c15 100644 --- a/test/graphql/mutations/update_event_proposal_test.rb +++ b/test/graphql/mutations/update_event_proposal_test.rb @@ -70,8 +70,11 @@ class Mutations::UpdateEventProposalTest < ActiveSupport::TestCase change = FormResponseChange.find_by!(response: event_proposal, field_identifier: "registration_policy") # Not assert_equal against new_registration_policy.as_json directly -- new_registration_policy # is never persisted, so its buckets have no id, while change.new_value reflects the buckets - # actually created by the mutation (real ids). equivalent_to? compares content, not identity. - assert RegistrationPolicy.build_from_hash(change.new_value).equivalent_to?(new_registration_policy) + # actually created by the mutation (real ids). Matches by key instead (see #11897). + assert_registration_policies_have_equivalent_buckets( + new_registration_policy, + RegistrationPolicy.build_from_hash(change.new_value) + ) end it "stores different previous_value and new_value" do diff --git a/test/graphql/mutations/update_event_test.rb b/test/graphql/mutations/update_event_test.rb index c0c86f4140f..1fe153457c7 100644 --- a/test/graphql/mutations/update_event_test.rb +++ b/test/graphql/mutations/update_event_test.rb @@ -55,8 +55,11 @@ class Mutations::UpdateEventTest < ActiveSupport::TestCase change = FormResponseChange.find_by!(response: event, field_identifier: "registration_policy") # Not assert_equal against new_registration_policy.as_json directly -- new_registration_policy # is never persisted, so its buckets have no id, while change.new_value reflects the buckets - # actually created by the mutation (real ids). equivalent_to? compares content, not identity. - assert RegistrationPolicy.build_from_hash(change.new_value).equivalent_to?(new_registration_policy) + # actually created by the mutation (real ids). Matches by key instead (see #11897). + assert_registration_policies_have_equivalent_buckets( + new_registration_policy, + RegistrationPolicy.build_from_hash(change.new_value) + ) end it "stores different previous_value and new_value" do diff --git a/test/models/registration_policy_test.rb b/test/models/registration_policy_test.rb index 47c704a87b9..56ece91a321 100644 --- a/test/models/registration_policy_test.rb +++ b/test/models/registration_policy_test.rb @@ -123,14 +123,15 @@ class RegistrationPolicyTest < ActiveSupport::TestCase end describe "#sync_buckets_from_hash!" do - it "updates matched buckets in place, preserving their row id (falls back to key-matching without an id)" do + it "creates a new row (and destroys the old one) for a hash with no id, even with a matching key" do policy = create(:registration_policy, buckets: [build(:registration_policy_bucket, key: "pcs", total_slots: 2)]) original_id = policy.buckets.first.id policy.sync_buckets_from_hash!([{ key: "pcs", name: "Player characters", total_slots: 5 }]) policy.reload - assert_equal [original_id], policy.buckets.map(&:id) + assert_equal 1, policy.buckets.size + assert_not_equal original_id, policy.buckets.first.id assert_equal 5, policy.buckets.first.total_slots assert_equal "Player characters", policy.buckets.first.name end @@ -141,8 +142,9 @@ class RegistrationPolicyTest < ActiveSupport::TestCase :registration_policy, buckets: [build(:registration_policy_bucket, key: "pcs"), build(:registration_policy_bucket, key: "npcs")] ) + pcs_id = policy.buckets.find { |bucket| bucket.key == "pcs" }.id - policy.sync_buckets_from_hash!([{ key: "pcs" }]) + policy.sync_buckets_from_hash!([{ id: pcs_id, key: "pcs" }]) policy.reload assert_equal ["pcs"], policy.buckets.map(&:key) @@ -150,8 +152,9 @@ class RegistrationPolicyTest < ActiveSupport::TestCase it "creates buckets for new keys" do policy = create(:registration_policy, buckets: [build(:registration_policy_bucket, key: "pcs")]) + pcs_id = policy.buckets.first.id - policy.sync_buckets_from_hash!([{ key: "pcs" }, { key: "npcs", name: "NPCs" }]) + policy.sync_buckets_from_hash!([{ id: pcs_id, key: "pcs" }, { key: "npcs", name: "NPCs" }]) policy.reload assert_equal %w[pcs npcs], policy.buckets.map(&:key) diff --git a/test/services/accept_event_proposal_service_test.rb b/test/services/accept_event_proposal_service_test.rb index 2850684b521..5aa7695dcc0 100644 --- a/test/services/accept_event_proposal_service_test.rb +++ b/test/services/accept_event_proposal_service_test.rb @@ -86,7 +86,9 @@ class AcceptEventProposalServiceTest < ActiveSupport::TestCase assert event.registration_policy.present? assert_not_equal event_proposal.registration_policy_id, event.registration_policy_id assert_not_equal(event_proposal.registration_policy.buckets.map(&:id), event.registration_policy.buckets.map(&:id)) - assert event.registration_policy.equivalent_to?(event_proposal.registration_policy) + # Not equivalent_to? -- it matches by id (see #11897), and this clone deliberately has + # different ids from its source (see build_from_hash_as_clone). Matches by key instead. + assert_registration_policies_have_equivalent_buckets(event_proposal.registration_policy, event.registration_policy) end it "copies attached images" do diff --git a/test/services/event_change_registration_policy_service_test.rb b/test/services/event_change_registration_policy_service_test.rb index e7669398223..f7067c0acaf 100644 --- a/test/services/event_change_registration_policy_service_test.rb +++ b/test/services/event_change_registration_policy_service_test.rb @@ -7,8 +7,9 @@ class EventChangeRegistrationPolicyServiceTest < ActiveSupport::TestCase let(:event) { create(:event, convention: convention) } let(:the_run) { create(:run, event: event) } let(:new_registration_policy) do - RegistrationPolicy.build_from_hash( - buckets: [ + build_edited_registration_policy( + event, + [ { key: "dogs", name: "Dogs", slots_limited: true, total_slots: 1 }, { key: "cats", name: "Cats", slots_limited: true, total_slots: 1 }, { key: "anything", name: "Anything", slots_limited: true, total_slots: 1, anything: true } @@ -31,7 +32,10 @@ class EventChangeRegistrationPolicyServiceTest < ActiveSupport::TestCase assert result.success? event.reload - assert event.registration_policy.equivalent_to?(new_registration_policy) + # Not equivalent_to? -- new_registration_policy here describes an entirely new set of bucket + # keys with no ids at all, so it can never be "equivalent" (by id) to anything, including the + # very policy it was used to build. Check that the requested keys actually took effect instead. + assert_equal new_registration_policy.buckets.map(&:key).sort, event.registration_policy.buckets.map(&:key).sort assert_equal original_registration_policy_id, event.registration_policy_id end @@ -281,7 +285,7 @@ class EventChangeRegistrationPolicyServiceTest < ActiveSupport::TestCase describe "with a removed bucket mapped to no preference, and no flex bucket in the new policy" do let(:new_registration_policy) do - RegistrationPolicy.build_from_hash(buckets: [{ key: "dogs", name: "Dogs", slots_limited: true, total_slots: 1 }]) + build_edited_registration_policy(event, [{ key: "dogs", name: "Dogs", slots_limited: true, total_slots: 1 }]) end let(:event) do @@ -436,13 +440,14 @@ class EventChangeRegistrationPolicyServiceTest < ActiveSupport::TestCase describe "with bucket_key_mappings mapping to nil, when the new policy disallows no-preference signups" do let(:new_registration_policy) do - RegistrationPolicy.build_from_hash( - prevent_no_preference_signups: true, - buckets: [ + build_edited_registration_policy( + event, + [ { key: "dogs", name: "Dogs", slots_limited: true, total_slots: 1 }, { key: "cats", name: "Cats", slots_limited: true, total_slots: 1 }, { key: "anything", name: "Anything", slots_limited: true, total_slots: 1, anything: true } - ] + ], + prevent_no_preference_signups: true ) end diff --git a/test/test_helper.rb b/test/test_helper.rb index 0498227d7c0..fd6a8e0922c 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -124,6 +124,35 @@ def bucket_with_key(registration_policy, key) registration_policy.buckets.find { |bucket| bucket.key == normalized_key } end + # Test-only convenience: mimics what the registration policy editor now sends for an edit to an + # existing policy (see #11895/#11897) -- each bucket spec carries the real id of whichever + # existing bucket has the same key, so RegistrationPolicy#sync_buckets_from_hash! matches it by + # id rather than creating (and destroying the old) row. A spec whose key has no existing match + # gets no id, correctly describing a brand-new bucket. + def build_edited_registration_policy(event, bucket_specs, **policy_attrs) + existing_by_key = event.registration_policy.buckets.index_by(&:key) + buckets = + bucket_specs.map do |spec| + existing = existing_by_key[RegistrationPolicyBucket.normalize_key(spec[:key])] + existing ? spec.merge(id: existing.id) : spec + end + RegistrationPolicy.build_from_hash(policy_attrs.merge(buckets: buckets)) + end + + # Test-only convenience: RegistrationPolicy#equivalent_to? matches by id (see #11897), so it + # can't 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 (both real scenarios with no id overlap by design). Matches buckets by key + # instead, which is what actually varies in those cases, and compares content the same way + # RegistrationPolicyBucket#equivalent_to? always has. + def assert_registration_policies_have_equivalent_buckets(expected, actual) + actual_buckets_by_key = actual.buckets.index_by(&:key) + assert_equal expected.buckets.map(&:key).sort, actual_buckets_by_key.keys.sort + expected.buckets.each do |bucket| + assert bucket.equivalent_to?(actual_buckets_by_key[bucket.key]), "bucket #{bucket.key.inspect} not equivalent" + end + end + # Counts SQL queries matching pattern issued while running the block, for asserting on N+1s # (e.g. assert_operator count_queries(/registration_policy_buckets/) { subject.call! }, :<=, 1). def count_queries(pattern)