From c2fc40668c34215a7e727e60647114f1b178eb8c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 20 Feb 2018 12:33:49 +0000 Subject: [PATCH] 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. --- .../concerns/issuable_collections.rb | 4 +- app/controllers/concerns/issues_action.rb | 8 +- .../concerns/merge_requests_action.rb | 7 +- app/controllers/projects/issues_controller.rb | 5 +- .../projects/merge_requests_controller.rb | 5 +- app/controllers/projects_controller.rb | 5 +- app/finders/issuable_finder.rb | 106 +++++++----------- app/finders/issues_finder.rb | 45 ++++++++ .../concerns/issuable_collections_spec.rb | 4 + 9 files changed, 111 insertions(+), 78 deletions(-) diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 0d7ee06deb6..f7ba305a59f 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/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 diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb index 3ba1235cee0..3b11a373368 100644 --- a/app/controllers/concerns/issues_action.rb +++ b/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 diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb index a9cc13038bf..b70db99b157 100644 --- a/app/controllers/concerns/merge_requests_action.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 33fced99132..73806454525 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 8eed957d9fe..a1af125547c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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! diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 72573e0765d..0370edc6e20 100644 --- a/app/controllers/projects_controller.rb +++ b/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' diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 0fe3000ca01..384a336e2bb 100644 --- a/app/finders/issuable_finder.rb +++ b/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] diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 83245aadf6e..d65c620e75a 100644 --- a/app/finders/issues_finder.rb +++ b/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) diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb index d7825364ed5..c1f42bbb9d7 100644 --- a/spec/controllers/concerns/issuable_collections_spec.rb +++ b/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