refactor(repetition): remove parent_id

app/models/champ.rb

@@ -4,19 +4,17 @@ class Champ < ApplicationRecord
   include ChampConditionalConcern
   include ChampsValidateConcern
 
-  self.ignored_columns += [:type_de_champ_id]
+  self.ignored_columns += [:type_de_champ_id, :parent_id]
 
   attr_readonly :stable_id
 
   belongs_to :dossier, inverse_of: false, touch: true, optional: false
-  belongs_to :parent, class_name: 'Champ', optional: true
   has_many_attached :piece_justificative_file
 
   # We declare champ specific relationships (Champs::CarteChamp, Champs::SiretChamp and Champs::RepetitionChamp)
   # here because otherwise we can't easily use includes in our queries.
   has_many :geo_areas, -> { order(:created_at) }, dependent: :destroy, inverse_of: :champ
   belongs_to :etablissement, optional: true, dependent: :destroy
-  has_many :champs, foreign_key: :parent_id, inverse_of: :parent
 
   delegate :procedure, to: :dossier
 
@@ -79,13 +77,8 @@ def type_de_champ
   delegate :used_by_routing_rules?, to: :type_de_champ
 
   scope :updated_since?, -> (date) { where('champs.updated_at > ?', date) }
-  scope :public_only, -> { where(private: false) }
-  scope :private_only, -> { where(private: true) }
-  scope :root, -> { where(parent_id: nil) }
   scope :prefilled, -> { where(prefilled: true) }
 
-  before_create :set_dossier_id, if: :needs_dossier_id?
-  before_validation :set_dossier_id, if: :needs_dossier_id?
   before_save :cleanup_if_empty
   before_save :normalize
   after_update_commit :fetch_external_data_later
@@ -232,7 +225,7 @@ def update_with_external_data!(data:)
   end
 
   def clone(fork = false)
-    champ_attributes = [:parent_id, :private, :row_id, :type, :stable_id, :stream]
+    champ_attributes = [:private, :row_id, :type, :stable_id, :stream]
     value_attributes = fork || !private? ? [:value, :value_json, :data, :external_id] : []
     relationships = fork || !private? ? [:etablissement, :geo_areas] : []
 
@@ -265,14 +258,6 @@ def html_id
     type_de_champ.html_id(row_id)
   end
 
-  def needs_dossier_id?
-    !dossier_id && parent_id
-  end
-
-  def set_dossier_id
-    self.dossier_id = parent.dossier_id
-  end
-
   def cleanup_if_empty
     if fetch_external_data? && persisted? && external_id_changed?
       self.data = nil

app/models/champs/repetition_champ.rb

@@ -12,11 +12,7 @@ def row_ids
   end
 
   def add_row(updated_by:)
-    # TODO: clean this up when parent_id is deprecated
-    row_id, added_champs = dossier.repetition_add_row(type_de_champ, updated_by:)
-    self.champs << added_champs
-    dossier.champs.reload if dossier.persisted?
-    row_id
+    dossier.repetition_add_row(type_de_champ, updated_by:)
   end
 
   def remove_row(row_id, updated_by:)

app/models/concerns/dossier_champs_concern.rb

@@ -40,7 +40,7 @@ def project_champs_private
   end
 
   def project_rows_for(type_de_champ)
-    [] if !type_de_champ.repetition?
+    return [] if !type_de_champ.repetition?
 
     children = revision.children_of(type_de_champ)
     row_ids = repetition_row_ids(type_de_champ)
@@ -84,7 +84,7 @@ def update_champs_attributes(attributes, scope, updated_by:)
   end
 
   def repetition_row_ids(type_de_champ)
-    [] if !type_de_champ.repetition?
+    return [] if !type_de_champ.repetition?
 
     stable_ids = revision.children_of(type_de_champ).map(&:stable_id)
     champs.filter { _1.stable_id.in?(stable_ids) && _1.row_id.present? }
@@ -98,10 +98,10 @@ def repetition_add_row(type_de_champ, updated_by:)
 
     row_id = ULID.generate
     types_de_champ = revision.children_of(type_de_champ)
-    # TODO: clean this up when parent_id is deprecated
-    added_champs = types_de_champ.map { _1.build_champ(row_id:, updated_by:) }
+    self.champs += types_de_champ.map { _1.build_champ(row_id:, updated_by:) }
+    champs.reload if persisted?
     @champs_by_public_id = nil
-    [row_id, added_champs]
+    row_id
   end
 
   def repetition_remove_row(type_de_champ, row_id, updated_by:)
@@ -158,13 +158,6 @@ def champ_with_attributes_for_update(type_de_champ, row_id, updated_by:)
       attributes[:data] = nil
     end
 
-    parent = revision.parent_of(type_de_champ)
-    if parent.present?
-      attributes[:parent] = champs.find { _1.stable_id == parent.stable_id }
-    else
-      attributes[:parent] = nil
-    end
-
     @champs_by_public_id = nil
 
     [champ, attributes]

app/models/concerns/dossier_clone_concern.rb

@@ -101,7 +101,6 @@ def clone(user: nil, fork: false)
         kopy.state = Dossier.states.fetch(:brouillon)
         kopy.champs = cloned_champs.values.map do |(_, champ)|
           champ.dossier = kopy
-          champ.parent = cloned_champs[champ.parent_id].second if champ.child?
           champ
         end
       end
@@ -152,33 +151,16 @@ def forked_groupe_instructeur_changed?
   def apply_diff(diff)
     champs_index = (champs_for_revision(scope: :public) + diff[:added]).index_by(&:public_id)
 
-    diff[:added].each do |champ|
-      if champ.child?
-        champ.update_columns(dossier_id: id, parent_id: champs_index.fetch(champ.parent.public_id).id)
-      else
-        champ.update_column(:dossier_id, id)
-      end
-    end
+    diff[:added].each { _1.update_column(:dossier_id, id) }
 
-    champs_to_remove = []
+    # a bit of a hack to work around unicity index
+    remove_group_id = ULID.generate
     diff[:updated].each do |champ|
-      old_champ = champs_index.fetch(champ.public_id)
-      champs_to_remove << old_champ
-
-      if champ.child?
-        # we need to do that in order to avoid a foreign key constraint
-        old_champ.update(row_id: nil)
-        champ.update_columns(dossier_id: id, parent_id: champs_index.fetch(champ.parent.public_id).id)
-      else
-        champ.update_column(:dossier_id, id)
-      end
+      champs_index.fetch(champ.public_id).update(row_id: remove_group_id)
+      champ.update_column(:dossier_id, id)
     end
 
-    champs_to_remove += diff[:removed]
-    children_champs_to_remove, root_champs_to_remove = champs_to_remove.partition(&:child?)
-
-    children_champs_to_remove.each(&:destroy!)
-    Champ.where(parent_id: root_champs_to_remove.map(&:id)).destroy_all
-    root_champs_to_remove.each(&:destroy!)
+    Champ.where(row_id: remove_group_id).destroy_all
+    diff[:removed].each(&:destroy!)
   end
 end

app/models/concerns/dossier_rebase_concern.rb

@@ -65,9 +65,7 @@ def rebase
       .tap { _1.default = Champ.none }
 
     # remove champ
-    children_champ, root_champ = changes_by_op[:remove].partition(&:child?)
-    children_champ.each { champs_by_stable_id[_1.stable_id].destroy_all }
-    root_champ.each { champs_by_stable_id[_1.stable_id].destroy_all }
+    changes_by_op[:remove].each { champs_by_stable_id[_1.stable_id].destroy_all }
 
     # update champ
     changes_by_op[:update].each { apply(_1, champs_by_stable_id[_1.stable_id]) }
@@ -78,8 +76,6 @@ def rebase
     # add champ (after changing dossier revision to avoid errors)
     changes_by_op[:add]
       .map { target_coordinates_by_stable_id[_1.stable_id] }
-      # add parent champs first so we can then add children
-      .sort_by { _1.child? ? 1 : 0 }
       .each { add_new_champs_for_revision(_1) }
   end
 
@@ -119,28 +115,24 @@ def apply(change, champs)
 
   def add_new_champs_for_revision(target_coordinate)
     if target_coordinate.child?
-      # If this type de champ is a child, we create a new champ for each row of the parent
-      parent_stable_id = target_coordinate.parent.stable_id
-
-      champs.filter { _1.stable_id == parent_stable_id }.each do |champ_repetition|
-        if champ_repetition.champs.present?
-          champ_repetition.champs.map(&:row_id).uniq.each do |row_id|
-            champs << create_champ(target_coordinate, champ_repetition, row_id:)
-          end
-        elsif target_coordinate.parent.mandatory?
-          champs << create_champ(target_coordinate, champ_repetition, row_id: ULID.generate)
+      row_ids = repetition_row_ids(target_coordinate.parent.type_de_champ)
+
+      if row_ids.present?
+        row_ids.each do |row_id|
+          create_champ(target_coordinate, row_id:)
         end
+      elsif target_coordinate.parent.mandatory?
+        create_champ(target_coordinate, row_id: ULID.generate)
       end
     else
-      create_champ(target_coordinate, self)
+      create_champ(target_coordinate)
     end
   end
 
-  def create_champ(target_coordinate, parent, row_id: nil)
-    target_coordinate
+  def create_champ(target_coordinate, row_id: nil)
+    self.champs << target_coordinate
       .type_de_champ
       .build_champ(rebased_at: Time.zone.now, row_id:)
-      .tap { parent.champs << _1 }
   end
 
   def purge_piece_justificative_file(champ)

app/models/dossier.rb

@@ -45,11 +45,7 @@ class Dossier < ApplicationRecord
 
   has_one_attached :justificatif_motivation
 
-  has_many :champs
-  # We have to remove champs in a particular order - champs with a reference to a parent have to be
-  # removed first, otherwise we get a foreign key constraint error.
-  has_many :champs_to_destroy, -> { order(:parent_id) }, class_name: 'Champ', inverse_of: false, dependent: :destroy
-
+  has_many :champs, dependent: :destroy
   has_many :commentaires, inverse_of: :dossier, dependent: :destroy
   has_many :preloaded_commentaires, -> { includes(:dossier_correction, piece_jointe_attachments: :blob) }, class_name: 'Commentaire', inverse_of: :dossier
 
@@ -1160,12 +1156,12 @@ def build_default_champs
 
   def build_default_champs_for(types_de_champ)
     self.champs << types_de_champ.flat_map do |type_de_champ|
+      champ = type_de_champ.build_champ(dossier: self)
       if type_de_champ.repetition? && (type_de_champ.private? || type_de_champ.mandatory?)
         row_id = ULID.generate
-        parent = type_de_champ.build_champ(dossier: self)
-        [parent] + revision.children_of(type_de_champ).map { _1.build_champ(dossier: self, parent:, row_id:) }
+        [champ] + revision.children_of(type_de_champ).map { _1.build_champ(dossier: self, row_id:) }
       else
-        type_de_champ.build_champ(dossier: self)
+        champ
       end
     end
   end

app/models/dossier_preloader.rb

@@ -81,20 +81,8 @@ def load_dossier(dossier, champs, pj_template: false)
     end
     dossier.association(:champs).target = champs
 
-    # remove once parent_id is deprecated
-    champs_by_parent_id = champs.group_by(&:parent_id)
-
     champs.each do |champ|
       champ.association(:dossier).target = dossier
-
-      # remove once parent_id is deprecated
-      if champ.repetition?
-        children = champs_by_parent_id.fetch(champ.id, [])
-        children.each do |child|
-          child.association(:parent).target = champ
-        end
-        champ.association(:champs).target = children
-      end
     end
 
     # We need to do this because of the check on `Etablissement#champ` in

db/migrate/20240321081721_remove_champs_foreign_key.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class RemoveChampsForeignKey < ActiveRecord::Migration[7.0]
+  def change
+    remove_foreign_key :champs, column: :parent_id
+    remove_index :champs, :parent_id
+  end
+end

db/schema.rb

@@ -270,7 +270,6 @@
     t.index ["dossier_id", "stream", "stable_id", "row_id"], name: "index_champs_on_dossier_id_and_stream_and_stable_id_and_row_id", unique: true
     t.index ["dossier_id"], name: "index_champs_on_dossier_id"
     t.index ["etablissement_id"], name: "index_champs_on_etablissement_id"
-    t.index ["parent_id"], name: "index_champs_on_parent_id"
     t.index ["row_id"], name: "index_champs_on_row_id"
     t.index ["stable_id"], name: "index_champs_on_stable_id"
     t.index ["type"], name: "index_champs_on_type"
@@ -1264,7 +1263,6 @@
   add_foreign_key "avis", "experts_procedures"
   add_foreign_key "batch_operations", "instructeurs"
   add_foreign_key "bulk_messages", "procedures"
-  add_foreign_key "champs", "champs", column: "parent_id"
   add_foreign_key "champs", "dossiers"
   add_foreign_key "champs", "etablissements"
   add_foreign_key "champs", "types_de_champ"

spec/components/editable_champ/editable_champ_component_spec.rb

@@ -5,7 +5,7 @@
   let(:types_de_champ_public) { [] }
   let(:types_de_champ_private) { [] }
   let(:dossier) { create(:dossier, :with_populated_champs, procedure:) }
-  let(:champ) { dossier.champs.first }
+  let(:champ) { (dossier.project_champs_public + dossier.project_champs_private).first }
 
   let(:component) { described_class.new(form: nil, champ:) }
 

spec/controllers/instructeurs/dossiers_controller_spec.rb

@@ -1037,7 +1037,7 @@
                   primary_value: 'primary',
                   secondary_value: 'secondary'
                 },
-                champ_repetition.champs.first.public_id => {
+                champ_repetition.rows.first.first.public_id => {
                   value: 'text'
                 },
                 champ_drop_down_list.public_id => {
@@ -1054,7 +1054,7 @@
           expect(champ_linked_drop_down_list.primary_value).to eq('primary')
           expect(champ_linked_drop_down_list.secondary_value).to eq('secondary')
           expect(champ_datetime.value).to eq(Time.zone.parse('2019-12-21T13:17:00').iso8601)
-          expect(champ_repetition.champs.first.value).to eq('text')
+          expect(champ_repetition.rows.first.first.value).to eq('text')
           expect(champ_drop_down_list.value).to eq('other value')
           expect(dossier.reload.last_champ_private_updated_at).to eq(now)
           expect(response).to have_http_status(200)

spec/factories/champ.rb

@@ -196,8 +196,8 @@
 
         evaluator.rows.times do
           row_id = ULID.generate
-          champ_repetition.champs << types_de_champ.map do |type_de_champ|
-            attrs = { dossier: champ_repetition.dossier, parent: champ_repetition, private: champ_repetition.private?, stable_id: type_de_champ.stable_id, row_id: }
+          champ_repetition.dossier.champs << types_de_champ.map do |type_de_champ|
+            attrs = { dossier: champ_repetition.dossier, private: champ_repetition.private?, stable_id: type_de_champ.stable_id, row_id: }
             build(:"champ_do_not_use_#{type_de_champ.type_champ}", **attrs)
           end
         end

spec/graphql/annotation_spec.rb

@@ -48,14 +48,14 @@
     end
 
     it 'add row' do
-      expect(annotation.champs.size).to eq(4)
+      expect(annotation.row_ids.size).to eq(2)
       expect(data).to eq(dossierModifierAnnotationAjouterLigne: {
         annotation: {
           id: annotation.to_typed_id
         },
         errors: nil
       })
-      expect(annotation.reload.champs.size).to eq(6)
+      expect(annotation.reload.row_ids.size).to eq(3)
     end
   end
 

spec/lib/recovery/dossier_life_cycle_spec.rb

@@ -83,7 +83,7 @@ def cleanup_export_file
 
       expect(reloaded_dossier.champs.count).not_to be(0)
 
-      expect(repetition(reloaded_dossier).champs.map(&:type)).to match_array(["Champs::PieceJustificativeChamp"])
+      expect(repetition(reloaded_dossier).rows.flatten.map(&:type)).to match_array(["Champs::PieceJustificativeChamp"])
       expect(pj_champ(reloaded_dossier).piece_justificative_file).to be_attached
       expect(carte(reloaded_dossier).geo_areas).to be_present
 

spec/models/champ_presentations/repetition_presentation_spec.rb

@@ -34,7 +34,7 @@
     stars.update(value: 4)
   end
 
-  let(:representation) { described_class.new(libelle, dossier.champs[0].reload.rows) }
+  let(:representation) { described_class.new(libelle, champ_repetition.rows) }
 
   describe '#to_s' do
     it 'returns a key-value representation' do