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

Fixes #765 - single => true' on facet now works on "more" screen #767

Closed
wants to merge 6 commits into from

Conversation

eranhirs
Copy link
Contributor

Fixes #765
The bug was:
In :solr_facet_params which queries solr when opening the more window, we do this:

# Now override with our specific things for fetching facet values
solr_params[:"facet.field"] = facet_field

The problem is that we override logic that :add_facetting_to_solr is doing for special usecases like query facets, pivot facets and more..

The changes are:
*1 - Seperated some of :add_facetting_to_solr logic to a new method named :handle_advanced_facets
*2 - Now I can call the :handle_advanced_facets method also from :solr_facet_params
*3 - In the overriding line I changed facet_field from single value to an array

The fixes:
*1 - as described in #765.
*2 - this will fix many problems with the 'more' screen we didn't reach.

@cbeer
Copy link
Member

cbeer commented Feb 11, 2014

I'd like to take a closer look at this and hopefully will have more comments, but before merging this PR should:

  • be rebased
  • have the indentations fixed (I'm not sure if you're using 4 spaces, a hard tab, etc.. I think our style is just 2 spaces)

@@ -493,8 +499,10 @@ def solr_facet_params(facet_field, user_params=params || {}, extra_controller_pa
solr_params = solr_search_params(user_params).merge(extra_controller_params)

# Now override with our specific things for fetching facet values
solr_params[:"facet.field"] = facet_field
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to re-use #add_facetting_to_solr in its entirety? I think there's some other stuff going on there that we might want too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wanted is to reuse :add_facetting_to_solr by sending the field_name with user_params, then in :add_facetting_to_solr return only one field in solr_params[:"facet.field"] if field_name is set.
This way we can delete the override line in :solr_facet_params.

To do that:
*1 - we need to refactor some more code, for example:

solr_parameters.append_facet_fields blacklight_config.facet_fields_to_add_to_solr

flattens to an array all the facet_fields, we need to change it too.

*2 - I think there is nothing more we need from :add_facetting_to_solr

@cbeer cbeer added this to the 5.1.0 milestone Feb 11, 2014
@eranhirs
Copy link
Contributor Author

Quite a mess with the indentation, I fixed it for the whole solr_helper.rb which made a lot of line changes.
Is it clear like this or should I fix it only in my code?

@cbeer
Copy link
Member

cbeer commented Feb 12, 2014

Replaced by #777

@cbeer cbeer closed this Feb 12, 2014
@cbeer
Copy link
Member

cbeer commented Feb 12, 2014

@eranhirs I've replaced this with #777, which fixes the same problem and fixes some other problems with the catalog#facet action too. Thanks for the contribution and the bug report.

@eranhirs
Copy link
Contributor Author

Great as long as the bug is fixed! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting ':single => true' on a facet does not affect the "more" screen
2 participants