-
Notifications
You must be signed in to change notification settings - Fork 0
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
Filter facets #18
Conversation
@@ -53,6 +53,28 @@ def filters | |||
@filters ||= parse_filters_param | |||
end | |||
|
|||
def filter_facets |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.?
There was a problem hiding this comment.
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.
@@ -75,6 +75,10 @@ def resources_base | |||
self.class.resource_class.where(nil) | |||
end | |||
|
|||
def resources_query |
There was a problem hiding this comment.
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
@@ -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)) |
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yuck, good fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions.
@@ -53,6 +53,28 @@ def filters | |||
@filters ||= parse_filters_param | |||
end | |||
|
|||
def filter_facets |
There was a problem hiding this comment.
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.?
end | ||
|
||
def index_collection | ||
collection = defined?(super) ? super : HalApi::PagedCollection.new([]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
collection = defined?(super) ? super : HalApi::PagedCollection.new([]) | ||
|
||
# add facets if defined, removing filters/facets with counts of 0 | ||
non_zero_facets = (filter_facets || {}).with_indifferent_access.tap do |hash| |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yuck, good fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
questions answered - lgtm
Adds filter facet attributes into the paged collection. It's up to the client to know how to interpret them, and change the
?filter
param.Also adds a
vary
link to paged collections, which gives you a templated link to this collection.The idea with
facets
is that they change as you apply filters, and represent what counts would return if you applied that filter+value in addition to any filters you currently have applied. There is nothing in the JSON to tell you what filters are currently applied (unless you parse it out of theself
link). So the client had better be tracking its own filter state.The
vary
link only appears if there are query params to vary (set by thevary_params
method). By default here, baseHalApi::Representer
s do not have a vary link (though you could argue we should allow varying by?zoom
). And paged representers do have a vary link, including?page,per,zoom,filters,sorts
which are all part of the HalApi::Controller by default.