From 970c3e4b2bd12162691ce003c1b926aa049c3e83 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Fri, 19 Nov 2021 12:48:52 +0000 Subject: [PATCH] specs: avoid executing business logic in Procedure factory Calling business logic in a factory is a code-smell, because it usually requires the object to be saved into database, and may have unintended consequences when the business logic is changed. Also, this allows to just build a published procedure, without saving it to the database. --- spec/factories/dossier.rb | 31 ++++---- spec/factories/individual.rb | 7 ++ spec/factories/procedure.rb | 76 +++++++++++-------- spec/factories/procedure_revision.rb | 17 +++++ .../procedure_revision_type_de_champ.rb | 12 +++ spec/helpers/dossier_helper_spec.rb | 2 +- spec/mailers/application_mailer_spec.rb | 2 +- spec/mailers/dossier_mailer_spec.rb | 11 +-- spec/models/procedure_presentation_spec.rb | 10 +-- .../dossier_projection_service_spec.rb | 2 +- spec/system/accessibilite/wcag_usager_spec.rb | 2 +- .../dossiers/_demande.html.haml_spec.rb | 2 +- 12 files changed, 112 insertions(+), 62 deletions(-) diff --git a/spec/factories/dossier.rb b/spec/factories/dossier.rb index 59f4abc37..50182c714 100644 --- a/spec/factories/dossier.rb +++ b/spec/factories/dossier.rb @@ -2,18 +2,20 @@ FactoryBot.define do factory :dossier do autorisation_donnees { true } state { Dossier.states.fetch(:brouillon) } - association :user + + user { association :user } transient do - procedure { nil } + for_individual? { false } + # For now a dossier must use a `create`d procedure, even if the dossier is only built (and not created). + # This is because saving the dossier fails when the procedure has not been saved beforehand + # (due to some internal ActiveRecord error). + # TODO: find a way to find the issue and just `build` the procedure. + procedure { create(:procedure, :published, :with_type_de_champ, :with_type_de_champ_private, for_individual: for_individual?) } end after(:build) do |dossier, evaluator| - if evaluator.procedure.present? - procedure = evaluator.procedure - else - procedure = create(:procedure, :published, :with_type_de_champ, :with_type_de_champ_private) - end + procedure = evaluator.procedure dossier.revision = procedure.active_revision @@ -22,7 +24,9 @@ FactoryBot.define do dossier.groupe_instructeur = procedure.routee? ? nil : procedure.defaut_groupe_instructeur end - dossier.build_default_individual + if procedure.for_individual? && dossier.individual.blank? + build(:individual, :empty, dossier: dossier) + end end trait :with_entreprise do @@ -42,12 +46,11 @@ FactoryBot.define do end trait :with_individual do - after(:build) do |dossier, evaluator| - # If the procedure was implicitely created by the factory, - # mark it automatically as for_individual. - if evaluator.procedure.nil? - dossier.procedure.update(for_individual: true) - end + transient do + for_individual? { true } + end + + after(:build) do |dossier, _evaluator| if !dossier.procedure.for_individual? raise 'Inconsistent factory: attempting to create a dossier :with_individual on a procedure that is not `for_individual?`' end diff --git a/spec/factories/individual.rb b/spec/factories/individual.rb index e93b3f6c0..c1aa66c5d 100644 --- a/spec/factories/individual.rb +++ b/spec/factories/individual.rb @@ -5,5 +5,12 @@ FactoryBot.define do prenom { 'Xavier' } birthdate { Date.new(1991, 11, 01) } association :dossier + + trait :empty do + gender { nil } + nom { nil } + prenom { nil } + birthdate { nil } + end end end diff --git a/spec/factories/procedure.rb b/spec/factories/procedure.rb index eb874dfa6..41e2b83fa 100644 --- a/spec/factories/procedure.rb +++ b/spec/factories/procedure.rb @@ -1,5 +1,6 @@ FactoryBot.define do sequence(:published_path) { |n| "fake_path#{n}" } + factory :procedure do sequence(:libelle) { |n| "Procedure #{n}" } description { "Demande de subvention à l'intention des associations" } @@ -26,24 +27,17 @@ FactoryBot.define do elsif procedure.administrateurs.empty? procedure.administrateurs = [build(:administrateur)] end - procedure.draft_revision = build(:procedure_revision, procedure: procedure) - evaluator.types_de_champ.each do |type_de_champ| - type_de_champ.revision = procedure.draft_revision - type_de_champ.private = false - type_de_champ.revision.revision_types_de_champ << build(:procedure_revision_type_de_champ, - revision: procedure.draft_revision, - position: type_de_champ.order_place, - type_de_champ: type_de_champ) - end + initial_revision = build(:procedure_revision, procedure: procedure) + add_types_de_champs(evaluator.types_de_champ, to: initial_revision, scope: :public) + add_types_de_champs(evaluator.types_de_champ_private, to: initial_revision, scope: :private) - evaluator.types_de_champ_private.each do |type_de_champ| - type_de_champ.revision = procedure.draft_revision - type_de_champ.private = true - type_de_champ.revision.revision_types_de_champ_private << build(:procedure_revision_type_de_champ, - revision: procedure.draft_revision, - position: type_de_champ.order_place, - type_de_champ: type_de_champ) + if procedure.brouillon? + procedure.draft_revision = initial_revision + else + procedure.published_revision = initial_revision + procedure.published_revision.published_at = Time.zone.now + procedure.draft_revision = build(:procedure_revision, from_original: initial_revision) end end @@ -71,11 +65,12 @@ FactoryBot.define do end factory :simple_procedure do + published + + for_individual { true } + after(:build) do |procedure, _evaluator| - procedure.for_individual = true build(:type_de_champ, libelle: 'Texte obligatoire', mandatory: true, procedure: procedure) - procedure.path = generate(:published_path) - procedure.publish! end end @@ -218,26 +213,27 @@ FactoryBot.define do end trait :published do - after(:build) do |procedure, _evaluator| - procedure.path = generate(:published_path) - procedure.publish! - end + aasm_state { :publiee } + path { generate(:published_path) } + published_at { Time.zone.now } + unpublished_at { nil } + closed_at { nil } end trait :closed do - after(:build) do |procedure, _evaluator| - procedure.path = generate(:published_path) - procedure.publish! - procedure.close! - end + published + + aasm_state { :close } + published_at { Time.zone.now - 1.second } + closed_at { Time.zone.now } end trait :unpublished do - after(:build) do |procedure, _evaluator| - procedure.path = generate(:published_path) - procedure.publish! - procedure.unpublish! - end + published + + aasm_state { :depubliee } + published_at { Time.zone.now - 1.second } + unpublished_at { Time.zone.now } end trait :discarded do @@ -308,3 +304,17 @@ FactoryBot.define do end end end + +def add_types_de_champs(types_de_champ, to: nil, scope: :public) + revision = to + association_name = scope == :private ? :revision_types_de_champ_private : :revision_types_de_champ + + types_de_champ.each do |type_de_champ| + type_de_champ.revision = revision + type_de_champ.private = (scope == :private) + type_de_champ.revision.public_send(association_name) << build(:procedure_revision_type_de_champ, + revision: revision, + position: type_de_champ.order_place, + type_de_champ: type_de_champ) + end +end diff --git a/spec/factories/procedure_revision.rb b/spec/factories/procedure_revision.rb index 050749066..1d7ee0fc3 100644 --- a/spec/factories/procedure_revision.rb +++ b/spec/factories/procedure_revision.rb @@ -1,4 +1,21 @@ FactoryBot.define do factory :procedure_revision do + transient do + from_original { nil } + end + + after(:build) do |revision, evaluator| + if evaluator.from_original + original = evaluator.from_original + + revision.procedure = original.procedure + original.revision_types_de_champ.each do |r_tdc| + revision.revision_types_de_champ << build(:procedure_revision_type_de_champ, from_original: r_tdc) + end + original.revision_types_de_champ_private.each do |r_tdc| + revision.revision_types_de_champ_private << build(:procedure_revision_type_de_champ, from_original: r_tdc) + end + end + end end end diff --git a/spec/factories/procedure_revision_type_de_champ.rb b/spec/factories/procedure_revision_type_de_champ.rb index 62359ab98..fc2b5b214 100644 --- a/spec/factories/procedure_revision_type_de_champ.rb +++ b/spec/factories/procedure_revision_type_de_champ.rb @@ -1,4 +1,16 @@ FactoryBot.define do factory :procedure_revision_type_de_champ do + transient do + from_original { nil } + end + + after(:build) do |revision_type_de_champ, evaluator| + if evaluator.from_original + original = evaluator.from_original + + revision_type_de_champ.type_de_champ = original.type_de_champ + revision_type_de_champ.position = original.position + end + end end end diff --git a/spec/helpers/dossier_helper_spec.rb b/spec/helpers/dossier_helper_spec.rb index d1e47d61d..9835cba2d 100644 --- a/spec/helpers/dossier_helper_spec.rb +++ b/spec/helpers/dossier_helper_spec.rb @@ -49,7 +49,7 @@ RSpec.describe DossierHelper, type: :helper do let(:procedure) { create(:simple_procedure, :for_individual) } context "when the individual is not provided" do - let(:individual) { nil } + let(:individual) { build(:individual, :empty) } it { is_expected.to be_blank } end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index c667d6cd4..f75be8e2c 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -1,6 +1,6 @@ RSpec.describe ApplicationMailer, type: :mailer do describe 'dealing with invalid emails' do - let(:dossier) { create(:dossier, procedure: build(:simple_procedure)) } + let(:dossier) { create(:dossier, procedure: create(:simple_procedure)) } subject { DossierMailer.notify_new_draft(dossier) } describe 'invalid emails are not sent' do diff --git a/spec/mailers/dossier_mailer_spec.rb b/spec/mailers/dossier_mailer_spec.rb index e521c94de..87e4bcdca 100644 --- a/spec/mailers/dossier_mailer_spec.rb +++ b/spec/mailers/dossier_mailer_spec.rb @@ -12,7 +12,7 @@ RSpec.describe DossierMailer, type: :mailer do end describe '.notify_new_draft' do - let(:dossier) { create(:dossier, procedure: build(:simple_procedure, :with_auto_archive)) } + let(:dossier) { create(:dossier, procedure: create(:simple_procedure, :with_auto_archive)) } subject { described_class.notify_new_draft(dossier) } @@ -27,7 +27,7 @@ RSpec.describe DossierMailer, type: :mailer do end describe '.notify_new_answer with dossier brouillon' do - let(:dossier) { create(:dossier, procedure: build(:simple_procedure)) } + let(:dossier) { create(:dossier, procedure: create(:simple_procedure)) } let(:commentaire) { create(:commentaire, dossier: dossier) } subject { described_class.with(commentaire: commentaire).notify_new_answer } @@ -39,8 +39,9 @@ RSpec.describe DossierMailer, type: :mailer do end describe '.notify_new_answer with dossier en construction' do - let(:dossier) { create(:dossier, state: "en_construction", procedure: build(:simple_procedure)) } + let(:dossier) { create(:dossier, :en_construction, procedure: create(:simple_procedure)) } let(:commentaire) { create(:commentaire, dossier: dossier) } + subject { described_class.with(commentaire: commentaire).notify_new_answer } it { expect(subject.subject).to include("Nouveau message") } @@ -51,7 +52,7 @@ RSpec.describe DossierMailer, type: :mailer do end describe '.notify_new_answer with commentaire discarded' do - let(:dossier) { create(:dossier, procedure: build(:simple_procedure)) } + let(:dossier) { create(:dossier, procedure: create(:simple_procedure)) } let(:commentaire) { create(:commentaire, dossier: dossier, discarded_at: 2.minutes.ago) } subject { described_class.with(commentaire: commentaire).notify_new_answer } @@ -83,7 +84,7 @@ RSpec.describe DossierMailer, type: :mailer do end describe '.notify_revert_to_instruction' do - let(:dossier) { create(:dossier, procedure: build(:simple_procedure)) } + let(:dossier) { create(:dossier, procedure: create(:simple_procedure)) } subject { described_class.notify_revert_to_instruction(dossier) } diff --git a/spec/models/procedure_presentation_spec.rb b/spec/models/procedure_presentation_spec.rb index c3d45668a..9dce6c912 100644 --- a/spec/models/procedure_presentation_spec.rb +++ b/spec/models/procedure_presentation_spec.rb @@ -320,8 +320,8 @@ describe ProcedurePresentation do let(:procedure) { create(:procedure, :for_individual) } - let!(:first_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'M', prenom: 'Alain', nom: 'Antonelli')) } - let!(:last_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'Mme', prenom: 'Zora', nom: 'Zemmour')) } + let!(:first_dossier) { create(:dossier, procedure: procedure, individual: build(:individual, gender: 'M', prenom: 'Alain', nom: 'Antonelli')) } + let!(:last_dossier) { create(:dossier, procedure: procedure, individual: build(:individual, gender: 'Mme', prenom: 'Zora', nom: 'Zemmour')) } context 'for gender column' do let(:column) { 'gender' } @@ -617,8 +617,8 @@ describe ProcedurePresentation do context 'for individual table' do let(:procedure) { create(:procedure, :for_individual) } - let!(:kept_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'Mme', prenom: 'Josephine', nom: 'Baker')) } - let!(:discarded_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'M', prenom: 'Jean', nom: 'Tremblay')) } + let!(:kept_dossier) { create(:dossier, procedure: procedure, individual: build(:individual, gender: 'Mme', prenom: 'Josephine', nom: 'Baker')) } + let!(:discarded_dossier) { create(:dossier, procedure: procedure, individual: build(:individual, gender: 'M', prenom: 'Jean', nom: 'Tremblay')) } context 'for gender column' do let(:filter) { [{ 'table' => 'individual', 'column' => 'gender', 'value' => 'Mme' }] } @@ -646,7 +646,7 @@ describe ProcedurePresentation do ] end - let!(:other_kept_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'M', prenom: 'Romuald', nom: 'Pistis')) } + let!(:other_kept_dossier) { create(:dossier, procedure: procedure, individual: build(:individual, gender: 'M', prenom: 'Romuald', nom: 'Pistis')) } it 'returns every dossier that matches any of the search criteria for a given column' do is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) diff --git a/spec/services/dossier_projection_service_spec.rb b/spec/services/dossier_projection_service_spec.rb index 096ac0e07..d08d8d396 100644 --- a/spec/services/dossier_projection_service_spec.rb +++ b/spec/services/dossier_projection_service_spec.rb @@ -92,7 +92,7 @@ describe DossierProjectionService do context 'for individual table' do let(:table) { 'individual' } let(:procedure) { create(:procedure, :for_individual, :with_type_de_champ, :with_type_de_champ_private) } - let(:dossier) { create(:dossier, procedure: procedure, individual: create(:individual, nom: 'Martin', prenom: 'Jacques', gender: 'M.')) } + let(:dossier) { create(:dossier, procedure: procedure, individual: build(:individual, nom: 'Martin', prenom: 'Jacques', gender: 'M.')) } context 'for prenom column' do let(:column) { 'prenom' } diff --git a/spec/system/accessibilite/wcag_usager_spec.rb b/spec/system/accessibilite/wcag_usager_spec.rb index 5ac9658aa..a563d5e0e 100644 --- a/spec/system/accessibilite/wcag_usager_spec.rb +++ b/spec/system/accessibilite/wcag_usager_spec.rb @@ -1,5 +1,5 @@ describe 'wcag rules for usager', js: true do - let(:procedure) { create(:procedure, :with_type_de_champ, :with_all_champs, :with_service, :for_individual, :published) } + let(:procedure) { create(:procedure, :published, :with_all_champs, :with_service, :for_individual) } let(:password) { 'a very complicated password' } let(:litteraire_user) { create(:user, password: password) } diff --git a/spec/views/shared/dossiers/_demande.html.haml_spec.rb b/spec/views/shared/dossiers/_demande.html.haml_spec.rb index cb48f4de2..9d166cc43 100644 --- a/spec/views/shared/dossiers/_demande.html.haml_spec.rb +++ b/spec/views/shared/dossiers/_demande.html.haml_spec.rb @@ -32,7 +32,7 @@ describe 'shared/dossiers/demande.html.haml', type: :view do end context 'when dossier was created by an individual' do - let(:individual) { create(:individual) } + let(:individual) { build(:individual) } it 'renders the individual identity infos' do expect(subject).to include(individual.gender)