Merge pull request #8390 from tchak/refactor-repetitions-row-id-use

refactor(repetition): use row_id instead of row

app/components/editable_champ/repetition_row_component/repetition_row_component.html.haml

@@ -1,9 +1,9 @@
-- row_dom_id = "row-#{SecureRandom.hex(4)}"
-.row{ id: row_dom_id }
+- row_id = @row.first.row_id
+.row{ id: row_id }
   - @row.each do |champ|
     = fields_for champ.input_name, champ do |form|
       = render EditableChamp::EditableChampComponent.new form: form, champ: champ, seen_at: @seen_at
 
   .flex.row-reverse{ 'data-turbo': 'true' }
-    = link_to champs_repetition_path(@champ.id, champ_ids: @row.map(&:id), row_id: row_dom_id), data: { turbo_method: :delete }, class: 'fr-btn fr-btn--sm fr-btn--tertiary fr-text-action-high--red-marianne' do
+    = link_to champs_repetition_path(@champ.id, row_id:), data: { turbo_method: :delete }, class: 'fr-btn fr-btn--sm fr-btn--tertiary fr-text-action-high--red-marianne' do
       Supprimer l’élément

app/controllers/champs/repetition_controller.rb

@@ -8,7 +8,7 @@ class Champs::RepetitionController < ApplicationController
 
   def remove
     champ = policy_scope(Champ).includes(:champs).find(params[:champ_id])
-    champ.champs.where(id: params[:champ_ids]).destroy_all
+    champ.champs.where(row_id: params[:row_id]).destroy_all
     @row_id = params[:row_id]
   end
 end

app/graphql/mutations/dossier_modifier_annotation.rb

@@ -40,13 +40,11 @@ module Mutations
     end
 
     def find_annotation(dossier, annotation_id)
-      stable_id, row = Champ.decode_typed_id(annotation_id)
+      stable_id, row_id = Champ.decode_typed_id(annotation_id)
 
       Champ.joins(:type_de_champ).find_by(type_de_champ: {
-        type_champ: annotation_type_champ,
-        stable_id:,
-        private: true
-      }, private: true, row:, dossier:)
+        type_champ: annotation_type_champ, stable_id:, private: true
+      }, private: true, row_id:, dossier:)
     end
 
     def annotation_type_champ

app/graphql/mutations/dossier_modifier_annotation_ajouter_ligne.rb

@@ -26,13 +26,11 @@ module Mutations
     private
 
     def find_annotation(dossier, annotation_id)
-      stable_id, row = Champ.decode_typed_id(annotation_id)
+      stable_id, row_id = Champ.decode_typed_id(annotation_id)
 
       Champ.joins(:type_de_champ).find_by(type_de_champ: {
-        type_champ: TypeDeChamp.type_champs.fetch(:repetition),
-        stable_id: stable_id,
-        private: true
-      }, private: true, row: row, dossier: dossier)
+        type_champ: TypeDeChamp.type_champs.fetch(:repetition), stable_id:, private: true
+      }, private: true, row_id:, dossier:)
     end
   end
 end

app/graphql/schema.graphql

@@ -2177,6 +2177,7 @@ type Revision {
 
 type Row {
   champs: [Champ!]!
+  id: ID!
 }
 
 type SelectionUtilisateur implements GeoArea {

app/graphql/types/champs/repetition_champ_type.rb

@@ -3,6 +3,7 @@ module Types::Champs
     implements Types::ChampType
 
     class Row < Types::BaseObject
+      global_id_field :id
       field :champs, [Types::ChampType], null: false
     end
 
@@ -18,7 +19,7 @@ module Types::Champs
     def rows
       Loaders::Association.for(object.class, champs: :type_de_champ).load(object).then do |champs|
         object.association(:champs).target = champs.filter(&:visible?)
-        object.rows.map { { champs: _1 } }
+        object.rows.map { { champs: _1, id: GraphQL::Schema::UniqueWithinType.encode('Row', _1.first.row_id) } }
       end
     end
   end

app/models/champ.rb

@@ -22,6 +22,7 @@
 #  type_de_champ_id               :integer
 #
 class Champ < ApplicationRecord
+  self.ignored_columns = [:row]
   belongs_to :dossier, inverse_of: false, touch: true, optional: false
   belongs_to :type_de_champ, inverse_of: :champ, optional: false
   belongs_to :parent, class_name: 'Champ', optional: true
@@ -77,7 +78,7 @@ class Champ < ApplicationRecord
     includes(:type_de_champ)
       .joins(dossier: { revision: :revision_types_de_champ })
       .where('procedure_revision_types_de_champ.type_de_champ_id = champs.type_de_champ_id')
-      .order(:row, :position)
+      .order(:row_id, :position)
   end
   scope :public_ordered, -> { public_only.ordered }
   scope :private_ordered, -> { private_only.ordered }
@@ -146,8 +147,8 @@ class Champ < ApplicationRecord
   end
 
   def to_typed_id
-    if row.present?
-      GraphQL::Schema::UniqueWithinType.encode('Champ', "#{stable_id}|#{row}")
+    if row_id.present?
+      GraphQL::Schema::UniqueWithinType.encode('Champ', "#{stable_id}|#{row_id}")
     else
       type_de_champ.to_typed_id
     end
@@ -225,7 +226,7 @@ class Champ < ApplicationRecord
   end
 
   def clone
-    champ_attributes = [:parent_id, :private, :row, :row_id, :type, :type_de_champ_id]
+    champ_attributes = [:parent_id, :private, :row_id, :type, :type_de_champ_id]
     value_attributes = private? ? [] : [:value, :value_json, :data, :external_id]
     relationships = private? ? [] : [:etablissement, :geo_areas]
 
@@ -237,7 +238,7 @@ class Champ < ApplicationRecord
   private
 
   def champs_for_condition
-    dossier.champs.filter { _1.row.nil? || _1.row == row }
+    dossier.champs.filter { _1.row_id.nil? || _1.row_id == row_id }
   end
 
   def html_id

app/models/champs/repetition_champ.rb

@@ -26,16 +26,15 @@ class Champs::RepetitionChamp < Champ
   delegate :libelle_for_export, to: :type_de_champ
 
   def rows
-    champs.group_by(&:row).values
+    champs.group_by(&:row_id).values
   end
 
   def add_row(revision)
     added_champs = []
     transaction do
-      row = (blank? ? -1 : champs.last.row) + 1
       row_id = ULID.generate
       revision.children_of(type_de_champ).each do |type_de_champ|
-        added_champs << type_de_champ.champ.build(row:, row_id:)
+        added_champs << type_de_champ.champ.build(row_id:)
       end
       self.champs << added_champs
     end

app/models/concerns/dossier_rebase_concern.rb

@@ -116,8 +116,8 @@ module DossierRebaseConcern
         .where(type_de_champ: { stable_id: parent_stable_id })
 
       champs_repetition.each do |champ_repetition|
-        champ_repetition.champs.index_by(&:row).each do |(row, champ)|
-          create_champ(target_coordinate, champ_repetition, row:, row_id: champ.row_id)
+        champ_repetition.champs.map(&:row_id).uniq.each do |row_id|
+          create_champ(target_coordinate, champ_repetition, row_id:)
         end
       end
     else
@@ -125,8 +125,8 @@ module DossierRebaseConcern
     end
   end
 
-  def create_champ(target_coordinate, parent, row: nil, row_id: nil)
-    params = { revision: target_coordinate.revision, row:, row_id: }.compact
+  def create_champ(target_coordinate, parent, row_id: nil)
+    params = { revision: target_coordinate.revision, row_id: }.compact
     champ = target_coordinate
       .type_de_champ
       .build_champ(params)

app/models/dossier_preloader.rb

@@ -112,7 +112,7 @@ class DossierPreloader
     end
 
     parent.association(name).target = champs.sort_by do |champ|
-      [champ.row, positions[dossier.revision_id][champ.type_de_champ_id]]
+      [champ.row_id, positions[dossier.revision_id][champ.type_de_champ_id]]
     end
 
     # Load children champs

app/models/geo_area.rb

@@ -66,7 +66,7 @@ class GeoArea < ApplicationRecord
         id: id,
         champ_label: champ.libelle,
         champ_id: champ.stable_id,
-        champ_row: champ.row,
+        champ_row: champ.row_id,
         champ_private: champ.private?,
         dossier_id: champ.dossier_id
       ).compact

db/migrate/20230110181415_remove_champs_row_index.rb

@@ -0,0 +1,5 @@
+class RemoveChampsRowIndex < ActiveRecord::Migration[6.1]
+  def change
+    remove_index :champs, name: "index_champs_on_type_de_champ_id_and_dossier_id_and_row"
+  end
+end

db/migrate/20230110181426_add_champs_row_id_index.rb

@@ -0,0 +1,7 @@
+class AddChampsRowIdIndex < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+
+  def change
+    add_index :champs, [:type_de_champ_id, :dossier_id, :row_id], unique: true, algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2023_01_09_140138) do
+ActiveRecord::Schema.define(version: 2023_01_10_181426) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
@@ -240,7 +240,7 @@ ActiveRecord::Schema.define(version: 2023_01_09_140138) do
     t.index ["row"], name: "index_champs_on_row"
     t.index ["row_id"], name: "index_champs_on_row_id"
     t.index ["type"], name: "index_champs_on_type"
-    t.index ["type_de_champ_id", "dossier_id", "row"], name: "index_champs_on_type_de_champ_id_and_dossier_id_and_row", unique: true
+    t.index ["type_de_champ_id", "dossier_id", "row_id"], name: "index_champs_on_type_de_champ_id_and_dossier_id_and_row_id", unique: true
     t.index ["type_de_champ_id"], name: "index_champs_on_type_de_champ_id"
   end
 

spec/factories/champ.rb

@@ -231,9 +231,10 @@ FactoryBot.define do
         parent = revision.revision_types_de_champ.find { |rtdc| rtdc.type_de_champ == champ_repetition.type_de_champ }
         types_de_champ = revision.revision_types_de_champ.filter { |rtdc| rtdc.parent == parent }.map(&:type_de_champ)
 
-        evaluator.rows.times do |row|
+        evaluator.rows.times do
+          row_id = ULID.generate
           champ_repetition.champs << types_de_champ.map do |type_de_champ|
-            build(:"champ_#{type_de_champ.type_champ}", dossier: champ_repetition.dossier, row: row, type_de_champ: type_de_champ, parent: champ_repetition, private: champ_repetition.private?)
+            build(:"champ_#{type_de_champ.type_champ}", dossier: champ_repetition.dossier, row_id:, type_de_champ: type_de_champ, parent: champ_repetition, private: champ_repetition.private?)
           end
         end
       end

spec/models/champ_spec.rb

@@ -501,14 +501,14 @@ describe Champ do
     let(:champ) { dossier.champs_public.find(&:repetition?) }
     let(:champ_text) { champ.champs.find { |c| c.type_champ == 'text' } }
     let(:champ_integer) { champ.champs.find { |c| c.type_champ == 'integer_number' } }
-    let(:champ_text_attrs) { attributes_for(:champ_text, type_de_champ: tdc_text, row: 1) }
+    let(:champ_text_attrs) { attributes_for(:champ_text, type_de_champ: tdc_text, row_id: ULID.generate) }
 
     before do
       procedure.active_revision.add_type_de_champ(libelle: 'sub integer', type_champ: 'integer_number', parent_stable_id: tdc_repetition.stable_id)
     end
 
     context 'when creating the model directly' do
-      let(:champ_text_row_1) { create(:champ_text, type_de_champ: tdc_text, row: 2, parent: champ, dossier: nil) }
+      let(:champ_text_row_1) { create(:champ_text, type_de_champ: tdc_text, row_id: ULID.generate, parent: champ, dossier: nil) }
 
       it 'associates nested champs to the parent dossier' do
         expect(champ_text_row_1.dossier_id).to eq(champ.dossier_id)

spec/models/dossier_rebase_concern_spec.rb

@@ -278,7 +278,7 @@ describe Dossier do
       # Add two rows then remove previous to last row in order to create a "hole" in the sequence
       repetition_champ.add_row(repetition_champ.dossier.revision)
       repetition_champ.add_row(repetition_champ.dossier.revision)
-      repetition_champ.champs.where(row: repetition_champ.champs.last.row - 1).destroy_all
+      repetition_champ.champs.where(row_id: repetition_champ.rows[-2].first.row_id).destroy_all
       repetition_champ.reload
     end