Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More filtering cleanup #945

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/filters/base_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
# BaseFilter is more like an abstract parent class, but it also serves
# as a NullObject version of the filter grouping, if you want one
class BaseFilter
# This constant should be overriden if you need the ContributionController to
# accept additional parameters for this filter to work
ALLOWED_PARAMS = {}

# This method should be overridden to return a hash with the following keys:
# * :name => a short string that the user will see that describes what type of filters these are
# * :filters => the output a call to FilterOptionBlueprint.render_as_hash that represent each filter option to check or uncheck
Expand Down
7 changes: 5 additions & 2 deletions app/filters/category_filter.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
class CategoryFilter < BaseFilter
PARAM_NAME = 'Category'
ALLOWED_PARAMS = {PARAM_NAME => {}}

def self.filter_grouping
{
name: "Categories",
Expand All @@ -8,9 +11,9 @@ def self.filter_grouping
end

def filter(scope)
return super unless parameters
return super if parameters[PARAM_NAME].blank?
scope.tagged_with(
Category.roots.where(id: parameters.keys).pluck('name'),
Category.roots.where(id: parameters[PARAM_NAME].keys).pluck('name'),
any: true
)
end
Expand Down
7 changes: 5 additions & 2 deletions app/filters/contact_method_filter.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
class ContactMethodFilter < BaseFilter
PARAM_NAME = 'ContactMethod'
ALLOWED_PARAMS = {PARAM_NAME => {}}

def self.filter_grouping
{
name: 'Contact Methods',
Expand All @@ -7,7 +10,7 @@ def self.filter_grouping
end

def filter(scope)
return super unless parameters
scope.joins(:person).where(people: { preferred_contact_method: parameters.keys })
return super if parameters[PARAM_NAME].blank?
scope.joins(:person).where(people: { preferred_contact_method: parameters[PARAM_NAME].keys })
end
end
9 changes: 6 additions & 3 deletions app/filters/contribution_type_filter.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
class ContributionTypeFilter
PARAM_NAME = 'ContributionType'
ALLOWED_PARAMS = {PARAM_NAME => {}}

def self.filter_grouping
{ name: 'Contribution Types', filter_options: [
{ id: 'ContributionType[Ask]', name: 'Ask' },
{ id: 'ContributionType[Offer]', name: 'Offer' }
{ id: "#{PARAM_NAME}[Ask]", name: 'Ask' },
{ id: "#{PARAM_NAME}[Offer]", name: 'Offer' }
]}
end
ALL_ALLOWED_TYPES = ['Ask', 'Offer'].freeze
Expand All @@ -14,7 +17,7 @@ def initialize(params)
end

def scopes
classes = parameters.blank? ? ALL_ALLOWED_TYPES : parameters.keys
classes = parameters.blank? || parameters[PARAM_NAME].blank? ? ALL_ALLOWED_TYPES : parameters[PARAM_NAME].keys
classes.intersection(ALL_ALLOWED_TYPES).map do |type|
type.constantize.matchable
end
Expand Down
7 changes: 5 additions & 2 deletions app/filters/service_area_filter.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
class ServiceAreaFilter < BaseFilter
PARAM_NAME = 'ServiceArea'
ALLOWED_PARAMS = {PARAM_NAME => {}}

def self.filter_grouping
{
name: 'Service Areas',
Expand All @@ -7,7 +10,7 @@ def self.filter_grouping
end

def filter(scope)
return super unless parameters
scope.where(service_area_id: parameters.keys)
return super if parameters[PARAM_NAME].blank?
scope.where(service_area_id: parameters[PARAM_NAME].keys)
end
end
16 changes: 16 additions & 0 deletions app/filters/urgency_filter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class UrgencyFilter < BaseFilter
PARAM_NAME = 'UrgencyLevel'
ALLOWED_PARAMS = {PARAM_NAME => {}}

def self.filter_grouping
{
name: 'Urgency',
filter_options: FilterOptionBlueprint.render_as_hash(UrgencyLevel::TYPES)
}
end

def filter(scope)
return super if parameters[PARAM_NAME].blank?
scope.where(urgency_level_id: parameters[PARAM_NAME].keys)
end
end
27 changes: 13 additions & 14 deletions app/models/browse_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,15 @@
# BrowseFilter lets the browse page filter contributions by parameters, delegating much of the actual filtering
# to the classes listed in BrowseFilter::FILTER_CLASSES
class BrowseFilter
FILTERS = {
'Category' => CategoryFilter,
'ServiceArea' => ServiceAreaFilter,
'ContactMethod' => ContactMethodFilter,
'ContributionType' => ContributionTypeFilter
# 'UrgencyLevel' => UrgencyLevelFilter
}.freeze
FILTER_CLASSES = FILTERS.values.freeze
ALLOWED_PARAMS = FILTERS.keys.each_with_object({}) do |key, hash|
hash[key] = {}
FILTER_CLASSES = [
CategoryFilter,
ServiceAreaFilter,
ContactMethodFilter,
ContributionTypeFilter,
UrgencyFilter
].freeze
ALLOWED_PARAMS = FILTER_CLASSES.each_with_object({}) do |filter, hash|
hash.merge! filter::ALLOWED_PARAMS
Comment on lines +13 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i look at this, i'm wondering if we even need ALLOWED_PARAMS anymore? Since each filter will extract only the params it needs, maybe ContributionsController doesn't need to invoke strong_params at all? Perhaps that addresses some of the duplication you mentioned?

end.freeze

attr_reader :parameters
Expand All @@ -29,16 +28,16 @@ def initialize(parameters)
def contributions
# ContributionTypeFilter is special and needs to come first because of Single Table Inheretence
# Currently, the only other option seemed to be pulling in a gem that supports UNION queries in SQL
starting_relations = ContributionTypeFilter.new(parameters['ContributionType']).scopes
starting_relations = ContributionTypeFilter.new(parameters).scopes

# So using whatever relations ContributionTypeFilter gives us (unmatched asks, unmatched offers, etc.)
@contributions ||= FILTERS.reduce(starting_relations) do |resulting_relations, (filter_name, klass)|
@contributions ||= FILTER_CLASSES.reduce(starting_relations) do |resulting_relations, filter_class|
# Skip ContributionTypeFilter because we've already used it
next resulting_relations if klass == ContributionTypeFilter
next resulting_relations if filter_class == ContributionTypeFilter

# then filter the relations further based on the rules in each class for doing so
resulting_relations.map do |scope|
klass.new(parameters[filter_name]).filter(scope)
filter_class.new(parameters).filter(scope)
end
end.flatten
end
Expand Down