Merge pull request #10771 from tchak/refactor-champs-revert

Champ related cleanups and refactoring (bis)

app/controllers/api/v1/dossiers_controller.rb

@@ -18,6 +18,7 @@ def index
 
   def show
     dossier = @dossiers.for_api.find(params[:id])
+    DossierPreloader.load_one(dossier)
 
     render json: { dossier: DossierSerializer.new(dossier).as_json }
   rescue ActiveRecord::RecordNotFound

app/controllers/champs/champ_controller.rb

@@ -9,7 +9,11 @@ class Champs::ChampController < ApplicationController
   def find_champ
     dossier = policy_scope(Dossier).includes(:champs, revision: [:types_de_champ]).find(params[:dossier_id])
     type_de_champ = dossier.find_type_de_champ_by_stable_id(params[:stable_id])
-    dossier.champ_for_update(type_de_champ, params_row_id, updated_by: current_user.email)
+    if type_de_champ.repetition?
+      dossier.project_champ(type_de_champ, nil)
+    else
+      dossier.champ_for_update(type_de_champ, params_row_id, updated_by: current_user.email)
+    end
   end
 
   def params_row_id

app/models/attestation_template.rb

@@ -87,13 +87,13 @@ def attestation_for(dossier)
   end
 
   def unspecified_champs_for_dossier(dossier)
-    champs_by_stable_id = dossier.champs_for_revision(root: true).index_by { "tdc#{_1.stable_id}" }
+    types_de_champ_by_tag_id = dossier.revision.types_de_champ.index_by { "tdc#{_1.stable_id}" }
 
     used_tags.filter_map do |used_tag|
-      corresponding_champ = champs_by_stable_id[used_tag]
+      corresponding_type_de_champ = types_de_champ_by_tag_id[used_tag]
 
-      if corresponding_champ && corresponding_champ.blank?
-        corresponding_champ
+      if corresponding_type_de_champ && dossier.project_champ(corresponding_type_de_champ, nil).blank?
+        corresponding_type_de_champ
       end
     end
   end

app/models/champ.rb

@@ -6,6 +6,8 @@ class Champ < ApplicationRecord
 
   self.ignored_columns += [:type_de_champ_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
@@ -94,7 +96,7 @@ def public?
   end
 
   def child?
-    parent_id.present?
+    row_id.present?
   end
 
   # used for the `required` html attribute

app/models/champs/repetition_champ.rb

@@ -37,7 +37,7 @@ def focusable_input_id
   end
 
   def blank?
-    champs.empty?
+    row_ids.empty?
   end
 
   def search_terms

app/models/concerns/champs_validate_concern.rb

@@ -16,9 +16,9 @@ def valid_champ_value?
     def validate_champ_value?
       case validation_context
       when :champs_public_value
-        public? && visible?
+        public? && in_dossier_revision? && visible?
       when :champs_private_value
-        private? && visible?
+        private? && in_dossier_revision? && visible?
       else
         false
       end
@@ -27,5 +27,9 @@ def validate_champ_value?
     def validate_champ_value_or_prefill?
       validate_champ_value? || validation_context == :prefill
     end
+
+    def in_dossier_revision?
+      dossier.revision.types_de_champ.any? { _1.stable_id == stable_id }
+    end
   end
 end

app/models/concerns/dossier_champs_concern.rb

@@ -33,6 +33,7 @@ def champs_for_export(types_de_champ, row_id = nil)
   end
 
   def project_champ(type_de_champ, row_id)
+    check_valid_row_id?(type_de_champ, row_id)
     champ = champs_by_public_id[type_de_champ.public_id(row_id)]
     if champ.nil?
       type_de_champ.build_champ(dossier: self, row_id:)
@@ -56,8 +57,8 @@ def champs_for_prefill(stable_ids)
     revision
       .types_de_champ
       .filter { _1.stable_id.in?(stable_ids) }
-      .filter { !revision.child?(_1) }
-      .map { champ_for_update(_1, nil, updated_by: nil) }
+      .filter { !_1.child?(revision) }
+      .map { _1.repetition? ? project_champ(_1, nil) : champ_for_update(_1, nil, updated_by: nil) }
   end
 
   def champ_for_update(type_de_champ, row_id, updated_by:)
@@ -81,6 +82,7 @@ def champs_by_public_id
   end
 
   def champ_for_export(type_de_champ, row_id)
+    check_valid_row_id?(type_de_champ, row_id)
     champ = champs_by_public_id[type_de_champ.public_id(row_id)]
     if champ.blank? || !champ.visible?
       nil
@@ -96,6 +98,7 @@ def champ_attributes_by_public_id(public_id, attributes, scope, updated_by:)
   end
 
   def champ_with_attributes_for_update(type_de_champ, row_id, updated_by:)
+    check_valid_row_id?(type_de_champ, row_id)
     attributes = type_de_champ.params_for_champ
     # TODO: Once we have the right index in place, we should change this to use `create_or_find_by` instead of `find_or_create_by`
     champ = champs
@@ -124,4 +127,14 @@ def champ_with_attributes_for_update(type_de_champ, row_id, updated_by:)
 
     [champ, attributes]
   end
+
+  def check_valid_row_id?(type_de_champ, row_id)
+    if type_de_champ.child?(revision)
+      if row_id.blank?
+        raise "type_de_champ #{type_de_champ.stable_id} must have a row_id because it is part of a repetition"
+      end
+    elsif row_id.present?
+      raise "type_de_champ #{type_de_champ.stable_id} can not have a row_id because it is not part of a repetition"
+    end
+  end
 end

app/models/concerns/dossier_sections_concern.rb

@@ -19,7 +19,7 @@ def sections_for(type_de_champ)
     end
 
     def auto_numbering_section_headers_for?(type_de_champ)
-      return false if revision.child?(type_de_champ)
+      return false if type_de_champ.child?(revision)
 
       sections_for(type_de_champ)&.none? { _1.libelle =~ /^\d/ }
     end

app/models/dossier.rb

@@ -51,7 +51,6 @@ class Dossier < ApplicationRecord
   has_many :champs_to_destroy, -> { order(:parent_id) }, class_name: 'Champ', inverse_of: false, dependent: :destroy
   has_many :champs_public, -> { root.public_only }, class_name: 'Champ', inverse_of: false
   has_many :champs_private, -> { root.private_only }, class_name: 'Champ', inverse_of: false
-  has_many :prefilled_champs_public, -> { root.public_only.prefilled }, class_name: 'Champ', inverse_of: false
 
   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
@@ -270,33 +269,16 @@ def classer_sans_suite(motivation: nil, instructeur: nil, processed_at: Time.zon
   scope :en_cours,                    -> { not_archived.state_en_construction_ou_instruction }
   scope :without_followers,           -> { where.missing(:follows) }
   scope :with_followers,              -> { left_outer_joins(:follows).where.not(follows: { id: nil }) }
-  scope :with_champs, -> {
-    includes(champs_public: [
-      :geo_areas,
-      piece_justificative_file_attachments: :blob,
-      champs: [piece_justificative_file_attachments: :blob]
-    ])
-  }
-
   scope :brouillons_recently_updated, -> { updated_since(2.days.ago).state_brouillon.order_by_updated_at }
-  scope :with_annotations, -> {
-    includes(champs_private: [
-      :geo_areas,
-      piece_justificative_file_attachments: :blob,
-      champs: [piece_justificative_file_attachments: :blob]
-    ])
-  }
   scope :for_api, -> {
-    with_champs
-      .with_annotations
-      .includes(commentaires: { piece_jointe_attachments: :blob },
-        justificatif_motivation_attachment: :blob,
-        attestation: [],
-        avis: { piece_justificative_file_attachment: :blob },
-        traitement: [],
-        etablissement: [],
-        individual: [],
-        user: [])
+    includes(commentaires: { piece_jointe_attachments: :blob },
+      justificatif_motivation_attachment: :blob,
+      attestation: [],
+      avis: { piece_justificative_file_attachment: :blob },
+      traitement: [],
+      etablissement: [],
+      individual: [],
+      user: [])
   }
 
   scope :with_notifiable_procedure, -> (opts = { notify_on_closed: false }) do
@@ -441,8 +423,6 @@ def classer_sans_suite(motivation: nil, instructeur: nil, processed_at: Time.zon
   validates :mandataire_last_name, presence: true, if: :for_tiers?
   validates :for_tiers, inclusion: { in: [true, false] }, if: -> { revision&.procedure&.for_individual? }
 
-  validates_associated :prefilled_champs_public, on: :champs_public_value
-
   def types_de_champ_public
     types_de_champ
   end
@@ -949,7 +929,7 @@ def previously_termine?
   end
 
   def remove_titres_identite!
-    champs_public.filter(&:titre_identite?).map(&:piece_justificative_file).each(&:purge_later)
+    champs.filter(&:titre_identite?).map(&:piece_justificative_file).each(&:purge_later)
   end
 
   def remove_piece_justificative_file_not_visible!
@@ -1164,7 +1144,7 @@ def user_from_france_connect?
   end
 
   def has_annotations?
-    revision.revision_types_de_champ_private.present?
+    revision.types_de_champ_private.present?
   end
 
   def hide_info_with_accuse_lecture?

app/models/dossier_preloader.rb

@@ -37,18 +37,12 @@ def self.load_one(dossier, pj_template: false)
 
   private
 
-  def revisions
+  def revisions(pj_template: false)
     @revisions ||= ProcedureRevision.where(id: @dossiers.pluck(:revision_id).uniq)
-      .includes(types_de_champ: { piece_justificative_template_attachment: :blob })
+      .includes(types_de_champ: pj_template ? { piece_justificative_template_attachment: :blob } : [])
       .index_by(&:id)
   end
 
-  # returns: { revision_id : { stable_id : position } }
-  def positions
-    @positions ||= revisions
-      .transform_values { |revision| revision.revision_types_de_champ.map { [_1.stable_id, _1.position] }.to_h }
-  end
-
   def load_dossiers(dossiers, pj_template: false)
     to_include = @includes_for_champ.dup
     to_include << [piece_justificative_file_attachments: :blob]
@@ -58,16 +52,10 @@ def load_dossiers(dossiers, pj_template: false)
       .where(dossier_id: dossiers)
       .to_a
 
-    children_champs, root_champs = all_champs.partition(&:child?)
-    champs_by_dossier = root_champs.group_by(&:dossier_id)
-    champs_by_dossier_by_parent = children_champs
-      .group_by(&:dossier_id)
-      .transform_values do |champs|
-        champs.group_by(&:parent_id)
-      end
+    champs_by_dossier = all_champs.group_by(&:dossier_id)
 
     dossiers.each do |dossier|
-      load_dossier(dossier, champs_by_dossier[dossier.id] || [], champs_by_dossier_by_parent[dossier.id] || {})
+      load_dossier(dossier, champs_by_dossier[dossier.id] || [], pj_template:)
     end
 
     load_etablissements(all_champs)
@@ -86,52 +74,36 @@ def load_etablissements(champs)
     end
   end
 
-  def load_dossier(dossier, champs, children_by_parent = {})
-    revision = revisions[dossier.revision_id]
+  def load_dossier(dossier, champs, pj_template: false)
+    revision = revisions(pj_template:)[dossier.revision_id]
     if revision.present?
       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)
 
-    champs_public, champs_private = champs.partition(&:public?)
-
-    dossier.association(:champs).target = []
-    load_champs(dossier, :champs_public, champs_public, dossier, children_by_parent)
-    load_champs(dossier, :champs_private, champs_private, dossier, children_by_parent)
-
-    # We need to do this because of the check on `Etablissement#champ` in
-    # `Etablissement#libelle_for_export`. By assigning `nil` to `target` we mark association
-    # as loaded and so the check on `Etablissement#champ` will not trigger n+1 query.
-    if dossier.etablissement
-      dossier.etablissement.association(:champ).target = nil
-    end
-  end
-
-  def load_champs(parent, name, champs, dossier, children_by_parent)
-    if champs.empty?
-      parent.association(name).target = [] # tells to Rails association has been loaded
-      return
-    end
+    # remove once parent_id is deprecated
+    champs_by_parent_id = champs.group_by(&:parent_id)
 
     champs.each do |champ|
       champ.association(:dossier).target = dossier
 
-      if parent.is_a?(Champ)
-        champ.association(:parent).target = parent
+      # 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
 
-    dossier.association(:champs).target += champs
-
-    parent.association(name).target = champs
-      .filter { positions[dossier.revision_id][_1.stable_id].present? }
-      .sort_by { [_1.row_id, positions[dossier.revision_id][_1.stable_id]] }
-
-    # Load children champs
-    champs.filter(&:block?).each do |parent_champ|
-      champs = children_by_parent[parent_champ.id] || []
-      parent_champ.association(:dossier).target = dossier
-
-      load_champs(parent_champ, :champs, champs, dossier, children_by_parent)
+    # We need to do this because of the check on `Etablissement#champ` in
+    # `Etablissement#libelle_for_export`. By assigning `nil` to `target` we mark association
+    # as loaded and so the check on `Etablissement#champ` will not trigger n+1 query.
+    if dossier.etablissement
+      dossier.etablissement.association(:champ).target = nil
     end
   end
 end

app/models/procedure.rb

@@ -209,20 +209,6 @@ def revisions_with_pending_dossiers
     includes(:draft_revision, :published_revision, administrateurs: :user)
   }
 
-  scope :for_download, -> {
-    includes(
-      :groupe_instructeurs,
-      dossiers: {
-        champs_public: [
-          piece_justificative_file_attachments: :blob,
-          champs: [
-            piece_justificative_file_attachments: :blob
-          ]
-        ]
-      }
-    )
-  }
-
   validates :libelle, presence: true, allow_blank: false, allow_nil: false
   validates :description, presence: true, allow_blank: false, allow_nil: false
   validates :administrateurs, presence: true

app/models/procedure_revision.rb

@@ -219,11 +219,6 @@ def parent_of(tdc)
       .find { _1.type_de_champ_id == tdc.id }.parent&.type_de_champ
   end
 
-  def child?(tdc)
-    revision_types_de_champ
-      .find { _1.type_de_champ_id == tdc.id }.child?
-  end
-
   def remove_children_of(tdc)
     children_of(tdc).each do |child|
       remove_type_de_champ(child.stable_id)

app/models/procedure_revision_type_de_champ.rb

@@ -32,8 +32,8 @@ def empty?
   end
 
   def siblings
-    if parent_id.present?
-      revision.revision_types_de_champ.where(parent_id: parent_id).ordered
+    if child?
+      revision.revision_types_de_champ.where(parent_id:).ordered
     elsif private?
       revision.revision_types_de_champ_private
     else

app/models/type_de_champ.rb

@@ -456,6 +456,10 @@ def public?
     !private?
   end
 
+  def child?(revision)
+    revision.revision_types_de_champ.find { _1.type_de_champ_id == id }&.child?
+  end
+
   def filename_for_attachement(attachment_sym)
     attachment = send(attachment_sym)
     if attachment.attached?

app/views/instructeurs/dossiers/_instruction_button_motivation.html.haml

@@ -12,7 +12,7 @@
       - if unspecified_attestation_champs.present?
         .warning
           Attention, les valeurs suivantes n’ont pas été renseignées mais sont nécessaires pour pouvoir envoyer une attestation valide :
-          - unspecified_annotations_privees, unspecified_champs = unspecified_attestation_champs.partition(&:private)
+          - unspecified_annotations_privees, unspecified_champs = unspecified_attestation_champs.partition(&:private?)
 
           - if unspecified_champs.present?
             %h4 Champs de la demande