From fc4d8362dcfb493c5a3791305c8ea60a406b0f77 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 1 Apr 2021 16:40:22 +0200 Subject: [PATCH 1/3] models: fix typo in comments --- app/controllers/users/dossiers_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/users/dossiers_controller.rb b/app/controllers/users/dossiers_controller.rb index f77eff7ab..40fbe0f03 100644 --- a/app/controllers/users/dossiers_controller.rb +++ b/app/controllers/users/dossiers_controller.rb @@ -360,8 +360,8 @@ 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 From 6eadcf614decd6254869b00f5429d6e0d08201f5 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 1 Apr 2021 16:48:12 +0200 Subject: [PATCH 2/3] spec: improve the dossier_controller timestamp specs Make the specs clearer, and better test the various timestamps. --- .../users/dossiers_controller_spec.rb | 39 ++++++++++++++----- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/spec/controllers/users/dossiers_controller_spec.rb b/spec/controllers/users/dossiers_controller_spec.rb index 1bd747264..d69c83670 100644 --- a/spec/controllers/users/dossiers_controller_spec.rb +++ b/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: { - id: piece_justificative_champ.id, - piece_justificative_file: file - } + 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) From 3499f5af9ab7c70d0ca5860f7034f0bc1717ca02 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 1 Apr 2021 15:22:47 +0000 Subject: [PATCH 3/3] =?UTF-8?q?models:=20remove=20invalid=20Dossier=20?= =?UTF-8?q?=E2=86=94=EF=B8=8E=20Champ=20inverse=20relationship?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Dossier.champs` is not really an inverse of `Champs.dossier`: when a Champ record is created, it should not always be added to dossier.champs (for instance if the champ is private). NB: this breaks the workaround we added in #3907 to fix the parent dossier not being touched in some cases (the workaround was to add an inverse relationship, but we now have to remove it). The new workaround is to watch for `changed_for_autosave?` on champs. Unlike `changed?`, `changed_for_autosave?` also detects changes to attachments. This allows us to touch both `last_champ_updated_at` and `updated_at` in a single pass. --- app/controllers/users/dossiers_controller.rb | 2 +- app/models/champ.rb | 2 +- app/models/dossier.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/users/dossiers_controller.rb b/app/controllers/users/dossiers_controller.rb index 40fbe0f03..f03d28f19 100644 --- a/app/controllers/users/dossiers_controller.rb +++ b/app/controllers/users/dossiers_controller.rb @@ -365,7 +365,7 @@ module Users @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 diff --git a/app/models/champ.rb b/app/models/champ.rb index 864ced9fb..8cb1d8804 100644 --- a/app/models/champ.rb +++ b/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 diff --git a/app/models/dossier.rb b/app/models/dossier.rb index aaf93e14f..2441035f0 100644 --- a/app/models/dossier.rb +++ b/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