From 43f936ec7722785e60b3f16f4d3f39a7bf4c1940 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Tue, 18 Jun 2019 14:31:06 +0200 Subject: [PATCH 1/6] =?UTF-8?q?Do=20not=20check=20for=20path=20availabilit?= =?UTF-8?q?y=20when=20in=20the=20=E2=80=9Cnew=20procedure=E2=80=9D=20form?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user can’t even enter the path here, it’s nil. fixes #3918 The @availability was always PATH_NOT_AVAILABLE when the form was reloaded after an error, because Procedure::path_availability always found an (archived) procedure with a nil path. It got confused and concluded its path was conflicting. 🤷🏻‍♂️ --- app/views/admin/procedures/new.html.haml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/views/admin/procedures/new.html.haml b/app/views/admin/procedures/new.html.haml index d79f2ef01..ccfde2a2f 100644 --- a/app/views/admin/procedures/new.html.haml +++ b/app/views/admin/procedures/new.html.haml @@ -6,7 +6,4 @@ = form_for @procedure, url: { controller: 'admin/procedures', action: :create }, multipart: true do |f| = render partial: 'informations', locals: { f: f } .text-right - - if @availability.in?(Procedure::PATH_CAN_PUBLISH) - = f.button 'Valider', class: 'btn btn-info', id: 'save-procedure' - - else - = f.button 'Valider', class: 'btn btn-info', id: 'save-procedure', disabled: true + = f.button 'Valider', class: 'btn btn-info', id: 'save-procedure' From 2393fa86be16a1722f66a08987c87f34a3450009 Mon Sep 17 00:00:00 2001 From: Christian Lautier <15379878+maatinito@users.noreply.github.com> Date: Wed, 19 Jun 2019 16:24:22 -1000 Subject: [PATCH 2/6] Mailer preview correction: procedure has now multiple administrators --- spec/mailers/previews/administration_mailer_preview.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/mailers/previews/administration_mailer_preview.rb b/spec/mailers/previews/administration_mailer_preview.rb index 0aaa8a069..e0fd0b0df 100644 --- a/spec/mailers/previews/administration_mailer_preview.rb +++ b/spec/mailers/previews/administration_mailer_preview.rb @@ -29,11 +29,11 @@ class AdministrationMailerPreview < ActionMailer::Preview private def procedure_1 - Procedure.new(id: 10, libelle: "Démarche des marches", administrateur: administrateur) + Procedure.new(id: 10, libelle: "Démarche des marches", administrateurs: [administrateur]) end def procedure_2 - Procedure.new(id: 20, libelle: "Démarche pieds", administrateur: administrateur) + Procedure.new(id: 20, libelle: "Démarche pieds", administrateurs: [administrateur]) end def administrateur From 91393be13cbb0f6f47033449d77ae6c55f211068 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 18 Jun 2019 13:09:05 +0000 Subject: [PATCH 3/6] tasks: extract rollback to a separate method --- ...to_champ_piece_jointe_migration_service.rb | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb index dc5a02c29..a31713eea 100644 --- a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb +++ b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb @@ -54,15 +54,7 @@ class PieceJustificativeToChampPieceJointeMigrationService # If anything goes wrong, we roll back the migration by destroying the newly created # types de champ, champs blobs and attachments. rake_puts "Error received. Rolling back migration of procedure #{procedure.id}…" - - types_de_champ_pj.each do |type_champ| - # First destroy all the individual champs on dossiers - type_champ.champ.each { |c| destroy_champ_pj(c.dossier.reload, c) } - # Now we can destroy the type de champ itself, - # without cascading the timestamp update on all attached dossiers. - type_champ.reload.destroy - end - + rollback_migration!(types_de_champ_pj) rake_puts "Migration of procedure #{procedure.id} rolled back." # Reraise the exception to abort the migration. @@ -144,6 +136,16 @@ class PieceJustificativeToChampPieceJointeMigrationService raise end + def rollback_migration!(types_de_champ_pj) + types_de_champ_pj.each do |type_champ| + # First destroy all the individual champs on dossiers + type_champ.champ.each { |c| destroy_champ_pj(c.dossier.reload, c) } + # Now we can destroy the type de champ itself, + # without cascading the timestamp update on all attached dossiers. + type_champ.reload.destroy + end + end + def make_blob(pj) storage_service.make_blob(pj.content, pj.updated_at.iso8601, filename: pj.original_filename) end From f8dda3ae45a33b14b8685533d39ee76b36e4475e Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 18 Jun 2019 13:28:26 +0000 Subject: [PATCH 4/6] tasks: don't abort rollback when a dossier fails Instead print a warning, and continue rolling back the other dossiers. --- ...to_champ_piece_jointe_migration_service.rb | 9 +++++++- ...amp_piece_jointe_migration_service_spec.rb | 21 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb index a31713eea..b59a93c86 100644 --- a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb +++ b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb @@ -139,7 +139,14 @@ class PieceJustificativeToChampPieceJointeMigrationService def rollback_migration!(types_de_champ_pj) types_de_champ_pj.each do |type_champ| # First destroy all the individual champs on dossiers - type_champ.champ.each { |c| destroy_champ_pj(c.dossier.reload, c) } + type_champ.champ.each do |champ| + begin + destroy_champ_pj(champ.dossier.reload, champ) + rescue => e + rake_puts e + rake_puts "Rolling back of champ #{champ.id} failed. Continuing to roll back…" + end + end # Now we can destroy the type de champ itself, # without cascading the timestamp update on all attached dossiers. type_champ.reload.destroy diff --git a/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb b/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb index b1e96df09..d9641c47a 100644 --- a/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb +++ b/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb @@ -276,5 +276,26 @@ describe PieceJustificativeToChampPieceJointeMigrationService do expect { try_convert(procedure) }.not_to change { dossier.champs.count } end end + + context 'when rolling back a dossier fails' do + before do + allow(service).to receive(:destroy_champ_pj) + .with(having_attributes(id: dossier.id), anything) + .and_raise(StandardError) + allow(service).to receive(:destroy_champ_pj) + .with(any_args) + .and_call_original + end + + it 'continues to roll back the other dossiers' do + expect { try_convert(procedure) } + .not_to change { failing_dossier.champs.count } + end + + it 'does not creates types de champ on the procedure' do + expect { try_convert(procedure) } + .not_to change { procedure.types_de_champ.count } + end + end end end From d5619368633cc574ea3967e62ae9ff120762595c Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 18 Jun 2019 15:06:26 +0000 Subject: [PATCH 5/6] task: ensure that hidden dossiers are rolled back --- ...ve_to_champ_piece_jointe_migration_service.rb | 2 +- ..._champ_piece_jointe_migration_service_spec.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb index b59a93c86..48b228266 100644 --- a/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb +++ b/app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb @@ -141,7 +141,7 @@ class PieceJustificativeToChampPieceJointeMigrationService # First destroy all the individual champs on dossiers type_champ.champ.each do |champ| begin - destroy_champ_pj(champ.dossier.reload, champ) + destroy_champ_pj(Dossier.unscope(where: :hidden_at).find(champ.dossier_id), champ) rescue => e rake_puts e rake_puts "Rolling back of champ #{champ.id} failed. Continuing to roll back…" diff --git a/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb b/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb index d9641c47a..df63d3014 100644 --- a/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb +++ b/spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb @@ -265,6 +265,22 @@ describe PieceJustificativeToChampPieceJointeMigrationService do .not_to change { ActiveStorage::Attachment.count } end + context 'when some dossiers to roll back are hidden' do + before do + dossier.update_column(:hidden_at, Time.zone.now) + end + + it 'does not create champs' do + expect { try_convert(procedure) } + .not_to change { dossier.champs.count } + end + + it 'does not change the hidden dossier timestamps' do + try_convert(procedure) + expect(dossier.updated_at).to eq(initial_dossier_timestamps[:updated_at]) + end + end + context 'when receiving a Signal interruption (like Ctrl+C)' do let(:exception) { Interrupt } From 9ce92d5c3b5fe144010102496d3ceb38771b6b94 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Mon, 17 Jun 2019 16:13:12 +0200 Subject: [PATCH 6/6] champs: show attachment actions only when the atachment is persisted Fix #3961 --- app/views/shared/attachment/_update.html.haml | 6 +- spec/factories/champ.rb | 2 +- .../attachment/_update.html.haml_spec.rb | 65 +++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 spec/views/shared/attachment/_update.html.haml_spec.rb diff --git a/app/views/shared/attachment/_update.html.haml b/app/views/shared/attachment/_update.html.haml index ee86ddd7b..7631d3411 100644 --- a/app/views/shared/attachment/_update.html.haml +++ b/app/views/shared/attachment/_update.html.haml @@ -5,8 +5,10 @@ = link_to('le modèle suivant', url_for(template), target: '_blank', rel: 'noopener') - attachment_id = attachment ? attachment.id : SecureRandom.uuid + - persisted = attachment && attachment.persisted? - user_can_destroy = defined?(user_can_destroy) ? user_can_destroy : false - - if attachment + + - if persisted .piece-justificative-actions{ id: "piece_justificative_#{attachment_id}" } .piece-justificative-action = render partial: "shared/attachment/show", locals: { attachment: attachment, user_can_upload: true } @@ -18,5 +20,5 @@ = form.file_field :piece_justificative_file, id: "piece_justificative_file_#{attachment_id}", - class: "piece-justificative-input #{'hidden' if attachment}", + class: "piece-justificative-input #{'hidden' if persisted}", direct_upload: true diff --git a/spec/factories/champ.rb b/spec/factories/champ.rb index af14458ad..0af61cbcb 100644 --- a/spec/factories/champ.rb +++ b/spec/factories/champ.rb @@ -147,7 +147,7 @@ FactoryBot.define do factory :champ_piece_justificative, class: 'Champs::PieceJustificativeChamp' do type_de_champ { create(:type_de_champ_piece_justificative) } - after(:create) do |champ, _evaluator| + after(:build) do |champ, _evaluator| champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain") end end diff --git a/spec/views/shared/attachment/_update.html.haml_spec.rb b/spec/views/shared/attachment/_update.html.haml_spec.rb new file mode 100644 index 000000000..111b42acd --- /dev/null +++ b/spec/views/shared/attachment/_update.html.haml_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +describe 'shared/attachment/_update.html.haml', type: :view do + let(:champ) { build(:champ_piece_justificative, dossier: create(:dossier)) } + let(:attachment) { nil } + let(:virus_scan_result) { nil } + let(:user_can_destroy) { false } + + subject do + form_for(champ.dossier) do |form| + render 'shared/attachment/update', { + attachment: attachment, + user_can_destroy: user_can_destroy, + form: form + } + end + end + + it 'renders a form field for uploading a file' do + expect(subject).to have_selector('input[type=file]:not(.hidden)') + end + + context 'when there is a attached file' do + let(:attachment) { champ.piece_justificative_file.attachment } + + it 'renders a form field for uploading a file' do + expect(subject).to have_selector('input[type=file]:not(.hidden)') + end + + it 'does not renders a link to the unsaved file' do + expect(subject).not_to have_content(attachment.filename.to_s) + end + + it 'doesn’t render action buttons' do + expect(subject).not_to have_link('Remplacer') + expect(subject).not_to have_link('Supprimer') + end + + context 'and the attachment has been saved' do + before { champ.save! } + + it 'renders a link to the file' do + expect(subject).to have_content(attachment.filename.to_s) + end + + it 'renders action buttons' do + expect(subject).to have_button('Remplacer') + end + + it 'hides the form field by default' do + expect(subject).to have_selector('input[type=file].hidden') + end + + it 'hides the Delete button by default' do + is_expected.not_to have_link('Supprimer') + end + + context 'and the user can delete the attachment' do + let(:user_can_destroy) { true } + + it { is_expected.to have_link('Supprimer') } + end + end + end +end