Merge branch 'create-todo-on-failing-build' into 'master'

Create a todo on failing MR build

Implements #14067. I worked on this with @DouweM (any mistakes are mine).

When a build fails for a commit, create a todo for the author of the merge request that commit is the HEAD of. If the commit isn't the HEAD commit of any MR, don't do anything. If there already is a todo for that user and MR, don't do anything.

Current limitations:
- This isn't configurable by project.
- The author of a merge request might not be the person who pushed the breaking commit.
- I haven't tested this with a working CI setup, just with the unit tests below and by modifying my DB directly.


See merge request !3177

15 files changed, 237 insertions, 33 deletions

diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 3ba27c40504..4bd46a76087 100644
--- a/app/finders/todos_finder.rb
+++ b/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
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 2f066682180..81b9b5d7052 100644
--- a/app/helpers/todos_helper.rb
+++ b/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
 
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 92327bdb08d..50190101f2b 100644
--- a/app/models/ci/build.rb
+++ b/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
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 1548a51e942..f774b6e0efb 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -46,6 +46,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
diff --git a/app/models/todo.rb b/app/models/todo.rb
index f8b59fe4126..3a091373329 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -1,6 +1,7 @@
 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
diff --git a/app/services/merge_requests/add_todo_when_build_fails_service.rb b/app/services/merge_requests/add_todo_when_build_fails_service.rb
new file mode 100644
index 00000000000..566049525cb
--- /dev/null
+++ b/app/services/merge_requests/add_todo_when_build_fails_service.rb
@@ -0,0 +1,17 @@
+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
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index e6837a18696..9d7fca6882d 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/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
diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb
index d6af12f9739..8fd6a4ea1f6 100644
--- a/app/services/merge_requests/merge_when_build_succeeds_service.rb
+++ b/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
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 8b3d56c2b4c..fe0579744b4 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/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 = []
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 42c5bca90fd..4bf4e144727 100644
--- a/app/services/todo_service.rb
+++ b/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,
diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml
index aa0aff86d4d..539f1dc6036 100644
--- a/app/views/dashboard/todos/_todo.html.haml
+++ b/app/views/dashboard/todos/_todo.html.haml
@@ -1,13 +1,13 @@
 %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
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index e3681ae93a5..f426e27afed 100644
--- a/spec/factories/todos.rb
+++ b/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
diff --git a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb
new file mode 100644
index 00000000000..f70716c9d19
--- /dev/null
+++ b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb
@@ -0,0 +1,81 @@
+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
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index fea8182bd30..31b93850c7c 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/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
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index a075496ee63..42147736532 100644
--- a/spec/services/todo_service_spec.rb
+++ b/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 = {})