Commit 5dad9a30 authored by Patrick Derichs's avatar Patrick Derichs Committed by Sean McGivern

Fix unexpected extra notification mails

Using custom_action and recipient filtering

Add more generic filtering to user_ids_notifiable_on

Add changelog entry

Remove commented class

Method is no longer needed

Overloading no longer required

Filter by action just in case of custom notification level

Add participant check

Fix unexpected extra notification mails

Using custom_action and recipient filtering

Add more generic filtering to user_ids_notifiable_on

Add changelog entry

Remove commented class

Method is no longer needed

Overloading no longer required

Filter by action just in case of custom notification level

Fix comment

Add repond_to? checks

Reverted custom_action filtering

Enhanced output of should_email helper

Changed :watch to :participating for custom notifiable users

Change spec variable name

Enhanced participating check

These conditions are no longer needed

Fix custom notification handling for participating type

Participating level should include maintainers

Fixed add_guest notification

Fix successful pipeline notification

Refactoring: Use maintainer? method on team instead

Add spec for new_issue: true for a custom group setting
which should have lower prio than an available project setting

Clean up specs
parent 2c5398ee
...@@ -47,14 +47,14 @@ class NotificationRecipient ...@@ -47,14 +47,14 @@ class NotificationRecipient
def suitable_notification_level? def suitable_notification_level?
case notification_level case notification_level
when :disabled, nil
false
when :custom
custom_enabled? || %i[participating mention].include?(@type)
when :watch, :participating
!action_excluded?
when :mention when :mention
@type == :mention @type == :mention
when :participating
!excluded_participating_action? && %i[participating mention watch].include?(@type)
when :custom
custom_enabled? || %i[participating mention].include?(@type)
when :watch
!excluded_watcher_action?
else else
false false
end end
...@@ -100,18 +100,14 @@ class NotificationRecipient ...@@ -100,18 +100,14 @@ class NotificationRecipient
end end
end end
def action_excluded?
excluded_watcher_action? || excluded_participating_action?
end
def excluded_watcher_action? def excluded_watcher_action?
return false unless @custom_action && notification_level == :watch return false unless @custom_action
NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action) NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
end end
def excluded_participating_action? def excluded_participating_action?
return false unless @custom_action && notification_level == :participating return false unless @custom_action
NotificationSetting::EXCLUDED_PARTICIPATING_EVENTS.include?(@custom_action) NotificationSetting::EXCLUDED_PARTICIPATING_EVENTS.include?(@custom_action)
end end
......
...@@ -135,7 +135,7 @@ module NotificationRecipientService ...@@ -135,7 +135,7 @@ module NotificationRecipientService
global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global) global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global)
user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action) user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action)
add_recipients(user_scope.where(id: user_ids), :watch, nil) add_recipients(user_scope.where(id: user_ids), :custom, nil)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
...@@ -391,7 +391,7 @@ module NotificationRecipientService ...@@ -391,7 +391,7 @@ module NotificationRecipientService
def build! def build!
return [] unless project return [] unless project
add_recipients(project.team.maintainers, :watch, nil) add_recipients(project.team.maintainers, :mention, nil)
end end
def acting_user def acting_user
......
---
title: Fix extra emails for custom notifications
merge_request: 25607
author:
type: fixed
...@@ -643,6 +643,64 @@ describe NotificationService, :mailer do ...@@ -643,6 +643,64 @@ describe NotificationService, :mailer do
end end
end end
describe 'Participating project notification settings have priority over group and global settings if available' do
let!(:group) { create(:group) }
let!(:maintainer) { group.add_owner(create(:user, username: 'maintainer')).user }
let!(:user1) { group.add_developer(create(:user, username: 'user_with_project_and_custom_setting')).user }
let(:project) { create(:project, :public, namespace: group) }
let(:issue) { create :issue, project: project, assignees: [assignee], description: '' }
before do
reset_delivered_emails!
create_notification_setting(user1, project, :participating)
end
context 'custom on group' do
[nil, true].each do |new_issue_value|
value_caption = new_issue_value || 'nil'
it "does not send an email to user1 when a new issue is created and new_issue is set to #{value_caption}" do
update_custom_notification(:new_issue, user1, resource: group, value: new_issue_value)
notification.new_issue(issue, maintainer)
should_not_email(user1)
end
end
end
context 'watch on group' do
it 'does not send an email' do
user1.notification_settings_for(group).update!(level: :watch)
notification.new_issue(issue, maintainer)
should_not_email(user1)
end
end
context 'custom on global, global on group' do
it 'does not send an email' do
user1.notification_settings_for(nil).update!(level: :custom)
user1.notification_settings_for(group).update!(level: :global)
notification.new_issue(issue, maintainer)
should_not_email(user1)
end
end
context 'watch on global, global on group' do
it 'does not send an email' do
user1.notification_settings_for(nil).update!(level: :watch)
user1.notification_settings_for(group).update!(level: :global)
notification.new_issue(issue, maintainer)
should_not_email(user1)
end
end
end
describe 'Issues' do describe 'Issues' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:project, :public, namespace: group) } let(:project) { create(:project, :public, namespace: group) }
...@@ -660,7 +718,7 @@ describe NotificationService, :mailer do ...@@ -660,7 +718,7 @@ describe NotificationService, :mailer do
end end
describe '#new_issue' do describe '#new_issue' do
it do it 'notifies the expected users' do
notification.new_issue(issue, @u_disabled) notification.new_issue(issue, @u_disabled)
should_email(assignee) should_email(assignee)
...@@ -1639,7 +1697,7 @@ describe NotificationService, :mailer do ...@@ -1639,7 +1697,7 @@ describe NotificationService, :mailer do
end end
describe '#project_was_moved' do describe '#project_was_moved' do
it do it 'notifies the expected users' do
notification.project_was_moved(project, "gitlab/gitlab") notification.project_was_moved(project, "gitlab/gitlab")
should_email(@u_watcher) should_email(@u_watcher)
......
...@@ -16,7 +16,9 @@ module EmailHelpers ...@@ -16,7 +16,9 @@ module EmailHelpers
end end
def should_email(user, times: 1, recipients: email_recipients) def should_email(user, times: 1, recipients: email_recipients)
expect(sent_to_user(user, recipients: recipients)).to eq(times) amount = sent_to_user(user, recipients: recipients)
failed_message = lambda { "User #{user.username} (#{user.id}): email test failed (expected #{times}, got #{amount})" }
expect(amount).to eq(times), failed_message
end end
def should_not_email(user, recipients: email_recipients) def should_not_email(user, recipients: email_recipients)
......
...@@ -17,11 +17,15 @@ module NotificationHelpers ...@@ -17,11 +17,15 @@ module NotificationHelpers
def create_user_with_notification(level, username, resource = project) def create_user_with_notification(level, username, resource = project)
user = create(:user, username: username) user = create(:user, username: username)
create_notification_setting(user, resource, level)
user
end
def create_notification_setting(user, resource, level)
setting = user.notification_settings_for(resource) setting = user.notification_settings_for(resource)
setting.level = level setting.level = level
setting.save setting.save
user
end end
# Create custom notifications # Create custom notifications
......
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