From d04f579ba71420418a569bce99256c243b1cced4 Mon Sep 17 00:00:00 2001 From: Nat Budin Date: Fri, 7 Aug 2026 10:41:14 -0700 Subject: [PATCH] Correlate registration policy buckets by id instead of key when editing (#11870) RegistrationPolicy.build_from_hash now keeps a bucket's id (only the policy-level id is stripped), and sync_buckets_from_hash! matches buckets by id when one is given, falling back to key-matching otherwise. This lets an edited bucket's key be renamed without losing its row/identity, while keeping existing callers that don't supply bucket ids working exactly as before. Co-Authored-By: Claude Sonnet 4.6 --- app/models/registration_policy.rb | 38 +++++++++++++++------- test/models/registration_policy_test.rb | 43 +++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 15 deletions(-) diff --git a/app/models/registration_policy.rb b/app/models/registration_policy.rb index 32075477328..3e05e037961 100644 --- a/app/models/registration_policy.rb +++ b/app/models/registration_policy.rb @@ -44,6 +44,11 @@ def self.unlimited # hooked into `new`/`assign_attributes`. IGNORED_HASH_KEYS = %w[__typename id created_at updated_at].freeze + # Unlike the policy-level hash, a bucket's id is kept: it's how sync_buckets_from_hash! recognizes + # "this is the same bucket, just edited" instead of "this bucket was removed and a new one added" + # once this detached policy is applied onto a persisted one via update_from!. + IGNORED_BUCKET_HASH_KEYS = %w[__typename created_at updated_at].freeze + def self.build_from_hash(hash) # Strip __typename (an Apollo Client cache-normalization artifact) and id/created_at/updated_at # (would let this detached policy alias a real persisted row). @@ -53,7 +58,7 @@ def self.build_from_hash(hash) policy.buckets = bucket_hashes.each_with_index.map do |bucket_hash, index| RegistrationPolicyBucket.new( - bucket_hash.to_h.stringify_keys.except(*IGNORED_HASH_KEYS).merge("position" => index + 1) + bucket_hash.to_h.stringify_keys.except(*IGNORED_BUCKET_HASH_KEYS).merge("position" => index + 1) ) end end @@ -80,25 +85,29 @@ def update_from!(other) sync_buckets_from_hash!(other.buckets.map(&:attributes)) end - # Matches buckets by key (never rewritten in place); destroys removed keys 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. This key-based matching is about this API's own write-identity model, not - # signups' bucket_key -- it isn't resolved by the deferred bucket_key->FK conversion on signups - # unless that work also redesigns this API around ids. + # 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 + # 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 } - desired_keys = bucket_hashes.map { |hash| RegistrationPolicyBucket.normalize_key(hash["key"]) } + existing_by_id = buckets.index_by(&:id) existing_by_key = buckets.index_by(&:key) - existing_by_key.except(*desired_keys).each_value(&:destroy!) + matches = bucket_hashes.map { |hash| match_existing_bucket(hash, existing_by_id, existing_by_key) } + matched_ids = matches.compact.to_set(&:id) + + buckets.reject { |bucket| matched_ids.include?(bucket.id) }.each(&:destroy!) bucket_hashes.each_with_index do |hash, index| - normalized_key = RegistrationPolicyBucket.normalize_key(hash["key"]) attrs = hash.except("id", "registration_policy_id", "created_at", "updated_at").merge("position" => index + 1) - existing = existing_by_key[normalized_key] + existing = matches[index] - existing ? existing.update!(attrs.except("key")) : buckets.create!(attrs) + existing ? existing.update!(attrs) : buckets.create!(attrs) end # The destroys above happened directly on the fetched records, not through the association @@ -172,6 +181,11 @@ def equivalent_to?(other) 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"])] + end + def validate_flex_bucket_uniqueness flex_buckets = buckets.reject(&:marked_for_destruction?).select(&:anything?) diff --git a/test/models/registration_policy_test.rb b/test/models/registration_policy_test.rb index 7292e0719cd..47c704a87b9 100644 --- a/test/models/registration_policy_test.rb +++ b/test/models/registration_policy_test.rb @@ -75,7 +75,7 @@ class RegistrationPolicyTest < ActiveSupport::TestCase assert policy.buckets.none?(&:persisted?) end - it "strips __typename, id, created_at, and updated_at from both the policy and its buckets" do + it "strips __typename, id, created_at, and updated_at from the policy itself" do policy = RegistrationPolicy.build_from_hash( "__typename" => "RegistrationPolicyType", @@ -86,7 +86,16 @@ class RegistrationPolicyTest < ActiveSupport::TestCase ) assert_nil policy.id - assert_nil policy.buckets.first.id + end + + it "strips __typename, created_at, and updated_at from buckets but keeps their id" do + policy = + RegistrationPolicy.build_from_hash( + "buckets" => [{ "__typename" => "RegistrationPolicyBucketType", "id" => 999_999, "key" => "pcs" }] + ) + + assert_equal 999_999, policy.buckets.first.id + assert_not policy.buckets.first.persisted? end end @@ -114,7 +123,7 @@ class RegistrationPolicyTest < ActiveSupport::TestCase end describe "#sync_buckets_from_hash!" do - it "updates matched buckets in place, preserving their row id" do + it "updates matched buckets in place, preserving their row id (falls back to key-matching without an id)" do policy = create(:registration_policy, buckets: [build(:registration_policy_bucket, key: "pcs", total_slots: 2)]) original_id = policy.buckets.first.id @@ -163,6 +172,18 @@ class RegistrationPolicyTest < ActiveSupport::TestCase assert_equal ["pcs"], policy.buckets.map(&:key) end + + it "matches a bucket by id, allowing its key to be renamed without losing the row" 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!([{ id: original_id, key: "player_characters", name: "PCs", total_slots: 5 }]) + policy.reload + + assert_equal [original_id], policy.buckets.map(&:id) + assert_equal "player_characters", policy.buckets.first.key + assert_equal 5, policy.buckets.first.total_slots + end end describe "#update_from!" do @@ -182,6 +203,22 @@ class RegistrationPolicyTest < ActiveSupport::TestCase assert_equal true, policy.freeze_no_preference_buckets assert_equal 9, policy.buckets.first.total_slots end + + it "preserves a bucket's row across a key rename, since build_from_hash keeps the bucket's id" do + policy = create(:registration_policy, buckets: [build(:registration_policy_bucket, key: "pcs", total_slots: 2)]) + original_id = policy.buckets.first.id + other = + RegistrationPolicy.build_from_hash( + buckets: [{ id: original_id, key: "player_characters", name: "Player characters", total_slots: 9 }] + ) + + policy.update_from!(other) + policy.reload + + assert_equal [original_id], policy.buckets.map(&:id) + assert_equal "player_characters", policy.buckets.first.key + assert_equal 9, policy.buckets.first.total_slots + end end describe "persistence" do