Commit ef8cf947 authored by Furkan Ayhan's avatar Furkan Ayhan Committed by Dylan Griffith

Implement creating TODOs for merge train errors

When a merge train is dropped, we sometimes send TODO notification,
sometimes don't. In this commit, we implement abort notifications.
parent 89a5f6f3
......@@ -22,6 +22,7 @@ module TodosHelper
when Todo::APPROVAL_REQUIRED then "set #{todo_action_subject(todo)} as an approver for"
when Todo::UNMERGEABLE then 'Could not merge'
when Todo::DIRECTLY_ADDRESSED then "directly addressed #{todo_action_subject(todo)} on"
when Todo::MERGE_TRAIN_REMOVED then "Removed from Merge Train:"
end
end
......@@ -197,6 +198,10 @@ module TodosHelper
"· #{content}".html_safe
end
def todo_author_display?(todo)
!todo.build_failed? && !todo.unmergeable?
end
private
def todos_design_path(todo, path_options)
......
......@@ -7,15 +7,16 @@ class Todo < ApplicationRecord
# Time to wait for todos being removed when not visible for user anymore.
# Prevents TODOs being removed by mistake, for example, removing access from a user
# and giving it back again.
WAIT_FOR_DELETE = 1.hour
WAIT_FOR_DELETE = 1.hour
ASSIGNED = 1
MENTIONED = 2
BUILD_FAILED = 3
MARKED = 4
APPROVAL_REQUIRED = 5 # This is an EE-only feature
UNMERGEABLE = 6
DIRECTLY_ADDRESSED = 7
ASSIGNED = 1
MENTIONED = 2
BUILD_FAILED = 3
MARKED = 4
APPROVAL_REQUIRED = 5 # This is an EE-only feature
UNMERGEABLE = 6
DIRECTLY_ADDRESSED = 7
MERGE_TRAIN_REMOVED = 8 # This is an EE-only feature
ACTION_NAMES = {
ASSIGNED => :assigned,
......@@ -24,7 +25,8 @@ class Todo < ApplicationRecord
MARKED => :marked,
APPROVAL_REQUIRED => :approval_required,
UNMERGEABLE => :unmergeable,
DIRECTLY_ADDRESSED => :directly_addressed
DIRECTLY_ADDRESSED => :directly_addressed,
MERGE_TRAIN_REMOVED => :merge_train_removed
}.freeze
belongs_to :author, class_name: "User"
......@@ -165,6 +167,10 @@ class Todo < ApplicationRecord
action == ASSIGNED
end
def merge_train_removed?
action == MERGE_TRAIN_REMOVED
end
def done?
state == 'done'
end
......
......@@ -4,7 +4,7 @@
.todo-item.todo-block.align-self-center
.todo-title
- unless todo.build_failed? || todo.unmergeable?
- if todo_author_display?(todo)
= todo_target_state_pill(todo)
%span.title-item.author-name.bold
......
......@@ -8,5 +8,10 @@ module EE
def todo_types_options
super + [{ id: 'Epic', text: 'Epic' }]
end
override :todo_author_display?
def todo_author_display?(todo)
super && !todo.merge_train_removed?
end
end
end
......@@ -25,11 +25,12 @@ module EE
# Called when 'merge train' is aborted
def abort(reason)
body = "removed this merge request from the merge train because #{reason}"
::TodoService.new.merge_train_removed(noteable)
##
# TODO: Abort message should be sent by the system, not a particular user.
# See https://gitlab.com/gitlab-org/gitlab/issues/29467.
body = "removed this merge request from the merge train because #{reason}"
create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
end
......@@ -49,11 +50,12 @@ module EE
# Called when 'add to merge train when pipeline succeeds' is aborted
def abort_add_when_pipeline_succeeds(reason)
body = "aborted automatic add to merge train because #{reason}"
::TodoService.new.merge_train_removed(noteable)
##
# TODO: Abort message should be sent by the system, not a particular user.
# See https://gitlab.com/gitlab-org/gitlab/issues/29467.
body = "aborted automatic add to merge train because #{reason}"
create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
end
end
......
......@@ -30,6 +30,16 @@ module EE
create_mention_todos(nil, epic, current_user, nil, skip_users)
end
# When a merge train is aborted for some reason, we should:
#
# * create a todo for each merge request participant
#
def merge_train_removed(merge_request)
merge_request.merge_participants.each do |user|
create_merge_train_removed_todo(merge_request, user)
end
end
private
override :attributes_for_target
......@@ -55,5 +65,10 @@ module EE
create_todos(approvers, attributes)
end
def create_merge_train_removed_todo(merge_request, user)
attributes = attributes_for_todo(merge_request.project, merge_request, user, ::Todo::MERGE_TRAIN_REMOVED)
create_todos(user, attributes)
end
end
end
---
title: Create TODOs for merge train errors
merge_request: 35951
author:
type: changed
......@@ -10,4 +10,25 @@ RSpec.describe ::TodosHelper do
)
end
end
describe '#todo_author_display?' do
using RSpec::Parameterized::TableSyntax
let!(:todo) { create(:todo) }
subject { helper.todo_author_display?(todo) }
where(:action, :result) do
::Todo::MERGE_TRAIN_REMOVED | false
::Todo::ASSIGNED | true
end
with_them do
before do
todo.action = action
end
it { is_expected.to eq(result) }
end
end
end
......@@ -6,6 +6,16 @@ RSpec.describe EE::SystemNotes::MergeTrainService do
let_it_be(:author) { create(:user) }
let_it_be(:project) { create(:project) }
shared_examples 'creates a removed merge train TODO' do
it 'creates Todo of MERGE_TRAIN_REMOVED' do
expect { subject }.to change { Todo.count }.to(1)
todo = Todo.last
expect(todo.target).to eq(noteable)
expect(todo.action).to eq(Todo::MERGE_TRAIN_REMOVED)
end
end
describe '#enqueue' do
subject { described_class.new(noteable: noteable, project: project, author: author).enqueue(noteable.merge_train) }
......@@ -56,6 +66,8 @@ RSpec.describe EE::SystemNotes::MergeTrainService do
it "posts the 'merge train' system note" do
expect(subject.note).to eq('removed this merge request from the merge train because source branch was updated')
end
it_behaves_like 'creates a removed merge train TODO'
end
describe '#add_when_pipeline_succeeds' do
......@@ -106,5 +118,7 @@ RSpec.describe EE::SystemNotes::MergeTrainService do
it "posts the 'add to merge train when pipeline succeeds' system note" do
expect(subject.note).to eq 'aborted automatic add to merge train because target branch was changed'
end
it_behaves_like 'creates a removed merge train TODO'
end
end
......@@ -323,6 +323,23 @@ RSpec.describe TodoService do
end
end
end
describe '#merge_train_removed' do
let(:merge_participants) { [admin, create(:user)] }
before do
allow(merge_request).to receive(:merge_participants).and_return(merge_participants)
end
it 'creates a pending todo for each merge_participant' do
merge_request.update!(merge_when_pipeline_succeeds: true, merge_user: admin)
service.merge_train_removed(merge_request)
merge_participants.each do |participant|
should_create_todo(user: participant, author: participant, target: merge_request, action: Todo::MERGE_TRAIN_REMOVED)
end
end
end
end
def should_create_todo(attributes = {})
......
......@@ -222,4 +222,24 @@ RSpec.describe TodosHelper do
end
end
end
describe '#todo_author_display?' do
using RSpec::Parameterized::TableSyntax
subject { helper.todo_author_display?(alert_todo) }
where(:action, :result) do
Todo::BUILD_FAILED | false
Todo::UNMERGEABLE | false
Todo::ASSIGNED | true
end
with_them do
before do
alert_todo.action = action
end
it { is_expected.to eq(result) }
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