summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-02-24 15:56:41 +0000
committerSean McGivern <sean@gitlab.com>2017-02-24 17:18:12 +0000
commit2da8bc3de9f8b63bd80a081c7e2880adee3edb71 (patch)
treebab714b283a3690d03301241663bebcb41509e40
parentfc567da473072bf11ea8d0fe8ddf9a596349f03f (diff)
downloadgitlab-ce-2da8bc3de9f8b63bd80a081c7e2880adee3edb71.tar.gz
Only create unmergeable todos onceonly-create-unmergeable-todo-once
Previously, we created an unmergeable todo when a merge request: 1. Had merge when pipeline succeeds set. 2. Became unmergeable. However, when merge when pipeline succeeds fails due to unmergeability, the flag isn't actually removed. And a merge request can become unmergeable multiple times, as every time the target branch is updated we need to re-check the mergeable status. This means that if the todo was marked done, and the MR was checked again, a new todo would be created for the same event. Instead of checking this, we should create the todo from the service responsible for merging when the pipeline succeeds. That way the todo is guaranteed to only be created when we care about it.
-rw-r--r--app/models/merge_request.rb4
-rw-r--r--app/services/merge_requests/merge_when_pipeline_succeeds_service.rb6
-rw-r--r--changelogs/unreleased/only-create-unmergeable-todo-once.yml4
-rw-r--r--spec/models/merge_request_spec.rb6
-rw-r--r--spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb25
5 files changed, 34 insertions, 11 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7eb875f1ef5..d6e7ed87555 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -91,10 +91,6 @@ class MergeRequest < ActiveRecord::Base
around_transition do |merge_request, transition, block|
Gitlab::Timeless.timeless(merge_request, &block)
end
-
- after_transition unchecked: :cannot_be_merged do |merge_request, transition|
- TodoService.new.merge_request_became_unmergeable(merge_request)
- end
end
validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?]
diff --git a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
index 5616edf8b4a..5081dd5a0c4 100644
--- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb
@@ -24,7 +24,11 @@ module MergeRequests
pipeline_merge_requests(pipeline) do |merge_request|
next unless merge_request.merge_when_build_succeeds?
- next unless merge_request.mergeable?
+
+ unless merge_request.mergeable?
+ todo_service.merge_request_became_unmergeable(merge_request)
+ next
+ end
MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
end
diff --git a/changelogs/unreleased/only-create-unmergeable-todo-once.yml b/changelogs/unreleased/only-create-unmergeable-todo-once.yml
new file mode 100644
index 00000000000..e675ed945ad
--- /dev/null
+++ b/changelogs/unreleased/only-create-unmergeable-todo-once.yml
@@ -0,0 +1,4 @@
+---
+title: Only create unmergeable todos once when MR fails to merge
+merge_request:
+author:
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index fa1b0396bcf..9331dc41a5e 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -833,12 +833,6 @@ describe MergeRequest, models: true do
it 'becomes unmergeable' do
expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
end
-
- it 'creates Todo on unmergeability' do
- expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(subject)
-
- subject.check_if_can_be_merged
- end
end
context 'when it has conflicts' do
diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
index f92978a33a3..0ff6e8fda16 100644
--- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
@@ -111,6 +111,31 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do
service.trigger(unrelated_pipeline)
end
end
+
+ context 'when the merge request is not mergeable' do
+ let(:mr_conflict) do
+ create(:merge_request, merge_when_build_succeeds: true, merge_user: user,
+ source_branch: 'master', target_branch: 'feature-conflict',
+ source_project: project, target_project: project)
+ end
+
+ let(:conflict_pipeline) do
+ create(:ci_pipeline, project: project, ref: mr_conflict.source_branch,
+ sha: mr_conflict.diff_head_sha, status: 'success')
+ end
+
+ it 'does not merge the merge request' do
+ expect(MergeWorker).not_to receive(:perform_async)
+
+ service.trigger(conflict_pipeline)
+ end
+
+ it 'creates todos for unmergeability' do
+ expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(mr_conflict)
+
+ service.trigger(conflict_pipeline)
+ end
+ end
end
describe "#cancel" do