Merge pull request #10807 from demarches-simplifiees/slow_drop_down_cleaning

Tech: simplification de la logique des champs listes

app/components/editable_champ/drop_down_list_component.rb

@@ -2,7 +2,7 @@
 
 class EditableChamp::DropDownListComponent < EditableChamp::EditableChampBaseComponent
   def render?
-    @champ.options?
+    @champ.drop_down_options.any?
   end
 
   def select_class_names

app/components/editable_champ/linked_drop_down_list_component.rb

@@ -5,6 +5,10 @@ class EditableChamp::LinkedDropDownListComponent < EditableChamp::EditableChampB
     :fieldset
   end
 
+  def render?
+    @champ.drop_down_options.any?
+  end
+
   private
 
   def secondary_label

app/components/editable_champ/linked_drop_down_list_component/linked_drop_down_list_component.html.haml

@@ -1,4 +1,3 @@
-- if @champ.options?
 .fr-fieldset__element.fr-mb-0
   .fr-select-group
     = render EditableChamp::ChampLabelComponent.new form: @form, champ: @champ, seen_at: @seen_at

app/components/editable_champ/multiple_drop_down_list_component/multiple_drop_down_list_component.html.haml

@@ -1,4 +1,3 @@
-- if @champ.options?
 - if @champ.render_as_checkboxes?
   = @form.collection_check_boxes :value, @champ.enabled_non_empty_options, :to_s, :to_s do |b|
     - capture do

app/graphql/types/champ_descriptor_type.rb

@@ -117,7 +117,7 @@ module Types
 
     def options
       if type_de_champ.drop_down_list?
-        type_de_champ.drop_down_list_options.reject(&:empty?)
+        type_de_champ.drop_down_options.reject(&:empty?)
       end
     end
 

app/graphql/types/champs/descriptor/drop_down_list_champ_descriptor_type.rb

@@ -12,7 +12,7 @@ module Types::Champs::Descriptor
     end
 
     def options
-      object.type_de_champ.drop_down_list_options.reject(&:empty?)
+      object.type_de_champ.drop_down_options.reject(&:empty?)
     end
   end
 end

app/graphql/types/champs/descriptor/linked_drop_down_list_champ_descriptor_type.rb

@@ -7,7 +7,7 @@ module Types::Champs::Descriptor
     field :options, [String], "List des options d’un champ avec selection.", null: true
 
     def options
-      object.type_de_champ.drop_down_list_options.reject(&:empty?)
+      object.type_de_champ.drop_down_options.reject(&:empty?)
     end
   end
 end

app/graphql/types/champs/descriptor/multiple_drop_down_list_champ_descriptor_type.rb

@@ -7,7 +7,7 @@ module Types::Champs::Descriptor
     field :options, [String], "List des options d’un champ avec selection.", null: true
 
     def options
-      object.type_de_champ.drop_down_list_options.reject(&:empty?)
+      object.type_de_champ.drop_down_options.reject(&:empty?)
     end
   end
 end

app/models/champ.rb

@@ -29,9 +29,8 @@ class Champ < ApplicationRecord
   delegate :libelle,
     :type_champ,
     :description,
-    :drop_down_list_options,
+    :drop_down_options,
     :drop_down_other?,
-    :drop_down_list_options?,
     :drop_down_list_enabled_non_empty_options,
     :drop_down_secondary_libelle,
     :drop_down_secondary_description,

app/models/champs/drop_down_list_champ.rb

@@ -16,10 +16,6 @@ class Champs::DropDownListChamp < Champ
     enabled_non_empty_options.size >= THRESHOLD_NB_OPTIONS_AS_AUTOCOMPLETE
   end
 
-  def options?
-    drop_down_list_options?
-  end
-
   def html_label?
     !render_as_radios?
   end

app/models/champs/linked_drop_down_list_champ.rb

@@ -3,10 +3,6 @@
 class Champs::LinkedDropDownListChamp < Champ
   delegate :primary_options, :secondary_options, to: :type_de_champ
 
-  def options?
-    drop_down_list_options?
-  end
-
   def primary_value
     if value.present?
       JSON.parse(value)[0]

app/models/champs/multiple_drop_down_list_champ.rb

@@ -3,10 +3,6 @@
 class Champs::MultipleDropDownListChamp < Champ
   validate :values_are_in_options, if: -> { value.present? && validate_champ_value_or_prefill? }
 
-  def options?
-    drop_down_list_options?
-  end
-
   def enabled_non_empty_options
     drop_down_list_enabled_non_empty_options
   end
@@ -92,6 +88,10 @@ class Champs::MultipleDropDownListChamp < Champ
     end
   end
 
+  def render?
+    @champ.drop_down_options.any?
+  end
+
   private
 
   def values_are_in_options

app/models/procedure_revision.rb

@@ -407,11 +407,11 @@ class ProcedureRevision < ApplicationRecord
     end
 
     if to_type_de_champ.drop_down_list?
-      if from_type_de_champ.drop_down_list_options != to_type_de_champ.drop_down_list_options
+      if from_type_de_champ.drop_down_options != to_type_de_champ.drop_down_options
         changes << ProcedureRevisionChange::UpdateChamp.new(from_type_de_champ,
           :drop_down_options,
-          from_type_de_champ.drop_down_list_options,
-          to_type_de_champ.drop_down_list_options)
+          from_type_de_champ.drop_down_options,
+          to_type_de_champ.drop_down_options)
       end
       if to_type_de_champ.linked_drop_down_list?
         if from_type_de_champ.drop_down_secondary_libelle != to_type_de_champ.drop_down_secondary_libelle

app/models/type_de_champ.rb

@@ -474,16 +474,30 @@ class TypeDeChamp < ApplicationRecord
     end
   end
 
+  def drop_down_options
+    Array.wrap(super)
+  end
+
+  def drop_down_list_enabled_non_empty_options(other: false)
+    list_options = drop_down_options.reject(&:empty?)
+
+    if other && drop_down_other?
+      list_options + [[I18n.t('shared.champs.drop_down_list.other'), Champs::DropDownListChamp::OTHER]]
+    else
+      list_options
+    end
+  end
+
   def drop_down_list_value
-    if drop_down_list_options.present?
-      drop_down_list_options.reject(&:empty?).join("\r\n")
+    if drop_down_options.present?
+      drop_down_options.reject(&:empty?).join("\r\n")
     else
       ''
     end
   end
 
   def drop_down_list_value=(value)
-    self.drop_down_options = parse_drop_down_list_value(value)
+    self.drop_down_options = value.to_s.lines.map(&:strip).reject(&:empty?)
   end
 
   def header_section_level_value
@@ -564,28 +578,6 @@ class TypeDeChamp < ApplicationRecord
     end
   end
 
-  def drop_down_list_options?
-    drop_down_list_options.any?
-  end
-
-  def drop_down_list_options
-    drop_down_options.presence || []
-  end
-
-  def drop_down_list_disabled_options
-    drop_down_list_options.filter { |v| (v =~ /^--.*--$/).present? }
-  end
-
-  def drop_down_list_enabled_non_empty_options(other: false)
-    list_options = (drop_down_list_options - drop_down_list_disabled_options).reject(&:empty?)
-
-    if other && drop_down_other?
-      list_options + [[I18n.t('shared.champs.drop_down_list.other'), Champs::DropDownListChamp::OTHER]]
-    else
-      list_options
-    end
-  end
-
   def layer_enabled?(layer)
     options && options[layer] && options[layer] != '0'
   end
@@ -773,13 +765,6 @@ class TypeDeChamp < ApplicationRecord
 
   private
 
-  DEFAULT_EMPTY = ['']
-  def parse_drop_down_list_value(value)
-    value = value ? value.split("\r\n").map(&:strip).join("\r\n") : ''
-    result = value.split(/[\r\n]|[\r]|[\n]|[\n\r]/).reject(&:empty?)
-    result.blank? ? [] : DEFAULT_EMPTY + result
-  end
-
   def populate_stable_id
     if !stable_id
       update_column(:stable_id, id)

app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb

@@ -3,7 +3,7 @@
 class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBase
   PRIMARY_PATTERN = /^--(.*)--$/
 
-  delegate :drop_down_list_options, to: :@type_de_champ
+  delegate :drop_down_options, to: :@type_de_champ
   validate :check_presence_of_primary_options
 
   def libelles_for_export
@@ -89,8 +89,10 @@ class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBas
   end
 
   def unpack_options
-    _, *options = drop_down_list_options
-    chunked = options.slice_before(PRIMARY_PATTERN)
+    chunked = drop_down_options
+      .reject(&:empty?) # TODO: remove after removing empty options
+      .slice_before(PRIMARY_PATTERN)
+
     chunked.map do |chunk|
       primary, *secondary = chunk
       secondary = add_blank_option_when_not_mandatory(secondary)
@@ -99,7 +101,8 @@ class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBas
   end
 
   def check_presence_of_primary_options
-    if !PRIMARY_PATTERN.match?(drop_down_list_options.second)
+    # TODO: replace by `drop_down_options.first` when the empty options are removed
+    if !PRIMARY_PATTERN.match?(drop_down_options.find(&:present?))
       errors.add(libelle.presence || "La liste", "doit commencer par une entrée de menu primaire de la forme <code style='white-space: pre-wrap;'>--texte--</code>")
     end
   end

lib/tasks/deployment/20240917131034_remove_empty_options_from_drop_down_list.rake

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+namespace :after_party do
+  desc 'Deployment task: remove_empty_options_from_drop_down_list'
+  task remove_empty_options_from_drop_down_list: :environment do
+    ids = TypeDeChamp
+      .where(type_champ: ['drop_down_list', 'multiple_drop_down_list', 'linked_drop_down_list'])
+      .where("options->'drop_down_options' @> '[\"\"]'::jsonb").ids
+
+    progress = ProgressReport.new(ids.count)
+
+    TypeDeChamp.where(id: ids).select(:id, :options, :type_champ).find_each do |drop_down_list|
+      drop_down_list.drop_down_options.delete('')
+      drop_down_list.save!(validate: false)
+
+      progress.inc
+    end
+
+    AfterParty::TaskRecord
+      .create version: AfterParty::TaskRecorder.new(__FILE__).timestamp
+  end
+end

spec/controllers/api/v2/graphql_controller_spec.rb

@@ -219,7 +219,7 @@ describe API::V2::GraphqlController do
                 description: tdc.description,
                 required: tdc.mandatory?,
                 champDescriptors: tdc.repetition? ? procedure.active_revision.children_of(tdc.reload).map { { id: _1.to_typed_id, __typename: format_type_champ(_1.type_champ) } } : nil,
-                options: tdc.drop_down_list? ? tdc.drop_down_list_options.reject(&:empty?) : nil
+                options: tdc.drop_down_list? ? tdc.drop_down_options.reject(&:empty?) : nil
               }.compact
             end,
             dossiers: {
@@ -1445,8 +1445,8 @@ describe API::V2::GraphqlController do
         end
 
         describe 'drop_down_list' do
-          let(:drop_down_list_options) { ['bijour'] }
-          let(:types_de_champ_private) { [{ type: :drop_down_list, options: drop_down_list_options }] }
+          let(:drop_down_options) { ['bijour'] }
+          let(:types_de_champ_private) { [{ type: :drop_down_list, options: drop_down_options }] }
           let(:query) do
             "mutation {
               dossierModifierAnnotationDropDownList(input: {
@@ -1466,7 +1466,7 @@ describe API::V2::GraphqlController do
           end
 
           context "success" do
-            let(:value) { drop_down_list_options.first }
+            let(:value) { drop_down_options.first }
             it 'should be a success' do
               expect(gql_errors).to eq(nil)
 
@@ -1479,7 +1479,7 @@ describe API::V2::GraphqlController do
             end
           end
           context "failure" do
-            let(:value) { drop_down_list_options.first.reverse }
+            let(:value) { drop_down_options.first.reverse }
             it 'should be a success' do
               expect(gql_errors).to eq(nil)
 

spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb

@@ -1243,7 +1243,7 @@ describe API::V2::GraphqlController do
             expect(gql_errors).to be_nil
             expect(gql_data[:groupeInstructeurModifier][:errors]).to be_nil
             expect(gql_data[:groupeInstructeurModifier][:groupeInstructeur][:id]).to eq(dossier.groupe_instructeur.to_typed_id)
-            expect(routing_champ.reload.drop_down_list_options).to match_array(procedure.groupe_instructeurs.active.map(&:label))
+            expect(routing_champ.reload.drop_down_options).to match_array(procedure.groupe_instructeurs.active.map(&:label))
             expect(procedure.groupe_instructeurs.active.map(&:routing_rule)).to match_array(procedure.groupe_instructeurs.active.map { ds_eq(champ_value(routing_champ.stable_id), constant(_1.label)) })
           }
         end
@@ -1297,7 +1297,7 @@ describe API::V2::GraphqlController do
           expect(gql_errors).to be_nil
           expect(gql_data[:groupeInstructeurCreer][:errors]).to be_nil
           expect(gql_data[:groupeInstructeurCreer][:groupeInstructeur][:id]).not_to be_nil
-          expect(routing_champ.reload.drop_down_list_options).to match_array(procedure.groupe_instructeurs.map(&:label))
+          expect(routing_champ.reload.drop_down_options).to match_array(procedure.groupe_instructeurs.map(&:label))
           expect(procedure.groupe_instructeurs.map(&:routing_rule)).to match_array(procedure.groupe_instructeurs.map { ds_eq(champ_value(routing_champ.stable_id), constant(_1.label)) })
         }
       end

spec/factories/type_de_champ.rb

@@ -79,12 +79,9 @@ FactoryBot.define do
     factory :type_de_champ_drop_down_list do
       libelle { 'Choix unique' }
       type_champ { TypeDeChamp.type_champs.fetch(:drop_down_list) }
-      drop_down_list_value { "val1\r\nval2\r\n--separateur--\r\nval3" }
+      drop_down_list_value { "val1\r\nval2\r\nval3" }
       trait :long do
-        drop_down_list_value { "alpha\r\nbravo\r\n--separateur--\r\ncharly\r\ndelta\r\necho\r\nfox-trot\r\ngolf" }
-      end
-      trait :without_selectable_values do
-        drop_down_list_value { "\r\n--separateur--\r\n--separateur 2--\r\n \r\n" }
+        drop_down_list_value { "alpha\r\nbravo\r\ncharly\r\ndelta\r\necho\r\nfox-trot\r\ngolf" }
       end
       trait :with_other do
         drop_down_other { true }
@@ -92,14 +89,14 @@ FactoryBot.define do
     end
     factory :type_de_champ_multiple_drop_down_list do
       type_champ { TypeDeChamp.type_champs.fetch(:multiple_drop_down_list) }
-      drop_down_list_value { "val1\r\nval2\r\n--separateur--\r\nval3" }
+      drop_down_list_value { "val1\r\nval2\r\nval3" }
       trait :long do
-        drop_down_list_value { "alpha\r\nbravo\r\n--separateur--\r\ncharly\r\ndelta\r\necho\r\nfox-trot\r\ngolf" }
+        drop_down_list_value { "alpha\r\nbravo\r\ncharly\r\ndelta\r\necho\r\nfox-trot\r\ngolf" }
       end
     end
     factory :type_de_champ_linked_drop_down_list do
       type_champ { TypeDeChamp.type_champs.fetch(:linked_drop_down_list) }
-      drop_down_list_value { "--primary--\nsecondary\n" }
+      drop_down_list_value { "--primary--\r\nsecondary\n" }
     end
     factory :type_de_champ_expression_reguliere do
       type_champ { TypeDeChamp.type_champs.fetch(:expression_reguliere) }

spec/lib/tasks/deployment/20240917131034_remove_empty_options_from_drop_down_list.rake_spec.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+describe '20240917131034_remove_empty_options_from_drop_down_list.rake' do
+  let(:rake_task) { Rake::Task['after_party:remove_empty_options_from_drop_down_list'] }
+
+  let!(:drop_down_list_with_empty_option) do
+    create(:type_de_champ_drop_down_list, drop_down_options: ['', '1', '2'])
+  end
+
+  let!(:drop_down_list_with_other_empty_option) do
+    create(:type_de_champ_drop_down_list, drop_down_options: ['1', '', '2', ''])
+  end
+
+  let!(:witness_drop_down_list) do
+    create(:type_de_champ_drop_down_list, drop_down_options: ['1', '2'])
+  end
+
+  before do
+    rake_task.invoke
+
+    [
+      drop_down_list_with_empty_option,
+      drop_down_list_with_other_empty_option,
+      witness_drop_down_list
+    ].each(&:reload)
+  end
+
+  after { rake_task.reenable }
+
+  it 'removes the empty option' do
+    expect(drop_down_list_with_empty_option.drop_down_options).to eq(['1', '2'])
+    expect(drop_down_list_with_other_empty_option.drop_down_options).to eq(['1', '2'])
+    expect(witness_drop_down_list.drop_down_options).to eq(['1', '2'])
+  end
+end

spec/models/champs/linked_drop_down_list_champ_spec.rb

@@ -78,7 +78,7 @@ describe Champs::LinkedDropDownListChamp do
   end
 
   describe '#mandatory_and_blank' do
-    let(:value) { "--Primary--\nSecondary" }
+    let(:value) { "--Primary--\r\nSecondary" }
 
     subject { described_class.new }
     before { allow(subject).to receive(:type_de_champ).and_return(type_de_champ) }

spec/models/type_de_champ_spec.rb

@@ -107,7 +107,7 @@ describe TypeDeChamp do
       context 'when the target type_champ is not drop_down_list' do
         let(:target_type_champ) { TypeDeChamp.type_champs.fetch(:text) }
 
-        it { expect(tdc.drop_down_options).to be_nil }
+        it { expect(tdc.drop_down_options).to be_empty }
       end
 
       context 'when the target type_champ is linked_drop_down_list' do
@@ -192,46 +192,21 @@ describe TypeDeChamp do
     end
   end
 
-  describe '#drop_down_list_options' do
-    let(:value) do
-      <<~EOS
-        Cohésion sociale
-        Dév.Eco / Emploi
-        Cadre de vie / Urb.
-        Pilotage / Ingénierie
-      EOS
-    end
-    let(:type_de_champ) { create(:type_de_champ_drop_down_list, drop_down_list_value: value) }
+  describe '#drop_down_options' do
+    let(:type_de_champ) { create(:type_de_champ_drop_down_list) }
 
-    it { expect(type_de_champ.drop_down_list_options).to eq ['', 'Cohésion sociale', 'Dév.Eco / Emploi', 'Cadre de vie / Urb.', 'Pilotage / Ingénierie'] }
+    it "splits input" do
+      type_de_champ.drop_down_list_value = nil
+      expect(type_de_champ.drop_down_options).to eq([])
 
-    context 'when one value is empty' do
-      let(:value) do
-        <<~EOS
-          Cohésion sociale
-          Cadre de vie / Urb.
-          Pilotage / Ingénierie
-        EOS
-      end
+      type_de_champ.drop_down_list_value = "\n\r"
+      expect(type_de_champ.drop_down_options).to eq([])
 
-      it { expect(type_de_champ.drop_down_list_options).to eq ['', 'Cohésion sociale', 'Cadre de vie / Urb.', 'Pilotage / Ingénierie'] }
+      type_de_champ.drop_down_list_value = " 1 / 2 \r\n 3"
+      expect(type_de_champ.drop_down_options).to eq(['1 / 2', '3'])
     end
   end
 
-  describe 'disabled_options' do
-    let(:value) do
-      <<~EOS
-        tip
-        --top--
-        --troupt--
-        ouaich
-      EOS
-    end
-    let(:type_de_champ) { create(:type_de_champ_drop_down_list, drop_down_list_value: value) }
-
-    it { expect(type_de_champ.drop_down_list_disabled_options).to match(['--top--', '--troupt--']) }
-  end
-
   describe '#public_only' do
     let(:procedure) { create(:procedure, :with_type_de_champ, :with_type_de_champ_private) }
 

spec/system/administrateurs/types_de_champ_spec.rb

@@ -162,7 +162,7 @@ describe 'As an administrateur I can edit types de champ', js: true do
     fill_in 'Options de la liste', with: 'Un menu', fill_options: { clear: :backspace }
     check "Proposer une option « autre » avec un texte libre"
 
-    wait_until { procedure.active_revision.types_de_champ_public.first.drop_down_list_options == ['', 'Un menu'] }
+    wait_until { procedure.active_revision.types_de_champ_public.first.drop_down_options == ['Un menu'] }
     wait_until { procedure.active_revision.types_de_champ_public.first.drop_down_other == "1" }
     expect(page).to have_content('Formulaire enregistré')
 

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

@@ -85,7 +85,7 @@ describe 'shared/dossiers/edit', type: :view do
     let(:types_de_champ_public) { [{ type: :multiple_drop_down_list, options: }] }
     let(:champ) { dossier.champs.first }
     let(:type_de_champ) { champ.type_de_champ }
-    let(:options) { type_de_champ.drop_down_list_options }
+    let(:options) { type_de_champ.drop_down_options }
     let(:enabled_options) { type_de_champ.drop_down_list_enabled_non_empty_options }
 
     context 'when the list is short' do