From 51aacabf1308b95ab91a00c63fa1fd3cf65256a6 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 29 May 2019 16:28:27 +0000 Subject: [PATCH 1/2] models: fix touch not propagating when using nested attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sometimes, when using nested attributes, touch doesn’t propagate to parent relationships. (see https://github.com/rails/rails/issues/26726) Specifically, this happens in our app when updating a dossier with only new attachements (but without changing the value of any fields). To work around this, we need to define the parent relationship explicitely. This is good practice anyway. Fix #3906 --- app/models/avis.rb | 2 +- app/models/champ.rb | 2 +- app/models/commentaire.rb | 2 +- app/models/dossier.rb | 10 ++--- app/models/piece_justificative.rb | 2 +- .../users/dossiers_controller_spec.rb | 43 +++++++++++++++++-- spec/factories/procedure.rb | 8 ++++ 7 files changed, 57 insertions(+), 12 deletions(-) diff --git a/app/models/avis.rb b/app/models/avis.rb index 7203870ea..251b80647 100644 --- a/app/models/avis.rb +++ b/app/models/avis.rb @@ -1,7 +1,7 @@ class Avis < ApplicationRecord include EmailSanitizableConcern - belongs_to :dossier, touch: true + belongs_to :dossier, inverse_of: :avis, touch: true belongs_to :gestionnaire belongs_to :claimant, class_name: 'Gestionnaire' diff --git a/app/models/champ.rb b/app/models/champ.rb index 06bc65871..91835499f 100644 --- a/app/models/champ.rb +++ b/app/models/champ.rb @@ -1,5 +1,5 @@ class Champ < ApplicationRecord - belongs_to :dossier, touch: true + belongs_to :dossier, inverse_of: :champs, touch: true belongs_to :type_de_champ, inverse_of: :champ belongs_to :parent, class_name: 'Champ' has_many :commentaires diff --git a/app/models/commentaire.rb b/app/models/commentaire.rb index 5a49b6e84..68b571164 100644 --- a/app/models/commentaire.rb +++ b/app/models/commentaire.rb @@ -1,5 +1,5 @@ class Commentaire < ApplicationRecord - belongs_to :dossier, touch: true + belongs_to :dossier, inverse_of: :commentaires, touch: true belongs_to :piece_justificative belongs_to :user diff --git a/app/models/dossier.rb b/app/models/dossier.rb index 3bd702d47..698e265be 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -19,18 +19,18 @@ class Dossier < ApplicationRecord has_one :individual, dependent: :destroy has_one :attestation, dependent: :destroy - has_many :pieces_justificatives, dependent: :destroy + has_many :pieces_justificatives, inverse_of: :dossier, dependent: :destroy has_one_attached :justificatif_motivation - has_many :champs, -> { root.public_only.ordered }, dependent: :destroy - has_many :champs_private, -> { root.private_only.ordered }, class_name: 'Champ', dependent: :destroy - has_many :commentaires, dependent: :destroy + has_many :champs, -> { root.public_only.ordered }, inverse_of: :dossier, dependent: :destroy + has_many :champs_private, -> { root.private_only.ordered }, class_name: 'Champ', inverse_of: :dossier, dependent: :destroy + has_many :commentaires, inverse_of: :dossier, dependent: :destroy has_many :invites, dependent: :destroy has_many :follows, -> { active } has_many :previous_follows, -> { inactive }, class_name: 'Follow' has_many :followers_gestionnaires, through: :follows, source: :gestionnaire has_many :previous_followers_gestionnaires, -> { distinct }, through: :previous_follows, source: :gestionnaire - has_many :avis, dependent: :destroy + has_many :avis, inverse_of: :dossier, dependent: :destroy has_many :dossier_operation_logs, dependent: :destroy diff --git a/app/models/piece_justificative.rb b/app/models/piece_justificative.rb index 20a95260c..48e4876fc 100644 --- a/app/models/piece_justificative.rb +++ b/app/models/piece_justificative.rb @@ -1,5 +1,5 @@ class PieceJustificative < ApplicationRecord - belongs_to :dossier, touch: true + belongs_to :dossier, inverse_of: :pieces_justificatives, touch: true belongs_to :type_de_piece_justificative has_one :commentaire diff --git a/spec/controllers/users/dossiers_controller_spec.rb b/spec/controllers/users/dossiers_controller_spec.rb index 9d68d2003..1da3997af 100644 --- a/spec/controllers/users/dossiers_controller_spec.rb +++ b/spec/controllers/users/dossiers_controller_spec.rb @@ -386,9 +386,11 @@ describe Users::DossiersController, type: :controller do describe '#update_brouillon' do before { sign_in(user) } + let!(:dossier) { create(:dossier, user: user) } let(:first_champ) { dossier.champs.first } let(:value) { 'beautiful value' } + let(:now) { Time.zone.parse('01/01/2100') } let(:submit_payload) do { id: dossier.id, @@ -402,7 +404,11 @@ describe Users::DossiersController, type: :controller do end let(:payload) { submit_payload } - subject { patch :update_brouillon, params: payload } + subject do + Timecop.freeze(now) do + patch :update_brouillon, params: payload + end + end context 'when the dossier cannot be updated by the user' do let!(:dossier) { create(:dossier, :en_instruction, user: user) } @@ -421,6 +427,7 @@ describe Users::DossiersController, type: :controller do expect(response).to redirect_to(merci_dossier_path(dossier)) expect(first_champ.reload.value).to eq('beautiful value') + expect(dossier.reload.updated_at.year).to eq(2100) expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_construction)) end @@ -549,9 +556,15 @@ describe Users::DossiersController, type: :controller do describe '#update' do before { sign_in(user) } - let!(:dossier) { create(:dossier, :en_construction, user: user) } + + let(:procedure) { create(:procedure, :published, :with_type_de_champ, :with_piece_justificative) } + let!(:dossier) { create(:dossier, :en_construction, user: user, procedure: procedure) } let(:first_champ) { dossier.champs.first } + let(:piece_justificative_champ) { dossier.champs.last } let(:value) { 'beautiful value' } + let(:file) { Rack::Test::UploadedFile.new("./spec/fixtures/files/piece_justificative_0.pdf", 'application/pdf') } + let(:now) { Time.zone.parse('01/01/2100') } + let(:submit_payload) do { id: dossier.id, @@ -565,7 +578,11 @@ describe Users::DossiersController, type: :controller do end let(:payload) { submit_payload } - subject { patch :update, params: payload } + subject do + Timecop.freeze(now) do + patch :update, params: payload + end + end context 'when the dossier cannot be updated by the user' do let!(:dossier) { create(:dossier, :en_instruction, user: user) } @@ -584,8 +601,28 @@ describe Users::DossiersController, type: :controller do expect(response).to redirect_to(demande_dossier_path(dossier)) expect(first_champ.reload.value).to eq('beautiful value') + expect(dossier.reload.updated_at.year).to eq(2100) expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_construction)) end + + context 'when only files champs are modified' do + let(:submit_payload) do + { + id: dossier.id, + dossier: { + champs_attributes: { + id: piece_justificative_champ.id, + piece_justificative_file: file + } + } + } + end + + it 'updates the dossier modification date' do + subject + expect(dossier.reload.updated_at.year).to eq(2100) + end + end end context 'when the update fails' do diff --git a/spec/factories/procedure.rb b/spec/factories/procedure.rb index 98488e884..8c62169b3 100644 --- a/spec/factories/procedure.rb +++ b/spec/factories/procedure.rb @@ -127,6 +127,14 @@ FactoryBot.define do end end + trait :with_piece_justificative do + after(:build) do |procedure, _evaluator| + type_de_champ = create(:type_de_champ_piece_justificative) + procedure.types_de_champ << type_de_champ + end + end + + # Deprecated trait :with_two_type_de_piece_justificative do after(:build) do |procedure, _evaluator| rib = create(:type_de_piece_justificative, :rib, order_place: 1) From 7e80b8a4dc1b4838df2f9032c7818a5efa4bbcd1 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Wed, 12 Jun 2019 17:10:53 +0000 Subject: [PATCH 2/2] Enable the Rails/InverseOf cop and add missing `inverse_of` --- .rubocop.yml | 2 +- app/models/champs/repetition_champ.rb | 2 +- app/models/dossier.rb | 4 ++-- app/models/gestionnaire.rb | 6 +++--- app/models/procedure.rb | 6 +++--- app/models/type_de_champ.rb | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 307bc088d..445c3400a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -726,7 +726,7 @@ Rails/HttpStatus: Enabled: false Rails/InverseOf: - Enabled: false + Enabled: true Rails/LexicallyScopedActionFilter: Enabled: false diff --git a/app/models/champs/repetition_champ.rb b/app/models/champs/repetition_champ.rb index 7f8313e16..50295a6f6 100644 --- a/app/models/champs/repetition_champ.rb +++ b/app/models/champs/repetition_champ.rb @@ -1,5 +1,5 @@ class Champs::RepetitionChamp < Champ - has_many :champs, -> { ordered }, foreign_key: :parent_id, dependent: :destroy + has_many :champs, -> { ordered }, foreign_key: :parent_id, inverse_of: :parent, dependent: :destroy accepts_nested_attributes_for :champs, allow_destroy: true diff --git a/app/models/dossier.rb b/app/models/dossier.rb index 698e265be..3449517ec 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -26,8 +26,8 @@ class Dossier < ApplicationRecord has_many :champs_private, -> { root.private_only.ordered }, class_name: 'Champ', inverse_of: :dossier, dependent: :destroy has_many :commentaires, inverse_of: :dossier, dependent: :destroy has_many :invites, dependent: :destroy - has_many :follows, -> { active } - has_many :previous_follows, -> { inactive }, class_name: 'Follow' + has_many :follows, -> { active }, inverse_of: :dossier + has_many :previous_follows, -> { inactive }, class_name: 'Follow', inverse_of: :dossier has_many :followers_gestionnaires, through: :follows, source: :gestionnaire has_many :previous_followers_gestionnaires, -> { distinct }, through: :previous_follows, source: :gestionnaire has_many :avis, inverse_of: :dossier, dependent: :destroy diff --git a/app/models/gestionnaire.rb b/app/models/gestionnaire.rb index 24a7b5498..c5ab8b7c2 100644 --- a/app/models/gestionnaire.rb +++ b/app/models/gestionnaire.rb @@ -12,12 +12,12 @@ class Gestionnaire < ApplicationRecord has_many :assign_to, dependent: :destroy has_many :procedures, through: :assign_to - has_many :assign_to_with_email_notifications, -> { with_email_notifications }, class_name: 'AssignTo' + has_many :assign_to_with_email_notifications, -> { with_email_notifications }, class_name: 'AssignTo', inverse_of: :gestionnaire has_many :procedures_with_email_notifications, through: :assign_to_with_email_notifications, source: :procedure has_many :dossiers, -> { state_not_brouillon }, through: :procedures - has_many :follows, -> { active } - has_many :previous_follows, -> { inactive }, class_name: 'Follow' + has_many :follows, -> { active }, inverse_of: :gestionnaire + has_many :previous_follows, -> { inactive }, class_name: 'Follow', inverse_of: :gestionnaire has_many :followed_dossiers, through: :follows, source: :dossier has_many :previously_followed_dossiers, -> { distinct }, through: :previous_follows, source: :dossier has_many :avis diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 0cfe9a71d..c1eddeba0 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -5,9 +5,9 @@ class Procedure < ApplicationRecord MAX_DUREE_CONSERVATION = 36 - has_many :types_de_piece_justificative, -> { ordered }, dependent: :destroy - has_many :types_de_champ, -> { root.public_only.ordered }, dependent: :destroy - has_many :types_de_champ_private, -> { root.private_only.ordered }, class_name: 'TypeDeChamp', dependent: :destroy + has_many :types_de_piece_justificative, -> { ordered }, inverse_of: :procedure, dependent: :destroy + has_many :types_de_champ, -> { root.public_only.ordered }, inverse_of: :procedure, dependent: :destroy + has_many :types_de_champ_private, -> { root.private_only.ordered }, class_name: 'TypeDeChamp', inverse_of: :procedure, dependent: :destroy has_many :dossiers, dependent: :restrict_with_exception has_many :deleted_dossiers, dependent: :destroy diff --git a/app/models/type_de_champ.rb b/app/models/type_de_champ.rb index f3b9efe86..924672ec9 100644 --- a/app/models/type_de_champ.rb +++ b/app/models/type_de_champ.rb @@ -32,7 +32,7 @@ class TypeDeChamp < ApplicationRecord belongs_to :procedure belongs_to :parent, class_name: 'TypeDeChamp' - has_many :types_de_champ, -> { ordered }, foreign_key: :parent_id, class_name: 'TypeDeChamp', dependent: :destroy + has_many :types_de_champ, -> { ordered }, foreign_key: :parent_id, class_name: 'TypeDeChamp', inverse_of: :parent, dependent: :destroy store_accessor :options, :cadastres, :quartiers_prioritaires, :parcelles_agricoles, :old_pj delegate :tags_for_template, to: :dynamic_type