Merge pull request #7546 from betagouv/fix_after_stable_id

fix: after_stable_id pb

app/models/procedure_revision.rb

@@ -44,26 +44,22 @@ class ProcedureRevision < ApplicationRecord
 
   def add_type_de_champ(params)
     parent_stable_id = params.delete(:parent_stable_id)
+    parent_coordinate, _ = coordinate_and_tdc(parent_stable_id)
+    parent_id = parent_coordinate&.id
+
     after_stable_id = params.delete(:after_stable_id)
+    after_coordinate, _ = coordinate_and_tdc(after_stable_id)
 
-    coordinate = {}
+    # the collection is orderd by (position, id), so we can use after_coordinate.position
-
+    # if not present, a big number is used to ensure the element is at the tail
-    if parent_stable_id.present?
+    position = (after_coordinate&.position) || 100_000
-      parent_coordinate, _ = coordinate_and_tdc(parent_stable_id)
-
-      coordinate[:parent_id] = parent_coordinate.id
-      coordinate[:position] = last_position(parent_coordinate.revision_types_de_champ, after_stable_id)
-    elsif params[:private]
-      coordinate[:position] = last_position(revision_types_de_champ_private, after_stable_id)
-    else
-      coordinate[:position] = last_position(revision_types_de_champ_public, after_stable_id)
-    end
 
     tdc = TypeDeChamp.new(params)
     if tdc.save
-      coordinate[:type_de_champ] = tdc
+      h = { type_de_champ: tdc, parent_id: parent_id, position: position }
-      coordinate = revision_types_de_champ.create!(coordinate)
+      coordinate = revision_types_de_champ.create!(h)
-      reorder(coordinate.siblings)
+
+      reorder(coordinate.reload.siblings)
     end
 
     # they are not aware of the addition
@@ -219,6 +215,8 @@ class ProcedureRevision < ApplicationRecord
   end
 
   def coordinate_and_tdc(stable_id)
+    return [nil, nil] if stable_id.blank?
+
     coordinate = revision_types_de_champ
       .joins(:type_de_champ)
       .find_by(type_de_champ: { stable_id: stable_id })
@@ -478,18 +476,6 @@ class ProcedureRevision < ApplicationRecord
     cloned_type_de_champ
   end
 
-  def last_position(siblings, after_stable_id = nil)
-    if after_stable_id.present?
-      siblings
-        .joins(:type_de_champ)
-        .find_by(types_de_champ: { stable_id: after_stable_id })
-        .position + 1
-    else
-      last = siblings.last
-      last.present? ? last.position + 1 : 0
-    end
-  end
-
   def conditions_are_valid?
     stable_ids = types_de_champ_public.map(&:stable_id)
 

spec/models/procedure_revision_spec.rb

@@ -10,7 +10,14 @@ describe ProcedureRevision do
 
   describe '#add_type_de_champ' do
     # tdc: public: text, repetition ; private: text ; +1 text child of repetition
-    let(:procedure) { create(:procedure, :with_type_de_champ, :with_type_de_champ_private, :with_repetition) }
+    let(:procedure) do
+      create(:procedure).tap do |p|
+        p.draft_revision.add_type_de_champ(type_champ: :text, libelle: 'l1')
+        parent = p.draft_revision.add_type_de_champ(type_champ: :repetition, libelle: 'l2')
+        p.draft_revision.add_type_de_champ(type_champ: :text, libelle: 'l2', parent_stable_id: parent.stable_id)
+        p.draft_revision.add_type_de_champ(type_champ: :text, libelle: 'l1 private', private: true)
+      end
+    end
     let(:text_params) { { type_champ: :text, libelle: 'text' } }
     let(:tdc_params) { text_params }
     let(:last_coordinate) { draft.revision_types_de_champ.last }
@@ -58,6 +65,27 @@ describe ProcedureRevision do
 
       it { expect(subject.errors.full_messages).not_to be_empty }
     end
+
+    context 'after_stable_id' do
+      context 'with a valid after_stable_id' do
+        let(:tdc_params) { text_params.merge(after_stable_id: draft.revision_types_de_champ_public.first.stable_id, libelle: 'in the middle') }
+
+        it 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'])
+        end
+      end
+
+      context 'with blank valid after_stable_id' do
+        let(:tdc_params) { text_params.merge(after_stable_id: '', libelle: 'in the middle') }
+
+        it do
+          subject
+          expect(draft.revision_types_de_champ_public.reload.map(&:libelle)).to eq(['l1', 'l2', 'in the middle'])
+        end
+      end
+    end
   end
 
   describe '#move_type_de_champ' do