diff options
author | Sean McGivern <sean@gitlab.com> | 2017-07-07 15:09:29 +0100 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2017-07-07 15:34:33 +0100 |
commit | 4209b647f87d99b21aebcc9f311e8214c2658136 (patch) | |
tree | 994064741174944171682117411052716c9bd0ff | |
parent | d40445e4c9964ae0ab793bfdd7ba530de4259716 (diff) | |
download | gitlab-ce-fix-mrs-merged-immediately.tar.gz |
Don't mark empty MRs as merged on push to the target branchfix-mrs-merged-immediately
When we push to an MR's target branch, we check if the MR's HEAD commit is
contained in the push. This lets us mark MRs as merged if they were merged
manually.
However, we also added a feature where you can create an empty MR from an
issue. If that MR is created around the time of a merge to the default branch,
we would process the push after creating the MR, and consider it to be a manual
merge.
To fix that, we exclude empty MRs from this process. If they are empty, they
were empty before the push we're processing, so we shouldn't touch them!
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/fix-mrs-merged-immediately.yml | 4 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 54 |
3 files changed, 51 insertions, 12 deletions
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index c5f959a1874..bc4a13cf4bc 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -35,11 +35,12 @@ module MergeRequests # target branch manually def close_merge_requests commit_ids = @commits.map(&:id) - merge_requests = @project.merge_requests.opened.where(target_branch: @branch_name).to_a + merge_requests = @project.merge_requests.preload(:merge_request_diff).opened.where(target_branch: @branch_name).to_a merge_requests = merge_requests.select(&:diff_head_commit) merge_requests = merge_requests.select do |merge_request| - commit_ids.include?(merge_request.diff_head_sha) + commit_ids.include?(merge_request.diff_head_sha) && + merge_request.merge_request_diff.state != 'empty' end filter_merge_requests(merge_requests).each do |merge_request| diff --git a/changelogs/unreleased/fix-mrs-merged-immediately.yml b/changelogs/unreleased/fix-mrs-merged-immediately.yml new file mode 100644 index 00000000000..41c06614e6d --- /dev/null +++ b/changelogs/unreleased/fix-mrs-merged-immediately.yml @@ -0,0 +1,4 @@ +--- +title: Don't mark empty MRs as merged on push to the target branch +merge_request: +author: diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 671a932441e..74dcf152cb8 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -98,18 +98,52 @@ describe MergeRequests::RefreshService, services: true do end context 'push to origin repo target branch' do - before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs + context 'when all MRs to the target branch had diffs' do + before do + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge state' do + expect(@merge_request.notes.last.note).to include('merged') + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + expect(@fork_merge_request.notes.last.note).to include('merged') + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + end end - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + context 'when an MR to be closed was empty already' do + let!(:empty_fork_merge_request) do + create(:merge_request, + source_project: @fork_project, + source_branch: 'master', + target_branch: 'master', + target_project: @project) + end + + before do + # This spec already has a fake push, so pretend that we were targeting + # feature all along. + empty_fork_merge_request.update_columns(target_branch: 'feature') + + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + empty_fork_merge_request.reload + end + + it 'only updates the non-empty MRs' do + expect(@merge_request).to be_merged + expect(@merge_request.notes.last.note).to include('merged') + + expect(@fork_merge_request).to be_merged + expect(@fork_merge_request.notes.last.note).to include('merged') + + expect(empty_fork_merge_request).to be_open + expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') + expect(empty_fork_merge_request.notes).to be_empty + end end end |