don't rely on order of notification levels

factor out #suitable_notification_level? and check manually by
notification level. this makes the notification logic clear and actually
reflect what is in the documentation as to what should happen with each
setting.

1 files changed, 26 insertions, 27 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