summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/finders/todos_finder.rb2
-rw-r--r--app/helpers/todos_helper.rb8
-rw-r--r--app/models/ci/build.rb1
-rw-r--r--app/models/commit_status.rb4
-rw-r--r--app/models/todo.rb9
-rw-r--r--app/services/merge_requests/add_todo_when_build_fails_service.rb17
-rw-r--r--app/services/merge_requests/base_service.rb25
-rw-r--r--app/services/merge_requests/merge_when_build_succeeds_service.rb23
-rw-r--r--app/services/merge_requests/refresh_service.rb7
-rw-r--r--app/services/todo_service.rb30
-rw-r--r--app/views/dashboard/todos/_todo.html.haml12
-rw-r--r--spec/factories/todos.rb4
-rw-r--r--spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb81
-rw-r--r--spec/services/merge_requests/refresh_service_spec.rb28
-rw-r--r--spec/services/todo_service_spec.rb19
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 cacbc13b391..8181416f7d5 100644
--- a/app/models/commit_status.rb
+++ b/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
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 = {})
View Lorry Controller Status