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

Filter facets #18

Merged
merged 9 commits into from
Sep 9, 2019
Merged
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
18 changes: 18 additions & 0 deletions lib/hal_api/controller/filtering.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,24 @@ def filters
@filters ||= parse_filters_param
end

def filter_facets
Copy link
Member Author

Choose a reason for hiding this comment

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

By default, the facets attribute won't render at all. You have to provide this method in your controller.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense to me - how does the API indicate which fields can be used as facets?
e.g. type, status, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

The specifics of this were implemented in Augury (i.e. left up to the specific app). Ended up being a hash of "facet name" (which is the ?filters= key name) to a list of options for the facet.

end

def index_collection
collection = defined?(super) ? super : HalApi::PagedCollection.new([])
Copy link
Member

Choose a reason for hiding this comment

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

I kinda thought some of this might end up in or extending paged collection, but I can see the separation of the controller knowing about the facets and setting them, like with other model data. I wonder if the controller or the paged collection should be more aware of the structure of it though...probably not, probably overkill to have mutator and other methods on the collection...maybe a method to add/set a facet?

I guess I am unsure where to put all the filtering functionality. I think your move here to keep it in a concern/mixin may actually be better, as it keeps most of the logic in the same place.

Copy link
Member Author

@cavis cavis Sep 5, 2019

Choose a reason for hiding this comment

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

I could see doing an optimization like that. But to start, I wanted to leave that up to the implementing app (augury.prx.org in this case) and see where that takes us. Rather than locking them into a more controlled data structure.


# add facets if defined, removing filters/facets with counts of 0
non_zero_facets = (filter_facets || {}).with_indifferent_access.tap do |hash|
Copy link
Member

Choose a reason for hiding this comment

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

Should the filter_key have more than just an id? Should there be a human readable label for it?
e.g. some_custom_filter would not be the best for display.

Copy link
Member

Choose a reason for hiding this comment

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

I have talked myself out of this, withdrawn!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd lump this in with "describing all available filters", which is sort of out-of-scope for this PR now. Opened #19.

hash.each do |filter_key, facets|
hash[filter_key] = facets.try(:select) { |f| f.try(:[], :count) > 0 }
hash.delete(filter_key) if hash[filter_key].blank?
end
end
collection.facets = non_zero_facets unless non_zero_facets.blank?

collection
end

private

def parse_filters_param
Expand Down
4 changes: 4 additions & 0 deletions lib/hal_api/controller/resources.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ def resources_base
self.class.resource_class.where(nil)
end

def resources_query
Copy link
Member Author

Choose a reason for hiding this comment

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

Convenience method for controllers implementing filter_facets. For instance, in the Augury campaigns controller I found myself doing:

def filter_facets
  {
    type: resources_query.group(:type).count,
    status: resources_query.group(:status).count,
  }
end

filtered(scoped(resources_base))
end

def find_base
filtered(scoped(included(resources_base)))
end
Expand Down
2 changes: 1 addition & 1 deletion lib/hal_api/paged_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class HalApi::PagedCollection
extend ActiveModel::Naming
extend Forwardable

attr_accessor :items, :request, :options
attr_accessor :items, :request, :options, :facets

def_delegators :items, :total_count, :prev_page, :next_page, :total_pages, :first_page?, :last_page?
alias_method :total, :total_count
Expand Down
2 changes: 1 addition & 1 deletion lib/hal_api/rails/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module HalApi
module Rails
VERSION = "0.6.0"
VERSION = "0.7.0"
end
end
1 change: 1 addition & 0 deletions lib/hal_api/representer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ class HalApi::Representer < Roar::Decorator
include HalApi::Representer::Caches
include HalApi::Representer::LinkSerialize
self_link
vary_link
profile_link
end
10 changes: 10 additions & 0 deletions lib/hal_api/representer/collection_paging.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module HalApi::Representer::CollectionPaging
class_eval do
property :count
property :total
property :facets

embeds :items, decorator: lambda{|*| item_decorator }, class: lambda{|*| item_class }, zoom: :always

Expand Down Expand Up @@ -36,6 +37,14 @@ def self_url(represented)
href_url_helper(represented.params)
end

def vary_url(represented)
href_url_helper(represented.params.except(*vary_params))
Copy link
Member Author

Choose a reason for hiding this comment

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

It was necessary to remove any of the "varied query params" before getting the url.

end

def vary_params
%w(page per zoom filters sorts)
end

def profile_url(represented)
model_uri(:collection, represented.item_class)
end
Expand All @@ -46,6 +55,7 @@ def profile_url(represented)
# if it is a lambda, execute in the context against the represented.parent (if there is one) or represented
def href_url_helper(options={})
if represented_url.nil?
options = options.except(:format)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an existing bug/annoyance where paged collections always had a self link with .json in it.

Copy link
Member

Choose a reason for hiding this comment

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

oh yuck, good fix!

result = url_for(options.merge(only_path: true)) rescue nil
if represented.parent
result ||= polymorphic_path([:api, represented.parent, represented.item_class], options) rescue nil
Expand Down
21 changes: 21 additions & 0 deletions lib/hal_api/representer/uri_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ def self_link
end
end

def vary_link
link(:vary) do
{
href: vary_url(represented) + vary_query_params,
templated: true,
} if vary_url(represented).present? && vary_params.present?
end
end

def profile_link
link(:profile) { profile_url(represented) }
end
Expand All @@ -46,6 +55,18 @@ def self_url(represented)
polymorphic_path([:api, rep])
end

def vary_url(represented)
self_url(represented)
end

def vary_params
[]
end

def vary_query_params
"{?#{vary_params.join(',')}}"
end

def becomes_represented_class(rep)
return rep unless rep.respond_to?(:becomes)
klass = rep.try(:item_class) || rep.class.try(:base_class)
Expand Down
35 changes: 35 additions & 0 deletions test/hal_api/controller/filtering_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,25 @@ class FilteringTestController < ActionController::Base
filter_params :one, :two, :three, :four, :five, six: :date, seven: :time

attr_accessor :filter_string
attr_accessor :no_facets

def params
{ filters: filter_string }
end

def filter_facets
{
one: [{count: 2}],
two: [],
three: [
{id: 'a', count: 4},
{id: 'b', count: 5},
{id: 'c', count: 0}
],
four: [{count: 0}],
five: [{id: 'a', count: 0}]
} unless no_facets
end
end

let(:controller) { FilteringTestController.new }
Expand Down Expand Up @@ -84,4 +99,24 @@ def params
err.must_be_instance_of(HalApi::Errors::BadFilterValueError)
end

it 'sets facets on the collection' do
controller.index_collection.facets[:one].must_equal [{'count' => 2}]
controller.index_collection.facets[:three].count.must_equal 2
controller.index_collection.facets[:three][0].must_equal({'id' => 'a', 'count' => 4})
controller.index_collection.facets[:three][1].must_equal({'id' => 'b', 'count' => 5})
end

it 'removes empty facets' do
controller.index_collection.facets.keys.must_include 'one'
controller.index_collection.facets.keys.must_include 'three'
controller.index_collection.facets[:three].map { |f| f['id'] }.wont_include 'c'
controller.index_collection.facets.keys.wont_include 'four'
controller.index_collection.facets.keys.wont_include 'five'
controller.index_collection.facets.keys.wont_include 'six'
end

it 'does not set facets if there are none' do
controller.no_facets = true
controller.index_collection.facets.must_be_nil
end
end
7 changes: 7 additions & 0 deletions test/hal_api/paged_collection_representer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
representer.represented_url.must_equal "api_stories_path"
end

it 'has a vary link' do
representer.represented.options[:url] = "api_stories_path"
json['_links']['vary'].wont_be_nil
json['_links']['vary']['href'].must_equal 'api_stories_path{?page,per,zoom,filters,sorts}'
json['_links']['vary']['templated'].must_equal true
end

it 'uses a lambda for a url method' do
representer.represented.options[:url] = ->(options){ options.keys.sort.join('/') }
representer.href_url_helper({foo: 1, bar: 2, camp: 3}).must_equal "bar/camp/foo"
Expand Down