Commit 2da8bc3d authored by Sean McGivern's avatar Sean McGivern

Only create unmergeable todos 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.
parent fc567da4
...@@ -91,10 +91,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -91,10 +91,6 @@ class MergeRequest < ActiveRecord::Base
around_transition do |merge_request, transition, block| around_transition do |merge_request, transition, block|
Gitlab::Timeless.timeless(merge_request, &block) Gitlab::Timeless.timeless(merge_request, &block)
end end
after_transition unchecked: :cannot_be_merged do |merge_request, transition|
TodoService.new.merge_request_became_unmergeable(merge_request)
end
end end
validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?] validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?]
......
...@@ -24,7 +24,11 @@ module MergeRequests ...@@ -24,7 +24,11 @@ module MergeRequests
pipeline_merge_requests(pipeline) do |merge_request| pipeline_merge_requests(pipeline) do |merge_request|
next unless merge_request.merge_when_build_succeeds? 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) MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
end end
......
---
title: Only create unmergeable todos once when MR fails to merge
merge_request:
author:
...@@ -833,12 +833,6 @@ describe MergeRequest, models: true do ...@@ -833,12 +833,6 @@ describe MergeRequest, models: true do
it 'becomes unmergeable' do it 'becomes unmergeable' do
expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
end 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 end
context 'when it has conflicts' do context 'when it has conflicts' do
......
...@@ -111,6 +111,31 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do ...@@ -111,6 +111,31 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do
service.trigger(unrelated_pipeline) service.trigger(unrelated_pipeline)
end end
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 end
describe "#cancel" do describe "#cancel" do
......
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