From 9dd1973e18f880afa3060109c9193fe51ff5f988 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Mon, 16 Dec 2024 13:08:56 +0000 Subject: [PATCH 1/2] fix(dossier): always remove previous champ version If champ type_de_champ gets out of sync with its type, the persisted champ will not be part of the filled champs collection. In the merge code, we need to remove the previous champ, disregarding its type. The problem should have been caught earlier, but our unique index is not actually unique because our version of PG misses `NULLS NOT DISTINCT`. The unique index only works for champs in repetitions. --- app/models/concerns/dossier_clone_concern.rb | 2 +- .../concerns/dossier_clone_concern_spec.rb | 25 +++++++++++-------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/dossier_clone_concern.rb b/app/models/concerns/dossier_clone_concern.rb index e147c9a20..470f30cf9 100644 --- a/app/models/concerns/dossier_clone_concern.rb +++ b/app/models/concerns/dossier_clone_concern.rb @@ -167,7 +167,7 @@ module DossierCloneConcern added_champs.each { _1.update_column(:dossier_id, id) } if updated_champs.present? - champs_index = filled_champs_public.index_by(&:public_id) + champs_index = champs.index_by(&:public_id) updated_champs.each do |champ| champs_index[champ.public_id]&.destroy! champ.update_column(:dossier_id, id) diff --git a/spec/models/concerns/dossier_clone_concern_spec.rb b/spec/models/concerns/dossier_clone_concern_spec.rb index 7854b7438..f7bab0f47 100644 --- a/spec/models/concerns/dossier_clone_concern_spec.rb +++ b/spec/models/concerns/dossier_clone_concern_spec.rb @@ -378,11 +378,15 @@ RSpec.describe DossierCloneConcern do } let(:removed_champ) { dossier.champs.find { _1.stable_id == 99 } } let(:updated_champ) { dossier.champs.find { _1.stable_id == 991 } } + let(:repetition_updated_champ) { champ_for_update(dossier.champs.find { _1.stable_id == 994 }) } + let(:forked_updated_champ) { champ_for_update(forked_dossier.champs.find { _1.stable_id == 991 }) } + let(:forked_repetition_updated_champ) { champ_for_update(forked_dossier.champs.find { _1.stable_id == 994 }) } before do dossier.champs.each do |champ| champ.update(value: 'old value') end + dossier.reload procedure.draft_revision.add_type_de_champ({ type_champ: TypeDeChamp.type_champs.fetch(:text), libelle: "Un nouveau champ text" @@ -395,18 +399,19 @@ RSpec.describe DossierCloneConcern do procedure.draft_revision.remove_type_de_champ(removed_champ.stable_id) procedure.draft_revision.find_and_ensure_exclusive_use(updated_champ.stable_id).update(libelle: "Un nouveau libelle") procedure.publish_revision! + added_champ.update(value: 'new value for added champ') + added_repetition_champ.update(value: "new value in repetition champ") + + forked_updated_champ.update(value: 'new value for updated champ') + forked_repetition_updated_champ.update(value: 'new value for updated champ in repetition') + updated_champ.update(type: 'Champs::TextareaChamp') + repetition_updated_champ.update(type: 'Champs::TextareaChamp') + dossier.reload + forked_dossier.reload end - subject { - added_champ.update(value: 'new value for added champ') - updated_champ.update(value: 'new value for updated champ') - added_repetition_champ.update(value: "new value in repetition champ") - dossier.reload - super() - } - - it { expect { subject }.to change { dossier.filled_champs.size }.by(1) } - it { expect { subject }.to change { dossier.filled_champs.sort_by(&:created_at).map(&:to_s) }.from(['old value', 'old value', 'Non', 'old value', 'old value']).to(['new value for updated champ', 'Non', 'old value', 'old value', 'new value for added champ', 'new value in repetition champ']) } + it { expect { subject }.to change { dossier.filled_champs.size }.by(3) } + it { expect { subject }.to change { dossier.filled_champs.sort_by(&:created_at).map(&:to_s) }.from(['old value', 'Non', 'old value']).to(['new value for updated champ', 'Non', 'new value for updated champ in repetition', 'old value', 'new value for added champ', 'new value in repetition champ']) } it "dossier after merge should be on last published revision" do expect(dossier.revision_id).to eq(procedure.revisions.first.id) From dc5bfb815959f0caeeb12e17c19af594420cb72f Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Mon, 16 Dec 2024 21:47:27 +0000 Subject: [PATCH 2/2] fix(dossier): remove champ duplicates --- .../t20241216remove_non_unique_champs_task.rb | 22 +++++++++++++++++++ ...41216remove_non_unique_champs_task_spec.rb | 22 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 app/tasks/maintenance/t20241216remove_non_unique_champs_task.rb create mode 100644 spec/tasks/maintenance/t20241216remove_non_unique_champs_task_spec.rb diff --git a/app/tasks/maintenance/t20241216remove_non_unique_champs_task.rb b/app/tasks/maintenance/t20241216remove_non_unique_champs_task.rb new file mode 100644 index 000000000..5ed7d6483 --- /dev/null +++ b/app/tasks/maintenance/t20241216remove_non_unique_champs_task.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Maintenance + class T20241216removeNonUniqueChampsTask < MaintenanceTasks::Task + # Documentation: cette tâche supprime les champs en double dans un dossier + + include RunnableOnDeployConcern + include StatementsHelpersConcern + + def collection + Dossier.state_not_brouillon.includes(champs: true) + end + + def process(dossier) + dossier.champs.filter { _1.row_id.nil? }.group_by(&:public_id).each do |_, champs| + if champs.size > 1 + champs.sort_by(&:id)[1..].each(&:destroy) + end + end + end + end +end diff --git a/spec/tasks/maintenance/t20241216remove_non_unique_champs_task_spec.rb b/spec/tasks/maintenance/t20241216remove_non_unique_champs_task_spec.rb new file mode 100644 index 000000000..61b490c27 --- /dev/null +++ b/spec/tasks/maintenance/t20241216remove_non_unique_champs_task_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "rails_helper" + +module Maintenance + RSpec.describe T20241216removeNonUniqueChampsTask do + describe "#process" do + subject(:process) { described_class.process(dossier) } + let(:procedure) { create(:procedure, types_de_champ_public: [{}]) } + let(:dossier) { create(:dossier, :with_populated_champs, procedure:) } + let(:type_de_champ) { dossier.revision.types_de_champ_public.first } + let(:champ_id) { dossier.champs.first.id } + + before { + dossier.champs.create(**type_de_champ.params_for_champ) + } + + it { expect { subject }.to change { dossier.reload.project_champ(type_de_champ).id }.from(dossier.champs.last.id).to(champ_id) } + it { expect { subject }.to change { Champ.count }.by(-1) } + end + end +end