Merge pull request #11182 from tchak/make-uniq-index-really-uniq

fix(dossier): make row_id unique

app/controllers/attachments_controller.rb

@@ -39,7 +39,7 @@ class AttachmentsController < ApplicationController
 
   def find_champ
     dossier = policy_scope(Dossier).includes(:champs).find(params[:dossier_id])
-    dossier.champs.find_by(stable_id: params[:stable_id], row_id: params[:row_id])
+    dossier.champs.find_by(stable_id: params[:stable_id], row_id: params[:row_id] || Champ::NULL_ROW_ID)
   end
 
   def attachment_options

app/models/champ.rb

@@ -77,6 +77,17 @@ class Champ < ApplicationRecord
     row_id.present? && is_type?(TypeDeChamp.type_champs.fetch(:repetition))
   end
 
+  NULL_ROW_ID = 'N'
+
+  def row_id
+    row_id_or_null = super
+    row_id_or_null == Champ::NULL_ROW_ID ? nil : row_id_or_null
+  end
+
+  def row_id=(row_id_or_nil)
+    super(row_id_or_nil || Champ::NULL_ROW_ID)
+  end
+
   # used for the `required` html attribute
   # check visibility to avoid hidden required input
   # which prevent the form from being sent.

app/models/concerns/dossier_champs_concern.rb

@@ -177,6 +177,22 @@ module DossierChampsConcern
     super.tap { reset_champs_cache }
   end
 
+  # FIXME: this is a temporary fix to remove duplicated champs with the same nil row_id
+  def tmp_fix_uniq_row_ids
+    duplicated_champ_ids = champs.where(row_id: [Champ::NULL_ROW_ID, nil])
+      .order(id: :desc)
+      .select(:id, :stream, :stable_id, :row_id)
+      .group_by { "#{_1.stream}-#{_1.public_id}" }
+      .values
+      .flat_map { _1[1..].map(&:id) }
+    Dossier.transaction do
+      if duplicated_champ_ids.present?
+        Dossier.no_touching { champs.where(id: duplicated_champ_ids).destroy_all }
+      end
+      champs.where(row_id: nil).update_all(row_id: Champ::NULL_ROW_ID)
+    end
+  end
+
   private
 
   def champs_by_public_id
@@ -210,10 +226,13 @@ module DossierChampsConcern
   def champ_upsert_by!(type_de_champ, row_id)
     check_valid_row_id_on_write?(type_de_champ, row_id)
     champ_attributes = type_de_champ.params_for_champ
-    # TODO: Once we have the right index in place, we should change this to use `create_or_find_by` instead of `find_or_create_by`
+
+    # FIXME: this is a temporary fix to remove duplicated champs with the same nil row_id
+    tmp_fix_uniq_row_ids
+
     champ = champs
       .create_with(**champ_attributes)
-      .find_or_create_by!(stable_id: type_de_champ.stable_id, row_id:)
+      .create_or_find_by!(stable_id: type_de_champ.stable_id, row_id: row_id || Champ::NULL_ROW_ID, stream: 'main')
 
     # Needed when a revision change the champ type in this case, we reset the champ data
     if champ.type != champ_attributes[:type]

app/models/dossier.rb

@@ -1039,7 +1039,7 @@ class Dossier < ApplicationRecord
           type_de_champ.build_champ(dossier: self, row_id: ULID.generate)
         end
       else
-        type_de_champ.build_champ(dossier: self)
+        type_de_champ.build_champ(dossier: self, row_id: Champ::NULL_ROW_ID)
       end
     end
   end

app/tasks/maintenance/t20241216remove_non_unique_champs_task.rb

@@ -8,15 +8,11 @@ module Maintenance
     include StatementsHelpersConcern
 
     def collection
-      Dossier.state_not_brouillon.includes(champs: true)
+      Dossier.includes(champs: true)
     end
 
     def process(dossier)
-      dossier.champs.filter { _1.row_id.nil? }.group_by(&:public_id).each do |_, champs|
-        if champs.size > 1
-          champs.sort_by(&:id)[1..].each(&:destroy)
-        end
-      end
+      dossier.tmp_fix_uniq_row_ids
     end
   end
 end

spec/factories/dossier.rb

@@ -305,7 +305,7 @@ def dossier_factory_create_champ_or_repetition(type_de_champ, dossier)
   end
 end
 
-def dossier_factory_create_champ(type_de_champ, dossier, row_id: nil)
+def dossier_factory_create_champ(type_de_champ, dossier, row_id: Champ::NULL_ROW_ID)
   return unless type_de_champ.fillable?
 
   value = if type_de_champ.drop_down_list?

spec/tasks/maintenance/t20241216remove_non_unique_champs_task_spec.rb

@@ -9,14 +9,16 @@ module Maintenance
       let(:procedure) { create(:procedure, types_de_champ_public: [{}]) }
       let(:dossier) { create(:dossier, :with_populated_champs, procedure:) }
       let(:type_de_champ) { dossier.revision.types_de_champ_public.first }
-      let(:champ_id) { dossier.champs.first.id }
 
       before {
         dossier.champs.create(**type_de_champ.params_for_champ)
       }
 
-      it { expect { subject }.to change { dossier.reload.project_champ(type_de_champ).id }.from(dossier.champs.last.id).to(champ_id) }
-      it { expect { subject }.to change { Champ.count }.by(-1) }
+      it { expect { subject }.not_to change { dossier.reload.updated_at } }
+      it { expect { subject }.not_to change { dossier.champs.order(id: :desc).first.id } }
+      it { expect { subject }.to change { dossier.champs.order(:id).first.id } }
+      it { expect { subject }.to change { dossier.champs.count }.by(-1) }
+      it { expect { subject }.to change { dossier.champs.where(row_id: nil).count }.from(1).to(0) }
     end
   end
 end