diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 5ba3a4a322cabc9b00a8e6fe42bc860025fbd6a2..70509e9066d21124e6b057a2a2a03e605a226f3b 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -59,8 +59,6 @@ module Emails def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil) 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)) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 324065c1162c444121dbcdaa7f56e85de97d3f01..4b78ba1029f55e3e9d380ed4abd123f11e46cc1f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -128,8 +128,17 @@ class MergeRequest < ActiveRecord::Base end 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) + 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) + TodoService.new.merge_request_became_unmergeable(merge_request) + 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) diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 8d466c3351067483a8a5182ec3b4783206d93524..eb54ab2cda648c51cd0cacd2c3eff0e0e3735d07 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -20,17 +20,6 @@ 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/views/notify/merge_request_unmergeable_email.html.haml b/app/views/notify/merge_request_unmergeable_email.html.haml index 578fa1fbce7888ee8762053b6505c8d8dcb1f034..7ec0c1ef39084527e9f96cef528aa1c50c89816c 100644 --- a/app/views/notify/merge_request_unmergeable_email.html.haml +++ b/app/views/notify/merge_request_unmergeable_email.html.haml @@ -1,6 +1,2 @@ %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)}: - - %ul - - @reasons.each do |reason| - %li= reason + 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. diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml index e4f9f1bf5e762cd513ff85972364fe45f07deb32..dcdd6db69d6dd45ce37a91aaf4356c8c5a82e32d 100644 --- a/app/views/notify/merge_request_unmergeable_email.text.haml +++ b/app/views/notify/merge_request_unmergeable_email.text.haml @@ -1,7 +1,4 @@ -Merge Request #{@merge_request.to_reference} can no longer be merged due to the following #{'reason'.pluralize(@reasons.count)}: - -- @reasons.each do |reason| - * #{reason} +Merge Request #{@merge_request.to_reference} can no longer be merged due to conflict. 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 775ca4ba0eb6b6b454bf446415e3d87a043ece5b..a9a45367b4ae5f892e6f18c357307e1318023e1b 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -416,16 +416,10 @@ describe Notify do 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)) - is_expected.to have_body_text('following reasons:') - reasons.each do |reason| - is_expected.to have_body_text(reason) - end + is_expected.to have_body_text('due to conflict.') end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3f028b3bd5c1cd342606bac7a76bccf884e707c8..7ae70c3afb466097003ffad86bb0a052587dc8cf 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1324,6 +1324,7 @@ describe MergeRequest do context 'when broken' do before do allow(subject).to receive(:broken?) { true } + allow(project.repository).to receive(:can_be_merged?).and_return(false) end it 'becomes unmergeable' do @@ -2150,9 +2151,11 @@ describe MergeRequest do before do allow(NotificationService).to receive(:new).and_return(notification_service) allow(TodoService).to receive(:new).and_return(todo_service) + + allow(subject.project.repository).to receive(:can_be_merged?).and_return(false) 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(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once @@ -2161,7 +2164,7 @@ describe MergeRequest do subject.mark_as_unmergeable 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(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_unmergeable 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 describe 'check_state?' do diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index d5fb4a7742c50bfd48d3d1bea4de0edb46ae7c19..e3b37739e8ed4700842dd09aae7499d75fd3a6a8 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -70,41 +70,6 @@ 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 }