Merge branch '12136-create-todos-for-dropped-merge-trains-V2' into 'master'

Implement creating TODOs for merge train errors

See merge request gitlab-org/gitlab!35951

app/helpers/todos_helper.rb

@@ -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)

app/models/todo.rb

@@ -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

app/views/dashboard/todos/_todo.html.haml

@@ -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

ee/app/helpers/ee/todos_helper.rb

@@ -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

ee/app/services/ee/system_notes/merge_train_service.rb

@@ -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

ee/app/services/ee/todo_service.rb

@@ -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

ee/changelogs/unreleased/12136-create-todos-for-dropped-merge-trains-V2.yml

+---
+title: Create TODOs for merge train errors
+merge_request: 35951
+author:
+type: changed

ee/spec/helpers/ee/todos_helper_spec.rb

@@ -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

ee/spec/services/ee/system_notes/merge_train_service_spec.rb

@@ -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

ee/spec/services/todo_service_spec.rb

@@ -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 = {})

spec/helpers/todos_helper_spec.rb

@@ -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