review: query the db only once

app/models/dossier.rb

@@ -1229,10 +1229,10 @@ class Dossier < ApplicationRecord
     cloned_dossier
   end
 
-  def find_champ_by_stable_id(stable_id)
+  def find_champs_by_stable_ids(stable_ids)
-    return nil if stable_id.blank?
+    return [] if stable_ids.compact.empty?
 
-    champs_public.joins(:type_de_champ).find_by(types_de_champ: { stable_id: stable_id })
+    champs_public.joins(:type_de_champ).where(types_de_champ: { stable_id: stable_ids })
   end
 
   private

app/models/prefill_params.rb

@@ -5,15 +5,47 @@ class PrefillParams
   end
 
   def to_a
-    @params
+    # Builds: [{ stable_id: 1, value: "a value" }, { stable_id: 2, value: "another value"}]
-      .to_unsafe_hash
+    stable_ids_and_values = convert_typed_ids_to_stable_ids
-      .map { |key, value| PrefillValue.new(@dossier, key, value) }
+
+    # Builds: [{stable_id: "1", champ: #<Champs::...>}, {stable_id: "2", champ: #<Champs::...>}]
+    stable_ids_and_champs = find_champs(stable_ids_and_values.map { _1[:stable_id] })
+
+    # Merges the arrays together, filters out the values we don't want and returns [{ id: champ_1_id, value: value_1}, ...]
+    merge_by_stable_ids(stable_ids_and_values + stable_ids_and_champs)
+      .map { PrefillValue.new(champ: _1[:champ], value: _1[:value]) }
       .filter(&:prefillable?)
       .map(&:to_h)
   end
 
   private
 
+  def convert_typed_ids_to_stable_ids
+    @params
+      .to_unsafe_hash
+      .map { |typed_id, value| { stable_id: stable_id_from_typed_id(typed_id), value: value } }
+      .filter { _1[:stable_id].present? && _1[:value].present? }
+  end
+
+  def find_champs(stable_ids)
+    @dossier
+      .find_champs_by_stable_ids(stable_ids)
+      .map { |champ| { stable_id: champ.stable_id.to_s, champ: champ } }
+  end
+
+  def merge_by_stable_ids(arrays_of_hashes_with_stable_id)
+    arrays_of_hashes_with_stable_id
+      .group_by { _1[:stable_id] }
+      .values
+      .map { _1.reduce(:merge) }
+  end
+
+  def stable_id_from_typed_id(typed_id)
+    Champ.id_from_typed_id(typed_id)
+  rescue
+    nil
+  end
+
   class PrefillValue
     AUTHORIZED_TYPES_DE_CHAMPS = [
       TypeDeChamp.type_champs.fetch(:text),
@@ -39,8 +71,8 @@ class PrefillParams
 
     attr_reader :champ, :value
 
-    def initialize(dossier, typed_id, value)
+    def initialize(champ:, value:)
-      @champ = find_champ(dossier, typed_id)
+      @champ = champ
       @value = value
     end
 
@@ -71,12 +103,5 @@ class PrefillParams
       champ.value = value
       champ.valid?
     end
-
-    def find_champ(dossier, typed_id)
-      stable_id = Champ.id_from_typed_id(typed_id) rescue nil
-      return unless stable_id
-
-      dossier.find_champ_by_stable_id(stable_id)
-    end
   end
 end

spec/controllers/users/dossiers_controller_spec.rb

@@ -1108,8 +1108,8 @@ describe Users::DossiersController, type: :controller do
               subject
 
               dossier = Dossier.last
-              expect(dossier.find_champ_by_stable_id(type_de_champ_1.stable_id).value).to eq(value_1)
+              expect(find_champ_by_stable_id(dossier, type_de_champ_1.stable_id).value).to eq(value_1)
-              expect(dossier.find_champ_by_stable_id(type_de_champ_2.stable_id).value).to eq(value_2)
+              expect(find_champ_by_stable_id(dossier, type_de_champ_2.stable_id).value).to eq(value_2)
             end
 
             it { is_expected.to redirect_to siret_dossier_path(id: Dossier.last) }
@@ -1209,9 +1209,9 @@ describe Users::DossiersController, type: :controller do
 
     context 'when not logged in' do
       it 'fails' do
-       subject
+        subject
-       expect { expect(response).to redirect_to(new_user_session_path) }
+        expect { expect(response).to redirect_to(new_user_session_path) }
-     end
+      end
     end
   end
 
@@ -1231,4 +1231,10 @@ describe Users::DossiersController, type: :controller do
       it { expect { subject }.to change { dossier.user.dossiers.count }.by(1) }
     end
   end
+
+  private
+
+  def find_champ_by_stable_id(dossier, stable_id)
+    dossier.champs_public.joins(:type_de_champ).find_by(types_de_champ: { stable_id: stable_id })
+  end
 end

spec/models/concern/dossier_prefillable_concern_spec.rb

@@ -20,11 +20,11 @@ RSpec.describe DossierPrefillableConcern do
       context 'when the champs are valid' do
         let!(:type_de_champ_1) { create(:type_de_champ_text, procedure: procedure) }
         let(:value_1) { "any value" }
-        let(:champ_id_1) { dossier.find_champ_by_stable_id(type_de_champ_1.stable_id).id }
+        let(:champ_id_1) { find_champ_by_stable_id(dossier, type_de_champ_1.stable_id).id }
 
         let!(:type_de_champ_2) { create(:type_de_champ_phone, procedure: procedure) }
         let(:value_2) { "33612345678" }
-        let(:champ_id_2) { dossier.find_champ_by_stable_id(type_de_champ_2.stable_id).id }
+        let(:champ_id_2) { find_champ_by_stable_id(dossier, type_de_champ_2.stable_id).id }
 
         let(:values) { [{ id: champ_id_1, value: value_1 }, { id: champ_id_2, value: value_2 }] }
 
@@ -38,7 +38,7 @@ RSpec.describe DossierPrefillableConcern do
       context 'when a champ is invalid' do
         let!(:type_de_champ) { create(:type_de_champ_phone, procedure: procedure) }
         let(:value) { "a non phone value" }
-        let(:champ_id) { dossier.find_champ_by_stable_id(type_de_champ.stable_id).id }
+        let(:champ_id) { find_champ_by_stable_id(dossier, type_de_champ.stable_id).id }
 
         let(:values) { [{ id: champ_id, value: value }] }
 
@@ -48,4 +48,10 @@ RSpec.describe DossierPrefillableConcern do
       end
     end
   end
+
+  private
+
+  def find_champ_by_stable_id(dossier, stable_id)
+    dossier.champs_public.joins(:type_de_champ).find_by(types_de_champ: { stable_id: stable_id })
+  end
 end

spec/models/dossier_spec.rb

@@ -1855,36 +1855,37 @@ describe Dossier do
     end
   end
 
-  describe "#find_champ_by_stable_id" do
+  describe '#find_champs_by_stable_ids' do
     let(:procedure) { create(:procedure, :published) }
     let(:dossier) { create(:dossier, :brouillon, procedure: procedure) }
 
-    subject { dossier.find_champ_by_stable_id(stable_id) }
+    subject { dossier.find_champs_by_stable_ids(stable_ids) }
 
-    context 'when the stable id is missing' do
+    context 'when stable_ids is empty' do
-      let(:stable_id) { nil }
+      let(:stable_ids) { [] }
 
-      it { expect(subject).to be_nil }
+      it { expect(subject).to match([]) }
     end
 
-    context 'when the stable id is blank' do
+    context 'when stable_ids contains nil or blank values' do
-      let(:stable_id) { "" }
+      let(:stable_ids) { [nil, ""] }
 
-      it { expect(subject).to be_nil }
+      it { expect(subject).to match([]) }
     end
 
-    context 'when the stable id is present' do
+    context 'when stable_ids contains present values' do
-      context 'when the dossier has no champ with the given stable id' do
+      context 'when the dossier has no champ with the given stable ids' do
-        let(:stable_id) { 'My Neighbor Totoro is the best film ever' }
+        let(:stable_ids) { ['My Neighbor Totoro', 'Miyazaki'] }
 
-        it { expect(subject).to be_nil }
+        it { expect(subject).to match([]) }
       end
 
-      context 'when the dossier has a champ with the given stable id' do
+      context 'when the dossier has champs with the given stable ids' do
-        let!(:type_de_champ) { create(:type_de_champ_text, procedure: procedure) }
+        let!(:type_de_champ_1) { create(:type_de_champ_text, procedure: procedure) }
-        let(:stable_id) { type_de_champ.stable_id }
+        let!(:type_de_champ_2) { create(:type_de_champ_textarea, procedure: procedure) }
+        let(:stable_ids) { [type_de_champ_1.stable_id, type_de_champ_2.stable_id] }
 
-        it { expect(subject).to eq(dossier.champs_public.joins(:type_de_champ).find_by(types_de_champ: { stable_id: stable_id })) }
+        it { expect(subject).to match(dossier.champs_public.joins(:type_de_champ).where(types_de_champ: { stable_id: stable_ids })) }
       end
     end
   end

spec/models/prefill_params_spec.rb

@@ -8,11 +8,11 @@ RSpec.describe PrefillParams do
     context "when the stable ids match the TypeDeChamp of the corresponding procedure" do
       let!(:type_de_champ_1) { create(:type_de_champ_text, procedure: procedure) }
       let(:value_1) { "any value" }
-      let(:champ_id_1) { dossier.find_champ_by_stable_id(type_de_champ_1.stable_id).id }
+      let(:champ_id_1) { find_champ_by_stable_id(dossier, type_de_champ_1.stable_id).id }
 
       let!(:type_de_champ_2) { create(:type_de_champ_textarea, procedure: procedure) }
       let(:value_2) { "another value" }
-      let(:champ_id_2) { dossier.find_champ_by_stable_id(type_de_champ_2.stable_id).id }
+      let(:champ_id_2) { find_champ_by_stable_id(dossier, type_de_champ_2.stable_id).id }
 
       let(:params) {
         {
@@ -50,7 +50,7 @@ RSpec.describe PrefillParams do
     shared_examples "a champ public value that is authorized" do |type_de_champ_name, value|
       context "when the type de champ is authorized (#{type_de_champ_name})" do
         let!(:type_de_champ) { create(type_de_champ_name, procedure: procedure) }
-        let(:champ_id) { dossier.find_champ_by_stable_id(type_de_champ.stable_id).id }
+        let(:champ_id) { find_champ_by_stable_id(dossier, type_de_champ.stable_id).id }
 
         let(:params) { { type_de_champ.to_typed_id => value } }
 
@@ -111,4 +111,10 @@ RSpec.describe PrefillParams do
     it_behaves_like "a champ public value that is unauthorized", :type_de_champ_mesri, "value"
     it_behaves_like "a champ public value that is unauthorized", :type_de_champ_carte, "value"
   end
+
+  private
+
+  def find_champ_by_stable_id(dossier, stable_id)
+    dossier.champs_public.joins(:type_de_champ).find_by(types_de_champ: { stable_id: stable_id })
+  end
 end