Merge branch '296031-assignee-or-filters' into 'master'

Add IssuableFinder support for OR filtering of assignees

See merge request gitlab-org/gitlab!59939

app/finders/issuable_finder.rb

@@ -50,7 +50,7 @@ class IssuableFinder
   attr_reader :original_params
   attr_writer :parent
 
-  delegate(*%i[assignee milestones], to: :params)
+  delegate(*%i[milestones], to: :params)
 
   class << self
     def scalar_params
@@ -148,7 +148,6 @@ class IssuableFinder
 
   # Negates all params found in `negatable_params`
   def filter_negated_items(items)
-    items = by_negated_assignee(items)
     items = by_negated_label(items)
     items = by_negated_milestone(items)
     items = by_negated_release(items)
@@ -365,32 +364,21 @@ class IssuableFinder
 
   def by_author(items)
     Issuables::AuthorFilter.new(
-      items,
       params: original_params,
       or_filters_enabled: or_filters_enabled?
-    ).filter
+    ).filter(items)
   end
 
   def by_assignee(items)
-    if params.filter_by_no_assignee?
-      items.unassigned
-    elsif params.filter_by_any_assignee?
-      items.assigned
-    elsif params.assignee
-      items.assigned_to(params.assignee)
-    elsif params.assignee_id? || params.assignee_username? # assignee not found
-      items.none
-    else
-      items
-    end
+    assignee_filter.filter(items)
   end
 
-  def by_negated_assignee(items)
-    # We want CE users to be able to say "Issues not assigned to either PersonA nor PersonB"
-    if not_params.assignees.present?
-      items.not_assigned_to(not_params.assignees)
-    else
-      items
+  def assignee_filter
+    strong_memoize(:assignee_filter) do
+      Issuables::AssigneeFilter.new(
+        params: original_params,
+        or_filters_enabled: or_filters_enabled?
+      )
     end
   end
 

app/finders/issuable_finder/params.rb

@@ -27,14 +27,6 @@ class IssuableFinder
       params.present?
     end
 
-    def filter_by_no_assignee?
-      params[:assignee_id].to_s.downcase == FILTER_NONE
-    end
-
-    def filter_by_any_assignee?
-      params[:assignee_id].to_s.downcase == FILTER_ANY
-    end
-
     def filter_by_no_label?
       downcased = label_names.map(&:downcase)
 
@@ -156,24 +148,6 @@ class IssuableFinder
       end
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def assignees
-      strong_memoize(:assignees) do
-        if assignee_id?
-          User.where(id: params[:assignee_id])
-        elsif assignee_username?
-          User.where(username: params[:assignee_username])
-        else
-          User.none
-        end
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    def assignee
-      assignees.first
-    end
-
     def label_names
       if labels?
         params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name]

app/finders/issuables/assignee_filter.rb

+# frozen_string_literal: true
+
+module Issuables
+  class AssigneeFilter < BaseFilter
+    def filter(issuables)
+      filtered = by_assignee(issuables)
+      filtered = by_assignee_union(filtered)
+      by_negated_assignee(filtered)
+    end
+
+    def includes_user?(user)
+      Array(params[:assignee_ids]).include?(user.id) ||
+        Array(params[:assignee_id]).include?(user.id) ||
+        Array(params[:assignee_username]).include?(user.username)
+    end
+
+    private
+
+    def by_assignee(issuables)
+      if filter_by_no_assignee?
+        issuables.unassigned
+      elsif filter_by_any_assignee?
+        issuables.assigned
+      elsif has_assignee_param?(params)
+        filter_by_assignees(issuables)
+      else
+        issuables
+      end
+    end
+
+    def by_assignee_union(issuables)
+      return issuables unless or_filters_enabled? && has_assignee_param?(or_params)
+
+      issuables.assigned_to(assignee_ids(or_params))
+    end
+
+    def by_negated_assignee(issuables)
+      return issuables unless has_assignee_param?(not_params)
+
+      issuables.not_assigned_to(assignee_ids(not_params))
+    end
+
+    def filter_by_no_assignee?
+      params[:assignee_id].to_s.downcase == FILTER_NONE
+    end
+
+    def filter_by_any_assignee?
+      params[:assignee_id].to_s.downcase == FILTER_ANY
+    end
+
+    def filter_by_assignees(issuables)
+      assignee_ids = assignee_ids(params)
+
+      return issuables.none if assignee_ids.blank?
+
+      assignee_ids.each do |assignee_id|
+        issuables = issuables.assigned_to(assignee_id)
+      end
+
+      issuables
+    end
+
+    def has_assignee_param?(specific_params)
+      return if specific_params.nil?
+
+      specific_params[:assignee_ids].present? || specific_params[:assignee_id].present? || specific_params[:assignee_username].present?
+    end
+
+    def assignee_ids(specific_params)
+      if specific_params[:assignee_ids].present?
+        Array(specific_params[:assignee_ids])
+      elsif specific_params[:assignee_id].present?
+        Array(specific_params[:assignee_id])
+      elsif specific_params[:assignee_username].present?
+        User.by_username(specific_params[:assignee_username]).select(:id)
+      end
+    end
+  end
+end

app/finders/issuables/author_filter.rb

@@ -2,7 +2,7 @@
 
 module Issuables
   class AuthorFilter < BaseFilter
-    def filter
+    def filter(issuables)
       filtered = by_author(issuables)
       filtered = by_author_union(filtered)
       by_negated_author(filtered)
@@ -21,7 +21,7 @@ module Issuables
     end
 
     def by_author_union(issuables)
-      return issuables unless or_filters_enabled? && or_params&.fetch(:author_username).present?
+      return issuables unless or_filters_enabled? && or_params&.fetch(:author_username, false).present?
 
       issuables.authored(User.by_username(or_params[:author_username]))
     end

app/finders/issuables/base_filter.rb

@@ -2,10 +2,12 @@
 
 module Issuables
   class BaseFilter
-    attr_reader :issuables, :params
+    attr_reader :params
 
-    def initialize(issuables, params:, or_filters_enabled: false)
-      @issuables = issuables
+    FILTER_NONE = 'none'
+    FILTER_ANY = 'any'
+
+    def initialize(params:, or_filters_enabled: false)
       @params = params
       @or_filters_enabled = or_filters_enabled
     end

app/finders/issues_finder.rb

@@ -52,7 +52,7 @@ class IssuesFinder < IssuableFinder
     # can always see confidential issues assigned to them. This is just an
     # optimization since a very common usecase of this Finder is to load the
     # count of issues assigned to the user for the header bar.
-    return Issue.all if current_user && params.assignees.include?(current_user)
+    return Issue.all if current_user && assignee_filter.includes_user?(current_user)
 
     return Issue.where('issues.confidential IS NOT TRUE') if params.user_cannot_see_confidential_issues?
 

app/helpers/issues_helper.rb

@@ -200,7 +200,7 @@ module IssuesHelper
       jira_integration_path: help_page_url('integration/jira/', anchor: 'view-jira-issues'),
       markdown_help_path: help_page_path('user/markdown'),
       max_attachment_size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes),
-      new_issue_path: new_project_issue_path(project, issue: { assignee_id: finder.assignee.try(:id), milestone_id: finder.milestones.first.try(:id) }),
+      new_issue_path: new_project_issue_path(project, issue: { milestone_id: finder.milestones.first.try(:id) }),
       project_import_jira_path: project_import_jira_path(project),
       project_labels_path: project_labels_path(project, include_ancestor_groups: true, format: :json),
       project_milestones_path: project_milestones_path(project, format: :json),

app/models/concerns/issuable.rb

@@ -101,20 +101,19 @@ module Issuable
     scope :unassigned, -> do
       where("NOT EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)")
     end
-    scope :assigned_to, ->(u) do
-      assignees_table = Arel::Table.new("#{to_ability_name}_assignees")
-      sql = assignees_table.project('true').where(assignees_table[:user_id].in(u.id)).where(Arel::Nodes::SqlLiteral.new("#{to_ability_name}_id = #{to_ability_name}s.id"))
-      where("EXISTS (#{sql.to_sql})")
-    end
-    # rubocop:enable GitlabSecurity/SqlInjection
+    scope :assigned_to, ->(users) do
+      assignees_class = self.reflect_on_association("#{to_ability_name}_assignees").klass
 
+      condition = assignees_class.where(user_id: users).where(Arel.sql("#{to_ability_name}_id = #{to_ability_name}s.id"))
+      where(condition.arel.exists)
+    end
     scope :not_assigned_to, ->(users) do
-      assignees_table = Arel::Table.new("#{to_ability_name}_assignees")
-      sql = assignees_table.project('true')
-                .where(assignees_table[:user_id].in(users))
-                .where(Arel::Nodes::SqlLiteral.new("#{to_ability_name}_id = #{to_ability_name}s.id"))
-      where(sql.exists.not)
+      assignees_class = self.reflect_on_association("#{to_ability_name}_assignees").klass
+
+      condition = assignees_class.where(user_id: users).where(Arel.sql("#{to_ability_name}_id = #{to_ability_name}s.id"))
+      where(condition.arel.exists.not)
     end
+    # rubocop:enable GitlabSecurity/SqlInjection
 
     scope :without_particular_labels, ->(label_names) do
       labels_table = Label.arel_table

app/views/projects/issues/_nav_btns.html.haml

@@ -15,8 +15,7 @@
     = button_tag _("Edit issues"), class: "gl-button btn btn-default gl-mr-3 js-bulk-update-toggle"
   - if show_new_issue_link?(@project)
     = link_to _("New issue"), new_project_issue_path(@project,
-                                                  issue: { assignee_id: finder.assignee.try(:id),
-                                                           milestone_id: finder.milestones.first.try(:id) }),
+                                                  issue: { milestone_id: finder.milestones.first.try(:id) }),
                                                   class: "gl-button btn btn-confirm",
                                                   id: "new_issue_link"
 

ee/app/finders/ee/issues_finder.rb

@@ -43,19 +43,6 @@ module EE
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    override :by_assignee
-    def by_assignee(items)
-      if params.assignees.any?
-        params.assignees.each do |assignee|
-          items = items.assigned_to(assignee)
-        end
-
-        return items
-      end
-
-      super
-    end
-
     def by_epic(items)
       return items unless params.by_epic?
 

ee/app/finders/ee/issues_finder/params.rb

@@ -30,19 +30,6 @@ module EE
         params[:weight].to_s.downcase == ::IssuableFinder::Params::FILTER_ANY
       end
 
-      override :assignees
-      # rubocop: disable CodeReuse/ActiveRecord
-      def assignees
-        strong_memoize(:assignees) do
-          if assignee_ids?
-            ::User.where(id: params[:assignee_ids])
-          else
-            super
-          end
-        end
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
       def epics
         if params[:include_subepics]
           ::Gitlab::ObjectHierarchy.new(::Epic.id_in(params[:epic_id])).base_and_descendants.select(:id)

ee/spec/requests/admin/credentials_controller_spec.rb

@@ -85,6 +85,8 @@ RSpec.describe Admin::CredentialsController, type: :request do
               create(:gpg_key, user: control_user, key: GpgHelpers::User1.public_key)
               create(:gpg_key, user: control_user, key: GpgHelpers::User1.public_key2)
 
+              get admin_credentials_path(filter: 'gpg_keys')
+
               control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
                 get admin_credentials_path(filter: 'gpg_keys')
               end.count

spec/finders/issues_finder_spec.rb

@@ -49,6 +49,11 @@ RSpec.describe IssuesFinder do
           let(:expected_issuables) { [issue3, issue4] }
         end
 
+        it_behaves_like 'assignee OR filter' do
+          let(:params) { { or: { assignee_id: [user.id, user2.id] } } }
+          let(:expected_issuables) { [issue1, issue2, issue3, issue5] }
+        end
+
         context 'when assignee_id does not exist' do
           it_behaves_like 'assignee NOT ID filter' do
             let(:params) { { not: { assignee_id: -100 } } }
@@ -79,6 +84,11 @@ RSpec.describe IssuesFinder do
             let(:expected_issuables) { [issue3, issue4] }
           end
 
+          it_behaves_like 'assignee OR filter' do
+            let(:params) { { or: { assignee_username: [user2.username, user3.username] } } }
+            let(:expected_issuables) { [issue2, issue3] }
+          end
+
           context 'when assignee_username does not exist' do
             it_behaves_like 'assignee NOT username filter' do
               before do

spec/helpers/issues_helper_spec.rb

@@ -312,7 +312,7 @@ RSpec.describe IssuesHelper do
         jira_integration_path: help_page_url('integration/jira/', anchor: 'view-jira-issues'),
         markdown_help_path: help_page_path('user/markdown'),
         max_attachment_size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes),
-        new_issue_path: new_project_issue_path(project, issue: { assignee_id: finder.assignee.id, milestone_id: finder.milestones.first.id }),
+        new_issue_path: new_project_issue_path(project, issue: { milestone_id: finder.milestones.first.id }),
         project_import_jira_path: project_import_jira_path(project),
         project_labels_path: project_labels_path(project, include_ancestor_groups: true, format: :json),
         project_milestones_path: project_milestones_path(project, format: :json),

spec/support/shared_examples/finders/assignees_filter_shared_examples.rb

@@ -24,6 +24,12 @@ RSpec.shared_examples 'assignee NOT username filter' do
   end
 end
 
+RSpec.shared_examples 'assignee OR filter' do
+  it 'returns issuables assigned to the given users' do
+    expect(issuables).to contain_exactly(*expected_issuables)
+  end
+end
+
 RSpec.shared_examples 'no assignee filter' do
   let(:params) { { assignee_id: 'None' } }