Merge branch 'issuable-or-filters-author' into 'master'

Add IssuableFinder support for OR filtering of authors [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!54444

app/finders/issuable_finder.rb

@@ -47,6 +47,7 @@ class IssuableFinder
   NEGATABLE_PARAMS_HELPER_KEYS = %i[project_id scope status include_subgroups].freeze
 
   attr_accessor :current_user, :params
+  attr_reader :original_params
   attr_writer :parent
 
   delegate(*%i[assignee milestones], to: :params)
@@ -87,7 +88,7 @@ class IssuableFinder
     end
 
     def valid_params
-      @valid_params ||= scalar_params + [array_params.merge(not: {})]
+      @valid_params ||= scalar_params + [array_params.merge(or: {}, not: {})]
     end
   end
 
@@ -101,6 +102,7 @@ class IssuableFinder
 
   def initialize(current_user, params = {})
     @current_user = current_user
+    @original_params = params
     @params = params_class.new(params, current_user, klass)
   end
 
@@ -142,7 +144,7 @@ class IssuableFinder
   end
 
   def should_filter_negated_args?
-    return false unless Feature.enabled?(:not_issuable_queries, params.group || params.project, default_enabled: true)
+    return false unless not_filters_enabled?
 
     # API endpoints send in `nil` values so we test if there are any non-nil
     not_params.present? && not_params.values.any?
@@ -150,7 +152,6 @@ class IssuableFinder
 
   # Negates all params found in `negatable_params`
   def filter_negated_items(items)
-    items = by_negated_author(items)
     items = by_negated_assignee(items)
     items = by_negated_label(items)
     items = by_negated_milestone(items)
@@ -372,31 +373,14 @@ class IssuableFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_author(items)
-    if params.author
-      items.where(author_id: params.author.id)
-    elsif params.no_author?
-      items.where(author_id: nil)
-    elsif params.author_id? || params.author_username? # author not found
-      items.none
-    else
-      items
-    end
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def by_negated_author(items)
-    if not_params.author
-      items.where.not(author_id: not_params.author.id)
-    elsif not_params.author_id? || not_params.author_username? # author not found
-      items.none
-    else
-      items
-    end
+    Issuables::AuthorFilter.new(
+      items,
+      params: original_params,
+      or_filters_enabled: or_filters_enabled?,
+      not_filters_enabled: not_filters_enabled?
+    ).filter
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def by_assignee(items)
     if params.filter_by_no_assignee?
@@ -514,4 +498,20 @@ class IssuableFinder
   def by_non_archived(items)
     params[:non_archived].present? ? items.non_archived : items
   end
+
+  def or_filters_enabled?
+    strong_memoize(:or_filters_enabled) do
+      Feature.enabled?(:or_issuable_queries, feature_flag_scope, default_enabled: :yaml)
+    end
+  end
+
+  def not_filters_enabled?
+    strong_memoize(:not_filters_enabled) do
+      Feature.enabled?(:not_issuable_queries, feature_flag_scope, default_enabled: :yaml)
+    end
+  end
+
+  def feature_flag_scope
+    params.group || params.project
+  end
 end

app/finders/issuable_finder/params.rb

@@ -27,19 +27,6 @@ class IssuableFinder
       params.present?
     end
 
-    def author_id?
-      params[:author_id].present? && params[:author_id] != NONE
-    end
-
-    def author_username?
-      params[:author_username].present? && params[:author_username] != NONE
-    end
-
-    def no_author?
-      # author_id takes precedence over author_username
-      params[:author_id] == NONE || params[:author_username] == NONE
-    end
-
     def filter_by_no_assignee?
       params[:assignee_id].to_s.downcase == FILTER_NONE
     end
@@ -169,20 +156,6 @@ class IssuableFinder
       end
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def author
-      strong_memoize(:author) do
-        if author_id?
-          User.find_by(id: params[:author_id])
-        elsif author_username?
-          User.find_by_username(params[:author_username])
-        else
-          nil
-        end
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     # rubocop: disable CodeReuse/ActiveRecord
     def assignees
       strong_memoize(:assignees) do

app/finders/issuables/author_filter.rb

+# frozen_string_literal: true
+
+module Issuables
+  class AuthorFilter < BaseFilter
+    def filter
+      filtered = by_author(issuables)
+      filtered = by_author_union(filtered)
+      by_negated_author(filtered)
+    end
+
+    private
+
+    def by_author(issuables)
+      if params[:author_id].present?
+        issuables.authored(params[:author_id])
+      elsif params[:author_username].present?
+        issuables.authored(User.by_username(params[:author_username]))
+      else
+        issuables
+      end
+    end
+
+    def by_author_union(issuables)
+      return issuables unless or_filters_enabled? && or_params&.fetch(:author_username).present?
+
+      issuables.authored(User.by_username(or_params[:author_username]))
+    end
+
+    def by_negated_author(issuables)
+      return issuables unless not_filters_enabled? && not_params.present?
+
+      if not_params[:author_id].present?
+        issuables.not_authored(not_params[:author_id])
+      elsif not_params[:author_username].present?
+        issuables.not_authored(User.by_username(not_params[:author_username]))
+      else
+        issuables
+      end
+    end
+  end
+end

app/finders/issuables/base_filter.rb

+# frozen_string_literal: true
+
+module Issuables
+  class BaseFilter
+    attr_reader :issuables, :params
+
+    def initialize(issuables, params:, or_filters_enabled: false, not_filters_enabled: false)
+      @issuables = issuables
+      @params = params
+      @or_filters_enabled = or_filters_enabled
+      @not_filters_enabled = not_filters_enabled
+    end
+
+    def filter
+      raise NotImplementedError
+    end
+
+    private
+
+    def or_params
+      params[:or]
+    end
+
+    def not_params
+      params[:not]
+    end
+
+    def or_filters_enabled?
+      @or_filters_enabled
+    end
+
+    def not_filters_enabled?
+      @not_filters_enabled
+    end
+  end
+end

app/models/concerns/issuable.rb

@@ -86,6 +86,7 @@ module Issuable
     before_validation :truncate_description_on_import!
 
     scope :authored, ->(user) { where(author_id: user) }
+    scope :not_authored, ->(user) { where.not(author_id: user) }
     scope :recent, -> { reorder(id: :desc) }
     scope :of_projects, ->(ids) { where(project_id: ids) }
     scope :opened, -> { with_state(:opened) }

config/feature_flags/development/or_issuable_queries.yml

+---
+name: or_issuable_queries
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54444
+rollout_issue_url:
+milestone: '13.10'
+type: development
+group: group::project management
+default_enabled: false

ee/app/finders/epics_finder.rb

@@ -25,6 +25,7 @@
 class EpicsFinder < IssuableFinder
   include TimeFrameFilter
   include Gitlab::Utils::StrongMemoize
+  extend ::Gitlab::Utils::Override
 
   IID_STARTS_WITH_PATTERN = %r{\A(\d)+\z}.freeze
 
@@ -123,13 +124,12 @@ class EpicsFinder < IssuableFinder
   end
 
   def filter_negated_items(items)
-    return items unless Feature.enabled?(:not_issuable_queries, group, default_enabled: true)
+    return items unless not_filters_enabled?
 
     # API endpoints send in `nil` values so we test if there are any non-nil
     return items unless not_params&.values&.any?
 
-    items = by_negated_label(items)
-    by_negated_author(items)
+    by_negated_label(items)
   end
 
   def group
@@ -242,4 +242,9 @@ class EpicsFinder < IssuableFinder
   def group_projects
     Project.in_namespace(permissioned_related_groups).with_issues_available_for_user(current_user)
   end
+
+  override :feature_flag_scope
+  def feature_flag_scope
+    group
+  end
 end

ee/spec/finders/epics_finder_spec.rb

@@ -105,6 +105,22 @@ RSpec.describe EpicsFinder do
           it 'returns all epics authored by the given user' do
             expect(epics(author_id: user.id)).to contain_exactly(epic2)
           end
+
+          context 'using OR' do
+            it 'returns all epics authored by any of the given users' do
+              expect(epics(or: { author_username: [epic2.author.username, epic3.author.username] })).to contain_exactly(epic2, epic3)
+            end
+
+            context 'when feature flag is disabled' do
+              before do
+                stub_feature_flags(or_issuable_queries: false)
+              end
+
+              it 'does not add any filter' do
+                expect(epics(or: { author_username: [epic2.author.username, epic3.author.username] })).to contain_exactly(epic1, epic2, epic3)
+              end
+            end
+          end
         end
 
         context 'by label' do

spec/finders/issues_finder_spec.rb

@@ -179,33 +179,54 @@ RSpec.describe IssuesFinder do
         end
       end
 
-      context 'filtering by author ID' do
-        let(:params) { { author_id: user2.id } }
+      context 'filtering by author' do
+        context 'by author ID' do
+          let(:params) { { author_id: user2.id } }
 
-        it 'returns issues created by that user' do
-          expect(issues).to contain_exactly(issue3)
+          it 'returns issues created by that user' do
+            expect(issues).to contain_exactly(issue3)
+          end
         end
-      end
 
-      context 'filtering by not author ID' do
-        let(:params) { { not: { author_id: user2.id } } }
+        context 'using OR' do
+          let(:issue6) { create(:issue, project: project2) }
+          let(:params) { { or: { author_username: [issue3.author.username, issue6.author.username] } } }
+
+          it 'returns issues created by any of the given users' do
+            expect(issues).to contain_exactly(issue3, issue6)
+          end
+
+          context 'when feature flag is disabled' do
+            before do
+              stub_feature_flags(or_issuable_queries: false)
+            end
 
-        it 'returns issues not created by that user' do
-          expect(issues).to contain_exactly(issue1, issue2, issue4, issue5)
+            it 'does not add any filter' do
+              expect(issues).to contain_exactly(issue1, issue2, issue3, issue4, issue5, issue6)
+            end
+          end
         end
-      end
 
-      context 'filtering by nonexistent author ID and issue term using CTE for search' do
-        let(:params) do
-          {
-            author_id: 'does-not-exist',
-            search: 'git',
-            attempt_group_search_optimizations: true
-          }
+        context 'filtering by NOT author ID' do
+          let(:params) { { not: { author_id: user2.id } } }
+
+          it 'returns issues not created by that user' do
+            expect(issues).to contain_exactly(issue1, issue2, issue4, issue5)
+          end
         end
 
-        it 'returns no results' do
-          expect(issues).to be_empty
+        context 'filtering by nonexistent author ID and issue term using CTE for search' do
+          let(:params) do
+            {
+              author_id: 'does-not-exist',
+              search: 'git',
+              attempt_group_search_optimizations: true
+            }
+          end
+
+          it 'returns no results' do
+            expect(issues).to be_empty
+          end
         end
       end
 

spec/finders/merge_requests_finder_spec.rb

@@ -41,30 +41,51 @@ RSpec.describe MergeRequestsFinder do
         expect(merge_requests).to contain_exactly(merge_request1)
       end
 
-      it 'filters by nonexistent author ID and MR term using CTE for search' do
-        params = {
-          author_id: 'does-not-exist',
-          search: 'git',
-          attempt_group_search_optimizations: true
-        }
+      context 'filtering by author' do
+        subject(:merge_requests) { described_class.new(user, params).execute }
 
-        merge_requests = described_class.new(user, params).execute
+        context 'using OR' do
+          let(:params) { { or: { author_username: [merge_request1.author.username, merge_request2.author.username] } } }
 
-        expect(merge_requests).to be_empty
-      end
+          before do
+            merge_request1.update!(author: create(:user))
+            merge_request2.update!(author: create(:user))
+          end
+
+          it 'returns merge requests created by any of the given users' do
+            expect(merge_requests).to contain_exactly(merge_request1, merge_request2)
+          end
 
-      context 'filtering by not author ID' do
-        let(:params) { { not: { author_id: user2.id } } }
+          context 'when feature flag is disabled' do
+            before do
+              stub_feature_flags(or_issuable_queries: false)
+            end
 
-        before do
-          merge_request2.update!(author: user2)
-          merge_request3.update!(author: user2)
+            it 'does not add any filter' do
+              expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3, merge_request4, merge_request5)
+            end
+          end
         end
 
-        it 'returns merge requests not created by that user' do
-          merge_requests = described_class.new(user, params).execute
+        context 'with nonexistent author ID and MR term using CTE for search' do
+          let(:params) { { author_id: 'does-not-exist', search: 'git', attempt_group_search_optimizations: true } }
+
+          it 'returns no results' do
+            expect(merge_requests).to be_empty
+          end
+        end
 
-          expect(merge_requests).to contain_exactly(merge_request1, merge_request4, merge_request5)
+        context 'filtering by not author ID' do
+          let(:params) { { not: { author_id: user2.id } } }
+
+          before do
+            merge_request2.update!(author: user2)
+            merge_request3.update!(author: user2)
+          end
+
+          it 'returns merge requests not created by that user' do
+            expect(merge_requests).to contain_exactly(merge_request1, merge_request4, merge_request5)
+          end
         end
       end