-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor catalog promotions - process variants instead promotion #13536
Conversation
src/Sylius/Bundle/CoreBundle/CommandHandler/VariantsUpdateHandler.php
Outdated
Show resolved
Hide resolved
57c3b91
to
f0df531
Compare
89ae455
to
39beb76
Compare
src/Sylius/Bundle/CoreBundle/Announcer/BatchedVariantsUpdateAnnouncer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Announcer/BatchedVariantsUpdateAnnouncer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/VariantsUpdateHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Announcer/BatchedVariantsUpdateAnnouncer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Announcer/BatchedVariantsUpdateAnnouncer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/VariantsUpdateHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/VariantsUpdateHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/VariantsUpdateHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Announcer/BatchedVariantsUpdateAnnouncerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Announcer/BatchedVariantsUpdateAnnouncerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Processor/AllCatalogPromotionsProcessorSpec.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General problem, that exist with this implementation is the usage of CatalogPromotionApplicatorInterface
- this service is checking only for exclusiveness or minimum price, both on channel pricing, but (if I see correctly) we are never checking scopes of promotions if they should be applied. At least, I don't see VarinatProvider usage here nor reimplementation of it. This suggest, that we have weak test coverage if we didn't detect it earlier
526a744
to
9d2b663
Compare
src/Sylius/Bundle/CoreBundle/Commander/UpdateVariantsCommander.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Processor/AllCatalogPromotionsProcessor.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/UpdateVariantsHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Processor/ProductCatalogPromotionsProcessor.php
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Applicator/CatalogPromotionApplicatorSpec.php
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/spec/Commander/UpdateVariantsCommanderSpec.php
Outdated
Show resolved
Hide resolved
6a89290
to
ebb2131
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should probably slowly remove variants provider.
src/Sylius/Bundle/CoreBundle/Checker/CatalogPromotionApplicableOnVariantChecker.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Checker/CatalogPromotionApplicableOnVariantChecker.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Checker/InForTaxonsScopeVariantChecker.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Checker/InForVariantsScopeVariantChecker.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/Checker/InForProductScopeVariantChecker.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/CoreBundle/CommandHandler/UpdateVariantsHandler.php
Outdated
Show resolved
Hide resolved
495c41c
to
b9a9609
Compare
79930d4
to
6ade27f
Compare
dee4a36
to
b28a47e
Compare
# @api @ui @todo | ||
# Scenario: Enabling catalog promotion during its operating time | ||
# Given the catalog promotion "Christmas sale" operates between "yesterday" and "tomorrow" | ||
# And this catalog promotion is disabled | ||
# When I enable "Christmas sale" catalog promotion | ||
# Then this catalog promotion should be active | ||
# And "PHP T-Shirt" variant should be discounted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`todo tag should be enough. Please revert. If the tests will be still executed, we need to improve GH Action step
interface ForVariantInCatalogPromotionScopeCheckerProviderInterface | ||
{ | ||
public function provide(CatalogPromotionScopeInterface $scope): VariantInScopeCheckerInterface; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Sylius\Bundle\PromotionBundle\Provider\EligibleCatalogPromotionsProviderInterface; | ||
use Sylius\Component\Core\Model\CatalogPromotionInterface; | ||
use Sylius\Bundle\CoreBundle\CommandDispatcher\ApplyCatalogPromotionsOnVariantsCommandDispatcherInterface; | ||
use Sylius\Component\Core\Repository\ProductVariantRepositoryInterface; | ||
|
||
final class AllCatalogPromotionsProcessor implements AllCatalogPromotionsProcessorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final class AllCatalogPromotionsProcessor implements AllCatalogPromotionsProcessorInterface | |
final class AllProductVariantsCatalogPromotionsProcessor implements AllProductVariantCatalogPromotionsProcessorInterface |
Thank you, Adam! 🎉 |
to make it possible to optimise Catalog Promotions processing, we want to process catalog promotion per variants in batches. the current solution prevents parallelization of the work.
it is initial PR.