Refactor IssuableFinder to extract model-specific logic

By extracting a new `filter_items` method, we can override that in the
IssuesFinder and MergeRequestsFinder separately, so we don't need checks that
the model is the correct one, because we can just use the class we're in to know
that.

We can do the same for the VALID_PARAMS constant, by making it a class method.

app/controllers/concerns/issuable_collections.rb

@@ -103,7 +103,7 @@ module IssuableCollections
       # @filter_params[:authorized_only] = true
     end
 
-    @filter_params.permit(IssuableFinder::VALID_PARAMS)
+    @filter_params.permit(finder_type.valid_params)
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
@@ -148,7 +148,7 @@ module IssuableCollections
 
   def finder
     strong_memoize(:finder) do
-      issuable_finder_for(@finder_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      issuable_finder_for(finder_type)
     end
   end
 

app/controllers/concerns/issues_action.rb

@@ -4,7 +4,6 @@ module IssuesAction
 
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def issues
-    @finder_type = IssuesFinder
     @issues = issuables_collection
               .non_archived
               .page(params[:page])
@@ -17,4 +16,11 @@ module IssuesAction
     end
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
+  private
+
+  def finder_type
+    (super if defined?(super)) ||
+      (IssuesFinder if action_name == 'issues')
+  end
 end

app/controllers/concerns/merge_requests_action.rb

@@ -4,8 +4,6 @@ module MergeRequestsAction
 
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def merge_requests
-    @finder_type = MergeRequestsFinder
-
     @merge_requests = issuables_collection.page(params[:page])
 
     @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
@@ -14,6 +12,11 @@ module MergeRequestsAction
 
   private
 
+  def finder_type
+    (super if defined?(super)) ||
+      (MergeRequestsFinder if action_name == 'merge_requests')
+  end
+
   def filter_params
     super.merge(non_archived: true)
   end

app/controllers/projects/issues_controller.rb

@@ -247,9 +247,8 @@ class Projects::IssuesController < Projects::ApplicationController
     Issues::UpdateService.new(project, current_user, update_params)
   end
 
-  def set_issuables_index
-    @finder_type = IssuesFinder
-    super
+  def finder_type
+    IssuesFinder
   end
 
   def whitelist_query_limiting

app/controllers/projects/merge_requests_controller.rb

@@ -333,9 +333,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @target_branches = @merge_request.target_project.repository.branch_names
   end
 
-  def set_issuables_index
-    @finder_type = MergeRequestsFinder
-    super
+  def finder_type
+    MergeRequestsFinder
   end
 
   def check_user_can_push_to_source_branch!

app/controllers/projects_controller.rb

@@ -282,7 +282,6 @@ class ProjectsController < Projects::ApplicationController
         @project_wiki = @project.wiki
         @wiki_home = @project_wiki.find_page('home', params[:version_id])
       elsif @project.feature_available?(:issues, current_user)
-        @finder_type = IssuesFinder
         @issues = issuables_collection.page(params[:page])
         @collection_type = 'Issue'
         @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
@@ -292,6 +291,10 @@ class ProjectsController < Projects::ApplicationController
     end
   end
 
+  def finder_type
+    IssuesFinder
+  end
+
   def determine_layout
     if [:new, :create].include?(action_name.to_sym)
       'application'

app/finders/issuable_finder.rb

@@ -25,36 +25,38 @@ class IssuableFinder
 
   NONE = '0'.freeze
 
-  SCALAR_PARAMS = %i[
-    assignee_id
-    assignee_username
-    author_id
-    author_username
-    authorized_only
-    due_date
-    group_id
-    iids
-    label_name
-    milestone_title
-    my_reaction_emoji
-    non_archived
-    project_id
-    scope
-    search
-    sort
-    state
-    include_subgroups
-  ].freeze
-  ARRAY_PARAMS = { label_name: [], iids: [], assignee_username: [] }.freeze
-
-  EE_SCALAR_PARAMS = %i[
-    weight
-  ].freeze
-
-  VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_PARAMS] + EE_SCALAR_PARAMS).freeze
-
   attr_accessor :current_user, :params
 
+  def self.scalar_params
+    @scalar_params ||= %i[
+      assignee_id
+      assignee_username
+      author_id
+      author_username
+      authorized_only
+      group_id
+      iids
+      label_name
+      milestone_title
+      my_reaction_emoji
+      non_archived
+      project_id
+      scope
+      search
+      sort
+      state
+      include_subgroups
+    ]
+  end
+
+  def self.array_params
+    @array_params ||= { label_name: [], iids: [], assignee_username: [] }
+  end
+
+  def self.valid_params
+    @valid_params ||= scalar_params + [array_params]
+  end
+
   def initialize(current_user, params = {})
     @current_user = current_user
     @params = params
@@ -62,6 +64,15 @@ class IssuableFinder
 
   def execute
     items = init_collection
+    items = filter_items(items)
+
+    # Filtering by project HAS TO be the last because we use the project IDs yielded by the issuable query thus far
+    items = by_project(items)
+
+    sort(items)
+  end
+
+  def filter_items(items)
     items = by_scope(items)
     items = by_created_at(items)
     items = by_state(items)
@@ -69,17 +80,11 @@ class IssuableFinder
     items = by_search(items)
     items = by_assignee(items)
     items = by_author(items)
-    items = by_weight(items)
-    items = by_due_date(items)
     items = by_non_archived(items)
     items = by_iids(items)
     items = by_milestone(items)
     items = by_label(items)
-    items = by_my_reaction_emoji(items)
-
-    # Filtering by project HAS TO be the last because we use the project IDs yielded by the issuable query thus far
-    items = by_project(items)
-    sort(items)
+    by_my_reaction_emoji(items)
   end
 
   def find(*params)
@@ -381,31 +386,6 @@ class IssuableFinder
     items
   end
 
-  def by_weight(items)
-    return items unless weights?
-
-    if filter_by_no_weight?
-      items.where(weight: [-1, nil])
-    elsif filter_by_any_weight?
-      items.where.not(weight: [-1, nil])
-    else
-      items.where(weight: params[:weight])
-    end
-  end
-
-  def weights?
-    params[:weight].present? && params[:weight] != Issue::WEIGHT_ALL &&
-      klass.column_names.include?('weight')
-  end
-
-  def filter_by_no_weight?
-    params[:weight] == Issue::WEIGHT_NONE
-  end
-
-  def filter_by_any_weight?
-    params[:weight] == Issue::WEIGHT_ANY
-  end
-
   def by_my_reaction_emoji(items)
     if params[:my_reaction_emoji].present? && current_user
       items = items.awarded(current_user, params[:my_reaction_emoji])
@@ -414,42 +394,6 @@ class IssuableFinder
     items
   end
 
-  def by_due_date(items)
-    if due_date?
-      if filter_by_no_due_date?
-        items = items.without_due_date
-      elsif filter_by_overdue?
-        items = items.due_before(Date.today)
-      elsif filter_by_due_this_week?
-        items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week)
-      elsif filter_by_due_this_month?
-        items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month)
-      end
-    end
-
-    items
-  end
-
-  def filter_by_no_due_date?
-    due_date? && params[:due_date] == Issue::NoDueDate.name
-  end
-
-  def filter_by_overdue?
-    due_date? && params[:due_date] == Issue::Overdue.name
-  end
-
-  def filter_by_due_this_week?
-    due_date? && params[:due_date] == Issue::DueThisWeek.name
-  end
-
-  def filter_by_due_this_month?
-    due_date? && params[:due_date] == Issue::DueThisMonth.name
-  end
-
-  def due_date?
-    params[:due_date].present? && klass.column_names.include?('due_date')
-  end
-
   def label_names
     if labels?
       params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name]

app/finders/issues_finder.rb

@@ -16,10 +16,17 @@
 #     sort: string
 #     my_reaction_emoji: string
 #     public_only: boolean
+#     due_date: date or '0', '', 'overdue', 'week', or 'month'
 #
 class IssuesFinder < IssuableFinder
+  prepend EE::IssuesFinder
+
   CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
 
+  def self.scalar_params
+    @scalar_params ||= super + [:due_date]
+  end
+
   def klass
     Issue.includes(:author)
   end
@@ -52,6 +59,46 @@ class IssuesFinder < IssuableFinder
     params.fetch(:public_only, false)
   end
 
+  def filter_items(items)
+    by_due_date(super)
+  end
+
+  def by_due_date(items)
+    if due_date?
+      if filter_by_no_due_date?
+        items = items.without_due_date
+      elsif filter_by_overdue?
+        items = items.due_before(Date.today)
+      elsif filter_by_due_this_week?
+        items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week)
+      elsif filter_by_due_this_month?
+        items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month)
+      end
+    end
+
+    items
+  end
+
+  def filter_by_no_due_date?
+    due_date? && params[:due_date] == Issue::NoDueDate.name
+  end
+
+  def filter_by_overdue?
+    due_date? && params[:due_date] == Issue::Overdue.name
+  end
+
+  def filter_by_due_this_week?
+    due_date? && params[:due_date] == Issue::DueThisWeek.name
+  end
+
+  def filter_by_due_this_month?
+    due_date? && params[:due_date] == Issue::DueThisMonth.name
+  end
+
+  def due_date?
+    params[:due_date].present?
+  end
+
   def user_can_see_all_confidential_issues?
     return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
 

ee/app/controllers/ee/boards/issues_controller.rb

@@ -4,7 +4,7 @@ module EE
       def issues_finder
         return super unless board.group_board?
 
-        IssuesFinder.new(current_user, group_id: board_parent.id)
+        ::IssuesFinder.new(current_user, group_id: board_parent.id)
       end
 
       def project

ee/app/controllers/groups/epics_controller.rb

@@ -70,9 +70,8 @@ class Groups::EpicsController < Groups::ApplicationController
     Epics::UpdateService.new(nil, current_user, epic_params)
   end
 
-  def set_issuables_index
-    @finder_type = EpicsFinder
-    super
+  def finder_type
+    EpicsFinder
   end
 
   def collection_type

ee/app/finders/ee/issues_finder.rb

+module EE
+  module IssuesFinder
+    extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
+
+    module ClassMethods
+      extend ::Gitlab::Utils::Override
+
+      override :scalar_params
+      def scalar_params
+        @scalar_params ||= super + [:weight]
+      end
+    end
+
+    override :filter_items
+    def filter_items(items)
+      by_weight(super)
+    end
+
+    private
+
+    def by_weight(items)
+      return items unless weights?
+
+      if filter_by_no_weight?
+        items.where(weight: [-1, nil])
+      elsif filter_by_any_weight?
+        items.where.not(weight: [-1, nil])
+      else
+        items.where(weight: params[:weight])
+      end
+    end
+
+    def weights?
+      params[:weight].present? && params[:weight] != ::Issue::WEIGHT_ALL
+    end
+
+    def filter_by_no_weight?
+      params[:weight] == ::Issue::WEIGHT_NONE
+    end
+
+    def filter_by_any_weight?
+      params[:weight] == ::Issue::WEIGHT_ANY
+    end
+  end
+end

spec/controllers/concerns/issuable_collections_spec.rb

@@ -8,6 +8,10 @@ describe IssuableCollections do
       def self.helper_method(name); end
 
       include IssuableCollections
+
+      def finder_type
+        IssuesFinder
+      end
     end
 
     controller = klass.new