Delete non-latest merge request diff files upon diffs reloadosw-delete-non-latest-mr-diff-files-upon-merge

4 files changed, 135 insertions, 39 deletions

diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb
new file mode 100644
index 00000000000..1c632847940
--- /dev/null
+++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb
@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_shared_state do
+  let(:merge_request) { create(:merge_request) }
+
+  let!(:subject) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    before do
+      stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+      3.times { merge_request.create_merge_request_diff }
+    end
+
+    it 'schedules non-latest merge request diffs removal' do
+      diffs = merge_request.merge_request_diffs
+
+      expect(diffs.count).to eq(4)
+
+      Timecop.freeze do
+        expect(DeleteDiffFilesWorker)
+          .to receive(:bulk_perform_in)
+          .with(5.minutes, [[diffs.first.id], [diffs.second.id]])
+        expect(DeleteDiffFilesWorker)
+          .to receive(:bulk_perform_in)
+          .with(10.minutes, [[diffs.third.id]])
+
+        subject.execute
+      end
+    end
+
+    it 'schedules no removal if it is already cleaned' do
+      merge_request.merge_request_diffs.each(&:clean!)
+
+      expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in)
+
+      subject.execute
+    end
+
+    it 'schedules no removal if it is empty' do
+      merge_request.merge_request_diffs.each { |diff| diff.update!(state: :empty) }
+
+      expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in)
+
+      subject.execute
+    end
+
+    it 'schedules no removal if there is no non-latest diffs' do
+      merge_request
+        .merge_request_diffs
+        .where.not(id: merge_request.latest_merge_request_diff_id)
+        .destroy_all
+
+      expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in)
+
+      subject.execute
+    end
+  end
+end
diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
deleted file mode 100644
index 57b6165cfb0..00000000000
--- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
+++ /dev/null
@@ -1,39 +0,0 @@
-require 'spec_helper'
-
-describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_store_caching do
-  let(:subject) { described_class.new }
-  let(:merge_request) { create(:merge_request) }
-
-  describe '#execute' do
-    before do
-      allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
-      allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
-    end
-
-    it 'retrieves the diff files to cache the highlighted result' do
-      new_diff = merge_request.merge_request_diff
-      cache_key = new_diff.diffs.cache_key
-
-      expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
-      expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
-
-      subject.execute(merge_request, new_diff)
-    end
-
-    it 'clears the cache for older diffs on the merge request' do
-      old_diff = merge_request.merge_request_diff
-      old_cache_key = old_diff.diffs.cache_key
-
-      subject.execute(merge_request, old_diff)
-
-      new_diff = merge_request.create_merge_request_diff
-      new_cache_key = new_diff.diffs.cache_key
-
-      expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
-      expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
-      expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
-
-      subject.execute(merge_request, new_diff)
-    end
-  end
-end
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index 70957431942..790ecce8ded 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -35,5 +35,17 @@ describe MergeRequests::PostMergeService do
 
       described_class.new(project, user, {}).execute(merge_request)
     end
+
+    it 'deletes non-latest diffs' do
+      diff_removal_service = instance_double(MergeRequests::DeleteNonLatestDiffsService, execute: nil)
+
+      expect(MergeRequests::DeleteNonLatestDiffsService)
+        .to receive(:new).with(merge_request)
+        .and_return(diff_removal_service)
+
+      described_class.new(project, user, {}).execute(merge_request)
+
+      expect(diff_removal_service).to have_received(:execute)
+    end
   end
 end
diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb
new file mode 100644
index 00000000000..a0a27d247fc
--- /dev/null
+++ b/spec/services/merge_requests/reload_diffs_service_spec.rb
@@ -0,0 +1,64 @@
+require 'spec_helper'
+
+describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_caching do
+  let(:current_user) { create(:user) }
+  let(:merge_request) { create(:merge_request) }
+  let(:subject) { described_class.new(merge_request, current_user) }
+
+  describe '#execute' do
+    it 'creates new merge request diff' do
+      expect { subject.execute }.to change { merge_request.merge_request_diffs.count }.by(1)
+    end
+
+    it 'calls update_diff_discussion_positions with correct params' do
+      old_diff_refs = merge_request.diff_refs
+      new_diff = merge_request.create_merge_request_diff
+      new_diff_refs = merge_request.diff_refs
+
+      expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff)
+      expect(merge_request).to receive(:update_diff_discussion_positions)
+        .with(old_diff_refs: old_diff_refs,
+              new_diff_refs: new_diff_refs,
+              current_user: current_user)
+
+      subject.execute
+    end
+
+    it 'does not change existing merge request diff' do
+      expect(merge_request.merge_request_diff).not_to receive(:save_git_content)
+
+      subject.execute
+    end
+
+    context 'cache clearing' do
+      before do
+        allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
+        allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
+      end
+
+      it 'retrieves the diff files to cache the highlighted result' do
+        new_diff = merge_request.create_merge_request_diff
+        cache_key = new_diff.diffs_collection.cache_key
+
+        expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff)
+        expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
+        expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
+
+        subject.execute
+      end
+
+      it 'clears the cache for older diffs on the merge request' do
+        old_diff = merge_request.merge_request_diff
+        old_cache_key = old_diff.diffs_collection.cache_key
+        new_diff = merge_request.create_merge_request_diff
+        new_cache_key = new_diff.diffs_collection.cache_key
+
+        expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff)
+        expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
+        expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
+        expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
+        subject.execute
+      end
+    end
+  end
+end