Commit 736d44ce authored by Marc Shaw's avatar Marc Shaw

Move checks from mergeable to mergeable_state?

We also change the status code from 406 -> 422 to better describe the
error that we are returning

MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/82465
Issue: gitlab.com/gitlab-org/gitlab/-/issues/354865

Changelog: fixed
parent 2ad63f89
......@@ -191,13 +191,17 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
def mergeable_discussions_state
# This avoids calling MergeRequest#mergeable_discussions_state without
# considering the state of the MR first. If a MR isn't mergeable, we can
# safely short-circuit it.
if merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
if Feature.enabled?(:change_response_code_merge_status, project, default_enabled: :yaml)
merge_request.mergeable_discussions_state?
else
false
# This avoids calling MergeRequest#mergeable_discussions_state without
# considering the state of the MR first. If a MR isn't mergeable, we can
# safely short-circuit it.
if merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
merge_request.mergeable_discussions_state?
else
false
end
end
end
......
......@@ -33,13 +33,17 @@ class MergeRequestPollWidgetEntity < Grape::Entity
# Booleans
expose :mergeable_discussions_state?, as: :mergeable_discussions_state do |merge_request|
# This avoids calling MergeRequest#mergeable_discussions_state without
# considering the state of the MR first. If a MR isn't mergeable, we can
# safely short-circuit it.
if merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
if Feature.enabled?(:change_response_code_merge_status, merge_request.project, default_enabled: :yaml)
merge_request.mergeable_discussions_state?
else
false
# This avoids calling MergeRequest#mergeable_discussions_state without
# considering the state of the MR first. If a MR isn't mergeable, we can
# safely short-circuit it.
if merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
merge_request.mergeable_discussions_state?
else
false
end
end
end
......
---
name: change_response_code_merge_status
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82465/
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/356930
milestone: '14.10'
type: development
group: group::code review
default_enabled: false
......@@ -1629,7 +1629,7 @@ This API returns specific HTTP status codes on failure:
|:------------|--------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------|
| `401` | `Unauthorized` | This user does not have permission to accept this merge request. |
| `405` | `Method Not Allowed` | The merge request cannot be accepted because it is `Draft`, `Closed`, `Pipeline Pending Completion`, or `Failed`. `Success` is required. |
| `406` | `Branch cannot be merged` | The branch has conflicts and cannot be merged. |
| `406` | `Branch cannot be merged` | The merge request can not be merged. |
| `409` | `SHA does not match HEAD of source branch` | The provided `sha` parameter does not match the HEAD of the source. |
For additional important notes on response data, read [Single merge request response notes](#single-merge-request-response-notes).
......
......@@ -117,8 +117,8 @@ module EE
super.concat(merge_request_approval_variables)
end
override :mergeable?
def mergeable?(skip_ci_check: false, skip_discussions_check: false)
override :mergeable_state?
def mergeable_state?(skip_ci_check: false, skip_discussions_check: false)
return false unless approved?
return false if has_denied_policies?
return false if merge_blocked_by_other_mrs?
......
......@@ -1155,15 +1155,13 @@ RSpec.describe MergeRequest do
end
end
describe '#mergeable?' do
subject { merge_request.mergeable? }
describe '#mergeable_state?' do
subject { merge_request.mergeable_state? }
context 'when using approvals' do
let(:user) { create(:user) }
before do
allow(merge_request).to receive(:mergeable_state?).and_return(true)
merge_request.target_project.update!(approvals_before_merge: 1)
project.add_developer(user)
end
......@@ -1216,6 +1214,26 @@ RSpec.describe MergeRequest do
end
end
end
context 'when blocking merge requests' do
before do
stub_licensed_features(blocking_merge_requests: true)
end
context 'when the merge request is blocked' do
let(:merge_request) { create(:merge_request, :blocked, source_project: project, target_project: project) }
it 'is not mergeable' do
is_expected.to be_falsey
end
end
context 'when merge request is not blocked' do
it 'is mergeable' do
is_expected.to be_truthy
end
end
end
end
describe '#on_train?' do
......
......@@ -178,8 +178,7 @@ RSpec.describe API::MergeRequests do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
expect(response).to have_gitlab_http_status(:not_acceptable)
expect(json_response['message']).to eq('Branch cannot be merged')
expect(response).to have_gitlab_http_status(:method_not_allowed)
end
it 'returns 200 if merge request was approved' do
......
......@@ -263,6 +263,7 @@ RSpec.describe MergeRequests::RefreshService do
describe 'Pipelines for merge requests', :sidekiq_inline do
let(:service) { described_class.new(project: project, current_user: current_user) }
let(:project) { create(:project, :repository, namespace: group, reset_approvals_on_push: true) }
let(:current_user) { merge_request.author }
let(:config) do
......
......@@ -458,7 +458,11 @@ module API
not_allowed! if !immediately_mergeable && !automatically_mergeable
render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: automatically_mergeable)
if Feature.enabled?(:change_response_code_merge_status, user_project, default_enabled: :yaml)
render_api_error!('Branch cannot be merged', 422) unless merge_request.mergeable?(skip_ci_check: automatically_mergeable)
else
render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: automatically_mergeable)
end
check_sha_param!(params, merge_request)
......
......@@ -2558,14 +2558,32 @@ RSpec.describe API::MergeRequests do
expect(response).to have_gitlab_http_status(:ok)
end
it "returns 406 if branch can't be merged" do
allow_any_instance_of(MergeRequest)
.to receive(:can_be_merged?).and_return(false)
context 'when change_response_code_merge_status is enabled' do
it "returns 422 if branch can't be merged" do
allow_any_instance_of(MergeRequest)
.to receive(:can_be_merged?).and_return(false)
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
expect(response).to have_gitlab_http_status(:not_acceptable)
expect(json_response['message']).to eq('Branch cannot be merged')
expect(response).to have_gitlab_http_status(:unprocessable_entity)
expect(json_response['message']).to eq('Branch cannot be merged')
end
end
context 'when change_response_code_merge_status is disabled' do
before do
stub_feature_flags(change_response_code_merge_status: false)
end
it "returns 406 if branch can't be merged" do
allow_any_instance_of(MergeRequest)
.to receive(:can_be_merged?).and_return(false)
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user)
expect(response).to have_gitlab_http_status(:not_acceptable)
expect(json_response['message']).to eq('Branch cannot be merged')
end
end
it "returns 405 if merge_request is not open" do
......
......@@ -182,4 +182,40 @@ RSpec.describe MergeRequestPollWidgetEntity do
end
end
end
describe '#mergeable_discussions_state?' do
context 'when change_response_code_merge_status is true' do
before do
stub_feature_flags(change_response_code_merge_status: true)
end
it 'returns mergeable discussions state' do
expect(subject[:mergeable_discussions_state]).to eq(true)
end
end
context 'when change_response_code_merge_status is false' do
context 'when merge request is in a mergeable state' do
before do
stub_feature_flags(change_response_code_merge_status: false)
allow(resource).to receive(:mergeable_discussions_state?).and_return(true)
end
it 'returns mergeable discussions state' do
expect(subject[:mergeable_discussions_state]).to eq(true)
end
end
context 'when merge request is not in a mergeable state' do
before do
stub_feature_flags(change_response_code_merge_status: false)
allow(resource).to receive(:mergeable_discussions_state?).and_return(false)
end
it 'returns mergeable discussions state' do
expect(subject[:mergeable_discussions_state]).to eq(false)
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment