This repository has been archived by the owner. It is now read-only.

Various improvements based on real-world usage #60

Merged
merged 46 commits into from Sep 29, 2017

Conversation

Projects
None yet
5 participants
@psihius
Collaborator

psihius commented Aug 30, 2017

Implemented InStock filter
Added 3 level deep nesting support for Option filtering
Added live elastic index updates via Doctrine listener
And other changes

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Aug 30, 2017

Member

Wow, thanks again, it's incredible! 🎉 Tests are failing though :(

Member

pamil commented Aug 30, 2017

Wow, thanks again, it's incredible! 🎉 Tests are failing though :(

@psihius

This comment has been minimized.

Show comment
Hide comment
@psihius

psihius Aug 30, 2017

Collaborator

Yeah, tests are failing, I had a glance, I think that's mostly a method signature fail. I'm gonna look at it tomorrow morning and try fix it.

Collaborator

psihius commented Aug 30, 2017

Yeah, tests are failing, I had a glance, I think that's mostly a method signature fail. I'm gonna look at it tomorrow morning and try fix it.

psihius added some commits Sep 14, 2017

Moved factories under their respective "Document" and "View" folders,…
… extracted factories to their own config file, adjusted tests to new document structure and much more
Merge branch 'origin-master'
# Conflicts:
#	composer.json
#	spec/Projection/ProductProjectorSpec.php
#	src/Factory/ProductDocumentFactory.php
#	src/Filter/Widget/Pager.php
#	src/Projection/ProductProjector.php
#	tests/DataFixtures/ORM/shop.yml
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_name_ascending.json
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_name_descending.json
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_price_ascending.json
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_price_descending.json
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_production_year_attribute_ascending.json
#	tests/Responses/Expected/WEB_GB/en_GB/product_list_page_sorted_by_production_year_attribute_descending.json
@psihius

This comment has been minimized.

Show comment
Hide comment
@psihius

psihius Sep 16, 2017

Collaborator

Okay, this is getting out of hand, I have fixed everything I could find, did some major work and this PR should be merged before any other work is done. @pamil can you please take care of this? And check that I have merged your changes correctly, there were some conflicts.

Collaborator

psihius commented Sep 16, 2017

Okay, this is getting out of hand, I have fixed everything I could find, did some major work and this PR should be merged before any other work is done. @pamil can you please take care of this? And check that I have merged your changes correctly, there were some conflicts.

Show outdated Hide outdated .gitignore
Show outdated Hide outdated src/Command/ResetProductIndexCommand.php
Show outdated Hide outdated src/Command/UpdateProductIndexCommand.php
Show outdated Hide outdated src/Command/UpdateProductIndexCommand.php
Show outdated Hide outdated src/Document/VariantDocument.php
Show outdated Hide outdated src/EventListener/ProductPublisher.php
Show outdated Hide outdated src/Factory/Document/PriceDocumentFactory.php
Show outdated Hide outdated tests/Controller/SearchControllerApiTest.php
Show outdated Hide outdated tests/Controller/SearchControllerApiTest.php

psihius added some commits Sep 26, 2017

Updated commands: moved lock release, changed product update to batch…
… mode, switched to hashmap lookup for already updated products
Merge branch 'origin-master'
# Conflicts:
#	composer.json
#	src/Factory/ProductDocumentFactory.php
Removed "setId" and "getId" type hints, as those can be strings or in…
…tegers and adjusted taxon factory to accept position as an optional param so productTaxons can have proper positions
$inStockBoolQuery = new BoolQuery();
$inStockBoolQuery->add(new RangeQuery('variants.stock', ['gte' => 1]), BoolQuery::SHOULD);

This comment has been minimized.

@JanTvrdik

JanTvrdik Sep 27, 2017

Shouldn't this be responsibility of InStock filter? Also in some cases it may make sense to allow searching in products that are no longer available for purchase.

@JanTvrdik

JanTvrdik Sep 27, 2017

Shouldn't this be responsibility of InStock filter? Also in some cases it may make sense to allow searching in products that are no longer available for purchase.

This comment has been minimized.

@psihius

psihius Sep 28, 2017

Collaborator

Blame how Elastic works - because those are nested documents, the contexts are different and it will return the document if it finds one with the defined option code regardless of it having stock or not.
Think each nested document filtering as it's own subquery knowing nothing about the main query, and main query only gets true or false to include a specific document or not in the result. That's why the nested stock filter.
If you can find a better way - I'm all ears.

@psihius

psihius Sep 28, 2017

Collaborator

Blame how Elastic works - because those are nested documents, the contexts are different and it will return the document if it finds one with the defined option code regardless of it having stock or not.
Think each nested document filtering as it's own subquery knowing nothing about the main query, and main query only gets true or false to include a specific document or not in the result. That's why the nested stock filter.
If you can find a better way - I'm all ears.

This comment has been minimized.

@JanTvrdik

JanTvrdik Sep 28, 2017

Oh, I see. Thanks for explaining the problem. The issue is when a product has e.g. red color variant in stock and blue color variant not in stock AND user in filter selects option[color] = blue. The InStock filter will return the product because the red variant is in stock and is unaware of the option filter requesting only blue variant.

The priceRange filter would have the same issue if variants.price (which may differ for each variant) would be used instead of price (currently computed as minimum of variants.price).

@JanTvrdik

JanTvrdik Sep 28, 2017

Oh, I see. Thanks for explaining the problem. The issue is when a product has e.g. red color variant in stock and blue color variant not in stock AND user in filter selects option[color] = blue. The InStock filter will return the product because the red variant is in stock and is unaware of the option filter requesting only blue variant.

The priceRange filter would have the same issue if variants.price (which may differ for each variant) would be used instead of price (currently computed as minimum of variants.price).

This comment has been minimized.

@psihius

psihius Sep 28, 2017

Collaborator

Yep, exactly.

With price it's not the same because when we put the product into elastic, we take the minimum price from all the variants and put that as products price (price field on the ProductDocument) and that one is used for filtering. And because live index update is implemented, if the variant is sold out, the index is immediatly updated and so is the min price on the product, as is the stock. So it does not have that problem as options do.

@psihius

psihius Sep 28, 2017

Collaborator

Yep, exactly.

With price it's not the same because when we put the product into elastic, we take the minimum price from all the variants and put that as products price (price field on the ProductDocument) and that one is used for filtering. And because live index update is implemented, if the variant is sold out, the index is immediatly updated and so is the min price on the product, as is the stock. So it does not have that problem as options do.

This comment has been minimized.

@psihius

psihius Sep 28, 2017

Collaborator

I was at this for a long few months, I have gone throught a lot of edge cases :)

@psihius

psihius Sep 28, 2017

Collaborator

I was at this for a long few months, I have gone throught a lot of edge cases :)

psihius added some commits Sep 28, 2017

Changed taxonSlug and taxonCode filters to a custom filter because th…
…ose are nested documents and filtering and sorting has to work accordingly.
@psihius

This comment has been minimized.

Show comment
Hide comment
@psihius

psihius Sep 29, 2017

Collaborator

@pamil @pjedrzejewski @lchrusciel Please review, done with all requested changes and a few adjustments/fixes

Travis build is failing due to some weird error, I don't have this one on my side and it's during installation phase.

Collaborator

psihius commented Sep 29, 2017

@pamil @pjedrzejewski @lchrusciel Please review, done with all requested changes and a few adjustments/fixes

Travis build is failing due to some weird error, I don't have this one on my side and it's during installation phase.

@psihius

This comment has been minimized.

Show comment
Hide comment
@psihius

psihius Sep 29, 2017

Collaborator

Fixed travis failing due to stability settings of composer.json :)

Collaborator

psihius commented Sep 29, 2017

Fixed travis failing due to stability settings of composer.json :)

@pamil

pamil approved these changes Sep 29, 2017

/**
* @param int $originalAmount
*/
public function setOriginalAmount(int $originalAmount = 0): void

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

$priceDocument->setOriginalAmount(); should not be a valid call

@pamil

pamil Sep 29, 2017

Member

$priceDocument->setOriginalAmount(); should not be a valid call

@@ -109,6 +139,22 @@ public function setCode(string $code): void
}
/**
* @return string
*/
public function getName(): string

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

?string

@pamil

pamil Sep 29, 2017

Member

?string

@@ -20,7 +32,19 @@
/**
* @var ProductInterface[]
*/
private $scheduledProducts = [];
private $scheduledInserts = [];

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

Inserts -> Insertions

@pamil

pamil Sep 29, 2017

Member

Inserts -> Insertions

} elseif ($entity instanceof ProductInterface) {
return $entity;
} else {
return null;

This comment has been minimized.

@pamil

pamil Sep 29, 2017

Member

This one will require some refactoring tho.

@pamil

pamil Sep 29, 2017

Member

This one will require some refactoring tho.

@pamil pamil merged commit 9360bb4 into Sylius:master Sep 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 29, 2017

Member

Thanks @psihius, that's a really enormous amount of work! 🥇

Member

pamil commented Sep 29, 2017

Thanks @psihius, that's a really enormous amount of work! 🥇

@psihius

This comment has been minimized.

Show comment
Hide comment
@psihius

psihius Sep 29, 2017

Collaborator

Y'r welcome :)

Collaborator

psihius commented Sep 29, 2017

Y'r welcome :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.