When a facet uses the OR operator in an hierarchical structure, the parent item should be ignored if a child is active

This issue is for the search_api solution

More details, see: #2211945: Logic flaw using OR operator with hierarchy

Comments

strykaizer’s picture

Status: Active » Needs review
StatusFileSize
new1.05 KB

Attached you can find a patch to solve this issue

Status: Needs review » Needs work

The last submitted patch, 1: ignore_parent_terms_in-2450227-1.patch, failed testing.

strykaizer’s picture

Status: Needs work » Needs review
StatusFileSize
new1.09 KB
FloriNet’s picture

Thank you so much @StryKaizer, patch in #3 works perfectly!

drunken monkey’s picture

StatusFileSize
new1.56 KB

Thanks for opening this and already providing a working patch! Looks good.
I just made a few changes:

+++ b/contrib/search_api_facetapi/plugins/facetapi/query_type_term.inc
@@ -54,6 +54,18 @@ class SearchApiFacetapiTerm extends FacetapiQueryType implements FacetapiQueryTy
+        foreach($active as $filter => $filter_array){

First off, Drupal coding standards require spaces around the parentheses here (and in the foreach loop header below).

Secondly, looping over the filters in this order will first check the parents and then the children, resulting in unnecessary calls to taxonomy_get_parents_all(). Reversing the order should be faster here.

I fixed this and improved the comments a bit, see the attached patch. Please verify it still works for you and I'll commit it in the next days.

strykaizer’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the improvements and nice catch on reversing the order for performance reasons.

I tested the patch in #5 and it works perfectly.

drunken monkey’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Good to hear, thanks for testing!
Committed. Thanks again!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

twod’s picture

Status: Closed (fixed) » Active
+++ b/contrib/search_api_facetapi/plugins/facetapi/query_type_term.inc
@@ -54,6 +54,27 @@ class SearchApiFacetapiTerm extends FacetapiQueryType implements FacetapiQueryTy
+      // When the operator is OR, remove parent terms from the active ones if
+      // children are active. If we don't do this, sending a term and its
+      // parent will produce the same results as just sending the parent.

This comment is wrong unless you have configured the index to be hierarchical.
I want to display the hierarchy, but have it indexed as a flat list of terms so I have set it to not index terms hierarchically. That is no longer possible as selecting both a parent term and its child term will only show content tagged with the child term.

twod’s picture

Status: Active » Needs review
StatusFileSize
new1.3 KB

How about not ignoring the parents when hierarchical indexing is disabled for a field?

drunken monkey’s picture

Status: Needs review » Active

How about not ignoring the parents when hierarchical indexing is disabled for a field?

No, we shouldn't check for concrete processors/alterations unless really necessary. A field might still not be indexed as a hierarchy, or it might be hierarchical even if the alteration is not present.

As said in #1460092-22: When clicking a hierarchy facet, I do want to include the parent AND the child even if the facet is configured as an OR, I can't really think of use cases for this, and would like to know if anyone else wants/needs this behavior?
Otherwise, I fear you'll have to implement it in custom code for your site.

twod’s picture

I thought the use case would be very common in that you can tag entities with either a parent or child term depending on how specific you want to be.

Ex: Terms are categories of stuff nodes (say companies) do:

A, a broad category of stuff
 -A2, subset of A
 -A3, other subset of A
B, completely different category where subsets aren't needed.
C, another broad category
 -C2, a subset of C
 -C3, other subset of C 
 --C4, a very specific subset of C3

Company 1 is tagged with both A and A3 because they do A, but focus mostly on A3.
Company 2 is tagged with C and C3 because they don't do any of the stuff in C2, can also do some of the stuff in C, but they're really specialized in working with C3.
Company 3 is tagged B and C2 because they do something completely unrelated to the other guys, they also tinker a bit with stuff in C2 but not enough to have a good grasp of everything under C.
Company 4 is tagged with C and A because they do a bit of everything in those categories, but aren't really specialized.

This way I can filter using varying degrees of specificity. If I want everyone really good at C3 (2), I can get that, but I could also include anyone who indicates they can work with all of C (4) in case the first company is busy.

There's no problem indexing terms like this non-hierarchically (currently using Sorl) and make those queries manually, but SearchAPI can't do it because C gets removed as soon as I have selected C3. Node 4 will rarely show up unless people only do really broad filtering.
I naturally want to show my category filter as a tree, but have a flat term index to avoid the specialists being tagged as doing more general work.

Company 3 can give you new tires (c3), but they have no idea how to replace your engine (c2). Company 2 may be able to help you with your engine, but they usually just do body work and only take other stuff if they have time and have no equipment to switch tires.

I agree that checking for the alteration is a bit weird since we can't guarantee that it's actually indexed hierarchically just because the data is there (would be easier to guarantee if the settings could be extracted), but since it's part of SearchAPI and the behavior with the patch appears to be consistent, it was the best solution I found so far.

If there was some other way of manually indicating I don't want to remove the parents, I'd be happy with that.
FacetAPI adds a display setting to each facet which they call 'Treat parent items as individual facet items', which I thought would help. (Indeed, my first iteration of this patch simply looked to see if $settings['individual_parent'] was set) But then I realized this wasn't part of SearchAPI and couldn't be relied upon to always mean the same thing.

drunken monkey’s picture

Status: Active » Fixed

OK, that does make sense. Still not sure how common it is, though. I'd think displaying C*, too, when filtering on C is what most sites want to do, and what most site visitors would expect. (Are you sure that on your site visitors understand the UI correctly?)

But I guess we can support this pretty easily by just adding a new setting to our own term plugin's config form that lets you decide this for yourself. It should default to the behavior originally implemented in this issue, though.
Also, I think we should create a new issue for it. But if you could provide a patch implementing that, I'd be happy to review and commit it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.