From 80647364ffaad3d100c64dd8a1ab32e8a9b4bcf9 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 19 Jul 2017 11:17:29 -0700
Subject: [PATCH 01/43] move the ability check to reject_users_without_access

---
 app/services/notification_recipient_service.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 9ac561e4bd2..1abf0335dc4 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -253,7 +253,6 @@ class NotificationRecipientService
     end
 
     users = users.to_a.compact.uniq
-    users = users.select { |u| u.can?(:receive_notifications) }
 
     users.reject do |user|
       global_notification_setting = user.global_notification_setting
@@ -287,6 +286,8 @@ class NotificationRecipientService
   end
 
   def reject_users_without_access(recipients, target)
+    recipients = recipients.select { |u| u.can?(:receive_notifications) }
+
     ability = case target
               when Issuable
                 :"read_#{target.to_ability_name}"

From b188e1b9e66eb02ac9b51364dfa33e206ea636c3 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 11:57:42 -0700
Subject: [PATCH 02/43] move notification_setting_for_user_project to a public
 class method

---
 .../notification_recipient_service.rb         | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 1abf0335dc4..07f5655b826 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -4,6 +4,18 @@
 class NotificationRecipientService
   attr_reader :project
 
+  def self.notification_setting_for_user_project(user, project)
+    project_setting = user.notification_settings_for(project)
+
+    return project_setting unless project_setting.global?
+
+    group_setting = user.notification_settings_for(project.group)
+
+    return group_setting unless group_setting.global?
+
+    user.global_notification_setting
+  end
+
   def initialize(project)
     @project = project
   end
@@ -55,7 +67,7 @@ class NotificationRecipientService
         :success_pipeline
       end
 
-    notification_setting = notification_setting_for_user_project(current_user, target.project)
+    notification_setting = NotificationRecipientService.notification_setting_for_user_project(current_user, target.project)
 
     return [] if notification_setting.mention? || notification_setting.disabled?
 
@@ -317,16 +329,4 @@ class NotificationRecipientService
   def build_custom_key(action, object)
     "#{action}_#{object.class.model_name.name.underscore}".to_sym
   end
-
-  def notification_setting_for_user_project(user, project)
-    project_setting = user.notification_settings_for(project)
-
-    return project_setting unless project_setting.global?
-
-    group_setting = user.notification_settings_for(project.group)
-
-    return group_setting unless group_setting.global?
-
-    user.global_notification_setting
-  end
 end

From 9cd46811d3dd2e6a0cb1b68ba390f9ab3fc587b7 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 11:58:30 -0700
Subject: [PATCH 03/43] protect against nil project/group/setting

---
 app/services/notification_recipient_service.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 07f5655b826..e1456158ff7 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -5,13 +5,13 @@ class NotificationRecipientService
   attr_reader :project
 
   def self.notification_setting_for_user_project(user, project)
-    project_setting = user.notification_settings_for(project)
+    project_setting = project && user.notification_settings_for(project)
 
-    return project_setting unless project_setting.global?
+    return project_setting unless project_setting.nil? || project_setting.global?
 
-    group_setting = user.notification_settings_for(project.group)
+    group_setting = project&.group && user.notification_settings_for(project.group)
 
-    return group_setting unless group_setting.global?
+    return group_setting unless group_setting.nil? || group_setting.global?
 
     user.global_notification_setting
   end

From cce1bc9a2f3bbba5ad2a256c96d85eb2276e6ac6 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 12:02:12 -0700
Subject: [PATCH 04/43] use notification_setting_for_user_project in
 reject_users

---
 .../notification_recipient_service.rb         | 20 ++-----------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index e1456158ff7..92049add32d 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -267,24 +267,8 @@ class NotificationRecipientService
     users = users.to_a.compact.uniq
 
     users.reject do |user|
-      global_notification_setting = user.global_notification_setting
-
-      next global_notification_setting.level == level unless project
-
-      setting = user.notification_settings_for(project)
-
-      if project.group && (setting.nil? || setting.global?)
-        setting = user.notification_settings_for(project.group)
-      end
-
-      # reject users who globally set mention notification and has no setting per project/group
-      next global_notification_setting.level == level unless setting
-
-      # reject users who set mention notification in project
-      next true if setting.level == level
-
-      # reject users who have mention level in project and disabled in global settings
-      setting.global? && global_notification_setting.level == level
+      setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
+      setting.present? && setting.level == level
     end
   end
 

From b1334ed94e09f584505f9804566e51a34817c8da Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 12:21:47 -0700
Subject: [PATCH 05/43] move the builders to classes with a #build

---
 .../notification_recipient_service.rb         | 598 ++++++++++--------
 1 file changed, 318 insertions(+), 280 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 92049add32d..97bbc47efb3 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -20,297 +20,335 @@ class NotificationRecipientService
     @project = project
   end
 
-  def build_recipients(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
-    custom_action = build_custom_key(action, target)
+  module Builder
 
-    recipients = participants(target, current_user)
-    recipients = add_project_watchers(recipients)
-    recipients = add_custom_notifications(recipients, custom_action)
-    recipients = reject_mention_users(recipients)
-
-    # Re-assign is considered as a mention of the new assignee so we add the
-    # new assignee to the list of recipients after we rejected users with
-    # the "on mention" notification level
-    case custom_action
-    when :reassign_merge_request
-      recipients << previous_assignee if previous_assignee
-      recipients << target.assignee
-    when :reassign_issue
-      previous_assignees = Array(previous_assignee)
-      recipients.concat(previous_assignees)
-      recipients.concat(target.assignees)
-    end
-
-    recipients = reject_muted_users(recipients)
-    recipients = add_subscribed_users(recipients, target)
-
-    if [:new_issue, :new_merge_request].include?(custom_action)
-      recipients = add_labels_subscribers(recipients, target)
-    end
-
-    recipients = reject_unsubscribed_users(recipients, target)
-    recipients = reject_users_without_access(recipients, target)
-
-    recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
-
-    recipients.uniq
-  end
-
-  def build_pipeline_recipients(target, current_user, action:)
-    return [] unless current_user
-
-    custom_action =
-      case action.to_s
-      when 'failed'
-        :failed_pipeline
-      when 'success'
-        :success_pipeline
+    class Base
+      attr_reader :project
+      def initialize(project)
+        @project = project
       end
 
-    notification_setting = NotificationRecipientService.notification_setting_for_user_project(current_user, target.project)
+      def build(*)
+        raise 'abstract'
+      end
 
-    return [] if notification_setting.mention? || notification_setting.disabled?
+      # Remove users with disabled notifications from array
+      # Also remove duplications and nil recipients
+      def reject_muted_users(users)
+        reject_users(users, :disabled)
+      end
 
-    return [] if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
+      protected
 
-    return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
+      # Ensure that if we modify this array, we aren't modifying the memoised
+      # participants on the target.
+      def participants(target, user)
+        return unless target.respond_to?(:participants)
 
-    reject_users_without_access([current_user], target)
+        target.participants(user).dup
+      end
+
+      # Get project/group users with CUSTOM notification level
+      def add_custom_notifications(recipients, action)
+        user_ids = []
+
+        # Users with a notification setting on group or project
+        user_ids += user_ids_notifiable_on(project, :custom, action)
+        user_ids += user_ids_notifiable_on(project.group, :custom, action)
+
+        # Users with global level custom
+        user_ids_with_project_level_global = user_ids_notifiable_on(project, :global)
+        user_ids_with_group_level_global   = user_ids_notifiable_on(project.group, :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, action)
+
+        recipients.concat(User.find(user_ids))
+      end
+
+      def add_project_watchers(recipients)
+        recipients.concat(project_watchers).compact
+      end
+
+      # Get project users with WATCH notification level
+      def project_watchers
+        project_members_ids = user_ids_notifiable_on(project)
+
+        user_ids_with_project_global = user_ids_notifiable_on(project, :global)
+        user_ids_with_group_global   = user_ids_notifiable_on(project.group, :global)
+
+        user_ids = user_ids_with_global_level_watch((user_ids_with_project_global + user_ids_with_group_global).uniq)
+
+        user_ids_with_project_setting = select_project_members_ids(project, user_ids_with_project_global, user_ids)
+        user_ids_with_group_setting = select_group_members_ids(project.group, project_members_ids, user_ids_with_group_global, user_ids)
+
+        User.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq).to_a
+      end
+
+      # Remove users with notification level 'Mentioned'
+      def reject_mention_users(users)
+        reject_users(users, :mention)
+      end
+
+      def add_subscribed_users(recipients, target)
+        return recipients unless target.respond_to? :subscribers
+
+        recipients + target.subscribers(project)
+      end
+
+      def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
+        return [] unless resource
+
+        if notification_level
+          settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level])
+          settings = settings.select { |setting| setting.event_enabled?(action) } if action.present?
+          settings.map(&:user_id)
+        else
+          resource.notification_settings.pluck(:user_id)
+        end
+      end
+
+      # Build a list of user_ids based on project notification settings
+      def select_project_members_ids(project, global_setting, user_ids_global_level_watch)
+        user_ids = user_ids_notifiable_on(project, :watch)
+
+        # If project setting is global, add to watch list if global setting is watch
+        global_setting.each do |user_id|
+          if user_ids_global_level_watch.include?(user_id)
+            user_ids << user_id
+          end
+        end
+
+        user_ids
+      end
+
+      # Build a list of user_ids based on group notification settings
+      def select_group_members_ids(group, project_members, global_setting, user_ids_global_level_watch)
+        uids = user_ids_notifiable_on(group, :watch)
+
+        # Group setting is watch, add to user_ids list if user is not project member
+        user_ids = []
+        uids.each do |user_id|
+          if project_members.exclude?(user_id)
+            user_ids << user_id
+          end
+        end
+
+        # Group setting is global, add to user_ids list if global setting is watch
+        global_setting.each do |user_id|
+          if project_members.exclude?(user_id) && user_ids_global_level_watch.include?(user_id)
+            user_ids << user_id
+          end
+        end
+
+        user_ids
+      end
+
+      def user_ids_with_global_level_watch(ids)
+        settings_with_global_level_of(:watch, ids).pluck(:user_id)
+      end
+
+      def user_ids_with_global_level_custom(ids, action)
+        settings = settings_with_global_level_of(:custom, ids)
+        settings = settings.select { |setting| setting.event_enabled?(action) }
+        settings.map(&:user_id)
+      end
+
+      def settings_with_global_level_of(level, ids)
+        NotificationSetting.where(
+          user_id: ids,
+          source_type: nil,
+          level: NotificationSetting.levels[level]
+        )
+      end
+
+      # Reject users which has certain notification level
+      #
+      # Example:
+      #   reject_users(users, :watch, project)
+      #
+      def reject_users(users, level)
+        level = level.to_s
+
+        unless NotificationSetting.levels.keys.include?(level)
+          raise 'Invalid notification level'
+        end
+
+        users = users.to_a.compact.uniq
+
+        users.reject do |user|
+          setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
+          setting.present? && setting.level == level
+        end
+      end
+
+      def reject_unsubscribed_users(recipients, target)
+        return recipients unless target.respond_to? :subscriptions
+
+        recipients.reject do |user|
+          subscription = target.subscriptions.find_by_user_id(user.id)
+          subscription && !subscription.subscribed
+        end
+      end
+
+      def reject_users_without_access(recipients, target)
+        recipients = recipients.select { |u| u.can?(:receive_notifications) }
+
+        ability = case target
+                  when Issuable
+                    :"read_#{target.to_ability_name}"
+                  when Ci::Pipeline
+                    :read_build # We have build trace in pipeline emails
+                  end
+
+        return recipients unless ability
+
+        recipients.select do |user|
+          user.can?(ability, target)
+        end
+      end
+
+      def add_labels_subscribers(recipients, target, labels: nil)
+        return recipients unless target.respond_to? :labels
+
+        (labels || target.labels).each do |label|
+          recipients += label.subscribers(project)
+        end
+
+        recipients
+      end
+
+      # Build event key to search on custom notification level
+      # Check NotificationSetting::EMAIL_EVENTS
+      def build_custom_key(action, object)
+        "#{action}_#{object.class.model_name.name.underscore}".to_sym
+      end
+    end
+
+    class Default < Base
+      def build(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
+        custom_action = build_custom_key(action, target)
+
+        recipients = participants(target, current_user)
+        recipients = add_project_watchers(recipients)
+        recipients = add_custom_notifications(recipients, custom_action)
+        recipients = reject_mention_users(recipients)
+
+        # Re-assign is considered as a mention of the new assignee so we add the
+        # new assignee to the list of recipients after we rejected users with
+        # the "on mention" notification level
+        case custom_action
+        when :reassign_merge_request
+          recipients << previous_assignee if previous_assignee
+          recipients << target.assignee
+        when :reassign_issue
+          previous_assignees = Array(previous_assignee)
+          recipients.concat(previous_assignees)
+          recipients.concat(target.assignees)
+        end
+
+        recipients = reject_muted_users(recipients)
+        recipients = add_subscribed_users(recipients, target)
+
+        if [:new_issue, :new_merge_request].include?(custom_action)
+          recipients = add_labels_subscribers(recipients, target)
+        end
+
+        recipients = reject_unsubscribed_users(recipients, target)
+        recipients = reject_users_without_access(recipients, target)
+
+        recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
+
+        recipients.uniq
+      end
+    end
+
+    class Pipeline < Base
+      def build(target, current_user, action:)
+        return [] unless current_user
+
+        custom_action =
+          case action.to_s
+          when 'failed'
+            :failed_pipeline
+          when 'success'
+            :success_pipeline
+          end
+
+        notification_setting = NotificationRecipientService.notification_setting_for_user_project(current_user, target.project)
+
+        return [] if notification_setting.mention? || notification_setting.disabled?
+
+        return [] if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
+
+        return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
+
+        reject_users_without_access([current_user], target)
+      end
+    end
+
+    class Relabeled < Base
+      def build(target, current_user, labels:)
+        recipients = add_labels_subscribers([], target, labels: labels)
+        recipients = reject_unsubscribed_users(recipients, target)
+        recipients = reject_users_without_access(recipients, target)
+        recipients.delete(current_user) unless current_user.notified_of_own_activity?
+        recipients.uniq
+      end
+    end
+
+    class NewNote < Base
+      def build(note)
+        target = note.noteable
+
+        ability, subject = if note.for_personal_snippet?
+                             [:read_personal_snippet, note.noteable]
+                           else
+                             [:read_project, note.project]
+                           end
+
+        mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
+
+        # Add all users participating in the thread (author, assignee, comment authors)
+        recipients = participants(target, note.author) || mentioned_users
+
+        unless note.for_personal_snippet?
+          # Merge project watchers
+          recipients = add_project_watchers(recipients)
+
+          # Merge project with custom notification
+          recipients = add_custom_notifications(recipients, :new_note)
+        end
+
+        # Reject users with Mention notification level, except those mentioned in _this_ note.
+        recipients = reject_mention_users(recipients - mentioned_users)
+        recipients = recipients + mentioned_users
+
+        recipients = reject_muted_users(recipients)
+
+        recipients = add_subscribed_users(recipients, note.noteable)
+        recipients = reject_unsubscribed_users(recipients, note.noteable)
+        recipients = reject_users_without_access(recipients, note.noteable)
+
+        recipients.delete(note.author) unless note.author.notified_of_own_activity?
+        recipients.uniq
+      end
+    end
   end
 
-  def build_relabeled_recipients(target, current_user, labels:)
-    recipients = add_labels_subscribers([], target, labels: labels)
-    recipients = reject_unsubscribed_users(recipients, target)
-    recipients = reject_users_without_access(recipients, target)
-    recipients.delete(current_user) unless current_user.notified_of_own_activity?
-    recipients.uniq
+  def build_recipients(*a)
+    Builder::Default.new(@project).build(*a)
+  end
+
+  def build_pipeline_recipients(*a)
+    Builder::Pipeline.new(@project).build(*a)
+  end
+
+  def build_relabeled_recipients(*a)
+    Builder::Relabeled.new(@project).build(*a)
   end
 
   def build_new_note_recipients(note)
-    target = note.noteable
-
-    ability, subject = if note.for_personal_snippet?
-                         [:read_personal_snippet, note.noteable]
-                       else
-                         [:read_project, note.project]
-                       end
-
-    mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
-
-    # Add all users participating in the thread (author, assignee, comment authors)
-    recipients = participants(target, note.author) || mentioned_users
-
-    unless note.for_personal_snippet?
-      # Merge project watchers
-      recipients = add_project_watchers(recipients)
-
-      # Merge project with custom notification
-      recipients = add_custom_notifications(recipients, :new_note)
-    end
-
-    # Reject users with Mention notification level, except those mentioned in _this_ note.
-    recipients = reject_mention_users(recipients - mentioned_users)
-    recipients = recipients + mentioned_users
-
-    recipients = reject_muted_users(recipients)
-
-    recipients = add_subscribed_users(recipients, note.noteable)
-    recipients = reject_unsubscribed_users(recipients, note.noteable)
-    recipients = reject_users_without_access(recipients, note.noteable)
-
-    recipients.delete(note.author) unless note.author.notified_of_own_activity?
-    recipients.uniq
-  end
-
-  # Remove users with disabled notifications from array
-  # Also remove duplications and nil recipients
-  def reject_muted_users(users)
-    reject_users(users, :disabled)
-  end
-
-  protected
-
-  # Ensure that if we modify this array, we aren't modifying the memoised
-  # participants on the target.
-  def participants(target, user)
-    return unless target.respond_to?(:participants)
-
-    target.participants(user).dup
-  end
-
-  # Get project/group users with CUSTOM notification level
-  def add_custom_notifications(recipients, action)
-    user_ids = []
-
-    # Users with a notification setting on group or project
-    user_ids += user_ids_notifiable_on(project, :custom, action)
-    user_ids += user_ids_notifiable_on(project.group, :custom, action)
-
-    # Users with global level custom
-    user_ids_with_project_level_global = user_ids_notifiable_on(project, :global)
-    user_ids_with_group_level_global   = user_ids_notifiable_on(project.group, :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, action)
-
-    recipients.concat(User.find(user_ids))
-  end
-
-  def add_project_watchers(recipients)
-    recipients.concat(project_watchers).compact
-  end
-
-  # Get project users with WATCH notification level
-  def project_watchers
-    project_members_ids = user_ids_notifiable_on(project)
-
-    user_ids_with_project_global = user_ids_notifiable_on(project, :global)
-    user_ids_with_group_global   = user_ids_notifiable_on(project.group, :global)
-
-    user_ids = user_ids_with_global_level_watch((user_ids_with_project_global + user_ids_with_group_global).uniq)
-
-    user_ids_with_project_setting = select_project_members_ids(project, user_ids_with_project_global, user_ids)
-    user_ids_with_group_setting = select_group_members_ids(project.group, project_members_ids, user_ids_with_group_global, user_ids)
-
-    User.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq).to_a
-  end
-
-  # Remove users with notification level 'Mentioned'
-  def reject_mention_users(users)
-    reject_users(users, :mention)
-  end
-
-  def add_subscribed_users(recipients, target)
-    return recipients unless target.respond_to? :subscribers
-
-    recipients + target.subscribers(project)
-  end
-
-  def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
-    return [] unless resource
-
-    if notification_level
-      settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level])
-      settings = settings.select { |setting| setting.event_enabled?(action) } if action.present?
-      settings.map(&:user_id)
-    else
-      resource.notification_settings.pluck(:user_id)
-    end
-  end
-
-  # Build a list of user_ids based on project notification settings
-  def select_project_members_ids(project, global_setting, user_ids_global_level_watch)
-    user_ids = user_ids_notifiable_on(project, :watch)
-
-    # If project setting is global, add to watch list if global setting is watch
-    global_setting.each do |user_id|
-      if user_ids_global_level_watch.include?(user_id)
-        user_ids << user_id
-      end
-    end
-
-    user_ids
-  end
-
-  # Build a list of user_ids based on group notification settings
-  def select_group_members_ids(group, project_members, global_setting, user_ids_global_level_watch)
-    uids = user_ids_notifiable_on(group, :watch)
-
-    # Group setting is watch, add to user_ids list if user is not project member
-    user_ids = []
-    uids.each do |user_id|
-      if project_members.exclude?(user_id)
-        user_ids << user_id
-      end
-    end
-
-    # Group setting is global, add to user_ids list if global setting is watch
-    global_setting.each do |user_id|
-      if project_members.exclude?(user_id) && user_ids_global_level_watch.include?(user_id)
-        user_ids << user_id
-      end
-    end
-
-    user_ids
-  end
-
-  def user_ids_with_global_level_watch(ids)
-    settings_with_global_level_of(:watch, ids).pluck(:user_id)
-  end
-
-  def user_ids_with_global_level_custom(ids, action)
-    settings = settings_with_global_level_of(:custom, ids)
-    settings = settings.select { |setting| setting.event_enabled?(action) }
-    settings.map(&:user_id)
-  end
-
-  def settings_with_global_level_of(level, ids)
-    NotificationSetting.where(
-      user_id: ids,
-      source_type: nil,
-      level: NotificationSetting.levels[level]
-    )
-  end
-
-  # Reject users which has certain notification level
-  #
-  # Example:
-  #   reject_users(users, :watch, project)
-  #
-  def reject_users(users, level)
-    level = level.to_s
-
-    unless NotificationSetting.levels.keys.include?(level)
-      raise 'Invalid notification level'
-    end
-
-    users = users.to_a.compact.uniq
-
-    users.reject do |user|
-      setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
-      setting.present? && setting.level == level
-    end
-  end
-
-  def reject_unsubscribed_users(recipients, target)
-    return recipients unless target.respond_to? :subscriptions
-
-    recipients.reject do |user|
-      subscription = target.subscriptions.find_by_user_id(user.id)
-      subscription && !subscription.subscribed
-    end
-  end
-
-  def reject_users_without_access(recipients, target)
-    recipients = recipients.select { |u| u.can?(:receive_notifications) }
-
-    ability = case target
-              when Issuable
-                :"read_#{target.to_ability_name}"
-              when Ci::Pipeline
-                :read_build # We have build trace in pipeline emails
-              end
-
-    return recipients unless ability
-
-    recipients.select do |user|
-      user.can?(ability, target)
-    end
-  end
-
-  def add_labels_subscribers(recipients, target, labels: nil)
-    return recipients unless target.respond_to? :labels
-
-    (labels || target.labels).each do |label|
-      recipients += label.subscribers(project)
-    end
-
-    recipients
-  end
-
-  # Build event key to search on custom notification level
-  # Check NotificationSetting::EMAIL_EVENTS
-  def build_custom_key(action, object)
-    "#{action}_#{object.class.model_name.name.underscore}".to_sym
+    Builder::NewNote.new(@project).build(note)
   end
 end

From 947bff886ba62087715dbc8b4d48e85588bd5322 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 12:25:32 -0700
Subject: [PATCH 06/43] move the build arguments to the initializers

---
 .../notification_recipient_service.rb         | 74 +++++++++++++++----
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 97bbc47efb3..a68967d38b9 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -21,11 +21,13 @@ class NotificationRecipientService
   end
 
   module Builder
-
     class Base
-      attr_reader :project
-      def initialize(project)
-        @project = project
+      def initialize(*)
+        raise 'abstract'
+      end
+
+      def build
+        raise 'abstract'
       end
 
       def build(*)
@@ -226,7 +228,22 @@ class NotificationRecipientService
     end
 
     class Default < Base
-      def build(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
+      attr_reader :project
+      attr_reader :target
+      attr_reader :current_user
+      attr_reader :action
+      attr_reader :previous_assignee
+      attr_reader :skip_current_user
+      def initialize(project, target, current_user, action:, previous_assignee: nil, skip_current_user: true)
+        @project = project
+        @target = target
+        @current_user = current_user
+        @action = action
+        @previous_assignee = previous_assignee
+        @skip_current_user = skip_current_user
+      end
+
+      def build
         custom_action = build_custom_key(action, target)
 
         recipients = participants(target, current_user)
@@ -264,7 +281,18 @@ class NotificationRecipientService
     end
 
     class Pipeline < Base
-      def build(target, current_user, action:)
+      attr_reader :project
+      attr_reader :target
+      attr_reader :current_user
+      attr_reader :action
+      def initialize(project, target, current_user, action:)
+        @project = project
+        @target = target
+        @current_user = current_user
+        @action = action
+      end
+
+      def build
         return [] unless current_user
 
         custom_action =
@@ -288,7 +316,18 @@ class NotificationRecipientService
     end
 
     class Relabeled < Base
-      def build(target, current_user, labels:)
+      attr_reader :project
+      attr_reader :target
+      attr_reader :current_user
+      attr_reader :labels
+      def initialize(project, target, current_user, labels:)
+        @project = project
+        @target = target
+        @current_user = current_user
+        @labels = labels
+      end
+
+      def build
         recipients = add_labels_subscribers([], target, labels: labels)
         recipients = reject_unsubscribed_users(recipients, target)
         recipients = reject_users_without_access(recipients, target)
@@ -298,9 +337,16 @@ class NotificationRecipientService
     end
 
     class NewNote < Base
-      def build(note)
-        target = note.noteable
+      attr_reader :project
+      attr_reader :note
+      attr_reader :target
+      def initialize(project, note)
+        @project = project
+        @note = note
+        @target = note.noteable
+      end
 
+      def build(note)
         ability, subject = if note.for_personal_snippet?
                              [:read_personal_snippet, note.noteable]
                            else
@@ -337,18 +383,18 @@ class NotificationRecipientService
   end
 
   def build_recipients(*a)
-    Builder::Default.new(@project).build(*a)
+    Builder::Default.new(@project, *a).build
   end
 
   def build_pipeline_recipients(*a)
-    Builder::Pipeline.new(@project).build(*a)
+    Builder::Pipeline.new(@project, *a).build
   end
 
   def build_relabeled_recipients(*a)
-    Builder::Relabeled.new(@project).build(*a)
+    Builder::Relabeled.new(@project, *a).build
   end
 
-  def build_new_note_recipients(note)
-    Builder::NewNote.new(@project).build(note)
+  def build_new_note_recipients(*a)
+    Builder::NewNote.new(@project, *a).build
   end
 end

From b010a556b6b0314b333eb13ecc7095dc361f67a2 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 12:36:37 -0700
Subject: [PATCH 07/43] factor out the `target` argument to helpers

---
 .../notification_recipient_service.rb         | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index a68967d38b9..edad4ff021a 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -30,7 +30,7 @@ class NotificationRecipientService
         raise 'abstract'
       end
 
-      def build(*)
+      def target
         raise 'abstract'
       end
 
@@ -44,7 +44,7 @@ class NotificationRecipientService
 
       # Ensure that if we modify this array, we aren't modifying the memoised
       # participants on the target.
-      def participants(target, user)
+      def participants(user)
         return unless target.respond_to?(:participants)
 
         target.participants(user).dup
@@ -92,7 +92,7 @@ class NotificationRecipientService
         reject_users(users, :mention)
       end
 
-      def add_subscribed_users(recipients, target)
+      def add_subscribed_users(recipients)
         return recipients unless target.respond_to? :subscribers
 
         recipients + target.subscribers(project)
@@ -184,7 +184,7 @@ class NotificationRecipientService
         end
       end
 
-      def reject_unsubscribed_users(recipients, target)
+      def reject_unsubscribed_users(recipients)
         return recipients unless target.respond_to? :subscriptions
 
         recipients.reject do |user|
@@ -193,7 +193,7 @@ class NotificationRecipientService
         end
       end
 
-      def reject_users_without_access(recipients, target)
+      def reject_users_without_access(recipients)
         recipients = recipients.select { |u| u.can?(:receive_notifications) }
 
         ability = case target
@@ -210,7 +210,7 @@ class NotificationRecipientService
         end
       end
 
-      def add_labels_subscribers(recipients, target, labels: nil)
+      def add_labels_subscribers(recipients, labels: nil)
         return recipients unless target.respond_to? :labels
 
         (labels || target.labels).each do |label|
@@ -246,7 +246,7 @@ class NotificationRecipientService
       def build
         custom_action = build_custom_key(action, target)
 
-        recipients = participants(target, current_user)
+        recipients = participants(current_user)
         recipients = add_project_watchers(recipients)
         recipients = add_custom_notifications(recipients, custom_action)
         recipients = reject_mention_users(recipients)
@@ -265,14 +265,14 @@ class NotificationRecipientService
         end
 
         recipients = reject_muted_users(recipients)
-        recipients = add_subscribed_users(recipients, target)
+        recipients = add_subscribed_users(recipients)
 
         if [:new_issue, :new_merge_request].include?(custom_action)
-          recipients = add_labels_subscribers(recipients, target)
+          recipients = add_labels_subscribers(recipients)
         end
 
-        recipients = reject_unsubscribed_users(recipients, target)
-        recipients = reject_users_without_access(recipients, target)
+        recipients = reject_unsubscribed_users(recipients)
+        recipients = reject_users_without_access(recipients)
 
         recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
 
@@ -311,7 +311,7 @@ class NotificationRecipientService
 
         return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
 
-        reject_users_without_access([current_user], target)
+        reject_users_without_access([current_user])
       end
     end
 
@@ -328,9 +328,9 @@ class NotificationRecipientService
       end
 
       def build
-        recipients = add_labels_subscribers([], target, labels: labels)
-        recipients = reject_unsubscribed_users(recipients, target)
-        recipients = reject_users_without_access(recipients, target)
+        recipients = add_labels_subscribers([], labels: labels)
+        recipients = reject_unsubscribed_users(recipients)
+        recipients = reject_users_without_access(recipients)
         recipients.delete(current_user) unless current_user.notified_of_own_activity?
         recipients.uniq
       end
@@ -356,7 +356,7 @@ class NotificationRecipientService
         mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
 
         # Add all users participating in the thread (author, assignee, comment authors)
-        recipients = participants(target, note.author) || mentioned_users
+        recipients = participants(note.author) || mentioned_users
 
         unless note.for_personal_snippet?
           # Merge project watchers
@@ -372,9 +372,9 @@ class NotificationRecipientService
 
         recipients = reject_muted_users(recipients)
 
-        recipients = add_subscribed_users(recipients, note.noteable)
-        recipients = reject_unsubscribed_users(recipients, note.noteable)
-        recipients = reject_users_without_access(recipients, note.noteable)
+        recipients = add_subscribed_users(recipients)
+        recipients = reject_unsubscribed_users(recipients)
+        recipients = reject_users_without_access(recipients)
 
         recipients.delete(note.author) unless note.author.notified_of_own_activity?
         recipients.uniq

From 734e21054a71be06aec897404d95b0905e6ed6c7 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 12:37:36 -0700
Subject: [PATCH 08/43] move build_custom_key to Default

---
 app/services/notification_recipient_service.rb | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index edad4ff021a..3b7a13ffed6 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -219,12 +219,6 @@ class NotificationRecipientService
 
         recipients
       end
-
-      # Build event key to search on custom notification level
-      # Check NotificationSetting::EMAIL_EVENTS
-      def build_custom_key(action, object)
-        "#{action}_#{object.class.model_name.name.underscore}".to_sym
-      end
     end
 
     class Default < Base
@@ -244,8 +238,6 @@ class NotificationRecipientService
       end
 
       def build
-        custom_action = build_custom_key(action, target)
-
         recipients = participants(current_user)
         recipients = add_project_watchers(recipients)
         recipients = add_custom_notifications(recipients, custom_action)
@@ -278,6 +270,12 @@ class NotificationRecipientService
 
         recipients.uniq
       end
+
+      # Build event key to search on custom notification level
+      # Check NotificationSetting::EMAIL_EVENTS
+      def custom_action
+        @custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
+      end
     end
 
     class Pipeline < Base

From cdc4b4d7a0ef75d5ecb5e7e8e69d35cf2a3bcf1c Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 20 Jul 2017 16:10:01 -0700
Subject: [PATCH 09/43] make recipients mutative during the build

---
 .../notification_recipient_service.rb         | 137 ++++++++++--------
 1 file changed, 73 insertions(+), 64 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 3b7a13ffed6..0bada20444d 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -34,24 +34,35 @@ class NotificationRecipientService
         raise 'abstract'
       end
 
+      def recipients
+        @recipients ||= []
+      end
+
+      def to_a
+        return recipients if @already_built
+        @already_built = true
+        build
+        recipients.uniq!
+        recipients.freeze
+        recipients
+      end
+
       # Remove users with disabled notifications from array
       # Also remove duplications and nil recipients
-      def reject_muted_users(users)
-        reject_users(users, :disabled)
+      def reject_muted_users
+        reject_users(:disabled)
       end
 
       protected
 
-      # Ensure that if we modify this array, we aren't modifying the memoised
-      # participants on the target.
-      def participants(user)
+      def add_participants(user)
         return unless target.respond_to?(:participants)
 
-        target.participants(user).dup
+        recipients.concat(target.participants(user))
       end
 
       # Get project/group users with CUSTOM notification level
-      def add_custom_notifications(recipients, action)
+      def add_custom_notifications(action)
         user_ids = []
 
         # Users with a notification setting on group or project
@@ -68,8 +79,9 @@ class NotificationRecipientService
         recipients.concat(User.find(user_ids))
       end
 
-      def add_project_watchers(recipients)
-        recipients.concat(project_watchers).compact
+      def add_project_watchers
+        recipients.concat(project_watchers)
+        recipients.compact!
       end
 
       # Get project users with WATCH notification level
@@ -88,14 +100,14 @@ class NotificationRecipientService
       end
 
       # Remove users with notification level 'Mentioned'
-      def reject_mention_users(users)
-        reject_users(users, :mention)
+      def reject_mention_users
+        reject_users(:mention)
       end
 
-      def add_subscribed_users(recipients)
-        return recipients unless target.respond_to? :subscribers
+      def add_subscribed_users
+        return unless target.respond_to? :subscribers
 
-        recipients + target.subscribers(project)
+        recipients.concat(target.subscribers(project))
       end
 
       def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
@@ -167,34 +179,35 @@ class NotificationRecipientService
       # Reject users which has certain notification level
       #
       # Example:
-      #   reject_users(users, :watch, project)
+      #   reject_users(:watch, project)
       #
-      def reject_users(users, level)
+      def reject_users(level)
         level = level.to_s
 
         unless NotificationSetting.levels.keys.include?(level)
           raise 'Invalid notification level'
         end
 
-        users = users.to_a.compact.uniq
+        recipients.compact!
+        recipients.uniq!
 
-        users.reject do |user|
+        recipients.reject! do |user|
           setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
           setting.present? && setting.level == level
         end
       end
 
-      def reject_unsubscribed_users(recipients)
-        return recipients unless target.respond_to? :subscriptions
+      def reject_unsubscribed_users
+        return unless target.respond_to? :subscriptions
 
-        recipients.reject do |user|
+        recipients.reject! do |user|
           subscription = target.subscriptions.find_by_user_id(user.id)
           subscription && !subscription.subscribed
         end
       end
 
-      def reject_users_without_access(recipients)
-        recipients = recipients.select { |u| u.can?(:receive_notifications) }
+      def reject_users_without_access
+        recipients.select! { |u| u.can?(:receive_notifications) }
 
         ability = case target
                   when Issuable
@@ -203,21 +216,19 @@ class NotificationRecipientService
                     :read_build # We have build trace in pipeline emails
                   end
 
-        return recipients unless ability
+        return unless ability
 
-        recipients.select do |user|
+        recipients.select! do |user|
           user.can?(ability, target)
         end
       end
 
-      def add_labels_subscribers(recipients, labels: nil)
-        return recipients unless target.respond_to? :labels
+      def add_labels_subscribers(labels: nil)
+        return unless target.respond_to? :labels
 
         (labels || target.labels).each do |label|
-          recipients += label.subscribers(project)
+          recipients.concat(label.subscribers(project))
         end
-
-        recipients
       end
     end
 
@@ -238,10 +249,10 @@ class NotificationRecipientService
       end
 
       def build
-        recipients = participants(current_user)
-        recipients = add_project_watchers(recipients)
-        recipients = add_custom_notifications(recipients, custom_action)
-        recipients = reject_mention_users(recipients)
+        add_participants(current_user)
+        add_project_watchers
+        add_custom_notifications(custom_action)
+        reject_mention_users
 
         # Re-assign is considered as a mention of the new assignee so we add the
         # new assignee to the list of recipients after we rejected users with
@@ -256,19 +267,17 @@ class NotificationRecipientService
           recipients.concat(target.assignees)
         end
 
-        recipients = reject_muted_users(recipients)
-        recipients = add_subscribed_users(recipients)
+        reject_muted_users
+        add_subscribed_users
 
         if [:new_issue, :new_merge_request].include?(custom_action)
-          recipients = add_labels_subscribers(recipients)
+          add_labels_subscribers
         end
 
-        recipients = reject_unsubscribed_users(recipients)
-        recipients = reject_users_without_access(recipients)
+        reject_unsubscribed_users
+        reject_users_without_access
 
         recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
-
-        recipients.uniq
       end
 
       # Build event key to search on custom notification level
@@ -303,13 +312,14 @@ class NotificationRecipientService
 
         notification_setting = NotificationRecipientService.notification_setting_for_user_project(current_user, target.project)
 
-        return [] if notification_setting.mention? || notification_setting.disabled?
+        return if notification_setting.mention? || notification_setting.disabled?
 
-        return [] if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
+        return if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
 
-        return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
+        return if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
 
-        reject_users_without_access([current_user])
+        recipients << current_user
+        reject_users_without_access
       end
     end
 
@@ -326,11 +336,10 @@ class NotificationRecipientService
       end
 
       def build
-        recipients = add_labels_subscribers([], labels: labels)
-        recipients = reject_unsubscribed_users(recipients)
-        recipients = reject_users_without_access(recipients)
+        add_labels_subscribers(labels: labels)
+        reject_unsubscribed_users
+        reject_users_without_access
         recipients.delete(current_user) unless current_user.notified_of_own_activity?
-        recipients.uniq
       end
     end
 
@@ -344,7 +353,7 @@ class NotificationRecipientService
         @target = note.noteable
       end
 
-      def build(note)
+      def build
         ability, subject = if note.for_personal_snippet?
                              [:read_personal_snippet, note.noteable]
                            else
@@ -354,45 +363,45 @@ class NotificationRecipientService
         mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
 
         # Add all users participating in the thread (author, assignee, comment authors)
-        recipients = participants(note.author) || mentioned_users
+        add_participants(note.author)
+        recipients.concat(mentioned_users) if recipients.empty?
 
         unless note.for_personal_snippet?
           # Merge project watchers
-          recipients = add_project_watchers(recipients)
+          add_project_watchers
 
           # Merge project with custom notification
-          recipients = add_custom_notifications(recipients, :new_note)
+          add_custom_notifications(:new_note)
         end
 
         # Reject users with Mention notification level, except those mentioned in _this_ note.
-        recipients = reject_mention_users(recipients - mentioned_users)
-        recipients = recipients + mentioned_users
+        reject_mention_users
+        recipients.concat(mentioned_users)
 
-        recipients = reject_muted_users(recipients)
+        reject_muted_users
 
-        recipients = add_subscribed_users(recipients)
-        recipients = reject_unsubscribed_users(recipients)
-        recipients = reject_users_without_access(recipients)
+        add_subscribed_users
+        reject_unsubscribed_users
+        reject_users_without_access
 
         recipients.delete(note.author) unless note.author.notified_of_own_activity?
-        recipients.uniq
       end
     end
   end
 
   def build_recipients(*a)
-    Builder::Default.new(@project, *a).build
+    Builder::Default.new(@project, *a).to_a
   end
 
   def build_pipeline_recipients(*a)
-    Builder::Pipeline.new(@project, *a).build
+    Builder::Pipeline.new(@project, *a).to_a
   end
 
   def build_relabeled_recipients(*a)
-    Builder::Relabeled.new(@project, *a).build
+    Builder::Relabeled.new(@project, *a).to_a
   end
 
   def build_new_note_recipients(*a)
-    Builder::NewNote.new(@project, *a).build
+    Builder::NewNote.new(@project, *a).to_a
   end
 end

From b05e6efd535990c8a54f470faa319e4b59ec12a3 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 12:55:04 -0700
Subject: [PATCH 10/43] add builder helpers with levels and start to separate
 out #filter!

---
 .../notification_recipient_service.rb         | 167 ++++++++++++------
 spec/services/notification_service_spec.rb    |   5 +
 2 files changed, 118 insertions(+), 54 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 0bada20444d..8d15202ef1c 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -20,13 +20,35 @@ class NotificationRecipientService
     @project = project
   end
 
+  class Recipient
+    attr_reader :user, :type
+    def initialize(builder, user, type)
+      @builder = builder
+      @user = user
+      @type = type
+    end
+
+    def notification_setting
+      @notification_setting ||=
+        NotificationRecipientService.notification_setting_for_user_project(user, @builder.project)
+    end
+
+    def notification_level
+      notification_setting&.level&.to_sym
+    end
+  end
+
   module Builder
     class Base
       def initialize(*)
         raise 'abstract'
       end
 
-      def build
+      def build!
+        raise 'abstract'
+      end
+
+      def filter!
         raise 'abstract'
       end
 
@@ -38,13 +60,22 @@ class NotificationRecipientService
         @recipients ||= []
       end
 
-      def to_a
-        return recipients if @already_built
-        @already_built = true
-        build
-        recipients.uniq!
-        recipients.freeze
-        recipients
+      def <<(arg)
+        users, type = arg
+        users = Array(users)
+        users.compact!
+        recipients.concat(users.map { |u| Recipient.new(self, u, type) })
+      end
+
+      def recipient_users
+        @recipient_users ||=
+          begin
+            build!
+            filter!
+            users = recipients.map(&:user)
+            users.uniq!
+            users.freeze
+          end
       end
 
       # Remove users with disabled notifications from array
@@ -53,12 +84,22 @@ class NotificationRecipientService
         reject_users(:disabled)
       end
 
+      def read_ability
+        @read_ability ||=
+          case target
+          when Issuable
+            :"read_#{target.to_ability_name}"
+          when Ci::Pipeline
+            :read_build # We have build trace in pipeline emails
+          end
+      end
+
       protected
 
       def add_participants(user)
         return unless target.respond_to?(:participants)
 
-        recipients.concat(target.participants(user))
+        self << [target.participants(user), :participating]
       end
 
       # Get project/group users with CUSTOM notification level
@@ -76,12 +117,11 @@ class 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, action)
 
-        recipients.concat(User.find(user_ids))
+        self << [User.find(user_ids), :watch]
       end
 
       def add_project_watchers
-        recipients.concat(project_watchers)
-        recipients.compact!
+        self << [project_watchers, :watch]
       end
 
       # Get project users with WATCH notification level
@@ -101,13 +141,17 @@ class NotificationRecipientService
 
       # Remove users with notification level 'Mentioned'
       def reject_mention_users
-        reject_users(:mention)
+        recipients.select! do |r|
+          next true if r.type == :mention
+          next true if r.type == :subscription
+          r.notification_level != :mention
+        end
       end
 
       def add_subscribed_users
         return unless target.respond_to? :subscribers
 
-        recipients.concat(target.subscribers(project))
+        self << [target.subscribers(project), :subscription]
       end
 
       def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
@@ -188,10 +232,9 @@ class NotificationRecipientService
           raise 'Invalid notification level'
         end
 
-        recipients.compact!
-        recipients.uniq!
+        recipients.reject! do |recipient|
+          user = recipient.user
 
-        recipients.reject! do |user|
           setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
           setting.present? && setting.level == level
         end
@@ -200,34 +243,34 @@ class NotificationRecipientService
       def reject_unsubscribed_users
         return unless target.respond_to? :subscriptions
 
-        recipients.reject! do |user|
+        recipients.reject! do |recipient|
+          user = recipient.user
           subscription = target.subscriptions.find_by_user_id(user.id)
           subscription && !subscription.subscribed
         end
       end
 
       def reject_users_without_access
-        recipients.select! { |u| u.can?(:receive_notifications) }
+        recipients.select! { |r| r.user.can?(:receive_notifications) }
 
-        ability = case target
-                  when Issuable
-                    :"read_#{target.to_ability_name}"
-                  when Ci::Pipeline
-                    :read_build # We have build trace in pipeline emails
-                  end
+        return unless read_ability
 
-        return unless ability
-
-        recipients.select! do |user|
-          user.can?(ability, target)
+        DeclarativePolicy.subject_scope do
+          recipients.select! do |recipient|
+            recipient.user.can?(read_ability, target)
+          end
         end
       end
 
+      def reject_user(user)
+        recipients.reject! { |r| r.user == user }
+      end
+
       def add_labels_subscribers(labels: nil)
         return unless target.respond_to? :labels
 
         (labels || target.labels).each do |label|
-          recipients.concat(label.subscribers(project))
+          self << [label.subscribers(project), :subscription]
         end
       end
     end
@@ -248,7 +291,7 @@ class NotificationRecipientService
         @skip_current_user = skip_current_user
       end
 
-      def build
+      def build!
         add_participants(current_user)
         add_project_watchers
         add_custom_notifications(custom_action)
@@ -259,12 +302,12 @@ class NotificationRecipientService
         # the "on mention" notification level
         case custom_action
         when :reassign_merge_request
-          recipients << previous_assignee if previous_assignee
-          recipients << target.assignee
+          self << [previous_assignee, :mention]
+          self << [target.assignee, :mention]
         when :reassign_issue
           previous_assignees = Array(previous_assignee)
-          recipients.concat(previous_assignees)
-          recipients.concat(target.assignees)
+          self << [previous_assignees, :mention]
+          self << [target.assignees, :mention]
         end
 
         reject_muted_users
@@ -273,13 +316,16 @@ class NotificationRecipientService
         if [:new_issue, :new_merge_request].include?(custom_action)
           add_labels_subscribers
         end
+      end
 
+      def filter!
         reject_unsubscribed_users
         reject_users_without_access
 
-        recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
+        reject_user(current_user) if skip_current_user && !current_user.notified_of_own_activity?
       end
 
+      private
       # Build event key to search on custom notification level
       # Check NotificationSetting::EMAIL_EVENTS
       def custom_action
@@ -299,7 +345,7 @@ class NotificationRecipientService
         @action = action
       end
 
-      def build
+      def build!
         return [] unless current_user
 
         custom_action =
@@ -318,7 +364,10 @@ class NotificationRecipientService
 
         return if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
 
-        recipients << current_user
+        self << [current_user, :subscriber]
+      end
+
+      def filter!
         reject_users_without_access
       end
     end
@@ -335,11 +384,14 @@ class NotificationRecipientService
         @labels = labels
       end
 
-      def build
+      def build!
         add_labels_subscribers(labels: labels)
+      end
+
+      def filter!
         reject_unsubscribed_users
         reject_users_without_access
-        recipients.delete(current_user) unless current_user.notified_of_own_activity?
+        reject_user(current_user) unless current_user.notified_of_own_activity?
       end
     end
 
@@ -353,18 +405,22 @@ class NotificationRecipientService
         @target = note.noteable
       end
 
-      def build
-        ability, subject = if note.for_personal_snippet?
-                             [:read_personal_snippet, note.noteable]
-                           else
-                             [:read_project, note.project]
-                           end
+      def read_ability
+        @read_ability ||=
+          case target
+          when Commit then nil
+          else :"read_#{target.class.model_name.name.underscore}"
+          end
+      end
 
-        mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
+      def subject
+        note.for_personal_snippet? ? note.noteable : note.project
+      end
 
+      def build!
         # Add all users participating in the thread (author, assignee, comment authors)
         add_participants(note.author)
-        recipients.concat(mentioned_users) if recipients.empty?
+        self << [note.mentioned_users, :mention] if recipients.empty?
 
         unless note.for_personal_snippet?
           # Merge project watchers
@@ -376,32 +432,35 @@ class NotificationRecipientService
 
         # Reject users with Mention notification level, except those mentioned in _this_ note.
         reject_mention_users
-        recipients.concat(mentioned_users)
+        self << [note.mentioned_users, :mention]
 
         reject_muted_users
 
         add_subscribed_users
+      end
+
+      def filter!
         reject_unsubscribed_users
         reject_users_without_access
 
-        recipients.delete(note.author) unless note.author.notified_of_own_activity?
+        reject_user(note.author) unless note.author.notified_of_own_activity?
       end
     end
   end
 
   def build_recipients(*a)
-    Builder::Default.new(@project, *a).to_a
+    Builder::Default.new(@project, *a).recipient_users
   end
 
   def build_pipeline_recipients(*a)
-    Builder::Pipeline.new(@project, *a).to_a
+    Builder::Pipeline.new(@project, *a).recipient_users
   end
 
   def build_relabeled_recipients(*a)
-    Builder::Relabeled.new(@project, *a).to_a
+    Builder::Relabeled.new(@project, *a).recipient_users
   end
 
   def build_new_note_recipients(*a)
-    Builder::NewNote.new(@project, *a).to_a
+    Builder::NewNote.new(@project, *a).recipient_users
   end
 end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 882ee7751b5..45d5d0ac9e0 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -309,6 +309,11 @@ describe NotificationService do
 
       before do
         build_team(note.project)
+
+        # make sure these users can read the project snippet!
+        project.add_guest(@u_guest_watcher)
+        project.add_guest(@u_guest_custom)
+
         note.project.add_master(note.author)
         reset_delivered_emails!
       end

From 943baa13922a5a2b7aab5a46765846996f23486b Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 12:55:13 -0700
Subject: [PATCH 11/43] move filtering logic into Recipient class

---
 .../notification_recipient_service.rb         | 201 +++++++++---------
 1 file changed, 102 insertions(+), 99 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 8d15202ef1c..3d3ace29c16 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -33,9 +33,69 @@ class NotificationRecipientService
         NotificationRecipientService.notification_setting_for_user_project(user, @builder.project)
     end
 
-    def notification_level
+    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
+      # as :disabled.
+      @notification_level ||=
+        case raw_notification_level
+        when :custom
+          notification_setting.event_enabled?(@builder.custom_action) ? :watch : :custom
+        else
+          raw_notification_level
+        end
+    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
+
+      return false if %i[watch participating].include?(notification_level) && excluded_watcher_action?
+
+      return false unless NotificationSetting.levels[notification_level] <= NotificationSetting.levels[type]
+
+      return false if unsubscribed?
+
+      true
+    end
+
+    def unsubscribed?
+      return false unless @builder.target.respond_to?(:subscriptions)
+
+      subscription = @builder.target.subscriptions.find_by_user_id(@user.id)
+      subscription && !subscription.subscribed
+    end
+
+    def own_activity?
+      return false unless @builder.acting_user
+      return false if @builder.acting_user.notified_of_own_activity?
+
+      user == @builder.acting_user
+    end
+
+    def has_access?
+      return false unless user.can?(:receive_notifications)
+      return true unless @builder.read_ability
+
+      DeclarativePolicy.subject_scope do
+        user.can?(@builder.read_ability, @builder.target)
+      end
+    end
+
+    def excluded_watcher_action?
+      return false if raw_notification_level == :custom
+
+      NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@builder.custom_action)
+    end
   end
 
   module Builder
@@ -49,7 +109,11 @@ class NotificationRecipientService
       end
 
       def filter!
-        raise 'abstract'
+        recipients.select!(&:notifiable?)
+      end
+
+      def acting_user
+        current_user
       end
 
       def target
@@ -78,12 +142,6 @@ class NotificationRecipientService
           end
       end
 
-      # Remove users with disabled notifications from array
-      # Also remove duplications and nil recipients
-      def reject_muted_users
-        reject_users(:disabled)
-      end
-
       def read_ability
         @read_ability ||=
           case target
@@ -99,23 +157,23 @@ class NotificationRecipientService
       def add_participants(user)
         return unless target.respond_to?(:participants)
 
-        self << [target.participants(user), :participating]
+        self << [target.participants(user), :watch]
       end
 
       # Get project/group users with CUSTOM notification level
-      def add_custom_notifications(action)
+      def add_custom_notifications
         user_ids = []
 
         # Users with a notification setting on group or project
-        user_ids += user_ids_notifiable_on(project, :custom, action)
-        user_ids += user_ids_notifiable_on(project.group, :custom, action)
+        user_ids += user_ids_notifiable_on(project, :custom)
+        user_ids += user_ids_notifiable_on(project.group, :custom)
 
         # Users with global level custom
         user_ids_with_project_level_global = user_ids_notifiable_on(project, :global)
         user_ids_with_group_level_global   = user_ids_notifiable_on(project.group, :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, action)
+        user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action)
 
         self << [User.find(user_ids), :watch]
       end
@@ -139,31 +197,22 @@ class NotificationRecipientService
         User.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq).to_a
       end
 
-      # Remove users with notification level 'Mentioned'
-      def reject_mention_users
-        recipients.select! do |r|
-          next true if r.type == :mention
-          next true if r.type == :subscription
-          r.notification_level != :mention
-        end
-      end
-
       def add_subscribed_users
         return unless target.respond_to? :subscribers
 
         self << [target.subscribers(project), :subscription]
       end
 
-      def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
+      def user_ids_notifiable_on(resource, notification_level = nil)
         return [] unless resource
 
+        scope = resource.notification_settings
+
         if notification_level
-          settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level])
-          settings = settings.select { |setting| setting.event_enabled?(action) } if action.present?
-          settings.map(&:user_id)
-        else
-          resource.notification_settings.pluck(:user_id)
+          scope = scope.where(level: NotificationSetting.levels[notification_level])
         end
+
+        scope.pluck(:user_id)
       end
 
       # Build a list of user_ids based on project notification settings
@@ -220,26 +269,6 @@ class NotificationRecipientService
         )
       end
 
-      # Reject users which has certain notification level
-      #
-      # Example:
-      #   reject_users(:watch, project)
-      #
-      def reject_users(level)
-        level = level.to_s
-
-        unless NotificationSetting.levels.keys.include?(level)
-          raise 'Invalid notification level'
-        end
-
-        recipients.reject! do |recipient|
-          user = recipient.user
-
-          setting = NotificationRecipientService.notification_setting_for_user_project(user, project)
-          setting.present? && setting.level == level
-        end
-      end
-
       def reject_unsubscribed_users
         return unless target.respond_to? :subscriptions
 
@@ -294,12 +323,9 @@ class NotificationRecipientService
       def build!
         add_participants(current_user)
         add_project_watchers
-        add_custom_notifications(custom_action)
-        reject_mention_users
+        add_custom_notifications
 
-        # Re-assign is considered as a mention of the new assignee so we add the
-        # new assignee to the list of recipients after we rejected users with
-        # the "on mention" notification level
+        # Re-assign is considered as a mention of the new assignee
         case custom_action
         when :reassign_merge_request
           self << [previous_assignee, :mention]
@@ -310,7 +336,6 @@ class NotificationRecipientService
           self << [target.assignees, :mention]
         end
 
-        reject_muted_users
         add_subscribed_users
 
         if [:new_issue, :new_merge_request].include?(custom_action)
@@ -318,14 +343,10 @@ class NotificationRecipientService
         end
       end
 
-      def filter!
-        reject_unsubscribed_users
-        reject_users_without_access
-
-        reject_user(current_user) if skip_current_user && !current_user.notified_of_own_activity?
+      def acting_user
+        current_user if skip_current_user
       end
 
-      private
       # Build event key to search on custom notification level
       # Check NotificationSetting::EMAIL_EVENTS
       def custom_action
@@ -345,30 +366,23 @@ class NotificationRecipientService
         @action = action
       end
 
+      def acting_user
+        nil
+      end
+
+      def custom_action
+        case action.to_s
+        when 'failed'
+          :failed_pipeline
+        when 'success'
+          :success_pipeline
+        end
+      end
+
       def build!
         return [] unless current_user
 
-        custom_action =
-          case action.to_s
-          when 'failed'
-            :failed_pipeline
-          when 'success'
-            :success_pipeline
-          end
-
-        notification_setting = NotificationRecipientService.notification_setting_for_user_project(current_user, target.project)
-
-        return if notification_setting.mention? || notification_setting.disabled?
-
-        return if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
-
-        return if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
-
-        self << [current_user, :subscriber]
-      end
-
-      def filter!
-        reject_users_without_access
+        self << [current_user, :watch]
       end
     end
 
@@ -387,12 +401,6 @@ class NotificationRecipientService
       def build!
         add_labels_subscribers(labels: labels)
       end
-
-      def filter!
-        reject_unsubscribed_users
-        reject_users_without_access
-        reject_user(current_user) unless current_user.notified_of_own_activity?
-      end
     end
 
     class NewNote < Base
@@ -420,30 +428,25 @@ class NotificationRecipientService
       def build!
         # Add all users participating in the thread (author, assignee, comment authors)
         add_participants(note.author)
-        self << [note.mentioned_users, :mention] if recipients.empty?
+        self << [note.mentioned_users, :mention]
 
         unless note.for_personal_snippet?
           # Merge project watchers
           add_project_watchers
 
           # Merge project with custom notification
-          add_custom_notifications(:new_note)
+          add_custom_notifications
         end
 
-        # Reject users with Mention notification level, except those mentioned in _this_ note.
-        reject_mention_users
-        self << [note.mentioned_users, :mention]
-
-        reject_muted_users
-
         add_subscribed_users
       end
 
-      def filter!
-        reject_unsubscribed_users
-        reject_users_without_access
+      def custom_action
+        :new_note
+      end
 
-        reject_user(note.author) unless note.author.notified_of_own_activity?
+      def acting_user
+        note.author
       end
     end
   end

From b69e81c3e993939228046516be897233dcaf4442 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 25 Jul 2017 22:22:31 -0700
Subject: [PATCH 12/43] rm the @builder argument and factor out
 .notifiable_users

---
 .../notification_recipient_service.rb         | 50 ++++++++++++++-----
 app/services/notification_service.rb          |  3 +-
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 3d3ace29c16..f944c1ae7f4 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -21,16 +21,25 @@ class NotificationRecipientService
   end
 
   class Recipient
+    def self.notifiable_users(users, *args)
+      users.map { |u| new(u, *args) }.select(&:notifiable?).map(&:user)
+    end
+
     attr_reader :user, :type
-    def initialize(builder, user, type)
-      @builder = builder
+    def initialize(user, project, type,
+                   custom_action: nil, target: nil, acting_user: nil, read_ability: nil)
+      @project = project
+      @custom_action = custom_action
+      @acting_user = acting_user
+      @read_ability = read_ability
+      @target = target
       @user = user
       @type = type
     end
 
     def notification_setting
       @notification_setting ||=
-        NotificationRecipientService.notification_setting_for_user_project(user, @builder.project)
+        NotificationRecipientService.notification_setting_for_user_project(user, @project)
     end
 
     def raw_notification_level
@@ -43,7 +52,11 @@ class NotificationRecipientService
       @notification_level ||=
         case raw_notification_level
         when :custom
-          notification_setting.event_enabled?(@builder.custom_action) ? :watch : :custom
+          if !@custom_action || notification_setting.event_enabled?(@custom_action)
+            :watch
+          else
+            :custom
+          end
         else
           raw_notification_level
         end
@@ -69,32 +82,34 @@ class NotificationRecipientService
     end
 
     def unsubscribed?
-      return false unless @builder.target.respond_to?(:subscriptions)
+      return false unless @target
+      return false unless @target.respond_to?(:subscriptions)
 
-      subscription = @builder.target.subscriptions.find_by_user_id(@user.id)
+      subscription = @target.subscriptions.find_by_user_id(@user.id)
       subscription && !subscription.subscribed
     end
 
     def own_activity?
-      return false unless @builder.acting_user
-      return false if @builder.acting_user.notified_of_own_activity?
+      return false unless @acting_user
+      return false if @acting_user.notified_of_own_activity?
 
-      user == @builder.acting_user
+      user == @acting_user
     end
 
     def has_access?
       return false unless user.can?(:receive_notifications)
-      return true unless @builder.read_ability
+      return true unless @read_ability
 
       DeclarativePolicy.subject_scope do
-        user.can?(@builder.read_ability, @builder.target)
+        user.can?(@read_ability, @target)
       end
     end
 
     def excluded_watcher_action?
+      return false unless @custom_action
       return false if raw_notification_level == :custom
 
-      NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@builder.custom_action)
+      NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
     end
   end
 
@@ -128,7 +143,16 @@ class NotificationRecipientService
         users, type = arg
         users = Array(users)
         users.compact!
-        recipients.concat(users.map { |u| Recipient.new(self, u, type) })
+        recipients.concat(users.map { |u| make_recipient(u, type) })
+      end
+
+      def make_recipient(user, type)
+        Recipient.new(user, project, type,
+          custom_action: custom_action,
+          target: target,
+          acting_user: acting_user,
+          read_ability: read_ability
+        )
       end
 
       def recipient_users
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index b94921d2a08..a094d97a295 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -270,8 +270,7 @@ class NotificationService
   end
 
   def project_was_moved(project, old_path_with_namespace)
-    recipients = project.team.members
-    recipients = NotificationRecipientService.new(project).reject_muted_users(recipients)
+    recipients = NotificationRecipientService::Recipient.notifiable_users(project.team.members, project, :watch)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(

From a4932d2da72af14eeea99e466117bdb767d111c7 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 25 Jul 2017 22:24:22 -0700
Subject: [PATCH 13/43] don't elevate to :watch if no @custom_action is
 provided

---
 app/services/notification_recipient_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index f944c1ae7f4..20d8a06903c 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -52,7 +52,7 @@ class NotificationRecipientService
       @notification_level ||=
         case raw_notification_level
         when :custom
-          if !@custom_action || notification_setting.event_enabled?(@custom_action)
+          if @custom_action && notification_setting.event_enabled?(@custom_action)
             :watch
           else
             :custom

From 4caa5a7e14a8aa1e973662bd1394468eac74f3ad Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 08:25:29 -0700
Subject: [PATCH 14/43] factor out .notifiable_users

---
 .../notification_recipient_service.rb         | 40 ++-----------------
 app/services/notification_service.rb          |  9 ++---
 2 files changed, 8 insertions(+), 41 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 20d8a06903c..3d36660495f 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -378,38 +378,6 @@ class NotificationRecipientService
       end
     end
 
-    class Pipeline < Base
-      attr_reader :project
-      attr_reader :target
-      attr_reader :current_user
-      attr_reader :action
-      def initialize(project, target, current_user, action:)
-        @project = project
-        @target = target
-        @current_user = current_user
-        @action = action
-      end
-
-      def acting_user
-        nil
-      end
-
-      def custom_action
-        case action.to_s
-        when 'failed'
-          :failed_pipeline
-        when 'success'
-          :success_pipeline
-        end
-      end
-
-      def build!
-        return [] unless current_user
-
-        self << [current_user, :watch]
-      end
-    end
-
     class Relabeled < Base
       attr_reader :project
       attr_reader :target
@@ -475,12 +443,12 @@ class NotificationRecipientService
     end
   end
 
-  def build_recipients(*a)
-    Builder::Default.new(@project, *a).recipient_users
+  def self.notifiable_users(*a)
+    Recipient.notifiable_users(*a)
   end
 
-  def build_pipeline_recipients(*a)
-    Builder::Pipeline.new(@project, *a).recipient_users
+  def build_recipients(*a)
+    Builder::Default.new(@project, *a).recipient_users
   end
 
   def build_relabeled_recipients(*a)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index a094d97a295..8a6ed88923d 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -270,7 +270,7 @@ class NotificationService
   end
 
   def project_was_moved(project, old_path_with_namespace)
-    recipients = NotificationRecipientService::Recipient.notifiable_users(project.team.members, project, :watch)
+    recipients = NotificationRecipientService.notifiable_users(project.team.members, project, :mention)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(
@@ -304,10 +304,9 @@ class NotificationService
 
     return unless mailer.respond_to?(email_template)
 
-    recipients ||= NotificationRecipientService.new(pipeline.project).build_pipeline_recipients(
-      pipeline,
-      pipeline.user,
-      action: pipeline.status
+    recipients ||= NotificationRecipientService.notifiable_users(
+      [pipeline.user], pipeline.project, :watch,
+      custom_action: :"#{pipeline.status}_pipeline"
     ).map(&:notification_email)
 
     if recipients.any?

From 6aa44382acbe1439c48da056ab9089e36124878f Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 08:36:14 -0700
Subject: [PATCH 15/43] rm a now-useless spec

---
 .../notification_recipient_service_spec.rb    | 34 -------------------
 1 file changed, 34 deletions(-)
 delete mode 100644 spec/services/notification_recipient_service_spec.rb

diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb
deleted file mode 100644
index 0eb0771fd29..00000000000
--- a/spec/services/notification_recipient_service_spec.rb
+++ /dev/null
@@ -1,34 +0,0 @@
-require 'spec_helper'
-
-describe NotificationRecipientService do
-  set(:user) { create(:user) }
-  set(:project) { create(:project, :public) }
-  set(:issue) { create(:issue, project: project) }
-
-  set(:watcher) do
-    watcher = create(:user)
-    setting = watcher.notification_settings_for(project)
-    setting.level = :watch
-    setting.save
-
-    watcher
-  end
-
-  subject { described_class.new(project) }
-
-  describe '#build_recipients' do
-    it 'does not modify the participants of the target' do
-      expect { subject.build_recipients(issue, user, action: :new_issue) }
-        .not_to change { issue.participants(user) }
-    end
-  end
-
-  describe '#build_new_note_recipients' do
-    set(:note) { create(:note_on_issue, noteable: issue, project: project) }
-
-    it 'does not modify the participants of the target' do
-      expect { subject.build_new_note_recipients(note) }
-        .not_to change { note.noteable.participants(note.author) }
-    end
-  end
-end

From 618b5d34463b1a54cca0d47d3d815b7e388a1db2 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 08:36:49 -0700
Subject: [PATCH 16/43] move the #build_* methods to static, parameterize the
 project

---
 .../notification_recipient_service.rb         | 61 +++++++++----------
 app/services/notification_service.rb          | 24 +++++---
 2 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 3d36660495f..788e06502ee 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -1,23 +1,21 @@
 #
 # Used by NotificationService to determine who should receive notification
 #
-class NotificationRecipientService
-  attr_reader :project
-
-  def self.notification_setting_for_user_project(user, project)
-    project_setting = project && user.notification_settings_for(project)
-
-    return project_setting unless project_setting.nil? || project_setting.global?
-
-    group_setting = project&.group && user.notification_settings_for(project.group)
-
-    return group_setting unless group_setting.nil? || group_setting.global?
-
-    user.global_notification_setting
+module NotificationRecipientService
+  def self.notifiable_users(*a)
+    Recipient.notifiable_users(*a)
   end
 
-  def initialize(project)
-    @project = project
+  def self.build_recipients(*a)
+    Builder::Default.new(*a).recipient_users
+  end
+
+  def self.build_relabeled_recipients(*a)
+    Builder::Relabeled.new(*a).recipient_users
+  end
+
+  def self.build_new_note_recipients(*a)
+    Builder::NewNote.new(*a).recipient_users
   end
 
   class Recipient
@@ -38,8 +36,7 @@ class NotificationRecipientService
     end
 
     def notification_setting
-      @notification_setting ||=
-        NotificationRecipientService.notification_setting_for_user_project(user, @project)
+      @notification_setting ||= find_notification_setting
     end
 
     def raw_notification_level
@@ -111,6 +108,20 @@ class NotificationRecipientService
 
       NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
     end
+
+    private
+
+    def find_notification_setting
+      project_setting = project && user.notification_settings_for(project)
+
+      return project_setting unless project_setting.nil? || project_setting.global?
+
+      group_setting = project&.group && user.notification_settings_for(project.group)
+
+      return group_setting unless group_setting.nil? || group_setting.global?
+
+      user.global_notification_setting
+    end
   end
 
   module Builder
@@ -442,20 +453,4 @@ class NotificationRecipientService
       end
     end
   end
-
-  def self.notifiable_users(*a)
-    Recipient.notifiable_users(*a)
-  end
-
-  def build_recipients(*a)
-    Builder::Default.new(@project, *a).recipient_users
-  end
-
-  def build_relabeled_recipients(*a)
-    Builder::Relabeled.new(@project, *a).recipient_users
-  end
-
-  def build_new_note_recipients(*a)
-    Builder::NewNote.new(@project, *a).recipient_users
-  end
 end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 8a6ed88923d..675b4536a26 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -77,7 +77,8 @@ class NotificationService
   #  * users with custom level checked with "reassign issue"
   #
   def reassigned_issue(issue, current_user, previous_assignees = [])
-    recipients = NotificationRecipientService.new(issue.project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
+      issue.project,
       issue,
       current_user,
       action: "reassign",
@@ -177,7 +178,8 @@ class NotificationService
   end
 
   def resolve_all_discussions(merge_request, current_user)
-    recipients = NotificationRecipientService.new(merge_request.target_project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
+      merge_request.target_project,
       merge_request,
       current_user,
       action: "resolve_all_discussions")
@@ -202,7 +204,7 @@ class NotificationService
 
     notify_method = "note_#{note.to_ability_name}_email".to_sym
 
-    recipients = NotificationRecipientService.new(note.project).build_new_note_recipients(note)
+    recipients = NotificationRecipientService.build_new_note_recipients(note.project, note)
     recipients.each do |recipient|
       mailer.send(notify_method, recipient.id, note.id).deliver_later
     end
@@ -282,7 +284,7 @@ class NotificationService
   end
 
   def issue_moved(issue, new_issue, current_user)
-    recipients = NotificationRecipientService.new(issue.project).build_recipients(issue, current_user, action: 'moved')
+    recipients = NotificationRecipientService.build_recipients(issue.project, issue, current_user, action: 'moved')
 
     recipients.map do |recipient|
       email = mailer.issue_moved_email(recipient, issue, new_issue, current_user)
@@ -317,7 +319,7 @@ class NotificationService
   protected
 
   def new_resource_email(target, project, method)
-    recipients = NotificationRecipientService.new(project).build_recipients(target, target.author, action: "new")
+    recipients = NotificationRecipientService.build_recipients(project, target, target.author, action: "new")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.id, target.id).deliver_later
@@ -325,7 +327,7 @@ class NotificationService
   end
 
   def new_mentions_in_resource_email(target, project, new_mentioned_users, current_user, method)
-    recipients = NotificationRecipientService.new(project).build_recipients(target, current_user, action: "new")
+    recipients = NotificationRecipientService.build_recipients(project, target, current_user, action: "new")
     recipients = recipients & new_mentioned_users
 
     recipients.each do |recipient|
@@ -336,7 +338,8 @@ class NotificationService
   def close_resource_email(target, project, current_user, method, skip_current_user: true)
     action = method == :merged_merge_request_email ? "merge" : "close"
 
-    recipients = NotificationRecipientService.new(project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
+      project,
       target,
       current_user,
       action: action,
@@ -352,7 +355,8 @@ class NotificationService
     previous_assignee_id = previous_record(target, 'assignee_id')
     previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
 
-    recipients = NotificationRecipientService.new(project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
+      project,
       target,
       current_user,
       action: "reassign",
@@ -371,7 +375,7 @@ class NotificationService
   end
 
   def relabeled_resource_email(target, project, labels, current_user, method)
-    recipients = NotificationRecipientService.new(project).build_relabeled_recipients(target, current_user, labels: labels)
+    recipients = NotificationRecipientService.build_relabeled_recipients(project, target, current_user, labels: labels)
     label_names = labels.map(&:name)
 
     recipients.each do |recipient|
@@ -380,7 +384,7 @@ class NotificationService
   end
 
   def reopen_resource_email(target, project, current_user, method, status)
-    recipients = NotificationRecipientService.new(project).build_recipients(target, current_user, action: "reopen")
+    recipients = NotificationRecipientService.build_recipients(project, target, current_user, action: "reopen")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later

From 618a3d125c0faf3b6e0484bc9955ffd0a597ad33 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 08:43:27 -0700
Subject: [PATCH 17/43] move Recipient to its own NotificationRecipient file

---
 app/models/notification_recipient.rb          | 102 +++++++++++++++
 .../notification_recipient_service.rb         | 116 ++----------------
 2 files changed, 109 insertions(+), 109 deletions(-)
 create mode 100644 app/models/notification_recipient.rb

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
new file mode 100644
index 00000000000..0b2eee56ff8
--- /dev/null
+++ b/app/models/notification_recipient.rb
@@ -0,0 +1,102 @@
+class NotificationRecipient
+  attr_reader :user, :project, :type
+  def initialize(user, project, type,
+                 custom_action: nil, target: nil, acting_user: nil, read_ability: nil)
+    @project = project
+    @custom_action = custom_action
+    @acting_user = acting_user
+    @read_ability = read_ability
+    @target = target
+    @user = user
+    @type = type
+  end
+
+  def notification_setting
+    @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
+    # as :disabled.
+    @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
+  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
+
+    return false if %i[watch participating].include?(notification_level) && excluded_watcher_action?
+
+    return false unless NotificationSetting.levels[notification_level] <= NotificationSetting.levels[type]
+
+    return false if unsubscribed?
+
+    true
+  end
+
+  def unsubscribed?
+    return false unless @target
+    return false unless @target.respond_to?(:subscriptions)
+
+    subscription = @target.subscriptions.find_by_user_id(@user.id)
+    subscription && !subscription.subscribed
+  end
+
+  def own_activity?
+    return false unless @acting_user
+    return false if @acting_user.notified_of_own_activity?
+
+    user == @acting_user
+  end
+
+  def has_access?
+    return false unless user.can?(:receive_notifications)
+    return true unless @read_ability
+
+    DeclarativePolicy.subject_scope do
+      user.can?(@read_ability, @target)
+    end
+  end
+
+  def excluded_watcher_action?
+    return false unless @custom_action
+    return false if raw_notification_level == :custom
+
+    NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
+  end
+
+  private
+
+  def find_notification_setting
+    project_setting = @project && user.notification_settings_for(@project)
+
+    return project_setting unless project_setting.nil? || project_setting.global?
+
+    group_setting = @project&.group && user.notification_settings_for(@project.group)
+
+    return group_setting unless group_setting.nil? || group_setting.global?
+
+    user.global_notification_setting
+  end
+end
+
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 788e06502ee..5d8c3a39af1 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -2,8 +2,12 @@
 # Used by NotificationService to determine who should receive notification
 #
 module NotificationRecipientService
-  def self.notifiable_users(*a)
-    Recipient.notifiable_users(*a)
+  def self.notifiable_users(users, *args)
+    users.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
+  end
+
+  def self.notifiable?(user, *args)
+    NotificationRecipient.new(user, *args).notifiable?
   end
 
   def self.build_recipients(*a)
@@ -18,112 +22,6 @@ module NotificationRecipientService
     Builder::NewNote.new(*a).recipient_users
   end
 
-  class Recipient
-    def self.notifiable_users(users, *args)
-      users.map { |u| new(u, *args) }.select(&:notifiable?).map(&:user)
-    end
-
-    attr_reader :user, :type
-    def initialize(user, project, type,
-                   custom_action: nil, target: nil, acting_user: nil, read_ability: nil)
-      @project = project
-      @custom_action = custom_action
-      @acting_user = acting_user
-      @read_ability = read_ability
-      @target = target
-      @user = user
-      @type = type
-    end
-
-    def notification_setting
-      @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
-      # as :disabled.
-      @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
-    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
-
-      return false if %i[watch participating].include?(notification_level) && excluded_watcher_action?
-
-      return false unless NotificationSetting.levels[notification_level] <= NotificationSetting.levels[type]
-
-      return false if unsubscribed?
-
-      true
-    end
-
-    def unsubscribed?
-      return false unless @target
-      return false unless @target.respond_to?(:subscriptions)
-
-      subscription = @target.subscriptions.find_by_user_id(@user.id)
-      subscription && !subscription.subscribed
-    end
-
-    def own_activity?
-      return false unless @acting_user
-      return false if @acting_user.notified_of_own_activity?
-
-      user == @acting_user
-    end
-
-    def has_access?
-      return false unless user.can?(:receive_notifications)
-      return true unless @read_ability
-
-      DeclarativePolicy.subject_scope do
-        user.can?(@read_ability, @target)
-      end
-    end
-
-    def excluded_watcher_action?
-      return false unless @custom_action
-      return false if raw_notification_level == :custom
-
-      NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
-    end
-
-    private
-
-    def find_notification_setting
-      project_setting = project && user.notification_settings_for(project)
-
-      return project_setting unless project_setting.nil? || project_setting.global?
-
-      group_setting = project&.group && user.notification_settings_for(project.group)
-
-      return group_setting unless group_setting.nil? || group_setting.global?
-
-      user.global_notification_setting
-    end
-  end
-
   module Builder
     class Base
       def initialize(*)
@@ -158,7 +56,7 @@ module NotificationRecipientService
       end
 
       def make_recipient(user, type)
-        Recipient.new(user, project, type,
+        NotificationRecipient.new(user, project, type,
           custom_action: custom_action,
           target: target,
           acting_user: acting_user,

From 651f50ac00bdf14ae660af0fea3d463d07d8d25f Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 09:32:37 -0700
Subject: [PATCH 18/43] make sure #custom_action is always defined

---
 app/services/notification_recipient_service.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 5d8c3a39af1..b286cc373bd 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -85,6 +85,10 @@ module NotificationRecipientService
           end
       end
 
+      def custom_action
+        nil
+      end
+
       protected
 
       def add_participants(user)

From 46d0bfdcb982d76b8595921a956b088df1573905 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 09:33:01 -0700
Subject: [PATCH 19/43] use intersection and diff operators instead of each

---
 .../notification_recipient_service.rb         | 24 ++-----------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index b286cc373bd..6b4e97aaab4 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -157,35 +157,15 @@ module NotificationRecipientService
         user_ids = user_ids_notifiable_on(project, :watch)
 
         # If project setting is global, add to watch list if global setting is watch
-        global_setting.each do |user_id|
-          if user_ids_global_level_watch.include?(user_id)
-            user_ids << user_id
-          end
-        end
-
-        user_ids
+        user_ids + (global_setting & user_ids_global_level_watch)
       end
 
       # Build a list of user_ids based on group notification settings
       def select_group_members_ids(group, project_members, global_setting, user_ids_global_level_watch)
         uids = user_ids_notifiable_on(group, :watch)
 
-        # Group setting is watch, add to user_ids list if user is not project member
-        user_ids = []
-        uids.each do |user_id|
-          if project_members.exclude?(user_id)
-            user_ids << user_id
-          end
-        end
-
         # Group setting is global, add to user_ids list if global setting is watch
-        global_setting.each do |user_id|
-          if project_members.exclude?(user_id) && user_ids_global_level_watch.include?(user_id)
-            user_ids << user_id
-          end
-        end
-
-        user_ids
+        uids + (global_setting & user_ids_global_level_watch) - project_members
       end
 
       def user_ids_with_global_level_watch(ids)

From d45b4065d8e0b6cc893ccde66e0c37f459ca901b Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 26 Jul 2017 09:33:23 -0700
Subject: [PATCH 20/43] make sure users have to be able to read_pipeline

to get pipeline failed notifications
---
 app/services/notification_service.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 675b4536a26..c330d62a2ce 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -308,7 +308,8 @@ class NotificationService
 
     recipients ||= NotificationRecipientService.notifiable_users(
       [pipeline.user], pipeline.project, :watch,
-      custom_action: :"#{pipeline.status}_pipeline"
+      custom_action: :"#{pipeline.status}_pipeline",
+      read_ability: :read_pipeline,
     ).map(&:notification_email)
 
     if recipients.any?

From 3829d7245a446fe80745f5e7c082e005fc013365 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Fri, 28 Jul 2017 11:38:35 -0700
Subject: [PATCH 21/43] require that the user be able to :read_build

to get a pipeline_failed email
---
 app/services/notification_service.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index c330d62a2ce..e9a67cac4d6 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -309,7 +309,8 @@ class NotificationService
     recipients ||= NotificationRecipientService.notifiable_users(
       [pipeline.user], pipeline.project, :watch,
       custom_action: :"#{pipeline.status}_pipeline",
-      read_ability: :read_pipeline,
+      read_ability: :read_build,
+      target: pipeline
     ).map(&:notification_email)
 
     if recipients.any?

From 19309b970556797027e57c212d9b9aa053c36892 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 31 Jul 2017 09:30:35 -0700
Subject: [PATCH 22/43] default the project to target.project

---
 app/models/notification_recipient.rb           | 12 +++++++++---
 app/services/notification_recipient_service.rb |  3 ++-
 app/services/notification_service.rb           |  4 ++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 0b2eee56ff8..96e8e32c644 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -1,14 +1,20 @@
 class NotificationRecipient
   attr_reader :user, :project, :type
-  def initialize(user, project, type,
-                 custom_action: nil, target: nil, acting_user: nil, read_ability: nil)
-    @project = project
+  def initialize(user, type,
+                 custom_action: nil,
+                 target: nil,
+                 acting_user: nil,
+                 read_ability: nil,
+                 project: nil)
     @custom_action = custom_action
     @acting_user = acting_user
     @read_ability = read_ability
     @target = target
+    @project = project || @target&.project
     @user = user
     @type = type
+
+    raise ArgumentError, "Project is missing" if @project.nil?
   end
 
   def notification_setting
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 6b4e97aaab4..0627cca9214 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -56,7 +56,8 @@ module NotificationRecipientService
       end
 
       def make_recipient(user, type)
-        NotificationRecipient.new(user, project, type,
+        NotificationRecipient.new(user, type,
+          project: project,
           custom_action: custom_action,
           target: target,
           acting_user: acting_user,
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index e9a67cac4d6..f5366b9ceab 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -272,7 +272,7 @@ class NotificationService
   end
 
   def project_was_moved(project, old_path_with_namespace)
-    recipients = NotificationRecipientService.notifiable_users(project.team.members, project, :mention)
+    recipients = NotificationRecipientService.notifiable_users(project.team.members, :mention, project: project)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(
@@ -307,7 +307,7 @@ class NotificationService
     return unless mailer.respond_to?(email_template)
 
     recipients ||= NotificationRecipientService.notifiable_users(
-      [pipeline.user], pipeline.project, :watch,
+      [pipeline.user], :watch,
       custom_action: :"#{pipeline.status}_pipeline",
       read_ability: :read_build,
       target: pipeline

From 8386d69f8f4a75a45e0226f4ad89b7d200c6d84d Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 12:51:14 -0700
Subject: [PATCH 23/43] .notifiable_users: compact the passed-in users

---
 app/services/notification_recipient_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 0627cca9214..d412e5414b3 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -3,7 +3,7 @@
 #
 module NotificationRecipientService
   def self.notifiable_users(users, *args)
-    users.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
+    users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
   end
 
   def self.notifiable?(user, *args)

From e7d136ebdabf3f784412b859bf3ad7427c88e7f6 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 31 Jul 2017 11:32:17 -0700
Subject: [PATCH 24/43] don't require project

it's not there in the case of personal snippets, f. ex., and we've
already guarded against its being missing in #find_notification_setting
---
 app/models/notification_recipient.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 96e8e32c644..1856dfe5734 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -1,5 +1,5 @@
 class NotificationRecipient
-  attr_reader :user, :project, :type
+  attr_reader :user, :type
   def initialize(user, type,
                  custom_action: nil,
                  target: nil,
@@ -13,8 +13,6 @@ class NotificationRecipient
     @project = project || @target&.project
     @user = user
     @type = type
-
-    raise ArgumentError, "Project is missing" if @project.nil?
   end
 
   def notification_setting

From 18288fe21e12bb524757a97472008eabfca5a352 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 31 Jul 2017 15:04:42 -0700
Subject: [PATCH 25/43] style fixes

---
 app/models/notification_recipient.rb           | 15 ++++++++-------
 app/services/notification_recipient_service.rb |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 1856dfe5734..28ac22da6e2 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -1,11 +1,13 @@
 class NotificationRecipient
   attr_reader :user, :type
-  def initialize(user, type,
-                 custom_action: nil,
-                 target: nil,
-                 acting_user: nil,
-                 read_ability: nil,
-                 project: nil)
+  def initialize(
+    user, type,
+    custom_action: nil,
+    target: nil,
+    acting_user: nil,
+    read_ability: nil,
+    project: nil
+  )
     @custom_action = custom_action
     @acting_user = acting_user
     @read_ability = read_ability
@@ -103,4 +105,3 @@ class NotificationRecipient
     user.global_notification_setting
   end
 end
-
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index d412e5414b3..083b5f2d229 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -56,7 +56,8 @@ module NotificationRecipientService
       end
 
       def make_recipient(user, type)
-        NotificationRecipient.new(user, type,
+        NotificationRecipient.new(
+          user, type,
           project: project,
           custom_action: custom_action,
           target: target,

From 3c56e41402301f26bf70afe13d2565e74e4427eb Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 31 Jul 2017 17:09:01 -0700
Subject: [PATCH 26/43] use a simple pluck, since equivalent filtering happens
 later

---
 app/services/notification_recipient_service.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 083b5f2d229..7c9bb54f020 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -175,9 +175,7 @@ module NotificationRecipientService
       end
 
       def user_ids_with_global_level_custom(ids, action)
-        settings = settings_with_global_level_of(:custom, ids)
-        settings = settings.select { |setting| setting.event_enabled?(action) }
-        settings.map(&:user_id)
+        settings_with_global_level_of(:custom, ids).pluck(:user_id)
       end
 
       def settings_with_global_level_of(level, ids)

From 488e8e79dd85e973e1b562fe0320f69b2bedec06 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Mon, 31 Jul 2017 18:56:56 -0700
Subject: [PATCH 27/43] force queries to include notification settings

---
 app/services/notification_recipient_service.rb | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 7c9bb54f020..1ce92c8cbdc 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -50,11 +50,20 @@ module NotificationRecipientService
 
       def <<(arg)
         users, type = arg
+
+        if users.is_a?(ActiveRecord::Relation)
+          users = users.includes(:notification_settings)
+        end
+
         users = Array(users)
         users.compact!
         recipients.concat(users.map { |u| make_recipient(u, type) })
       end
 
+      def user_scope
+        User.includes(:notification_settings)
+      end
+
       def make_recipient(user, type)
         NotificationRecipient.new(
           user, type,
@@ -114,7 +123,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)
 
-        self << [User.find(user_ids), :watch]
+        self << [user_scope.where(id: user_ids), :watch]
       end
 
       def add_project_watchers
@@ -133,7 +142,7 @@ module NotificationRecipientService
         user_ids_with_project_setting = select_project_members_ids(project, user_ids_with_project_global, user_ids)
         user_ids_with_group_setting = select_group_members_ids(project.group, project_members_ids, user_ids_with_group_global, user_ids)
 
-        User.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq).to_a
+        user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq)
       end
 
       def add_subscribed_users

From c2dd4239c939e003dfe569196ec2d39e2478606e Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 10:42:54 -0700
Subject: [PATCH 28/43] short-circuit if there is no policy, and add
 :read_project check

---
 app/models/notification_recipient.rb           |  9 ++++++---
 app/services/notification_recipient_service.rb |  6 +-----
 lib/declarative_policy.rb                      | 13 +++++++++----
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 28ac22da6e2..837b62ec0cb 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -76,10 +76,13 @@ class NotificationRecipient
   end
 
   def has_access?
-    return false unless user.can?(:receive_notifications)
-    return true unless @read_ability
-
     DeclarativePolicy.subject_scope do
+      return false unless user.can?(:receive_notifications)
+      return false if @project && !user.can?(:read_project, @project)
+
+      return true unless @read_ability
+      return true unless DeclarativePolicy.has_policy?(@target)
+
       user.can?(@read_ability, @target)
     end
   end
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 1ce92c8cbdc..540e568fed2 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -308,11 +308,7 @@ module NotificationRecipientService
       end
 
       def read_ability
-        @read_ability ||=
-          case target
-          when Commit then nil
-          else :"read_#{target.class.model_name.name.underscore}"
-          end
+        @read_ability ||= :"read_#{target.class.model_name.name.underscore}"
       end
 
       def subject
diff --git a/lib/declarative_policy.rb b/lib/declarative_policy.rb
index b1eb1a6cef1..4936669a73a 100644
--- a/lib/declarative_policy.rb
+++ b/lib/declarative_policy.rb
@@ -28,7 +28,12 @@ module DeclarativePolicy
 
       subject = find_delegate(subject)
 
-      class_for_class(subject.class)
+      class_for_class(subject.class) \
+        or raise "no policy for #{subject.class.name}"
+    end
+
+    def has_policy?(subject)
+      !class_for_class(subject.class).nil?
     end
 
     private
@@ -51,9 +56,7 @@ module DeclarativePolicy
         end
       end
 
-      policy_class = subject_class.instance_variable_get(CLASS_CACHE_IVAR)
-      raise "no policy for #{subject.class.name}" if policy_class.nil?
-      policy_class
+      subject_class.instance_variable_get(CLASS_CACHE_IVAR)
     end
 
     def compute_class_for_class(subject_class)
@@ -71,6 +74,8 @@ module DeclarativePolicy
           nil
         end
       end
+
+      nil
     end
 
     def find_delegate(subject)

From 444c8584491ce0efe2314747326da6ab7d17490c Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:42:24 -0700
Subject: [PATCH 29/43] use safe navigation on notification_setting

---
 app/models/notification_recipient.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 837b62ec0cb..a14254178e1 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -31,7 +31,7 @@ class NotificationRecipient
     @notification_level ||=
       case raw_notification_level
       when :custom
-        if @custom_action && notification_setting.event_enabled?(@custom_action)
+        if @custom_action && notification_setting&.event_enabled?(@custom_action)
           :watch
         else
           :custom

From 4af2c647132bee8b2ef87d0d190936449759071c Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:42:46 -0700
Subject: [PATCH 30/43] fix comment

---
 app/models/notification_recipient.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index a14254178e1..6a8287c88b8 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -27,7 +27,8 @@ class NotificationRecipient
 
   def notification_level
     # custom is treated the same as watch if it's enabled - otherwise it's
-    # as :disabled.
+    # set to :custom, meaning to send exactly when our type is :participating
+    # or :mention.
     @notification_level ||=
       case raw_notification_level
       when :custom

From e5496e1e8ef3fa82e6cefcababcb9ad3b55a11c3 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:42:59 -0700
Subject: [PATCH 31/43] use the accessor for `type`

---
 app/models/notification_recipient.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index 6a8287c88b8..c307f0ad5b6 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -54,7 +54,7 @@ class NotificationRecipient
 
     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 NotificationSetting.levels[notification_level] <= NotificationSetting.levels[@type]
 
     return false if unsubscribed?
 

From 9d53418acb736476e4ede216c1ba9d08a5e3554a Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:43:24 -0700
Subject: [PATCH 32/43] clearer argument name

---
 app/services/notification_recipient_service.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 540e568fed2..cfc1fab210a 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -48,8 +48,8 @@ module NotificationRecipientService
         @recipients ||= []
       end
 
-      def <<(arg)
-        users, type = arg
+      def <<(pair)
+        users, type = pair
 
         if users.is_a?(ActiveRecord::Relation)
           users = users.includes(:notification_settings)

From 934305ffa3eb1662a367761058ae8ded210d3336 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:51:01 -0700
Subject: [PATCH 33/43] rm unused NewNote#subject

its functionality is swept into the project permission check
in NotificationRecipient#has_access? now
---
 app/services/notification_recipient_service.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index cfc1fab210a..b2d805e9f27 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -311,10 +311,6 @@ module NotificationRecipientService
         @read_ability ||= :"read_#{target.class.model_name.name.underscore}"
       end
 
-      def subject
-        note.for_personal_snippet? ? note.noteable : note.project
-      end
-
       def build!
         # Add all users participating in the thread (author, assignee, comment authors)
         add_participants(note.author)

From 246951bba7965f4257aa50377a981f3c85c67f1e Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:52:43 -0700
Subject: [PATCH 34/43] unmemoize read_ability

since it's only called once now in make_recipient
---
 app/services/notification_recipient_service.rb | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index b2d805e9f27..97ff1f99f02 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -87,13 +87,12 @@ module NotificationRecipientService
       end
 
       def read_ability
-        @read_ability ||=
-          case target
-          when Issuable
-            :"read_#{target.to_ability_name}"
-          when Ci::Pipeline
-            :read_build # We have build trace in pipeline emails
-          end
+        case target
+        when Issuable
+          :"read_#{target.to_ability_name}"
+        when Ci::Pipeline
+          :read_build # We have build trace in pipeline emails
+        end
       end
 
       def custom_action
@@ -308,7 +307,9 @@ module NotificationRecipientService
       end
 
       def read_ability
-        @read_ability ||= :"read_#{target.class.model_name.name.underscore}"
+        return nil if target.nil?
+
+        :"read_#{target.class.model_name.name.underscore}"
       end
 
       def build!

From 0487009d3707c8180211bfc4b3c5a8a9daec50b8 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:53:43 -0700
Subject: [PATCH 35/43] deparameterize `project`

since 99% of the time it's `target.project` anyways.
---
 .../notification_recipient_service.rb         | 30 +++++++-----
 app/services/notification_service.rb          | 48 ++++++++-----------
 2 files changed, 37 insertions(+), 41 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 97ff1f99f02..b36bc90c884 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -44,6 +44,10 @@ module NotificationRecipientService
         raise 'abstract'
       end
 
+      def project
+        target.project
+      end
+
       def recipients
         @recipients ||= []
       end
@@ -138,7 +142,7 @@ module NotificationRecipientService
 
         user_ids = user_ids_with_global_level_watch((user_ids_with_project_global + user_ids_with_group_global).uniq)
 
-        user_ids_with_project_setting = select_project_members_ids(project, user_ids_with_project_global, user_ids)
+        user_ids_with_project_setting = select_project_members_ids(user_ids_with_project_global, user_ids)
         user_ids_with_group_setting = select_group_members_ids(project.group, project_members_ids, user_ids_with_group_global, user_ids)
 
         user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq)
@@ -163,7 +167,7 @@ module NotificationRecipientService
       end
 
       # Build a list of user_ids based on project notification settings
-      def select_project_members_ids(project, global_setting, user_ids_global_level_watch)
+      def select_project_members_ids(global_setting, user_ids_global_level_watch)
         user_ids = user_ids_notifiable_on(project, :watch)
 
         # If project setting is global, add to watch list if global setting is watch
@@ -230,14 +234,12 @@ module NotificationRecipientService
     end
 
     class Default < Base
-      attr_reader :project
       attr_reader :target
       attr_reader :current_user
       attr_reader :action
       attr_reader :previous_assignee
       attr_reader :skip_current_user
-      def initialize(project, target, current_user, action:, previous_assignee: nil, skip_current_user: true)
-        @project = project
+      def initialize(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
         @target = target
         @current_user = current_user
         @action = action
@@ -280,12 +282,10 @@ module NotificationRecipientService
     end
 
     class Relabeled < Base
-      attr_reader :project
       attr_reader :target
       attr_reader :current_user
       attr_reader :labels
-      def initialize(project, target, current_user, labels:)
-        @project = project
+      def initialize(target, current_user, labels:)
         @target = target
         @current_user = current_user
         @labels = labels
@@ -297,13 +297,17 @@ module NotificationRecipientService
     end
 
     class NewNote < Base
-      attr_reader :project
       attr_reader :note
-      attr_reader :target
-      def initialize(project, note)
-        @project = project
+      def initialize(note)
         @note = note
-        @target = note.noteable
+      end
+
+      def target
+        note.noteable
+      end
+
+      def project
+        note.project
       end
 
       def read_ability
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index f5366b9ceab..c93f82999dc 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -42,7 +42,7 @@ class NotificationService
   #  * users with custom level checked with "new issue"
   #
   def new_issue(issue, current_user)
-    new_resource_email(issue, issue.project, :new_issue_email)
+    new_resource_email(issue, :new_issue_email)
   end
 
   # When issue text is updated, we should send an email to:
@@ -52,7 +52,6 @@ class NotificationService
   def new_mentions_in_issue(issue, new_mentioned_users, current_user)
     new_mentions_in_resource_email(
       issue,
-      issue.project,
       new_mentioned_users,
       current_user,
       :new_mention_in_issue_email
@@ -67,7 +66,7 @@ class NotificationService
   #  * users with custom level checked with "close issue"
   #
   def close_issue(issue, current_user)
-    close_resource_email(issue, issue.project, current_user, :closed_issue_email)
+    close_resource_email(issue, current_user, :closed_issue_email)
   end
 
   # When we reassign an issue we should send an email to:
@@ -78,7 +77,6 @@ class NotificationService
   #
   def reassigned_issue(issue, current_user, previous_assignees = [])
     recipients = NotificationRecipientService.build_recipients(
-      issue.project,
       issue,
       current_user,
       action: "reassign",
@@ -103,7 +101,7 @@ class NotificationService
   #  * watchers of the issue's labels
   #
   def relabeled_issue(issue, added_labels, current_user)
-    relabeled_resource_email(issue, issue.project, added_labels, current_user, :relabeled_issue_email)
+    relabeled_resource_email(issue, added_labels, current_user, :relabeled_issue_email)
   end
 
   # When create a merge request we should send an email to:
@@ -114,7 +112,7 @@ class NotificationService
   #  * users with custom level checked with "new merge request"
   #
   def new_merge_request(merge_request, current_user)
-    new_resource_email(merge_request, merge_request.target_project, :new_merge_request_email)
+    new_resource_email(merge_request, :new_merge_request_email)
   end
 
   # When merge request text is updated, we should send an email to:
@@ -124,7 +122,6 @@ class NotificationService
   def new_mentions_in_merge_request(merge_request, new_mentioned_users, current_user)
     new_mentions_in_resource_email(
       merge_request,
-      merge_request.target_project,
       new_mentioned_users,
       current_user,
       :new_mention_in_merge_request_email
@@ -138,7 +135,7 @@ class NotificationService
   #  * users with custom level checked with "reassign merge request"
   #
   def reassigned_merge_request(merge_request, current_user)
-    reassign_resource_email(merge_request, merge_request.target_project, current_user, :reassigned_merge_request_email)
+    reassign_resource_email(merge_request, current_user, :reassigned_merge_request_email)
   end
 
   # When we add labels to a merge request we should send an email to:
@@ -146,21 +143,20 @@ class NotificationService
   #  * watchers of the mr's labels
   #
   def relabeled_merge_request(merge_request, added_labels, current_user)
-    relabeled_resource_email(merge_request, merge_request.target_project, added_labels, current_user, :relabeled_merge_request_email)
+    relabeled_resource_email(merge_request, added_labels, current_user, :relabeled_merge_request_email)
   end
 
   def close_mr(merge_request, current_user)
-    close_resource_email(merge_request, merge_request.target_project, current_user, :closed_merge_request_email)
+    close_resource_email(merge_request, current_user, :closed_merge_request_email)
   end
 
   def reopen_issue(issue, current_user)
-    reopen_resource_email(issue, issue.project, current_user, :issue_status_changed_email, 'reopened')
+    reopen_resource_email(issue, current_user, :issue_status_changed_email, 'reopened')
   end
 
   def merge_mr(merge_request, current_user)
     close_resource_email(
       merge_request,
-      merge_request.target_project,
       current_user,
       :merged_merge_request_email,
       skip_current_user: !merge_request.merge_when_pipeline_succeeds?
@@ -170,7 +166,6 @@ class NotificationService
   def reopen_mr(merge_request, current_user)
     reopen_resource_email(
       merge_request,
-      merge_request.target_project,
       current_user,
       :merge_request_status_email,
       'reopened'
@@ -179,7 +174,6 @@ class NotificationService
 
   def resolve_all_discussions(merge_request, current_user)
     recipients = NotificationRecipientService.build_recipients(
-      merge_request.target_project,
       merge_request,
       current_user,
       action: "resolve_all_discussions")
@@ -204,7 +198,7 @@ class NotificationService
 
     notify_method = "note_#{note.to_ability_name}_email".to_sym
 
-    recipients = NotificationRecipientService.build_new_note_recipients(note.project, note)
+    recipients = NotificationRecipientService.build_new_note_recipients(note)
     recipients.each do |recipient|
       mailer.send(notify_method, recipient.id, note.id).deliver_later
     end
@@ -284,7 +278,7 @@ class NotificationService
   end
 
   def issue_moved(issue, new_issue, current_user)
-    recipients = NotificationRecipientService.build_recipients(issue.project, issue, current_user, action: 'moved')
+    recipients = NotificationRecipientService.build_recipients(issue, current_user, action: 'moved')
 
     recipients.map do |recipient|
       email = mailer.issue_moved_email(recipient, issue, new_issue, current_user)
@@ -320,16 +314,16 @@ class NotificationService
 
   protected
 
-  def new_resource_email(target, project, method)
-    recipients = NotificationRecipientService.build_recipients(project, target, target.author, action: "new")
+  def new_resource_email(target, method)
+    recipients = NotificationRecipientService.build_recipients(target, target.author, action: "new")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.id, target.id).deliver_later
     end
   end
 
-  def new_mentions_in_resource_email(target, project, new_mentioned_users, current_user, method)
-    recipients = NotificationRecipientService.build_recipients(project, target, current_user, action: "new")
+  def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method)
+    recipients = NotificationRecipientService.build_recipients(target, current_user, action: "new")
     recipients = recipients & new_mentioned_users
 
     recipients.each do |recipient|
@@ -337,11 +331,10 @@ class NotificationService
     end
   end
 
-  def close_resource_email(target, project, current_user, method, skip_current_user: true)
+  def close_resource_email(target, current_user, method, skip_current_user: true)
     action = method == :merged_merge_request_email ? "merge" : "close"
 
     recipients = NotificationRecipientService.build_recipients(
-      project,
       target,
       current_user,
       action: action,
@@ -353,12 +346,11 @@ class NotificationService
     end
   end
 
-  def reassign_resource_email(target, project, current_user, method)
+  def reassign_resource_email(target, current_user, method)
     previous_assignee_id = previous_record(target, 'assignee_id')
     previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
 
     recipients = NotificationRecipientService.build_recipients(
-      project,
       target,
       current_user,
       action: "reassign",
@@ -376,8 +368,8 @@ class NotificationService
     end
   end
 
-  def relabeled_resource_email(target, project, labels, current_user, method)
-    recipients = NotificationRecipientService.build_relabeled_recipients(project, target, current_user, labels: labels)
+  def relabeled_resource_email(target, labels, current_user, method)
+    recipients = NotificationRecipientService.build_relabeled_recipients(target, current_user, labels: labels)
     label_names = labels.map(&:name)
 
     recipients.each do |recipient|
@@ -385,8 +377,8 @@ class NotificationService
     end
   end
 
-  def reopen_resource_email(target, project, current_user, method, status)
-    recipients = NotificationRecipientService.build_recipients(project, target, current_user, action: "reopen")
+  def reopen_resource_email(target, current_user, method, status)
+    recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen")
 
     recipients.each do |recipient|
       mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later

From 4dfda5f2662afe32fcaa95b851fe6bdff9e20c08 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 11:58:44 -0700
Subject: [PATCH 36/43] declarative_policy rubocop fix

---
 lib/declarative_policy.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/declarative_policy.rb b/lib/declarative_policy.rb
index 4936669a73a..ae65653645b 100644
--- a/lib/declarative_policy.rb
+++ b/lib/declarative_policy.rb
@@ -28,8 +28,9 @@ module DeclarativePolicy
 
       subject = find_delegate(subject)
 
-      class_for_class(subject.class) \
-        or raise "no policy for #{subject.class.name}"
+      policy_class = class_for_class(subject.class)
+      raise "no policy for #{subject.class.name}" if policy_class.nil?
+      policy_class
     end
 
     def has_policy?(subject)

From 1ccdd99fbd82c082a992ea344938dca17c337140 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 13:18:51 -0700
Subject: [PATCH 37/43] disable the delegate cop

---
 app/services/notification_recipient_service.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index b36bc90c884..0e1527c1a7a 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -44,6 +44,7 @@ module NotificationRecipientService
         raise 'abstract'
       end
 
+      # rubocop:disable Rails/Delegate
       def project
         target.project
       end

From 56a40a2b6548d57c2c2a32b34a76c157ae5fdeab Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Tue, 1 Aug 2017 13:26:55 -0700
Subject: [PATCH 38/43] rm unused methods

---
 .../notification_recipient_service.rb         | 26 -------------------
 1 file changed, 26 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 0e1527c1a7a..5d394f67631 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -199,32 +199,6 @@ module NotificationRecipientService
         )
       end
 
-      def reject_unsubscribed_users
-        return unless target.respond_to? :subscriptions
-
-        recipients.reject! do |recipient|
-          user = recipient.user
-          subscription = target.subscriptions.find_by_user_id(user.id)
-          subscription && !subscription.subscribed
-        end
-      end
-
-      def reject_users_without_access
-        recipients.select! { |r| r.user.can?(:receive_notifications) }
-
-        return unless read_ability
-
-        DeclarativePolicy.subject_scope do
-          recipients.select! do |recipient|
-            recipient.user.can?(read_ability, target)
-          end
-        end
-      end
-
-      def reject_user(user)
-        recipients.reject! { |r| r.user == user }
-      end
-
       def add_labels_subscribers(labels: nil)
         return unless target.respond_to? :labels
 

From eaa503d679e3c2a1396091efebda49a91637cb02 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 2 Aug 2017 14:14:53 -0700
Subject: [PATCH 39/43] move the read_ability logic into NotificationRecipient

---
 app/models/notification_recipient.rb          | 22 +++++++++++++++----
 .../notification_recipient_service.rb         | 16 --------------
 app/services/notification_service.rb          |  1 -
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index c307f0ad5b6..418b42d8f1d 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -5,12 +5,10 @@ class NotificationRecipient
     custom_action: nil,
     target: nil,
     acting_user: nil,
-    read_ability: nil,
     project: nil
   )
     @custom_action = custom_action
     @acting_user = acting_user
-    @read_ability = read_ability
     @target = target
     @project = project || @target&.project
     @user = user
@@ -81,10 +79,10 @@ class NotificationRecipient
       return false unless user.can?(:receive_notifications)
       return false if @project && !user.can?(:read_project, @project)
 
-      return true unless @read_ability
+      return true unless read_ability
       return true unless DeclarativePolicy.has_policy?(@target)
 
-      user.can?(@read_ability, @target)
+      user.can?(read_ability, @target)
     end
   end
 
@@ -97,6 +95,22 @@ class NotificationRecipient
 
   private
 
+  def read_ability
+    return @read_ability if instance_variable_defined?(:@read_ability)
+
+    @read_ability =
+      case @target
+      when Issuable
+        :"read_#{@target.to_ability_name}"
+      when Ci::Pipeline
+        :read_build # We have build trace in pipeline emails
+      when ActiveRecord::Base
+        :"read_#{@target.class.model_name.name.underscore}"
+      else
+        nil
+      end
+  end
+
   def find_notification_setting
     project_setting = @project && user.notification_settings_for(@project)
 
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 5d394f67631..7d4f8110f84 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -76,7 +76,6 @@ module NotificationRecipientService
           custom_action: custom_action,
           target: target,
           acting_user: acting_user,
-          read_ability: read_ability
         )
       end
 
@@ -91,15 +90,6 @@ module NotificationRecipientService
           end
       end
 
-      def read_ability
-        case target
-        when Issuable
-          :"read_#{target.to_ability_name}"
-        when Ci::Pipeline
-          :read_build # We have build trace in pipeline emails
-        end
-      end
-
       def custom_action
         nil
       end
@@ -285,12 +275,6 @@ module NotificationRecipientService
         note.project
       end
 
-      def read_ability
-        return nil if target.nil?
-
-        :"read_#{target.class.model_name.name.underscore}"
-      end
-
       def build!
         # Add all users participating in the thread (author, assignee, comment authors)
         add_participants(note.author)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index c93f82999dc..2e52296f048 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -303,7 +303,6 @@ class NotificationService
     recipients ||= NotificationRecipientService.notifiable_users(
       [pipeline.user], :watch,
       custom_action: :"#{pipeline.status}_pipeline",
-      read_ability: :read_build,
       target: pipeline
     ).map(&:notification_email)
 

From 273cd9c3c9d94e1d40249073ec23e4eba820f260 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 2 Aug 2017 14:16:00 -0700
Subject: [PATCH 40/43] remove build_relabeled_recipients

---
 .../notification_recipient_service.rb         | 19 -------------------
 app/services/notification_service.rb          |  8 +++++++-
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 7d4f8110f84..8c7a36d4cb0 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -14,10 +14,6 @@ module NotificationRecipientService
     Builder::Default.new(*a).recipient_users
   end
 
-  def self.build_relabeled_recipients(*a)
-    Builder::Relabeled.new(*a).recipient_users
-  end
-
   def self.build_new_note_recipients(*a)
     Builder::NewNote.new(*a).recipient_users
   end
@@ -246,21 +242,6 @@ module NotificationRecipientService
       end
     end
 
-    class Relabeled < Base
-      attr_reader :target
-      attr_reader :current_user
-      attr_reader :labels
-      def initialize(target, current_user, labels:)
-        @target = target
-        @current_user = current_user
-        @labels = labels
-      end
-
-      def build!
-        add_labels_subscribers(labels: labels)
-      end
-    end
-
     class NewNote < Base
       attr_reader :note
       def initialize(note)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 2e52296f048..a62085dffd4 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -368,7 +368,13 @@ class NotificationService
   end
 
   def relabeled_resource_email(target, labels, current_user, method)
-    recipients = NotificationRecipientService.build_relabeled_recipients(target, current_user, labels: labels)
+    recipients = labels.flat_map { |l| l.subscribers(target.project) }
+    recipients = NotificationRecipientService.notifiable_users(
+      recipients, :subscription,
+      target: target,
+      acting_user: current_user,
+    )
+
     label_names = labels.map(&:name)
 
     recipients.each do |recipient|

From e4789cdfdcc2cebb080c96b5165733043db0ce5e Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 2 Aug 2017 14:21:31 -0700
Subject: [PATCH 41/43] add a comment warning that note.project may be nil

---
 app/services/notification_recipient_service.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 8c7a36d4cb0..0382ecd4abb 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -252,6 +252,10 @@ module NotificationRecipientService
         note.noteable
       end
 
+      # NOTE: may be nil, in the case of a PersonalSnippet
+      #
+      # (this is okay because NotificationRecipient is written
+      # to handle nil projects)
       def project
         note.project
       end

From 17147f2027896c557cb52dbb5344b87e7d4db1ea Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Wed, 2 Aug 2017 18:54:18 -0700
Subject: [PATCH 42/43] rubocop style fix

---
 app/services/notification_recipient_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 0382ecd4abb..21c9c314a2a 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -71,7 +71,7 @@ module NotificationRecipientService
           project: project,
           custom_action: custom_action,
           target: target,
-          acting_user: acting_user,
+          acting_user: acting_user
         )
       end
 

From 120fd3b3aea9cda81384c6488a1136c176ab3965 Mon Sep 17 00:00:00 2001
From: "http://jneen.net/" <jneen@jneen.net>
Date: Thu, 3 Aug 2017 11:52:53 -0700
Subject: [PATCH 43/43] another rubocop style fix

---
 app/services/notification_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index a62085dffd4..df04b1a4fe3 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -372,7 +372,7 @@ class NotificationService
     recipients = NotificationRecipientService.notifiable_users(
       recipients, :subscription,
       target: target,
-      acting_user: current_user,
+      acting_user: current_user
     )
 
     label_names = labels.map(&:name)