diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 99687d305e7..a0ad7f3c609 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -127,6 +127,7 @@ class MergeRequest < ActiveRecord::Base after_transition unchecked: :cannot_be_merged do |merge_request, transition| NotificationService.new.merge_request_unmergeable(merge_request) + TodoService.new.merge_request_became_unmergeable(merge_request) end def check_state?(merge_status) 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 850deb0ac7a..9a4e6eb2e88 100644 --- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb @@ -24,11 +24,7 @@ module MergeRequests pipeline_merge_requests(pipeline) do |merge_request| next unless merge_request.merge_when_pipeline_succeeds? - - unless merge_request.mergeable? - todo_service.merge_request_became_unmergeable(merge_request) - next - end + next unless merge_request.mergeable? merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params) end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index ffd48e842c2..f91cd03bf5c 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -98,12 +98,12 @@ class TodoService # When a build fails on the HEAD of a merge request we should: # - # * create a todo for author of MR to fix it - # * create a todo for merge_user to keep an eye on it + # * create a todo for each merge participant # def merge_request_build_failed(merge_request) - create_build_failed_todo(merge_request, merge_request.author) - create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? + merge_request.merge_participants.each do |user| + create_build_failed_todo(merge_request, user) + end end # When a new commit is pushed to a merge request we should: @@ -116,20 +116,22 @@ class TodoService # When a build is retried to a merge request we should: # - # * mark all pending todos related to the merge request for the author as done - # * mark all pending todos related to the merge request for the merge_user as done + # * mark all pending todos related to the merge request as done for each merge participant # def merge_request_build_retried(merge_request) - mark_pending_todos_as_done(merge_request, merge_request.author) - mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? + merge_request.merge_participants.each do |user| + mark_pending_todos_as_done(merge_request, user) + end end - # When a merge request could not be automatically merged due to its unmergeable state we should: + # When a merge request could not be merged due to its unmergeable state we should: # - # * create a todo for a merge_user + # * create a todo for each merge participant # def merge_request_became_unmergeable(merge_request) - create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? + merge_request.merge_participants.each do |user| + create_unmergeable_todo(merge_request, user) + end end # When create a note we should: @@ -275,9 +277,9 @@ class TodoService create_todos(todo_author, attributes) end - def create_unmergeable_todo(merge_request, merge_user) - attributes = attributes_for_todo(merge_request.project, merge_request, merge_user, Todo::UNMERGEABLE) - create_todos(merge_user, attributes) + def create_unmergeable_todo(merge_request, todo_author) + attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::UNMERGEABLE) + create_todos(todo_author, attributes) end def attributes_for_target(target) diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f13d29884d4..762bf616268 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -31,6 +31,9 @@ A Todo appears in your Todos dashboard when: - you are `@mentioned` in a comment on a commit, - a job in the CI pipeline running for your merge request failed, but this job is not allowed to fail. +- a merge request becomes unmergeable, and you are either: + - the author, or + - have set it to automatically merge once pipeline succeeds. ### Directly addressed Todos 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 240aa638f79..8838742a637 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 @@ -112,32 +112,6 @@ 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_pipeline_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', - head_pipeline_of: mr_conflict) - 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 diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 562b89e6767..9a51c873b30 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -721,17 +721,18 @@ describe TodoService do end describe '#merge_request_build_failed' do - it 'creates a pending todo for the merge request author' do - service.merge_request_build_failed(mr_unassigned) + let(:merge_participants) { [mr_unassigned.author, admin] } - should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED) + before do + allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants) end - it 'creates a pending todo for merge_user' do - mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin) + it 'creates a pending todo for each merge_participant' do service.merge_request_build_failed(mr_unassigned) - should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED) + merge_participants.each do |participant| + should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::BUILD_FAILED) + end end end @@ -747,11 +748,19 @@ describe TodoService do end describe '#merge_request_became_unmergeable' do - it 'creates a pending todo for a merge_user' do + let(:merge_participants) { [admin, create(:user)] } + + before do + allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants) + end + + it 'creates a pending todo for each merge_participant' do mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin) service.merge_request_became_unmergeable(mr_unassigned) - should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::UNMERGEABLE) + merge_participants.each do |participant| + should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::UNMERGEABLE) + end end end