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.
This commit is contained in:
parent
5048c8d505
commit
c2fc40668c
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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!
|
||||
|
|
|
@ -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'
|
||||
|
|
|
@ -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]
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
|
@ -8,6 +8,10 @@ describe IssuableCollections do
|
|||
def self.helper_method(name); end
|
||||
|
||||
include IssuableCollections
|
||||
|
||||
def finder_type
|
||||
IssuesFinder
|
||||
end
|
||||
end
|
||||
|
||||
controller = klass.new
|
||||
|
|
Loading…
Reference in New Issue