Commit d9446b28 authored by Sean McGivern's avatar Sean McGivern

Merge branch '6598-notify-only-open-unmergeable-mr' into 'master'

Notify conflict only for opened/locked merge requests

See merge request gitlab-org/gitlab-ce!20125
parents 6938335c ea25fbb8
...@@ -129,9 +129,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -129,9 +129,7 @@ class MergeRequest < ActiveRecord::Base
after_transition unchecked: :cannot_be_merged do |merge_request, transition| after_transition unchecked: :cannot_be_merged do |merge_request, transition|
begin begin
# Merge request can become unmergeable due to many reasons. if merge_request.notify_conflict?
# 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
...@@ -708,6 +706,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -708,6 +706,10 @@ class MergeRequest < ActiveRecord::Base
should_remove_source_branch? || force_remove_source_branch? should_remove_source_branch? || force_remove_source_branch?
end end
def notify_conflict?
(opened? || locked?) && !project.repository.can_be_merged?(diff_head_sha, target_branch)
end
def related_notes def related_notes
# Fetch comments only from last 100 commits # Fetch comments only from last 100 commits
commits_for_notes_limit = 100 commits_for_notes_limit = 100
......
---
title: Notify conflict for only open merge request
merge_request: 20125
author:
type: fixed
...@@ -111,7 +111,7 @@ by yourself (except when an issue is due). You will only receive automatic ...@@ -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 notifications when somebody else comments or adds changes to the ones that
you've created or mentions you. 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, If a user has also set the merge request to automatically merge once pipeline succeeds,
then that user will also be notified. then that user will also be notified.
......
...@@ -31,7 +31,7 @@ A Todo appears in your Todos dashboard when: ...@@ -31,7 +31,7 @@ A Todo appears in your Todos dashboard when:
- you are `@mentioned` in a comment on a commit, - you are `@mentioned` in a comment on a commit,
- a job in the CI pipeline running for your merge request failed, but this - a job in the CI pipeline running for your merge request failed, but this
job is not allowed to fail. 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 - the author, or
- have set it to automatically merge once pipeline succeeds. - have set it to automatically merge once pipeline succeeds.
......
...@@ -2134,8 +2134,7 @@ describe MergeRequest do ...@@ -2134,8 +2134,7 @@ describe MergeRequest do
describe 'transition to cannot_be_merged' do describe 'transition to cannot_be_merged' do
let(:notification_service) { double(:notification_service) } let(:notification_service) { double(:notification_service) }
let(:todo_service) { double(:todo_service) } let(:todo_service) { double(:todo_service) }
subject { create(:merge_request, state, merge_status: :unchecked) }
subject { create(:merge_request, merge_status: :unchecked) }
before do before do
allow(NotificationService).to receive(:new).and_return(notification_service) allow(NotificationService).to receive(:new).and_return(notification_service)
...@@ -2144,33 +2143,52 @@ describe MergeRequest do ...@@ -2144,33 +2143,52 @@ describe MergeRequest do
allow(subject.project.repository).to receive(:can_be_merged?).and_return(false) allow(subject.project.repository).to receive(:can_be_merged?).and_return(false)
end end
it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do [:opened, :locked].each do |state|
expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once context state do
expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once let(:state) { state }
subject.mark_as_unmergeable it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do
subject.mark_as_unchecked expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once
subject.mark_as_unmergeable expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
end
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).not_to receive(:merge_request_unmergeable)
expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice expect(todo_service).not_to receive(:merge_request_became_unmergeable)
expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
subject.mark_as_unmergeable subject.mark_as_unmergeable
subject.mark_as_unchecked end
subject.mark_as_mergeable end
subject.mark_as_unchecked
subject.mark_as_unmergeable
end end
it 'does not notify whenever merge request is newly unmergeable due to other reasons' do [:closed, :merged].each do |state|
allow(subject.project.repository).to receive(:can_be_merged?).and_return(true) let(:state) { state }
expect(notification_service).not_to receive(:merge_request_unmergeable) context state do
expect(todo_service).not_to receive(:merge_request_became_unmergeable) 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
end end
......
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