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

4 files changed, 132 insertions, 0 deletions

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