Retire RegistrationPolicyBucket#key as an identity mechanism in the app layer

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

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)

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)

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

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

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,17 +142,19 @@ 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)
     end
 
     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)

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

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
 

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)