Merge pull request #6120 from betagouv/improve_instructeur_show_array

Améliore le temps de réponse de la vue d'une procédure par un instructeur

app/controllers/instructeurs/procedures_controller.rb

@@ -108,10 +108,10 @@ module Instructeurs
 
       @dossiers = @dossiers.where(id: filtered_sorted_paginated_ids)
 
-      @dossiers = procedure_presentation.eager_load_displayed_fields(@dossiers)
-
       @dossiers = @dossiers.sort_by { |d| filtered_sorted_paginated_ids.index(d.id) }
 
+      @projected_dossiers = DossierProjectionService.project(filtered_sorted_paginated_ids, procedure_presentation.displayed_fields)
+
       kaminarize(page, filtered_sorted_ids.count)
 
       assign_exports

app/models/procedure_presentation.rb

@@ -90,10 +90,6 @@ class ProcedurePresentation < ApplicationRecord
     ]
   end
 
-  def displayed_fields_values(dossier)
-    displayed_fields.map { |field| get_value(dossier, field[TABLE], field[COLUMN]) }
-  end
-
   def sorted_ids(dossiers, instructeur)
     table, column, order = sort.values_at(TABLE, COLUMN, 'order')
 
@@ -178,25 +174,6 @@ class ProcedurePresentation < ApplicationRecord
     end.reduce(:&)
   end
 
-  def eager_load_displayed_fields(dossiers)
-    relations_to_include = displayed_fields
-      .pluck(TABLE)
-      .reject { |table| table == 'self' }
-      .map do |table|
-        case table
-        when TYPE_DE_CHAMP
-          { champs: :type_de_champ }
-        when TYPE_DE_CHAMP_PRIVATE
-          { champs_private: :type_de_champ }
-        else
-          table
-        end
-      end
-      .uniq
-
-    dossiers.includes(relations_to_include)
-  end
-
   def human_value_for_filter(filter)
     case filter[TABLE]
     when TYPE_DE_CHAMP, TYPE_DE_CHAMP_PRIVATE
@@ -314,23 +291,6 @@ class ProcedurePresentation < ApplicationRecord
     end
   end
 
-  def get_value(dossier, table, column)
-    case table
-    when 'self'
-      dossier.send(column)&.strftime('%d/%m/%Y')
-    when 'user', 'individual', 'etablissement'
-      dossier.send(table)&.send(column)
-    when 'followers_instructeurs'
-      dossier.send(table)&.map { |g| g.send(column) }&.join(', ')
-    when TYPE_DE_CHAMP
-      dossier.champs.find { |c| c.stable_id == column.to_i }.to_s
-    when TYPE_DE_CHAMP_PRIVATE
-      dossier.champs_private.find { |c| c.stable_id == column.to_i }.to_s
-    when 'groupe_instructeur'
-      dossier.groupe_instructeur.label
-    end
-  end
-
   def field_hash(label, table, column)
     {
       'label' => label,

app/services/dossier_projection_service.rb

@@ -0,0 +1,88 @@
+class DossierProjectionService
+  class DossierProjection < Struct.new(:dossier, :columns)
+  end
+
+  TABLE = 'table'
+  COLUMN = 'column'
+
+  # Returns [DossierProjection(dossier, columns)] ordered by dossiers_ids
+  # and the columns orderd by fields.
+  #
+  # It tries to be fast by using `pluck` (or at least `select`)
+  # to avoid deserializing entire records.
+  #
+  # It stores its intermediary queries results in an hash in the corresponding field.
+  # ex: field_email[:id_value_h] = { dossier_id_1: email_1, dossier_id_3: email_3 }
+  #
+  # Those hashes are needed because:
+  # - the order of the intermediary query results are unknown
+  # - some values can be missing (if a revision added or removed them)
+  def self.project(dossiers_ids, fields)
+    champ_fields, other_fields = fields
+      .partition { |f| ['type_de_champ', 'type_de_champ_private'].include?(f[TABLE]) }
+
+    if champ_fields.present?
+      Champ
+        .includes(:type_de_champ)
+        .where(
+          # as querying the champs table is costly
+          # we fetch all the requested champs at once
+          types_de_champ: { stable_id: champ_fields.map { |f| f[COLUMN] } },
+          dossier_id: dossiers_ids
+        )
+        .select(:dossier_id, :value, :type_de_champ_id, :stable_id) # we cannot pluck :value, as we need the champ.to_s method
+        .group_by(&:stable_id) # the champs are redispatched to their respective fields
+        .map do |stable_id, champs|
+        field = champ_fields.find { |f| f[COLUMN] == stable_id.to_s }
+        field[:id_value_h] = champs.to_h { |c| [c.dossier_id, c.to_s] }
+      end
+    end
+
+    other_fields.each do |field|
+      field[:id_value_h] = case field[TABLE]
+      when 'self'
+        Dossier
+          .where(id: dossiers_ids)
+          .pluck(:id, field[COLUMN].to_sym)
+          .to_h { |id, col| [id, col&.strftime('%d/%m/%Y')] }
+      when 'user'
+        Dossier
+          .joins(:user)
+          .where(id: dossiers_ids)
+          .pluck('dossiers.id, users.email')
+          .to_h
+      when 'individual'
+        Individual
+          .where(dossier_id: dossiers_ids)
+          .pluck(:dossier_id, field[COLUMN].to_sym)
+          .to_h
+      when 'etablissement'
+        Etablissement
+          .where(dossier_id: dossiers_ids)
+          .pluck(:dossier_id, field[COLUMN].to_sym)
+          .to_h
+      when 'groupe_instructeur'
+        Dossier
+          .joins(:groupe_instructeur)
+          .where(id: dossiers_ids)
+          .pluck('dossiers.id, groupe_instructeurs.label')
+          .to_h
+      when 'followers_instructeurs'
+        Follow
+          .active
+          .joins(instructeur: :user)
+          .where(dossier_id: dossiers_ids)
+          .pluck('dossier_id, users.email')
+          .group_by { |dossier_id, _| dossier_id }
+          .to_h { |dossier_id, dossier_id_emails| [dossier_id, dossier_id_emails.map { |_, email| email }&.join(', ')] }
+      end
+    end
+
+    Dossier
+      .select(:id, :state, :archived) # the dossier object is needed in the view
+      .find(dossiers_ids) # keeps dossiers_ids order and raise exception if one is missing
+      .map do |dossier|
+      DossierProjection.new(dossier, fields.map { |f| f[:id_value_h][dossier.id] })
+    end
+  end
+end

app/views/instructeurs/procedures/show.html.haml

@@ -129,25 +129,27 @@
                     = submit_tag "Enregistrer", class: 'button'
 
         %tbody
-          - @dossiers.each do |dossier|
+          - @projected_dossiers.each do |p|
+            - dossier = p.dossier
+            - path = instructeur_dossier_path(@procedure, dossier.id)
+
             %tr
               %td.folder-col
-                = link_to(instructeur_dossier_path(@procedure, dossier), class: 'cell-link') do
+                %a.cell-link{ href: path }
                   %span.icon.folder
                     - if @not_archived_notifications_dossier_ids.include?(dossier.id)
                       %span.notifications{ 'aria-label': 'notifications' }
 
               %td.number-col
-                = link_to(instructeur_dossier_path(@procedure, dossier), class: 'cell-link') do
+                %a.cell-link{ href: path }= dossier.id
-                  = dossier.id
 
-              - @procedure_presentation.displayed_fields_values(dossier).each do |value|
+              - p.columns.each do |column|
                 %td
-                  = link_to(value, instructeur_dossier_path(@procedure, dossier), class: 'cell-link')
+                  %a.cell-link{ href: path }= column
 
               %td.status-col
-                = link_to(instructeur_dossier_path(@procedure, dossier), class: 'cell-link') do
+                %a.cell-link{ href: path }= status_badge(dossier.state)
-                  = status_badge(dossier.state)
+
               %td.action-col.follow-col= render partial: 'dossier_actions', locals: { procedure: @procedure, dossier: dossier, dossier_is_followed: @followed_dossiers_id.include?(dossier.id) }
       = paginate @dossiers
     - else

spec/models/procedure_presentation_spec.rb

@@ -125,126 +125,6 @@ describe ProcedurePresentation do
     it { expect(subject.displayed_fields_for_select).to eq([[["label1", "table1/column1"], ["label2", "table2/column2"]], ["user/email"]]) }
   end
 
-  describe '#get_value' do
-    let(:procedure_presentation) { create(:procedure_presentation, procedure: procedure, assign_to: assign_to, displayed_fields: [{ 'table' => table, 'column' => column }]) }
-
-    subject { procedure_presentation.displayed_fields_values(dossier).first }
-
-    context 'for self table' do
-      let(:table) { 'self' }
-
-      context 'for created_at column' do
-        let(:column) { 'created_at' }
-        let(:dossier) { Timecop.freeze(Time.zone.local(1992, 3, 22)) { create(:dossier, procedure: procedure) } }
-
-        it { is_expected.to eq('22/03/1992') }
-      end
-
-      context 'for en_construction_at column' do
-        let(:column) { 'en_construction_at' }
-        let(:dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 17)) }
-
-        it { is_expected.to eq('17/10/2018') }
-      end
-
-      context 'for updated_at column' do
-        let(:column) { 'updated_at' }
-        let(:dossier) { create(:dossier, procedure: procedure) }
-
-        before { dossier.touch(time: Time.zone.local(2018, 9, 25)) }
-
-        it { is_expected.to eq('25/09/2018') }
-      end
-    end
-
-    context 'for user table' do
-      let(:table) { 'user' }
-      let(:column) { 'email' }
-
-      let(:dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'bla@yopmail.com')) }
-
-      it { is_expected.to eq('bla@yopmail.com') }
-    end
-
-    context 'for individual table' do
-      let(:table) { 'individual' }
-      let(:procedure) { create(:procedure, :for_individual, :with_type_de_champ, :with_type_de_champ_private) }
-      let(:dossier) { create(:dossier, procedure: procedure, individual: create(:individual, nom: 'Martin', prenom: 'Jacques', gender: 'M.')) }
-
-      context 'for prenom column' do
-        let(:column) { 'prenom' }
-
-        it { is_expected.to eq('Jacques') }
-      end
-
-      context 'for nom column' do
-        let(:column) { 'nom' }
-
-        it { is_expected.to eq('Martin') }
-      end
-
-      context 'for gender column' do
-        let(:column) { 'gender' }
-
-        it { is_expected.to eq('M.') }
-      end
-    end
-
-    context 'for etablissement table' do
-      let(:table) { 'etablissement' }
-      let(:column) { 'code_postal' } # All other columns work the same, no extra test required
-
-      let!(:dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '75008')) }
-
-      it { is_expected.to eq('75008') }
-    end
-
-    context 'for groupe_instructeur table' do
-      let(:table) { 'groupe_instructeur' }
-      let(:column) { 'label' }
-
-      let!(:dossier) { create(:dossier, procedure: procedure) }
-
-      it { is_expected.to eq('défaut') }
-    end
-
-    context 'for followers_instructeurs table' do
-      let(:table) { 'followers_instructeurs' }
-      let(:column) { 'email' }
-
-      let(:dossier) { create(:dossier, procedure: procedure) }
-      let!(:follow1) { create(:follow, dossier: dossier, instructeur: create(:instructeur, email: 'user1@host')) }
-      let!(:follow2) { create(:follow, dossier: dossier, instructeur: create(:instructeur, email: 'user2@host')) }
-
-      it "return emails of followers instructeurs" do
-        emails_to_display = procedure_presentation.displayed_fields_values(dossier).first.split(', ').sort
-        expect(emails_to_display).to eq ["user1@host", "user2@host"]
-      end
-    end
-
-    context 'for type_de_champ table' do
-      let(:table) { 'type_de_champ' }
-      let(:column) { procedure.types_de_champ.first.stable_id.to_s }
-
-      let(:dossier) { create(:dossier, procedure: procedure) }
-
-      before { dossier.champs.first.update(value: 'kale') }
-
-      it { is_expected.to eq('kale') }
-    end
-
-    context 'for type_de_champ_private table' do
-      let(:table) { 'type_de_champ_private' }
-      let(:column) { procedure.types_de_champ_private.first.stable_id.to_s }
-
-      let(:dossier) { create(:dossier, procedure: procedure) }
-
-      before { dossier.champs_private.first.update(value: 'quinoa') }
-
-      it { is_expected.to eq('quinoa') }
-    end
-  end
-
   describe '#sorted_ids' do
     let(:instructeur) { create(:instructeur) }
     let(:assign_to) { create(:assign_to, procedure: procedure, instructeur: instructeur) }
@@ -755,136 +635,6 @@ describe ProcedurePresentation do
     end
   end
 
-  describe '#eager_load_displayed_fields' do
-    let(:procedure_presentation) { create(:procedure_presentation, procedure: procedure, assign_to: assign_to, displayed_fields: [{ 'table' => table, 'column' => column }]) }
-    let!(:dossier) { create(:dossier, :en_construction, procedure: procedure) }
-    let(:displayed_dossier) { procedure_presentation.eager_load_displayed_fields(procedure.dossiers).first }
-
-    context 'for type de champ' do
-      let(:table) { 'type_de_champ' }
-      let(:column) { procedure.types_de_champ.first.stable_id.to_s }
-
-      it 'preloads the champs relation' do
-        # Ideally, we would only preload the champs for the matching column
-
-        expect(displayed_dossier.association(:champs)).to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for type de champ private' do
-      let(:table) { 'type_de_champ_private' }
-      let(:column) { procedure.types_de_champ_private.first.stable_id.to_s }
-
-      it 'preloads the champs relation' do
-        # Ideally, we would only preload the champs for the matching column
-
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for user' do
-      let(:table) { 'user' }
-      let(:column) { 'email' }
-
-      it 'preloads the user relation' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for individual' do
-      let(:procedure) { create(:procedure, :for_individual, :with_type_de_champ, :with_type_de_champ_private) }
-      let(:table) { 'individual' }
-      let(:column) { 'nom' }
-
-      it 'preloads the individual relation' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for etablissement' do
-      let(:table) { 'etablissement' }
-      let(:column) { 'siret' }
-
-      it 'preloads the etablissement relation' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for followers_instructeurs' do
-      let(:table) { 'followers_instructeurs' }
-      let(:column) { 'email' }
-
-      it 'preloads the followers_instructeurs relation' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).not_to be_loaded
-      end
-    end
-
-    context 'for groupe_instructeur' do
-      let(:table) { 'groupe_instructeur' }
-      let(:column) { 'label' }
-
-      it 'preloads the groupe_instructeur relation' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-        expect(displayed_dossier.association(:groupe_instructeur)).to be_loaded
-      end
-    end
-
-    context 'for self' do
-      let(:table) { 'self' }
-      let(:column) { 'created_at' }
-
-      it 'does not preload anything' do
-        expect(displayed_dossier.association(:champs)).not_to be_loaded
-        expect(displayed_dossier.association(:champs_private)).not_to be_loaded
-        expect(displayed_dossier.association(:user)).not_to be_loaded
-        expect(displayed_dossier.association(:individual)).not_to be_loaded
-        expect(displayed_dossier.association(:etablissement)).not_to be_loaded
-        expect(displayed_dossier.association(:followers_instructeurs)).not_to be_loaded
-      end
-    end
-  end
-
   describe "#human_value_for_filter" do
     let(:filters) { { "suivis" => [{ "label" => "label1", "table" => "type_de_champ", "column" => first_type_de_champ_id, "value" => "true" }] } }
 

spec/services/dossier_projection_service_spec.rb

@@ -0,0 +1,157 @@
+describe DossierProjectionService do
+  describe '#project' do
+    subject { described_class.project(dossiers_ids, fields) }
+
+    context 'with multiple dossier' do
+      let!(:procedure) { create(:procedure, :with_type_de_champ) }
+      let!(:dossier_1) { create(:dossier, procedure: procedure) }
+      let!(:dossier_2) { create(:dossier, procedure: procedure) }
+      let!(:dossier_3) { create(:dossier, procedure: procedure) }
+
+      let(:dossiers_ids) { [dossier_3.id, dossier_1.id, dossier_2.id] }
+      let(:fields) do
+        [
+          {
+            "table" => "type_de_champ",
+            "column" => procedure.types_de_champ[0].stable_id.to_s
+          }
+        ]
+      end
+
+      before do
+        dossier_1.champs.first.update(value: 'champ_1')
+        dossier_2.champs.first.update(value: 'champ_2')
+        dossier_3.champs.first.destroy
+      end
+
+      let(:result) { subject }
+
+      it 'respects the dossiers_ids order and returns nil for empty result' do
+        expect(result.length).to eq(3)
+        expect(result[0].dossier.id).to eq(dossier_3.id)
+        expect(result[1].dossier.id).to eq(dossier_1.id)
+        expect(result[2].dossier.id).to eq(dossier_2.id)
+
+        expect(result[0].columns[0]).to be nil
+        expect(result[1].columns[0]).to eq('champ_1')
+        expect(result[2].columns[0]).to eq('champ_2')
+      end
+    end
+
+    context 'attributes by attributes' do
+      let(:fields) { [{ "table" => table, "column" => column }] }
+      let(:dossiers_ids) { [dossier.id] }
+
+      subject { super()[0].columns[0] }
+
+      context 'for self table' do
+        let(:table) { 'self' }
+
+        context 'for created_at column' do
+          let(:column) { 'created_at' }
+          let(:dossier) { Timecop.freeze(Time.zone.local(1992, 3, 22)) { create(:dossier) } }
+
+          it { is_expected.to eq('22/03/1992') }
+        end
+
+        context 'for en_construction_at column' do
+          let(:column) { 'en_construction_at' }
+          let(:dossier) { create(:dossier, :en_construction, en_construction_at: Time.zone.local(2018, 10, 17)) }
+
+          it { is_expected.to eq('17/10/2018') }
+        end
+
+        context 'for updated_at column' do
+          let(:column) { 'updated_at' }
+          let(:dossier) { create(:dossier) }
+
+          before { dossier.touch(time: Time.zone.local(2018, 9, 25)) }
+
+          it { is_expected.to eq('25/09/2018') }
+        end
+      end
+
+      context 'for user table' do
+        let(:table) { 'user' }
+        let(:column) { 'email' }
+
+        let(:dossier) { create(:dossier, user: create(:user, email: 'bla@yopmail.com')) }
+
+        it { is_expected.to eq('bla@yopmail.com') }
+      end
+
+      context 'for individual table' do
+        let(:table) { 'individual' }
+        let(:procedure) { create(:procedure, :for_individual, :with_type_de_champ, :with_type_de_champ_private) }
+        let(:dossier) { create(:dossier, procedure: procedure, individual: create(:individual, nom: 'Martin', prenom: 'Jacques', gender: 'M.')) }
+
+        context 'for prenom column' do
+          let(:column) { 'prenom' }
+
+          it { is_expected.to eq('Jacques') }
+        end
+
+        context 'for nom column' do
+          let(:column) { 'nom' }
+
+          it { is_expected.to eq('Martin') }
+        end
+
+        context 'for gender column' do
+          let(:column) { 'gender' }
+
+          it { is_expected.to eq('M.') }
+        end
+      end
+
+      context 'for etablissement table' do
+        let(:table) { 'etablissement' }
+        let(:column) { 'code_postal' } # All other columns work the same, no extra test required
+
+        let!(:dossier) { create(:dossier, etablissement: create(:etablissement, code_postal: '75008')) }
+
+        it { is_expected.to eq('75008') }
+      end
+
+      context 'for groupe_instructeur table' do
+        let(:table) { 'groupe_instructeur' }
+        let(:column) { 'label' }
+
+        let!(:dossier) { create(:dossier) }
+
+        it { is_expected.to eq('défaut') }
+      end
+
+      context 'for followers_instructeurs table' do
+        let(:table) { 'followers_instructeurs' }
+        let(:column) { 'email' }
+
+        let(:dossier) { create(:dossier) }
+        let!(:follow1) { create(:follow, dossier: dossier, instructeur: create(:instructeur, email: 'user1@host')) }
+        let!(:follow2) { create(:follow, dossier: dossier, instructeur: create(:instructeur, email: 'user2@host')) }
+
+        it { is_expected.to eq "user1@host, user2@host" }
+      end
+
+      context 'for type_de_champ table' do
+        let(:table) { 'type_de_champ' }
+        let(:dossier) { create(:dossier) }
+        let(:column) { dossier.procedure.types_de_champ.first.stable_id.to_s }
+
+        before { dossier.champs.first.update(value: 'kale') }
+
+        it { is_expected.to eq('kale') }
+      end
+
+      context 'for type_de_champ_private table' do
+        let(:table) { 'type_de_champ_private' }
+        let(:dossier) { create(:dossier) }
+        let(:column) { dossier.procedure.types_de_champ_private.first.stable_id.to_s }
+
+        before { dossier.champs_private.first.update(value: 'quinoa') }
+
+        it { is_expected.to eq('quinoa') }
+      end
+    end
+  end
+end