Commit 5b994b81 authored by Mark Chao's avatar Mark Chao

Notify only when unmergeable due to conflict

There is still the edge case when 'no commits' changes to 'conflict'
would not trigger notification, which we ignore for now.

Calling can_be_merged? can cause exception (e.g. non-UTF8)
Ignore those by rescueing.

Remove unmergeable_reason as now only conflict is notified

Update spec
parent 222284a6
...@@ -59,8 +59,6 @@ module Emails ...@@ -59,8 +59,6 @@ module Emails
def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil) def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id) setup_merge_request_mail(merge_request_id, recipient_id)
@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)) mail_answer_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason))
end end
......
...@@ -128,9 +128,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -128,9 +128,18 @@ class MergeRequest < ActiveRecord::Base
end end
after_transition unchecked: :cannot_be_merged do |merge_request, transition| after_transition unchecked: :cannot_be_merged do |merge_request, transition|
begin
# Merge request can become unmergeable due to many reasons.
# We only notify if it is due to conflict.
unless merge_request.project.repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
NotificationService.new.merge_request_unmergeable(merge_request) NotificationService.new.merge_request_unmergeable(merge_request)
TodoService.new.merge_request_became_unmergeable(merge_request) TodoService.new.merge_request_became_unmergeable(merge_request)
end end
rescue Gitlab::Git::CommandError
# Checking mergeability can trigger exception, e.g. non-utf8
# We ignore this type of errors.
end
end
def check_state?(merge_status) def check_state?(merge_status)
[:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym) [:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym)
......
...@@ -20,17 +20,6 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -20,17 +20,6 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end end
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 def cancel_merge_when_pipeline_succeeds_path
if can_cancel_merge_when_pipeline_succeeds?(current_user) if can_cancel_merge_when_pipeline_succeeds?(current_user)
cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request) cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request)
......
%p %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'.pluralize(@reasons.count)}: 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 conflict.
%ul
- @reasons.each do |reason|
%li= reason
Merge Request #{@merge_request.to_reference} can no longer be merged due to the following #{'reason'.pluralize(@reasons.count)}: Merge Request #{@merge_request.to_reference} can no longer be merged due to conflict.
- @reasons.each do |reason|
* #{reason}
Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
......
...@@ -416,16 +416,10 @@ describe Notify do ...@@ -416,16 +416,10 @@ describe Notify do
end end
it 'has the correct subject and body' do 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 aggregate_failures do
is_expected.to have_referable_subject(merge_request, reply: true) 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(project_merge_request_path(project, merge_request))
is_expected.to have_body_text('following reasons:') is_expected.to have_body_text('due to conflict.')
reasons.each do |reason|
is_expected.to have_body_text(reason)
end
end end
end end
end end
......
...@@ -1324,6 +1324,7 @@ describe MergeRequest do ...@@ -1324,6 +1324,7 @@ describe MergeRequest do
context 'when broken' do context 'when broken' do
before do before do
allow(subject).to receive(:broken?) { true } allow(subject).to receive(:broken?) { true }
allow(project.repository).to receive(:can_be_merged?).and_return(false)
end end
it 'becomes unmergeable' do it 'becomes unmergeable' do
...@@ -2150,9 +2151,11 @@ describe MergeRequest do ...@@ -2150,9 +2151,11 @@ describe MergeRequest do
before do before do
allow(NotificationService).to receive(:new).and_return(notification_service) allow(NotificationService).to receive(:new).and_return(notification_service)
allow(TodoService).to receive(:new).and_return(todo_service) allow(TodoService).to receive(:new).and_return(todo_service)
allow(subject.project.repository).to receive(:can_be_merged?).and_return(false)
end end
it 'notifies, but does not notify again if rechecking still results in cannot_be_merged' do it 'notifies conflict, 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(notification_service).to receive(:merge_request_unmergeable).with(subject).once
expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
...@@ -2161,7 +2164,7 @@ describe MergeRequest do ...@@ -2161,7 +2164,7 @@ describe MergeRequest do
subject.mark_as_unmergeable subject.mark_as_unmergeable
end end
it 'notifies whenever merge request is newly unmergeable' do it 'notifies conflict, whenever newly unmergeable' do
expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice
expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
...@@ -2171,6 +2174,15 @@ describe MergeRequest do ...@@ -2171,6 +2174,15 @@ describe MergeRequest do
subject.mark_as_unchecked subject.mark_as_unchecked
subject.mark_as_unmergeable subject.mark_as_unmergeable
end end
it 'does not notify whenever merge request is newly unmergeable due to other reasons' do
allow(subject.project.repository).to receive(:can_be_merged?).and_return(true)
expect(notification_service).not_to receive(:merge_request_unmergeable)
expect(todo_service).not_to receive(:merge_request_became_unmergeable)
subject.mark_as_unmergeable
end
end end
describe 'check_state?' do describe 'check_state?' do
......
...@@ -70,41 +70,6 @@ describe MergeRequestPresenter do ...@@ -70,41 +70,6 @@ describe MergeRequestPresenter do
end end
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 describe '#conflict_resolution_path' do
let(:project) { create :project } let(:project) { create :project }
let(:user) { create :user } let(:user) { create :user }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment