solr_facet_params defaults to sort=index #976
Conversation
|
Sorry, I can't get tests to run locally, so have to do the thing where I let travis do it for me, wait 20 minutes for travis, correct what travis found, etc. Will get tests passing again next week, unless someone says this is not a direction to bother with. thanks! |
| @@ -224,7 +224,7 @@ def solr_facet_params(facet_field, user_params=params || {}, extra_controller_pa | |||
| # override any field-specific default in the solr request handler. | |||
| solr_params[:"f.#{facet_field}.facet.limit"] = limit + 1 | |||
| solr_params[:"f.#{facet_field}.facet.offset"] = ( input.fetch(Blacklight::Solr::FacetPaginator.request_keys[:page] , 1).to_i - 1 ) * ( limit ) | |||
| solr_params[:"f.#{facet_field}.facet.sort"] = input[ Blacklight::Solr::FacetPaginator.request_keys[:sort] ] if input[ Blacklight::Solr::FacetPaginator.request_keys[:sort] ] | |||
| solr_params[:"f.#{facet_field}.facet.sort"] = input[ Blacklight::Solr::FacetPaginator.request_keys[:sort] ] || "index" | |||
There was a problem hiding this comment.
I'm not sure this is the right solution:
-
do we always want to default to sorting by index? It seems typical, at least in our use, to default to sorting by count. I also think sorting by count is the solr default.
-
if i read this right, we'll be sending (number of facets) parameters on every search. Wouldn't it be better to inspect the echo'ed params and parse that our appropriately? It looks like we have code to do this (and that defaults to count) here:
But you're right, it seems like it isn't working correctly.
There was a problem hiding this comment.
I am not sure that we have guaranteed access to echo'd parameters -- does it depend on if Solr is set up to echo, or does BL ask for echo'd parameters regardless?
But if you don't like this solution, I won't do more work to try and make the tests pass (I still can't get tests to run locally on my machine despite many frustrating hours spent on it).
But I really do want a bugfix in my app that's demonstrating the bug, along with the BL demo. I don't really have time to work on figuring out or implementing a more complete solution right now. Is there any chance you'd accept this simple solution until someone has the time and inclination to implement a better one?
There was a problem hiding this comment.
Or does #977 end up taking care of this for the standard case?
The Blacklight action for listing individual facet values (eg from a 'more' click on facet)
in the default/common case does not send a facet.sort parameter, so gets the Solr default
of sort by 'count'.
However, the parts of BL that display the individual facet list assume that if no explicit sort is given, the sort will be 'index' (ie alphabetical, more or less).
This leads to a mis-match you can see in the demo app, where the values are sorted by 'count',
but in the sort choices at the bottom of the screen, the 'A-Z Sort' choice is mistakenly shown as selected.
Eg: http://demo.projectblacklight.org/catalog/facet/language_facet
This commit makes 'index' the default sort in fetching again, as the UX expects, and as used to be the case in older versions of BL when the UX was written.
This is just the simplest thing to make the common case seen in the demo app consistent again. Things that might be nice but we are not doing here:
input[ Blacklight::Solr::FacetPaginator.request_keys[:sort] ]It just restores the common standard case, as displayed in the demo app, to consistency with as simple code as possible, returning BL to how it worked in past versions (not entirely sure when it changed).