From cf8c084a59f2836d66a81cb34e5de61cef35a5b6 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Fri, 8 Apr 2022 17:07:54 +0200 Subject: [PATCH] refactor: extract download_and_zip in to a shared service --- app/services/downloadable_file_service.rb | 27 ++++++++++++++ app/services/procedure_archive_service.rb | 28 +------------- spec/jobs/archive_creation_job_spec.rb | 4 +- .../downloadable_file_service_spec.rb | 37 +++++++++++++++++++ .../procedure_archive_service_spec.rb | 31 ---------------- 5 files changed, 67 insertions(+), 60 deletions(-) create mode 100644 app/services/downloadable_file_service.rb create mode 100644 spec/services/downloadable_file_service_spec.rb diff --git a/app/services/downloadable_file_service.rb b/app/services/downloadable_file_service.rb new file mode 100644 index 000000000..97909578b --- /dev/null +++ b/app/services/downloadable_file_service.rb @@ -0,0 +1,27 @@ +class DownloadableFileService + ARCHIVE_CREATION_DIR = ENV.fetch('ARCHIVE_CREATION_DIR') { '/tmp' } + + def self.download_and_zip(procedure, attachments, filename, &block) + Dir.mktmpdir(nil, ARCHIVE_CREATION_DIR) do |tmp_dir| + export_dir = File.join(tmp_dir, filename) + zip_path = File.join(ARCHIVE_CREATION_DIR, "#{filename}.zip") + + begin + FileUtils.remove_entry_secure(export_dir) if Dir.exist?(export_dir) + Dir.mkdir(export_dir) + + download_manager = DownloadManager::ProcedureAttachmentsExport.new(procedure, attachments, export_dir) + download_manager.download_all + + Dir.chdir(tmp_dir) do + File.delete(zip_path) if File.exist?(zip_path) + system 'zip', '-0', '-r', zip_path, filename + end + yield(zip_path) + ensure + FileUtils.remove_entry_secure(export_dir) if Dir.exist?(export_dir) + File.delete(zip_path) if File.exist?(zip_path) + end + end + end +end diff --git a/app/services/procedure_archive_service.rb b/app/services/procedure_archive_service.rb index 31f0786f6..6da543ac5 100644 --- a/app/services/procedure_archive_service.rb +++ b/app/services/procedure_archive_service.rb @@ -1,8 +1,6 @@ require 'tempfile' class ProcedureArchiveService - ARCHIVE_CREATION_DIR = ENV.fetch('ARCHIVE_CREATION_DIR') { '/tmp' } - def initialize(procedure) @procedure = procedure end @@ -27,9 +25,9 @@ class ProcedureArchiveService attachments = ActiveStorage::DownloadableFile.create_list_from_dossiers(dossiers) - download_and_zip(archive, attachments) do |zip_filepath| ArchiveUploader.new(procedure: @procedure, archive: archive, filepath: zip_filepath) .upload + DownloadableFileService.download_and_zip(@procedure, attachments, zip_root_folder(archive)) do |zip_filepath| end end @@ -45,30 +43,6 @@ class ProcedureArchiveService private - def download_and_zip(archive, attachments, &block) - Dir.mktmpdir(nil, ARCHIVE_CREATION_DIR) do |tmp_dir| - archive_dir = File.join(tmp_dir, zip_root_folder(archive)) - zip_path = File.join(ARCHIVE_CREATION_DIR, "#{zip_root_folder(archive)}.zip") - - begin - FileUtils.remove_entry_secure(archive_dir) if Dir.exist?(archive_dir) - Dir.mkdir(archive_dir) - - download_manager = DownloadManager::ProcedureAttachmentsExport.new(@procedure, attachments, archive_dir) - download_manager.download_all - - Dir.chdir(tmp_dir) do - File.delete(zip_path) if File.exist?(zip_path) - system 'zip', '-0', '-r', zip_path, zip_root_folder(archive) - end - yield(zip_path) - ensure - FileUtils.remove_entry_secure(archive_dir) if Dir.exist?(archive_dir) - File.delete(zip_path) if File.exist?(zip_path) - end - end - end - def zip_root_folder(archive) "procedure-#{@procedure.id}-#{archive.id}" end diff --git a/spec/jobs/archive_creation_job_spec.rb b/spec/jobs/archive_creation_job_spec.rb index 2f4f38425..985a0d1d8 100644 --- a/spec/jobs/archive_creation_job_spec.rb +++ b/spec/jobs/archive_creation_job_spec.rb @@ -11,7 +11,7 @@ describe ArchiveCreationJob, type: :job do before { expect(InstructeurMailer).not_to receive(:send_archive) } it 'does not send email and forward error for retry' do - allow_any_instance_of(ProcedureArchiveService).to receive(:download_and_zip).and_raise(StandardError, "kaboom") + allow(DownloadableFileService).to receive(:download_and_zip).and_raise(StandardError, "kaboom") expect { job.perform_now }.to raise_error(StandardError, "kaboom") expect(archive.reload.failed?).to eq(true) end @@ -20,7 +20,7 @@ describe ArchiveCreationJob, type: :job do context 'when it works' do let(:mailer) { double('mailer', deliver_later: true) } before do - allow_any_instance_of(ProcedureArchiveService).to receive(:download_and_zip).and_return(true) + allow(DownloadableFileService).to receive(:download_and_zip).and_return(true) expect(InstructeurMailer).to receive(:send_archive).and_return(mailer) end diff --git a/spec/services/downloadable_file_service_spec.rb b/spec/services/downloadable_file_service_spec.rb new file mode 100644 index 000000000..5e8c3c497 --- /dev/null +++ b/spec/services/downloadable_file_service_spec.rb @@ -0,0 +1,37 @@ +describe DownloadableFileService do + let(:procedure) { create(:procedure, :published) } + let(:service) { ProcedureArchiveService.new(procedure) } + + describe '#download_and_zip' do + let(:archive) { build(:archive, id: '3') } + let(:filename) { service.send(:zip_root_folder, archive) } + + it 'create a tmpdir while block is running' do + previous_dir_list = Dir.entries(DownloadableFileService::ARCHIVE_CREATION_DIR) + + DownloadableFileService.download_and_zip(procedure, [], filename) do |_zip_file| + new_dir_list = Dir.entries(DownloadableFileService::ARCHIVE_CREATION_DIR) + expect(previous_dir_list).not_to eq(new_dir_list) + end + end + + it 'cleans up its tmpdir after block execution' do + expect { DownloadableFileService.download_and_zip(procedure, [], filename) { |zip_file| } } + .not_to change { Dir.entries(DownloadableFileService::ARCHIVE_CREATION_DIR) } + end + + it 'creates a zip with zip utility' do + expected_zip_path = File.join(DownloadableFileService::ARCHIVE_CREATION_DIR, "#{service.send(:zip_root_folder, archive)}.zip") + expect(DownloadableFileService).to receive(:system).with('zip', '-0', '-r', expected_zip_path, an_instance_of(String)) + DownloadableFileService.download_and_zip(procedure, [], filename) { |zip_path| } + end + + it 'cleans up its generated zip' do + expected_zip_path = File.join(DownloadableFileService::ARCHIVE_CREATION_DIR, "#{service.send(:zip_root_folder, archive)}.zip") + DownloadableFileService.download_and_zip(procedure, [], filename) do |_zip_path| + expect(File.exist?(expected_zip_path)).to be_truthy + end + expect(File.exist?(expected_zip_path)).to be_falsey + end + end +end diff --git a/spec/services/procedure_archive_service_spec.rb b/spec/services/procedure_archive_service_spec.rb index 72c99a62a..73b7c7917 100644 --- a/spec/services/procedure_archive_service_spec.rb +++ b/spec/services/procedure_archive_service_spec.rb @@ -172,37 +172,6 @@ describe ProcedureArchiveService do end end - describe '#download_and_zip' do - let(:archive) { build(:archive, id: '3') } - it 'create a tmpdir while block is running' do - previous_dir_list = Dir.entries(ProcedureArchiveService::ARCHIVE_CREATION_DIR) - - service.send(:download_and_zip, archive, []) do |_zip_file| - new_dir_list = Dir.entries(ProcedureArchiveService::ARCHIVE_CREATION_DIR) - expect(previous_dir_list).not_to eq(new_dir_list) - end - end - - it 'cleans up its tmpdir after block execution' do - expect { service.send(:download_and_zip, archive, []) { |zip_file| } } - .not_to change { Dir.entries(ProcedureArchiveService::ARCHIVE_CREATION_DIR) } - end - - it 'creates a zip with zip utility' do - expected_zip_path = File.join(ProcedureArchiveService::ARCHIVE_CREATION_DIR, "#{service.send(:zip_root_folder, archive)}.zip") - expect(service).to receive(:system).with('zip', '-0', '-r', expected_zip_path, an_instance_of(String)) - service.send(:download_and_zip, archive, []) { |zip_path| } - end - - it 'cleans up its generated zip' do - expected_zip_path = File.join(ProcedureArchiveService::ARCHIVE_CREATION_DIR, "#{service.send(:zip_root_folder, archive)}.zip") - service.send(:download_and_zip, archive, []) do |_zip_path| - expect(File.exist?(expected_zip_path)).to be_truthy - end - expect(File.exist?(expected_zip_path)).to be_falsey - end - end - private def create_dossier_for_month(year, month)