Merge pull request #6045 from betagouv/cleanup-inverse-relationships

Nettoyage de la relation inverse Dossier ↔︎ Champs (#6045)

app/controllers/users/dossiers_controller.rb

@@ -360,12 +360,12 @@ module Users
 
       if champs_params[:dossier]
         @dossier.assign_attributes(champs_params[:dossier])
-        # FIXME in some cases a removed repetition bloc row is submitted.
-        # In this case it will be trated as a new records and action will fail.
+        # FIXME: in some cases a removed repetition bloc row is submitted.
+        # In this case it will be treated as a new record, and the action will fail.
         @dossier.champs.filter(&:repetition?).each do |champ|
           champ.champs = champ.champs.filter(&:persisted?)
         end
-        if @dossier.champs.any?(&:changed?)
+        if @dossier.champs.any?(&:changed_for_autosave?)
           @dossier.last_champ_updated_at = Time.zone.now
         end
         if !@dossier.save

app/models/champ.rb

@@ -18,7 +18,7 @@
 #  type_de_champ_id               :integer
 #
 class Champ < ApplicationRecord
-  belongs_to :dossier, -> { with_discarded }, inverse_of: :champs, touch: true, optional: false
+  belongs_to :dossier, -> { with_discarded }, 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
   has_many :commentaires

app/models/dossier.rb

@@ -65,8 +65,8 @@ class Dossier < ApplicationRecord
   has_one_attached :justificatif_motivation
   has_one_attached :pdf_export_for_instructeur
 
-  has_many :champs, -> { root.public_ordered }, inverse_of: :dossier, dependent: :destroy
-  has_many :champs_private, -> { root.private_ordered }, class_name: 'Champ', inverse_of: :dossier, dependent: :destroy
+  has_many :champs, -> { root.public_ordered }, inverse_of: false, dependent: :destroy
+  has_many :champs_private, -> { root.private_ordered }, class_name: 'Champ', inverse_of: false, dependent: :destroy
   has_many :commentaires, inverse_of: :dossier, dependent: :destroy
   has_many :invites, dependent: :destroy
   has_many :follows, -> { active }, inverse_of: :dossier

spec/controllers/users/dossiers_controller_spec.rb

@@ -619,13 +619,14 @@ describe Users::DossiersController, type: :controller do
       it 'updates the champs' do
         subject
         expect(first_champ.reload.value).to eq('beautiful value')
-        expect(first_champ.dossier.reload.last_champ_updated_at).to eq(now)
         expect(piece_justificative_champ.reload.piece_justificative_file).to be_attached
       end
 
-      it 'updates the dossier modification date' do
+      it 'updates the dossier timestamps' do
         subject
-        expect(dossier.reload.updated_at.year).to eq(2100)
+        dossier.reload
+        expect(dossier.updated_at).to eq(now)
+        expect(dossier.last_champ_updated_at).to eq(now)
       end
 
       it 'updates the dossier state' do
@@ -635,22 +636,34 @@ describe Users::DossiersController, type: :controller do
 
       it { is_expected.to redirect_to(demande_dossier_path(dossier)) }
 
-      context 'when only files champs are modified' do
+      context 'when only a single file champ are modified' do
+        # A bug in ActiveRecord causes records changed through grand-parent <->  parent <-> child
+        # relationships do not touch the grand-parent record on change.
+        # This situation is hit when updating just the attachment of a champ (and not the
+        # champ itself).
+        #
+        # This test ensures that, whatever workaround we wrote for this, it still works properly.
+        #
+        # See https://github.com/rails/rails/issues/26726
         let(:submit_payload) do
           {
             id: dossier.id,
             dossier: {
-              champs_attributes: {
+              champs_attributes: [
+                {
                   id: piece_justificative_champ.id,
                   piece_justificative_file: file
                 }
+              ]
             }
           }
         end
 
-        it 'updates the dossier modification date' do
+        it 'updates the dossier timestamps' do
           subject
-          expect(dossier.reload.updated_at.year).to eq(2100)
+          dossier.reload
+          expect(dossier.updated_at).to eq(now)
+          expect(dossier.last_champ_updated_at).to eq(now)
         end
       end
     end
@@ -667,6 +680,12 @@ describe Users::DossiersController, type: :controller do
       it { expect(response).to render_template(:modifier) }
       it { expect(flash.alert).to eq(['nop']) }
 
+      it 'does not update the dossier timestamps' do
+        dossier.reload
+        expect(dossier.updated_at).not_to eq(now)
+        expect(dossier.last_champ_updated_at).not_to eq(now)
+      end
+
       it 'does not send an email' do
         expect(NotificationMailer).not_to receive(:send_initiated_notification)