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

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

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

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