Create a todo on failing MR build

app/finders/todos_finder.rb

@@ -36,7 +36,7 @@ class TodosFinder
   private
 
   def action_id?
-    action_id.present? && [Todo::ASSIGNED, Todo::MENTIONED].include?(action_id.to_i)
+    action_id.present? && [Todo::ASSIGNED, Todo::MENTIONED, Todo::BUILD_FAILED].include?(action_id.to_i)
   end
 
   def action_id

app/helpers/todos_helper.rb

@@ -11,6 +11,7 @@ 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'
     end
   end
 
@@ -28,8 +29,11 @@ module TodosHelper
       namespace_project_commit_path(todo.project.namespace.becomes(Namespace), todo.project,
                                     todo.target, anchor: anchor)
     else
-      polymorphic_path([todo.project.namespace.becomes(Namespace),
-                        todo.project, todo.target], anchor: anchor)
+      path = [todo.project.namespace.becomes(Namespace), todo.project, todo.target]
+
+      path.unshift(:builds) if todo.build_failed?
+
+      polymorphic_path(path, anchor: anchor)
     end
   end
 

app/models/ci/build.rb

@@ -53,6 +53,7 @@ module Ci
         new_build.stage_idx = build.stage_idx
         new_build.trigger_request = build.trigger_request
         new_build.save
+        MergeRequests::AddTodoWhenBuildFailsService.new(build.project, nil).close(new_build)
         new_build
       end
     end

app/models/commit_status.rb

@@ -45,6 +45,10 @@ class CommitStatus < ActiveRecord::Base
     after_transition [:pending, :running] => :success do |commit_status|
       MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.commit.project, nil).trigger(commit_status)
     end
+
+    after_transition any => :failed do |commit_status|
+      MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.commit.project, nil).execute(commit_status)
+    end
   end
 
   delegate :sha, :short_sha, to: :commit

app/models/todo.rb

 class Todo < ActiveRecord::Base
-  ASSIGNED  = 1
-  MENTIONED = 2
+  ASSIGNED     = 1
+  MENTIONED    = 2
+  BUILD_FAILED = 3
 
   belongs_to :author, class_name: "User"
   belongs_to :note
@@ -28,6 +29,10 @@ class Todo < ActiveRecord::Base
     state :done
   end
 
+  def build_failed?
+    action == BUILD_FAILED
+  end
+
   def body
     if note.present?
       note.note

app/services/merge_requests/add_todo_when_build_fails_service.rb

+module MergeRequests
+  class AddTodoWhenBuildFailsService < MergeRequests::BaseService
+    # Adds a todo to the parent merge_request when a CI build fails
+    def execute(commit_status)
+      each_merge_request(commit_status) do |merge_request|
+        todo_service.merge_request_build_failed(merge_request)
+      end
+    end
+
+    # Closes any pending build failed todos for the parent MRs when a build is retried
+    def close(commit_status)
+      each_merge_request(commit_status) do |merge_request|
+        todo_service.merge_request_build_retried(merge_request)
+      end
+    end
+  end
+end

app/services/merge_requests/base_service.rb

@@ -38,5 +38,30 @@ module MergeRequests
     def filter_params
       super(:merge_request)
     end
+
+    def merge_request_from(commit_status)
+      branches = commit_status.ref
+
+      # This is for ref-less builds
+      branches ||= @project.repository.branch_names_contains(commit_status.sha)
+
+      return [] if branches.blank?
+
+      merge_requests = @project.origin_merge_requests.opened.where(source_branch: branches).to_a
+      merge_requests += @project.fork_merge_requests.opened.where(source_branch: branches).to_a
+
+      merge_requests.uniq.select(&:source_project)
+    end
+
+    def each_merge_request(commit_status)
+      merge_request_from(commit_status).each do |merge_request|
+        ci_commit = merge_request.ci_commit
+
+        next unless ci_commit
+        next unless ci_commit.sha == commit_status.sha
+
+        yield merge_request, ci_commit
+      end
+    end
   end
 end

app/services/merge_requests/merge_when_build_succeeds_service.rb

@@ -20,15 +20,9 @@ module MergeRequests
 
     # Triggers the automatic merge of merge_request once the build succeeds
     def trigger(commit_status)
-      merge_requests = merge_request_from(commit_status)
-
-      merge_requests.each do |merge_request|
+      each_merge_request(commit_status) do |merge_request, ci_commit|
         next unless merge_request.merge_when_build_succeeds?
         next unless merge_request.mergeable?
-
-        ci_commit = merge_request.ci_commit
-        next unless ci_commit
-        next unless ci_commit.sha == commit_status.sha
         next unless ci_commit.success?
 
         MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
@@ -47,20 +41,5 @@ module MergeRequests
       end
     end
 
-    private
-
-    def merge_request_from(commit_status)
-      branches = commit_status.ref
-
-      # This is for ref-less builds
-      branches ||= @project.repository.branch_names_contains(commit_status.sha)
-
-      return [] if branches.blank?
-
-      merge_requests = @project.origin_merge_requests.opened.where(source_branch: branches).to_a
-      merge_requests += @project.fork_merge_requests.opened.where(source_branch: branches).to_a
-
-      merge_requests.uniq.select(&:source_project)
-    end
   end
 end

app/services/merge_requests/refresh_service.rb

@@ -12,6 +12,7 @@ module MergeRequests
       close_merge_requests
       reload_merge_requests
       reset_merge_when_build_succeeds
+      mark_pending_todos_done
 
       # Leave a system note if a branch was deleted/added
       if branch_added? || branch_removed?
@@ -80,6 +81,12 @@ module MergeRequests
       merge_requests_for_source_branch.each(&:reset_merge_when_build_succeeds)
     end
 
+    def mark_pending_todos_done
+      merge_requests_for_source_branch.each do |merge_request|
+        todo_service.merge_request_push(merge_request, @current_user)
+      end
+    end
+
     def find_new_commits
       if branch_added?
         @commits = []

app/services/todo_service.rb

@@ -80,6 +80,30 @@ class TodoService
     mark_pending_todos_as_done(merge_request, current_user)
   end
 
+  # When a build fails on the HEAD of a merge request we should:
+  #
+  #  * create a todo for that user to fix it
+  #
+  def merge_request_build_failed(merge_request)
+    create_build_failed_todo(merge_request)
+  end
+
+  # When a new commit is pushed to a merge request we should:
+  #
+  #  * mark all pending todos related to the merge request for that user as done
+  #
+  def merge_request_push(merge_request, current_user)
+    mark_pending_todos_as_done(merge_request, current_user)
+  end
+
+  # 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
+  #
+  def merge_request_build_retried(merge_request)
+    mark_pending_todos_as_done(merge_request, merge_request.author)
+  end
+
   # When create a note we should:
   #
   #  * mark all pending todos related to the noteable for the note author as done
@@ -145,6 +169,12 @@ 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)
+  end
+
   def attributes_for_target(target)
     attributes = {
       project_id: target.project.id,

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

 %li{class: "todo todo-#{todo.done? ? 'done' : 'pending'}", id: dom_id(todo), data:{url: todo_target_path(todo)} }
   .todo-item.todo-block
     = image_tag avatar_icon(todo.author_email, 40), class: 'avatar s40', alt:''
-
     .todo-title.title
-      %span.author-name
-        - if todo.author
-          = link_to_author(todo)
-        - else
-          (removed)
+      - unless todo.build_failed?
+        %span.author-name
+          - if todo.author
+            = link_to_author(todo)
+          - else
+            (removed)
       %span.todo-label
         = todo_action_name(todo)
         - if todo.target

spec/factories/todos.rb

@@ -18,5 +18,9 @@ FactoryGirl.define do
       commit_id RepoHelpers.sample_commit.id
       target_type "Commit"
     end
+
+    trait :build_failed do
+      action { Todo::BUILD_FAILED }
+    end
   end
 end

spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb

+require 'spec_helper'
+
+# Write specs in this file.
+describe MergeRequests::AddTodoWhenBuildFailsService do
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { create(:project) }
+  let(:sha) { '1234567890abcdef1234567890abcdef12345678' }
+  let(:ci_commit) { create(:ci_commit_with_one_job, ref: merge_request.source_branch, project: project, sha: sha) }
+  let(:service) { MergeRequests::AddTodoWhenBuildFailsService.new(project, user, commit_message: 'Awesome message') }
+  let(:todo_service) { TodoService.new }
+
+  let(:merge_request) do
+    create(:merge_request, merge_user: user, source_branch: 'master',
+                           target_branch: 'feature', source_project: project, target_project: project,
+                           state: 'opened')
+  end
+
+  before do
+    allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit)
+    allow(service).to receive(:todo_service).and_return(todo_service)
+  end
+
+  describe '#execute' do
+    context 'commit status with ref' do
+      let(:commit_status) { create(:generic_commit_status, ref: merge_request.source_branch, commit: ci_commit) }
+
+      it 'notifies the todo service' do
+        expect(todo_service).to receive(:merge_request_build_failed).with(merge_request)
+        service.execute(commit_status)
+      end
+    end
+
+    context 'commit status with non-HEAD ref' do
+      let(:commit_status) { create(:generic_commit_status, ref: merge_request.source_branch) }
+
+      it 'does not notify the todo service' do
+        expect(todo_service).not_to receive(:merge_request_build_failed)
+        service.execute(commit_status)
+      end
+    end
+
+    context 'commit status without ref' do
+      let(:commit_status) { create(:generic_commit_status) }
+
+      it 'does not notify the todo service' do
+        expect(todo_service).not_to receive(:merge_request_build_failed)
+        service.execute(commit_status)
+      end
+    end
+  end
+
+  describe '#close' do
+    context 'commit status with ref' do
+      let(:commit_status) { create(:generic_commit_status, ref: merge_request.source_branch, commit: ci_commit) }
+
+      it 'notifies the todo service' do
+        expect(todo_service).to receive(:merge_request_build_retried).with(merge_request)
+        service.close(commit_status)
+      end
+    end
+
+    context 'commit status with non-HEAD ref' do
+      let(:commit_status) { create(:generic_commit_status, ref: merge_request.source_branch) }
+
+      it 'does not notify the todo service' do
+        expect(todo_service).not_to receive(:merge_request_build_retried)
+        service.close(commit_status)
+      end
+    end
+
+    context 'commit status without ref' do
+      let(:commit_status) { create(:generic_commit_status) }
+
+      it 'does not notify the todo service' do
+        expect(todo_service).not_to receive(:merge_request_build_retried)
+        service.close(commit_status)
+      end
+    end
+  end
+end

spec/services/merge_requests/refresh_service_spec.rb

@@ -27,6 +27,20 @@ describe MergeRequests::RefreshService, services: true do
                                    target_branch: 'feature',
                                    target_project: @project)
 
+      @build_failed_todo = create(:todo,
+                                  :build_failed,
+                                  user: @user,
+                                  project: @project,
+                                  target: @merge_request,
+                                  author: @user)
+
+      @fork_build_failed_todo = create(:todo,
+                                       :build_failed,
+                                       user: @user,
+                                       project: @project,
+                                       target: @merge_request,
+                                       author: @user)
+
       @commits = @merge_request.commits
 
       @oldrev = @commits.last.id
@@ -51,6 +65,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request.merge_when_build_succeeds).to be_falsey}
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
+      it { expect(@build_failed_todo).to be_done }
+      it { expect(@fork_build_failed_todo).to be_done }
     end
 
     context 'push to origin repo target branch' do
@@ -63,6 +79,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_merged }
       it { expect(@fork_merge_request).to be_merged }
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
+      it { expect(@build_failed_todo).to be_pending }
+      it { expect(@fork_build_failed_todo).to be_pending }
     end
 
     context 'manual merge of source branch' do
@@ -82,6 +100,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request.diffs.size).to be > 0 }
       it { expect(@fork_merge_request).to be_merged }
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
+      it { expect(@build_failed_todo).to be_pending }
+      it { expect(@fork_build_failed_todo).to be_pending }
     end
 
     context 'push to fork repo source branch' do
@@ -101,6 +121,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_open }
       it { expect(@fork_merge_request.notes.last.note).to include('Added 4 commits') }
       it { expect(@fork_merge_request).to be_open }
+      it { expect(@build_failed_todo).to be_pending }
+      it { expect(@fork_build_failed_todo).to be_pending }
     end
 
     context 'push to fork repo target branch' do
@@ -113,6 +135,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
       it { expect(@fork_merge_request).to be_open }
+      it { expect(@build_failed_todo).to be_pending }
+      it { expect(@fork_build_failed_todo).to be_pending }
     end
 
     context 'push to origin repo target branch after fork project was removed' do
@@ -126,6 +150,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_merged }
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
+      it { expect(@build_failed_todo).to be_pending }
+      it { expect(@fork_build_failed_todo).to be_pending }
     end
 
     context 'push new branch that exists in a merge request' do
@@ -153,6 +179,8 @@ describe MergeRequests::RefreshService, services: true do
     def reload_mrs
       @merge_request.reload
       @fork_merge_request.reload
+      @build_failed_todo.reload
+      @fork_build_failed_todo.reload
     end
   end
 end

spec/services/todo_service_spec.rb

@@ -305,6 +305,25 @@ describe TodoService, services: true do
         expect(second_todo.reload).to be_done
       end
     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)
+
+        should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED)
+      end
+    end
+
+    describe '#merge_request_push' do
+      it 'marks related pending todos to the target for the user as done' do
+        first_todo = create(:todo, :build_failed, user: author, project: project, target: mr_assigned, author: john_doe)
+        second_todo = create(:todo, :build_failed, user: john_doe, project: project, target: mr_assigned, author: john_doe)
+        service.merge_request_push(mr_assigned, author)
+
+        expect(first_todo.reload).to be_done
+        expect(second_todo.reload).not_to be_done
+      end
+    end
   end
 
   def should_create_todo(attributes = {})