Merge pull request #7200 from tchak/custom-form-input-name

refactor(dossier): use champ id as champ attributes key

app/controllers/application_controller.rb

@@ -364,4 +364,20 @@ class ApplicationController < ActionController::Base
   def set_customizable_view_path
     prepend_view_path "app/custom_views"
   end
+
+  # Extract a value from params based on the "path"
+  #
+  # params: { dossiers: { champs_attributes: { 1234 => { value: "hello" } } } }
+  #
+  # Usage: read_param_value("dossiers[champs_attributes][1234]", "value")
+  def read_param_value(path, name)
+    parts = path.split(/\[|\]\[|\]/) + [name]
+    parts.reduce(params) do |value, part|
+      if part.to_i != 0
+        value[part.to_i] || value[part]
+      else
+        value[part]
+      end
+    end
+  end
 end

app/controllers/champs/carte_controller.rb

@@ -2,7 +2,6 @@ class Champs::CarteController < ApplicationController
   before_action :authenticate_logged_user!
 
   def index
-    @selector = ".carte-#{params[:champ_id]}"
     @champ = policy_scope(Champ).find(params[:champ_id])
     @focus = params[:focus].present?
   end

app/controllers/champs/dossier_link_controller.rb

@@ -2,12 +2,7 @@ class Champs::DossierLinkController < ApplicationController
   before_action :authenticate_logged_user!
 
   def show
-    @position = params[:position]
-
-    if params[:dossier].key?(:champs_attributes)
-      @dossier_id = params[:dossier][:champs_attributes][params[:position]][:value]
-    else
-      @dossier_id = params[:dossier][:champs_private_attributes][params[:position]][:value]
-    end
+    @champ = policy_scope(Champ).find(params[:champ_id])
+    @linked_dossier_id = read_param_value(@champ.input_name, 'value')
   end
 end

app/controllers/champs/repetition_controller.rb

@@ -3,13 +3,6 @@ class Champs::RepetitionController < ApplicationController
 
   def show
     @champ = policy_scope(Champ).includes(:champs).find(params[:champ_id])
-    @position = params[:position]
-    @champ.add_row
-
-    if @champ.private?
-      @attribute = "dossier[champs_private_attributes][#{@position}][champs_attributes]"
-    else
-      @attribute = "dossier[champs_attributes][#{@position}][champs_attributes]"
-    end
+    @champs = @champ.add_row
   end
 end

app/controllers/champs/siret_controller.rb

@@ -2,9 +2,9 @@ class Champs::SiretController < ApplicationController
   before_action :authenticate_logged_user!
 
   def show
-    @position = params[:position]
-    extract_siret
-    find_etablisement
+    @champ = policy_scope(Champ).find(params[:champ_id])
+    @siret = read_param_value(@champ.input_name, 'value')
+    @etablissement = @champ.etablissement
 
     if @siret.empty?
       return clear_siret_and_etablissement
@@ -34,22 +34,6 @@ class Champs::SiretController < ApplicationController
 
   private
 
-  def extract_siret
-    if params[:dossier].key?(:champs_attributes)
-      @siret = params[:dossier][:champs_attributes][@position][:value]
-      @attribute = "dossier[champs_attributes][#{@position}][etablissement_attributes]"
-    else
-      @siret = params[:dossier][:champs_private_attributes][@position][:value]
-      @attribute = "dossier[champs_private_attributes][#{@position}][etablissement_attributes]"
-    end
-  end
-
-  def find_etablisement
-    @champ = policy_scope(Champ).find(params[:champ_id])
-    @etablissement = @champ.etablissement
-    @procedure_id = @champ.dossier.procedure.id
-  end
-
   def find_etablissement_with_siret
     APIEntrepriseService.create_etablissement(@champ, @siret, current_user.id)
   end

app/helpers/application_helper.rb

@@ -60,15 +60,6 @@ module ApplicationHelper
     end
   end
 
-  def render_champ(champ)
-    champ_selector = "##{champ.input_group_id}"
-    form_html = render 'shared/dossiers/edit', dossier: champ.dossier
-    champ_html = Nokogiri::HTML.fragment(form_html).at_css(champ_selector).to_s
-    # rubocop:disable Rails/OutputSafety
-    raw("document.querySelector('#{champ_selector}').outerHTML = \"#{escape_javascript(champ_html)}\";")
-    # rubocop:enable Rails/OutputSafety
-  end
-
   def remove_element(selector, timeout: 0, inner: false)
     script = "(function() {";
     script << "var el = document.querySelector('#{selector}');"

app/models/champ.rb

@@ -137,6 +137,23 @@ class Champ < ApplicationRecord
     "#{html_id}-input"
   end
 
+  # A predictable string to use when generating an input name for this champ.
+  #
+  # Rail's FormBuilder can auto-generate input names, using the form "dossier[champs_attributes][5]",
+  # where [5] is the index of the field in the form.
+  # However the field index makes it difficult to render a single field, independent from the ordering of the others.
+  #
+  # Luckily, this is only used to make the name unique, but the actual value is ignored when Rails parses nested
+  # attributes. So instead of the field index, this method uses the champ id; which gives us an independent and
+  # predictable input name.
+  def input_name
+    if parent_id
+      "#{parent.input_name}[#{champs_attributes_accessor}][#{id}]"
+    else
+      "dossier[#{champs_attributes_accessor}][#{id}]"
+    end
+  end
+
   def labelledby_id
     "#{html_id}-label"
   end
@@ -169,6 +186,14 @@ class Champ < ApplicationRecord
     "#{stable_id}-#{id}"
   end
 
+  def champs_attributes_accessor
+    if private?
+      "champs_private_attributes"
+    else
+      "champs_attributes"
+    end
+  end
+
   def needs_dossier_id?
     !dossier_id && parent_id
   end

app/models/champs/repetition_champ.rb

@@ -28,12 +28,15 @@ class Champs::RepetitionChamp < Champ
   end
 
   def add_row
+    added_champs = []
     transaction do
       row = (blank? ? -1 : champs.last.row) + 1
       type_de_champ.types_de_champ.each do |type_de_champ|
-        self.champs << type_de_champ.champ.build(row: row)
+        added_champs << type_de_champ.champ.build(row: row)
       end
+      self.champs << added_champs
     end
+    added_champs
   end
 
   def blank?

app/views/champs/carte/index.js.erb

@@ -1,6 +1,6 @@
 <%= render_flash(timeout: 5000, fixed: true) %>
 
-<%= render_to_element("#{@selector} + .geo-areas",
+<%= render_to_element("##{@champ.input_group_id} .geo-areas",
   partial: 'shared/champs/carte/geo_areas',
   locals: { champ: @champ, editing: true }) %>
 

app/views/champs/dossier_link/show.js.erb

@@ -1,3 +1,3 @@
-<%= render_to_element(".dossier-link-#{@position} .help-block",
+<%= render_to_element("##{@champ.input_group_id} .help-block",
   partial: 'shared/champs/dossier_link/help_block',
-  locals: { id: @dossier_id }) %>
+  locals: { id: @linked_dossier_id }) %>

app/views/champs/piece_justificative/show.js.erb

@@ -1,4 +1,6 @@
-<%= render_champ(@champ) %>
+<%= fields_for @champ.input_name, @champ do |form| %>
+  <%= render_to_element("##{@champ.input_group_id}", partial: "shared/dossiers/editable_champs/editable_champ", locals: { champ: @champ, form: form }, outer: true) %>
+<% end %>
 
 <% attachment = @champ.piece_justificative_file.attachment %>
 <% if attachment.virus_scanner.pending? %>

app/views/champs/repetition/_show.html.haml

@@ -1,16 +0,0 @@
-- champs = champ.rows.last
-- if champs.present?
-  - index = (champ.rows.size - 1) * champs.size
-  - row_dom_id = "row-#{SecureRandom.hex(4)}"
-  %div{ class: "row row-#{champs.first.row}", id: row_dom_id }
-    -# Tell the controller which DOM element should be removed once the row deletion is successful
-    = hidden_field_tag 'deleted_row_dom_ids[]', row_dom_id, disabled: true
-
-    - champs.each.with_index(index) do |champ, index|
-      = fields_for "#{attribute}[#{index}]", champ do |form|
-        = render partial: "shared/dossiers/editable_champs/editable_champ", locals: { champ: champ, form: form }
-        = form.hidden_field :id
-        = form.hidden_field :_destroy, disabled: true
-    .flex.row-reverse
-      %button.button.danger.remove-row
-        Supprimer l’élément

app/views/champs/repetition/show.js.erb

@@ -1,3 +1,3 @@
-<%= append_to_element(".repetition-#{@position}",
-  partial: 'champs/repetition/show',
-  locals: { champ: @champ, attribute: @attribute }) %>
+<%= fields_for @champ.input_name, @champ do |form| %>
+  <%= append_to_element("##{@champ.input_group_id} .repetition", partial: 'shared/dossiers/editable_champs/repetition_row', locals: { form: form, champ: @champ, row: @champs }) %>
+<% end %>

app/views/champs/siret/show.js.erb

@@ -1,6 +1,3 @@
-<%= render_to_element(".siret-info-#{@position}",
+<%= render_to_element("##{@champ.input_group_id} .siret-info",
   partial: 'shared/champs/siret/etablissement',
-  locals: {
-    siret: @siret,
-    attribute: @attribute,
-    etablissement: @etablissement }) %>
+  locals: { siret: @siret, etablissement: @etablissement }) %>

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

@@ -6,10 +6,9 @@
   - if @dossier.champs_private.present?
     %section
       = form_for @dossier, url: annotations_instructeur_dossier_path(@dossier.procedure, @dossier), html: { class: 'form' } do |f|
-        = f.fields_for :champs_private, f.object.champs_private do |champ_form|
-          - champ = champ_form.object
-          = render partial: "shared/dossiers/editable_champs/editable_champ",
-            locals: { champ: champ, form: champ_form, seen_at: @annotations_privees_seen_at }
+        - @dossier.champs_private.each do |champ|
+          = fields_for champ.input_name, champ do |form|
+            = render partial: "shared/dossiers/editable_champs/editable_champ", locals: { form: form, champ: champ, seen_at: @annotations_privees_seen_at }
 
         .send-wrapper
           = f.submit 'Sauvegarder', class: 'button primary send', data: { disable: true }

app/views/shared/dossiers/_edit.html.haml

@@ -35,10 +35,9 @@
         dossier.procedure.groupe_instructeurs.order(:label).map { |gi| [gi.label, gi.id] },
         { include_blank: dossier.brouillon? }
 
-    = f.fields_for :champs, dossier.champs do |champ_form|
-      - champ = champ_form.object
-      = render partial: "shared/dossiers/editable_champs/editable_champ",
-        locals: { champ: champ, form: champ_form }
+    - dossier.champs.each do |champ|
+      = fields_for champ.input_name, champ do |form|
+        = render partial: "shared/dossiers/editable_champs/editable_champ", locals: { form: form, champ: champ }
 
     - if !dossier.for_procedure_preview?
       .dossier-edit-sticky-footer

app/views/shared/dossiers/editable_champs/_carte.html.haml

@@ -1,4 +1,4 @@
-= react_component("MapEditor", { featureCollection: champ.to_feature_collection, url: champs_carte_features_path(champ), options: champ.render_options }, class: "carte-#{champ.id}")
+= react_component("MapEditor", featureCollection: champ.to_feature_collection, url: champs_carte_features_path(champ), options: champ.render_options)
 
 .geo-areas
   = render partial: 'shared/champs/carte/geo_areas', locals: { champ: champ, editing: true }

app/views/shared/dossiers/editable_champs/_dossier_link.html.haml

@@ -1,11 +1,11 @@
-.dossier-link{ class: "dossier-link-#{form.index}" }
+.dossier-link
   = form.number_field :value,
     id: champ.input_id,
     aria: { describedby: champ.describedby_id },
     placeholder: "Numéro de dossier",
     autocomplete: 'off',
     required: champ.mandatory?,
-    data: { remote: true, url: champs_dossier_link_path(form.index) }
+    data: { remote: true, url: champs_dossier_link_path(champ.id) }
 
   .help-block
     = render partial: 'shared/champs/dossier_link/help_block', locals: { id: champ.value }

app/views/shared/dossiers/editable_champs/_drop_down_list.html.haml

@@ -19,4 +19,4 @@
     = form.select :value, champ.options, { selected: champ.selected}, required: champ.mandatory?, id: champ.input_id, aria: { describedby: champ.describedby_id }
 
   - if champ.drop_down_other?
-    = render partial: "shared/dossiers/drop_down_other_input", locals: { form: form, champ: champ }
+    = render partial: "shared/dossiers/editable_champs/drop_down_other_input", locals: { form: form, champ: champ }

app/views/shared/dossiers/editable_champs/_editable_champ.html.haml

@@ -8,5 +8,6 @@
     = render partial: 'shared/dossiers/editable_champs/champ_label', locals: { form: form, champ: champ, seen_at: defined?(seen_at) ? seen_at : nil }
   - if champ.type_champ == "titre_identite"
     %p.notice  Carte nationale d’identité (uniquement le recto), passeport, titre de séjour ou autre justificatif d’identité. Formats acceptés : jpg/png
-  = render partial: "shared/dossiers/editable_champs/#{champ.type_champ}",
-    locals: { champ: champ, form: form }
+
+  = form.hidden_field :id, value: champ.id
+  = render partial: "shared/dossiers/editable_champs/#{champ.type_champ}", locals: { form: form, champ: champ }

app/views/shared/dossiers/editable_champs/_repetition.html.haml

@@ -1,24 +1,9 @@
-%div{ class: "repetition-#{form.index}" }
+.repetition
   - champ.rows.each do |champs|
-    - row_dom_id = "row-#{SecureRandom.hex(4)}"
-    %div{ class: "row row-#{champs.first.row}", id: row_dom_id }
-      -# Tell the controller which DOM element should be removed once the row deletion is successful
-      = hidden_field_tag 'deleted_row_dom_ids[]', row_dom_id, disabled: true
-
-      - champs.each do |champ|
-        = form.fields_for :champs, champ do |form|
-          = render partial: 'shared/dossiers/editable_champs/editable_champ', locals: { champ: form.object, form: form }
-          = form.hidden_field :_destroy, disabled: true
-      .flex.row-reverse
-        - if champ.persisted?
-          %button.button.danger.remove-row{ type: :button }
-            Supprimer l’élément
-        - else
-          %button.button.danger{ type: :button }
-            Supprimer l’élément
+    = render partial: 'shared/dossiers/editable_champs/repetition_row', locals: { form: form, champ: champ, row: champs }
 
 - if champ.persisted?
-  = link_to champs_repetition_path(form.index), class: 'button add-row', data: { remote: true, disable: true, method: 'POST', params: { champ_id: champ&.id }.to_query } do
+  = link_to champs_repetition_path(champ.id), class: 'button add-row', data: { remote: true, disable: true, method: 'POST' } do
     %span.icon.add
     Ajouter un élément pour « #{champ.libelle} »
 - else

app/views/shared/dossiers/editable_champs/_repetition_row.html.haml

@@ -0,0 +1,13 @@
+- row_dom_id = "row-#{SecureRandom.hex(4)}"
+.row{ id: row_dom_id }
+  -# Tell the controller which DOM element should be removed once the row deletion is successful
+  = hidden_field_tag 'deleted_row_dom_ids[]', row_dom_id, disabled: true
+
+  - row.each do |champ|
+    = fields_for champ.input_name, champ do |form|
+      = render partial: 'shared/dossiers/editable_champs/editable_champ', locals: { form: form, champ: champ }
+      = form.hidden_field :_destroy, disabled: true
+
+  .flex.row-reverse
+    %button.button.danger.remove-row{ type: :button }
+      Supprimer l’élément

app/views/shared/dossiers/editable_champs/_siret.html.haml

@@ -2,11 +2,11 @@
   id: champ.input_id,
   aria: { describedby: champ.describedby_id },
   placeholder: champ.libelle,
-  data: { remote: true, debounce: true, url: champs_siret_path(form.index), params: { champ_id: champ&.id }.to_query, spinner: true },
+  data: { remote: true, debounce: true, url: champs_siret_path(champ.id), spinner: true },
   required: champ.mandatory?,
   pattern: "[0-9]{14}",
   title: "Le numéro de SIRET doit comporter exactement 14 chiffres"
 .spinner.right.hidden
-.siret-info{ class: "siret-info-#{form.index}" }
+.siret-info
   - if champ.etablissement.present?
     = render partial: 'shared/dossiers/editable_champs/etablissement_titre', locals: { etablissement: champ.etablissement }

config/routes.rb

@@ -141,18 +141,17 @@ Rails.application.routes.draw do
   end
 
   namespace :champs do
-    get ':position/siret', to: 'siret#show', as: :siret
-    get ':position/dossier_link', to: 'dossier_link#show', as: :dossier_link
-    post ':position/carte', to: 'carte#show', as: :carte
+    get ':champ_id/siret', to: 'siret#show', as: :siret
+    get ':champ_id/dossier_link', to: 'dossier_link#show', as: :dossier_link
+    post ':champ_id/carte', to: 'carte#show', as: :carte
+    post ':champ_id/repetition', to: 'repetition#show', as: :repetition
 
     get ':champ_id/carte/features', to: 'carte#index', as: :carte_features
     post ':champ_id/carte/features', to: 'carte#create'
-    post ':champ_id/carte/features/import', to: 'carte#import'
     patch ':champ_id/carte/features/:id', to: 'carte#update'
     delete ':champ_id/carte/features/:id', to: 'carte#destroy'
 
-    post ':position/repetition', to: 'repetition#show', as: :repetition
-    put 'piece_justificative/:champ_id', to: 'piece_justificative#update', as: :piece_justificative
+    put ':champ_id/piece_justificative', to: 'piece_justificative#update', as: :piece_justificative
   end
 
   resources :attachments, only: [:show, :destroy]

spec/controllers/champs/dossier_link_controller_spec.rb

@@ -1,22 +1,26 @@
 describe Champs::DossierLinkController, type: :controller do
   let(:user) { create(:user) }
-  let(:procedure) { create(:procedure, :published) }
+  let(:procedure) { create(:procedure, :published, :with_dossier_link) }
 
   describe '#show' do
     let(:dossier) { create(:dossier, user: user, procedure: procedure) }
+    let(:champ) { dossier.champs.first }
 
     context 'when user is connected' do
       render_views
       before { sign_in user }
 
+      let(:champs_attributes) do
+        champ_attributes = []
+        champ_attributes[champ.id] = { value: dossier_id }
+        champ_attributes
+      end
       let(:params) do
         {
+          champ_id: champ.id,
           dossier: {
-            champs_attributes: {
-              '1' => { value: dossier_id.to_s }
+            champs_attributes: champs_attributes
           }
-          },
-          position: '1'
         }
       end
       let(:dossier_id) { dossier.id }
@@ -30,7 +34,7 @@ describe Champs::DossierLinkController, type: :controller do
           expect(response.body).to include('Dossier en brouillon')
           expect(response.body).to include(procedure.libelle)
           expect(response.body).to include(procedure.organisation)
-          expect(response.body).to include('.dossier-link-1 .help-block')
+          expect(response.body).to include("##{champ.input_group_id} .help-block")
         end
       end
 
@@ -42,14 +46,14 @@ describe Champs::DossierLinkController, type: :controller do
 
         it 'renders error message' do
           expect(response.body).to include('Ce dossier est inconnu')
-          expect(response.body).to include('.dossier-link-1 .help-block')
+          expect(response.body).to include("##{champ.input_group_id} .help-block")
         end
       end
     end
 
     context 'when user is not connected' do
       before do
-        get :show, params: { position: '1' }, format: :js, xhr: true
+        get :show, params: { champ_id: champ.id }, format: :js, xhr: true
       end
 
       it { expect(response.code).to eq('401') }

spec/controllers/champs/siret_controller_spec.rb

@@ -1,23 +1,22 @@
 describe Champs::SiretController, type: :controller do
   let(:user) { create(:user) }
-  let(:procedure) do
-    tdc_siret = build(:type_de_champ_siret, procedure: nil)
-    create(:procedure, :published, types_de_champ: [tdc_siret])
-  end
+  let(:procedure) { create(:procedure, :published, :with_siret) }
 
   describe '#show' do
     let(:dossier) { create(:dossier, user: user, procedure: procedure) }
     let(:champ) { dossier.champs.first }
 
+    let(:champs_attributes) do
+      champ_attributes = []
+      champ_attributes[champ.id] = { value: siret }
+      champ_attributes
+    end
     let(:params) do
       {
         champ_id: champ.id,
         dossier: {
-          champs_attributes: {
-            '1' => { value: siret.to_s }
+          champs_attributes: champs_attributes
         }
-        },
-        position: '1'
       }
     end
     let(:siret) { '' }
@@ -47,7 +46,7 @@ describe Champs::SiretController, type: :controller do
         end
 
         it 'clears any information or error message' do
-          expect(response.body).to include('.siret-info-1')
+          expect(response.body).to include("##{champ.input_group_id} .siret-info")
           expect(response.body).to include('innerHTML = ""')
         end
       end
@@ -120,7 +119,7 @@ describe Champs::SiretController, type: :controller do
     end
 
     context 'when user is not signed in' do
-      subject! { get :show, params: { position: '1' }, format: :js, xhr: true }
+      subject! { get :show, params: { champ_id: champ.id }, format: :js, xhr: true }
 
       it { expect(response.code).to eq('401') }
     end

spec/factories/procedure.rb

@@ -154,6 +154,12 @@ FactoryBot.define do
       end
     end
 
+    trait :with_siret do
+      after(:build) do |procedure, _evaluator|
+        build(:type_de_champ_siret, procedure: procedure)
+      end
+    end
+
     trait :with_yes_no do
       after(:build) do |procedure, _evaluator|
         build(:type_de_champ_yes_no, procedure: procedure)

spec/system/users/brouillon_spec.rb

@@ -113,7 +113,7 @@ describe 'The user' do
 
     click_on 'Ajouter un élément pour'
 
-    within '.row-1' do
+    within '.repetition .row:first-child' do
       fill_in('sub type de champ', with: 'un autre texte')
     end
 
@@ -124,7 +124,7 @@ describe 'The user' do
 
     expect(page).to have_content('Supprimer', count: 2)
 
-    within '.row-1' do
+    within '.repetition .row:first-child' do
       click_on 'Supprimer l’élément'
     end