TAN-5720 Inconsistent custom fields ordering (Part 2)

back/.rubocop_todo.yml

@@ -201,6 +201,7 @@ RSpec/DescribeClass:
     - 'spec/tasks/single_use/migrate_homepage_craftjson_task_spec.ignore.rb'
     - 'spec/tasks/single_use/rename_gv_profile_spec.ignore.rb'
     - 'spec/tasks/single_use/substitute_cl_gv_task_spec.ignore.rb'
+    - 'spec/tasks/single_use/custom_field_orderings_spec.ignore.rb'
 
 # Offense count: 149
 # This cop supports safe autocorrection (--autocorrect).
@@ -337,6 +338,7 @@ RSpec/SpecFilePathSuffix:
     - 'spec/tasks/single_use/move_ideation_custom_forms_to_project_spec.ignore.rb'
     - 'spec/tasks/single_use/rename_gv_profile_spec.ignore.rb'
     - 'spec/tasks/single_use/substitute_cl_gv_task_spec.ignore.rb'
+    - 'spec/tasks/single_use/custom_field_orderings_spec.ignore.rb'
 
 # Offense count: 77
 RSpec/StubbedMock:

back/app/models/custom_field.rb

@@ -47,7 +47,9 @@
 #
 # Indexes
 #
-#  index_custom_fields_on_resource_type_and_resource_id  (resource_type,resource_id)
+#  index_custom_fields_on_ordering                         (ordering) UNIQUE WHERE (resource_id IS NULL)
+#  index_custom_fields_on_resource_id_and_ordering_unique  (resource_id,ordering) UNIQUE
+#  index_custom_fields_on_resource_type_and_resource_id    (resource_type,resource_id)
 #
 
 # support table :

back/app/models/custom_form.rb

@@ -31,28 +31,6 @@ class CustomForm < ApplicationRecord
   before_validation :sanitize_print_start_multiloc
   before_validation :sanitize_print_end_multiloc
 
-  # Fixes custom fields ordering by:
-  # - Moving the first container field (page) to the first position if any
-  # - Ensuring consecutive integers without gaps or duplicates
-  #
-  # This method can be removed once we are confident that inconsistencies in the ordering
-  # are no longer introduced.
-  def heal_fields_ordering!
-    CustomForm.transaction do
-      fields = custom_fields.to_a
-
-      first_container_idx = fields.index(&:page?)
-      fields.insert(0, fields.delete_at(first_container_idx)) if first_container_idx&.positive?
-
-      fields.each.with_index do |field, index|
-        next if field.ordering == index
-
-        logger.debug('Healing field ordering', field_id: field.id, from: field.ordering, to: index)
-        field.update_column(:ordering, index)
-      end
-    end
-  end
-
   # Timestamp when the fields (not the form) were last updated.
   def fields_updated!
     update!(fields_last_updated_at: Time.now)

back/db/migrate/20251029135211_add_uniqueness_constraint_to_custom_fields_ordering.rb

@@ -0,0 +1,17 @@
+class AddUniquenessConstraintToCustomFieldsOrdering < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def change
+    # Input forms
+    add_index :custom_fields, %i[resource_id ordering],
+      unique: true,
+      name: 'index_custom_fields_on_resource_id_and_ordering_unique',
+      algorithm: :concurrently
+
+    # Registration fields (no resource_id)
+    add_index :custom_fields, :ordering,
+      unique: true,
+      where: 'resource_id IS NULL',
+      algorithm: :concurrently
+  end
+end

back/db/structure.sql

@@ -365,6 +365,8 @@ DROP INDEX IF EXISTS public.index_email_campaigns_campaign_email_commands_on_rec
 DROP INDEX IF EXISTS public.index_dismissals_on_campaign_name_and_user_id;
 DROP INDEX IF EXISTS public.index_custom_forms_on_participation_context;
 DROP INDEX IF EXISTS public.index_custom_fields_on_resource_type_and_resource_id;
+DROP INDEX IF EXISTS public.index_custom_fields_on_resource_id_and_ordering_unique;
+DROP INDEX IF EXISTS public.index_custom_fields_on_ordering;
 DROP INDEX IF EXISTS public.index_custom_field_options_on_custom_field_id_and_key;
 DROP INDEX IF EXISTS public.index_custom_field_options_on_custom_field_id;
 DROP INDEX IF EXISTS public.index_custom_field_option_images_on_custom_field_option_id;
@@ -5226,6 +5228,20 @@ CREATE INDEX index_custom_field_options_on_custom_field_id ON public.custom_fiel
 CREATE UNIQUE INDEX index_custom_field_options_on_custom_field_id_and_key ON public.custom_field_options USING btree (custom_field_id, key);
 
 
+--
+-- Name: index_custom_fields_on_ordering; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE UNIQUE INDEX index_custom_fields_on_ordering ON public.custom_fields USING btree (ordering) WHERE (resource_id IS NULL);
+
+
+--
+-- Name: index_custom_fields_on_resource_id_and_ordering_unique; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE UNIQUE INDEX index_custom_fields_on_resource_id_and_ordering_unique ON public.custom_fields USING btree (resource_id, ordering);
+
+
 --
 -- Name: index_custom_fields_on_resource_type_and_resource_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -7870,6 +7886,7 @@ ALTER TABLE ONLY public.ideas_topics
 SET search_path TO public,shared_extensions;
 
 INSERT INTO "schema_migrations" (version) VALUES
+('20251029135211'),
 ('20251021150138'),
 ('20251001090229'),
 ('20251001090208'),

back/engines/commercial/idea_custom_fields/app/controllers/idea_custom_fields/web_api/v1/idea_custom_fields_controller.rb

@@ -160,7 +160,7 @@ def update_fields!(page_temp_ids_to_ids_mapping, option_temp_ids_to_ids_mapping)
           end
           update_statements! field, statements_params, errors, index if statements_params
           relate_map_config_to_field(field, field_params, errors, index)
-          field.set_list_position(index)
+          field.move_to_bottom
           count_fields(field)
         end
         raise UpdateAllFailedError, errors if errors.present?

back/engines/commercial/user_custom_fields/app/controllers/user_custom_fields/web_api/v1/user_custom_fields_controller.rb

@@ -62,7 +62,6 @@ def update
         end
 
         def reorder
-          fix_reordering
           if @custom_field.insert_at(custom_field_params(@custom_field)[:ordering])
             SideFxCustomFieldService.new.after_update(@custom_field, current_user)
             render json: serialize_custom_fields(@custom_field.reload, params: jsonapi_serializer_params_with_locked_fields), status: :ok
@@ -102,16 +101,6 @@ def custom_field_params(resource)
         def serialize_custom_fields(...)
           UserCustomFields::WebApi::V1::UserCustomFieldSerializer.new(...).serializable_hash.to_json
         end
-
-        # Fix the ordering so it is sequential - sometimes some fields can get set to the same order position
-        def fix_reordering
-          fields = CustomField.registration.order(:ordering)
-          if fields.pluck(:ordering) != (0..fields.size - 1).to_a
-            fields.each_with_index do |field, index|
-              field.set_list_position(index)
-            end
-          end
-        end
       end
     end
   end

back/engines/commercial/user_custom_fields/spec/acceptance/user_custom_fields_spec.rb

@@ -256,19 +256,6 @@
         expect(CustomField.registration.order(:ordering)[1].id).to eq id
         expect(CustomField.registration.order(:ordering).map(&:ordering)).to eq (0..3).to_a
       end
-
-      example 'Fix the custom field order when ordering has gone wrong' do
-        ActiveRecord::Base.connection.execute("UPDATE custom_fields SET ordering = 0 WHERE id != '#{custom_field.id}'")
-        expect(custom_field.ordering).to eq 3
-        expect(CustomField.registration.order(:ordering).map(&:ordering)).to eq [0, 0, 0, 3]
-        do_request
-        expect(response_status).to eq 200
-        assert_status 200
-        expect(response_data.dig(:attributes, :ordering)).to match ordering
-        expect(custom_field.reload.ordering).to eq 1
-        expect(CustomField.registration.order(:ordering)[1].id).to eq id
-        expect(CustomField.registration.order(:ordering).map(&:ordering)).to eq (0..3).to_a
-      end
     end
 
     delete 'web_api/v1/users/custom_fields/:id' do

back/lib/tasks/single_use/20251103_custom_fields_orderings.rake

@@ -0,0 +1,51 @@
+namespace :fix_existing_tenants do
+  desc 'Fix custom fields orderings and make sure the first field is a page.'
+  task :custom_fields_orderings, [] => [:environment] do
+    reporter = ScriptReporter.new
+    Tenant.all.each do |tenant|
+      tenant.switch do
+        # rubocop:disable Performance/CollectionLiteralInLoop
+        ([nil] + CustomForm.all).each do |form|
+          fields = CustomField.where(resource_id: form&.id).order(:ordering)
+
+          # Check if ordering is correct
+          next if fields.pluck(:ordering) == (0...fields.size).to_a
+
+          # Fix orderings
+          fields.each.with_index do |field, index|
+            field.update_column(:ordering, index)
+          end
+          reporter.add_change(
+            nil,
+            'Fixed orderings',
+            context: { tenant: tenant.host, form: form&.id }
+          )
+
+          # First field must be a page
+          next if !form
+
+          fields = CustomField.where(resource_id: form.id).order(:ordering)
+          next if fields.first.page?
+
+          first_page = fields.find(&:page?)
+          if !first_page
+            reporter.add_error(
+              'Form has no pages!',
+              context: { tenant: tenant.host, form: form.id }
+            )
+            next
+          end
+
+          first_page.move_to_top
+          reporter.add_change(
+            nil,
+            'Page moved to top',
+            context: { tenant: tenant.host, form: form.id, page: first_page.id }
+          )
+        end
+        # rubocop:enable Performance/CollectionLiteralInLoop
+      end
+    end
+    reporter.report!('fix_custom_fields_orderings_report.json', verbose: true)
+  end
+end

back/spec/models/custom_form_spec.rb

@@ -3,35 +3,6 @@
 require 'rails_helper'
 
 RSpec.describe CustomForm do
-  describe '#heal_fields_ordering!' do
-    let(:form) { create(:custom_form) }
-
-    it 'moves the first container field to the first position' do
-      f1, f2 = form.custom_fields = [
-        create(:custom_field_text, ordering: 0),
-        create(:custom_field_page, ordering: 1)
-      ]
-
-      form.heal_fields_ordering!
-
-      expect(form.custom_fields.reload).to eq [f2, f1]
-      expect(form.custom_fields.pluck(:ordering)).to eq [0, 1]
-    end
-
-    it 'breaks the ordering ties (page comes first)' do
-      f1, f2, f3 = form.custom_fields = [
-        create(:custom_field_text, ordering: 0),
-        create(:custom_field_page, ordering: 0),
-        create(:custom_field_page, ordering: 1)
-      ]
-
-      form.heal_fields_ordering!
-
-      expect(form.custom_fields.reload).to match_array [f2, f1, f3]
-      expect(form.custom_fields.pluck(:ordering)).to eq [0, 1, 2]
-    end
-  end
-
   describe 'multiloc_sanitization' do
     it 'sanitizes script tags in the description' do
       custom_form = create(

back/spec/tasks/single_use/custom_field_orderings_spec.ignore.rb

@@ -0,0 +1,45 @@
+require 'rails_helper'
+
+describe 'fix_existing_tenants:custom_fields_orderings rake task' do
+  before { load_rake_tasks_if_not_loaded }
+
+  it 'Fixes the orderings' do
+    form1 = create(:custom_form)
+    create(:custom_field, resource: form1, input_type: 'number', key: 'field2')
+    create(:custom_field_page, resource: form1, key: 'field1').update_column(:ordering, 0)
+    create(:custom_field, resource: form1, input_type: 'number', key: 'field3').update_column(:ordering, 3)
+    create(:custom_field, resource: form1, input_type: 'text', key: 'field4').update_column(:ordering, 3)
+    create(:custom_field_page, resource: form1, key: 'field5').update_column(:ordering, 4)
+
+    form2 = create(:custom_form)
+    create(:custom_field_page, resource: form2, key: 'field1')
+    create(:custom_field, resource: form2, input_type: 'number', key: 'field2')
+    create(:custom_field, resource: form2, input_type: 'text', key: 'field3')
+    create(:custom_field_page, resource: form2, key: 'field4')
+
+    create(:custom_field, resource_id: nil, resource_type: 'User', input_type: 'text', key: 'field1')
+    create(:custom_field, resource_id: nil, resource_type: 'User', input_type: 'number', key: 'field2').update_column(:ordering, 2)
+
+    Rake::Task['fix_existing_tenants:custom_fields_orderings'].invoke
+
+    expect(form1.reload.custom_fields.pluck(:key, :ordering)).to eq([
+      ['field1', 0],
+      ['field2', 1],
+      ['field3', 2],
+      ['field4', 3],
+      ['field5', 4]
+    ])
+
+    expect(form2.reload.custom_fields.pluck(:key, :ordering)).to eq([
+      ['field1', 0],
+      ['field2', 1],
+      ['field3', 2],
+      ['field4', 3]
+    ])
+
+    expect(CustomField.where(resource_id: nil).order(:ordering).pluck(:key, :ordering)).to eq([
+      ['field1', 0],
+      ['field2', 1]
+    ])
+  end
+end