summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOswaldo Ferreira <oswaldo@gitlab.com>2019-05-21 18:14:22 -0300
committerOswaldo Ferreira <oswaldo@gitlab.com>2019-06-12 16:10:35 -0300
commitf3fd706f3558a558c348718c1ee39f765b2bf9e6 (patch)
tree1d9fafaf09b61f174c836d0aa536bdd4a5ddb905
parent50cddd36e505d49ebdb2b20c5c37ede56abad1df (diff)
downloadgitlab-ce-osw-sync-merge-ref-upon-mergeability-check.tar.gz
Automatically update MR merge-ref along merge statusosw-sync-merge-ref-upon-mergeability-check
This couples the code that transitions the `MergeRequest#merge_status` and refs/merge-requests/:iid/merge ref update. In general, instead of directly telling `MergeToRefService` to update the merge ref, we should rely on `MergeabilityCheckService` to keep both the merge status and merge ref synced. Now, if the merge_status is `can_be_merged` it means the merge-ref is also updated to the latest. We've also updated the logic to be more systematic and less user-based.
-rw-r--r--app/controllers/projects/merge_requests_controller.rb6
-rw-r--r--app/models/merge_request.rb36
-rw-r--r--app/services/merge_requests/merge_to_ref_service.rb20
-rw-r--r--app/services/merge_requests/mergeability_check_service.rb104
-rw-r--r--app/services/service_response.rb15
-rw-r--r--changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml5
-rw-r--r--doc/api/merge_requests.md20
-rw-r--r--lib/api/entities.rb2
-rw-r--r--lib/api/merge_requests.rb24
-rw-r--r--spec/models/merge_request_spec.rb87
-rw-r--r--spec/requests/api/merge_requests_spec.rb80
-rw-r--r--spec/services/merge_requests/merge_to_ref_service_spec.rb41
-rw-r--r--spec/services/merge_requests/mergeability_check_service_spec.rb240
-rw-r--r--spec/services/service_response_spec.rb16
14 files changed, 472 insertions, 224 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9e7e3ed5afb..58b1f0196fa 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def show
close_merge_request_if_no_source_project
- mark_merge_request_mergeable
+ @merge_request.check_mergeability
respond_to do |format|
format.html do
@@ -251,10 +251,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@merge_request.has_no_commits? && !@merge_request.target_branch_exists?
end
- def mark_merge_request_mergeable
- @merge_request.check_if_can_be_merged
- end
-
def merge!
# Disable the CI check if auto_merge_strategy is specified since we have
# to wait until CI completes to know
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7481f1939ad..4fcaac75655 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -725,19 +725,16 @@ class MergeRequest < ApplicationRecord
MergeRequests::ReloadDiffsService.new(self, current_user).execute
end
- # rubocop: enable CodeReuse/ServiceClass
-
- def check_if_can_be_merged
- return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write?
- can_be_merged =
- !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch)
+ def check_mergeability
+ MergeRequests::MergeabilityCheckService.new(self).execute
+ end
+ # rubocop: enable CodeReuse/ServiceClass
- if can_be_merged
- mark_as_mergeable
- else
- mark_as_unmergeable
- end
+ # Returns boolean indicating the merge_status should be rechecked in order to
+ # switch to either can_be_merged or cannot_be_merged.
+ def recheck_merge_status?
+ self.class.state_machines[:merge_status].check_state?(merge_status)
end
def merge_event
@@ -763,7 +760,7 @@ class MergeRequest < ApplicationRecord
def mergeable?(skip_ci_check: false)
return false unless mergeable_state?(skip_ci_check: skip_ci_check)
- check_if_can_be_merged
+ check_mergeability
can_be_merged? && !should_be_rebased?
end
@@ -778,15 +775,6 @@ class MergeRequest < ApplicationRecord
true
end
- def mergeable_to_ref?
- return false unless mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
-
- # Given the `merge_ref_path` will have the same
- # state the `target_branch` would have. Ideally
- # we need to check if it can be merged to it.
- project.repository.can_be_merged?(diff_head_sha, target_branch)
- end
-
def ff_merge_possible?
project.repository.ancestor?(target_branch_sha, diff_head_sha)
end
@@ -1099,6 +1087,12 @@ class MergeRequest < ApplicationRecord
target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path)
end
+ # Returns the current merge-ref HEAD commit.
+ #
+ def merge_ref_head
+ project.repository.commit(merge_ref_path)
+ end
+
def ref_path
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
end
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb
index 87147d90c32..8670b9ccf3d 100644
--- a/app/services/merge_requests/merge_to_ref_service.rb
+++ b/app/services/merge_requests/merge_to_ref_service.rb
@@ -20,20 +20,14 @@ module MergeRequests
raise_error('Conflicts detected during merge') unless commit_id
- commit = project.commit(commit_id)
- target_id, source_id = commit.parent_ids
-
- success(commit_id: commit.id,
- target_id: target_id,
- source_id: source_id)
- rescue MergeError => error
+ success(commit_id: commit_id)
+ rescue MergeError, ArgumentError => error
error(error.message)
end
private
def validate!
- authorization_check!
error_check!
end
@@ -43,21 +37,13 @@ module MergeRequests
error =
if !hooks_validation_pass?(merge_request)
hooks_validation_error(merge_request)
- elsif !@merge_request.mergeable_to_ref?
- "Merge request is not mergeable to #{target_ref}"
- elsif !source
+ elsif source.blank?
'No source for merge'
end
raise_error(error) if error
end
- def authorization_check!
- unless Ability.allowed?(current_user, :admin_merge_request, project)
- raise_error("You are not allowed to merge to this ref")
- end
- end
-
def target_ref
merge_request.merge_ref_path
end
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
new file mode 100644
index 00000000000..505c8594ce0
--- /dev/null
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -0,0 +1,104 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ class MergeabilityCheckService < ::BaseService
+ include Gitlab::Utils::StrongMemoize
+
+ delegate :project, to: :@merge_request
+ delegate :repository, to: :project
+
+ def initialize(merge_request)
+ @merge_request = merge_request
+ end
+
+ # Updates the MR merge_status. Whenever it switches to a can_be_merged state,
+ # the merge-ref is refreshed.
+ #
+ # recheck - When given, it'll enforce a merge-ref refresh if it's outdated,
+ # even if the current merge_status is can_be_merged or cannot_be_merged.
+ # Given MergeRequests::RefreshService is called async, it might happen that the target
+ # branch gets updated, but the MergeRequest#merge_status lags behind. So in scenarios
+ # where we need the current state of the merge ref in repository, the `recheck`
+ # argument is required.
+ #
+ # Returns a ServiceResponse indicating merge_status is/became can_be_merged
+ # and the merge-ref is synced. Success in case of being/becoming mergeable,
+ # error otherwise.
+ def execute(recheck: false)
+ return ServiceResponse.error(message: 'Invalid argument') unless merge_request
+ return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
+
+ recheck! if recheck
+ update_merge_status
+
+ unless merge_request.can_be_merged?
+ return ServiceResponse.error(message: 'Merge request is not mergeable')
+ end
+
+ ServiceResponse.success(payload: payload)
+ end
+
+ private
+
+ attr_reader :merge_request
+
+ def payload
+ strong_memoize(:payload) do
+ {
+ merge_ref_head: merge_ref_head_payload
+ }
+ end
+ end
+
+ def merge_ref_head_payload
+ commit = merge_request.merge_ref_head
+
+ return unless commit
+
+ target_id, source_id = commit.parent_ids
+
+ {
+ commit_id: commit.id,
+ source_id: source_id,
+ target_id: target_id
+ }
+ end
+
+ def update_merge_status
+ return unless merge_request.recheck_merge_status?
+
+ if can_git_merge? && merge_to_ref
+ merge_request.mark_as_mergeable
+ else
+ merge_request.mark_as_unmergeable
+ end
+ end
+
+ def recheck!
+ if !merge_request.recheck_merge_status? && outdated_merge_ref?
+ merge_request.mark_as_unchecked
+ end
+ end
+
+ # Checks if the existing merge-ref is synced with the target branch.
+ #
+ # Returns true if the merge-ref does not exists or is out of sync.
+ def outdated_merge_ref?
+ ref_head = merge_request.merge_ref_head
+
+ return true unless ref_head
+ return true unless target_id = merge_request.target_branch_sha
+
+ ref_head.parent_id != target_id
+ end
+
+ def can_git_merge?
+ !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
+ end
+
+ def merge_to_ref
+ result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+ result[:status] == :success
+ end
+ end
+end
diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index 1de30e68d87..f3437ba16de 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -1,19 +1,20 @@
# frozen_string_literal: true
class ServiceResponse
- def self.success(message: nil)
- new(status: :success, message: message)
+ def self.success(message: nil, payload: {})
+ new(status: :success, message: message, payload: payload)
end
- def self.error(message:, http_status: nil)
- new(status: :error, message: message, http_status: http_status)
+ def self.error(message:, payload: {}, http_status: nil)
+ new(status: :error, message: message, payload: payload, http_status: http_status)
end
- attr_reader :status, :message, :http_status
+ attr_reader :status, :message, :http_status, :payload
- def initialize(status:, message: nil, http_status: nil)
+ def initialize(status:, message: nil, payload: {}, http_status: nil)
self.status = status
self.message = message
+ self.payload = payload
self.http_status = http_status
end
@@ -27,5 +28,5 @@ class ServiceResponse
private
- attr_writer :status, :message, :http_status
+ attr_writer :status, :message, :http_status, :payload
end
diff --git a/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml b/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml
new file mode 100644
index 00000000000..1f40089adb8
--- /dev/null
+++ b/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml
@@ -0,0 +1,5 @@
+---
+title: Sync merge ref upon mergeability check
+merge_request: 28513
+author:
+type: added
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index dd7810c3403..7b58aa3100e 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -1191,33 +1191,29 @@ Parameters:
}
```
-## Merge to default merge ref path
+## Returns the up to date merge-ref HEAD commit
Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge`
-ref, of the target project repository. This ref will have the state the target branch would have if
+ref, of the target project repository, if possible. This ref will have the state the target branch would have if
a regular merge action was taken.
-This is not a regular merge action given it doesn't change the merge request state in any manner.
+This is not a regular merge action given it doesn't change the merge request target branch state in any manner.
-This ref (`refs/merge-requests/:iid/merge`) is **always** overwritten when submitting
-requests to this API, so none of its state is kept or used in the process.
+This ref (`refs/merge-requests/:iid/merge`) isn't necessarily overwritten when submitting
+requests to this API, though it'll make sure the ref has the latest possible state.
-If the merge request has conflicts, is empty or already merged,
-you'll get a `400` and a descriptive error message. If you don't have permissions to do so,
-you'll get a `403`.
+If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message.
-It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in
-case of `200`.
+It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in case of `200`.
```
-PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref
+GET /projects/:id/merge_requests/:merge_request_iid/merge_ref
```
Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
- `merge_request_iid` (required) - Internal ID of MR
-- `merge_commit_message` (optional) - Custom merge commit message
```json
{
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 8840accf675..82aa8310a1c 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -703,7 +703,7 @@ module API
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 for more
# information.
expose :merge_status do |merge_request|
- merge_request.check_if_can_be_merged
+ merge_request.check_mergeability
merge_request.merge_status
end
expose :diff_head_sha, as: :sha
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 955624404f1..bf87e9ec2ff 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -397,28 +397,16 @@ module API
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end
- desc 'Merge a merge request to its default temporary merge ref path'
- params do
- optional :merge_commit_message, type: String, desc: 'Custom merge commit message'
- end
- put ':id/merge_requests/:merge_request_iid/merge_to_ref' do
+ desc 'Returns the up to date merge-ref HEAD commit'
+ get ':id/merge_requests/:merge_request_iid/merge_ref' do
merge_request = find_project_merge_request(params[:merge_request_iid])
- authorize! :admin_merge_request, user_project
-
- merge_params = {
- commit_message: params[:merge_commit_message]
- }
-
- result = ::MergeRequests::MergeToRefService
- .new(merge_request.target_project, current_user, merge_params)
- .execute(merge_request)
+ result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
- if result[:status] == :success
- present result.slice(:commit_id), 200
+ if result.success?
+ present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
else
- http_status = result[:http_status] || 400
- render_api_error!(result[:message], http_status)
+ render_api_error!(result.message, 400)
end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c6251326c22..fc28c216b21 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1996,57 +1996,6 @@ 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) }
@@ -2060,7 +2009,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_if_can_be_merged)
+ expect(subject).to receive(:check_mergeability)
expect(subject).to receive(:can_be_merged?) { true }
expect(subject.mergeable?).to be_truthy
@@ -2074,7 +2023,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_if_can_be_merged)
+ expect(subject).to receive(:check_mergeability)
subject.mergeable?
end
@@ -3142,38 +3091,6 @@ 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 9f9180bc8c9..76d093e0774 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -1546,52 +1546,80 @@ describe API::MergeRequests do
end
end
- describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do
- let(:pipeline) { create(:ci_pipeline_without_jobs) }
+ 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 }
+
let(:url) do
- "/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref"
+ "/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref"
end
it 'returns the generated ID from the merge service in case of success' do
- put api(url, user), params: { merge_commit_message: 'Custom message' }
-
- commit = project.commit(json_response['commit_id'])
+ get api(url, user)
expect(response).to have_gitlab_http_status(200)
- expect(json_response['commit_id']).to be_present
- expect(commit.message).to eq('Custom message')
+ expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
end
- it "returns 400 if branch can't be merged" do
- merge_request.update!(state: 'merged')
+ context 'when merge-ref is not synced with merge status' do
+ before do
+ merge_request.update!(merge_status: 'cannot_be_merged')
+ end
- put api(url, user)
+ it 'returns 200 if MR can be merged' do
+ get api(url, user)
- expect(response).to have_gitlab_http_status(400)
- expect(json_response['message'])
- .to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
+ expect(response).to have_gitlab_http_status(200)
+ expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
+ end
+
+ it 'returns 400 if MR cannot be merged' do
+ expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_request|
+ expect(merge_request).to receive(:execute) { { status: :failed } }
+ end
+
+ get api(url, user)
+
+ expect(response).to have_gitlab_http_status(400)
+ expect(json_response['message']).to eq('Merge request is not mergeable')
+ end
end
- it 'returns 403 if user has no permissions to merge to the ref' do
- user2 = create(:user)
- project.add_reporter(user2)
+ 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) }
- put api(url, user2)
+ it 'returns 404' do
+ project.add_guest(user)
- expect(response).to have_gitlab_http_status(403)
- expect(json_response['message']).to eq('403 Forbidden')
+ get api(url, user)
+
+ expect(response).to have_gitlab_http_status(404)
+ expect(json_response['message']).to eq('404 Not found')
+ end
end
- it 'returns 404 for an invalid merge request IID' do
- put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user)
+ context 'when invalid merge request IID' do
+ let(:merge_request_iid) { '12345' }
- expect(response).to have_gitlab_http_status(404)
+ it 'returns 404' do
+ get api(url, user)
+
+ expect(response).to have_gitlab_http_status(404)
+ end
end
- 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)
+ context 'when merge request ID is used instead IID' do
+ let(:merge_request_iid) { merge_request.id }
- expect(response).to have_gitlab_http_status(404)
+ it 'returns 404' do
+ get api(url, user)
+
+ expect(response).to have_gitlab_http_status(404)
+ end
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 0ac23050caf..5d492e4b013 100644
--- a/spec/services/merge_requests/merge_to_ref_service_spec.rb
+++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb
@@ -32,10 +32,8 @@ 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.id).to eq(result[:commit_id])
+ expect(ref_head.sha).to eq(result[:commit_id])
end
end
@@ -72,10 +70,6 @@ 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',
@@ -92,6 +86,12 @@ 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(merge_request).to receive(:mergeable_to_ref?) { false }
+ allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil }
- error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
+ error_message = 'Conflicts detected during merge'
result = service.execute(merge_request)
@@ -170,28 +170,5 @@ 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
new file mode 100644
index 00000000000..8800907c304
--- /dev/null
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -0,0 +1,240 @@
+# 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 'recheck enforced' do
+ subject { described_class.new(merge_request).execute(recheck: true) }
+
+ context 'when MR is mergeable but merge-ref does not exists' do
+ before do
+ merge_request.mark_as_mergeable!
+ end
+
+ it_behaves_like 'mergeable merge request'
+ end
+
+ context 'when MR is mergeable but target-branch is out of sync with merge-ref' do
+ let(:fake_commit) { build(:commit) }
+
+ before do
+ MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
+
+ allow(merge_request).to receive(:target_branch_sha) { fake_commit.id }
+
+ merge_request.mark_as_mergeable!
+ end
+
+ context 'when successfully updates the merge-ref' do
+ it 'keeps merge status as can_be_merged' do
+ expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
+ end
+
+ it 'updates the merge ref' do
+ expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
+ expect(merge_to_ref).to receive(:execute).and_call_original
+ end
+
+ subject
+ 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
+
+ context 'when fails to update the merge-ref' do
+ before do
+ expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
+ expect(merge_to_ref).to receive(:execute).and_return(status: :failed)
+ end
+ 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
+ end
+ end
+ end
+end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 30bd4d6820b..e790d272e61 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -16,6 +16,13 @@ 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
@@ -33,6 +40,15 @@ 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