diff options
-rw-r--r-- | app/models/notification_recipient.rb | 20 | ||||
-rw-r--r-- | app/services/notification_recipient_service.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/fix-extra-emails-for-custom-notifications.yml | 5 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 62 | ||||
-rw-r--r-- | spec/support/helpers/email_helpers.rb | 4 | ||||
-rw-r--r-- | spec/support/helpers/notification_helpers.rb | 8 |
6 files changed, 84 insertions, 19 deletions
diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index 793cce191fa..377ac3febb6 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -47,14 +47,14 @@ class NotificationRecipient def suitable_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 @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 false end @@ -100,18 +100,14 @@ class NotificationRecipient end end - def action_excluded? - excluded_watcher_action? || excluded_participating_action? - end - def excluded_watcher_action? - return false unless @custom_action && notification_level == :watch + return false unless @custom_action NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action) end def excluded_participating_action? - return false unless @custom_action && notification_level == :participating + return false unless @custom_action NotificationSetting::EXCLUDED_PARTICIPATING_EVENTS.include?(@custom_action) end diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 760962346fb..ca3f0b73096 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -135,7 +135,7 @@ module NotificationRecipientService 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) - add_recipients(user_scope.where(id: user_ids), :watch, nil) + add_recipients(user_scope.where(id: user_ids), :custom, nil) end # rubocop: enable CodeReuse/ActiveRecord @@ -391,7 +391,7 @@ module NotificationRecipientService def build! return [] unless project - add_recipients(project.team.maintainers, :watch, nil) + add_recipients(project.team.maintainers, :mention, nil) end def acting_user diff --git a/changelogs/unreleased/fix-extra-emails-for-custom-notifications.yml b/changelogs/unreleased/fix-extra-emails-for-custom-notifications.yml new file mode 100644 index 00000000000..6eb3225e4a1 --- /dev/null +++ b/changelogs/unreleased/fix-extra-emails-for-custom-notifications.yml @@ -0,0 +1,5 @@ +--- +title: Fix extra emails for custom notifications +merge_request: 25607 +author: +type: fixed diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8e34406133a..4b40c86574f 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -643,6 +643,64 @@ describe NotificationService, :mailer do 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 let(:group) { create(:group) } let(:project) { create(:project, :public, namespace: group) } @@ -660,7 +718,7 @@ describe NotificationService, :mailer do end describe '#new_issue' do - it do + it 'notifies the expected users' do notification.new_issue(issue, @u_disabled) should_email(assignee) @@ -1639,7 +1697,7 @@ describe NotificationService, :mailer do end describe '#project_was_moved' do - it do + it 'notifies the expected users' do notification.project_was_moved(project, "gitlab/gitlab") should_email(@u_watcher) diff --git a/spec/support/helpers/email_helpers.rb b/spec/support/helpers/email_helpers.rb index ad6e1064499..ed049daba80 100644 --- a/spec/support/helpers/email_helpers.rb +++ b/spec/support/helpers/email_helpers.rb @@ -16,7 +16,9 @@ module EmailHelpers end 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 def should_not_email(user, recipients: email_recipients) diff --git a/spec/support/helpers/notification_helpers.rb b/spec/support/helpers/notification_helpers.rb index 8d84510fb73..44c2051598c 100644 --- a/spec/support/helpers/notification_helpers.rb +++ b/spec/support/helpers/notification_helpers.rb @@ -17,11 +17,15 @@ module NotificationHelpers def create_user_with_notification(level, username, resource = project) 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.level = level setting.save - - user end # Create custom notifications |