Merge branch '23524-notify-automerge-user-of-failed-build' into 'master'

Notify the user who set auto-merge when merge is not possible

Closes #23524

See merge request !8056

app/helpers/todos_helper.rb

@@ -11,9 +11,10 @@ module TodosHelper
     case todo.action
     when Todo::ASSIGNED then 'assigned you'
     when Todo::MENTIONED then 'mentioned you on'
-    when Todo::BUILD_FAILED then 'The build failed for your'
+    when Todo::BUILD_FAILED then 'The build failed for'
     when Todo::MARKED then 'added a todo for'
     when Todo::APPROVAL_REQUIRED then 'set you as an approver for'
+    when Todo::UNMERGEABLE then 'Could not merge'
     end
   end
 

app/models/merge_request.rb

@@ -91,6 +91,10 @@ 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?]

app/models/todo.rb

@@ -6,13 +6,15 @@ class Todo < ActiveRecord::Base
   BUILD_FAILED      = 3
   MARKED            = 4
   APPROVAL_REQUIRED = 5 # This is an EE-only feature
+  UNMERGEABLE       = 6
 
   ACTION_NAMES = {
     ASSIGNED => :assigned,
     MENTIONED => :mentioned,
     BUILD_FAILED => :build_failed,
     MARKED => :marked,
-    APPROVAL_REQUIRED => :approval_required
+    APPROVAL_REQUIRED => :approval_required,
+    UNMERGEABLE => :unmergeable
   }
 
   belongs_to :author, class_name: "User"
@@ -66,6 +68,10 @@ class Todo < ActiveRecord::Base
     end
   end
 
+  def unmergeable?
+    action == UNMERGEABLE
+  end
+
   def build_failed?
     action == BUILD_FAILED
   end

app/services/todo_service.rb

@@ -98,10 +98,12 @@ class TodoService
 
   # When a build fails on the HEAD of a merge request we should:
   #
-  #  * create a todo for that user to fix it
+  #  * create a todo for author of MR to fix it
+  #  * create a todo for merge_user to keep an eye on it
   #
   def merge_request_build_failed(merge_request)
-    create_build_failed_todo(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_build_succeeds?
   end
 
   # When a new commit is pushed to a merge request we should:
@@ -115,11 +117,21 @@ 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
   #
   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_build_succeeds?
   end
-
+  
+  # When a merge request could not be automatically merged due to its unmergeable state we should:
+  #
+  #  * create a todo for a merge_user
+  #
+  def merge_request_became_unmergeable(merge_request)
+    create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds?
+  end
+  
   # When create a note we should:
   #
   #  * mark all pending todos related to the noteable for the note author as done
@@ -236,10 +248,14 @@ class TodoService
     create_todos(mentioned_users, attributes)
   end
 
-  def create_build_failed_todo(merge_request)
-    author = merge_request.author
-    attributes = attributes_for_todo(merge_request.project, merge_request, author, Todo::BUILD_FAILED)
-    create_todos(author, attributes)
+  def create_build_failed_todo(merge_request, todo_author)
+    attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::BUILD_FAILED)
+    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)
   end
 
   def attributes_for_target(target)

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

@@ -3,7 +3,7 @@
 
   .todo-item.todo-block
     .todo-title.title
-      - unless todo.build_failed?
+      - unless todo.build_failed? || todo.unmergeable?
         = todo_target_state_pill(todo)
 
         %span.author-name

changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml

+---
+title: Create a TODO for user who set auto-merge when a build fails, merge conflict occurs
+merge_request: 8056
+author: twonegatives

spec/factories/todos.rb

@@ -28,6 +28,10 @@ FactoryGirl.define do
       action { Todo::APPROVAL_REQUIRED }
     end
 
+    trait :unmergeable do
+      action { Todo::UNMERGEABLE }
+    end
+
     trait :done do
       state :done
     end

spec/models/merge_request_spec.rb

@@ -773,10 +773,12 @@ describe MergeRequest, models: true do
     subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
 
     context 'when it is not broken and has no conflicts' do
-      it 'is marked as mergeable' do
+      before do
         allow(subject).to receive(:broken?) { false }
         allow(project.repository).to receive(:can_be_merged?).and_return(true)
+      end
 
+      it 'is marked as mergeable' do
         expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
       end
     end
@@ -787,6 +789,12 @@ 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

spec/services/todo_service_spec.rb

@@ -469,6 +469,13 @@ describe TodoService, services: true do
 
         should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED)
       end
+
+      it 'creates a pending todo for merge_user' do
+        mr_unassigned.update(merge_when_build_succeeds: true, merge_user: admin)
+        service.merge_request_build_failed(mr_unassigned)
+
+        should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED)
+      end
     end
 
     describe '#merge_request_push' do
@@ -482,6 +489,15 @@ describe TodoService, services: true do
       end
     end
 
+    describe '#merge_request_became_unmergeable' do
+      it 'creates a pending todo for a merge_user' do
+        mr_unassigned.update(merge_when_build_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)
+      end
+    end
+    
     describe '#mark_todo' do
       it 'creates a todo from a merge request' do
         service.mark_todo(mr_unassigned, author)