Merge branch '2052-fix-merge-slash-approvals' into 'master'

Check if a merge request is approved when merging from API or slash command

Closes #2052

See merge request !1962

app/models/merge_request.rb

@@ -490,6 +490,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def mergeable?(skip_ci_check: false)
+    return false unless approved?
     return false unless mergeable_state?(skip_ci_check: skip_ci_check)
 
     check_if_can_be_merged

changelogs/unreleased-ee/2052-fix-merge-slash-approvals.yml

+---
+title: Check if a merge request is approved when merging from API or slash command
+merge_request:
+author:

spec/models/merge_request_spec.rb

@@ -1187,6 +1187,26 @@ describe MergeRequest, models: true do
 
       expect(subject.mergeable?).to be_truthy
     end
+
+    context 'when using approvals' do
+      let(:user) { create(:user) }
+      before do
+        allow(subject).to receive(:mergeable_state?).and_return(true)
+
+        subject.target_project.update_attributes(approvals_before_merge: 1)
+        project.team << [user, :developer]
+      end
+
+      it 'return false if not approved' do
+        expect(subject.mergeable?).to be_falsey
+      end
+
+      it 'return true if approved' do
+        subject.approvals.create(user: user)
+
+        expect(subject.mergeable?).to be_truthy
+      end
+    end
   end
 
   describe '#mergeable_state?' do
@@ -1956,6 +1976,22 @@ describe MergeRequest, models: true do
           expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_truthy
         end
       end
+
+      context 'with approvals' do
+        before do
+          merge_request.target_project.update_attributes(approvals_before_merge: 1)
+        end
+
+        it 'is not mergeable when not approved' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_falsey
+        end
+
+        it 'is mergeable when approved' do
+          merge_request.approvals.create(user: user)
+
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_truthy
+        end
+      end
     end
   end
 

spec/requests/api/merge_requests_spec.rb

@@ -635,6 +635,27 @@ describe API::MergeRequests do
       expect(json_response['message']).to eq('405 Method Not Allowed')
     end
 
+    it 'returns 405 if merge request was not approved' do
+      project.team << [create(:user), :developer]
+      project.update_attributes(approvals_before_merge: 1)
+
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
+
+      expect(response).to have_http_status(406)
+      expect(json_response['message']).to eq('Branch cannot be merged')
+    end
+
+    it 'returns 200 if merge request was approved' do
+      approver = create(:user)
+      project.team << [approver, :developer]
+      project.update_attributes(approvals_before_merge: 1)
+      merge_request.approvals.create(user: approver)
+
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
+
+      expect(response).to have_http_status(200)
+    end
+
     it "returns 401 if user has no permissions to merge" do
       user2 = create(:user)
       project.team << [user2, :reporter]

spec/services/merge_requests/update_service_spec.rb

@@ -230,6 +230,35 @@ describe MergeRequests::UpdateService, services: true do
 
         it { expect(@merge_request.state).to eq('opened') }
       end
+
+      context 'when not approved' do
+        before do
+          merge_request.update_attributes(approvals_before_merge: 1)
+
+          perform_enqueued_jobs do
+            service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('opened') }
+      end
+
+      context 'when approved' do
+        before do
+          merge_request.update_attributes(approvals_before_merge: 1)
+          merge_request.approvals.create(user: user)
+
+          perform_enqueued_jobs do
+            service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('merged') }
+      end
     end
 
     context 'todos' do

spec/services/slash_commands/interpret_service_spec.rb

@@ -355,6 +355,29 @@ describe SlashCommands::InterpretService, services: true do
           let(:issuable) { build(:merge_request, source_project: project) }
         end
       end
+
+      context 'not approved merge request can not be merged' do
+        before do
+          merge_request.target_project.update_attributes(approvals_before_merge: 1)
+        end
+
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { build(:merge_request, source_project: project) }
+        end
+      end
+
+      context 'approved merge request can be merged' do
+        before do
+          merge_request.update_attributes(approvals_before_merge: 1)
+          merge_request.approvals.create(user: developer)
+        end
+
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { build(:merge_request, source_project: project) }
+        end
+      end
     end
 
     it_behaves_like 'title command' do