Merge pull request #10123 from tchak/fix-multiple-champ-copies-bug

fix(champ): fix multiple champ copies bug

app/models/champs/repetition_champ.rb

@@ -41,17 +41,19 @@ class Champs::RepetitionChamp < Champ
   end
 
   def rows_for_export
-    champs = dossier.champs_by_stable_id_with_row
     row_ids.map.with_index(1) do |row_id, index|
-      Champs::RepetitionChamp::Row.new(index: index, row_id:, dossier_id: dossier_id.to_s, champs:)
+      Champs::RepetitionChamp::Row.new(index:, row_id:, dossier:)
     end
   end
 
   class Row < Hashie::Dash
     property :index
     property :row_id
-    property :dossier_id
+    property :dossier
-    property :champs
+
+    def dossier_id
+      dossier.id.to_s
+    end
 
     def read_attribute_for_serialization(attribute)
       self[attribute]
@@ -61,7 +63,7 @@ class Champs::RepetitionChamp < Champ
       [
         ['Dossier ID', :dossier_id],
         ['Ligne', :index]
-      ] + Dossier.champs_for_export(types_de_champ, champs, row_id)
+      ] + dossier.champs_for_export(types_de_champ, row_id)
     end
   end
 end

app/models/dossier.rb

@@ -1270,29 +1270,20 @@ class Dossier < ApplicationRecord
     if procedure.routing_enabled?
       columns << ['Groupe instructeur', groupe_instructeur.label]
     end
-    columns + self.class.champs_for_export(types_de_champ, champs_by_stable_id_with_row)
+    columns + champs_for_export(types_de_champ)
   end
 
   # Get all the champs values for the types de champ in the final list.
   # Dossier might not have corresponding champ – display nil.
   # To do so, we build a virtual champ when there is no value so we can call for_export with all indexes
-  def self.champs_for_export(types_de_champ, champs, row_id = nil)
+  def champs_for_export(types_de_champ, row_id = nil)
     types_de_champ.flat_map do |type_de_champ|
-      champ = champs[[row_id, type_de_champ.stable_id].compact]
+      champ = champ_for_export(type_de_champ, row_id)
-
-      exported_values = if champ.nil? || !champ.visible?
-        # some champs export multiple columns
-        # ex: commune.for_export => [commune, insee, departement]
-        # so we build a fake champ to have the right export
-        type_de_champ.champ.build.for_export
-      else
-        champ.for_export
-      end
 
       # nil => [nil]
       # text => [text]
       # [commune, insee, departement] => [commune, insee, departement]
-      wrapped_exported_values = [exported_values].flatten
+      wrapped_exported_values = [champ.for_export].flatten
 
       wrapped_exported_values.map.with_index do |champ_value, index|
         [type_de_champ.libelle_for_export(index), champ_value]
@@ -1393,12 +1384,10 @@ class Dossier < ApplicationRecord
     user.france_connect_information.present?
   end
 
-  def champs_by_stable_id_with_row
-    champs_for_revision.index_by(&:stable_id_with_row)
-  end
-
   def champs_for_revision(scope: nil, root: false)
     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)
@@ -1413,7 +1402,7 @@ class Dossier < ApplicationRecord
 
   def project_champ(type_de_champ, row_id)
     stable_id_with_row = [row_id, type_de_champ.stable_id].compact
-    champ = champs.find { _1.stable_id_with_row == stable_id_with_row }
+    champ = champs_by_stable_id_with_row[stable_id_with_row]
     if champ.nil?
       type_de_champ.build_champ(dossier: self, row_id:)
     else
@@ -1421,8 +1410,25 @@ class Dossier < ApplicationRecord
     end
   end
 
+  def champ_for_export(type_de_champ, row_id)
+    stable_id_with_row = [row_id, type_de_champ.stable_id].compact
+    champ = champs_by_stable_id_with_row[stable_id_with_row]
+    if champ.nil? || !champ.visible?
+      # some champs export multiple columns
+      # ex: commune.for_export => [commune, insee, departement]
+      # so we build a fake champ to have the right export
+      type_de_champ.build_champ(dossier: self, row_id:)
+    else
+      champ
+    end
+  end
+
   private
 
+  def champs_by_stable_id_with_row
+    @champs_by_stable_id_with_row ||= champs.sort_by(&:id).index_by(&:stable_id_with_row)
+  end
+
   def create_missing_traitemets
     if en_construction_at.present? && traitements.en_construction.empty?
       self.traitements.passer_en_construction(processed_at: en_construction_at)

spec/graphql/annotation_spec.rb

@@ -3,6 +3,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(:query) { '' }
   let(:context) { { administrateur_id: admin.id, procedure_ids: admin.procedure_ids, write_access: true } }
@@ -21,7 +22,7 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
     let(:dossier) { create(:dossier, :en_construction, :with_populated_annotations, procedure: procedure) }
     let(:dossiers) { [dossier] }
 
-    let(:annotation) { dossier.champs_private.find(&:repetition?) }
+    let(:annotation) { champs_private.find(&:repetition?) }
     let(:query) { DOSSIER_MODIFIER_ANNOTATION_AJOUTER_LIGNE_MUTATION }
     let(:variables) do
       {
@@ -34,7 +35,7 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
     end
 
     context 'with invalid champ' do
-      let(:annotation) { dossier.champs_private.last }
+      let(:annotation) { champs_private.last }
 
       it 'return error' do
         expect(data).to eq(dossierModifierAnnotationAjouterLigne: {
@@ -60,7 +61,7 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
     let(:dossier) { create(:dossier, :en_construction, :with_populated_annotations, procedure: procedure) }
     let(:dossiers) { [dossier] }
 
-    let(:annotation) { dossier.champs_private.last }
+    let(:annotation) { champs_private.last }
     let(:query) { DOSSIER_MODIFIER_ANNOTATION_TEXT_MUTATION }
     let(:variables) do
       {
@@ -84,7 +85,7 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
     end
 
     context 'with invalid champ' do
-      let(:annotation) { dossier.champs_private.find(&:repetition?) }
+      let(:annotation) { champs_private.find(&:repetition?) }
 
       it 'return error' do
         expect(data).to eq(dossierModifierAnnotationText: {
@@ -95,8 +96,8 @@ RSpec.describe Mutations::DossierModifierAnnotation, type: :graphql do
     end
 
     context 'with rows' do
-      let(:annotation) { dossier.champs_private.find(&:repetition?).rows.first.first }
+      let(:annotation) { champs_private.find(&:repetition?).rows.first.first }
-      let(:other_annotation) { dossier.champs_private.find(&:repetition?).rows.second.first }
+      let(:other_annotation) { champs_private.find(&:repetition?).rows.second.first }
 
       it 'update champ' do
         expect(data).to eq(dossierModifierAnnotationText: {

spec/models/dossier_spec.rb

@@ -1897,9 +1897,9 @@ describe Dossier, type: :model do
       let(:repetition_second_revision_champ) { dossier_second_revision.champs_public.find(&:repetition?) }
       let(:dossier) { create(:dossier, procedure: procedure) }
       let(:dossier_second_revision) { create(:dossier, procedure: procedure) }
-      let(:dossier_champs_for_export) { Dossier.champs_for_export(procedure.types_de_champ_for_procedure_presentation.not_repetition, dossier.champs_by_stable_id_with_row) }
+      let(:dossier_champs_for_export) { dossier.champs_for_export(procedure.types_de_champ_for_procedure_presentation.not_repetition) }
-      let(:dossier_second_revision_champs_for_export) { Dossier.champs_for_export(procedure.types_de_champ_for_procedure_presentation.not_repetition, dossier_second_revision.champs_by_stable_id_with_row) }
+      let(:dossier_second_revision_champs_for_export) { dossier_second_revision.champs_for_export(procedure.types_de_champ_for_procedure_presentation.not_repetition) }
-      let(:repetition_second_revision_champs_for_export) { Dossier.champs_for_export(procedure.types_de_champ_for_procedure_presentation.repetition, dossier.champs_by_stable_id_with_row) }
+      let(:repetition_second_revision_champs_for_export) { dossier.champs_for_export(procedure.types_de_champ_for_procedure_presentation.repetition) }
 
       context "when procedure published" do
         before do
@@ -1937,7 +1937,7 @@ describe Dossier, type: :model do
             repetition = proc_test.types_de_champ_for_procedure_presentation.repetition.first
             type_champs = proc_test.types_de_champ_for_procedure_presentation(repetition).to_a
             expect(type_champs.size).to eq(1)
-            expect(Dossier.champs_for_export(type_champs, dossier.champs_by_stable_id_with_row).size).to eq(3)
+            expect(dossier.champs_for_export(type_champs).size).to eq(3)
           end
         end
       end
@@ -1960,7 +1960,7 @@ describe Dossier, type: :model do
       let(:text_tdc) { procedure.active_revision.types_de_champ_public.second }
       let(:tdcs) { dossier.champs_public.map(&:type_de_champ) }
 
-      subject { Dossier.champs_for_export(tdcs, dossier.champs_by_stable_id_with_row) }
+      subject { dossier.champs_for_export(tdcs) }
 
       before do
         text_tdc.update(condition: ds_eq(champ_value(yes_no_tdc.stable_id), constant(true)))

spec/views/shared/dossiers/_edit.html.haml_spec.rb

@@ -10,17 +10,15 @@ describe 'shared/dossiers/edit', type: :view do
   let(:dossier) { create(:dossier, :with_populated_champs, procedure:) }
 
   context 'when there are some champs' do
-    let(:champs_by_stable_id_with_row) { dossier.champs_by_stable_id_with_row }
-
     let(:type_de_champ_header_section) { procedure.draft_types_de_champ_public.find(&:header_section?) }
     let(:type_de_champ_explication) { procedure.draft_types_de_champ_public.find(&:explication?) }
     let(:type_de_champ_dossier_link) { procedure.draft_types_de_champ_public.find(&:dossier_link?) }
     let(:type_de_champ_checkbox) { procedure.draft_types_de_champ_public.find(&:checkbox?) }
     let(:type_de_champ_textarea) { procedure.draft_types_de_champ_public.find(&:textarea?) }
 
-    let(:champ_checkbox) { champs_by_stable_id_with_row[[type_de_champ_checkbox.stable_id]] }
+    let(:champ_checkbox) { dossier.project_champ(type_de_champ_checkbox, nil) }
-    let(:champ_dossier_link) { champs_by_stable_id_with_row[[type_de_champ_dossier_link.stable_id]] }
+    let(:champ_dossier_link) { dossier.project_champ(type_de_champ_dossier_link, nil) }
-    let(:champ_textarea) { champs_by_stable_id_with_row[[type_de_champ_textarea.stable_id]] }
+    let(:champ_textarea) { dossier.project_champ(type_de_champ_textarea, nil) }
 
     let(:types_de_champ_public) { [{ type: :checkbox }, { type: :header_section }, { type: :explication }, { type: :dossier_link }, { type: :textarea }] }