Merge branch 'fix-extra-emails-for-custom-notifications' into 'master'

Fix extra emails for custom notifications

Closes #56861

See merge request gitlab-org/gitlab-ce!25607

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