DEV: Refactor save_custom_fields methods (#24495)

Operate a key at a time, to make it clearer what's going on.

This also fixes a bug where array integer fields would get re-written
even when there wasn't a change.

app/models/concerns/has_custom_fields.rb

@@ -21,6 +21,20 @@ module HasCustomFields
       types[key]
     end
 
+    def self.serialize(value, type)
+      if value.is_a?(Hash) || type == :json
+        value.to_json
+      elsif TrueClass === value
+        "t"
+      elsif FalseClass === value
+        "f"
+      elsif Integer === value
+        value.to_s
+      else
+        value
+      end
+    end
+
     def self.cast_custom_field(key, value, types, return_array = true)
       return value unless type = get_custom_field_type(types, key)
 
@@ -100,7 +114,7 @@ module HasCustomFields
 
     def get_custom_field_type(name)
       @custom_field_types ||= {}
-      @custom_field_types[name]
+      @custom_field_types[name] || :string
     end
 
     def preload_custom_fields(objects, fields)
@@ -236,54 +250,40 @@ module HasCustomFields
 
   def save_custom_fields(force = false)
     if force || !custom_fields_clean?
-      dup = @custom_fields.dup.with_indifferent_access
-      array_fields = {}
-
       ActiveRecord::Base.transaction do
-        _custom_fields.reload.each do |f|
+        dup = @custom_fields.dup.with_indifferent_access
-          if dup[f.name].is_a?(Array)
+        fields_by_key = _custom_fields.reload.group_by(&:name)
-            # we need to collect Arrays fully before we can compare them
+
-            if !array_fields.has_key?(f.name)
+        (dup.keys.to_set + fields_by_key.keys.to_set).each do |key|
-              array_fields[f.name] = [f]
+          fields = fields_by_key[key] || []
-            else
+          value = dup[key]
-              array_fields[f.name] << f
+          field_type = self.class.get_custom_field_type(key)
-            end
+
-          elsif dup[f.name].is_a?(Hash)
+          if Array === field_type || (field_type != :json && Array === value)
-            if dup[f.name].to_json != f.value
+            value = value || []
-              f.destroy!
+            value.compact!
-            else
+            sub_type = field_type[0]
-              dup.delete(f.name)
+
+            value.map! { |v| HasCustomFields::Helpers.serialize(v, sub_type) }
+
+            unless value == fields.map(&:value)
+              fields.each(&:destroy!)
+
+              value.each { |subv| _custom_fields.create!(name: key, value: subv) }
             end
           else
-            t = {}
+            if value.nil?
-            self.class.append_custom_field(t, f.name, f.value)
+              fields.each(&:destroy!)
-
-            if dup.has_key?(f.name) && dup[f.name] == t[f.name]
-              dup.delete(f.name)
             else
-              f.destroy!
+              value = HasCustomFields::Helpers.serialize(value, field_type)
+
+              field = fields.find { |f| f.value == value }
+              fields.select { |f| f != field }.each(&:destroy!)
+
+              create_singular(key, value) if !field
             end
           end
         end
-
-        # let's iterate through our arrays and compare them
-        array_fields.each do |field_name, fields|
-          if fields.length == dup[field_name].length && fields.map(&:value) == dup[field_name]
-            dup.delete(field_name)
-          else
-            fields.each(&:destroy!)
-          end
-        end
-
-        dup.each do |k, v|
-          field_type = self.class.get_custom_field_type(k)
-
-          if v.is_a?(Array) && field_type != :json
-            v.each { |subv| _custom_fields.create!(name: k, value: subv) }
-          else
-            create_singular(k, v, field_type)
-          end
-        end
       end
 
       on_custom_fields_change