Merge remote-tracking branch 'ce/master' into ce-to-ee-2017-08-03

GITALY_SERVER_VERSION

-0.26.0
+0.27.0

GITLAB_SHELL_VERSION

-5.4.0
+5.7.0

Gemfile

@@ -326,11 +326,11 @@ group :development, :test do
   gem 'pry-rails', '~> 0.3.4'
 
   gem 'awesome_print', '~> 1.2.0', require: false
-  gem 'fuubar', '~> 2.0.0'
+  gem 'fuubar', '~> 2.2.0'
 
   gem 'database_cleaner', '~> 1.5.0'
   gem 'factory_girl_rails', '~> 4.7.0'
-  gem 'rspec-rails', '~> 3.5.0'
+  gem 'rspec-rails', '~> 3.6.0'
   gem 'rspec-retry', '~> 0.4.5'
   gem 'spinach-rails', '~> 0.2.1'
   gem 'spinach-rerun-reporter', '~> 0.0.2'

Gemfile.lock

@@ -164,7 +164,7 @@ GEM
       devise (~> 4.0)
       railties
       rotp (~> 2.0)
-    diff-lcs (1.2.5)
+    diff-lcs (1.3)
     diffy (3.1.0)
     docile (1.1.5)
     domain_name (0.5.20161021)
@@ -274,8 +274,8 @@ GEM
     foreman (0.78.0)
       thor (~> 0.19.1)
     formatador (0.2.5)
-    fuubar (2.0.0)
-      rspec (~> 3.0)
+    fuubar (2.2.0)
+      rspec-core (~> 3.0)
       ruby-progressbar (~> 1.4)
     gemnasium-gitlab-service (0.2.6)
       rugged (~> 0.21)
@@ -420,7 +420,7 @@ GEM
       json (~> 1.8)
       multi_xml (>= 0.5.2)
     httpclient (2.8.2)
-    i18n (0.8.1)
+    i18n (0.8.6)
     ice_nine (0.11.2)
     influxdb (0.2.3)
       cause
@@ -639,7 +639,7 @@ GEM
     pry-rails (0.3.5)
       pry (>= 0.9.10)
     pyu-ruby-sasl (0.0.3.3)
-    rack (1.6.5)
+    rack (1.6.8)
     rack-accept (0.4.5)
       rack (>= 0.4)
     rack-attack (4.4.1)
@@ -687,7 +687,7 @@ GEM
     rainbow (2.2.2)
       rake
     raindrops (0.18.0)
-    rake (10.5.0)
+    rake (12.0.0)
     rblineprof (0.3.6)
       debugger-ruby_core_source (~> 1.3)
     rdoc (4.2.2)
@@ -731,30 +731,26 @@ GEM
       chunky_png
     rqrcode-rails3 (0.1.7)
       rqrcode (>= 0.4.2)
-    rspec (3.5.0)
-      rspec-core (~> 3.5.0)
-      rspec-expectations (~> 3.5.0)
-      rspec-mocks (~> 3.5.0)
-    rspec-core (3.5.0)
-      rspec-support (~> 3.5.0)
-    rspec-expectations (3.5.0)
+    rspec-core (3.6.0)
+      rspec-support (~> 3.6.0)
+    rspec-expectations (3.6.0)
       diff-lcs (>= 1.2.0, < 2.0)
-      rspec-support (~> 3.5.0)
-    rspec-mocks (3.5.0)
+      rspec-support (~> 3.6.0)
+    rspec-mocks (3.6.0)
       diff-lcs (>= 1.2.0, < 2.0)
-      rspec-support (~> 3.5.0)
-    rspec-rails (3.5.0)
+      rspec-support (~> 3.6.0)
+    rspec-rails (3.6.0)
       actionpack (>= 3.0)
       activesupport (>= 3.0)
       railties (>= 3.0)
-      rspec-core (~> 3.5.0)
-      rspec-expectations (~> 3.5.0)
-      rspec-mocks (~> 3.5.0)
-      rspec-support (~> 3.5.0)
+      rspec-core (~> 3.6.0)
+      rspec-expectations (~> 3.6.0)
+      rspec-mocks (~> 3.6.0)
+      rspec-support (~> 3.6.0)
     rspec-retry (0.4.5)
       rspec-core
     rspec-set (0.1.3)
-    rspec-support (3.5.0)
+    rspec-support (3.6.0)
     rspec_profiling (0.0.5)
       activerecord
       pg
@@ -889,7 +885,7 @@ GEM
     truncato (0.7.8)
       htmlentities (~> 4.3.1)
       nokogiri (~> 1.6.1)
-    tzinfo (1.2.2)
+    tzinfo (1.2.3)
       thread_safe (~> 0.1)
     u2f (0.2.1)
     uglifier (2.7.2)
@@ -1006,7 +1002,7 @@ DEPENDENCIES
   fog-rackspace (~> 0.1.1)
   font-awesome-rails (~> 4.7)
   foreman (~> 0.78.0)
-  fuubar (~> 2.0.0)
+  fuubar (~> 2.2.0)
   gemnasium-gitlab-service (~> 0.2)
   gemojione (~> 3.0)
   gettext (~> 3.2.2)
@@ -1113,7 +1109,7 @@ DEPENDENCIES
   responders (~> 2.0)
   rouge (~> 2.0)
   rqrcode-rails3 (~> 0.1.7)
-  rspec-rails (~> 3.5.0)
+  rspec-rails (~> 3.6.0)
   rspec-retry (~> 0.4.5)
   rspec-set (~> 0.1.3)
   rspec_profiling (~> 0.0.5)
@@ -1167,4 +1163,4 @@ DEPENDENCIES
   wikicloth (= 0.8.1)
 
 BUNDLED WITH
-   1.15.1
+   1.15.3

app/assets/javascripts/ajax_loading_spinner.js

@@ -10,7 +10,7 @@ class AjaxLoadingSpinner {
     e.target.setAttribute('disabled', '');
     const iconElement = e.target.querySelector('i');
     // get first fa- icon
-    const originalIcon = iconElement.className.match(/(fa-)([^\s]+)/g).first();
+    const originalIcon = iconElement.className.match(/(fa-)([^\s]+)/g)[0];
     iconElement.dataset.icon = originalIcon;
     AjaxLoadingSpinner.toggleLoadingIcon(iconElement);
     $(e.target).off('ajax:beforeSend', AjaxLoadingSpinner.ajaxBeforeSend);

app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js

@@ -92,7 +92,7 @@ $(() => {
           });
       },
       selectDefaultStage() {
-        const stage = this.state.stages.first();
+        const stage = this.state.stages[0];
         this.selectStage(stage);
       },
       selectStage(stage) {

app/assets/javascripts/dispatcher.js

@@ -575,7 +575,7 @@ import initGroupAnalytics from './init_group_analytics';
           initGroupAnalytics();
           break;
       }
-      switch (path.first()) {
+      switch (path[0]) {
         case 'sessions':
         case 'omniauth_callbacks':
           if (!gon.u2f) break;

app/assets/javascripts/dropzone_input.js

@@ -217,7 +217,7 @@ window.DropzoneInput = (function() {
         value = e.clipboardData.getData('text/plain');
       }
       value = value.split("\r");
-      return value.first();
+      return value[0];
     };
 
     const showSpinner = function(e) {

app/assets/javascripts/extensions/array.js

-// TODO: remove this
-
-// eslint-disable-next-line no-extend-native
-Array.prototype.first = function first() {
-  return this[0];
-};
-
-// eslint-disable-next-line no-extend-native
-Array.prototype.last = function last() {
-  return this[this.length - 1];
-};

app/assets/javascripts/filtered_search/dropdown_utils.js

@@ -123,11 +123,11 @@ class DropdownUtils {
 
     if (!allowMultiple && itemInExistingTokens) {
       updatedItem.droplab_hidden = true;
-    } else if (!lastKey || searchInput.split('').last() === ' ') {
+    } else if (!lastKey || _.last(searchInput.split('')) === ' ') {
       updatedItem.droplab_hidden = false;
     } else if (lastKey) {
       const split = lastKey.split(':');
-      const tokenName = split[0].split(' ').last();
+      const tokenName = _.last(split[0].split(' '));
 
       const match = updatedItem.hint.indexOf(tokenName.toLowerCase()) === -1;
       updatedItem.droplab_hidden = tokenName ? match : false;

app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js

@@ -179,7 +179,7 @@ class FilteredSearchDropdownManager {
       // Eg. token = 'label:'
 
       const split = lastToken.split(':');
-      const dropdownName = split[0].split(' ').last();
+      const dropdownName = _.last(split[0].split(' '));
       this.loadDropdown(split.length > 1 ? dropdownName : '');
     } else if (lastToken) {
       // Token has been initialized into an object because it has a value

app/assets/javascripts/filtered_search/filtered_search_manager.js

@@ -375,7 +375,7 @@ class FilteredSearchManager {
       const fragments = searchToken.split(':');
       if (fragments.length > 1) {
         const inputValues = fragments[0].split(' ');
-        const tokenKey = inputValues.last();
+        const tokenKey = _.last(inputValues);
 
         if (inputValues.length > 1) {
           inputValues.pop();

app/assets/javascripts/main.js

@@ -16,9 +16,6 @@ import 'mousetrap';
 import 'mousetrap/plugins/pause/mousetrap-pause';
 import 'vendor/fuzzaldrin-plus';
 
-// extensions
-import './extensions/array';
-
 // expose common libraries as globals (TODO: remove these)
 window.jQuery = jQuery;
 window.$ = jQuery;

app/models/notification_recipient.rb

+class NotificationRecipient
+  attr_reader :user, :type
+  def initialize(
+    user, type,
+    custom_action: nil,
+    target: nil,
+    acting_user: nil,
+    project: nil
+  )
+    @custom_action = custom_action
+    @acting_user = acting_user
+    @target = target
+    @project = project || @target&.project
+    @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
+    # set to :custom, meaning to send exactly when our type is :participating
+    # or :mention.
+    @notification_level ||=
+      case raw_notification_level
+      when :custom
+        if @custom_action && notification_setting&.event_enabled?(@custom_action)
+          :watch
+        else
+          :custom
+        end
+      else
+        raw_notification_level
+      end
+  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?
+    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
+
+  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 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)
+
+    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

app/models/project_services/jira_service.rb

@@ -104,7 +104,7 @@ class JiraService < IssueTrackerService
   def close_issue(entity, external_issue)
     issue = jira_request { client.Issue.find(external_issue.iid) }
 
-    return if issue.nil? || issue.resolution.present? || !jira_issue_transition_id.present?
+    return if issue.nil? || has_resolution?(issue) || !jira_issue_transition_id.present?
 
     commit_id = if entity.is_a?(Commit)
                   entity.id
@@ -118,7 +118,7 @@ class JiraService < IssueTrackerService
     # may or may not be allowed. Refresh the issue after transition and check
     # if it is closed, so we don't have one comment for every commit.
     issue = jira_request { client.Issue.find(issue.key) } if transition_issue(issue)
-    add_issue_solved_comment(issue, commit_id, commit_url) if issue.resolution
+    add_issue_solved_comment(issue, commit_id, commit_url) if has_resolution?(issue)
   end
 
   def create_cross_reference_note(mentioned, noteable, author)
@@ -216,6 +216,10 @@ class JiraService < IssueTrackerService
     end
   end
 
+  def has_resolution?(issue)
+    issue.respond_to?(:resolution) && issue.resolution.present?
+  end
+
   def comment_exists?(issue, message)
     comments = jira_request { issue.comments }
 

app/services/notification_recipient_service.rb

 #
 # Used by NotificationService to determine who should receive notification
 #
-class NotificationRecipientService
-  attr_reader :project
-
-  def initialize(project)
-    @project = project
+module NotificationRecipientService
+  def self.notifiable_users(users, *args)
+    users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
   end
 
-  def build_recipients(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)
+  def self.notifiable?(user, *args)
+    NotificationRecipient.new(user, *args).notifiable?
+  end
 
-    recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity?
+  def self.build_recipients(*a)
+    Builder::Default.new(*a).recipient_users
+  end
 
-    recipients.uniq
+  def self.build_new_note_recipients(*a)
+    Builder::NewNote.new(*a).recipient_users
   end
 
-  def build_pipeline_recipients(target, current_user, action:)
-    return [] unless current_user
+  module Builder
+    class Base
+      def initialize(*)
+        raise 'abstract'
+      end
 
-    custom_action =
-      case action.to_s
-      when 'failed'
-        :failed_pipeline
-      when 'success'
-        :success_pipeline
+      def build!
+        raise 'abstract'
       end
 
-    notification_setting = notification_setting_for_user_project(current_user, target.project)
+      def filter!
+        recipients.select!(&:notifiable?)
+      end
 
-    return [] if notification_setting.mention? || notification_setting.disabled?
+      def acting_user
+        current_user
+      end
 
-    return [] if notification_setting.custom? && !notification_setting.event_enabled?(custom_action)
+      def target
+        raise 'abstract'
+      end
 
-    return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
+      # rubocop:disable Rails/Delegate
+      def project
+        target.project
+      end
 
-    reject_users_without_access([current_user], target)
-  end
+      def recipients
+        @recipients ||= []
+      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
-  end
+      def <<(pair)
+        users, type = pair
 
-  def build_new_note_recipients(note)
-    target = note.noteable
+        if users.is_a?(ActiveRecord::Relation)
+          users = users.includes(:notification_settings)
+        end
 
-    ability, subject = if note.for_personal_snippet?
-                         [:read_personal_snippet, note.noteable]
-                       else
-                         [:read_project, note.project]
-                       end
+        users = Array(users)
+        users.compact!
+        recipients.concat(users.map { |u| make_recipient(u, type) })
+      end
 
-    mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
+      def user_scope
+        User.includes(:notification_settings)
+      end
 
-    # Add all users participating in the thread (author, assignee, comment authors)
-    recipients = participants(target, note.author) || mentioned_users
+      def make_recipient(user, type)
+        NotificationRecipient.new(
+          user, type,
+          project: project,
+          custom_action: custom_action,
+          target: target,
+          acting_user: acting_user
+        )
+      end
 
-    unless note.for_personal_snippet?
-      # Merge project watchers
-      recipients = add_project_watchers(recipients)
+      def recipient_users
+        @recipient_users ||=
+          begin
+            build!
+            filter!
+            users = recipients.map(&:user)
+            users.uniq!
+            users.freeze
+          end
+      end
 
-      # Merge project with custom notification
-      recipients = add_custom_notifications(recipients, :new_note)
-    end
+      def custom_action
+        nil
+      end
 
-    # Reject users with Mention notification level, except those mentioned in _this_ note.
-    recipients = reject_mention_users(recipients - mentioned_users)
-    recipients = recipients + mentioned_users
+      protected
 
-    recipients = reject_muted_users(recipients)
+      def add_participants(user)
+        return unless target.respond_to?(:participants)
 
-    recipients = add_subscribed_users(recipients, note.noteable)
-    recipients = reject_unsubscribed_users(recipients, note.noteable)
-    recipients = reject_users_without_access(recipients, note.noteable)
+        self << [target.participants(user), :watch]
+      end
 
-    recipients.delete(note.author) unless note.author.notified_of_own_activity?
-    recipients.uniq
-  end
+      # Get project/group users with CUSTOM notification level
+      def add_custom_notifications
+        user_ids = []
 
-  # Remove users with disabled notifications from array
-  # Also remove duplications and nil recipients
-  def reject_muted_users(users)
-    reject_users(users, :disabled)
-  end
+        # Users with a notification setting on group or project
+        user_ids += user_ids_notifiable_on(project, :custom)
+        user_ids += user_ids_notifiable_on(project.group, :custom)
 
-  protected
+        # 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)
 
-  # 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)
+        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)
 
-    target.participants(user).dup
-  end
+        self << [user_scope.where(id: user_ids), :watch]
+      end
 
-  # Get project/group users with CUSTOM notification level
-  def add_custom_notifications(recipients, action)
-    user_ids = []
+      def add_project_watchers
+        self << [project_watchers, :watch]
+      end
 
-    # 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)
+      # Get project users with WATCH notification level
+      def project_watchers
+        project_members_ids = user_ids_notifiable_on(project)
 
-    # 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)
+        user_ids_with_project_global = user_ids_notifiable_on(project, :global)
+        user_ids_with_group_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_watch((user_ids_with_project_global + user_ids_with_group_global).uniq)
 
-    recipients.concat(User.find(user_ids))
-  end
+        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)
 
-  def add_project_watchers(recipients)
-    recipients.concat(project_watchers).compact
-  end
+        user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq)
+      end
 
-  # Get project users with WATCH notification level
-  def project_watchers
-    project_members_ids = user_ids_notifiable_on(project)
+      def add_subscribed_users
+        return unless target.respond_to? :subscribers
 
-    user_ids_with_project_global = user_ids_notifiable_on(project, :global)
-    user_ids_with_group_global   = user_ids_notifiable_on(project.group, :global)
+        self << [target.subscribers(project), :subscription]
+      end
 
-    user_ids = user_ids_with_global_level_watch((user_ids_with_project_global + user_ids_with_group_global).uniq)
+      def user_ids_notifiable_on(resource, notification_level = nil)
+        return [] unless resource
 
-    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)
+        scope = resource.notification_settings
 
-    User.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq).to_a
-  end
+        if notification_level
+          scope = scope.where(level: NotificationSetting.levels[notification_level])
+        end
 
-  # Remove users with notification level 'Mentioned'
-  def reject_mention_users(users)
-    reject_users(users, :mention)
-  end
+        scope.pluck(:user_id)
+      end
 
-  def add_subscribed_users(recipients, target)
-    return recipients unless target.respond_to? :subscribers
+      # Build a list of user_ids based on project notification settings
+      def select_project_members_ids(global_setting, user_ids_global_level_watch)
+        user_ids = user_ids_notifiable_on(project, :watch)
 
-    recipients + target.subscribers(project)
-  end
+        # If project setting is global, add to watch list if global setting is watch
+        user_ids + (global_setting & user_ids_global_level_watch)
+      end
 
-  def user_ids_notifiable_on(resource, notification_level = nil, action = nil)
-    return [] unless resource
+      # 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)
 
-    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
+        # Group setting is global, add to user_ids list if global setting is watch
+        uids + (global_setting & user_ids_global_level_watch) - project_members
+      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)
+      def user_ids_with_global_level_watch(ids)
+        settings_with_global_level_of(:watch, ids).pluck(:user_id)
+      end
 
-    # 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
+      def user_ids_with_global_level_custom(ids, action)
+        settings_with_global_level_of(:custom, ids).pluck(:user_id)
       end
-    end
 
-    user_ids
-  end
+      def settings_with_global_level_of(level, ids)
+        NotificationSetting.where(
+          user_id: ids,
+          source_type: nil,
+          level: NotificationSetting.levels[level]
+        )
+      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)
+      def add_labels_subscribers(labels: nil)
+        return unless target.respond_to? :labels
 
-    # 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
+        (labels || target.labels).each do |label|
+          self << [label.subscribers(project), :subscription]
+        end
       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
+    class Default < Base
+      attr_reader :target
+      attr_reader :current_user
+      attr_reader :action
+      attr_reader :previous_assignee
+      attr_reader :skip_current_user
+      def initialize(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
+        @target = target
+        @current_user = current_user
+        @action = action
+        @previous_assignee = previous_assignee
+        @skip_current_user = skip_current_user
       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
+      def build!
+        add_participants(current_user)
+        add_project_watchers
+        add_custom_notifications
+
+        # Re-assign is considered as a mention of the new assignee
+        case custom_action
+        when :reassign_merge_request
+          self << [previous_assignee, :mention]
+          self << [target.assignee, :mention]
+        when :reassign_issue
+          previous_assignees = Array(previous_assignee)
+          self << [previous_assignees, :mention]
+          self << [target.assignees, :mention]
+        end
+
+        add_subscribed_users
+
+        if [:new_issue, :new_merge_request].include?(custom_action)
+          add_labels_subscribers
+        end
+      end
 
-  # Reject users which has certain notification level
-  #
-  # Example:
-  #   reject_users(users, :watch, project)
-  #
-  def reject_users(users, level)
-    level = level.to_s
+      def acting_user
+        current_user if skip_current_user
+      end
 
-    unless NotificationSetting.levels.keys.include?(level)
-      raise 'Invalid notification level'
+      # 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
 
-    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
-
-      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)
+    class NewNote < Base
+      attr_reader :note
+      def initialize(note)
+        @note = note
       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
-    end
-  end
+      def target
+        note.noteable
+      end
 
-  def reject_unsubscribed_users(recipients, target)
-    return recipients unless target.respond_to? :subscriptions
+      # 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
 
-    recipients.reject do |user|
-      subscription = target.subscriptions.find_by_user_id(user.id)
-      subscription && !subscription.subscribed
-    end
-  end
+      def build!
+        # Add all users participating in the thread (author, assignee, comment authors)
+        add_participants(note.author)
+        self << [note.mentioned_users, :mention]
 
-  def reject_users_without_access(recipients, target)
-    ability = case target
-              when Issuable
-                :"read_#{target.to_ability_name}"
-              when Ci::Pipeline
-                :read_build # We have build trace in pipeline emails
-              end
+        unless note.for_personal_snippet?
+          # Merge project watchers
+          add_project_watchers
 
-    return recipients unless ability
+          # Merge project with custom notification
+          add_custom_notifications
+        end
 
-    recipients.select do |user|
-      user.can?(ability, target)
-    end
-  end
+        add_subscribed_users
+      end
 
-  def add_labels_subscribers(recipients, target, labels: nil)
-    return recipients unless target.respond_to? :labels
+      def custom_action
+        :new_note
+      end
 
-    (labels || target.labels).each do |label|
-      recipients += label.subscribers(project)
+      def acting_user
+        note.author
+      end
     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
-
-  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

app/services/notification_service.rb

@@ -44,7 +44,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:
@@ -54,7 +54,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
@@ -69,7 +68,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:
@@ -79,7 +78,7 @@ 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,
       current_user,
       action: "reassign",
@@ -104,7 +103,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:
@@ -116,7 +115,7 @@ class NotificationService
   #  * approvers of the 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:
@@ -126,7 +125,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
@@ -140,7 +138,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:
@@ -148,15 +146,15 @@ 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
 
   # When we add approvers to a merge request we should send an email to:
@@ -170,7 +168,6 @@ class NotificationService
   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?
@@ -180,7 +177,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'
@@ -196,7 +192,7 @@ 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,
       current_user,
       action: "resolve_all_discussions")
@@ -225,7 +221,7 @@ class NotificationService
   def send_new_note_notifications(note)
     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)
     recipients.each do |recipient|
       mailer.send(notify_method, recipient.id, note.id).deliver_later
     end
@@ -293,8 +289,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.notifiable_users(project.team.members, :mention, project: project)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(
@@ -306,7 +301,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, current_user, action: 'moved')
 
     recipients.map do |recipient|
       email = mailer.issue_moved_email(recipient, issue, new_issue, current_user)
@@ -328,10 +323,10 @@ 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], :watch,
+      custom_action: :"#{pipeline.status}_pipeline",
+      target: pipeline
     ).map(&:notification_email)
 
     if recipients.any?
@@ -341,16 +336,16 @@ class NotificationService
 
   protected
 
-  def new_resource_email(target, project, method)
-    recipients = NotificationRecipientService.new(project).build_recipients(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.new(project).build_recipients(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|
@@ -358,10 +353,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.new(project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
       target,
       current_user,
       action: action,
@@ -373,11 +368,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.new(project).build_recipients(
+    recipients = NotificationRecipientService.build_recipients(
       target,
       current_user,
       action: "reassign",
@@ -395,8 +390,14 @@ class NotificationService
     end
   end
 
-  def relabeled_resource_email(target, project, labels, current_user, method)
-    recipients = NotificationRecipientService.new(project).build_relabeled_recipients(target, current_user, labels: labels)
+  def relabeled_resource_email(target, labels, current_user, method)
+    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|
@@ -404,8 +405,8 @@ class NotificationService
     end
   end
 
-  def reopen_resource_email(target, project, current_user, method, status)
-    recipients = NotificationRecipientService.new(project).build_recipients(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

lib/banzai/renderer.rb

@@ -132,6 +132,8 @@ module Banzai
     end
 
     def self.cacheless_render(text, context = {})
+      return text.to_s unless text.present?
+
       Gitlab::Metrics.measure(:banzai_cacheless_render) do
         result = render_result(text, context)
 

lib/declarative_policy.rb

@@ -28,7 +28,13 @@ module DeclarativePolicy
 
       subject = find_delegate(subject)
 
-      class_for_class(subject.class)
+      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)
+      !class_for_class(subject.class).nil?
     end
 
     private
@@ -51,9 +57,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 +75,8 @@ module DeclarativePolicy
           nil
         end
       end
+
+      nil
     end
 
     def find_delegate(subject)

spec/javascripts/abuse_reports_spec.js

@@ -6,10 +6,10 @@ import '~/abuse_reports';
     const FIXTURE = 'abuse_reports/abuse_reports_list.html.raw';
     const MAX_MESSAGE_LENGTH = 500;
 
-    let messages;
+    let $messages;
 
     const assertMaxLength = $message => expect($message.text().length).toEqual(MAX_MESSAGE_LENGTH);
-    const findMessage = searchText => messages.filter(
+    const findMessage = searchText => $messages.filter(
       (index, element) => element.innerText.indexOf(searchText) > -1,
     ).first();
 
@@ -18,7 +18,7 @@ import '~/abuse_reports';
     beforeEach(function () {
       loadFixtures(FIXTURE);
       this.abuseReports = new global.AbuseReports();
-      messages = $('.abuse-reports .message');
+      $messages = $('.abuse-reports .message');
     });
 
     it('should truncate long messages', () => {

spec/javascripts/ajax_loading_spinner_spec.js

-import '~/extensions/array';
 import 'jquery';
 import 'jquery-ujs';
 import '~/ajax_loading_spinner';

spec/javascripts/extensions/array_spec.js

-/* eslint-disable space-before-function-paren, no-var */
-
-import '~/extensions/array';
-
-(function() {
-  describe('Array extensions', function() {
-    describe('first', function() {
-      return it('returns the first item', function() {
-        var arr;
-        arr = [0, 1, 2, 3, 4, 5];
-        return expect(arr.first()).toBe(0);
-      });
-    });
-    describe('last', function() {
-      return it('returns the last item', function() {
-        var arr;
-        arr = [0, 1, 2, 3, 4, 5];
-        return expect(arr.last()).toBe(5);
-      });
-    });
-  });
-}).call(window);

spec/javascripts/filtered_search/dropdown_utils_spec.js

-import '~/extensions/array';
 import '~/filtered_search/dropdown_utils';
 import '~/filtered_search/filtered_search_tokenizer';
 import '~/filtered_search/filtered_search_dropdown_manager';

spec/javascripts/filtered_search/filtered_search_dropdown_manager_spec.js

-import '~/extensions/array';
 import '~/filtered_search/filtered_search_visual_tokens';
 import '~/filtered_search/filtered_search_tokenizer';
 import '~/filtered_search/filtered_search_dropdown_manager';

spec/javascripts/filtered_search/filtered_search_token_keys_spec.js

-import '~/extensions/array';
 import '~/filtered_search/filtered_search_token_keys';
 
 describe('Filtered Search Token Keys', () => {

spec/javascripts/filtered_search/filtered_search_tokenizer_spec.js

-import '~/extensions/array';
 import '~/filtered_search/filtered_search_token_keys';
 import '~/filtered_search/filtered_search_tokenizer';
 

spec/models/project_services/jira_service_spec.rb

@@ -153,6 +153,15 @@ describe JiraService do
       expect(WebMock).not_to have_requested(:post, @remote_link_url)
     end
 
+    it "does not send comment or remote links to issues with unknown resolution" do
+      allow_any_instance_of(JIRA::Resource::Issue).to receive(:respond_to?).with(:resolution).and_return(false)
+
+      @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
+
+      expect(WebMock).not_to have_requested(:post, @comment_url)
+      expect(WebMock).not_to have_requested(:post, @remote_link_url)
+    end
+
     it "references the GitLab commit/merge request" do
       stub_config_setting(base_url: custom_base_url)
 

spec/services/notification_recipient_service_spec.rb

-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

spec/services/notification_service_spec.rb

@@ -306,6 +306,11 @@ describe NotificationService, :mailer 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