From f14228f0f2f69a967c483e4aa4ef1568e5fdc49b Mon Sep 17 00:00:00 2001 From: twonegatives Date: Tue, 13 Dec 2016 00:55:33 +0300 Subject: Notify the user who set auto-merge when a build fails --- app/helpers/todos_helper.rb | 2 +- app/services/todo_service.rb | 15 +++++++++------ .../23524-notify-automerge-user-of-failed-build.yml | 4 ++++ spec/services/todo_service_spec.rb | 7 +++++++ 4 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 09c69786791..3c039b43f5d 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -11,7 +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' + when Todo::BUILD_FAILED then 'The build failed for' when Todo::MARKED then 'added a todo for' when Todo::APPROVAL_REQUIRED then 'set you as an approver for' end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f8e6b2ef094..d67c166275a 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -98,10 +98,12 @@ class TodoService # When a build fails on the HEAD of a merge request we should: # - # * create a todo for that user to fix it + # * create a todo for author of MR to fix it + # * create a todo for merge_user to keep an eye on it # def merge_request_build_failed(merge_request) - create_build_failed_todo(merge_request) + create_build_failed_todo(merge_request, merge_request.author) + create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end # When a new commit is pushed to a merge request we should: @@ -115,9 +117,11 @@ class TodoService # 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 + # * mark all pending todos related to the merge request for the merge_user as done # def merge_request_build_retried(merge_request) mark_pending_todos_as_done(merge_request, merge_request.author) + mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end # When create a note we should: @@ -236,10 +240,9 @@ 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) + def create_build_failed_todo(merge_request, todo_author) + attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::BUILD_FAILED) + create_todos(todo_author, attributes) end def attributes_for_target(target) diff --git a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml new file mode 100644 index 00000000000..4b6f9d1efe9 --- /dev/null +++ b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml @@ -0,0 +1,4 @@ +--- +title: Create a TODO for user who set auto-merge when a build fails +merge_request: +author: diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index ed55791d24e..d8a9ca20b36 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -469,6 +469,13 @@ describe TodoService, services: true do should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED) end + + it 'creates a pending todo for merge_user' do + mr_unassigned.update(merge_when_build_succeeds: true, merge_user: admin) + service.merge_request_build_failed(mr_unassigned) + + should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED) + end end describe '#merge_request_push' do -- cgit v1.2.1 From 85e0b99471b58078e1e50494ae26eb13430d3a9f Mon Sep 17 00:00:00 2001 From: twonegatives Date: Wed, 14 Dec 2016 17:37:31 +0300 Subject: Notify the user who set auto-merge when merge conflict occurs --- app/helpers/todos_helper.rb | 1 + app/models/merge_request.rb | 4 ++++ app/models/todo.rb | 8 +++++++- app/services/todo_service.rb | 15 ++++++++++++++- app/views/dashboard/todos/_todo.html.haml | 2 +- .../23524-notify-automerge-user-of-failed-build.yml | 6 +++--- spec/factories/todos.rb | 4 ++++ spec/models/merge_request_spec.rb | 10 +++++++++- spec/services/todo_service_spec.rb | 9 +++++++++ 9 files changed, 52 insertions(+), 7 deletions(-) diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 3c039b43f5d..56c05cb54e6 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -14,6 +14,7 @@ module TodosHelper when Todo::BUILD_FAILED then 'The build failed for' when Todo::MARKED then 'added a todo for' when Todo::APPROVAL_REQUIRED then 'set you as an approver for' + when Todo::UNMERGEABLE then 'Could not merge' end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b73d7acefea..22a3a5fe42e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -91,6 +91,10 @@ class MergeRequest < ActiveRecord::Base around_transition do |merge_request, transition, block| Gitlab::Timeless.timeless(merge_request, &block) end + + after_transition unchecked: :cannot_be_merged do |merge_request, transition| + TodoService.new.merge_request_became_unmergeable(merge_request) + end end validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?] diff --git a/app/models/todo.rb b/app/models/todo.rb index f5ade1cc293..4c99aa0d3be 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -6,13 +6,15 @@ class Todo < ActiveRecord::Base BUILD_FAILED = 3 MARKED = 4 APPROVAL_REQUIRED = 5 # This is an EE-only feature + UNMERGEABLE = 6 ACTION_NAMES = { ASSIGNED => :assigned, MENTIONED => :mentioned, BUILD_FAILED => :build_failed, MARKED => :marked, - APPROVAL_REQUIRED => :approval_required + APPROVAL_REQUIRED => :approval_required, + UNMERGEABLE => :unmergeable } belongs_to :author, class_name: "User" @@ -66,6 +68,10 @@ class Todo < ActiveRecord::Base end end + def unmergeable? + action == UNMERGEABLE + end + def build_failed? action == BUILD_FAILED end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index d67c166275a..1bd6ce416ab 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -123,7 +123,15 @@ class TodoService mark_pending_todos_as_done(merge_request, merge_request.author) mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end - + + # When a merge request could not be automatically merged due to its unmergeable state we should: + # + # * create a todo for a merge_user + # + def merge_request_became_unmergeable(merge_request) + create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? + end + # When create a note we should: # # * mark all pending todos related to the noteable for the note author as done @@ -245,6 +253,11 @@ class TodoService create_todos(todo_author, attributes) end + def create_unmergeable_todo(merge_request, merge_user) + attributes = attributes_for_todo(merge_request.project, merge_request, merge_user, Todo::UNMERGEABLE) + create_todos(merge_user, 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 cc077fad32a..7fe175c1419 100644 --- a/app/views/dashboard/todos/_todo.html.haml +++ b/app/views/dashboard/todos/_todo.html.haml @@ -3,7 +3,7 @@ .todo-item.todo-block .todo-title.title - - unless todo.build_failed? + - unless todo.build_failed? || todo.unmergeable? = todo_target_state_pill(todo) %span.author-name diff --git a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml index 4b6f9d1efe9..268be6b9b83 100644 --- a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml +++ b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml @@ -1,4 +1,4 @@ --- -title: Create a TODO for user who set auto-merge when a build fails -merge_request: -author: +title: Create a TODO for user who set auto-merge when a build fails, merge conflict occurs +merge_request: 8056 +author: twonegatives diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 866e663f026..dc247e2d96d 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -27,6 +27,10 @@ FactoryGirl.define do action { Todo::APPROVAL_REQUIRED } end + trait :unmergeable do + action { Todo::UNMERGEABLE } + end + trait :done do state :done end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1b71d00eb8f..59e629edf4c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -740,10 +740,12 @@ describe MergeRequest, models: true do subject { create(:merge_request, source_project: project, merge_status: :unchecked) } context 'when it is not broken and has no conflicts' do - it 'is marked as mergeable' do + before do allow(subject).to receive(:broken?) { false } allow(project.repository).to receive(:can_be_merged?).and_return(true) + end + it 'is marked as mergeable' do expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') end end @@ -754,6 +756,12 @@ describe MergeRequest, models: true do it 'becomes unmergeable' do expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') end + + it 'creates Todo on unmergeability' do + expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(subject) + + subject.check_if_can_be_merged + end end context 'when it has conflicts' do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index d8a9ca20b36..13d584a8975 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -489,6 +489,15 @@ describe TodoService, services: true do end end + describe '#merge_request_became_unmergeable' do + it 'creates a pending todo for a merge_user' do + mr_unassigned.update(merge_when_build_succeeds: true, merge_user: admin) + service.merge_request_became_unmergeable(mr_unassigned) + + should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::UNMERGEABLE) + end + end + describe '#mark_todo' do it 'creates a todo from a merge request' do service.mark_todo(mr_unassigned, author) -- cgit v1.2.1