diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-05-20 00:18:22 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-05-20 00:18:22 +0300 |
commit | 3f6f2bbe14a7b6da59f87918c84617b8a3e4d1f1 (patch) | |
tree | a90fad419c99aa718466832b891b4727d2a20952 | |
parent | 18ef054bc7eb14a7b817af5f66f140f2f3dd4ae6 (diff) | |
parent | 6b834f2cbcfc26fe3123b6682ed7e20618e31d1b (diff) |
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
-rw-r--r-- | app/finders/todos_finder.rb | 2 | ||||
-rw-r--r-- | app/helpers/todos_helper.rb | 8 | ||||
-rw-r--r-- | app/models/ci/build.rb | 1 | ||||
-rw-r--r-- | app/models/commit_status.rb | 4 | ||||
-rw-r--r-- | app/models/todo.rb | 9 | ||||
-rw-r--r-- | app/services/merge_requests/add_todo_when_build_fails_service.rb | 17 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 25 | ||||
-rw-r--r-- | app/services/merge_requests/merge_when_build_succeeds_service.rb | 23 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 7 | ||||
-rw-r--r-- | app/services/todo_service.rb | 30 | ||||
-rw-r--r-- | app/views/dashboard/todos/_todo.html.haml | 12 | ||||
-rw-r--r-- | spec/factories/todos.rb | 4 | ||||
-rw-r--r-- | spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb | 81 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/todo_service_spec.rb | 19 |
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 = {}) |