Merge remote-tracking branch 'upstream/master' into 30634-protected-pipeline

* upstream/master: (67 commits)
  Revert "Merge branch 'revert-12499' into 'master'"
  Prevent accidental deletion of protected MR source branch by repeating checks before actual deletion
  Document that GitLab 9.3 requires the TRIGGER permission on MySQL
  Instrument Unicorn with Ruby exporter
  Remove group modal like remove project modal. Closes #33130
  Update prometheus client gem
  Enables the option in user preferences to turn on the new navigation
  Simplify authentication logic in the v4 users API for !12445.
  wait_for_requests is not needed when AJAX is not in play
  Don't resolve fork relationships for projects pending delete
  Clean up the ForkedProjectLink specs
  Remove unnecessary clear_stubs calls
  Add test for GitalyClient::Ref#find_ref_name
  DeleteMergedBranchesService should not delete protected branches
  Optimize creation of commit API by using Repository#commit instead of Repository#commits
  Update CHANGELOG.md for 9.3.4
  Make autosize fields more performant and remove broken autosize handle
  Update GITLAB_SHELL_VERSION to 5.1.1
  Fixed the y_label not setting correctly for each graph on the monitoring dashboard
  Refactor and copyedit "Using Docker images" docs
  ...

3 files changed, 92 insertions, 21 deletions

diff --git a/spec/services/delete_merged_branches_service_spec.rb b/spec/services/delete_merged_branches_service_spec.rb
index cae74df9c90..fe21ca0b3cb 100644
--- a/spec/services/delete_merged_branches_service_spec.rb
+++ b/spec/services/delete_merged_branches_service_spec.rb
@@ -24,6 +24,14 @@ describe DeleteMergedBranchesService, services: true do
       expect(project.repository.branch_names).to include('master')
     end
 
+    it 'keeps protected branches' do
+      create(:protected_branch, project: project, name: 'improve/awesome')
+
+      service.execute
+
+      expect(project.repository.branch_names).to include('improve/awesome')
+    end
+
     context 'user without rights' do
       let(:user) { create(:user) }
 
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 711059208c1..19d9e4049fe 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe MergeRequests::MergeService, services: true do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let(:merge_request) { create(:merge_request, assignee: user2) }
+  let(:merge_request) { create(:merge_request, :simple, author: user2, assignee: user2) }
   let(:project) { merge_request.project }
 
   before do
@@ -133,18 +133,65 @@ describe MergeRequests::MergeService, services: true do
       it { expect(todo).to be_done }
     end
 
-    context 'remove source branch by author' do
-      let(:service) do
-        merge_request.merge_params['force_remove_source_branch'] = '1'
-        merge_request.save!
-        MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message')
+    context 'source branch removal' do
+      context 'when the source branch is protected' do
+        let(:service) do
+          MergeRequests::MergeService.new(project, user, should_remove_source_branch: '1')
+        end
+
+        before do
+          create(:protected_branch, project: project, name: merge_request.source_branch)
+        end
+
+        it 'does not delete the source branch' do
+          expect(DeleteBranchService).not_to receive(:new)
+          service.execute(merge_request)
+        end
       end
 
-      it 'removes the source branch' do
-        expect(DeleteBranchService).to receive(:new)
-          .with(merge_request.source_project, merge_request.author)
-          .and_call_original
-        service.execute(merge_request)
+      context 'when the source branch is the default branch' do
+        let(:service) do
+          MergeRequests::MergeService.new(project, user, should_remove_source_branch: '1')
+        end
+
+        before do
+          allow(project).to receive(:root_ref?).with(merge_request.source_branch).and_return(true)
+        end
+
+        it 'does not delete the source branch' do
+          expect(DeleteBranchService).not_to receive(:new)
+          service.execute(merge_request)
+        end
+      end
+
+      context 'when the source branch can be removed' do
+        context 'when MR author set the source branch to be removed' do
+          let(:service) do
+            merge_request.merge_params['force_remove_source_branch'] = '1'
+            merge_request.save!
+            MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message')
+          end
+
+          it 'removes the source branch using the author user' do
+            expect(DeleteBranchService).to receive(:new)
+              .with(merge_request.source_project, merge_request.author)
+              .and_call_original
+            service.execute(merge_request)
+          end
+        end
+
+        context 'when MR merger set the source branch to be removed' do
+          let(:service) do
+            MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message', should_remove_source_branch: '1')
+          end
+
+          it 'removes the source branch using the current user' do
+            expect(DeleteBranchService).to receive(:new)
+              .with(merge_request.source_project, user)
+              .and_call_original
+            service.execute(merge_request)
+          end
+        end
       end
     end
 
diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb
index c9e63efbc14..35373675894 100644
--- a/spec/services/quick_actions/interpret_service_spec.rb
+++ b/spec/services/quick_actions/interpret_service_spec.rb
@@ -359,18 +359,18 @@ describe QuickActions::InterpretService, services: true do
       let(:content) { "/assign @#{developer.username}" }
 
       context 'Issue' do
-        it 'fetches assignee and populates assignee_id if content contains /assign' do
+        it 'fetches assignee and populates assignee_ids if content contains /assign' do
           _, updates = service.execute(content, issue)
 
-          expect(updates).to eq(assignee_ids: [developer.id])
+          expect(updates[:assignee_ids]).to match_array([developer.id])
         end
       end
 
       context 'Merge Request' do
-        it 'fetches assignee and populates assignee_id if content contains /assign' do
+        it 'fetches assignee and populates assignee_ids if content contains /assign' do
           _, updates = service.execute(content, merge_request)
 
-          expect(updates).to eq(assignee_id: developer.id)
+          expect(updates).to eq(assignee_ids: [developer.id])
         end
       end
     end
@@ -383,7 +383,7 @@ describe QuickActions::InterpretService, services: true do
       end
 
       context 'Issue' do
-        it 'fetches assignee and populates assignee_id if content contains /assign' do
+        it 'fetches assignee and populates assignee_ids if content contains /assign' do
           _, updates = service.execute(content, issue)
 
           expect(updates[:assignee_ids]).to match_array([developer.id])
@@ -391,10 +391,10 @@ describe QuickActions::InterpretService, services: true do
       end
 
       context 'Merge Request' do
-        it 'fetches assignee and populates assignee_id if content contains /assign' do
+        it 'fetches assignee and populates assignee_ids if content contains /assign' do
           _, updates = service.execute(content, merge_request)
 
-          expect(updates).to eq(assignee_id: developer.id)
+          expect(updates).to eq(assignee_ids: [developer.id])
         end
       end
     end
@@ -422,11 +422,27 @@ describe QuickActions::InterpretService, services: true do
       end
 
       context 'Merge Request' do
-        it 'populates assignee_id: nil if content contains /unassign' do
-          merge_request.update(assignee_id: developer.id)
+        it 'populates assignee_ids: [] if content contains /unassign' do
+          merge_request.update(assignee_ids: [developer.id])
           _, updates = service.execute(content, merge_request)
 
-          expect(updates).to eq(assignee_id: nil)
+          expect(updates).to eq(assignee_ids: [])
+        end
+      end
+    end
+
+    context 'reassign command' do
+      let(:content) { '/reassign' }
+
+      context 'Issue' do
+        it 'reassigns user if content contains /reassign @user' do
+          user = create(:user)
+
+          issue.update(assignee_ids: [developer.id])
+
+          _, updates = service.execute("/reassign @#{user.username}", issue)
+
+          expect(updates).to eq(assignee_ids: [user.id])
         end
       end
     end