Correlate registration policy buckets by id instead of key when editing

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?)
 

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