added 2 new facets plugins for processor and query_type. Together the… #235
Conversation
…y provide a new pre_query processor on facets to AND facet groups together while still allowing OR within a facet group. This is based on initial code provided by Andy Broomfield
|
Thanks @snpower ! @andybroomfield and @ekes said they'd look and @ekes mentioned we probably want some tests. |
|
Looks good from testing so far, thanks @snpower. Before, the Just right listing should not show.
With the And filter, it is now filtered when over 18s is checked.
But returns as expected when under 18s checked.
|
|
@finnlewis I nabbed Stella's time over the weekend when she was offically not working! Any chance someone else from the LGD team would be able to put the necessary tests for this together; I have a feeling that might be a quicker way for us to get this complete and merged? |
|
@andybroomfield not totally clear by your screen shots - is the first shot showing that it does not work or is it all good? |
|
@Adnan-cds would you like to take a look at this too? Hoping to get it merged soon. |
|
@finnlewis that is the before screenshot. |
…etween-facet-groups-and-or-within-them
|
@andybroomfield 's looking at adding a Functional test. I've had a quick look at doing a Kernel one, but it's probably more 'prophesy' than is worth it. But while doing so I did come up with a couple of questions I'll pop in a review. |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function preQuery(FacetInterface $facet) { |
There was a problem hiding this comment.
sorry, is which part needed?
There was a problem hiding this comment.
The content of the preQuery just seems to getActiveItems from the $facet and then set the same value back?
Or does this cause the facet to do something, looking it the class it doesn't seem to.
| * {@inheritdoc} | ||
| */ | ||
| public function build() { | ||
| $query_operator = $this->facet->getQueryOperator(); |
There was a problem hiding this comment.
This seems like it's very similar to the Drupal\facets\Plugin\facets\query_type\SearchApiString without the missing facet error handling. Can it inherit that, rather than base, and just override the execute() that is different?
There was a problem hiding this comment.
yes, it probably could alright
There was a problem hiding this comment.
@andybroomfield Think we were agreed this should be enabled on the facet by default. So I've added it here.
|
@andybroomfield has not had a chance to write tests for this yet, so we're not merging just yet, sorry! |
This test creates 4 facets in two groups, and four directory entries in a test directory channel. It then tries to click on each facet, testing that the facets work (in the default configuration) as OR facets within the group, and AND facets accross the facet groups.
|
I might be missing the point, but I don't think this is working quite as I would expect. I have a few items.
When I check England, I see only the England result, which is expected, but I can no longer see the other facets to be able to select Scotland:
I think what I am trying to do is be able to select Enland and Scotland to be able see a list of items that are tagged Enland OR Scotland. Then be able to reduce that set by selecting an Organisation Type. Is there a way we can get the other facets with a group to be displayed? |
|
Possible related https://www.drupal.org/project/facets/issues/3276669 |
|
@markconroy could you run this by your team and explore the UX that I describe above? Is this intended? Can we find a way to work around this specific case? @3mkay ? |
|
The quick and dirty method is just to re-run the query without facets, since that is what is used to generate them (means all applied facets are visible). /**
* {@inheritdoc}
*/
public function build() {
$query_operator = $this->facet->getQueryOperator();
$temp_query = $this->query->getOriginalQuery();
// dpm(get_class_methods($temp_query));
$temp_query->preExecute();
// dpm(array_keys($temp_query->getOptions()));
// dpm($this->query->getConditionGroup()->getConditions());
$conditions = &$temp_query->getConditionGroup()->getConditions();
// dpm($conditions);
foreach ($conditions as $key => $condition) {
if ($condition instanceof \Drupal\search_api\Query\ConditionGroupInterface) {
$tags = $condition->getTags();
foreach($tags as $tag) {
if (strpos($tag, 'facet:localgov_directory_facets_filter.') === 0) {
unset($conditions[$key]);
}
}
}
}
$temp_query->execute();
$avalible_facets = $temp_query->getResults()->getExtraData('search_api_facets');
$results = $avalible_facets['localgov_directory_facets_filter'] ?? $this->results;
if (!empty($results)) {
$facet_results = [];
foreach ($results as $result) {
if ($result['count'] || $query_operator === 'or') {
$result_filter = $result['filter'] ?? '';
if ($result_filter[0] === '"') {
$result_filter = substr($result_filter, 1);
}
if ($result_filter[strlen($result_filter) - 1] === '"') {
$result_filter = substr($result_filter, 0, -1);
}
$count = $result['count'];
$result = new Result($this->facet, $result_filter, $result_filter, $count);
$facet_results[] = $result;
}
}
$this->facet->setResults($facet_results);
}
return $this->facet;
} |
|
From further discussion, it sounds like the behaviour I outlined above is expected. @ekes is happy for the motivation on this and we can either merge this in as is, or see if @andybroomfield's approach above will make it even better! @markconroy confirmed that there is no a huge rush for this, they already have it working. |
|
I've started a new branch with a refined version of this code. |
|
Discussing at Merge Monday. @andybroomfield is keen to know how this is playing out at Essex. @3mkay do you know who to talk to to find out how this is being used? Our motivation is to understand whether to go with how this pull request is working as is, or explore the branch that @andybroomfield created with the enhancements. |
|
Just to add, I would be happy with this functionality to go out sooner and we can then work a fix on my other branch. My hunch is that real world use the issue above is not something that is relevant as much as most directory entries have good cross catogrisation accross the filters. |
|
Here is an example of where it might be an issue, though it took a few ticks to get there. As a user, I would expect to now expand my options for areas covered, special needs and support type.
|
|
We're keen to push forward with this and bring it in if it is an improvement. @andybroomfield wants to have a look at it again. In practise we think this is good to go ahead with. @andybroomfield will test soon with some live BHCC directoires. |
|
Update from Merge Monday call, @andybroomfield is hoping to test with real live directories in the next week or two. Otherwise we're all happy to merge this. Watch this space! |
|
On @andybroomfield 's backlog for the forthcoming sprint, where they'll be looking at directoires. |
|
@andybroomfield and team in the process of testing this and working on some changes which will come back as a PR into this branch. |
This is to address the issue in this comment #235 (comment) Instead of just using the facets from the result set, re-run the query, each time eliminating all the facets except each group, and filter out its own so there is a list of each sibling facet that could be set with an OR that also mathces an AND in the other group. This then produces a filtered list of sibling facets that should be reachable within the AND accross the groups and OR within them.
|
@ekes is working on #269 with @andybroomfield Once that is merged into this branch we should be a good to go! :) |
|
@finnlewis @andybroomfield can I see this working somewhere? Many thanks! |
|
Interesting question @willguv, I wonder if we can spin up a Gitpod instance with this branch to demonstrate? |
|
@andybroomfield will get back to this at some point, but not top of the list. Mainly just needs to review comment from @ekes above. |
|
Just checking in on this at Merge Monday. @andybroomfield is hoping to be able to progress the other pull request #269 and then get back to this one. From discussion today, we think we should re-target this to the 3.x branch to support Drupal 10 initially, then back-port to the 2.x / Drupal 9 branch if necessary. @snpower @markconroy any issues with re-targeting this pull request to the 3.x branch by creating a new branch and new pull request? |
|
Fine with me. |
|
Would it b e ok if I rebased this to 3.x? |
|
I don't see any issue with that. |
|
I've converted this back to draft as it needs to be rebased on 3.x. |
|
Discussing this in Tech Drop-in, @NDITGC needs this! @andybroomfield and @ekes in helpting to test this and #235 to bring into the fold. Note: @NDITGC is still on Drupal 9, so it would be valuable to get this merged to the Drupal 9 branches and Drupal 10. |
This is to address the issue in this comment #235 (comment) Instead of just using the facets from the result set, re-run the query, each time eliminating all the facets except each group, and filter out its own so there is a list of each sibling facet that could be set with an OR that also mathces an AND in the other group. This then produces a filtered list of sibling facets that should be reachable within the AND accross the groups and OR within them.
|
@andybroomfield and @snpower is this one now redundant as we merged #325 ? |
|
Closing this - thanks for the patience! |
…y provide a new pre_query processor on facets to AND facet groups together while still allowing OR within a facet group. This is based on initial code provided by Andy Broomfield