diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f112c06e26f4b7c078617b3ab1b2f851a565ce04..6c96c8ca39187ab47f3b5864a81d0454ed9d11b6 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -129,9 +129,7 @@ class MergeRequest < ActiveRecord::Base 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) + if merge_request.notify_conflict? NotificationService.new.merge_request_unmergeable(merge_request) TodoService.new.merge_request_became_unmergeable(merge_request) end @@ -708,6 +706,10 @@ class MergeRequest < ActiveRecord::Base should_remove_source_branch? || force_remove_source_branch? end + def notify_conflict? + (opened? || locked?) && !project.repository.can_be_merged?(diff_head_sha, target_branch) + end + def related_notes # Fetch comments only from last 100 commits commits_for_notes_limit = 100 diff --git a/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml new file mode 100644 index 0000000000000000000000000000000000000000..ae92c20fa1a0012857ebfe117fe7a86a5b0a1545 --- /dev/null +++ b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml @@ -0,0 +1,5 @@ +--- +title: Notify conflict for only open merge request +merge_request: 20125 +author: +type: fixed diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index edb0c6bdc3099caaacfd02663eb176b741d0a8d7..5dc62a30128e38b2db0146d4722bd2042cb31a80 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -111,7 +111,7 @@ 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 an open merge request becomes unmergeable due to conflict, 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. diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index 762bf6162681c01898400bd8324d73f540f183e4..760cd87d4cc0c5c0fb5f4a6a32bce2159ffe0bad 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -31,7 +31,7 @@ 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: +- an open merge request becomes unmergeable due to conflict, and you are either: - the author, or - have set it to automatically merge once pipeline succeeds. diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2c75816654edc76fec698494339680566b37d9fb..ec72fefd137cbb20070c1590132f5bcb90801f13 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2134,8 +2134,7 @@ describe MergeRequest do 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) } + subject { create(:merge_request, state, merge_status: :unchecked) } before do allow(NotificationService).to receive(:new).and_return(notification_service) @@ -2144,33 +2143,52 @@ describe MergeRequest do allow(subject.project.repository).to receive(:can_be_merged?).and_return(false) end - 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 + [:opened, :locked].each do |state| + context state do + let(:state) { state } - subject.mark_as_unmergeable - subject.mark_as_unchecked - subject.mark_as_unmergeable - end + 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 + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_unmergeable + end + + 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 + + subject.mark_as_unmergeable + subject.mark_as_unchecked + subject.mark_as_mergeable + 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) - 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 + expect(notification_service).not_to receive(:merge_request_unmergeable) + expect(todo_service).not_to receive(:merge_request_became_unmergeable) - subject.mark_as_unmergeable - subject.mark_as_unchecked - subject.mark_as_mergeable - subject.mark_as_unchecked - subject.mark_as_unmergeable + subject.mark_as_unmergeable + end + 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) + [:closed, :merged].each do |state| + let(:state) { state } - expect(notification_service).not_to receive(:merge_request_unmergeable) - expect(todo_service).not_to receive(:merge_request_became_unmergeable) + context state do + it 'does not notify' do + expect(notification_service).not_to receive(:merge_request_unmergeable) + expect(todo_service).not_to receive(:merge_request_became_unmergeable) - subject.mark_as_unmergeable + subject.mark_as_unmergeable + end + end end end