Merge branch '47488-remove-unmergeable-notification-for-no-commits' into 'master'
Notify only merge request unmergeable due to conflict See merge request gitlab-org/gitlab-ce!19548
This commit is contained in:
commit
a8f4f48e4a
|
@ -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
|
||||||
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
|
||||||
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)
|
||||||
|
|
|
@ -1,6 +1,2 @@
|
||||||
%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
|
|
||||||
|
|
|
@ -1,7 +1,4 @@
|
||||||
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)}
|
||||||
|
|
||||||
|
|
|
@ -1413,8 +1413,11 @@ module Gitlab
|
||||||
end
|
end
|
||||||
|
|
||||||
def can_be_merged?(source_sha, target_branch)
|
def can_be_merged?(source_sha, target_branch)
|
||||||
target_sha = find_branch(target_branch, true).target
|
if target_sha = find_branch(target_branch, true)&.target
|
||||||
!gitaly_conflicts_client(source_sha, target_sha).conflicts?
|
!gitaly_conflicts_client(source_sha, target_sha).conflicts?
|
||||||
|
else
|
||||||
|
false
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def search_files_by_name(query, ref)
|
def search_files_by_name(query, ref)
|
||||||
|
|
|
@ -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
|
||||||
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
|
||||||
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
|
||||||
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
|
||||||
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
|
||||||
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 }
|
||||||
|
|
Loading…
Reference in New Issue