Merge branch 'refactor-issuable-finder-to-use-inheritance' into 'master'

Refactor IssuableFinder to extract model-specific logic

See merge request gitlab-org/gitlab-ce!17236

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
 
@@ -146,7 +146,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

@@ -243,9 +243,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

@@ -323,9 +323,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

@@ -279,7 +279,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)
@@ -289,6 +288,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,32 +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
-
-  VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_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
@@ -58,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)
@@ -65,16 +80,11 @@ class IssuableFinder
     items = by_search(items)
     items = by_assignee(items)
     items = by_author(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)
@@ -396,42 +406,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,15 @@
 #     sort: string
 #     my_reaction_emoji: string
 #     public_only: boolean
+#     due_date: date or '0', '', 'overdue', 'week', or 'month'
 #
 class IssuesFinder < IssuableFinder
   CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
 
+  def self.scalar_params
+    @scalar_params ||= super + [:due_date]
+  end
+
   def klass
     Issue.includes(:author)
   end
@@ -52,6 +57,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)
 

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