From d4198869fb6e4c74374410e30fb9b572778745f5 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 28 Sep 2023 12:41:19 +0200 Subject: [PATCH] chore(exports): dropdown menu re-uses the same pending export or create a fresh one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pour permettre d'avoir des données fraiches dans un export tout en conservant l'historique des exports, la demande d'export depuis le menu créé toujours un nouvel export sauf: - si un autre export identique est déjà en préparation - si un autre export identique s'est terminé il y a moins de 5 minutes Co-Authored-By: Lisa Durand --- .../dossiers/export_dropdown_component.rb | 3 +- .../dossiers/export_link_component.rb | 11 ++--- .../export_link_component.html.haml | 4 +- .../administrateurs/exports_controller.rb | 6 +-- .../instructeurs/procedures_controller.rb | 6 +-- app/models/export.rb | 42 ++++++++----------- spec/models/export_spec.rb | 40 ++++++++++++++++-- 7 files changed, 62 insertions(+), 50 deletions(-) diff --git a/app/components/dossiers/export_dropdown_component.rb b/app/components/dossiers/export_dropdown_component.rb index 3978153e0..098dae369 100644 --- a/app/components/dossiers/export_dropdown_component.rb +++ b/app/components/dossiers/export_dropdown_component.rb @@ -21,11 +21,10 @@ class Dossiers::ExportDropdownComponent < ApplicationComponent item.fetch(:format) != :json || @procedure.active_revision.carte? end - def download_export_path(export_format:, force_export: false, no_progress_notification: nil) + def download_export_path(export_format:, no_progress_notification: nil) @export_url.call(@procedure, export_format: export_format, statut: @statut, - force_export: force_export, no_progress_notification: no_progress_notification) end end diff --git a/app/components/dossiers/export_link_component.rb b/app/components/dossiers/export_link_component.rb index e7ffea973..0fe2967aa 100644 --- a/app/components/dossiers/export_link_component.rb +++ b/app/components/dossiers/export_link_component.rb @@ -11,10 +11,9 @@ class Dossiers::ExportLinkComponent < ApplicationComponent @export_url = export_url end - def download_export_path(export_format:, statut:, force_export: false, no_progress_notification: nil) + def download_export_path(export_format:, statut:, no_progress_notification: nil) @export_url.call(@procedure, export_format: export_format, - force_export: force_export, statut: statut, no_progress_notification: no_progress_notification) end @@ -63,11 +62,9 @@ class Dossiers::ExportLinkComponent < ApplicationComponent def refresh_button_options(export) { - title: t(".refresh_old_export", export_format: ".#{export.format}"), - "aria-label" => t(".refresh_old_export", export_format: ".#{export.format}"), - class: class_names("fr-btn fr-btn--sm fr-icon-refresh-line fr-btn--tertiary" => true, - "fr-btn--icon" => !export.failed?, - "fr-btn--icon-left" => export.failed?) + title: t(".refresh_old_export"), + "aria-label" => t(".refresh_old_export"), + class: "fr-btn fr-btn--sm fr-icon-refresh-line fr-btn--tertiary fr-btn--icon-left" } end end diff --git a/app/components/dossiers/export_link_component/export_link_component.html.haml b/app/components/dossiers/export_link_component/export_link_component.html.haml index f7c7d0ee8..941ca0935 100644 --- a/app/components/dossiers/export_link_component/export_link_component.html.haml +++ b/app/components/dossiers/export_link_component/export_link_component.html.haml @@ -13,6 +13,6 @@ .flex.flex-gap-2 = export_button(export) - - if export.old? || export.failed? - = button_to refresh_button_options(export)[:title], download_export_path(export_format: export.format, statut: export.statut, force_export: true), refresh_button_options(export) + - if export.failed? + = button_to refresh_button_options(export)[:title], download_export_path(export_format: export.format, statut: export.statut), refresh_button_options(export) diff --git a/app/controllers/administrateurs/exports_controller.rb b/app/controllers/administrateurs/exports_controller.rb index b5674c306..28f84778b 100644 --- a/app/controllers/administrateurs/exports_controller.rb +++ b/app/controllers/administrateurs/exports_controller.rb @@ -4,7 +4,7 @@ module Administrateurs before_action :ensure_not_super_admin! def download - export = Export.find_or_create_export(export_format, all_groupe_instructeurs, force: force_export?, **export_options) + export = Export.find_or_create_fresh_export(export_format, all_groupe_instructeurs, **export_options) @dossiers_count = export.count if export.available? @@ -37,10 +37,6 @@ module Administrateurs @export_format ||= params[:export_format] end - def force_export? - @force_export ||= params[:force_export].present? - end - def export_options @export_options ||= { time_span_type: params[:time_span_type], diff --git a/app/controllers/instructeurs/procedures_controller.rb b/app/controllers/instructeurs/procedures_controller.rb index 569a7f5e6..4c2b4d121 100644 --- a/app/controllers/instructeurs/procedures_controller.rb +++ b/app/controllers/instructeurs/procedures_controller.rb @@ -172,7 +172,7 @@ module Instructeurs .visible_by_administration .exists?(groupe_instructeur_id: groupe_instructeur_ids) && !instructeur_as_manager? - export = Export.find_or_create_export(export_format, groupe_instructeurs, force: force_export?, **export_options) + export = Export.find_or_create_fresh_export(export_format, groupe_instructeurs, **export_options) @procedure = procedure @statut = export_options[:statut] @@ -309,10 +309,6 @@ module Instructeurs @export_format ||= params[:export_format] end - def force_export? - @force_export ||= params[:force_export].present? - end - def export_options @export_options ||= { time_span_type: params[:time_span_type], diff --git a/app/models/export.rb b/app/models/export.rb index a20ea8dc4..65c809a27 100644 --- a/app/models/export.rb +++ b/app/models/export.rb @@ -59,27 +59,28 @@ class Export < ApplicationRecord time_span_type == Export.time_span_types.fetch(:monthly) ? 30.days.ago : nil end - def old? - updated_at < 10.minutes.ago || filters_changed? - end - - def filters_changed? - procedure_presentation&.snapshot != procedure_presentation_snapshot - end - def filtered? procedure_presentation_id.present? end - def self.find_or_create_export(format, groupe_instructeurs, time_span_type: time_span_types.fetch(:everything), statut: statuts.fetch(:tous), procedure_presentation: nil, force: false) - export = create_or_find_export(format, groupe_instructeurs, time_span_type: time_span_type, statut: statut, procedure_presentation: procedure_presentation) + def self.find_or_create_fresh_export(format, groupe_instructeurs, time_span_type: time_span_types.fetch(:everything), statut: statuts.fetch(:tous), procedure_presentation: nil) + attributes = { + format:, + time_span_type:, + statut:, + key: generate_cache_key(groupe_instructeurs.map(&:id), procedure_presentation) + } - if export.available? && export.old? && force - export.destroy! - create_or_find_export(format, groupe_instructeurs, time_span_type: time_span_type, statut: statut, procedure_presentation: procedure_presentation) - else - export - end + recent_export = pending + .or(generated.where(updated_at: (5.minutes.ago)..)) + .includes(:procedure_presentation) + .find_by(attributes) + + return recent_export if recent_export.present? + + create!(**attributes, groupe_instructeurs:, + procedure_presentation:, + procedure_presentation_snapshot: procedure_presentation&.snapshot) end def self.for_groupe_instructeurs(groupe_instructeurs_ids) @@ -93,15 +94,6 @@ class Export < ApplicationRecord ]) end - def self.create_or_find_export(format, groupe_instructeurs, time_span_type:, statut:, procedure_presentation:) - create_with(groupe_instructeurs: groupe_instructeurs, procedure_presentation: procedure_presentation, procedure_presentation_snapshot: procedure_presentation&.snapshot) - .includes(:procedure_presentation) - .create_or_find_by(format: format, - time_span_type: time_span_type, - statut: statut, - key: generate_cache_key(groupe_instructeurs.map(&:id), procedure_presentation)) - end - def self.generate_cache_key(groupe_instructeurs_ids, procedure_presentation = nil) if procedure_presentation.present? [ diff --git a/spec/models/export_spec.rb b/spec/models/export_spec.rb index ce8ea103d..f53d38195 100644 --- a/spec/models/export_spec.rb +++ b/spec/models/export_spec.rb @@ -79,7 +79,7 @@ RSpec.describe Export, type: :model do end end - describe '.find_or_create_export' do + describe '.find_or_create_fresh_export' do let!(:procedure) { create(:procedure) } let!(:gi_1) { create(:groupe_instructeur, procedure: procedure, instructeurs: [create(:instructeur)]) } let!(:pp) { gi_1.instructeurs.first.procedure_presentation_and_errors_for_procedure_id(procedure.id).first } @@ -87,18 +87,50 @@ RSpec.describe Export, type: :model do context 'with procedure_presentation having different filters' do it 'works once' do - expect { Export.find_or_create_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } + expect { Export.find_or_create_fresh_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } .to change { Export.count }.by(1) end it 'works once, changes procedure_presentation, recreate a new' do - expect { Export.find_or_create_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } + expect { Export.find_or_create_fresh_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } .to change { Export.count }.by(1) pp.add_filter('tous', 'self/updated_at', '10/12/2021') - expect { Export.find_or_create_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } + expect { Export.find_or_create_fresh_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) } .to change { Export.count }.by(1) end end + + context 'with existing matching export' do + def find_or_create = + Export.find_or_create_fresh_export(:zip, [gi_1], time_span_type: Export.time_span_types.fetch(:everything), statut: Export.statuts.fetch(:tous), procedure_presentation: pp) + + context 'freshly generate export' do + before { find_or_create.update!(job_status: :generated, updated_at: 1.second.ago) } + + it 'returns current pending export' do + current_export = find_or_create + + expect(find_or_create).to eq(current_export) + end + end + + context 'old generated export' do + before { find_or_create.update!(job_status: :generated, updated_at: 1.hour.ago) } + + it 'returns a new export' do + expect { find_or_create }.to change { Export.count }.by(1) + end + end + + context 'pending export' do + before { find_or_create.update!(updated_at: 1.hour.ago) } + + it 'returns current pending export' do + current_export = find_or_create + expect(find_or_create).to eq(current_export) + end + end + end end describe '.dossiers_for_export' do