Merge pull request #7300 from tchak/reapply-rollback-revisions

Reapply improvements on procedure_revision

app/models/procedure_revision.rb

@@ -54,7 +54,7 @@ class ProcedureRevision < ApplicationRecord
   end
 
   def find_or_clone_type_de_champ(stable_id)
-    type_de_champ = find_type_de_champ_by_stable_id(stable_id)
+    type_de_champ = types_de_champ.find_by!(stable_id: stable_id)
 
     if type_de_champ.only_present_on_draft?
       type_de_champ
@@ -66,35 +66,37 @@ class ProcedureRevision < ApplicationRecord
   end
 
   def move_type_de_champ(stable_id, position)
-    type_de_champ = find_type_de_champ_by_stable_id(stable_id)
+    # Ensure that if this is a child, it's parent is cloned to the new revision
+    clone_parent_to_draft_revision(stable_id)
 
-    if type_de_champ.parent.present?
-      repetition_type_de_champ = find_or_clone_type_de_champ(stable_id).parent
+    coordinate = revision_types_de_champ
+      .joins(:type_de_champ)
+      .find_by(type_de_champ: { stable_id: stable_id })
 
-      move_type_de_champ_hash(repetition_type_de_champ.types_de_champ.to_a, type_de_champ, position).each do |(id, position)|
-        type_de_champ = repetition_type_de_champ.types_de_champ.find(id)
-        type_de_champ.update!(order_place: position)
-        type_de_champ.revision_type_de_champ&.update!(position: position)
-      end
-    else
-      liste = type_de_champ.private? ? types_de_champ_private : types_de_champ_public
+    siblings = coordinate.siblings.to_a
 
-      move_type_de_champ_hash(liste.to_a, type_de_champ, position).each do |(id, position)|
-        revision_types_de_champ.find_by!(type_de_champ_id: id).update!(position: position)
-      end
-    end
+    siblings.insert(position, siblings.delete_at(siblings.index(coordinate)))
+
+    reorder(siblings)
   end
 
   def remove_type_de_champ(stable_id)
-    type_de_champ = find_type_de_champ_by_stable_id(stable_id)
+    # Ensure that if this is a child, it's parent is cloned to the new revision
+    clone_parent_to_draft_revision(stable_id)
 
-    if type_de_champ.only_present_on_draft?
-      type_de_champ.destroy
-    elsif type_de_champ.parent.present?
-      find_or_clone_type_de_champ(stable_id).destroy
-    else
-      types_de_champ.delete(type_de_champ)
+    coordinate = revision_types_de_champ
+      .joins(:type_de_champ)
+      .find_by(type_de_champ: { stable_id: stable_id })
+
+    tdc = coordinate.type_de_champ
+
+    coordinate.destroy
+
+    if tdc.revision_types_de_champ.empty?
+      tdc.destroy
     end
+
+    reorder(coordinate.siblings)
   end
 
   def draft?
@@ -150,6 +152,17 @@ class ProcedureRevision < ApplicationRecord
 
   private
 
+  def reorder(siblings)
+    siblings.to_a.compact.each.with_index do |sibling, position|
+        sibling.update(position: position)
+
+        # FIXME: to remove when order_place is no longer used
+        if sibling.parent_id.present?
+          sibling.type_de_champ.update!(order_place: position)
+        end
+      end
+  end
+
   def compare_attestation_template(from_at, to_at)
     changes = []
     if from_at.nil? && to_at.present?
@@ -391,25 +404,11 @@ class ProcedureRevision < ApplicationRecord
     cloned_type_de_champ
   end
 
-  def find_type_de_champ_by_stable_id(stable_id)
-    types_de_champ.find_by(stable_id: stable_id) || types_de_champ_in_repetition.find_by(stable_id: stable_id)
-  end
+  def clone_parent_to_draft_revision(stable_id)
+    type_de_champ = types_de_champ.find_by!(stable_id: stable_id)
 
-  def types_de_champ_in_repetition
-    parent_ids = types_de_champ.repetition.ids
-    TypeDeChamp.where(parent_id: parent_ids)
-  end
-
-  def move_type_de_champ_hash(types_de_champ, type_de_champ, new_index)
-    old_index = types_de_champ.index(type_de_champ)
-
-    if types_de_champ.delete_at(old_index)
-      types_de_champ.insert(new_index, type_de_champ)
-        .map.with_index do |type_de_champ, index|
-          [type_de_champ.id, index]
-        end
-    else
-      []
+    if type_de_champ.parent_id.present? && type_de_champ.only_present_on_draft?
+      find_or_clone_type_de_champ(type_de_champ.parent.stable_id)
     end
   end
 end

app/models/procedure_revision_type_de_champ.rb

@@ -28,6 +28,16 @@ class ProcedureRevisionTypeDeChamp < ApplicationRecord
     type_de_champ.private?
   end
 
+  def siblings
+    if parent_id.present?
+      revision.revision_types_de_champ.where(parent_id: parent_id).ordered
+    elsif private?
+      revision.revision_types_de_champ_private
+    else
+      revision.revision_types_de_champ_public
+    end
+  end
+
   private
 
   def set_position

spec/models/dossier_spec.rb

@@ -1453,7 +1453,7 @@ describe Dossier do
       it "should have champs from all revisions" do
         expect(dossier.types_de_champ.map(&:libelle)).to eq([text_type_de_champ.libelle, datetime_type_de_champ.libelle, "Yes/no", explication_type_de_champ.libelle, commune_type_de_champ.libelle, repetition_type_de_champ.libelle])
         expect(dossier_second_revision.types_de_champ.map(&:libelle)).to eq([datetime_type_de_champ.libelle, "Updated yes/no", explication_type_de_champ.libelle, 'Commune de naissance', "Repetition", "New text field"])
-        expect(dossier_champs_for_export.map { |(libelle)| libelle }).to eq([text_type_de_champ.libelle, datetime_type_de_champ.libelle, "Updated yes/no", "Commune de naissance", "Commune de naissance (Code insee)", "New text field"])
+        expect(dossier_champs_for_export.map { |(libelle)| libelle }).to eq([datetime_type_de_champ.libelle, text_type_de_champ.libelle, "Updated yes/no", "Commune de naissance", "Commune de naissance (Code insee)", "New text field"])
         expect(dossier_champs_for_export).to eq(dossier_second_revision_champs_for_export)
         expect(repetition_second_revision_champs_for_export.map { |(libelle)| libelle }).to eq(procedure.types_de_champ_for_procedure_presentation.repetition.map(&:libelle_for_export))
         expect(repetition_second_revision_champs_for_export.first.size).to eq(2)

spec/models/procedure_presentation_and_revisions_spec.rb

@@ -41,7 +41,7 @@ describe ProcedurePresentation do
 
         before do
           created_tdc0 = procedure.draft_revision.add_type_de_champ(tdc0)
-          procedure.draft_revision.move_type_de_champ(created_tdc0.stable_id, 0)
+          procedure.draft_revision.reload.move_type_de_champ(created_tdc0.stable_id, 0)
           procedure.publish_revision!
         end
 

spec/models/procedure_revision_spec.rb

@@ -53,21 +53,21 @@ 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)
-        type_de_champ_public.update(order_place: nil)
+
         draft.move_type_de_champ(type_de_champ_public.stable_id, 2)
         draft.reload
+
         expect(draft.types_de_champ_public.index(type_de_champ_public)).to eq(2)
-        expect(draft.procedure.types_de_champ.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)
-        last_type_de_champ.update(order_place: nil)
+
         draft.move_type_de_champ(last_type_de_champ.stable_id, 0)
         draft.reload
+
         expect(draft.types_de_champ_public.index(last_type_de_champ)).to eq(0)
-        expect(draft.procedure.types_de_champ.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
     end
@@ -92,17 +92,19 @@ describe ProcedureRevision do
       end
 
       it 'move down' do
-        expect(type_de_champ_repetition.types_de_champ.index(second_child)).to eq(1)
+        expect(draft.children_of(type_de_champ_repetition).index(second_child)).to eq(1)
+
         draft.move_type_de_champ(second_child.stable_id, 2)
-        type_de_champ_repetition.reload
-        expect(type_de_champ_repetition.types_de_champ.index(second_child)).to eq(2)
+
+        expect(draft.children_of(type_de_champ_repetition).index(second_child)).to eq(2)
       end
 
       it 'move up' do
-        expect(type_de_champ_repetition.types_de_champ.index(last_child)).to eq(2)
+        expect(draft.children_of(type_de_champ_repetition).index(last_child)).to eq(2)
+
         draft.move_type_de_champ(last_child.stable_id, 0)
-        type_de_champ_repetition.reload
-        expect(type_de_champ_repetition.types_de_champ.index(last_child)).to eq(0)
+
+        expect(draft.children_of(type_de_champ_repetition).index(last_child)).to eq(0)
       end
     end
   end
@@ -124,15 +126,92 @@ describe ProcedureRevision do
       end
     end
 
+    context 'with multiple tdc' do
+      context 'in public tdc' do
+        let(:procedure) { create(:procedure, :with_type_de_champ, types_de_champ_count: 3) }
+
+        it 'reorders' do
+          expect(draft.revision_types_de_champ_public.pluck(:position)).to eq([0, 1, 2])
+
+          draft.remove_type_de_champ(draft.types_de_champ_public[1].stable_id)
+
+          expect(draft.revision_types_de_champ_public.pluck(:position)).to eq([0, 1])
+        end
+      end
+
+      context 'in repetition tdc' do
+        let(:procedure) { create(:procedure, :with_repetition) }
+        let!(:second_child) do
+          draft.add_type_de_champ({
+            type_champ: TypeDeChamp.type_champs.fetch(:text),
+            libelle: "second child",
+            parent_id: type_de_champ_repetition.stable_id
+          })
+        end
+
+        let!(:last_child) do
+          draft.add_type_de_champ({
+            type_champ: TypeDeChamp.type_champs.fetch(:text),
+            libelle: "last child",
+            parent_id: type_de_champ_repetition.stable_id
+          })
+        end
+
+        it 'reorders' do
+          children = draft.children_of(type_de_champ_repetition)
+          expect(children.pluck(:position)).to eq([0, 1, 2])
+          expect(children.pluck(:order_place)).to eq([0, 1, 2])
+
+          draft.remove_type_de_champ(children[1].stable_id)
+
+          children.reload
+
+          expect(children.pluck(:position)).to eq([0, 1])
+          expect(children.pluck(:order_place)).to eq([0, 1])
+        end
+      end
+    end
+
     context 'for a type_de_champ_repetition' do
       let(:procedure) { create(:procedure, :with_repetition) }
+      let!(:child) { child = draft.children_of(type_de_champ_repetition).first }
 
       it 'can remove its children' do
-        draft.remove_type_de_champ(type_de_champ_repetition.types_de_champ.first.stable_id)
+        draft.remove_type_de_champ(child.stable_id)
 
         expect(type_de_champ_repetition.types_de_champ).to be_empty
+        expect { child.reload }.to raise_error ActiveRecord::RecordNotFound
         expect(draft.types_de_champ_public.size).to eq(1)
       end
+
+      it 'can remove the parent' do
+        draft.remove_type_de_champ(type_de_champ_repetition.stable_id)
+
+        expect { child.reload }.to raise_error ActiveRecord::RecordNotFound
+        expect { type_de_champ_repetition.reload }.to raise_error ActiveRecord::RecordNotFound
+        expect(draft.types_de_champ_public).to be_empty
+      end
+
+      context 'when there already is a revision with this child' do
+        let!(:new_draft) { procedure.create_new_revision }
+
+        it 'can remove its children only in the new revision' do
+          new_draft.remove_type_de_champ(child.stable_id)
+
+          expect { child.reload }.not_to raise_error
+          expect(draft.children_of(type_de_champ_repetition).size).to eq(1)
+          expect(new_draft.children_of(type_de_champ_repetition)).to be_empty
+        end
+
+        it 'can remove the parent only in the new revision' do
+          new_draft.remove_type_de_champ(type_de_champ_repetition.stable_id)
+
+          expect { child.reload }.not_to raise_error
+          expect { type_de_champ_repetition.reload }.not_to raise_error
+          expect(draft.types_de_champ_public.size).to eq(1)
+          expect(new_draft.types_de_champ_public).to be_empty
+        end
+      end
     end
   end