From eba9a8712398e216f18403dca5ee798460a2e96d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Vantomme?= Date: Mon, 7 Feb 2022 11:42:44 +0100 Subject: [PATCH 1/5] refactor(API Entreprise): raise an error on blank token --- app/models/api_entreprise_token.rb | 19 +++- config/locales/api_entreprise.en.yml | 4 + config/locales/api_entreprise.fr.yml | 4 + spec/models/api_entreprise_token_spec.rb | 120 +++++++++++++++++++++++ 4 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 config/locales/api_entreprise.en.yml create mode 100644 config/locales/api_entreprise.fr.yml create mode 100644 spec/models/api_entreprise_token_spec.rb diff --git a/app/models/api_entreprise_token.rb b/app/models/api_entreprise_token.rb index ff0dbf49e..a8d18a3c5 100644 --- a/app/models/api_entreprise_token.rb +++ b/app/models/api_entreprise_token.rb @@ -1,16 +1,18 @@ class APIEntrepriseToken - attr_reader :token + TokenError = Class.new(StandardError) def initialize(token) @token = token end - def roles - decoded_token["roles"] if token.present? + def token + raise TokenError, I18n.t("api_entreprise.errors.missing_token") if @token.blank? + + @token end def expired? - Time.zone.now.to_i >= decoded_token["exp"] if token.present? + decoded_token.key?("exp") && decoded_token["exp"] <= Time.zone.now.to_i end def role?(role) @@ -19,7 +21,14 @@ class APIEntrepriseToken private + def roles + Array(decoded_token["roles"]) + end + def decoded_token - JWT.decode(token, nil, false)[0] + @decoded_token ||= {} + @decoded_token[token] ||= JWT.decode(token, nil, false)[0] + rescue JWT::DecodeError => e + raise TokenError, e.message end end diff --git a/config/locales/api_entreprise.en.yml b/config/locales/api_entreprise.en.yml new file mode 100644 index 000000000..ee815c16e --- /dev/null +++ b/config/locales/api_entreprise.en.yml @@ -0,0 +1,4 @@ +en: + api_entreprise: + errors: + missing_token: "the API Entreprise token cannot be blank" diff --git a/config/locales/api_entreprise.fr.yml b/config/locales/api_entreprise.fr.yml new file mode 100644 index 000000000..f8d81a523 --- /dev/null +++ b/config/locales/api_entreprise.fr.yml @@ -0,0 +1,4 @@ +fr: + api_entreprise: + errors: + missing_token: "le jeton API Entreprise ne peut être vide" diff --git a/spec/models/api_entreprise_token_spec.rb b/spec/models/api_entreprise_token_spec.rb new file mode 100644 index 000000000..1480f7da3 --- /dev/null +++ b/spec/models/api_entreprise_token_spec.rb @@ -0,0 +1,120 @@ +describe APIEntrepriseToken, type: :model do + let(:api_entreprise_token) { APIEntrepriseToken.new(token) } + + describe "#token" do + subject { api_entreprise_token.token } + + context "without token" do + let(:token) { nil } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with a blank token" do + let(:token) { "" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with an invalid token" do + let(:token) { "NOT-A-VALID-TOKEN" } + + it { expect(subject).to equal(token) } + end + + context "with a valid token" do + let(:token) { "eyJhbGciOiJIUzI1NiJ9.eyJ1aWQiOiI2NjRkZWEyMS02YWFlLTQwZmYtYWM0Mi1kZmQ3ZGE4YjQ3NmUiLCJqdGkiOiJhcGktZW50cmVwcmlzZS1zdGFnaW5nIiwicm9sZXMiOlsiY2VydGlmaWNhdF9jbmV0cCIsInByb2J0cCIsImV0YWJsaXNzZW1lbnRzIiwicHJpdmlsZWdlcyIsInVwdGltZSIsImF0dGVzdGF0aW9uc19hZ2VmaXBoIiwiYWN0ZXNfaW5waSIsImJpbGFuc19pbnBpIiwiYWlkZXNfY292aWRfZWZmZWN0aWZzIiwiY2VydGlmaWNhdF9yZ2VfYWRlbWUiLCJhdHRlc3RhdGlvbnNfc29jaWFsZXMiLCJlbnRyZXByaXNlX2FydGlzYW5hbGUiLCJmbnRwX2NhcnRlX3BybyIsImNvbnZlbnRpb25zX2NvbGxlY3RpdmVzIiwiZXh0cmFpdHNfcmNzIiwiZXh0cmFpdF9jb3VydF9pbnBpIiwiY2VydGlmaWNhdF9hZ2VuY2VfYmlvIiwibXNhX2NvdGlzYXRpb25zIiwiZG9jdW1lbnRzX2Fzc29jaWF0aW9uIiwiZW9yaV9kb3VhbmVzIiwiYXNzb2NpYXRpb25zIiwiYmlsYW5zX2VudHJlcHJpc2VfYmRmIiwiZW50cmVwcmlzZXMiLCJxdWFsaWJhdCIsImNlcnRpZmljYXRfb3BxaWJpIiwiZW50cmVwcmlzZSIsImV0YWJsaXNzZW1lbnQiXSwic3ViIjoic3RhZ2luZyBkZXZlbG9wbWVudCIsImlhdCI6MTY0MTMwNDcxNCwidmVyc2lvbiI6IjEuMCIsImV4cCI6MTY4ODQ3NTUxNH0.xID66pIlMnBR5_6nG-GidFBzK4Tuuy5ZsWfkMEVB_Ek" } + + it { expect(subject).to equal(token) } + end + end + + describe "#role?" do + subject { api_entreprise_token.role?(role) } + + context "without token" do + let(:token) { nil } + let(:role) { "actes_inpi" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with a blank token" do + let(:token) { "" } + let(:role) { "actes_inpi" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with an invalid token" do + let(:token) { "NOT-A-VALID-TOKEN" } + let(:role) { "actes_inpi" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with a valid token" do + let(:token) { "eyJhbGciOiJIUzI1NiJ9.eyJ1aWQiOiI2NjRkZWEyMS02YWFlLTQwZmYtYWM0Mi1kZmQ3ZGE4YjQ3NmUiLCJqdGkiOiJhcGktZW50cmVwcmlzZS1zdGFnaW5nIiwicm9sZXMiOlsiY2VydGlmaWNhdF9jbmV0cCIsInByb2J0cCIsImV0YWJsaXNzZW1lbnRzIiwicHJpdmlsZWdlcyIsInVwdGltZSIsImF0dGVzdGF0aW9uc19hZ2VmaXBoIiwiYWN0ZXNfaW5waSIsImJpbGFuc19pbnBpIiwiYWlkZXNfY292aWRfZWZmZWN0aWZzIiwiY2VydGlmaWNhdF9yZ2VfYWRlbWUiLCJhdHRlc3RhdGlvbnNfc29jaWFsZXMiLCJlbnRyZXByaXNlX2FydGlzYW5hbGUiLCJmbnRwX2NhcnRlX3BybyIsImNvbnZlbnRpb25zX2NvbGxlY3RpdmVzIiwiZXh0cmFpdHNfcmNzIiwiZXh0cmFpdF9jb3VydF9pbnBpIiwiY2VydGlmaWNhdF9hZ2VuY2VfYmlvIiwibXNhX2NvdGlzYXRpb25zIiwiZG9jdW1lbnRzX2Fzc29jaWF0aW9uIiwiZW9yaV9kb3VhbmVzIiwiYXNzb2NpYXRpb25zIiwiYmlsYW5zX2VudHJlcHJpc2VfYmRmIiwiZW50cmVwcmlzZXMiLCJxdWFsaWJhdCIsImNlcnRpZmljYXRfb3BxaWJpIiwiZW50cmVwcmlzZSIsImV0YWJsaXNzZW1lbnQiXSwic3ViIjoic3RhZ2luZyBkZXZlbG9wbWVudCIsImlhdCI6MTY0MTMwNDcxNCwidmVyc2lvbiI6IjEuMCIsImV4cCI6MTY4ODQ3NTUxNH0.xID66pIlMnBR5_6nG-GidFBzK4Tuuy5ZsWfkMEVB_Ek" } + + context "but an unfetchable role" do + let(:role) { "NOT-A-ROLE" } + + it { expect(subject).to be_falsey } + end + + context "and a fetchable role" do + let(:role) { "actes_inpi" } + + it { expect(subject).to be_truthy } + end + end + end + + describe "#expired?" do + subject { api_entreprise_token.expired? } + + context "without token" do + let(:token) { nil } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with a blank token" do + let(:token) { "" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with an invalid token" do + let(:token) { "NOT-A-VALID-TOKEN" } + + it { expect { subject }.to raise_exception(APIEntrepriseToken::TokenError) } + end + + context "with a valid not expiring token" do + # never expire + let(:token) { "eyJhbGciOiJIUzI1NiJ9.eyJ1aWQiOiI2NjRkZWEyMS02YWFlLTQwZmYtYWM0Mi1kZmQ3ZGE4YjQ3NmUiLCJqdGkiOiJhcGktZW50cmVwcmlzZS1zdGFnaW5nIiwicm9sZXMiOlsiY2VydGlmaWNhdF9jbmV0cCIsInByb2J0cCIsImV0YWJsaXNzZW1lbnRzIiwicHJpdmlsZWdlcyIsInVwdGltZSIsImF0dGVzdGF0aW9uc19hZ2VmaXBoIiwiYWN0ZXNfaW5waSIsImJpbGFuc19pbnBpIiwiYWlkZXNfY292aWRfZWZmZWN0aWZzIiwiY2VydGlmaWNhdF9yZ2VfYWRlbWUiLCJhdHRlc3RhdGlvbnNfc29jaWFsZXMiLCJlbnRyZXByaXNlX2FydGlzYW5hbGUiLCJmbnRwX2NhcnRlX3BybyIsImNvbnZlbnRpb25zX2NvbGxlY3RpdmVzIiwiZXh0cmFpdHNfcmNzIiwiZXh0cmFpdF9jb3VydF9pbnBpIiwiY2VydGlmaWNhdF9hZ2VuY2VfYmlvIiwibXNhX2NvdGlzYXRpb25zIiwiZG9jdW1lbnRzX2Fzc29jaWF0aW9uIiwiZW9yaV9kb3VhbmVzIiwiYXNzb2NpYXRpb25zIiwiYmlsYW5zX2VudHJlcHJpc2VfYmRmIiwiZW50cmVwcmlzZXMiLCJxdWFsaWJhdCIsImNlcnRpZmljYXRfb3BxaWJpIiwiZW50cmVwcmlzZSIsImV0YWJsaXNzZW1lbnQiXSwic3ViIjoic3RhZ2luZyBkZXZlbG9wbWVudCIsImlhdCI6MTY0MTMwNDcxNCwidmVyc2lvbiI6IjEuMCJ9.6GvMpHhPXmRuY06YMym-kp_67tQhgHxDys3YIH58ws8" } + + it { expect(subject).to be_falsey } + end + + context "with a valid expiring token" do + include ActiveSupport::Testing::TimeHelpers + + # expire on Jul 4, 2023 14:58:34 + let(:token) { "eyJhbGciOiJIUzI1NiJ9.eyJ1aWQiOiI2NjRkZWEyMS02YWFlLTQwZmYtYWM0Mi1kZmQ3ZGE4YjQ3NmUiLCJqdGkiOiJhcGktZW50cmVwcmlzZS1zdGFnaW5nIiwicm9sZXMiOlsiY2VydGlmaWNhdF9jbmV0cCIsInByb2J0cCIsImV0YWJsaXNzZW1lbnRzIiwicHJpdmlsZWdlcyIsInVwdGltZSIsImF0dGVzdGF0aW9uc19hZ2VmaXBoIiwiYWN0ZXNfaW5waSIsImJpbGFuc19pbnBpIiwiYWlkZXNfY292aWRfZWZmZWN0aWZzIiwiY2VydGlmaWNhdF9yZ2VfYWRlbWUiLCJhdHRlc3RhdGlvbnNfc29jaWFsZXMiLCJlbnRyZXByaXNlX2FydGlzYW5hbGUiLCJmbnRwX2NhcnRlX3BybyIsImNvbnZlbnRpb25zX2NvbGxlY3RpdmVzIiwiZXh0cmFpdHNfcmNzIiwiZXh0cmFpdF9jb3VydF9pbnBpIiwiY2VydGlmaWNhdF9hZ2VuY2VfYmlvIiwibXNhX2NvdGlzYXRpb25zIiwiZG9jdW1lbnRzX2Fzc29jaWF0aW9uIiwiZW9yaV9kb3VhbmVzIiwiYXNzb2NpYXRpb25zIiwiYmlsYW5zX2VudHJlcHJpc2VfYmRmIiwiZW50cmVwcmlzZXMiLCJxdWFsaWJhdCIsImNlcnRpZmljYXRfb3BxaWJpIiwiZW50cmVwcmlzZSIsImV0YWJsaXNzZW1lbnQiXSwic3ViIjoic3RhZ2luZyBkZXZlbG9wbWVudCIsImlhdCI6MTY0MTMwNDcxNCwidmVyc2lvbiI6IjEuMCIsImV4cCI6MTY4ODQ3NTUxNH0.xID66pIlMnBR5_6nG-GidFBzK4Tuuy5ZsWfkMEVB_Ek" } + + it "must be false when token has not expired yet" do + travel_to Time.zone.local(2021) do + expect(subject).to be_falsey + end + end + + it "must be true when token has expired" do + travel_to Time.zone.local(2025) do + expect(subject).to be_truthy + end + end + end + end +end From e269077c40db63bf9919a16148544fe2dd3ee3f8 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Fri, 11 Feb 2022 08:45:16 +0100 Subject: [PATCH 2/5] refactor(attestation_template): cleanup relationships --- .../instructeurs/dossiers_controller.rb | 2 +- app/models/attestation_template.rb | 3 +-- app/models/dossier.rb | 9 +++----- app/models/procedure.rb | 9 ++++---- ...12184331_revise_attestation_templates.rake | 4 ++-- spec/factories/dossier.rb | 5 ++--- .../cron/declarative_procedures_job_spec.rb | 5 ++--- ...84331_revise_attestation_templates_spec.rb | 6 +++--- spec/models/attestation_template_spec.rb | 5 +++-- spec/models/procedure_spec.rb | 21 +++++++++++++------ 10 files changed, 36 insertions(+), 33 deletions(-) diff --git a/app/controllers/instructeurs/dossiers_controller.rb b/app/controllers/instructeurs/dossiers_controller.rb index ee6af02e3..78a4c3db7 100644 --- a/app/controllers/instructeurs/dossiers_controller.rb +++ b/app/controllers/instructeurs/dossiers_controller.rb @@ -32,7 +32,7 @@ module Instructeurs end def apercu_attestation - @attestation = dossier.procedure.attestation_template.render_attributes_for(dossier: dossier) + @attestation = dossier.attestation_template.render_attributes_for(dossier: dossier) render 'administrateurs/attestation_templates/show', formats: [:pdf] end diff --git a/app/models/attestation_template.rb b/app/models/attestation_template.rb index deab7b0a2..af10f136f 100644 --- a/app/models/attestation_template.rb +++ b/app/models/attestation_template.rb @@ -15,7 +15,6 @@ class AttestationTemplate < ApplicationRecord include ActionView::Helpers::NumberHelper include TagsSubstitutionConcern - belongs_to :procedure, optional: true has_many :revisions, class_name: 'ProcedureRevision', inverse_of: :attestation_template, dependent: :nullify has_one_attached :logo @@ -120,7 +119,7 @@ class AttestationTemplate < ApplicationRecord end def procedure - revisions.last&.procedure || super + revisions.last&.procedure end private diff --git a/app/models/dossier.rb b/app/models/dossier.rb index f80e09aa2..44c0b4c10 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -137,6 +137,7 @@ class Dossier < ApplicationRecord belongs_to :user, optional: true has_one :france_connect_information, through: :user + has_one :attestation_template, through: :revision has_one :procedure, through: :revision has_many :types_de_champ, through: :revision has_many :types_de_champ_private, through: :revision @@ -384,7 +385,7 @@ class Dossier < ApplicationRecord .where.not(user: users_who_submitted) end - scope :for_api_v2, -> { includes(procedure: [:administrateurs, :attestation_template], etablissement: [], individual: [], traitement: []) } + scope :for_api_v2, -> { includes(procedure: [:administrateurs], revision: [:attestation_template], etablissement: [], individual: [], traitement: []) } scope :with_notifications, -> do joins(:follows) @@ -721,10 +722,6 @@ class Dossier < ApplicationRecord { lon: lon, lat: lat, zoom: zoom } end - def attestation_template - revision.attestation_template - end - def unspecified_attestation_champs if attestation_template&.activated? attestation_template.unspecified_champs_for_dossier(self) @@ -828,7 +825,7 @@ class Dossier < ApplicationRecord end def attestation_activated? - termine? && procedure.attestation_template&.activated? + termine? && attestation_template&.activated? end def after_passer_en_construction diff --git a/app/models/procedure.rb b/app/models/procedure.rb index f3894815c..06741df9b 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -84,7 +84,7 @@ class Procedure < ApplicationRecord has_many :experts, through: :experts_procedures has_one :module_api_carto, dependent: :destroy - has_one :attestation_template, dependent: :destroy + has_one :legacy_attestation_template, class_name: 'AttestationTemplate', dependent: :destroy has_many :attestation_templates, through: :revisions, source: :attestation_template belongs_to :parent_procedure, class_name: 'Procedure', optional: true @@ -434,7 +434,6 @@ class Procedure < ApplicationRecord populate_champ_stable_ids include_list = { - attestation_template: [], draft_revision: { revision_types_de_champ: { type_de_champ: :types_de_champ @@ -580,7 +579,7 @@ class Procedure < ApplicationRecord touch(:whitelisted_at) end - def active_attestation_template + def attestation_template published_attestation_template || draft_attestation_template end @@ -588,9 +587,9 @@ class Procedure < ApplicationRecord # As an optimization, don’t check the predefined templates (they are presumed correct) if closed_mail.present? tag_present = closed_mail.body.to_s.include?("--lien attestation--") - if active_attestation_template&.activated? && !tag_present + if attestation_template&.activated? && !tag_present :missing_tag - elsif !active_attestation_template&.activated? && tag_present + elsif !attestation_template&.activated? && tag_present :extraneous_tag end end diff --git a/lib/tasks/deployment/20220112184331_revise_attestation_templates.rake b/lib/tasks/deployment/20220112184331_revise_attestation_templates.rake index 24116586c..bda63a2f8 100644 --- a/lib/tasks/deployment/20220112184331_revise_attestation_templates.rake +++ b/lib/tasks/deployment/20220112184331_revise_attestation_templates.rake @@ -4,13 +4,13 @@ namespace :after_party do rake_puts "Running deploy task 'revise_attestation_templates'" revisions = ProcedureRevision - .joins(procedure: :attestation_template) + .joins(procedure: :legacy_attestation_template) .where(attestation_template_id: nil) progress = ProgressReport.new(revisions.count) revisions.find_each do |revision| - attestation_template_id = revision.procedure.attestation_template.id + attestation_template_id = revision.procedure.legacy_attestation_template.id revision.update_column(:attestation_template_id, attestation_template_id) progress.inc diff --git a/spec/factories/dossier.rb b/spec/factories/dossier.rb index 17c270ffa..34fca0b18 100644 --- a/spec/factories/dossier.rb +++ b/spec/factories/dossier.rb @@ -188,9 +188,8 @@ FactoryBot.define do trait :with_attestation do after(:build) do |dossier, _evaluator| - if dossier.procedure.attestation_template.blank? - dossier.procedure.attestation_template = build(:attestation_template) - end + dossier.revision.attestation_template ||= build(:attestation_template) + dossier.association(:attestation_template).target = dossier.revision.attestation_template dossier.attestation = dossier.build_attestation end end diff --git a/spec/jobs/cron/declarative_procedures_job_spec.rb b/spec/jobs/cron/declarative_procedures_job_spec.rb index f64210d7a..764dda70a 100644 --- a/spec/jobs/cron/declarative_procedures_job_spec.rb +++ b/spec/jobs/cron/declarative_procedures_job_spec.rb @@ -5,8 +5,8 @@ RSpec.describe Cron::DeclarativeProceduresJob, type: :job do let(:state) { nil } let(:procedure) { create(:procedure, :published, :for_individual, :with_instructeur, declarative_with_state: state) } - let(:nouveau_dossier1) { create(:dossier, :en_construction, :with_individual, procedure: procedure) } - let(:nouveau_dossier2) { create(:dossier, :en_construction, :with_individual, procedure: procedure) } + let(:nouveau_dossier1) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure: procedure) } + let(:nouveau_dossier2) { create(:dossier, :en_construction, :with_individual, :with_attestation, procedure: procedure) } let(:dossier_recu) { create(:dossier, :en_instruction, :with_individual, procedure: procedure) } let(:dossier_brouillon) { create(:dossier, procedure: procedure) } let(:dossier_repasse_en_construction) { create(:dossier, :en_construction, :with_individual, procedure: procedure) } @@ -22,7 +22,6 @@ RSpec.describe Cron::DeclarativeProceduresJob, type: :job do dossier_repasse_en_construction ] - create(:attestation_template, procedure: procedure) Cron::DeclarativeProceduresJob.new.perform dossiers.each(&:reload) diff --git a/spec/lib/tasks/deployment/20220112184331_revise_attestation_templates_spec.rb b/spec/lib/tasks/deployment/20220112184331_revise_attestation_templates_spec.rb index 5abc757d3..3a39cfedc 100644 --- a/spec/lib/tasks/deployment/20220112184331_revise_attestation_templates_spec.rb +++ b/spec/lib/tasks/deployment/20220112184331_revise_attestation_templates_spec.rb @@ -1,7 +1,7 @@ describe '20220112184331_revise_attestation_templates' do let(:rake_task) { Rake::Task['after_party:revise_attestation_templates'] } let(:procedure) { create(:procedure) } - let(:attestation_template) { create(:attestation_template, procedure: procedure) } + let(:attestation_template) { create(:attestation_template, procedure_id: procedure.id) } subject(:run_task) do attestation_template @@ -13,10 +13,10 @@ describe '20220112184331_revise_attestation_templates' do describe 'revise_attestation_templates' do it 'attaches the attestation_template to the latest revision (without removing the link between attestation_template and procedure for now)' do - expect(attestation_template.procedure.revisions.first.attestation_template_id).to be_nil + expect(procedure.revisions.first.attestation_template_id).to be_nil run_task expect(attestation_template.procedure_id).to eq(procedure.id) - expect(attestation_template.procedure.revisions.first.attestation_template_id).to eq(attestation_template.id) + expect(procedure.revisions.first.attestation_template_id).to eq(attestation_template.id) end end end diff --git a/spec/models/attestation_template_spec.rb b/spec/models/attestation_template_spec.rb index c6778934f..ee0dbed2f 100644 --- a/spec/models/attestation_template_spec.rb +++ b/spec/models/attestation_template_spec.rb @@ -95,7 +95,8 @@ describe AttestationTemplate, type: :model do create(:procedure, types_de_champ: types_de_champ, types_de_champ_private: types_de_champ_private, - for_individual: for_individual) + for_individual: for_individual, + attestation_template: attestation_template) end let(:for_individual) { false } let(:individual) { nil } @@ -106,7 +107,7 @@ describe AttestationTemplate, type: :model do let(:template_title) { 'title' } let(:template_body) { 'body' } let(:attestation_template) do - build(:attestation_template, procedure: procedure, + build(:attestation_template, title: template_title, body: template_body, logo: @logo, diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 4602aa7dd..88f2b04e4 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -40,7 +40,8 @@ describe Procedure do end describe 'closed mail template body' do - let(:procedure) { create(:procedure) } + let(:procedure) { create(:procedure, attestation_template: attestation_template) } + let(:attestation_template) { nil } subject { procedure.closed_mail_template.rich_body.body.to_html } @@ -49,7 +50,7 @@ describe Procedure do end context 'for procedures with an attestation' do - before { create(:attestation_template, procedure: procedure, activated: activated) } + let(:attestation_template) { build(:attestation_template, activated: activated) } context 'when the attestation is inactive' do let(:activated) { false } @@ -414,7 +415,16 @@ describe Procedure do describe 'clone' do let(:service) { create(:service) } - let(:procedure) { create(:procedure, received_mail: received_mail, service: service, types_de_champ: [type_de_champ_0, type_de_champ_1, type_de_champ_2, type_de_champ_pj, type_de_champ_repetition], types_de_champ_private: [type_de_champ_private_0, type_de_champ_private_1, type_de_champ_private_2, type_de_champ_private_repetition], api_particulier_token: '123456789012345', api_particulier_scopes: ['cnaf_famille']) } + let(:procedure) do + create(:procedure, + received_mail: received_mail, + service: service, + attestation_template: build(:attestation_template, logo: logo, signature: signature), + types_de_champ: [type_de_champ_0, type_de_champ_1, type_de_champ_2, type_de_champ_pj, type_de_champ_repetition], + types_de_champ_private: [type_de_champ_private_0, type_de_champ_private_1, type_de_champ_private_2, type_de_champ_private_repetition], + api_particulier_token: '123456789012345', + api_particulier_scopes: ['cnaf_famille']) + end let(:type_de_champ_0) { build(:type_de_champ, position: 0) } let(:type_de_champ_1) { build(:type_de_champ, position: 1) } let(:type_de_champ_2) { build(:type_de_champ_drop_down_list, position: 2) } @@ -427,6 +437,8 @@ describe Procedure do let(:received_mail) { build(:received_mail) } let(:from_library) { false } let(:administrateur) { procedure.administrateurs.first } + let(:logo) { Rack::Test::UploadedFile.new('spec/fixtures/files/white.png', 'image/png') } + let(:signature) { Rack::Test::UploadedFile.new('spec/fixtures/files/black.png', 'image/png') } let(:groupe_instructeur_1) { create(:groupe_instructeur, procedure: procedure, label: "groupe_1") } let(:instructeur_1) { create(:instructeur) } @@ -435,9 +447,6 @@ describe Procedure do let!(:assign_to_2) { create(:assign_to, procedure: procedure, groupe_instructeur: groupe_instructeur_1, instructeur: instructeur_2) } before do - @logo = Rack::Test::UploadedFile.new('spec/fixtures/files/white.png', 'image/png') - @signature = Rack::Test::UploadedFile.new('spec/fixtures/files/black.png', 'image/png') - @attestation_template = create(:attestation_template, procedure: procedure, logo: @logo, signature: @signature) @procedure = procedure.clone(administrateur, from_library) @procedure.save end From 76b1b85fa7a5d45e01b05ae77855824036ce7a78 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 10 Feb 2022 19:42:39 +0100 Subject: [PATCH 3/5] fix(attestation_template): add revision diff support --- app/models/attestation_template.rb | 16 ++++ app/models/concerns/dossier_rebase_concern.rb | 4 +- app/models/procedure_revision.rb | 82 +++++++++++++++++-- ...sion_change_attestation_template.html.haml | 15 ++++ .../_revision_change_type_de_champ.html.haml | 56 +++++++++++++ .../procedures/_revision_changes.html.haml | 61 +------------- .../administrateurs/revision_changes/fr.yml | 7 ++ spec/models/procedure_revision_spec.rb | 24 ++++++ 8 files changed, 199 insertions(+), 66 deletions(-) create mode 100644 app/views/administrateurs/procedures/_revision_change_attestation_template.html.haml create mode 100644 app/views/administrateurs/procedures/_revision_change_type_de_champ.html.haml diff --git a/app/models/attestation_template.rb b/app/models/attestation_template.rb index af10f136f..668447938 100644 --- a/app/models/attestation_template.rb +++ b/app/models/attestation_template.rb @@ -122,6 +122,22 @@ class AttestationTemplate < ApplicationRecord revisions.last&.procedure end + def logo_checksum + logo.attached? ? logo.checksum : nil + end + + def signature_checksum + signature.attached? ? signature.checksum : nil + end + + def logo_filename + logo.attached? ? logo.filename : nil + end + + def signature_filename + signature.attached? ? signature.filename : nil + end + private def used_tags diff --git a/app/models/concerns/dossier_rebase_concern.rb b/app/models/concerns/dossier_rebase_concern.rb index 004d3f405..1117b7ebe 100644 --- a/app/models/concerns/dossier_rebase_concern.rb +++ b/app/models/concerns/dossier_rebase_concern.rb @@ -14,7 +14,9 @@ module DossierRebaseConcern def rebase attachments_to_purge = [] geo_areas_to_delete = [] - changes_by_type_de_champ = revision.compare(procedure.published_revision).group_by { |change| change[:stable_id] } + changes_by_type_de_champ = revision.compare(procedure.published_revision) + .filter { |change| change[:model] == :type_de_champ } + .group_by { |change| change[:stable_id] } changes_by_type_de_champ.each do |stable_id, changes| type_de_champ = find_type_de_champ_by_stable_id(stable_id) diff --git a/app/models/procedure_revision.rb b/app/models/procedure_revision.rb index da60bb296..b1fd83078 100644 --- a/app/models/procedure_revision.rb +++ b/app/models/procedure_revision.rb @@ -108,13 +108,16 @@ class ProcedureRevision < ApplicationRecord end def different_from?(revision) - types_de_champ != revision.types_de_champ || types_de_champ_private != revision.types_de_champ_private + types_de_champ != revision.types_de_champ || + types_de_champ_private != revision.types_de_champ_private || + attestation_template != revision.attestation_template end def compare(revision) changes = [] changes += compare_types_de_champ(types_de_champ, revision.types_de_champ) changes += compare_types_de_champ(types_de_champ_private, revision.types_de_champ_private) + changes += compare_attestation_template(attestation_template, revision.attestation_template) changes end @@ -127,12 +130,65 @@ class ProcedureRevision < ApplicationRecord ) end - def attestation_template - super || procedure.attestation_template - end - private + def compare_attestation_template(from_at, to_at) + changes = [] + if from_at.nil? && to_at.present? + changes << { + model: :attestation_template, + op: :add + } + elsif to_at.present? + if from_at.title != to_at.title + changes << { + model: :attestation_template, + op: :update, + attribute: :title, + from: from_at.title, + to: to_at.title + } + end + if from_at.body != to_at.body + changes << { + model: :attestation_template, + op: :update, + attribute: :body, + from: from_at.body, + to: to_at.body + } + end + if from_at.footer != to_at.footer + changes << { + model: :attestation_template, + op: :update, + attribute: :footer, + from: from_at.footer, + to: to_at.footer + } + end + if from_at.logo_checksum != to_at.logo_checksum + changes << { + model: :attestation_template, + op: :update, + attribute: :logo, + from: from_at.logo_filename, + to: to_at.logo_filename + } + end + if from_at.signature_checksum != to_at.signature_checksum + changes << { + model: :attestation_template, + op: :update, + attribute: :signature, + from: from_at.signature_filename, + to: to_at.signature_filename + } + end + end + changes + end + def compare_types_de_champ(from_tdc, to_tdc) if from_tdc == to_tdc [] @@ -144,11 +200,11 @@ class ProcedureRevision < ApplicationRecord to_sids = to_h.keys removed = (from_sids - to_sids).map do |sid| - { op: :remove, label: from_h[sid].libelle, private: from_h[sid].private?, position: from_sids.index(sid), stable_id: sid } + { model: :type_de_champ, op: :remove, label: from_h[sid].libelle, private: from_h[sid].private?, position: from_sids.index(sid), stable_id: sid } end added = (to_sids - from_sids).map do |sid| - { op: :add, label: to_h[sid].libelle, private: to_h[sid].private?, position: to_sids.index(sid), stable_id: sid } + { model: :type_de_champ, op: :add, label: to_h[sid].libelle, private: to_h[sid].private?, position: to_sids.index(sid), stable_id: sid } end kept = from_sids.intersection(to_sids) @@ -157,7 +213,7 @@ class ProcedureRevision < ApplicationRecord .map { |sid| [sid, from_sids.index(sid), to_sids.index(sid)] } .filter { |_, from_index, to_index| from_index != to_index } .map do |sid, from_index, to_index| - { op: :move, label: from_h[sid].libelle, private: from_h[sid].private?, from: from_index, to: to_index, position: to_index, stable_id: sid } + { model: :type_de_champ, op: :move, label: from_h[sid].libelle, private: from_h[sid].private?, from: from_index, to: to_index, position: to_index, stable_id: sid } end changed = kept @@ -177,6 +233,7 @@ class ProcedureRevision < ApplicationRecord changes = [] if from_type_de_champ.type_champ != to_type_de_champ.type_champ changes << { + model: :type_de_champ, op: :update, attribute: :type_champ, label: from_type_de_champ.libelle, @@ -188,6 +245,7 @@ class ProcedureRevision < ApplicationRecord end if from_type_de_champ.libelle != to_type_de_champ.libelle changes << { + model: :type_de_champ, op: :update, attribute: :libelle, label: from_type_de_champ.libelle, @@ -199,6 +257,7 @@ class ProcedureRevision < ApplicationRecord end if from_type_de_champ.description != to_type_de_champ.description changes << { + model: :type_de_champ, op: :update, attribute: :description, label: from_type_de_champ.libelle, @@ -210,6 +269,7 @@ class ProcedureRevision < ApplicationRecord end if from_type_de_champ.mandatory? != to_type_de_champ.mandatory? changes << { + model: :type_de_champ, op: :update, attribute: :mandatory, label: from_type_de_champ.libelle, @@ -222,6 +282,7 @@ class ProcedureRevision < ApplicationRecord if to_type_de_champ.drop_down_list? if from_type_de_champ.drop_down_list_options != to_type_de_champ.drop_down_list_options changes << { + model: :type_de_champ, op: :update, attribute: :drop_down_options, label: from_type_de_champ.libelle, @@ -234,6 +295,7 @@ class ProcedureRevision < ApplicationRecord if to_type_de_champ.linked_drop_down_list? if from_type_de_champ.drop_down_secondary_libelle != to_type_de_champ.drop_down_secondary_libelle changes << { + model: :type_de_champ, op: :update, attribute: :drop_down_secondary_libelle, label: from_type_de_champ.libelle, @@ -244,6 +306,7 @@ class ProcedureRevision < ApplicationRecord end if from_type_de_champ.drop_down_secondary_description != to_type_de_champ.drop_down_secondary_description changes << { + model: :type_de_champ, op: :update, attribute: :drop_down_secondary_description, label: from_type_de_champ.libelle, @@ -255,6 +318,7 @@ class ProcedureRevision < ApplicationRecord end if from_type_de_champ.drop_down_other != to_type_de_champ.drop_down_other changes << { + model: :type_de_champ, op: :update, attribute: :drop_down_other, label: from_type_de_champ.libelle, @@ -267,6 +331,7 @@ class ProcedureRevision < ApplicationRecord elsif to_type_de_champ.carte? if from_type_de_champ.carte_optional_layers != to_type_de_champ.carte_optional_layers changes << { + model: :type_de_champ, op: :update, attribute: :carte_layers, label: from_type_de_champ.libelle, @@ -279,6 +344,7 @@ class ProcedureRevision < ApplicationRecord elsif to_type_de_champ.piece_justificative? if from_type_de_champ.piece_justificative_template_checksum != to_type_de_champ.piece_justificative_template_checksum changes << { + model: :type_de_champ, op: :update, attribute: :piece_justificative_template, label: from_type_de_champ.libelle, diff --git a/app/views/administrateurs/procedures/_revision_change_attestation_template.html.haml b/app/views/administrateurs/procedures/_revision_change_attestation_template.html.haml new file mode 100644 index 000000000..690c5841b --- /dev/null +++ b/app/views/administrateurs/procedures/_revision_change_attestation_template.html.haml @@ -0,0 +1,15 @@ +- case change[:op] +- when :add + %li.mb-1= t(:add, scope: [:administrateurs, :revision_changes, :attestation_template]) +- when :update + - case change[:attribute] + - when :title + %li.mb-1= t(:update_title, scope: [:administrateurs, :revision_changes, :attestation_template], to: change[:to]) + - when :body + %li.mb-1= t(:update_body, scope: [:administrateurs, :revision_changes, :attestation_template]) + - when :footer + %li.mb-1= t(:update_footer, scope: [:administrateurs, :revision_changes, :attestation_template]) + - when :logo + %li.mb-1= t(:update_logo, scope: [:administrateurs, :revision_changes, :attestation_template], to: change[:to]) + - when :signature + %li.mb-1= t(:update_signature, scope: [:administrateurs, :revision_changes, :attestation_template], to: change[:to]) diff --git a/app/views/administrateurs/procedures/_revision_change_type_de_champ.html.haml b/app/views/administrateurs/procedures/_revision_change_type_de_champ.html.haml new file mode 100644 index 000000000..d2e13821f --- /dev/null +++ b/app/views/administrateurs/procedures/_revision_change_type_de_champ.html.haml @@ -0,0 +1,56 @@ +- postfix = change[:private] ? '_private' : '' +- case change[:op] +- when :add + %li.mb-1= t("add#{postfix}", label: change[:label], scope: [:administrateurs, :revision_changes]) +- when :remove + %li.mb-1= t("remove#{postfix}", label: change[:label], scope: [:administrateurs, :revision_changes]) +- when :update + - case change[:attribute] + - when :libelle + %li.mb-1= t("update_libelle#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) + - when :type_champ + %li.mb-1= t("update_type_champ#{postfix}", label: change[:label], to: t("activerecord.attributes.type_de_champ.type_champs.#{change[:to]}"), scope: [:administrateurs, :revision_changes]) + - when :description + %li.mb-1= t("update_description#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) + - when :drop_down_secondary_libelle + %li.mb-1= t("update_drop_down_secondary_libelle#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) + - when :drop_down_secondary_description + %li.mb-1= t("update_drop_down_secondary_description#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) + - when :mandatory + - if change[:from] == false + -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory.enabled') + -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory_private.enabled') + %li.mb-1= t("administrateurs.revision_changes.update_mandatory#{postfix}.enabled", label: change[:label]) + - else + -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory.disabled') + -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory_private.disabled') + %li.mb-1= t("administrateurs.revision_changes.update_mandatory#{postfix}.disabled", label: change[:label]) + - when :piece_justificative_template + -# i18n-tasks-use t('administrateurs.revision_changes.update_piece_justificative_template') + -# i18n-tasks-use t('administrateurs.revision_changes.update_piece_justificative_template_private') + %li.mb-1= t("administrateurs.revision_changes.update_piece_justificative_template#{postfix}", label: change[:label]) + - when :drop_down_options + - added = change[:to].sort - change[:from].sort + - removed = change[:from].sort - change[:to].sort + %li.mb-1 + = t("update_drop_down_options#{postfix}", scope: [:administrateurs, :revision_changes], label: change[:label]) + %ul + - if added.present? + %li= t(:add_option, scope: [:administrateurs, :revision_changes], items: added.map{ |term| "« #{term.strip} »" }.join(", ")) + - if removed.present? + %li= t(:remove_option, scope: [:administrateurs, :revision_changes], items: removed.map{ |term| "« #{term.strip} »" }.join(", ")) + - when :drop_down_other + - if change[:from] == false + %li.mb-1= t("administrateurs.revision_changes.update_drop_down_other#{postfix}.enabled", label: change[:label]) + - else + %li.mb-1= t("administrateurs.revision_changes.update_drop_down_other#{postfix}.disabled", label: change[:label]) + - when :carte_layers + - added = change[:to].sort - change[:from].sort + - removed = change[:from].sort - change[:to].sort + %li.mb-1 + = t("update_carte_layers#{postfix}", scope: [:administrateurs, :revision_changes], label: change[:label]) + %ul + - if added.present? + %li= t(:add_option, scope: [:administrateurs, :revision_changes], items: added.map{ |term| "« #{t(term, scope: [:administrateurs, :carte_layers])} »" }.join(", ")) + - if removed.present? + %li= t(:remove_option, scope: [:administrateurs, :revision_changes], items: removed.map{ |term| "« #{t(term, scope: [:administrateurs, :carte_layers])} »" }.join(", ")) diff --git a/app/views/administrateurs/procedures/_revision_changes.html.haml b/app/views/administrateurs/procedures/_revision_changes.html.haml index d56915cb9..edcd43345 100644 --- a/app/views/administrateurs/procedures/_revision_changes.html.haml +++ b/app/views/administrateurs/procedures/_revision_changes.html.haml @@ -1,61 +1,8 @@ %ul.revision-changes - - changes.each do |change| - - postfix = change[:private] ? '_private' : '' - - case change[:op] - - when :add - %li.mb-1= t("add#{postfix}", label: change[:label], scope: [:administrateurs, :revision_changes]) - - when :remove - %li.mb-1= t("remove#{postfix}", label: change[:label], scope: [:administrateurs, :revision_changes]) - - when :update - - case change[:attribute] - - when :libelle - %li.mb-1= t("update_libelle#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) - - when :type_champ - %li.mb-1= t("update_type_champ#{postfix}", label: change[:label], to: t("activerecord.attributes.type_de_champ.type_champs.#{change[:to]}"), scope: [:administrateurs, :revision_changes]) - - when :description - %li.mb-1= t("update_description#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) - - when :drop_down_secondary_libelle - %li.mb-1= t("update_drop_down_secondary_libelle#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) - - when :drop_down_secondary_description - %li.mb-1= t("update_drop_down_secondary_description#{postfix}", label: change[:label], to: change[:to], scope: [:administrateurs, :revision_changes]) - - when :mandatory - - if change[:from] == false - -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory.enabled') - -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory_private.enabled') - %li.mb-1= t("administrateurs.revision_changes.update_mandatory#{postfix}.enabled", label: change[:label]) - - else - -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory.disabled') - -# i18n-tasks-use t('administrateurs.revision_changes.update_mandatory_private.disabled') - %li.mb-1= t("administrateurs.revision_changes.update_mandatory#{postfix}.disabled", label: change[:label]) - - when :piece_justificative_template - -# i18n-tasks-use t('administrateurs.revision_changes.update_piece_justificative_template') - -# i18n-tasks-use t('administrateurs.revision_changes.update_piece_justificative_template_private') - %li.mb-1= t("administrateurs.revision_changes.update_piece_justificative_template#{postfix}", label: change[:label]) - - when :drop_down_options - - added = change[:to].sort - change[:from].sort - - removed = change[:from].sort - change[:to].sort - %li.mb-1 - = t("update_drop_down_options#{postfix}", scope: [:administrateurs, :revision_changes], label: change[:label]) - %ul - - if added.present? - %li= t(:add_option, scope: [:administrateurs, :revision_changes], items: added.map{ |term| "« #{term.strip} »" }.join(", ")) - - if removed.present? - %li= t(:remove_option, scope: [:administrateurs, :revision_changes], items: removed.map{ |term| "« #{term.strip} »" }.join(", ")) - - when :drop_down_other - - if change[:from] == false - %li.mb-1= t("administrateurs.revision_changes.update_drop_down_other#{postfix}.enabled", label: change[:label]) - - else - %li.mb-1= t("administrateurs.revision_changes.update_drop_down_other#{postfix}.disabled", label: change[:label]) - - when :carte_layers - - added = change[:to].sort - change[:from].sort - - removed = change[:from].sort - change[:to].sort - %li.mb-1 - = t("update_carte_layers#{postfix}", scope: [:administrateurs, :revision_changes], label: change[:label]) - %ul - - if added.present? - %li= t(:add_option, scope: [:administrateurs, :revision_changes], items: added.map{ |term| "« #{t(term, scope: [:administrateurs, :carte_layers])} »" }.join(", ")) - - if removed.present? - %li= t(:remove_option, scope: [:administrateurs, :revision_changes], items: removed.map{ |term| "« #{t(term, scope: [:administrateurs, :carte_layers])} »" }.join(", ")) + - changes.filter { |change| change[:model] == :attestation_template }.each do |change| + = render partial: 'administrateurs/procedures/revision_change_attestation_template', locals: { change: change } + - changes.filter { |change| change[:model] == :type_de_champ }.each do |change| + = render partial: 'administrateurs/procedures/revision_change_type_de_champ', locals: { change: change } - move_changes, move_private_changes = changes.filter { |change| change[:op] == :move }.partition { |change| !change[:private] } - if move_changes.size != 0 %li.mb-1= t(:move, scope: [:administrateurs, :revision_changes], count: move_changes.size) diff --git a/config/locales/views/administrateurs/revision_changes/fr.yml b/config/locales/views/administrateurs/revision_changes/fr.yml index 53ca50c99..4f1be5a0b 100644 --- a/config/locales/views/administrateurs/revision_changes/fr.yml +++ b/config/locales/views/administrateurs/revision_changes/fr.yml @@ -1,6 +1,13 @@ fr: administrateurs: revision_changes: + attestation_template: + add: Un model d’attestation à été ajouté + update_title: Le titre de l’attestation à été modifié. Le nouveau titre est « %{to} » + update_body: Le corps du document de l’attestation à été modifié + update_footer: Le pied de page de l’attestation à été modifié + update_logo: Le logo de l’attestation à été modifié. Le nouveau logo est « %{to} » + update_signature: La signature de l’attestation à été modifié. La nouvelle signature est « %{to} » has_changes: Modifications en cours (appliqué à la prochaine publication) add: Le champ « %{label} » a été ajouté remove: Le champ « %{label} » a été supprimé diff --git a/spec/models/procedure_revision_spec.rb b/spec/models/procedure_revision_spec.rb index 49df242ae..c989f951a 100644 --- a/spec/models/procedure_revision_spec.rb +++ b/spec/models/procedure_revision_spec.rb @@ -172,6 +172,7 @@ describe ProcedureRevision do expect(procedure.active_revision.different_from?(new_revision)).to be_truthy expect(procedure.active_revision.compare(new_revision)).to eq([ { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, @@ -182,6 +183,7 @@ describe ProcedureRevision do new_revision.find_or_clone_type_de_champ(new_revision.types_de_champ.first.stable_id).update(libelle: 'modifier le libelle') expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :update, attribute: :libelle, label: type_de_champ_first.libelle, @@ -191,6 +193,7 @@ describe ProcedureRevision do stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, @@ -202,6 +205,7 @@ describe ProcedureRevision do new_revision.move_type_de_champ(new_revision.types_de_champ.second.stable_id, 2) expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :update, attribute: :libelle, label: type_de_champ_first.libelle, @@ -211,12 +215,14 @@ describe ProcedureRevision do stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, stable_id: new_type_de_champ.stable_id }, { + model: :type_de_champ, op: :move, label: type_de_champ_second.libelle, private: false, @@ -230,12 +236,14 @@ describe ProcedureRevision do new_revision.remove_type_de_champ(new_revision.types_de_champ.first.stable_id) expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :remove, label: type_de_champ_first.libelle, private: false, stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, @@ -247,18 +255,21 @@ describe ProcedureRevision do new_revision.find_or_clone_type_de_champ(new_revision.types_de_champ.last.stable_id).update(mandatory: true) expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :remove, label: type_de_champ_first.libelle, private: false, stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, stable_id: new_type_de_champ.stable_id }, { + model: :type_de_champ, op: :update, attribute: :description, label: type_de_champ_second.libelle, @@ -268,6 +279,7 @@ describe ProcedureRevision do stable_id: type_de_champ_second.stable_id }, { + model: :type_de_champ, op: :update, attribute: :mandatory, label: type_de_champ_second.libelle, @@ -282,18 +294,21 @@ describe ProcedureRevision do new_revision.find_or_clone_type_de_champ(new_revision.types_de_champ.last.types_de_champ.first.stable_id).update(drop_down_options: ['one', 'two']) expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :remove, label: type_de_champ_first.libelle, private: false, stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, stable_id: new_type_de_champ.stable_id }, { + model: :type_de_champ, op: :update, attribute: :description, label: type_de_champ_second.libelle, @@ -303,6 +318,7 @@ describe ProcedureRevision do stable_id: type_de_champ_second.stable_id }, { + model: :type_de_champ, op: :update, attribute: :mandatory, label: type_de_champ_second.libelle, @@ -312,6 +328,7 @@ describe ProcedureRevision do stable_id: type_de_champ_second.stable_id }, { + model: :type_de_champ, op: :update, attribute: :type_champ, label: "sub type de champ", @@ -321,6 +338,7 @@ describe ProcedureRevision do stable_id: new_revision.types_de_champ.last.types_de_champ.first.stable_id }, { + model: :type_de_champ, op: :update, attribute: :drop_down_options, label: "sub type de champ", @@ -335,18 +353,21 @@ describe ProcedureRevision do new_revision.find_or_clone_type_de_champ(new_revision.types_de_champ.last.types_de_champ.first.stable_id).update(options: { cadastres: true, znieff: true }) expect(procedure.active_revision.compare(new_revision.reload)).to eq([ { + model: :type_de_champ, op: :remove, label: type_de_champ_first.libelle, private: false, stable_id: type_de_champ_first.stable_id }, { + model: :type_de_champ, op: :add, label: "Un champ text", private: false, stable_id: new_type_de_champ.stable_id }, { + model: :type_de_champ, op: :update, attribute: :description, label: type_de_champ_second.libelle, @@ -356,6 +377,7 @@ describe ProcedureRevision do stable_id: type_de_champ_second.stable_id }, { + model: :type_de_champ, op: :update, attribute: :mandatory, label: type_de_champ_second.libelle, @@ -365,6 +387,7 @@ describe ProcedureRevision do stable_id: type_de_champ_second.stable_id }, { + model: :type_de_champ, op: :update, attribute: :type_champ, label: "sub type de champ", @@ -374,6 +397,7 @@ describe ProcedureRevision do stable_id: new_revision.types_de_champ.last.types_de_champ.first.stable_id }, { + model: :type_de_champ, op: :update, attribute: :carte_layers, label: "sub type de champ", From 08030bcdb776b38f428f3a6f85b280dc9791bc58 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Fri, 11 Feb 2022 08:43:29 +0100 Subject: [PATCH 4/5] fix(attestation_template): handle attestation changes on procedures without revisions --- .../attestation_templates_controller.rb | 7 +- app/models/attestation_template.rb | 16 ++-- .../attestation_templates_controller_spec.rb | 75 ++++++++++++++++++- spec/factories/procedure_revision.rb | 1 + 4 files changed, 90 insertions(+), 9 deletions(-) diff --git a/app/controllers/administrateurs/attestation_templates_controller.rb b/app/controllers/administrateurs/attestation_templates_controller.rb index d1f66ebad..939cb8a25 100644 --- a/app/controllers/administrateurs/attestation_templates_controller.rb +++ b/app/controllers/administrateurs/attestation_templates_controller.rb @@ -7,7 +7,7 @@ module Administrateurs end def update - attestation_template = @procedure.draft_attestation_template.revise! + attestation_template = @procedure.draft_attestation_template.find_or_revise! if attestation_template.update(activated_attestation_params) AttestationTemplate @@ -26,6 +26,11 @@ module Administrateurs attestation_template = build_attestation(activated_attestation_params) if attestation_template.save + if @procedure.publiee? && !@procedure.feature_enabled?(:procedure_revisions) + # If revisions support is not enabled and procedure is published + # attach the same attestation template to published revision. + @procedure.published_revision.update(attestation_template: attestation_template) + end flash.notice = "L'attestation a bien été sauvegardée" else flash.alert = attestation_template.errors.full_messages.join('
') diff --git a/app/models/attestation_template.rb b/app/models/attestation_template.rb index 668447938..eebfc83f6 100644 --- a/app/models/attestation_template.rb +++ b/app/models/attestation_template.rb @@ -103,17 +103,19 @@ class AttestationTemplate < ApplicationRecord } end - def revise! - if revisions.size > 1 + def find_or_revise! + if revisions.size > 1 && procedure.feature_enabled?(:procedure_revisions) + # If attestation template belongs to more then one revision + # and procedure has revisions enabled – revise attestation template. attestation_template = dup attestation_template.save! - revisions - .last - .procedure - .draft_revision - .update!(attestation_template: attestation_template) + procedure.draft_revision.update!(attestation_template: attestation_template) attestation_template else + # If procedure has only one revision or revisions are not supported + # apply updates directly to the attestation template. + # If it is a published procedure with revisions disabled, + # draft and published attestation template will be the same. self end end diff --git a/spec/controllers/administrateurs/attestation_templates_controller_spec.rb b/spec/controllers/administrateurs/attestation_templates_controller_spec.rb index 451bd30f0..5d56b2d39 100644 --- a/spec/controllers/administrateurs/attestation_templates_controller_spec.rb +++ b/spec/controllers/administrateurs/attestation_templates_controller_spec.rb @@ -3,7 +3,7 @@ include ActionDispatch::TestProcess describe Administrateurs::AttestationTemplatesController, type: :controller do let(:admin) { create(:administrateur) } let(:attestation_template) { build(:attestation_template) } - let!(:procedure) { create :procedure, administrateur: admin, attestation_template: attestation_template } + let(:procedure) { create :procedure, administrateur: admin, attestation_template: attestation_template } let(:logo) { fixture_file_upload('spec/fixtures/files/white.png', 'image/png') } let(:logo2) { fixture_file_upload('spec/fixtures/files/white.png', 'image/png') } let(:signature) { fixture_file_upload('spec/fixtures/files/black.png', 'image/png') } @@ -13,6 +13,7 @@ describe Administrateurs::AttestationTemplatesController, type: :controller do let(:invalid_logo) { fixture_file_upload('spec/fixtures/files/invalid_file_format.json', 'application/json') } before do + procedure sign_in(admin.user) Timecop.freeze(Time.zone.now) end @@ -142,6 +143,42 @@ describe Administrateurs::AttestationTemplatesController, type: :controller do it { expect(flash.alert).to be_present } it { expect(procedure.draft_attestation_template).to be nil } end + + context 'when procedure is published' do + let(:procedure) { create(:procedure, :published, administrateur: admin, attestation_template: attestation_template) } + let(:attestation_template) { nil } + let(:attestation_params) { { title: 't', body: 'b', footer: '', activated: true } } + let(:revisions_enabled) { false } + + before do + if revisions_enabled + Flipper.enable(:procedure_revisions, procedure) + end + + post :create, + params: { + procedure_id: procedure.id, + attestation_template: attestation_params + } + procedure.reload + end + + context 'and revisions are not activated' do + it do + expect(procedure.draft_attestation_template).to eq(procedure.published_attestation_template) + expect(procedure.draft_attestation_template.title).to eq('t') + end + end + + context 'and revisions are activated' do + let(:revisions_enabled) { true } + it do + expect(procedure.draft_attestation_template).not_to eq(procedure.published_attestation_template) + expect(procedure.draft_attestation_template.title).to eq('t') + expect(procedure.published_attestation_template).to be_nil + end + end + end end describe 'PATCH #update' do @@ -184,5 +221,41 @@ describe Administrateurs::AttestationTemplatesController, type: :controller do it { expect(response).to redirect_to edit_admin_procedure_attestation_template_path(procedure) } it { expect(flash.alert).to eq('nop') } end + + context 'when procedure is published' do + let(:procedure) { create(:procedure, :published, administrateur: admin, attestation_template: attestation_template) } + let(:attestation_template) { create(:attestation_template, title: 'a') } + let(:attestation_params) { { title: 't', body: 'b', footer: '', activated: true } } + let(:revisions_enabled) { false } + + before do + if revisions_enabled + Flipper.enable(:procedure_revisions, procedure) + end + + patch :update, + params: { + procedure_id: procedure.id, + attestation_template: attestation_params + } + procedure.reload + end + + context 'and revisions are not activated' do + it do + expect(procedure.draft_attestation_template).to eq(procedure.published_attestation_template) + expect(procedure.draft_attestation_template.title).to eq('t') + end + end + + context 'and revisions are activated' do + let(:revisions_enabled) { true } + it do + expect(procedure.draft_attestation_template).not_to eq(procedure.published_attestation_template) + expect(procedure.draft_attestation_template.title).to eq('t') + expect(procedure.published_attestation_template.title).to eq('a') + end + end + end end end diff --git a/spec/factories/procedure_revision.rb b/spec/factories/procedure_revision.rb index 1d7ee0fc3..d2887aa9f 100644 --- a/spec/factories/procedure_revision.rb +++ b/spec/factories/procedure_revision.rb @@ -9,6 +9,7 @@ FactoryBot.define do original = evaluator.from_original revision.procedure = original.procedure + revision.attestation_template_id = original.attestation_template_id 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 From fac77d97efd98012f5def7eb25ab859192427075 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Fri, 11 Feb 2022 10:46:09 +0100 Subject: [PATCH 5/5] task(attestation_template): reassign_redundant_attestation_templates --- ...ssign_redundant_attestation_templates.rake | 28 ++++++++++ ...gn_redundant_attestation_templates_spec.rb | 51 +++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates.rake create mode 100644 spec/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates_spec.rb diff --git a/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates.rake b/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates.rake new file mode 100644 index 000000000..094e567bc --- /dev/null +++ b/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates.rake @@ -0,0 +1,28 @@ +namespace :after_party do + desc 'Deployment task: reassign_redundant_attestation_templates' + task reassign_redundant_attestation_templates: :environment do + rake_puts "Running deploy task 'reassign_redundant_attestation_templates'" + + procedures = Procedure.publiees_ou_closes.joins(:draft_attestation_template, :published_attestation_template) + progress = ProgressReport.new(procedures.count) + + # On all published procedures with disabled revisions draft_attestation_template should be the same as published_attestation_template + # Let's not destroy redundant attestation_templates for now. We can clean orphans later. + procedures.find_each do |procedure| + progress.inc + if !procedure.feature_enabled?(:procedure_revisions) + draft_attestation_template = procedure.draft_attestation_template + published_attestation_template = procedure.published_attestation_template + if draft_attestation_template && published_attestation_template && draft_attestation_template != published_attestation_template + procedure.published_revision.update(attestation_template_id: draft_attestation_template.id) + end + end + end + progress.finish + + # Update task as completed. If you remove the line below, the task will + # run with every deploy (or every time you call after_party:run). + AfterParty::TaskRecord + .create version: AfterParty::TaskRecorder.new(__FILE__).timestamp + end +end diff --git a/spec/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates_spec.rb b/spec/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates_spec.rb new file mode 100644 index 000000000..908aeaa4a --- /dev/null +++ b/spec/lib/tasks/deployment/20220211090402_reassign_redundant_attestation_templates_spec.rb @@ -0,0 +1,51 @@ +describe '20220211090402_reassign_redundant_attestation_templates' do + let(:rake_task) { Rake::Task['after_party:reassign_redundant_attestation_templates'] } + let(:procedure) { create(:procedure, :published) } + let(:procedure_with_revisions) { create(:procedure, :published) } + + before do + procedure.published_revision.update(attestation_template: create(:attestation_template)) + procedure.draft_revision.update(attestation_template: procedure.published_attestation_template.dup) + + Flipper.enable(:procedure_revisions, procedure_with_revisions) + procedure_with_revisions.published_revision.update(attestation_template: create(:attestation_template)) + procedure_with_revisions.draft_revision.update(attestation_template: procedure_with_revisions.published_attestation_template.dup) + end + + subject(:run_task) do + rake_task.invoke + procedure.reload + procedure_with_revisions.reload + end + + after { rake_task.reenable } + + describe 'reassign_redundant_attestation_templates' do + it 'reassign draft attestation template as published attestation template on procedures without revisions' do + expect(procedure.published_attestation_template).not_to be_nil + expect(procedure.draft_attestation_template).not_to be_nil + expect(procedure.draft_attestation_template).not_to eq(procedure.published_attestation_template) + + expect(procedure_with_revisions.published_attestation_template).not_to be_nil + expect(procedure_with_revisions.draft_attestation_template).not_to be_nil + expect(procedure_with_revisions.draft_attestation_template).not_to eq(procedure_with_revisions.published_attestation_template) + + orphans = AttestationTemplate.where(procedure_id: nil).left_outer_joins(:revisions).filter { |a| a.revisions.empty? } + expect(orphans).to eq([]) + to_be_orphan = procedure.published_attestation_template + + run_task + + expect(procedure.published_attestation_template).not_to be_nil + expect(procedure.draft_attestation_template).not_to be_nil + expect(procedure.draft_attestation_template).to eq(procedure.published_attestation_template) + + expect(procedure_with_revisions.published_attestation_template).not_to be_nil + expect(procedure_with_revisions.draft_attestation_template).not_to be_nil + expect(procedure_with_revisions.draft_attestation_template).not_to eq(procedure_with_revisions.published_attestation_template) + + orphans = AttestationTemplate.where(procedure_id: nil).left_outer_joins(:revisions).filter { |a| a.revisions.empty? } + expect(orphans).to eq([to_be_orphan]) + end + end +end