Merge pull request #10040 from opf/fix/backup-specs

amended spec for backup's tmp file cleanup

app/workers/backup_job.rb

@@ -161,23 +161,27 @@ def create_backup_archive!(file_name:, db_dump_file_name:, attachments: attachme
       zipfile.get_output_stream("openproject.sql") { |f| f.write File.read(db_dump_file_name) }
     end
 
-    # delete locally cached files that were downloaded just for the backup
-    paths_to_clean.each do |path|
-      FileUtils.rm_rf path
-    end
+    remove_paths! paths_to_clean # delete locally cached files that were downloaded just for the backup
 
     @archived = true
 
     file_name
   end
 
+  def remove_paths!(paths)
+    paths.each do |path|
+      FileUtils.rm_rf path
+    end
+  end
+
   def get_cache_folder_path(attachment)
     # expecting paths like /tmp/op_uploaded_files/1639754082-3468-0002-0911/file.ext
     # just making extra sure so we don't delete anything wrong later on
     unless attachment.diskfile.path =~ /#{attachment.file.cache_dir}\/[^\/]+\/[^\/]+/
       raise "Unexpected cache path for attachment ##{attachment.id}: #{attachment.diskfile}"
     end
 
+    # returning parent as each cached file is in a separate folder which shall be removed too
     Pathname(attachment.diskfile.path).parent.to_s
   end
 

spec/workers/backup_job_spec.rb

@@ -57,7 +57,7 @@
 
     let(:db_dump_success) { false }
 
-    let(:arguments) { [{ backup: backup, user: user, **opts }] }
+    let(:arguments) { [{ backup: backup, user: user, **opts.except(:remote_storage) }] }
 
     let(:user) { FactoryBot.create :admin }
 
@@ -105,7 +105,11 @@ def backed_up_attachment(attachment)
         "attachment/file/#{attachment.id}/#{attachment.filename}"
       end
 
-      before { perform }
+      before do
+        allow(job).to receive(:remove_paths!)
+
+        perform
+      end
 
       it "destroys any previous backups" do
         expect(Backup.find_by(id: previous_backup.id)).to be_nil
@@ -127,6 +131,16 @@ def backed_up_attachment(attachment)
         it "does not include pending direct uploads" do
           expect(backup_files).not_to include backed_up_attachment(pending_direct_upload)
         end
+
+        if opts[:remote_storage] == true
+          it "cleans up locally cached files afterwards" do
+            expect(job).to have_received(:remove_paths!).with([Pathname(attachment.diskfile.path).parent.to_s])
+          end
+        else
+          it "does not clean up files afterwards as none were cached" do
+            expect(job).to have_received(:remove_paths!).with([])
+          end
+        end
       else
         it "does not include attachments in the backup" do
           expect(backup_files).not_to include backed_up_attachment(attachment)
@@ -140,6 +154,35 @@ def backed_up_attachment(attachment)
     it_behaves_like "it creates a backup"
   end
 
+  context(
+    "with remote storage",
+    with_config: {
+      attachments_storage: :fog,
+      fog: {
+        directory: MockCarrierwave.bucket,
+        credentials: MockCarrierwave.credentials
+      }
+    }
+  ) do
+    let(:dummy_path) { "/tmp/op_uploaded_files/1639754082-3468-0002-0911/file.ext" }
+
+    before do
+      FileUtils.mkdir_p Pathname(dummy_path).parent.to_s
+      File.open(dummy_path, "w") { |f| f.puts 'dummy' }
+
+      allow_any_instance_of(LocalFileUploader).to receive(:cached?).and_return(true)
+      allow_any_instance_of(LocalFileUploader)
+        .to receive(:local_file)
+        .and_return(File.new(dummy_path))
+    end
+
+    after do
+      FileUtils.rm dummy_path if File.exist? dummy_path
+    end
+
+    it_behaves_like "it creates a backup", remote_storage: true
+  end
+
   context "with include_attachments: false" do
     it_behaves_like "it creates a backup", include_attachments: false
   end