Revert "Automatically update MR merge-ref along merge status"

5 files changed, 141 insertions, 251 deletions

diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index fc28c216b21..c6251326c22 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1996,6 +1996,57 @@ describe MergeRequest do
     end
   end
 
+  describe '#check_if_can_be_merged' do
+    let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
+
+    shared_examples 'checking if can be merged' do
+      context 'when it is not broken and has no conflicts' do
+        before do
+          allow(subject).to receive(:broken?) { false }
+          allow(project.repository).to receive(:can_be_merged?).and_return(true)
+        end
+
+        it 'is marked as mergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
+        end
+      end
+
+      context 'when broken' do
+        before do
+          allow(subject).to receive(:broken?) { true }
+          allow(project.repository).to receive(:can_be_merged?).and_return(false)
+        end
+
+        it 'becomes unmergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+        end
+      end
+
+      context 'when it has conflicts' do
+        before do
+          allow(subject).to receive(:broken?) { false }
+          allow(project.repository).to receive(:can_be_merged?).and_return(false)
+        end
+
+        it 'becomes unmergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+        end
+      end
+    end
+
+    context 'when merge_status is unchecked' do
+      subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
+
+      it_behaves_like 'checking if can be merged'
+    end
+
+    context 'when merge_status is unchecked' do
+      subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) }
+
+      it_behaves_like 'checking if can be merged'
+    end
+  end
+
   describe '#mergeable?' do
     let(:project) { create(:project) }
 
@@ -2009,7 +2060,7 @@ describe MergeRequest do
 
     it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
       allow(subject).to receive(:mergeable_state?) { true }
-      expect(subject).to receive(:check_mergeability)
+      expect(subject).to receive(:check_if_can_be_merged)
       expect(subject).to receive(:can_be_merged?) { true }
 
       expect(subject.mergeable?).to be_truthy
@@ -2023,7 +2074,7 @@ describe MergeRequest do
 
     it 'checks if merge request can be merged' do
       allow(subject).to receive(:mergeable_ci_state?) { true }
-      expect(subject).to receive(:check_mergeability)
+      expect(subject).to receive(:check_if_can_be_merged)
 
       subject.mergeable?
     end
@@ -3091,6 +3142,38 @@ describe MergeRequest do
     end
   end
 
+  describe '#mergeable_to_ref?' do
+    it 'returns true when merge request is mergeable' do
+      subject = create(:merge_request)
+
+      expect(subject.mergeable_to_ref?).to be(true)
+    end
+
+    it 'returns false when merge request is already merged' do
+      subject = create(:merge_request, :merged)
+
+      expect(subject.mergeable_to_ref?).to be(false)
+    end
+
+    it 'returns false when merge request is closed' do
+      subject = create(:merge_request, :closed)
+
+      expect(subject.mergeable_to_ref?).to be(false)
+    end
+
+    it 'returns false when merge request is work in progress' do
+      subject = create(:merge_request, title: 'WIP: The feature')
+
+      expect(subject.mergeable_to_ref?).to be(false)
+    end
+
+    it 'returns false when merge request has no commits' do
+      subject = create(:merge_request, source_branch: 'empty-branch', target_branch: 'master')
+
+      expect(subject.mergeable_to_ref?).to be(false)
+    end
+  end
+
   describe '#merge_participants' do
     it 'contains author' do
       expect(subject.merge_participants).to eq([subject.author])
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 4cb4fcc890d..9f9180bc8c9 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -1546,65 +1546,52 @@ describe API::MergeRequests do
     end
   end
 
-  describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref" do
-    before do
-      merge_request.mark_as_unchecked!
-    end
-
-    let(:merge_request_iid) { merge_request.iid }
-
+  describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do
+    let(:pipeline) { create(:ci_pipeline_without_jobs) }
     let(:url) do
-      "/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref"
+      "/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref"
     end
 
     it 'returns the generated ID from the merge service in case of success' do
-      get api(url, user)
+      put api(url, user), params: { merge_commit_message: 'Custom message' }
+
+      commit = project.commit(json_response['commit_id'])
 
       expect(response).to have_gitlab_http_status(200)
-      expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
+      expect(json_response['commit_id']).to be_present
+      expect(commit.message).to eq('Custom message')
     end
 
     it "returns 400 if branch can't be merged" do
-      merge_request.update!(merge_status: 'cannot_be_merged')
+      merge_request.update!(state: 'merged')
 
-      get api(url, user)
+      put api(url, user)
 
       expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']).to eq('Merge request is not mergeable')
+      expect(json_response['message'])
+        .to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
     end
 
-    context 'when user has no access to the MR' do
-      let(:project) { create(:project, :private) }
-      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-
-      it 'returns 404' do
-        project.add_guest(user)
+    it 'returns 403 if user has no permissions to merge to the ref' do
+      user2 = create(:user)
+      project.add_reporter(user2)
 
-        get api(url, user)
+      put api(url, user2)
 
-        expect(response).to have_gitlab_http_status(404)
-        expect(json_response['message']).to eq('404 Not found')
-      end
+      expect(response).to have_gitlab_http_status(403)
+      expect(json_response['message']).to eq('403 Forbidden')
     end
 
-    context 'when invalid merge request IID' do
-      let(:merge_request_iid) { '12345' }
-
-      it 'returns 404' do
-        get api(url, user)
+    it 'returns 404 for an invalid merge request IID' do
+      put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user)
 
-        expect(response).to have_gitlab_http_status(404)
-      end
+      expect(response).to have_gitlab_http_status(404)
     end
 
-    context 'when merge request ID is used instead IID' do
-      let(:merge_request_iid) { merge_request.id }
-
-      it 'returns 404' do
-        get api(url, user)
+    it "returns 404 if the merge request id is used instead of iid" do
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
 
-        expect(response).to have_gitlab_http_status(404)
-      end
+      expect(response).to have_gitlab_http_status(404)
     end
   end
 
diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb
index 5d492e4b013..0ac23050caf 100644
--- a/spec/services/merge_requests/merge_to_ref_service_spec.rb
+++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -32,8 +32,10 @@ describe MergeRequests::MergeToRefService do
 
       expect(result[:status]).to eq(:success)
       expect(result[:commit_id]).to be_present
+      expect(result[:source_id]).to eq(merge_request.source_branch_sha)
+      expect(result[:target_id]).to eq(merge_request.target_branch_sha)
       expect(repository.ref_exists?(target_ref)).to be(true)
-      expect(ref_head.sha).to eq(result[:commit_id])
+      expect(ref_head.id).to eq(result[:commit_id])
     end
   end
 
@@ -70,6 +72,10 @@ describe MergeRequests::MergeToRefService do
   let(:merge_request) { create(:merge_request, :simple) }
   let(:project) { merge_request.project }
 
+  before do
+    project.add_maintainer(user)
+  end
+
   describe '#execute' do
     let(:service) do
       described_class.new(project, user, commit_message: 'Awesome message',
@@ -86,12 +92,6 @@ describe MergeRequests::MergeToRefService do
     it_behaves_like 'successfully evaluates pre-condition checks'
 
     context 'commit history comparison with regular MergeService' do
-      before do
-        # The merge service needs an authorized user while merge-to-ref
-        # doesn't.
-        project.add_maintainer(user)
-      end
-
       let(:merge_ref_service) do
         described_class.new(project, user, {})
       end
@@ -136,9 +136,9 @@ describe MergeRequests::MergeToRefService do
         let(:merge_method) { :merge }
 
         it 'returns error' do
-          allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil }
+          allow(merge_request).to receive(:mergeable_to_ref?) { false }
 
-          error_message = 'Conflicts detected during merge'
+          error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
 
           result = service.execute(merge_request)
 
@@ -170,5 +170,28 @@ describe MergeRequests::MergeToRefService do
 
       it { expect(todo).not_to be_done }
     end
+
+    context 'when merge request is WIP state' do
+      it 'fails to merge' do
+        merge_request = create(:merge_request, title: 'WIP: The feature')
+
+        result = service.execute(merge_request)
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
+      end
+    end
+
+    it 'returns error when user has no authorization to admin the merge request' do
+      unauthorized_user = create(:user)
+      project.add_reporter(unauthorized_user)
+
+      service = described_class.new(project, unauthorized_user)
+
+      result = service.execute(merge_request)
+
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('You are not allowed to merge to this ref')
+    end
   end
 end
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
deleted file mode 100644
index aa0485467ed..00000000000
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ /dev/null
@@ -1,187 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe MergeRequests::MergeabilityCheckService do
-  shared_examples_for 'unmergeable merge request' do
-    it 'updates or keeps merge status as cannot_be_merged' do
-      subject
-
-      expect(merge_request.merge_status).to eq('cannot_be_merged')
-    end
-
-    it 'does not change the merge ref HEAD' do
-      expect { subject }.not_to change(merge_request, :merge_ref_head)
-    end
-
-    it 'returns ServiceResponse.error' do
-      result = subject
-
-      expect(result).to be_a(ServiceResponse)
-      expect(result).to be_error
-    end
-  end
-
-  shared_examples_for 'mergeable merge request' do
-    it 'updates or keeps merge status as can_be_merged' do
-      subject
-
-      expect(merge_request.merge_status).to eq('can_be_merged')
-    end
-
-    it 'updates the merge ref' do
-      expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
-    end
-
-    it 'returns ServiceResponse.success' do
-      result = subject
-
-      expect(result).to be_a(ServiceResponse)
-      expect(result).to be_success
-    end
-
-    it 'ServiceResponse has merge_ref_head payload' do
-      result = subject
-
-      expect(result.payload.keys).to contain_exactly(:merge_ref_head)
-      expect(result.payload[:merge_ref_head].keys)
-        .to contain_exactly(:commit_id, :target_id, :source_id)
-    end
-  end
-
-  describe '#execute' do
-    let(:project) { create(:project, :repository) }
-    let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
-    let(:repo) { project.repository }
-
-    subject { described_class.new(merge_request).execute }
-
-    before do
-      project.add_developer(merge_request.author)
-    end
-
-    it_behaves_like 'mergeable merge request'
-
-    context 'when multiple calls to the service' do
-      it 'returns success' do
-        subject
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.success?).to be(true)
-      end
-
-      it 'second call does not change the merge-ref' do
-        expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
-        expect { subject }.not_to change(merge_request, :merge_ref_head)
-      end
-    end
-
-    context 'when broken' do
-      before do
-        allow(merge_request).to receive(:broken?) { true }
-        allow(project.repository).to receive(:can_be_merged?) { false }
-      end
-
-      it_behaves_like 'unmergeable merge request'
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge request is not mergeable')
-      end
-    end
-
-    context 'when it has conflicts' do
-      before do
-        allow(merge_request).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?) { false }
-      end
-
-      it_behaves_like 'unmergeable merge request'
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge request is not mergeable')
-      end
-    end
-
-    context 'when MR cannot be merged and has no merge ref' do
-      before do
-        merge_request.mark_as_unmergeable!
-      end
-
-      it_behaves_like 'unmergeable merge request'
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge request is not mergeable')
-      end
-    end
-
-    context 'when MR cannot be merged and has outdated merge ref' do
-      before do
-        MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
-        merge_request.mark_as_unmergeable!
-      end
-
-      it_behaves_like 'unmergeable merge request'
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge request is not mergeable')
-      end
-    end
-
-    context 'when merge request is not given' do
-      subject { described_class.new(nil).execute }
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.message).to eq('Invalid argument')
-      end
-    end
-
-    context 'when read only DB' do
-      it 'returns ServiceResponse.error' do
-        allow(Gitlab::Database).to receive(:read_only?) { true }
-
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.message).to eq('Unsupported operation')
-      end
-    end
-
-    context 'when MR is mergeable but merge-ref does not exists' do
-      before do
-        merge_request.mark_as_mergeable!
-      end
-
-      it 'keeps merge status as can_be_merged' do
-        expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
-      end
-
-      it 'returns ServiceResponse.error' do
-        result = subject
-
-        expect(result).to be_a(ServiceResponse)
-        expect(result.error?).to be(true)
-        expect(result.message).to eq('Merge ref was not found')
-      end
-    end
-  end
-end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index e790d272e61..30bd4d6820b 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -16,13 +16,6 @@ describe ServiceResponse do
       expect(response).to be_success
       expect(response.message).to eq('Good orange')
     end
-
-    it 'creates a successful response with payload' do
-      response = described_class.success(payload: { good: 'orange' })
-
-      expect(response).to be_success
-      expect(response.payload).to eq(good: 'orange')
-    end
   end
 
   describe '.error' do
@@ -40,15 +33,6 @@ describe ServiceResponse do
       expect(response.message).to eq('Bad apple')
       expect(response.http_status).to eq(400)
     end
-
-    it 'creates a failed response with payload' do
-      response = described_class.error(message: 'Bad apple',
-                                       payload: { bad: 'apple' })
-
-      expect(response).to be_error
-      expect(response.message).to eq('Bad apple')
-      expect(response.payload).to eq(bad: 'apple')
-    end
   end
 
   describe '#success?' do