Directly use the the MultiDynamicAggregate filter type for options

[MartijnHarte]

Fixes #82.

[JanTvrdik]

It's not that simple, OptionMultiDynamicAggregate handles some stuff that the generic MultiDynamicAggregate does not take care of. Specifically it filters out variants that are not in stock.

[JanTvrdik]

@MartijnHarte You need sth similar to (this is a simplified, untested version of what we use in production)

<?php declare(strict_types = 1);

namespace MangoSylius\ElasticsearchBundle\Filter\Widget;

use ONGR\ElasticsearchDSL\Aggregation\Bucketing\FilterAggregation;
use ONGR\ElasticsearchDSL\Query\Compound\BoolQuery;
use ONGR\ElasticsearchDSL\Query\Joining\NestedQuery;
use ONGR\ElasticsearchDSL\Query\TermLevel\RangeQuery;
use ONGR\ElasticsearchDSL\Query\TermLevel\TermQuery;
use Sylius\ElasticSearchPlugin\Filter\Widget\MultiDynamicAggregateOverride;


class OptionsMultiDynamicAggregateX extends MultiDynamicAggregateOverride
{
	protected function addSubFilterAggregation(
		$filterAggregation,
		&$deepLevelAggregation,
		$terms,
		$aggName
	) {
		$filterQuery = $this->getFilterQuery($terms);
		$innerFilterAggregation = new FilterAggregation(
			$aggName,
			$filterQuery
		);
		$innerFilterAggregation->addAggregation($deepLevelAggregation);
		$filterAggregation->addAggregation($innerFilterAggregation);
	}


	protected function getFilterQuery($terms)
	{
		[$path, $field] = explode('>', $this->getDocumentField());
		$boolQuery = new BoolQuery();
		$boolQuery->add(new RangeQuery('variants.stock', ['gte' => 1]));

		foreach ($terms as $groupName => $values) {
			$innerBoolQuery = new BoolQuery();

			foreach ($values as $value) {
				$nestedBoolQuery = new BoolQuery();
				$nestedBoolQuery->add(new TermQuery($this->getNameField(), $groupName));
				$nestedBoolQuery->add(new TermQuery($field, $value));

				$innerBoolQuery->add(
					new NestedQuery(
						$path,
						$nestedBoolQuery
					),
					BoolQuery::SHOULD
				);
			}

			$boolQuery->add($innerBoolQuery);
		}

		return new NestedQuery('variants', $boolQuery, ['inner_hits' => ['_source' => false, 'size' => 100]]);
	}
}

[MartijnHarte]

@JanTvrdik I see.

Shouldn't the InStock filter be responsible for filtering out variants that are not in stock? It seems to me now that some implementation choices were made for options, which perhaps are not compatible with all, or most use cases. Am I missing someting?

Besides, the current configuration + option filter type doesn't work out of the box, a change seems to be necessary?

[JanTvrdik]

See #60 (comment)

[MartijnHarte]

Thanks for linking to the explanation!

[MartijnHarte]

@JanTvrdik I just took your code example and experimented with it. I just don't see how stock is related to the issue..

My situation:

• I have created two options: Size and Color.
• I have created a product with a variant "L - Red" with a stock of 0, not tracked.
• The same product has a variant "M - Blue" with a stock of 10, tracked.

This results in the initially available filters:
Size: L, M
Color: Blue, Red

This seems weird to me, since only "M - Blue" is in stock. What's happening here looks like that if one variant of a product is available, all variants appear as options.

Next when enabling the filter "Size: M", both colors Blue and Red remain available. When enabling filter "size: L" instead of M, both colors disappear.
The same goes for filtering on colors. When filtering on color Red both sizes disappear, while filtering on color Blue instead of Red results in both sizes M and L.

When changing your code from:

$boolQuery->add(new RangeQuery('variants.stock', ['gte' => 1]));

to:

$boolQuery->add(new RangeQuery('variants.stock', ['gte' => 1]), BoolQuery::SHOULD);
$boolQuery->add(new TermQuery('variants.is_tracked', false), BoolQuery::SHOULD);

all options always remain available, no matter what filter is enabled.

Since I don't have products which have no stock, but are tracked, this solution behaves exactly the same as my PR. At least, I don't see any difference.

While stock might be an issue, I don't think it's related to this particular issue. Besides, I agree to what you said in #60 (comment), in some cases it may make sense to allow searching in products that are no longer available for purchase.

The actual issue regarding this PR would be that the SyliusElasticsearchPlugin does not work out of the box with options. In #60 (comment) I read that elastic search should be the one to blame, now I am no expert on elastic search, but that seems a bit shortsighted..
A part of this plugin is the structure of the index. Perhaps we should review the way products are indexed?

Last but not least, I wrote this very basic example request for elastic search, where I filter on the color red. The response supplies me with the correct remaining size, L.

Request:

{
	"size": 0,
	"aggregations": {
		"NESTED_OPTIONS": {
			"nested": {
				"path": "variants.options"
			},
			"aggregations": {
				"FILTER_ON_COLOR": {
					"filter": {
						"bool": {
							"must": [
								{
									"term": {
										"variants.options.name.raw": "Color"
									}
								},
								{
									"term": {
										"variants.options.value.raw": "Red"
									}
								}
							]
						}
					},
					"aggregations": {
						"REVERSE_TO_VARIANTS": {
							"reverse_nested": {
                    			"path": "variants"
                			},
                			"aggregations": {
                				"NESTED_OPTIONS": {
                					"nested": {
                						"path": "variants.options"
                					},
                					"aggregations": {
                						"FILTER_ON_SIZE_KEY_WITHIN_COLOR_CONTEXT": {
                							"filter": {
                								"bool": {
                									"must": [
                										{
                											"term": {
                												"variants.options.name.raw": "Size"
                											}
                										}
                									]
                								}
                							},
                							"aggregations": {
                								"REMAINING_SIZE_VALUES": {
                									"terms": {
														"field": "variants.options.value.raw"
	                								}	
                								}
                							}
                						}
                					}
                				}
                			}
						}
					}
				}
			}
		}
	}
}

Response:

{
    "took": 1,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "failed": 0
    },
    "hits": {
        "total": 9,
        "max_score": 0,
        "hits": []
    },
    "aggregations": {
        "NESTED_OPTIONS": {
            "doc_count": 102,
            "FILTER_ON_COLOR": {
                "doc_count": 1,
                "REVERSE_TO_VARIANTS": {
                    "doc_count": 1,
                    "NESTED_OPTIONS": {
                        "doc_count": 2,
                        "FILTER_ON_SIZE_KEY_WITHIN_COLOR_CONTEXT": {
                            "doc_count": 1,
                            "REMAINING_SIZE_VALUES": {
                                "doc_count_error_upper_bound": 0,
                                "sum_other_doc_count": 0,
                                "buckets": [
                                    {
                                        "key": "L",
                                        "doc_count": 1
                                    }
                                ]
                            }
                        }
                    }
                }
            }
        }
    }
}

Given this request + response, it doesn't seem to be a problem of elastic search, but the way we make use of it?

[JanTvrdik]

stock of 0, not tracked

not tracked = infinite supply (intended for non-physical stuff, such as song downloads)

I did not read the following text. Could you try it again with all items tracked?

[MartijnHarte]

Please read all, it's about how stock does not seem to be relevant at all.

[JanTvrdik]

This results in the initially available filters:

Well, you need to enable the stock filter as well.

[JanTvrdik]

Also make sure that showZeroOption is false or even better, print the count of matched variants.

[11mb]

Hi @JanTvrdik and @MartijnHarte, I think this discussion about stock being relevant or not is a little bit of a sidetrack in this PR. In my opinion the possibility to filter out options that are out of stock should be configurable from the yml, and not hardcoded. @JanTvrdik If that is tackled is this PR mergeable or should it be further improved?

[psihius]

Well, you have a bigger issue here - you can't use stock ONGR filter because it does not support more than 2 level's, and since product options are a 3rd level - it will not work at all.

Also, the implementation has an "OR" query for filtering - either products are in stock OR they are not tracked. So if you have an infinite product - you just set it as not tracked. If it's tracked - you need it to have a proper amount (otherwise it will not allow you to check out the product).

Now, on the fact that you might wana show products with 0 stock that are tracked - well, just add an option that you can pass to the filter with a default value and add an if that modifies the query. But outright just deleting an important class like this is not a good idea, nor should this PR be accepted as it's right now.