Merge branch 'bugfix.notify-custom-participants' into 'master'

Bugfix.notify custom participants

Closes #36610

See merge request !13680

4 files changed, 46 insertions, 31 deletions

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index dc862565a71..183e098d819 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -27,46 +27,45 @@ class NotificationRecipient
     @notification_setting ||= find_notification_setting
   end
 
-  def raw_notification_level
-    notification_setting&.level&.to_sym
-  end
-
   def notification_level
-    # custom is treated the same as watch if it's enabled - otherwise it's
-    # set to :custom, meaning to send exactly when our type is :participating
-    # or :mention.
-    @notification_level ||=
-      case raw_notification_level
-      when :custom
-        if @custom_action && notification_setting&.event_enabled?(@custom_action)
-          :watch
-        else
-          :custom
-        end
-      else
-        raw_notification_level
-      end
+    @notification_level ||= notification_setting&.level&.to_sym
   end
 
   def notifiable?
     return false unless has_access?
     return false if own_activity?
 
-    return true if @type == :subscription
-
-    return false if notification_level.nil? || notification_level == :disabled
-
-    return %i[participating mention].include?(@type) if notification_level == :custom
+    # even users with :disabled notifications receive manual subscriptions
+    return !unsubscribed? if @type == :subscription
 
-    return false if %i[watch participating].include?(notification_level) && excluded_watcher_action?
-
-    return false unless NotificationSetting.levels[notification_level] <= NotificationSetting.levels[@type]
+    return false unless suitable_notification_level?
 
+    # check this last because it's expensive
+    # nobody should receive notifications if they've specifically unsubscribed
     return false if unsubscribed?
 
     true
   end
 
+  def suitable_notification_level?
+    case notification_level
+    when :disabled, nil
+      false
+    when :custom
+      custom_enabled? || %i[participating mention].include?(@type)
+    when :watch, :participating
+      !excluded_watcher_action?
+    when :mention
+      @type == :mention
+    else
+      false
+    end
+  end
+
+  def custom_enabled?
+    @custom_action && notification_setting&.event_enabled?(@custom_action)
+  end
+
   def unsubscribed?
     return false unless @target
     return false unless @target.respond_to?(:subscriptions)
@@ -98,7 +97,7 @@ class NotificationRecipient
 
   def excluded_watcher_action?
     return false unless @custom_action
-    return false if raw_notification_level == :custom
+    return false if notification_level == :custom
 
     NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
   end
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 21c9c314a2a..c9f07c140f7 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -95,7 +95,7 @@ module NotificationRecipientService
       def add_participants(user)
         return unless target.respond_to?(:participants)
 
-        self << [target.participants(user), :watch]
+        self << [target.participants(user), :participating]
       end
 
       # Get project/group users with CUSTOM notification level
diff --git a/changelogs/unreleased/bugfix-notify-custom-participants.yml b/changelogs/unreleased/bugfix-notify-custom-participants.yml
new file mode 100644
index 00000000000..04fcb95e18a
--- /dev/null
+++ b/changelogs/unreleased/bugfix-notify-custom-participants.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed: Notifications weren't sending to participating users with a `Custom` notification setting.
+merge_request: 13680
+author: jneen
+type: fixed
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 44b2d28d1d4..5b349017c8b 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -130,7 +130,18 @@ describe NotificationService, :mailer do
         project.add_master(issue.author)
         project.add_master(assignee)
         project.add_master(note.author)
-        create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy')
+
+        @u_custom_off = create_user_with_notification(:custom, 'custom_off')
+        project.add_guest(@u_custom_off)
+
+        create(
+          :note_on_issue,
+          author: @u_custom_off,
+          noteable: issue,
+          project_id: issue.project_id,
+          note: 'i think @subscribed_participant should see this'
+        )
+
         update_custom_notification(:new_note, @u_guest_custom, resource: project)
         update_custom_notification(:new_note, @u_custom_global)
       end
@@ -140,8 +151,7 @@ describe NotificationService, :mailer do
           add_users_with_subscription(note.project, issue)
           reset_delivered_emails!
 
-          # Ensure create SentNotification by noteable = issue 6 times, not noteable = note
-          expect(SentNotification).to receive(:record).with(issue, any_args).exactly(8).times
+          expect(SentNotification).to receive(:record).with(issue, any_args).exactly(9).times
 
           notification.new_note(note)
 
@@ -153,6 +163,7 @@ describe NotificationService, :mailer do
           should_email(@subscriber)
           should_email(@watcher_and_subscriber)
           should_email(@subscribed_participant)
+          should_email(@u_custom_off)
           should_not_email(@u_guest_custom)
           should_not_email(@u_guest_watcher)
           should_not_email(note.author)