TODO: Allow selection of AND/OR modifier for facets

Still needed for current patch:

  • fix the count for inactive items (which shows 0 at the moment).
  • Add testHideEmptyCount in an integration test.
Files: 

Comments

StryKaizer created an issue. See original summary.

michiellucas’s picture

For a project we are using your dev version of facets for a search page, we would need the facets to act as OR filters.

Can you point us to the right direction to create this feature for facets?

Thanks

borisson_’s picture

Hi,

First of all, this would need an extra setting in the facet entity / schema. I'd suggest something like: comparison_operator that defaults to "AND". Not sure if that's a good name. (naming things, it's hard.)

- src/FacetInterface.php -> extra getter and setter.
- src/Entity/Facet -> implementation of the new getter & setter. + addition to the config_export in the annotation.
- config/schema/facets.facet.schema.yml

Of course this will need to be set in the Form as well and saved on the entity.

You can have a look at the patch in #2625188: Allow limiting to one active item on how to do that, that also adds a new setting for the facet entity. (everything in patch; except for the part in the widget is quite similar to what you would need to do.)

The rest of the patch will probably have to go into the DefaultFacetManager. Not sure how this should be solved though, I'd suggest you have a look at how facet_api in d7 solved this problem.

If you need more anything else, you can ask here; or find us on irc in #drupal-search-api.

Anyway, thank you for taking a look at this!

michiellucas’s picture

Hi,

i added the query_operator variable to the options array, is that ok ?

I changed also the query and now have a working version of the OR implementation. I added a patch, can you please have a look and give me some feedback?

Next thing on my list is create checkboxes.

Thanks

borisson_’s picture

We have a different issue for the checkboxes, but it can surely be added trough this issue as well, we can close #2596337: Widget: CheckboxWidget.

I found a few very small issues with the patch:

  1. +++ b/src/Entity/Facet.php
    @@ -252,6 +252,14 @@ class Facet extends ConfigEntityBase implements FacetInterface {
    +
    

    There's one newline too much here.

  2. +++ b/src/Form/FacetDisplayForm.php
    @@ -529,6 +538,9 @@ class FacetDisplayForm extends EntityForm {
    +    //Query operator
    

    Doesn't need the extra comment here.

Otherwise this looks great, we haven't enabled tests yet on the issue queue; so I hope that all tests still pass.

michiellucas’s picture

It's ok the new ticket, i will work on it there https://www.drupal.org/node/2596337

I fixed your issues. Thanks for the feedback!

michiellucas’s picture

Problem that i now have is that i also need to show the facet items that are not active. Example

Dogs(12)
Cats(4) -> if cats is active, dogs disappears but that needs to change. When operator OR is chosen the facet needs to render al available filters.

I am looking into public function fillFacetsWithResults($facets) { where i can add the "not-active" facets.

If anyone has a better idea, shoot.

Thanks

michiellucas’s picture

Removed the checkbox plugin from patch, was a mistake

borisson_’s picture

Status: Active » Needs review
FileSize
643 bytes
8.22 KB

I had a really small code style fix; otherwise this is already looking great.
It looks like tests pass but they needed a fix for something else, just committed that.

I'll write a specific test for this now.

ricovandevin’s picture

For OR we also need an option to display the non-active options within a facet after one option is made active as michiellucas already stated. Do we tackle this in this issue too or do we create a separate issue for that?

Dogs(12)
Cats(4) -> if cats is active, dogs disappears but that needs to change. When operator OR is chosen the facet needs to render al available filters.

borisson_’s picture

Status: Needs review » Needs work
FileSize
9.74 KB
1.52 KB

I added a test, that currently fails. When that test passes, this functionality is ok.

@michiellucas, if you have time to work on this, that would of course be greatly appreciated, I don't think I'll be able to do a lot this week.

borisson_’s picture

@ricovandevin: Sorry for the late response, I think we should also solve that in this issue, yes.

StryKaizer’s picture

Version: » 8.x-1.x-dev
FileSize
3.07 KB
12.57 KB

#10 showing inactive items is addressed in the attached patch.
We still need to fix the count for inactive items (which shows 0 at the moment).

I also removed the count-check on a widget-level, as the widget should not care about this (It should be managed on another level).

borisson_’s picture

So where should the count-check go to?

StryKaizer’s picture

#14, its already in the QueryTypeString plugin if I'm not mistaken.

borisson_’s picture

Oh, it is, awesome. We still have to fix the tests, before we can get this in.

StryKaizer’s picture

Issue summary: View changes

Todo for this issue:
- tests
- fix the count for inactive items (which shows 0 at the moment).

borisson_’s picture

Status: Needs work » Needs review

Setting to NR for the bot.

Status: Needs review » Needs work

The last submitted patch, 13: support_and_or_for_facets-2625186-13.patch, failed testing.

borisson_’s picture

This patch didn't apply anymore, it does now. I also removed the changes made in the checkbox widget. That widget is being reworked in #2596337: Widget: CheckboxWidget.

borisson_’s picture

Status: Needs work » Needs review

Go testbot, go!

Status: Needs review » Needs work

The last submitted patch, 20: support_and_or_for_facets-2625186-20.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 23: support_and_or_for_facets-2625186-23.patch, failed testing.

borisson_’s picture

One of the tests that currently fails because of this is the LinksWidgetTest::testHideEmptyCount test. Because we removed that functionality out of the widget but left it in the test.

The functionality still works but we don't have a test to prove it, if we get in #2596337: Widget: CheckboxWidget, that adds a WidgetIntegrationTest - that would be great spot to add this test in. This should take us back to 1 fail.

Status: Needs review » Needs work

The last submitted patch, 25: support_and_or_for_facets-2625186-24.patch, failed testing.

borisson_’s picture

Because we're using 'OR' by default, this assertion had to be updated. Tests are green.

Next up is getting the count right again.

borisson_’s picture

This works locally, needs more tests and a big cleanup to make sure that everything works as possible + needs update to interface but it looks like it works. Woo!

Status: Needs review » Needs work

The last submitted patch, 28: support_and_or_for_facets-2625186-28.patch, failed testing.

borisson_’s picture

Attached patch should fix the broken tests.

borisson_’s picture

borisson_’s picture

This needed a reroll because of the other recent commits. Writing a hide empty count integration test now - we removed that unit test in #25.

borisson_’s picture

@StryKaizer mentioned in irc that the that this could benifit from extra comments, so I added some and renamed $query2 to $unfiltered_query.

borisson_’s picture

The discussion in irc mentioned some things about "supportsFeature", this adds that to the interface + the search api base-plugin. Not sure how to continue here.

Status: Needs review » Needs work

The last submitted patch, 35: support_and_or_for_facets-2625186-35.patch, failed testing.

borisson_’s picture

The last submitted patch, 20: support_and_or_for_facets-2625186-20.patch, failed testing.

The last submitted patch, 23: support_and_or_for_facets-2625186-23.patch, failed testing.

The last submitted patch, 25: support_and_or_for_facets-2625186-24.patch, failed testing.

The last submitted patch, 27: support_and_or_for_facets-2625186-27.patch, failed testing.

The last submitted patch, 28: support_and_or_for_facets-2625186-28.patch, failed testing.

The last submitted patch, 30: support_and_or_for_facets-2625186-30.patch, failed testing.

The last submitted patch, 31: support_and_or_for_facets-2625186-31.patch, failed testing.

The last submitted patch, 32: support_and_or_for_facets-2625186-32.patch, failed testing.

The last submitted patch, 33: support_and_or_for_facets-2625186-33.patch, failed testing.

The last submitted patch, 34: support_and_or_for_facets-2625186-34.patch, failed testing.

The last submitted patch, 35: support_and_or_for_facets-2625186-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: support_and_or_for_facets-2625186-37.patch, failed testing.

borisson_’s picture

Issue tags: +Needs reroll

So this is one that we actually need to reroll.

borisson_’s picture

borisson_’s picture

Can @Nick_vh or @StryKaizer help me explain where we have to go with the supportsFeature code?

I was happy with #34 and I'm not sure I know how to continue now.

borisson_’s picture

Let's reroll and commit #34 we can do the other part in a followup later.

borisson_’s picture

Attached a reroll of #34, if the tests pass I'll commit this.

  • borisson_ committed 3a44b3d on 8.x-1.x
    Issue #2625186 by borisson_, michiellucas, StryKaizer: Support and/or...
borisson_’s picture

Status: Needs review » Fixed

Committed, thanks a bunch for your contributions @StryKaizer & @michiellucas.

Status: Fixed » Closed (fixed)

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