Merge pull request #9919 from mfo/US/better-renumber

feat(add_type_de_champ): stop renumbering all procedure_revision_type_de_champ

app/models/procedure_revision.rb

@@ -41,21 +41,23 @@ class ProcedureRevision < ApplicationRecord
     after_stable_id = params.delete(:after_stable_id)
     after_coordinate, _ = coordinate_and_tdc(after_stable_id)
 
-    # the collection is orderd by (position, id), so we can use after_coordinate.position
+    siblings = siblings_for(parent_coordinate:, private_tdc: params[:private])
-    # if not present, a big number is used to ensure the element is at the tail
-    position = (after_coordinate&.position) || 100_000
 
     tdc = TypeDeChamp.new(params)
     if tdc.save
-      h = { type_de_champ: tdc, parent_id: parent_id, position: position }
+      # moving all the impacted tdc down
-      coordinate = revision_types_de_champ.create!(h)
+      position = next_position_for(after_coordinate:, siblings:)
+      siblings.where("position >= ?", position).update_all("position = position + 1")
 
-      renumber(coordinate.reload.siblings)
+      # insertion of the new tdc
+      h = { type_de_champ: tdc, parent_id: parent_id, position: position }
+      revision_types_de_champ.create!(h)
     end
 
     # they are not aware of the addition
     types_de_champ_public.reset
     types_de_champ_private.reset
+    reload
 
     tdc
   rescue => e
@@ -74,12 +76,15 @@ class ProcedureRevision < ApplicationRecord
 
   def move_type_de_champ(stable_id, position)
     coordinate, _ = coordinate_and_tdc(stable_id)
+    siblings = coordinate.siblings
 
-    siblings = coordinate.siblings.to_a
+    if position > coordinate.position
+      siblings.where(position: coordinate.position..position).update_all("position = position - 1")
+    else
+      siblings.where(position: position..coordinate.position).update_all("position = position + 1")
+    end
+    coordinate.update_column(:position, position)
 
-    siblings.insert(position, siblings.delete_at(siblings.index(coordinate)))
-
-    renumber(siblings)
     coordinate.reload
 
     coordinate
@@ -98,11 +103,11 @@ class ProcedureRevision < ApplicationRecord
     tdc.destroy_if_orphan
 
     # they are not aware of the removal
+    coordinate.siblings.where("position >= ?", coordinate.position).update_all("position = position - 1")
+
     types_de_champ_public.reset
     types_de_champ_private.reset
 
-    renumber(coordinate.siblings)
-
     coordinate
   end
 
@@ -248,9 +253,23 @@ class ProcedureRevision < ApplicationRecord
     end
   end
 
-  def renumber(siblings)
+  def siblings_for(parent_coordinate: nil, private_tdc: false)
-    siblings.to_a.compact.each.with_index do |sibling, position|
+    if parent_coordinate
-      sibling.update_column(:position, position)
+      parent_coordinate.revision_types_de_champ
+    elsif private_tdc
+      revision_types_de_champ_private
+    else
+      revision_types_de_champ_public
+    end
+  end
+
+  def next_position_for(siblings:, after_coordinate: nil)
+    if siblings.to_a.empty? # first element of the list, starts at 0
+      0
+    elsif after_coordinate # middle of the list, between two items
+      after_coordinate.position + 1
+    else # last element of the list, end with last position + 1
+      siblings.to_a.last.position + 1
     end
   end
 

app/tasks/maintenance/update_draft_revision_type_de_champs_task.rb

@@ -29,8 +29,7 @@ module Maintenance
       fail "TypeDeChamp not found ! #{typed_id}" if stable_id.nil?
 
       tdc = revision.find_and_ensure_exclusive_use(stable_id)
-
+      revision.move_type_de_champ(stable_id, Integer(row['new_position']))
-      revision.move_type_de_champ(stable_id, compute_position(row, tdc.revision_type_de_champ))
 
       tdc.update!(
         libelle: row["new_libelle"].strip,
@@ -38,17 +37,5 @@ module Maintenance
         mandatory: row["new_required"] == "true"
       )
     end
-
-    private
-
-    def compute_position(row, rtdc)
-      position = Integer(row["new_position"])
-
-      if rtdc.child?
-        position - rtdc.parent.position - 1
-      else
-        position
-      end
-    end
   end
 end

spec/models/dossier_spec.rb

@@ -1911,7 +1911,18 @@ describe Dossier, type: :model do
 
   describe "champs_for_export" do
     context 'with a unconditionnal procedure' do
-      let(:procedure) { create(:procedure, :with_type_de_champ, :with_datetime, :with_yes_no, :with_explication, :with_commune, :with_repetition, zones: [create(:zone)]) }
+      let(:procedure) { create(:procedure, types_de_champ_public:, zones: [create(:zone)]) }
+      let(:types_de_champ_public) do
+        [
+          { type: :text },
+          { type: :datetime },
+          { type: :yes_no },
+          { type: :explication },
+          { type: :communes },
+          { type: :repetition, children: [{ type: :text }] }
+        ]
+      end
+
       let(:text_type_de_champ) { procedure.active_revision.types_de_champ_public.find { |type_de_champ| type_de_champ.type_champ == TypeDeChamp.type_champs.fetch(:text) } }
       let(:yes_no_type_de_champ) { procedure.active_revision.types_de_champ_public.find { |type_de_champ| type_de_champ.type_champ == TypeDeChamp.type_champs.fetch(:yes_no) } }
       let(:datetime_type_de_champ) { procedure.active_revision.types_de_champ_public.find { |type_de_champ| type_de_champ.type_champ == TypeDeChamp.type_champs.fetch(:datetime) } }
@@ -1931,7 +1942,7 @@ describe Dossier, type: :model do
           procedure.publish!
           dossier
           procedure.draft_revision.remove_type_de_champ(text_type_de_champ.stable_id)
-          procedure.draft_revision.add_type_de_champ(type_champ: TypeDeChamp.type_champs.fetch(:text), libelle: 'New text field')
+          coordinate = procedure.draft_revision.add_type_de_champ(type_champ: TypeDeChamp.type_champs.fetch(:text), libelle: 'New text field', after_stable_id: repetition_champ.stable_id)
           procedure.draft_revision.find_and_ensure_exclusive_use(yes_no_type_de_champ.stable_id).update(libelle: 'Updated yes/no')
           procedure.draft_revision.find_and_ensure_exclusive_use(commune_type_de_champ.stable_id).update(libelle: 'Commune de naissance')
           procedure.draft_revision.find_and_ensure_exclusive_use(repetition_type_de_champ.stable_id).update(libelle: 'Repetition')

spec/models/procedure_revision_spec.rb

@@ -28,6 +28,7 @@ describe ProcedureRevision do
       it 'public' do
         expect { subject }.to change { draft.types_de_champ_public.size }.from(2).to(3)
         expect(draft.types_de_champ_public.last).to eq(subject)
+        expect(draft.revision_types_de_champ_public.map(&:position)).to eq([0, 1, 2])
 
         expect(last_coordinate.position).to eq(2)
         expect(last_coordinate.type_de_champ).to eq(subject)
@@ -37,7 +38,12 @@ describe ProcedureRevision do
     context 'with a private tdc' do
       let(:tdc_params) { text_params.merge(private: true) }
 
-      it { expect { subject }.to change { draft.types_de_champ_private.count }.from(1).to(2) }
+      it 'private' do
+        expect { subject }.to change { draft.types_de_champ_private.count }.from(1).to(2)
+        expect(draft.types_de_champ_private.last).to eq(subject)
+        expect(draft.revision_types_de_champ_private.map(&:position)).to eq([0, 1])
+        expect(last_coordinate.position).to eq(1)
+      end
     end
 
     context 'with a repetition child' do
@@ -46,6 +52,7 @@ describe ProcedureRevision do
       it do
         expect { subject }.to change { draft.reload.types_de_champ.count }.from(4).to(5)
         expect(draft.children_of(type_de_champ_repetition).last).to eq(subject)
+        expect(draft.children_of(type_de_champ_repetition).map(&:revision_type_de_champ).map(&:position)).to eq([0, 1])
 
         expect(last_coordinate.position).to eq(1)
 
@@ -74,6 +81,7 @@ describe ProcedureRevision do
           expect(draft.revision_types_de_champ_public.map(&:libelle)).to eq(['l1', 'l2'])
           subject
           expect(draft.revision_types_de_champ_public.reload.map(&:libelle)).to eq(['l1', 'in the middle', 'l2'])
+          expect(draft.revision_types_de_champ_public.map(&:position)).to eq([0, 1, 2])
         end
       end
 
@@ -95,20 +103,19 @@ describe ProcedureRevision do
     context 'with 4 types de champ publiques' do
       it 'move down' do
         expect(draft.types_de_champ_public.index(type_de_champ_public)).to eq(0)
-
+        stable_id_before = draft.revision_types_de_champ_public.map(&:stable_id)
         draft.move_type_de_champ(type_de_champ_public.stable_id, 2)
         draft.reload
-
+        expect(draft.revision_types_de_champ_public.map(&:position)).to eq([0, 1, 2, 3])
         expect(draft.types_de_champ_public.index(type_de_champ_public)).to eq(2)
         expect(draft.procedure.types_de_champ_for_procedure_presentation.not_repetition.index(type_de_champ_public)).to eq(2)
       end
 
       it 'move up' do
         expect(draft.types_de_champ_public.index(last_type_de_champ)).to eq(3)
-
         draft.move_type_de_champ(last_type_de_champ.stable_id, 0)
         draft.reload
-
+        expect(draft.revision_types_de_champ_public.map(&:position)).to eq([0, 1, 2, 3])
         expect(draft.types_de_champ_public.index(last_type_de_champ)).to eq(0)
         expect(draft.procedure.types_de_champ_for_procedure_presentation.not_repetition.index(last_type_de_champ)).to eq(0)
       end

spec/tasks/maintenance/update_draft_revision_type_de_champs_task_spec.rb

@@ -21,9 +21,9 @@ module Maintenance
         demarche_id,id,new_libelle,new_description,new_required,new_position,delete_flag
         #{procedure.id},#{find_by_stable_id(12).to_typed_id},[NEW] Number,[NEW] Number desc,true,0,
         #{procedure.id},#{find_by_stable_id(13).to_typed_id},Bloc,[NEW] bloc desc,,1,
-        #{procedure.id},#{find_by_stable_id(132).to_typed_id},[NEW] RepNum,,true,2,
+        #{procedure.id},#{find_by_stable_id(132).to_typed_id},[NEW] RepNum,,true,0,
-        #{procedure.id},#{find_by_stable_id(131).to_typed_id},[NEW] RepText,,,3,
+        #{procedure.id},#{find_by_stable_id(131).to_typed_id},[NEW] RepText,,,1,
-        #{procedure.id},#{find_by_stable_id(11).to_typed_id},[supp] Text,,,4,true
+        #{procedure.id},#{find_by_stable_id(11).to_typed_id},[supp] Text,,,2,true
       CSV
     end