Add latest changes from gitlab-org/gitlab@13-4-stable-ee

12 files changed, 402 insertions, 52 deletions

diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb
new file mode 100644
index 00000000000..bb7b70f1ba2
--- /dev/null
+++ b/spec/services/merge_requests/base_service_spec.rb
@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::BaseService do
+  include ProjectForksHelper
+
+  let_it_be(:project) { create(:project, :repository) }
+  let(:title) { 'Awesome merge_request' }
+  let(:params) do
+    {
+      title: title,
+      description: 'please fix',
+      source_branch: 'feature',
+      target_branch: 'master'
+    }
+  end
+
+  subject { MergeRequests::CreateService.new(project, project.owner, params) }
+
+  describe '#execute_hooks' do
+    shared_examples 'enqueues Jira sync worker' do
+      it do
+        Sidekiq::Testing.fake! do
+          expect { subject.execute }.to change(JiraConnect::SyncMergeRequestWorker.jobs, :size).by(1)
+        end
+      end
+    end
+
+    shared_examples 'does not enqueue Jira sync worker' do
+      it do
+        Sidekiq::Testing.fake! do
+          expect { subject.execute }.not_to change(JiraConnect::SyncMergeRequestWorker.jobs, :size)
+        end
+      end
+    end
+
+    context 'with a Jira subscription' do
+      before do
+        create(:jira_connect_subscription, namespace: project.namespace)
+      end
+
+      context 'MR contains Jira issue key' do
+        let(:title) { 'Awesome merge_request with issue JIRA-123' }
+
+        it_behaves_like 'enqueues Jira sync worker'
+      end
+
+      context 'MR does not contain Jira issue key' do
+        it_behaves_like 'does not enqueue Jira sync worker'
+      end
+    end
+
+    context 'without a Jira subscription' do
+      it_behaves_like 'does not enqueue Jira sync worker'
+    end
+  end
+end
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index f99be26927d..f83b8d98ce8 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -88,6 +88,10 @@ RSpec.describe MergeRequests::BuildService do
       let(:source_project) { fork_project(project, user) }
       let(:merge_request) { described_class.new(project, user, mr_params).execute }
 
+      before do
+        project.add_reporter(user)
+      end
+
       it 'assigns force_remove_source_branch' do
         expect(merge_request.force_remove_source_branch?).to be_truthy
       end
@@ -510,7 +514,7 @@ RSpec.describe MergeRequests::BuildService do
       let(:target_project) { create(:project, :public, :repository) }
 
       before do
-        target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        target_project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
       end
 
       it 'sets the target_project correctly' do
diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb
new file mode 100644
index 00000000000..b38ccee4aa0
--- /dev/null
+++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb
@@ -0,0 +1,146 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::CleanupRefsService do
+  describe '.schedule' do
+    let(:merge_request) { build(:merge_request) }
+
+    it 'schedules MergeRequestCleanupRefsWorker' do
+      expect(MergeRequestCleanupRefsWorker)
+        .to receive(:perform_in)
+        .with(described_class::TIME_THRESHOLD, merge_request.id)
+
+      described_class.schedule(merge_request)
+    end
+  end
+
+  describe '#execute' do
+    before do
+      # Need to re-enable this as it's being stubbed in spec_helper for
+      # performance reasons but is needed to run for this test.
+      allow(Gitlab::Git::KeepAround).to receive(:execute).and_call_original
+    end
+
+    subject(:result) { described_class.new(merge_request).execute }
+
+    shared_examples_for 'service that cleans up merge request refs' do
+      it 'creates keep around ref and deletes merge request refs' do
+        old_ref_head = ref_head
+
+        aggregate_failures do
+          expect(result[:status]).to eq(:success)
+          expect(kept_around?(old_ref_head)).to be_truthy
+          expect(ref_head).to be_nil
+        end
+      end
+
+      context 'when merge request has merge ref' do
+        before do
+          MergeRequests::MergeToRefService
+            .new(merge_request.project, merge_request.author)
+            .execute(merge_request)
+        end
+
+        it 'caches merge ref sha and deletes merge ref' do
+          old_merge_ref_head = merge_request.merge_ref_head
+
+          aggregate_failures do
+            expect(result[:status]).to eq(:success)
+            expect(kept_around?(old_merge_ref_head)).to be_truthy
+            expect(merge_request.reload.merge_ref_sha).to eq(old_merge_ref_head.id)
+            expect(ref_exists?(merge_request.merge_ref_path)).to be_falsy
+          end
+        end
+
+        context 'when merge ref sha cannot be cached' do
+          before do
+            allow(merge_request)
+              .to receive(:update_column)
+              .with(:merge_ref_sha, merge_request.merge_ref_head.id)
+              .and_return(false)
+          end
+
+          it_behaves_like 'service that does not clean up merge request refs'
+        end
+      end
+
+      context 'when keep around ref cannot be created' do
+        before do
+          allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around|
+            expect(keep_around).to receive(:kept_around?).and_return(false)
+          end
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    shared_examples_for 'service that does not clean up merge request refs' do
+      it 'does not delete merge request refs' do
+        aggregate_failures do
+          expect(result[:status]).to eq(:error)
+          expect(ref_head).to be_present
+        end
+      end
+    end
+
+    context 'when merge request is closed' do
+      let(:merge_request) { create(:merge_request, :closed) }
+
+      context "when closed #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(latest_closed_at: described_class::TIME_THRESHOLD.ago)
+        end
+
+        it_behaves_like 'service that cleans up merge request refs'
+      end
+
+      context "when closed later than #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(latest_closed_at: (described_class::TIME_THRESHOLD - 1.day).ago)
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    context 'when merge request is merged' do
+      let(:merge_request) { create(:merge_request, :merged) }
+
+      context "when merged #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(merged_at: described_class::TIME_THRESHOLD.ago)
+        end
+
+        it_behaves_like 'service that cleans up merge request refs'
+      end
+
+      context "when merged later than #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(merged_at: (described_class::TIME_THRESHOLD - 1.day).ago)
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    context 'when merge request is not closed nor merged' do
+      let(:merge_request) { create(:merge_request, :opened) }
+
+      it_behaves_like 'service that does not clean up merge request refs'
+    end
+  end
+
+  def kept_around?(commit)
+    Gitlab::Git::KeepAround.new(merge_request.project.repository).kept_around?(commit.id)
+  end
+
+  def ref_head
+    merge_request.project.repository.commit(merge_request.ref_path)
+  end
+
+  def ref_exists?(ref)
+    merge_request.project.repository.ref_exists?(ref)
+  end
+end
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index e518e439a84..e7ac286f48b 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/spec/services/merge_requests/close_service_spec.rb
@@ -99,6 +99,12 @@ RSpec.describe MergeRequests::CloseService do
       described_class.new(project, user).execute(merge_request)
     end
 
+    it 'schedules CleanupRefsService' do
+      expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request)
+
+      described_class.new(project, user).execute(merge_request)
+    end
+
     context 'current user is not authorized to close merge request' do
       before do
         perform_enqueued_jobs do
diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb
index 14133731e37..5132eac0158 100644
--- a/spec/services/merge_requests/conflicts/list_service_spec.rb
+++ b/spec/services/merge_requests/conflicts/list_service_spec.rb
@@ -30,14 +30,13 @@ RSpec.describe MergeRequests::Conflicts::ListService do
     it 'returns a falsey value when one of the MR branches is missing' do
       merge_request = create_merge_request('conflict-resolvable')
       merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable')
-      merge_request.clear_memoized_source_branch_exists
 
       expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
     end
 
     it 'returns a falsey value when the MR does not support new diff notes' do
       merge_request = create_merge_request('conflict-resolvable')
-      merge_request.merge_request_diff.update(start_commit_sha: nil)
+      merge_request.merge_request_diff.update!(start_commit_sha: nil)
 
       expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
     end
diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb
index db46bd37eea..4dd70627977 100644
--- a/spec/services/merge_requests/create_pipeline_service_spec.rb
+++ b/spec/services/merge_requests/create_pipeline_service_spec.rb
@@ -5,13 +5,14 @@ require 'spec_helper'
 RSpec.describe MergeRequests::CreatePipelineService do
   include ProjectForksHelper
 
-  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:project, reload: true) { create(:project, :repository) }
   let_it_be(:user) { create(:user) }
   let(:service) { described_class.new(project, actor, params) }
   let(:actor) { user }
   let(:params) { {} }
 
   before do
+    stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
     project.add_developer(user)
   end
 
@@ -58,9 +59,27 @@ RSpec.describe MergeRequests::CreatePipelineService do
           expect(subject.project).to eq(project)
         end
 
-        context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do
+        context 'when source branch is protected' do
+          context 'when actor does not have permission to update the protected branch in target project' do
+            let!(:protected_branch) { create(:protected_branch, name: '*', project: project) }
+
+            it 'creates a pipeline in the source project' do
+              expect(subject.project).to eq(source_project)
+            end
+          end
+
+          context 'when actor has permission to update the protected branch in target project' do
+            let!(:protected_branch) { create(:protected_branch, :developers_can_merge, name: '*', project: project) }
+
+            it 'creates a pipeline in the target project' do
+              expect(subject.project).to eq(project)
+            end
+          end
+        end
+
+        context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do
           before do
-            stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false)
+            stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true)
           end
 
           it 'creates a pipeline in the source project' do
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index bb62e594e7a..d042b318d02 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -7,7 +7,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
-  let(:assignee) { create(:user) }
+  let(:user2) { create(:user) }
 
   describe '#execute' do
     context 'valid params' do
@@ -26,7 +26,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       before do
         project.add_maintainer(user)
-        project.add_developer(assignee)
+        project.add_developer(user2)
         allow(service).to receive(:execute_hooks)
       end
 
@@ -75,7 +75,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
               description: "well this is not done yet\n/wip",
               source_branch: 'feature',
               target_branch: 'master',
-              assignees: [assignee]
+              assignees: [user2]
             }
           end
 
@@ -91,7 +91,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
               description: "well this is not done yet\n/wip",
               source_branch: 'feature',
               target_branch: 'master',
-              assignees: [assignee]
+              assignees: [user2]
             }
           end
 
@@ -108,17 +108,17 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             description: 'please fix',
             source_branch: 'feature',
             target_branch: 'master',
-            assignees: [assignee]
+            assignees: [user2]
           }
         end
 
-        it { expect(merge_request.assignees).to eq([assignee]) }
+        it { expect(merge_request.assignees).to eq([user2]) }
 
         it 'creates a todo for new assignee' do
           attributes = {
             project: project,
             author: user,
-            user: assignee,
+            user: user2,
             target_id: merge_request.id,
             target_type: merge_request.class.name,
             action: Todo::ASSIGNED,
@@ -129,6 +129,34 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
+      context 'when reviewer is assigned' do
+        let(:opts) do
+          {
+            title: 'Awesome merge_request',
+            description: 'please fix',
+            source_branch: 'feature',
+            target_branch: 'master',
+            reviewers: [user2]
+          }
+        end
+
+        it { expect(merge_request.reviewers).to eq([user2]) }
+
+        it 'creates a todo for new reviewer' do
+          attributes = {
+            project: project,
+            author: user,
+            user: user2,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect(Todo.where(attributes).count).to eq 1
+        end
+      end
+
       context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do
         let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) }
         let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) }
@@ -212,7 +240,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             end
 
             before do
-              target_project.add_developer(assignee)
+              stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
+              target_project.add_developer(user2)
               target_project.add_maintainer(user)
             end
 
@@ -338,6 +367,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           end
         end
       end
+
+      it_behaves_like 'reviewer_ids filter' do
+        let(:execute) { service.execute }
+      end
     end
 
     it_behaves_like 'issuable record that supports quick actions' do
@@ -361,7 +394,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             assignee_ids: create(:user).id,
             milestone_id: 1,
             title: 'Title',
-            description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"),
+            description: %(/assign @#{user2.username}\n/milestone %"#{milestone.name}"),
             source_branch: 'feature',
             target_branch: 'master'
           }
@@ -369,12 +402,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
         before do
           project.add_maintainer(user)
-          project.add_maintainer(assignee)
+          project.add_maintainer(user2)
         end
 
         it 'assigns and sets milestone to issuable from command' do
           expect(merge_request).to be_persisted
-          expect(merge_request.assignees).to eq([assignee])
+          expect(merge_request.assignees).to eq([user2])
           expect(merge_request.milestone).to eq(milestone)
         end
       end
@@ -382,7 +415,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
     context 'merge request create service' do
       context 'asssignee_id' do
-        let(:assignee) { create(:user) }
+        let(:user2) { create(:user) }
 
         before do
           project.add_maintainer(user)
@@ -405,12 +438,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
 
         it 'saves assignee when user id is valid' do
-          project.add_maintainer(assignee)
-          opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] }
+          project.add_maintainer(user2)
+          opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] }
 
           merge_request = described_class.new(project, user, opts).execute
 
-          expect(merge_request.assignees).to eq([assignee])
+          expect(merge_request.assignees).to eq([user2])
         end
 
         context 'when assignee is set' do
@@ -418,24 +451,24 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             {
               title: 'Title',
               description: 'Description',
-              assignee_ids: [assignee.id],
+              assignee_ids: [user2.id],
               source_branch: 'feature',
               target_branch: 'master'
             }
           end
 
           it 'invalidates open merge request counter for assignees when merge request is assigned' do
-            project.add_maintainer(assignee)
+            project.add_maintainer(user2)
 
             described_class.new(project, user, opts).execute
 
-            expect(assignee.assigned_open_merge_requests_count).to eq 1
+            expect(user2.assigned_open_merge_requests_count).to eq 1
           end
         end
 
         context "when issuable feature is private" do
           before do
-            project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE,
+            project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE,
                                            merge_requests_access_level: ProjectFeature::PRIVATE)
           end
 
@@ -443,8 +476,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
           levels.each do |level|
             it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
-              project.update(visibility_level: level)
-              opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] }
+              project.update!(visibility_level: level)
+              opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] }
 
               merge_request = described_class.new(project, user, opts).execute
 
@@ -470,7 +503,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       before do
         project.add_maintainer(user)
-        project.add_developer(assignee)
+        project.add_developer(user2)
       end
 
       it 'creates a `MergeRequestsClosingIssues` record for each issue' do
@@ -498,7 +531,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       context 'when user can not access source project' do
         before do
-          target_project.add_developer(assignee)
+          target_project.add_developer(user2)
           target_project.add_maintainer(user)
         end
 
@@ -510,7 +543,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       context 'when user can not access target project' do
         before do
-          target_project.add_developer(assignee)
+          target_project.add_developer(user2)
           target_project.add_maintainer(user)
         end
 
@@ -562,7 +595,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
       end
 
       before do
-        project.add_developer(assignee)
+        project.add_developer(user2)
         project.add_maintainer(user)
       end
 
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
index 377615bbc6f..cdaacaf5fca 100644
--- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb
+++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb
@@ -19,7 +19,7 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s
 
       expect(diffs.count).to eq(4)
 
-      Timecop.freeze do
+      freeze_time do
         expect(DeleteDiffFilesWorker)
           .to receive(:bulk_perform_in)
           .with(5.minutes, [[diffs.first.id], [diffs.second.id]])
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 11e341994f7..8328f461029 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -152,6 +152,7 @@ RSpec.describe MergeRequests::MergeService do
         let(:commit)       { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") }
 
         before do
+          stub_jira_service_test
           project.update!(has_external_issue_tracker: true)
           jira_service_settings
           stub_jira_urls(jira_issue.id)
@@ -175,7 +176,7 @@ RSpec.describe MergeRequests::MergeService do
           end
 
           it 'does not close issue' do
-            jira_tracker.update(jira_issue_transition_id: nil)
+            jira_tracker.update!(jira_issue_transition_id: nil)
 
             expect_any_instance_of(JiraService).not_to receive(:transition_issue)
 
@@ -388,7 +389,7 @@ RSpec.describe MergeRequests::MergeService do
           error_message = 'Failed to squash. Should be done manually'
 
           allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil)
-          merge_request.update(squash: true)
+          merge_request.update!(squash: true)
 
           service.execute(merge_request)
 
@@ -402,7 +403,7 @@ RSpec.describe MergeRequests::MergeService do
           error_message = 'another squash is already in progress'
 
           allow_any_instance_of(MergeRequest).to receive(:squash_in_progress?).and_return(true)
-          merge_request.update(squash: true)
+          merge_request.update!(squash: true)
 
           service.execute(merge_request)
 
@@ -420,7 +421,7 @@ RSpec.describe MergeRequests::MergeService do
           %w(semi-linear ff).each do |merge_method|
             it "logs and saves error if merge is #{merge_method} only" do
               merge_method = 'rebase_merge' if merge_method == 'semi-linear'
-              merge_request.project.update(merge_method: merge_method)
+              merge_request.project.update!(merge_method: merge_method)
               error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch'
               allow(service).to receive(:execute_hooks)
 
@@ -434,6 +435,43 @@ RSpec.describe MergeRequests::MergeService do
           end
         end
       end
+
+      context 'when not mergeable' do
+        let!(:error_message) { 'Merge request is not mergeable' }
+
+        context 'with failing CI' do
+          before do
+            allow(merge_request).to receive(:mergeable_ci_state?) { false }
+          end
+
+          it 'logs and saves error' do
+            service.execute(merge_request)
+
+            expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message))
+          end
+        end
+
+        context 'with unresolved discussions' do
+          before do
+            allow(merge_request).to receive(:mergeable_discussions_state?) { false }
+          end
+
+          it 'logs and saves error' do
+            service.execute(merge_request)
+
+            expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message))
+          end
+
+          context 'when passing `skip_discussions_check: true` as `options` parameter' do
+            it 'merges the merge request' do
+              service.execute(merge_request, skip_discussions_check: true)
+
+              expect(merge_request).to be_valid
+              expect(merge_request).to be_merged
+            end
+          end
+        end
+      end
     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 a51a896ca96..402f753c0af 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -50,7 +50,7 @@ RSpec.describe MergeRequests::PostMergeService do
     end
 
     it 'marks MR as merged regardless of errors when closing issues' do
-      merge_request.update(target_branch: 'foo')
+      merge_request.update!(target_branch: 'foo')
       allow(project).to receive(:default_branch).and_return('foo')
 
       issue = create(:issue, project: project)
@@ -72,6 +72,12 @@ RSpec.describe MergeRequests::PostMergeService do
       subject
     end
 
+    it 'schedules CleanupRefsService' do
+      expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request)
+
+      subject
+    end
+
     context 'when the merge request has review apps' do
       it 'cancels all review app deployments' do
         pipeline = create(:ci_pipeline,
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 0696e8a247f..cace1e0bf09 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -225,6 +225,10 @@ RSpec.describe MergeRequests::RefreshService do
         context 'when service runs on forked project' do
           let(:project) { @fork_project }
 
+          before do
+            stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
+          end
+
           it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do
             expect { subject }
               .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1)
@@ -617,7 +621,7 @@ RSpec.describe MergeRequests::RefreshService do
         before do
           stub_feature_flags(track_resource_state_change_events: state_tracking_enabled)
 
-          @fork_project.destroy
+          @fork_project.destroy!
           service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
           reload_mrs
         end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index c3433c8c9d2..6b7463d4996 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -52,6 +52,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           title: 'New title',
           description: 'Also please fix',
           assignee_ids: [user.id],
+          reviewer_ids: [user.id],
           state_event: 'close',
           label_ids: [label.id],
           target_branch: 'target',
@@ -75,6 +76,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         expect(@merge_request).to be_valid
         expect(@merge_request.title).to eq('New title')
         expect(@merge_request.assignees).to match_array([user])
+        expect(@merge_request.reviewers).to match_array([user])
         expect(@merge_request).to be_closed
         expect(@merge_request.labels.count).to eq(1)
         expect(@merge_request.labels.first.title).to eq(label.name)
@@ -161,6 +163,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1")
         end
       end
+
+      it_behaves_like 'reviewer_ids filter' do
+        let(:opts) { {} }
+        let(:execute) { update_merge_request(opts) }
+      end
+
+      context 'with an existing reviewer' do
+        let(:merge_request) do
+          create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id])
+        end
+
+        context 'when merge_request_reviewer feature is enabled' do
+          before do
+            stub_feature_flags(merge_request_reviewer: true)
+          end
+
+          let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } }
+
+          it 'removes reviewers' do
+            expect(update_merge_request(opts).reviewers).to eq []
+          end
+        end
+      end
     end
 
     context 'after_save callback to store_mentions' do
@@ -379,11 +404,31 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         end
       end
 
-      context 'when the milestone is removed' do
+      context 'when reviewers gets changed' do
         before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
+          update_merge_request({ reviewer_ids: [user2.id] })
+        end
+
+        it 'marks pending todo as done' do
+          expect(pending_todo.reload).to be_done
+        end
+
+        it 'creates a pending todo for new review request' do
+          attributes = {
+            project: project,
+            author: user,
+            user: user2,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect(Todo.where(attributes).count).to eq 1
         end
+      end
 
+      context 'when the milestone is removed' do
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -393,12 +438,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           end
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           merge_request.milestone = create(:milestone, project: project)
 
-          merge_request.save
+          merge_request.save!
 
           perform_enqueued_jobs do
             update_merge_request(milestone_id: "")
@@ -410,10 +453,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when the milestone is changed' do
-        before do
-          stub_feature_flags(track_resource_milestone_change_events: false)
-        end
-
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
@@ -429,8 +468,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           expect(pending_todo.reload).to be_done
         end
 
-        it_behaves_like 'system notes for milestones'
-
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
           perform_enqueued_jobs do
             update_merge_request(milestone: create(:milestone, project: project))
@@ -628,7 +665,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
 
     context 'updating asssignee_ids' do
       it 'does not update assignee when assignee_id is invalid' do
-        merge_request.update(assignee_ids: [user.id])
+        merge_request.update!(assignee_ids: [user.id])
 
         update_merge_request(assignee_ids: [-1])
 
@@ -636,7 +673,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       it 'unassigns assignee when user id is 0' do
-        merge_request.update(assignee_ids: [user.id])
+        merge_request.update!(assignee_ids: [user.id])
 
         update_merge_request(assignee_ids: [0])
 
@@ -664,7 +701,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         levels.each do |level|
           it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
             assignee = create(:user)
-            project.update(visibility_level: level)
+            project.update!(visibility_level: level)
             feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level"
             project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)