diff options
| -rw-r--r-- | app/models/merge_request.rb | 8 | ||||
| -rw-r--r-- | changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml | 5 | ||||
| -rw-r--r-- | doc/workflow/notifications.md | 2 | ||||
| -rw-r--r-- | doc/workflow/todos.md | 2 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 62 |
5 files changed, 52 insertions, 27 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f112c06e26f..6c96c8ca391 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 00000000000..ae92c20fa1a --- /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 edb0c6bdc30..5dc62a30128 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 762bf616268..760cd87d4cc 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 2c75816654e..ec72fefd137 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 |