From 19c63911340f81f3f94659f702abc5d2f1527870 Mon Sep 17 00:00:00 2001 From: lulalala Date: Tue, 17 Apr 2018 11:13:40 +0800 Subject: [PATCH 1/9] Update doc redirect link --- doc/workflow/merge_requests.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/workflow/merge_requests.md b/doc/workflow/merge_requests.md index a68bb8b27ca..dc6da1938f3 100644 --- a/doc/workflow/merge_requests.md +++ b/doc/workflow/merge_requests.md @@ -1 +1 @@ -This document was moved to [user/project/merge_requests](../user/project/merge_requests.md). +This document was moved to [user/project/merge_requests/index.md](../user/project/merge_requests/index.md). From bf669717a86e9bbe87ba1f97d4433b79081d2b97 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 28 Mar 2018 10:49:13 +0800 Subject: [PATCH 2/9] Update doc --- app/services/notification_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 55a1735e54b..8b3ddab5e19 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -129,6 +129,7 @@ class NotificationService # When create a merge request we should send an email to: # + # * mr author # * mr assignee if their notification level is not Disabled # * project team members with notification level higher then Participating # * watchers of the mr's labels From 7517105303c264484d8677c81268f9f43ecc5593 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 28 Mar 2018 11:20:12 +0800 Subject: [PATCH 3/9] Add cannot_be_merged_recheck merge_status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First, transitions between can_be_merged & cannot_be_merged are removed, as they are currently blocked in `check_if_can_be_merged`. `can_be_merge` always returns to `unchecked` first, before it can transition to `cannot_be_merged` (and vice versa). We want to avoid repeated notification triggered by repeated transition between `cannot_be_merged` & `unchecked`. So we added `cannot_be_merged_recheck` state, similar to `unchecked`, but as a mean to remember it’s from cannot_be_merged. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18042/#note_65945407 Since `unchecked` and `cannot_be_merged_recheck` both mean “we are in the middle of checking if it is mergeable”, quite often we need to see if merge_status is in either one of them, so `check_state?` is added to achieve this. --- app/models/merge_request.rb | 14 ++++-- .../project_services/drone_ci_service.rb | 2 +- .../conflicts_controller_spec.rb | 2 +- .../merge_requests_controller_spec.rb | 2 +- .../user_resolves_conflicts_spec.rb | 2 +- spec/models/merge_request_spec.rb | 47 +++++++++++++++++++ .../conflicts/list_service_spec.rb | 2 +- 7 files changed, 62 insertions(+), 9 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 628c61d5d69..ea5057c2a78 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -104,24 +104,30 @@ class MergeRequest < ActiveRecord::Base state_machine :merge_status, initial: :unchecked do event :mark_as_unchecked do - transition [:can_be_merged, :cannot_be_merged] => :unchecked + transition [:can_be_merged] => :unchecked + transition [:cannot_be_merged] => :cannot_be_merged_recheck end event :mark_as_mergeable do - transition [:unchecked, :cannot_be_merged] => :can_be_merged + transition [:unchecked, :cannot_be_merged_recheck] => :can_be_merged end event :mark_as_unmergeable do - transition [:unchecked, :can_be_merged] => :cannot_be_merged + transition [:unchecked, :cannot_be_merged_recheck] => :cannot_be_merged end state :unchecked + state :cannot_be_merged_recheck state :can_be_merged state :cannot_be_merged around_transition do |merge_request, transition, block| Gitlab::Timeless.timeless(merge_request, &block) end + + def check_state?(merge_status) + [:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym) + end end validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?] @@ -602,7 +608,7 @@ class MergeRequest < ActiveRecord::Base end def check_if_can_be_merged - return unless unchecked? && Gitlab::Database.read_write? + return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write? can_be_merged = !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch) diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb index 71b10fc6bc1..a4bf427ac0b 100644 --- a/app/models/project_services/drone_ci_service.rb +++ b/app/models/project_services/drone_ci_service.rb @@ -115,6 +115,6 @@ class DroneCiService < CiService def merge_request_valid?(data) data[:object_attributes][:state] == 'opened' && - data[:object_attributes][:merge_status] == 'unchecked' + MergeRequest.state_machines[:merge_status].check_state?(data[:object_attributes][:merge_status]) end end diff --git a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb index 2d7647a6e12..397cc79bde4 100644 --- a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb @@ -5,7 +5,7 @@ describe Projects::MergeRequests::ConflictsController do let(:user) { project.owner } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request_with_conflicts) do - create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index c8cc6b374f6..d3042be9e8b 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -7,7 +7,7 @@ describe Projects::MergeRequestsController do let(:user) { project.owner } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request_with_conflicts) do - create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/features/merge_request/user_resolves_conflicts_spec.rb b/spec/features/merge_request/user_resolves_conflicts_spec.rb index 19995559fae..ac3c734599c 100644 --- a/spec/features/merge_request/user_resolves_conflicts_spec.rb +++ b/spec/features/merge_request/user_resolves_conflicts_spec.rb @@ -10,7 +10,7 @@ describe 'Merge request > User resolves conflicts', :js do end def create_merge_request(source_branch) - create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 04379e7d2c3..bcc836acefa 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2064,6 +2064,53 @@ describe MergeRequest do expect(subject.merge_jid).to be_nil end end + + describe 'transition to cannot_be_merged' do + let(:notification_service) { double(:notification_service) } + let(:todo_service) { double(:todo_service) } + + subject { create(:merge_request, merge_status: :unchecked) } + + before do + allow(NotificationService).to receive(:new).and_return(notification_service) + allow(TodoService).to receive(:new).and_return(todo_service) + end + + it 'notifies, but does not notify again if rechecking still results in cannot_be_merged' do + expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once + expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_unmergeable + end + + it 'notifies whenever merge request is newly unmergeable' do + expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice + expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_mergeable + subject.mark_as_unchecked + subject.mark_as_unmergeable + end + end + + describe 'check_state?' do + it 'indicates whether MR is still checking for mergeability' do + state_machine = described_class.state_machines[:merge_status] + check_states = [:unchecked, :cannot_be_merged_recheck] + + check_states.each do |merge_status| + expect(state_machine.check_state?(merge_status)).to be true + end + + (state_machine.states.map(&:name) - check_states).each do |merge_status| + expect(state_machine.check_state?(merge_status)).to be false + end + end + end end describe '#should_be_rebased?' do diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 837b8a56d12..d57852615d9 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe MergeRequests::Conflicts::ListService do describe '#can_be_resolved_in_ui?' do def create_merge_request(source_branch) - create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr| + create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end From 179a1ee7c669e758f8dc9fb5e9c8f2563012a6e1 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 9 May 2018 17:43:47 +0800 Subject: [PATCH 4/9] Add MergeRequest#merge_participants For notifying via todo or email. --- app/models/merge_request.rb | 10 +++++++++ spec/models/merge_request_spec.rb | 35 +++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ea5057c2a78..dd4daf3711b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -333,6 +333,16 @@ class MergeRequest < ActiveRecord::Base update_column(:merge_jid, jid) end + def merge_participants + participants = [author] + + if merge_when_pipeline_succeeds? && !participants.include?(merge_user) + participants << merge_user + end + + participants + end + def first_commit merge_request_diff ? merge_request_diff.first_commit : compare_commits.first end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index bcc836acefa..d1cc57fd103 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2242,4 +2242,39 @@ describe MergeRequest do expect(merge_request.can_allow_maintainer_to_push?(user)).to be_truthy end end + + describe '#merge_participants' do + it 'contains author' do + expect(subject.merge_participants).to eq([subject.author]) + end + + describe 'when merge_when_pipeline_succeeds? is true' do + describe 'when merge user is author' do + let(:user) { create(:user) } + subject do + create(:merge_request, + merge_when_pipeline_succeeds: true, + merge_user: user, + author: user) + end + + it 'contains author only' do + expect(subject.merge_participants).to eq([subject.author]) + end + end + + describe 'when merge user and author are different users' do + let(:merge_user) { create(:user) } + subject do + create(:merge_request, + merge_when_pipeline_succeeds: true, + merge_user: merge_user) + end + + it 'contains author and merge user' do + expect(subject.merge_participants).to eq([subject.author, merge_user]) + end + end + end + end end From dc174e9655267e89e1b7c63f8c9f4dac069069c7 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 9 May 2018 17:49:33 +0800 Subject: [PATCH 5/9] Notify with email when merge request became unmergeable Display MR unmergeable reasons --- app/mailers/emails/merge_requests.rb | 8 ++++ app/models/merge_request.rb | 4 ++ app/presenters/merge_request_presenter.rb | 11 ++++++ .../notification_recipient_service.rb | 25 ++++++++++++ app/services/notification_service.rb | 17 ++++++++ .../merge_request_unmergeable_email.html.haml | 6 +++ .../merge_request_unmergeable_email.text.haml | 11 ++++++ .../unreleased/mr-conflict-notification.yml | 5 +++ doc/workflow/notifications.md | 4 ++ spec/mailers/notify_spec.rb | 39 +++++++++++++++++++ .../merge_request_presenter_spec.rb | 35 +++++++++++++++++ spec/services/notification_service_spec.rb | 26 +++++++++++++ 12 files changed, 191 insertions(+) create mode 100644 app/views/notify/merge_request_unmergeable_email.html.haml create mode 100644 app/views/notify/merge_request_unmergeable_email.text.haml create mode 100644 changelogs/unreleased/mr-conflict-notification.yml diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index b3f2aeb08ca..c2333180663 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -56,6 +56,14 @@ module Emails mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end + def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil) + setup_merge_request_mail(merge_request_id, recipient_id) + + @merge_request_presenter = MergeRequestPresenter.new(@merge_request, current_user: current_user) + + mail_answer_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason)) + end + def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index dd4daf3711b..99687d305e7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -125,6 +125,10 @@ class MergeRequest < ActiveRecord::Base Gitlab::Timeless.timeless(merge_request, &block) end + after_transition unchecked: :cannot_be_merged do |merge_request, transition| + NotificationService.new.merge_request_unmergeable(merge_request) + end + def check_state?(merge_status) [:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym) end diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 4b4132af2d0..ad839d9840a 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -20,6 +20,17 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end end + def unmergeable_reasons + strong_memoize(:unmergeable_reasons) do + reasons = [] + reasons << "no commits" if merge_request.has_no_commits? + reasons << "source branch is missing" unless merge_request.source_branch_exists? + reasons << "target branch is missing" unless merge_request.target_branch_exists? + reasons << "has merge conflicts" unless merge_request.project.repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch) + reasons + end + end + def cancel_merge_when_pipeline_succeeds_path if can_cancel_merge_when_pipeline_succeeds?(current_user) cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 5658699664d..4fa38665abc 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -18,6 +18,10 @@ module NotificationRecipientService Builder::NewNote.new(*a).notification_recipients end + def self.build_merge_request_unmergeable_recipients(*a) + Builder::MergeRequestUnmergeable.new(*a).notification_recipients + end + module Builder class Base def initialize(*) @@ -330,5 +334,26 @@ module NotificationRecipientService note.author end end + + class MergeRequestUnmergeable < Base + attr_reader :target + def initialize(merge_request) + @target = merge_request + end + + def build! + target.merge_participants.each do |user| + add_recipients(user, :participating, nil) + end + end + + def custom_action + :unmergeable_merge_request + end + + def acting_user + nil + end + end end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 8b3ddab5e19..636cfbf5b45 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -149,6 +149,15 @@ class NotificationService end end + # When a merge request is found to be unmergeable, we should send an email to: + # + # * mr author + # * mr merge user if set + # + def merge_request_unmergeable(merge_request) + merge_request_unmergeable_email(merge_request) + end + # When merge request text is updated, we should send an email to: # # * newly mentioned project team members with notification level higher than Participating @@ -485,6 +494,14 @@ class NotificationService end end + def merge_request_unmergeable_email(merge_request) + recipients = NotificationRecipientService.build_merge_request_unmergeable_recipients(merge_request) + + recipients.each do |recipient| + mailer.merge_request_unmergeable_email(recipient.user.id, merge_request.id).deliver_later + end + end + def mailer Notify end diff --git a/app/views/notify/merge_request_unmergeable_email.html.haml b/app/views/notify/merge_request_unmergeable_email.html.haml new file mode 100644 index 00000000000..f51a31c0eda --- /dev/null +++ b/app/views/notify/merge_request_unmergeable_email.html.haml @@ -0,0 +1,6 @@ +%p + Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to the following reason(s): + + %ul + - @merge_request_presenter.unmergeable_reasons.each do |reason| + %li= reason diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml new file mode 100644 index 00000000000..2acf0de34fc --- /dev/null +++ b/app/views/notify/merge_request_unmergeable_email.text.haml @@ -0,0 +1,11 @@ +Merge Request #{@merge_request.to_reference} can no longer be merged due to the following reason(s): + +- @merge_request_presenter.unmergeable_reasons.each do |reason| + * #{reason} + +Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} + += merge_path_description(@merge_request, 'to') + +Author: #{@merge_request.author_name} +Assignee: #{@merge_request.assignee_name} diff --git a/changelogs/unreleased/mr-conflict-notification.yml b/changelogs/unreleased/mr-conflict-notification.yml new file mode 100644 index 00000000000..d3d5f1fc373 --- /dev/null +++ b/changelogs/unreleased/mr-conflict-notification.yml @@ -0,0 +1,5 @@ +--- +title: When MR becomes unmergeable, notify and create todo for author and merge user +merge_request: 18042 +author: +type: added diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index f1501c81b27..fc592b99860 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -106,6 +106,10 @@ by yourself (except when an issue is due). You will only receive automatic notifications when somebody else comments or adds changes to the ones that you've created or mentions you. +If a merge request becomes unmergeable, its author will be notified about the cause. +If a user has also set the merge request to automatically merge once pipeline succeeds, +then that user will also be notified. + ### Email Headers Notification emails include headers that provide extra content about the notification received: diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 84ddbbbf2ee..bed7675db4f 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -390,6 +390,45 @@ describe Notify do end end + describe 'that are unmergeable' do + set(:merge_request) do + create(:merge_request, :conflict, + source_project: project, + target_project: project, + author: current_user, + assignee: assignee, + description: 'Awesome description') + end + + subject { described_class.merge_request_unmergeable_email(recipient.id, merge_request.id) } + + it_behaves_like 'a multiple recipients email' + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { merge_request } + end + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' + + it 'is sent as the merge request author' do + sender = subject.header[:from].addrs[0] + expect(sender.display_name).to eq(merge_request.author.name) + expect(sender.address).to eq(gitlab_sender) + end + + it 'has the correct subject and body' do + reasons = %w[foo bar] + + allow_any_instance_of(MergeRequestPresenter).to receive(:unmergeable_reasons).and_return(reasons) + aggregate_failures do + is_expected.to have_referable_subject(merge_request, reply: true) + is_expected.to have_body_text(project_merge_request_path(project, merge_request)) + reasons.each do |reason| + is_expected.to have_body_text(reason) + end + end + end + end + shared_examples 'a push to an existing merge request' do let(:push_user) { create(:user) } diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index e3b37739e8e..d5fb4a7742c 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -70,6 +70,41 @@ describe MergeRequestPresenter do end end + describe "#unmergeable_reasons" do + let(:presenter) { described_class.new(resource, current_user: user) } + + before do + # Mergeable base state + allow(resource).to receive(:has_no_commits?).and_return(false) + allow(resource).to receive(:source_branch_exists?).and_return(true) + allow(resource).to receive(:target_branch_exists?).and_return(true) + allow(resource.project.repository).to receive(:can_be_merged?).and_return(true) + end + + it "handles mergeable request" do + expect(presenter.unmergeable_reasons).to eq([]) + end + + it "handles no commit" do + allow(resource).to receive(:has_no_commits?).and_return(true) + + expect(presenter.unmergeable_reasons).to eq(["no commits"]) + end + + it "handles branches missing" do + allow(resource).to receive(:source_branch_exists?).and_return(false) + allow(resource).to receive(:target_branch_exists?).and_return(false) + + expect(presenter.unmergeable_reasons).to eq(["source branch is missing", "target branch is missing"]) + end + + it "handles merge conflict" do + allow(resource.project.repository).to receive(:can_be_merged?).and_return(false) + + expect(presenter.unmergeable_reasons).to eq(["has merge conflicts"]) + end + end + describe '#conflict_resolution_path' do let(:project) { create :project } let(:user) { create :user } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 5f28bc123f3..831678b949d 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1224,6 +1224,32 @@ describe NotificationService, :mailer do end end + describe '#merge_request_unmergeable' do + it "sends email to merge request author" do + notification.merge_request_unmergeable(merge_request) + + should_email(merge_request.author) + expect(email_recipients.size).to eq(1) + end + + describe 'when merge_when_pipeline_succeeds is true' do + before do + merge_request.update_attributes( + merge_when_pipeline_succeeds: true, + merge_user: create(:user) + ) + end + + it "sends email to merge request author and merge_user" do + notification.merge_request_unmergeable(merge_request) + + should_email(merge_request.author) + should_email(merge_request.merge_user) + expect(email_recipients.size).to eq(2) + end + end + end + describe '#closed_merge_request' do before do update_custom_notification(:close_merge_request, @u_guest_custom, resource: project) From 953bb41f25270c07ab12c17472ef0fe8ab848301 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 9 May 2018 17:55:00 +0800 Subject: [PATCH 6/9] Create TODO when MR became unmergeable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old behavior of creating TODO when “Merge When Pipeline Succeeds” service fails, is generalized to: Create a TODO whenever MR became unmergeable (and similar to notification, MR author and merge_user are both applicable) --- app/models/merge_request.rb | 1 + .../merge_when_pipeline_succeeds_service.rb | 6 +--- app/services/todo_service.rb | 30 ++++++++++--------- doc/workflow/todos.md | 3 ++ ...rge_when_pipeline_succeeds_service_spec.rb | 26 ---------------- spec/services/todo_service_spec.rb | 25 +++++++++++----- 6 files changed, 38 insertions(+), 53 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 99687d305e7..a0ad7f3c609 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -127,6 +127,7 @@ class MergeRequest < ActiveRecord::Base after_transition unchecked: :cannot_be_merged do |merge_request, transition| NotificationService.new.merge_request_unmergeable(merge_request) + TodoService.new.merge_request_became_unmergeable(merge_request) end def check_state?(merge_status) diff --git a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb index 850deb0ac7a..9a4e6eb2e88 100644 --- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb @@ -24,11 +24,7 @@ module MergeRequests pipeline_merge_requests(pipeline) do |merge_request| next unless merge_request.merge_when_pipeline_succeeds? - - unless merge_request.mergeable? - todo_service.merge_request_became_unmergeable(merge_request) - next - end + next unless merge_request.mergeable? merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params) end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index ffd48e842c2..f91cd03bf5c 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -98,12 +98,12 @@ class TodoService # When a build fails on the HEAD of a merge request we should: # - # * create a todo for author of MR to fix it - # * create a todo for merge_user to keep an eye on it + # * create a todo for each merge participant # def merge_request_build_failed(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_pipeline_succeeds? + merge_request.merge_participants.each do |user| + create_build_failed_todo(merge_request, user) + end end # When a new commit is pushed to a merge request we should: @@ -116,20 +116,22 @@ 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 + # * mark all pending todos related to the merge request as done for each merge participant # 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_pipeline_succeeds? + merge_request.merge_participants.each do |user| + mark_pending_todos_as_done(merge_request, user) + end end - # When a merge request could not be automatically merged due to its unmergeable state we should: + # When a merge request could not be merged due to its unmergeable state we should: # - # * create a todo for a merge_user + # * create a todo for each merge participant # def merge_request_became_unmergeable(merge_request) - create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? + merge_request.merge_participants.each do |user| + create_unmergeable_todo(merge_request, user) + end end # When create a note we should: @@ -275,9 +277,9 @@ 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) + def create_unmergeable_todo(merge_request, todo_author) + attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::UNMERGEABLE) + create_todos(todo_author, attributes) end def attributes_for_target(target) diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f13d29884d4..762bf616268 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -31,6 +31,9 @@ A Todo appears in your Todos dashboard when: - you are `@mentioned` in a comment on a commit, - a job in the CI pipeline running for your merge request failed, but this job is not allowed to fail. +- a merge request becomes unmergeable, and you are either: + - the author, or + - have set it to automatically merge once pipeline succeeds. ### Directly addressed Todos diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb index 240aa638f79..8838742a637 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb @@ -112,32 +112,6 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do service.trigger(unrelated_pipeline) end end - - context 'when the merge request is not mergeable' do - let(:mr_conflict) do - create(:merge_request, merge_when_pipeline_succeeds: true, merge_user: user, - source_branch: 'master', target_branch: 'feature-conflict', - source_project: project, target_project: project) - end - - let(:conflict_pipeline) do - create(:ci_pipeline, project: project, ref: mr_conflict.source_branch, - sha: mr_conflict.diff_head_sha, status: 'success', - head_pipeline_of: mr_conflict) - end - - it 'does not merge the merge request' do - expect(MergeWorker).not_to receive(:perform_async) - - service.trigger(conflict_pipeline) - end - - it 'creates todos for unmergeability' do - expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(mr_conflict) - - service.trigger(conflict_pipeline) - end - end end describe "#cancel" do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 562b89e6767..9a51c873b30 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -721,17 +721,18 @@ describe TodoService do 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) + let(:merge_participants) { [mr_unassigned.author, admin] } - should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED) + before do + allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants) end - it 'creates a pending todo for merge_user' do - mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin) + it 'creates a pending todo for each merge_participant' do service.merge_request_build_failed(mr_unassigned) - should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED) + merge_participants.each do |participant| + should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::BUILD_FAILED) + end end end @@ -747,11 +748,19 @@ describe TodoService do end describe '#merge_request_became_unmergeable' do - it 'creates a pending todo for a merge_user' do + let(:merge_participants) { [admin, create(:user)] } + + before do + allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants) + end + + it 'creates a pending todo for each merge_participant' do mr_unassigned.update(merge_when_pipeline_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) + merge_participants.each do |participant| + should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::UNMERGEABLE) + end end end From a660f548966bc8232cb56c4e344b0da3c91b3701 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 18 May 2018 09:50:01 +0800 Subject: [PATCH 7/9] Plural reason(s) --- app/mailers/emails/merge_requests.rb | 2 +- app/views/notify/merge_request_unmergeable_email.html.haml | 4 ++-- app/views/notify/merge_request_unmergeable_email.text.haml | 4 ++-- spec/mailers/notify_spec.rb | 1 + 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index c2333180663..5ba3a4a322c 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -59,7 +59,7 @@ module Emails def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) - @merge_request_presenter = MergeRequestPresenter.new(@merge_request, current_user: current_user) + @reasons = MergeRequestPresenter.new(@merge_request, current_user: current_user).unmergeable_reasons mail_answer_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason)) end diff --git a/app/views/notify/merge_request_unmergeable_email.html.haml b/app/views/notify/merge_request_unmergeable_email.html.haml index f51a31c0eda..e84905a5fad 100644 --- a/app/views/notify/merge_request_unmergeable_email.html.haml +++ b/app/views/notify/merge_request_unmergeable_email.html.haml @@ -1,6 +1,6 @@ %p - Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to the following reason(s): + Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to the following #{pluralize(@reasons, 'reason')}: %ul - - @merge_request_presenter.unmergeable_reasons.each do |reason| + - @reasons.each do |reason| %li= reason diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml index 2acf0de34fc..4f0901c761a 100644 --- a/app/views/notify/merge_request_unmergeable_email.text.haml +++ b/app/views/notify/merge_request_unmergeable_email.text.haml @@ -1,6 +1,6 @@ -Merge Request #{@merge_request.to_reference} can no longer be merged due to the following reason(s): +Merge Request #{@merge_request.to_reference} can no longer be merged due to the following #{pluralize(@reasons, 'reason')}: -- @merge_request_presenter.unmergeable_reasons.each do |reason| +- @reasons.each do |reason| * #{reason} Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index bed7675db4f..e9b3ccf31fb 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -422,6 +422,7 @@ describe Notify do aggregate_failures do is_expected.to have_referable_subject(merge_request, reply: true) is_expected.to have_body_text(project_merge_request_path(project, merge_request)) + is_expected.to have_body_text('reasons:') reasons.each do |reason| is_expected.to have_body_text(reason) end From b1ab1609b66f6cb47bb7d7b3babdd29cd0aae3c4 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 18 May 2018 17:02:35 +0800 Subject: [PATCH 8/9] Avoid race condition of re-triggering mark_as_unchecked --- app/models/merge_request.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a0ad7f3c609..9c4384a6e42 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -104,8 +104,8 @@ class MergeRequest < ActiveRecord::Base state_machine :merge_status, initial: :unchecked do event :mark_as_unchecked do - transition [:can_be_merged] => :unchecked - transition [:cannot_be_merged] => :cannot_be_merged_recheck + transition [:can_be_merged, :unchecked] => :unchecked + transition [:cannot_be_merged, :cannot_be_merged_recheck] => :cannot_be_merged_recheck end event :mark_as_mergeable do From 13aa6f6740118743e47f24f01aa54fed30079f4d Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Thu, 24 May 2018 09:51:25 +0800 Subject: [PATCH 9/9] Add spec --- spec/models/merge_request_spec.rb | 56 +++++++++++++++++++------------ 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index d1cc57fd103..92e33a64d26 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1245,38 +1245,50 @@ describe MergeRequest do describe '#check_if_can_be_merged' do let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } - subject { create(:merge_request, source_project: project, merge_status: :unchecked) } + shared_examples 'checking if can be merged' do + context 'when it is not broken and has no conflicts' do + before do + allow(subject).to receive(:broken?) { false } + allow(project.repository).to receive(:can_be_merged?).and_return(true) + end - context 'when it is not broken and has no conflicts' do - before do - allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?).and_return(true) + it 'is marked as mergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') + end end - it 'is marked as mergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') + context 'when broken' do + before do + allow(subject).to receive(:broken?) { true } + end + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end + end + + context 'when it has conflicts' do + before do + allow(subject).to receive(:broken?) { false } + allow(project.repository).to receive(:can_be_merged?).and_return(false) + end + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end end end - context 'when broken' do - before do - allow(subject).to receive(:broken?) { true } - end + context 'when merge_status is unchecked' do + subject { create(:merge_request, source_project: project, merge_status: :unchecked) } - it 'becomes unmergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') - end + it_behaves_like 'checking if can be merged' end - context 'when it has conflicts' do - before do - allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?).and_return(false) - end + context 'when merge_status is unchecked' do + subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) } - it 'becomes unmergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') - end + it_behaves_like 'checking if can be merged' end end