Commit 88c9a057 authored by Stan Hu's avatar Stan Hu

Merge branch '19035-fix-merge-issue' into 'master'

Fix a wrong MR status when merge_when_build_succeeds & project.only_allow_merge_if_build_succeeds are true

## What does this MR do?

Fix a wrong MR status when merge_when_build_succeeds & project.only_allow_merge_if_build_succeeds are true.

## Are there points in the code the reviewer needs to double check?

@stanhu I reused your proposal from the issue, I think it's a good enough solution.

## What are the relevant issue numbers?

Fixes #19035.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4912
parents 5d174ee5 e3d464d8
...@@ -23,6 +23,7 @@ v 8.9.1 ...@@ -23,6 +23,7 @@ v 8.9.1
- Fix mobile Safari bug where horizontal nav arrows would flicker on scroll - Fix mobile Safari bug where horizontal nav arrows would flicker on scroll
- Fix in auto merge when pipeline is nil - Fix in auto merge when pipeline is nil
- Fix GitLab import project deleting imported file straight after being uploaded - Fix GitLab import project deleting imported file straight after being uploaded
- Fix a wrong MR status when merge_when_build_succeeds & project.only_allow_merge_if_build_succeeds are true
v 8.9.0 v 8.9.0
- Fix builds API response not including commit data - Fix builds API response not including commit data
......
...@@ -199,7 +199,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -199,7 +199,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def merge def merge
return access_denied! unless @merge_request.can_be_merged_by?(current_user) return access_denied! unless @merge_request.can_be_merged_by?(current_user)
unless @merge_request.mergeable? # Disable the CI check if merge_when_build_succeeds is enabled since we have
# to wait until CI completes to know
unless @merge_request.mergeable?(skip_ci_check: merge_when_build_succeeds_active?)
@status = :failed @status = :failed
return return
end end
...@@ -395,4 +397,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -395,4 +397,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def ensure_ref_fetched def ensure_ref_fetched
@merge_request.ensure_ref_fetched @merge_request.ensure_ref_fetched
end end
def merge_when_build_succeeds_active?
params[:merge_when_build_succeeds].present? &&
@merge_request.pipeline && @merge_request.pipeline.active?
end
end end
...@@ -264,19 +264,19 @@ class MergeRequest < ActiveRecord::Base ...@@ -264,19 +264,19 @@ class MergeRequest < ActiveRecord::Base
self.title.sub(WIP_REGEX, "") self.title.sub(WIP_REGEX, "")
end end
def mergeable? def mergeable?(skip_ci_check: false)
return false unless mergeable_state? return false unless mergeable_state?(skip_ci_check: skip_ci_check)
check_if_can_be_merged check_if_can_be_merged
can_be_merged? can_be_merged?
end end
def mergeable_state? def mergeable_state?(skip_ci_check: false)
return false unless open? return false unless open?
return false if work_in_progress? return false if work_in_progress?
return false if broken? return false if broken?
return false unless mergeable_ci_state? return false unless skip_ci_check || mergeable_ci_state?
true true
end end
......
...@@ -264,6 +264,18 @@ describe Projects::MergeRequestsController do ...@@ -264,6 +264,18 @@ describe Projects::MergeRequestsController do
merge_when_build_succeeds merge_when_build_succeeds
end end
context 'when project.only_allow_merge_if_build_succeeds? is true' do
before do
project.update_column(:only_allow_merge_if_build_succeeds, true)
end
it 'returns :merge_when_build_succeeds' do
merge_when_build_succeeds
expect(assigns(:status)).to eq(:merge_when_build_succeeds)
end
end
end 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