From 084a68a121ac0e58d901d06c7616b73e8a89bac1 Mon Sep 17 00:00:00 2001
From: Paul Chavard <pro@paul.chavard.net>
Date: Tue, 27 Aug 2024 10:29:10 +0200
Subject: [PATCH 1/2] refactor(repetition): consolidate repetition manipulation
 methods

---
 .../champs/repetition_controller.rb           |  7 +-
 ...ssier_modifier_annotation_ajouter_ligne.rb |  2 +-
 app/graphql/types/dossier_type.rb             |  4 +-
 app/lib/data_fixer/dossier_champs_missing.rb  | 83 -------------------
 app/models/champs/repetition_champ.rb         | 31 +++----
 app/models/concerns/dossier_champs_concern.rb | 71 ++++++++++++----
 .../concerns/dossier_searchable_concern.rb    |  4 +-
 app/models/dossier.rb                         | 25 ++++--
 app/models/dossier_preloader.rb               |  4 +-
 app/models/procedure_revision.rb              | 13 +--
 .../prefill_repetition_type_de_champ.rb       |  3 +-
 app/serializers/champ_serializer.rb           |  6 +-
 app/serializers/dossier_serializer.rb         |  8 +-
 spec/graphql/annotation_spec.rb               |  2 +-
 .../data_fixer/dossier_champs_missing_spec.rb | 58 -------------
 spec/lib/recovery/dossier_life_cycle_spec.rb  |  2 +-
 .../repetition_presentation_spec.rb           | 11 ++-
 .../concerns/dossier_champs_concern_spec.rb   | 51 ++++++++++++
 .../concerns/dossier_rebase_concern_spec.rb   |  4 +-
 .../tags_substitution_concern_spec.rb         |  5 +-
 spec/models/dossier_spec.rb                   | 48 +++++------
 .../pieces_justificatives_service_spec.rb     | 12 +--
 .../procedure_export_service_zip_spec.rb      |  4 +-
 .../fix_missing_champs_task_spec.rb           | 19 -----
 24 files changed, 202 insertions(+), 275 deletions(-)
 delete mode 100644 app/lib/data_fixer/dossier_champs_missing.rb
 delete mode 100644 spec/lib/data_fixer/dossier_champs_missing_spec.rb
 delete mode 100644 spec/tasks/maintenance/fix_missing_champs_task_spec.rb

diff --git a/app/controllers/champs/repetition_controller.rb b/app/controllers/champs/repetition_controller.rb
index 1fd2db8dd..625d98602 100644
--- a/app/controllers/champs/repetition_controller.rb
+++ b/app/controllers/champs/repetition_controller.rb
@@ -2,14 +2,13 @@
 
 class Champs::RepetitionController < Champs::ChampController
   def add
-    row = @champ.add_row(@champ.dossier.revision)
-    @first_champ_id = row.map(&:focusable_input_id).compact.first
-    @row_id = row.first&.row_id
+    @row_id = @champ.add_row(updated_by: current_user.email)
+    @first_champ_id = @champ.focusable_input_id
     @row_number = @row_id.nil? ? 0 : @champ.row_ids.find_index(@row_id) + 1
   end
 
   def remove
-    @champ.remove_row(params[:row_id])
+    @champ.remove_row(params[:row_id], updated_by: current_user.email)
     @to_remove = "safe-row-selector-#{params[:row_id]}"
     @to_focus = @champ.focusable_input_id || helpers.dom_id(@champ, :create_repetition)
   end
diff --git a/app/graphql/mutations/dossier_modifier_annotation_ajouter_ligne.rb b/app/graphql/mutations/dossier_modifier_annotation_ajouter_ligne.rb
index bf12432f6..cf65b97b1 100644
--- a/app/graphql/mutations/dossier_modifier_annotation_ajouter_ligne.rb
+++ b/app/graphql/mutations/dossier_modifier_annotation_ajouter_ligne.rb
@@ -16,7 +16,7 @@ module Mutations
         return { errors: ["L’annotation \"#{annotation_id}\" n’existe pas"] }
       end
 
-      annotation.add_row(dossier.revision)
+      annotation.add_row(updated_by: instructeur.email)
 
       { annotation:, errors: nil }
     end
diff --git a/app/graphql/types/dossier_type.rb b/app/graphql/types/dossier_type.rb
index b30adf204..3236b3a4a 100644
--- a/app/graphql/types/dossier_type.rb
+++ b/app/graphql/types/dossier_type.rb
@@ -170,7 +170,7 @@ module Types
           .for(object, private: false)
           .load(ApplicationRecord.id_from_typed_id(id))
       else
-        object.champs_for_revision(scope: :public, root: true).filter(&:visible?)
+        object.project_champs_public.filter(&:visible?)
       end
     end
 
@@ -180,7 +180,7 @@ module Types
           .for(object, private: true)
           .load(ApplicationRecord.id_from_typed_id(id))
       else
-        object.champs_for_revision(scope: :private, root: true).filter(&:visible?)
+        object.project_champs_private.filter(&:visible?)
       end
     end
 
diff --git a/app/lib/data_fixer/dossier_champs_missing.rb b/app/lib/data_fixer/dossier_champs_missing.rb
deleted file mode 100644
index 0ea95a17a..000000000
--- a/app/lib/data_fixer/dossier_champs_missing.rb
+++ /dev/null
@@ -1,83 +0,0 @@
-# frozen_string_literal: true
-
-# some race condition (regarding double submit of dossier.passer_en_construction!) might remove champs
-#   until now we haven't decided to push a stronger fix than an UI change
-#   so we might have to recreate some deleted champs and notify administration
-class DataFixer::DossierChampsMissing
-  def fix
-    fixed_on_origin = apply_fix(@original_dossier)
-
-    fixed_on_other = Dossier.where(editing_fork_origin_id: @original_dossier.id)
-      .map(&method(:apply_fix))
-
-    [fixed_on_origin, fixed_on_other.sum].sum
-  end
-
-  private
-
-  attr_reader :original_dossier
-
-  def initialize(dossier:)
-    @original_dossier = dossier
-  end
-
-  def apply_fix(dossier)
-    added_champs_root = fix_champs_root(dossier)
-    added_champs_in_repetition = fix_champs_in_repetition(dossier)
-
-    added_champs = added_champs_root + added_champs_in_repetition
-    if !added_champs.empty?
-      dossier.save!
-      log_champs_added(dossier, added_champs)
-      added_champs.size
-    else
-      0
-    end
-  end
-
-  def fix_champs_root(dossier)
-    champs_root, _ = dossier.champs.partition { _1.parent_id.blank? }
-    expected_tdcs = dossier.revision.revision_types_de_champ.filter { _1.parent.blank? }.map(&:type_de_champ)
-
-    expected_tdcs.filter { !champs_root.map(&:stable_id).include?(_1.stable_id) }
-      .map do |missing_tdc|
-                    champ_root_missing = missing_tdc.build_champ
-
-                    dossier.champs_public << champ_root_missing
-                    champ_root_missing
-                  end
-  end
-
-  def fix_champs_in_repetition(dossier)
-    champs_repetition, _ = dossier.champs.partition(&:repetition?)
-
-    champs_repetition.flat_map do |champ_repetition|
-      champ_repetition_missing = champ_repetition.rows.flat_map do |row|
-        row_id = row.first.row_id
-        expected_tdcs = dossier.revision.children_of(champ_repetition.type_de_champ)
-        row_tdcs = row.map(&:type_de_champ)
-
-        (expected_tdcs - row_tdcs).map do |missing_tdc|
-          champ_repetition_missing = missing_tdc.build_champ(row_id: row_id)
-          champ_repetition.champs << champ_repetition_missing
-          champ_repetition_missing
-        end
-      end
-    end
-  end
-
-  def log_champs_added(dossier, added_champs)
-    app_traces = caller.reject { _1.match?(%r{/ruby/.+/gems/}) }.map { _1.sub(Rails.root.to_s, "") }
-
-    payload = {
-      message: "DataFixer::DossierChampsMissing",
-      dossier_id: dossier.id,
-      champs_ids: added_champs.map(&:id).join(","),
-      caller: app_traces
-    }
-
-    logger = Lograge.logger || Rails.logger
-
-    logger.info payload.to_json
-  end
-end
diff --git a/app/models/champs/repetition_champ.rb b/app/models/champs/repetition_champ.rb
index 0731dea3e..e289171dc 100644
--- a/app/models/champs/repetition_champ.rb
+++ b/app/models/champs/repetition_champ.rb
@@ -1,35 +1,26 @@
 # frozen_string_literal: true
 
 class Champs::RepetitionChamp < Champ
-  accepts_nested_attributes_for :champs
+  delegate :libelle_for_export, to: :type_de_champ
 
   def rows
-    dossier
-      .champs_for_revision(scope: type_de_champ)
-      .group_by(&:row_id)
-      .sort
-      .map(&:second)
+    dossier.project_rows_for(type_de_champ)
   end
 
   def row_ids
-    rows.map { _1.first.row_id }
+    dossier.repetition_row_ids(type_de_champ)
   end
 
-  def add_row(revision)
-    added_champs = []
-    transaction do
-      row_id = ULID.generate
-      revision.children_of(type_de_champ).each do |type_de_champ|
-        added_champs << type_de_champ.build_champ(row_id:)
-      end
-      self.champs << added_champs
-    end
-    added_champs
+  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
   end
 
-  def remove_row(row_id)
-    dossier.champs.where(row_id:).destroy_all
-    dossier.champs.reload
+  def remove_row(row_id, updated_by:)
+    dossier.repetition_remove_row(type_de_champ, row_id, updated_by:)
   end
 
   def focusable_input_id
diff --git a/app/models/concerns/dossier_champs_concern.rb b/app/models/concerns/dossier_champs_concern.rb
index 501fdda56..5612b2dc9 100644
--- a/app/models/concerns/dossier_champs_concern.rb
+++ b/app/models/concerns/dossier_champs_concern.rb
@@ -3,21 +3,10 @@
 module DossierChampsConcern
   extend ActiveSupport::Concern
 
-  def champs_for_revision(scope: nil, root: false)
+  def champs_for_revision(scope: nil)
     champs_index = champs.group_by(&:stable_id)
-      # Due to some bad data we can have multiple copies of the same champ. Ignore extra copy.
-      .transform_values { _1.sort_by(&:id).uniq(&:row_id) }
-
-    if scope.is_a?(TypeDeChamp)
-      revision
-        .children_of(scope)
-        .flat_map { champs_index[_1.stable_id] || [] }
-        .filter(&:child?) # TODO: remove once bad data (child champ without a row id) is cleaned
-    else
-      revision
-        .types_de_champ_for(scope:, root:)
-        .flat_map { champs_index[_1.stable_id] || [] }
-    end
+    revision.types_de_champ_for(scope:)
+      .flat_map { champs_index[_1.stable_id] || [] }
   end
 
   # Get all the champs values for the types de champ in the final list.
@@ -42,6 +31,25 @@ module DossierChampsConcern
     end
   end
 
+  def project_champs_public
+    revision.types_de_champ_public.map { project_champ(_1, nil) }
+  end
+
+  def project_champs_private
+    revision.types_de_champ_private.map { project_champ(_1, nil) }
+  end
+
+  def project_rows_for(type_de_champ)
+    [] if !type_de_champ.repetition?
+
+    children = revision.children_of(type_de_champ)
+    row_ids = repetition_row_ids(type_de_champ)
+
+    row_ids.map do |row_id|
+      children.map { project_champ(_1, row_id) }
+    end
+  end
+
   def find_type_de_champ_by_stable_id(stable_id, scope = nil)
     case scope
     when :public
@@ -75,6 +83,41 @@ module DossierChampsConcern
     assign_attributes(champs_attributes:)
   end
 
+  def repetition_row_ids(type_de_champ)
+    [] 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? }
+      .map(&:row_id)
+      .uniq
+      .sort
+  end
+
+  def repetition_add_row(type_de_champ, updated_by:)
+    raise "Can't add row to non-repetition type de champ" if !type_de_champ.repetition?
+
+    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:) }
+    @champs_by_public_id = nil
+    [row_id, added_champs]
+  end
+
+  def repetition_remove_row(type_de_champ, row_id, updated_by:)
+    raise "Can't remove row from non-repetition type de champ" if !type_de_champ.repetition?
+
+    champs.where(row_id:).destroy_all
+    champs.reload if persisted?
+    @champs_by_public_id = nil
+  end
+
+  def reload
+    super.tap do
+      @champs_by_public_id = nil
+    end
+  end
+
   private
 
   def champs_by_public_id
diff --git a/app/models/concerns/dossier_searchable_concern.rb b/app/models/concerns/dossier_searchable_concern.rb
index f7433337c..13f65a602 100644
--- a/app/models/concerns/dossier_searchable_concern.rb
+++ b/app/models/concerns/dossier_searchable_concern.rb
@@ -15,7 +15,7 @@ module DossierSearchableConcern
 
       search_terms = [
         user&.email,
-        *champs_public.flat_map(&:search_terms),
+        *project_champs_public.flat_map(&:search_terms),
         *etablissement&.search_terms,
         individual&.nom,
         individual&.prenom,
@@ -23,7 +23,7 @@ module DossierSearchableConcern
         mandataire_last_name
       ].compact_blank.join(' ')
 
-      private_search_terms = champs_private.flat_map(&:search_terms).compact_blank.join(' ')
+      private_search_terms = project_champs_private.flat_map(&:search_terms).compact_blank.join(' ')
 
       sql = "UPDATE dossiers SET search_terms = :search_terms, private_search_terms = :private_search_terms WHERE id = :id"
       sanitized_sql = self.class.sanitize_sql_array([sql, search_terms:, private_search_terms:, id:])
diff --git a/app/models/dossier.rb b/app/models/dossier.rb
index 689c20ac5..dc8d97740 100644
--- a/app/models/dossier.rb
+++ b/app/models/dossier.rb
@@ -479,10 +479,10 @@ class Dossier < ApplicationRecord
       champs_private << champ
     end
     champs_public.filter { _1.repetition? && _1.mandatory? }.each do |champ|
-      champ.add_row(revision)
+      champ.add_row(updated_by: nil)
     end
     champs_private.filter(&:repetition?).each do |champ|
-      champ.add_row(revision)
+      champ.add_row(updated_by: nil)
     end
   end
 
@@ -942,14 +942,21 @@ class Dossier < ApplicationRecord
   end
 
   def check_mandatory_and_visible_champs
-    champs_for_revision(scope: :public)
-      .filter { _1.child? ? _1.parent.visible? : true }
-      .filter(&:visible?)
-      .filter(&:mandatory_blank?)
-      .map do |champ|
-        champ.errors.add(:value, :missing)
+    project_champs_public.filter(&:visible?).each do |champ|
+      if champ.mandatory_blank?
+        error = champ.errors.add(:value, :missing)
+        errors.import(error)
       end
-      .each { errors.import(_1) }
+      if champ.repetition?
+        champ.rows.each do |champs|
+          champs.filter(&:visible?).filter(&:mandatory_blank?).each do |champ|
+            error = champ.errors.add(:value, :missing)
+            errors.import(error)
+          end
+        end
+      end
+    end
+    errors
   end
 
   def demander_un_avis!(avis)
diff --git a/app/models/dossier_preloader.rb b/app/models/dossier_preloader.rb
index 4eea05534..7b32e65f4 100644
--- a/app/models/dossier_preloader.rb
+++ b/app/models/dossier_preloader.rb
@@ -80,8 +80,8 @@ class DossierPreloader
       dossier.association(:revision).target = revision
     end
     dossier.association(:champs).target = champs
-    dossier.association(:champs_public).target = dossier.champs_for_revision(scope: :public, root: true)
-    dossier.association(:champs_private).target = dossier.champs_for_revision(scope: :private, root: true)
+    dossier.association(:champs_public).target = dossier.project_champs_public
+    dossier.association(:champs_private).target = dossier.project_champs_private
 
     # remove once parent_id is deprecated
     champs_by_parent_id = champs.group_by(&:parent_id)
diff --git a/app/models/procedure_revision.rb b/app/models/procedure_revision.rb
index 80d4b7b2c..6bd3fe08a 100644
--- a/app/models/procedure_revision.rb
+++ b/app/models/procedure_revision.rb
@@ -179,19 +179,14 @@ class ProcedureRevision < ApplicationRecord
     dossier
   end
 
-  def types_de_champ_for(scope: nil, root: false)
-    # We return an unordered collection
-    return types_de_champ if !root && scope.nil?
-    return types_de_champ.filter { scope == :public ? _1.public? : _1.private? } if !root
-
-    # We return an ordered collection
+  def types_de_champ_for(scope: nil)
     case scope
     when :public
-      types_de_champ_public
+      types_de_champ.filter(&:public?)
     when :private
-      types_de_champ_private
+      types_de_champ.filter(&:private?)
     else
-      types_de_champ_public + types_de_champ_private
+      types_de_champ
     end
   end
 
diff --git a/app/models/types_de_champ/prefill_repetition_type_de_champ.rb b/app/models/types_de_champ/prefill_repetition_type_de_champ.rb
index 1696c6b7c..5af53c178 100644
--- a/app/models/types_de_champ/prefill_repetition_type_de_champ.rb
+++ b/app/models/types_de_champ/prefill_repetition_type_de_champ.rb
@@ -56,8 +56,7 @@ class TypesDeChamp::PrefillRepetitionTypeDeChamp < TypesDeChamp::PrefillTypeDeCh
     def to_assignable_attributes
       return unless repetition.is_a?(Hash)
 
-      row = champ.rows[index] || champ.add_row(revision)
-      row_id = row.first.row_id
+      row_id = champ.row_ids[index] || champ.add_row(updated_by: nil)
 
       repetition.map do |key, value|
         next unless key.is_a?(String) && key.starts_with?("champ_")
diff --git a/app/serializers/champ_serializer.rb b/app/serializers/champ_serializer.rb
index bd8c43271..5a555cd27 100644
--- a/app/serializers/champ_serializer.rb
+++ b/app/serializers/champ_serializer.rb
@@ -48,11 +48,7 @@ class ChampSerializer < ActiveModel::Serializer
   end
 
   def rows
-    object.dossier
-      .champs_for_revision(scope: object.type_de_champ)
-      .group_by(&:row_id)
-      .values
-      .map.with_index(1) { |champs, index| Row.new(index:, champs:) }
+    object.rows.map.with_index(1) { |champs, index| Row.new(index:, champs:) }
   end
 
   def include_etablissement?
diff --git a/app/serializers/dossier_serializer.rb b/app/serializers/dossier_serializer.rb
index 804a10c89..474a79131 100644
--- a/app/serializers/dossier_serializer.rb
+++ b/app/serializers/dossier_serializer.rb
@@ -32,7 +32,7 @@ class DossierSerializer < ActiveModel::Serializer
   has_many :champs, serializer: ChampSerializer
 
   def champs
-    champs = object.champs_public.reject { |c| c.type_de_champ.old_pj.present? }
+    champs = object.project_champs_public.reject { |c| c.type_de_champ.old_pj.present? }
 
     if object.expose_legacy_carto_api?
       champ_carte = champs.find do |champ|
@@ -52,12 +52,16 @@ class DossierSerializer < ActiveModel::Serializer
     champs
   end
 
+  def champs_private
+    object.project_champs_private
+  end
+
   def cerfa
     []
   end
 
   def pieces_justificatives
-    object.champs_public.filter { |champ| champ.type_de_champ.old_pj }.map do |champ|
+    object.project_champs_public.filter { |champ| champ.type_de_champ.old_pj }.map do |champ|
       {
         created_at: champ.created_at&.in_time_zone('UTC'),
         type_de_piece_justificative_id: champ.type_de_champ.old_pj[:stable_id],
diff --git a/spec/graphql/annotation_spec.rb b/spec/graphql/annotation_spec.rb
index cbec007d7..cd3d731f1 100644
--- a/spec/graphql/annotation_spec.rb
+++ b/spec/graphql/annotation_spec.rb
@@ -5,7 +5,7 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
   let(:procedure) { create(:procedure, :published, :for_individual, types_de_champ_private: [{ type: :repetition, children: [{ libelle: 'Nom' }, { type: :integer_number, libelle: 'Age' }] }, {}], administrateurs: [admin]) }
   let(:dossiers) { [] }
   let(:instructeur) { create(:instructeur, followed_dossiers: dossiers) }
-  let(:champs_private) { dossier.champs_for_revision(scope: :private, root: true) }
+  let(:champs_private) { dossier.project_champs_private }
 
   let(:query) { '' }
   let(:context) { { administrateur_id: admin.id, procedure_ids: admin.procedure_ids, write_access: true } }
diff --git a/spec/lib/data_fixer/dossier_champs_missing_spec.rb b/spec/lib/data_fixer/dossier_champs_missing_spec.rb
deleted file mode 100644
index 8936147eb..000000000
--- a/spec/lib/data_fixer/dossier_champs_missing_spec.rb
+++ /dev/null
@@ -1,58 +0,0 @@
-# frozen_string_literal: true
-
-describe DataFixer::DossierChampsMissing do
-  describe '#fix' do
-    let(:procedure) { create(:procedure, types_de_champ_public: [{ type: :datetime }, { type: :dossier_link }]) }
-    let(:dossier) { create(:dossier, procedure:) }
-
-    context 'when dossier does not have a fork' do
-      before { dossier.champs_public.first.destroy }
-      subject { described_class.new(dossier:).fix }
-
-      it 'add missing champs to the dossier' do
-        expect { subject }.to change { dossier.champs_public.count }.from(1).to(2)
-      end
-
-      it 'returns number of added champs' do
-        expect(subject).to eq(1)
-      end
-    end
-
-    context 'when dossier have a fork' do
-      before { dossier.champs_public.first.destroy }
-      let(:create_fork) { dossier.find_or_create_editing_fork(dossier.user) }
-      subject do
-        create_fork
-        described_class.new(dossier:).fix
-      end
-
-      it 'add missing champs to the fork too' do
-        expect { subject }.to change { create_fork.champs_public.count }.from(1).to(2)
-      end
-
-      it 'sums number of added champs for dossier and editing_fork_origin_id' do
-        expect(subject).to eq(2)
-      end
-    end
-
-    context 'when dossier have missing champ on repetition' do
-      let(:procedure) { create(:procedure, types_de_champ_public: [{ type: :repetition, children: [{ type: :text }, { type: :decimal_number }] }]) }
-      let(:dossier) { create(:dossier, :with_populated_champs, procedure:) }
-      let(:champ_repetition) { dossier.champs_public.first }
-      let(:initial_champ_count) { dossier.champs.count }
-      before do
-        initial_champ_count
-        champ_repetition.champs.first.destroy
-      end
-      subject { described_class.new(dossier:).fix }
-
-      it 'add missing champs to repetition' do
-        expect { subject }.to change { dossier.champs.count }.from(initial_champ_count - 1).to(initial_champ_count)
-      end
-
-      it 'counts number of added champs for dossier.repetitions' do
-        expect(subject).to eq(1)
-      end
-    end
-  end
-end
diff --git a/spec/lib/recovery/dossier_life_cycle_spec.rb b/spec/lib/recovery/dossier_life_cycle_spec.rb
index 79c9bc311..863e3e8fe 100644
--- a/spec/lib/recovery/dossier_life_cycle_spec.rb
+++ b/spec/lib/recovery/dossier_life_cycle_spec.rb
@@ -17,7 +17,7 @@ describe 'Dossier::Recovery::LifeCycle' do
     let(:dossier) do
       d = create(:dossier, procedure:)
 
-      repetition(d).add_row(d.revision)
+      repetition(d).add_row(updated_by: 'test')
       pj_champ(d).piece_justificative_file.attach(some_file)
       carte(d).update(geo_areas: [geo_area])
       d.etablissement = create(:etablissement, :with_exercices)
diff --git a/spec/models/champ_presentations/repetition_presentation_spec.rb b/spec/models/champ_presentations/repetition_presentation_spec.rb
index 37c7fec5a..3c1e1696e 100644
--- a/spec/models/champ_presentations/repetition_presentation_spec.rb
+++ b/spec/models/champ_presentations/repetition_presentation_spec.rb
@@ -15,16 +15,21 @@ describe ChampPresentations::RepetitionPresentation do
   }
 
   let(:dossier) { create(:dossier, procedure:) }
+  let(:champ_repetition) { dossier.champs.find(&:repetition?) }
 
   before do
-    nom, stars = dossier.champs[0].rows.first
+    champ_repetition.add_row(updated_by: 'test')
+    champ_repetition.add_row(updated_by: 'test')
+    row1, row2, row3 = champ_repetition.rows
+
+    nom, stars = row1
     nom.update(value: "ruby")
     stars.update(value: 5)
 
-    nom, stars = dossier.champs[0].add_row(dossier.procedure.active_revision)
+    nom = row2.first
     nom.update(value: "js")
 
-    nom, stars = dossier.champs[0].add_row(dossier.procedure.active_revision)
+    nom, stars = row3
     nom.update(value: "rust")
     stars.update(value: 4)
   end
diff --git a/spec/models/concerns/dossier_champs_concern_spec.rb b/spec/models/concerns/dossier_champs_concern_spec.rb
index 0d88a4243..a4168faab 100644
--- a/spec/models/concerns/dossier_champs_concern_spec.rb
+++ b/spec/models/concerns/dossier_champs_concern_spec.rb
@@ -104,6 +104,57 @@ RSpec.describe DossierChampsConcern do
     end
   end
 
+  describe '#project_champs_public' do
+    subject { dossier.project_champs_public }
+
+    it { expect(subject.size).to eq(4) }
+  end
+
+  describe '#project_champs_private' do
+    subject { dossier.project_champs_private }
+
+    it { expect(subject.size).to eq(1) }
+  end
+
+  describe '#repetition_row_ids' do
+    let(:type_de_champ_repetition) { dossier.find_type_de_champ_by_stable_id(993) }
+    subject { dossier.repetition_row_ids(type_de_champ_repetition) }
+
+    it { expect(subject.size).to eq(1) }
+  end
+
+  describe '#project_rows_for' do
+    let(:type_de_champ_repetition) { dossier.find_type_de_champ_by_stable_id(993) }
+    subject { dossier.project_rows_for(type_de_champ_repetition) }
+
+    it { expect(subject.size).to eq(1) }
+    it { expect(subject.first.size).to eq(1) }
+  end
+
+  describe '#repetition_add_row' do
+    let(:type_de_champ_repetition) { dossier.find_type_de_champ_by_stable_id(993) }
+    let(:row_ids) { dossier.repetition_row_ids(type_de_champ_repetition) }
+    subject do
+      # TODO: clean this up when parent_id is deprecated
+      row_id, added_champs = dossier.repetition_add_row(type_de_champ_repetition, updated_by: 'test')
+      dossier.champs << added_champs
+      row_id
+    end
+
+    it { expect { subject }.to change { dossier.repetition_row_ids(type_de_champ_repetition).size }.by(1) }
+    it { expect(subject).to be_in(row_ids) }
+  end
+
+  describe '#repetition_remove_row' do
+    let(:type_de_champ_repetition) { dossier.find_type_de_champ_by_stable_id(993) }
+    let(:row_id) { dossier.repetition_row_ids(type_de_champ_repetition).first }
+    let(:row_ids) { dossier.repetition_row_ids(type_de_champ_repetition) }
+    subject { dossier.repetition_remove_row(type_de_champ_repetition, row_id, updated_by: 'test') }
+
+    it { expect { subject }.to change { dossier.repetition_row_ids(type_de_champ_repetition).size }.by(-1) }
+    it { row_id; subject; expect(row_id).not_to be_in(row_ids) }
+  end
+
   describe "#champs_for_export" do
     subject { dossier.champs_for_export(dossier.revision.types_de_champ_public) }
 
diff --git a/spec/models/concerns/dossier_rebase_concern_spec.rb b/spec/models/concerns/dossier_rebase_concern_spec.rb
index 3a1ff0e1b..50df35b9d 100644
--- a/spec/models/concerns/dossier_rebase_concern_spec.rb
+++ b/spec/models/concerns/dossier_rebase_concern_spec.rb
@@ -346,8 +346,8 @@ describe DossierRebaseConcern do
         datetime_champ.update(value: Time.zone.now.to_s)
         text_champ.update(value: 'bonjour')
         # Add two rows then remove previous to last row in order to create a "hole" in the sequence
-        repetition_champ.add_row(repetition_champ.dossier.revision)
-        repetition_champ.add_row(repetition_champ.dossier.revision)
+        repetition_champ.add_row(updated_by: 'test')
+        repetition_champ.add_row(updated_by: 'test')
         repetition_champ.champs.where(row_id: repetition_champ.rows[-2].first.row_id).destroy_all
         repetition_champ.reload
       end
diff --git a/spec/models/concerns/tags_substitution_concern_spec.rb b/spec/models/concerns/tags_substitution_concern_spec.rb
index 742532c97..42b0d30a0 100644
--- a/spec/models/concerns/tags_substitution_concern_spec.rb
+++ b/spec/models/concerns/tags_substitution_concern_spec.rb
@@ -233,9 +233,8 @@ describe TagsSubstitutionConcern, type: :model do
       let(:dossier) { create(:dossier, procedure:) }
 
       before do
-        repetition = dossier.champs_public
-          .find { |champ| champ.libelle == 'Répétition' }
-        repetition.add_row(dossier.revision)
+        repetition = dossier.project_champs_public.find(&:repetition?)
+        repetition.add_row(updated_by: 'test')
         paul_champs, pierre_champs = repetition.rows
 
         paul_champs.first.update(value: 'Paul')
diff --git a/spec/models/dossier_spec.rb b/spec/models/dossier_spec.rb
index 41f21b271..26b623346 100644
--- a/spec/models/dossier_spec.rb
+++ b/spec/models/dossier_spec.rb
@@ -515,7 +515,7 @@ describe Dossier, type: :model do
 
           context 'when piece_justificative' do
             let(:types_de_champ_public) { [{ type: :piece_justificative }] }
-            let(:champ) { dossier.champs_for_revision(scope: :public).find(&:piece_justificative?) }
+            let(:champ) { dossier.project_champs_public.find(&:piece_justificative?) }
 
             context 'when not visible' do
               let(:visible) { false }
@@ -530,7 +530,7 @@ describe Dossier, type: :model do
 
           context 'when titre identite' do
             let(:types_de_champ_public) { [{ type: :titre_identite }] }
-            let(:champ) { dossier.champs_for_revision(scope: :public).find(&:piece_justificative?) }
+            let(:champ) { dossier.project_champs_public.find(&:piece_justificative?) }
 
             context 'when not visible' do
               let(:visible) { false }
@@ -728,10 +728,10 @@ describe Dossier, type: :model do
   end
 
   describe "#unspecified_attestation_champs" do
-    let(:procedure) { create(:procedure, attestation_template: attestation_template, types_de_champ_public: types_de_champ, types_de_champ_private: types_de_champ_private) }
-    let(:dossier) { create(:dossier, :en_instruction, procedure: procedure) }
+    let(:procedure) { create(:procedure, attestation_template:, types_de_champ_public:, types_de_champ_private:) }
+    let(:dossier) { create(:dossier, :en_instruction, procedure:) }
 
-    let(:types_de_champ) { [tdc_1, tdc_2, tdc_3, tdc_4] }
+    let(:types_de_champ_public) { [tdc_1, tdc_2, tdc_3, tdc_4] }
     let(:types_de_champ_private) { [tdc_5, tdc_6, tdc_7, tdc_8] }
 
     let(:tdc_1) { { libelle: "specified champ-in-title" } }
@@ -744,7 +744,7 @@ describe Dossier, type: :model do
     let(:tdc_8) { { libelle: "unspecified annotation privée-in-body" } }
 
     before do
-      (dossier.champs_public + dossier.champs_private)
+      (dossier.project_champs_public + dossier.project_champs_private)
         .filter { |c| c.libelle.match?(/^specified/) }
         .each { |c| c.update_attribute(:value, "specified") }
     end
@@ -799,7 +799,7 @@ describe Dossier, type: :model do
       let(:attestation_template) { build(:attestation_template, :v2) }
 
       before do
-        tdc_content = (types_de_champ + types_de_champ_private).filter_map do |tdc_config|
+        tdc_content = (types_de_champ_public + types_de_champ_private).filter_map do |tdc_config|
           next if tdc_config[:libelle].include?("in-title")
 
           {
@@ -1607,7 +1607,7 @@ describe Dossier, type: :model do
 
     context "with mandatory champs" do
       let(:type_de_champ) { { mandatory: true } }
-      let(:champ_with_error) { dossier.champs_public.first }
+      let(:champ_with_error) { dossier.champs.first }
 
       before do
         champ_with_error.value = nil
@@ -1616,7 +1616,7 @@ describe Dossier, type: :model do
 
       it 'should have errors' do
         expect(errors).not_to be_empty
-        expect(errors.first.full_message).to eq("doit être rempli")
+        expect(errors.first.full_message).to eq("Le champ « Value » doit être rempli")
       end
 
       context "conditionaly visible" do
@@ -1649,7 +1649,7 @@ describe Dossier, type: :model do
 
         it 'should have errors' do
           expect(errors).not_to be_empty
-          expect(errors.first.full_message).to eq("doit être rempli")
+          expect(errors.first.full_message).to eq("Le champ « Value » doit être rempli")
         end
       end
     end
@@ -1660,39 +1660,37 @@ describe Dossier, type: :model do
       let(:type_de_champ_repetition) { revision.types_de_champ.first }
 
       context "when no champs" do
-        let(:champ_with_error) { dossier.champs_public.first }
-
         it 'should have errors' do
-          dossier.champs_public.first.champs.destroy_all
-          expect(dossier.champs_public.first.rows).to be_empty
+          dossier.champs.first.row_ids.each do |row_id|
+            dossier.repetition_remove_row(type_de_champ_repetition, row_id, updated_by: 'test')
+          end
+          expect(dossier.champs.first.rows).to be_empty
           expect(errors).not_to be_empty
-          expect(errors.first.full_message).to eq("doit être rempli")
+          expect(errors.first.full_message).to eq("Le champ « Value » doit être rempli")
         end
       end
 
       context "when mandatory champ inside repetition" do
-        let(:champ_with_error) { dossier.champs_public.first.champs.first }
-
         it 'should have errors' do
-          expect(dossier.champs_public.first.rows).not_to be_empty
-          expect(errors.first.full_message).to eq("doit être rempli")
+          expect(dossier.champs.first.rows).not_to be_empty
+          expect(errors).not_to be_empty
+          expect(errors.first.full_message).to eq("Le champ « Value » doit être rempli")
         end
 
         context "conditionaly visible" do
-          let(:champ_with_error) { dossier.champs_public.second.champs.first }
           let(:types_de_champ) { [{ type: :yes_no, stable_id: 99, mandatory: false }, type_de_champ] }
           let(:type_de_champ) { { type: :repetition, mandatory: true, children: [{ mandatory: true }], condition: ds_eq(champ_value(99), constant(true)) } }
 
           it 'should not have errors' do
-            expect(dossier.champs_public.second.rows).not_to be_empty
+            expect(dossier.champs.second.rows).not_to be_empty
             expect(errors).to be_empty
           end
 
           it 'should have errors' do
-            dossier.champs_public.first.update(value: 'true')
-            expect(dossier.champs_public.second.rows).not_to be_empty
+            dossier.champs.first.update(value: 'true')
+            expect(dossier.champs.second.rows).not_to be_empty
             expect(errors).not_to be_empty
-            expect(errors.first.full_message).to eq("doit être rempli")
+            expect(errors.first.full_message).to eq("Le champ « Value » doit être rempli")
           end
         end
       end
@@ -2040,7 +2038,7 @@ describe Dossier, type: :model do
           procedure.publish!
           dossier
           procedure.draft_revision.remove_type_de_champ(text_type_de_champ.stable_id)
-          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)
+          coordinate = procedure.draft_revision.add_type_de_champ(type_champ: TypeDeChamp.type_champs.fetch(:text), libelle: 'New text field', after_stable_id: repetition_type_de_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')
diff --git a/spec/services/pieces_justificatives_service_spec.rb b/spec/services/pieces_justificatives_service_spec.rb
index 0e58f6c3f..61661ece2 100644
--- a/spec/services/pieces_justificatives_service_spec.rb
+++ b/spec/services/pieces_justificatives_service_spec.rb
@@ -56,11 +56,11 @@ describe PiecesJustificativesService do
       let(:second_champ) { repetition(dossier).champs.second }
 
       before do
-        repetition(dossier).add_row(dossier.revision)
+        repetition(dossier).add_row(updated_by: 'test')
         attach_file_to_champ(first_champ)
         attach_file_to_champ(first_champ)
 
-        repetition(dossier).add_row(dossier.revision)
+        repetition(dossier).add_row(updated_by: 'test')
         attach_file_to_champ(second_champ)
       end
 
@@ -527,11 +527,11 @@ describe PiecesJustificativesService do
       repet_0 = repetition(dossier_1, index: 0)
       repet_1 = repetition(dossier_1, index: 1)
 
-      repet_0.add_row(dossier_1.revision)
-      repet_0.add_row(dossier_1.revision)
+      repet_0.add_row(updated_by: 'test')
+      repet_0.add_row(updated_by: 'test')
 
-      repet_1.add_row(dossier_1.revision)
-      repet_1.add_row(dossier_1.revision)
+      repet_1.add_row(updated_by: 'test')
+      repet_1.add_row(updated_by: 'test')
     end
 
     it do
diff --git a/spec/services/procedure_export_service_zip_spec.rb b/spec/services/procedure_export_service_zip_spec.rb
index c872eb679..696a62067 100644
--- a/spec/services/procedure_export_service_zip_spec.rb
+++ b/spec/services/procedure_export_service_zip_spec.rb
@@ -15,11 +15,11 @@ describe ProcedureExportService do
     dossiers.each do |dossier|
       attach_file_to_champ(pj_champ(dossier))
 
-      repetition(dossier).add_row(dossier.revision)
+      repetition(dossier).add_row(updated_by: 'test')
       attach_file_to_champ(repetition(dossier).champs.first)
       attach_file_to_champ(repetition(dossier).champs.first)
 
-      repetition(dossier).add_row(dossier.revision)
+      repetition(dossier).add_row(updated_by: 'test')
       attach_file_to_champ(repetition(dossier).champs.second)
     end
 
diff --git a/spec/tasks/maintenance/fix_missing_champs_task_spec.rb b/spec/tasks/maintenance/fix_missing_champs_task_spec.rb
deleted file mode 100644
index aad7f753f..000000000
--- a/spec/tasks/maintenance/fix_missing_champs_task_spec.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-require "rails_helper"
-
-module Maintenance
-  RSpec.describe FixMissingChampsTask do
-    describe "#process" do
-      subject(:process) { described_class.process(dossiers) }
-      let(:procedure) { create(:procedure, types_de_champ_public:) }
-      let(:types_de_champ_public) { [{ type: :text, libelle: 'l1' }, { type: :text, libelle: 'l2' }] }
-      let(:dossier_1) { create(:dossier, procedure:) }
-      let(:dossiers) { [dossier_1] }
-      it "add missing champs" do
-        dossier_1.champs.last.destroy
-        expect { subject }.to change { dossier_1.champs.count }.by(1)
-      end
-    end
-  end
-end

From a8e605f4b37cea0294c974b74376a3d9d6e06ea4 Mon Sep 17 00:00:00 2001
From: Paul Chavard <pro@paul.chavard.net>
Date: Fri, 27 Sep 2024 11:35:29 +0200
Subject: [PATCH 2/2] chore: clean some dead code

---
 .../maintenance/fix_missing_champs_task.rb    | 23 -------------------
 lib/tasks/data_fixer.rake                     | 15 ------------
 2 files changed, 38 deletions(-)
 delete mode 100644 app/tasks/maintenance/fix_missing_champs_task.rb

diff --git a/app/tasks/maintenance/fix_missing_champs_task.rb b/app/tasks/maintenance/fix_missing_champs_task.rb
deleted file mode 100644
index c7c92eb1a..000000000
--- a/app/tasks/maintenance/fix_missing_champs_task.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-# frozen_string_literal: true
-
-# bundle exec maintenance_tasks perform Maintenance::FixMissingChampsTask --arguments procedure_ids:id1,id2,id3
-module Maintenance
-  class FixMissingChampsTask < MaintenanceTasks::Task
-    attribute :procedure_ids, array: true, default: []
-
-    def collection
-      Dossier.joins(:procedure).where(procedure: { id: procedure_ids }).in_batches
-    end
-
-    def process(dossiers)
-      # rubocop:disable Rails/FindEach
-      DossierPreloader.new(dossiers).all.each do |dossier|
-        # rubocop:enable Rails/FindEach
-        maybe_fixable = [dossier, dossier.editing_forks.first].compact.any? { _1.champs.size < _1.revision.types_de_champ.size }
-        if maybe_fixable
-          DataFixer::DossierChampsMissing.new(dossier:).fix
-        end
-      end
-    end
-  end
-end
diff --git a/lib/tasks/data_fixer.rake b/lib/tasks/data_fixer.rake
index f4419bc8c..46f3dd821 100644
--- a/lib/tasks/data_fixer.rake
+++ b/lib/tasks/data_fixer.rake
@@ -28,19 +28,4 @@ namespace :data_fixer do
       end
     end
   end
-
-  desc <<~EOD
-    Given a dossier_id in argument, run the DossierChampsMissing.
-    ex: rails data_fixer:dossier_missing_champ\[1\]
-  EOD
-  task :dossier_missing_champ, [:dossier_id] => :environment do |_t, args|
-    dossier = Dossier.find(args[:dossier_id])
-    result = DataFixer::DossierChampsMissing.new(dossier:).fix
-
-    if result > 0
-      rake_puts "Dossier#[#{args[:dossier_id]}] fixed"
-    else
-      rake_puts "Dossier#[#{args[:dossier_id]}] not fixed"
-    end
-  end
 end