[MinimumPrice] Handle minimum price cart #13291

SirDomin · 2021-11-12T00:18:00Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
License	MIT

Based on: #13266

src/Sylius/Component/Core/Promotion/Action/UnitDiscountPromotionActionCommand.php

AdamKasp · 2021-11-17T16:03:30Z

And probably you missed scenario with distributing discount on the product with minimum price and without it.

...s/promotion/applying_promotion_rules/receiving_discount_with_minimum_price_specified.feature

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

src/Sylius/Component/Core/Promotion/Action/UnitDiscountPromotionActionCommand.php

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

...res/promotion/applying_promotion_coupon/applying_promotion_coupon_with_minimum_price.feature

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

src/Sylius/Bundle/CoreBundle/Resources/config/services/promotion.xml

src/Sylius/Component/Core/Promotion/Action/UnitFixedDiscountPromotionActionCommand.php

src/Sylius/Component/Core/spec/Promotion/Applicator/UnitsPromotionAdjustmentsApplicatorSpec.php

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

src/Sylius/Component/Core/Promotion/Action/UnitDiscountPromotionActionCommand.php

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

src/Sylius/Behat/Context/Api/Shop/CartContext.php

src/Sylius/Behat/Context/Api/Shop/OrderContext.php

lchrusciel · 2021-11-24T19:59:02Z

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php

        }

        $splitPromotion = $this->proportionalDistributor->distribute($itemsTotals, $promotionAmount);
+        $this->distributeWithMinimumPrice($splitPromotion, $itemsTotals, $minimumPrices);


Why can't we put this logic inside of proportionalDistributor with an additional, third argument? Part of me also is thinking about VO slowly :P

i didnt really wanted to touch this place, this is just another implementation, just additional step which handles all minimum price tasks

The issue with this implementation is that it won't work out-of-the-box for any existent and new implementation if we won't remember about this method. Relaying on dev knowledge and memory is a route downhill. I would vote for adjusting the namespace or introducing a new one and implement here both of them with bc layer and deprecation of the old usage

lchrusciel · 2021-11-24T20:00:56Z

src/Sylius/Component/Core/Promotion/Action/UnitDiscountPromotionActionCommand.php

+
+        $minimumPrice = $variant->getChannelPricingForChannel($channel)->getMinimumPrice();
+
+        $adjustment->setAmount($this->calculate($unit->getTotal(), $minimumPrice, -$amount));


I'm wondering if this should not be extracted logic - assigning/calculating amount of x or variants minimal price

lchrusciel · 2021-11-24T20:01:25Z

src/Sylius/Component/Core/Promotion/Applicator/UnitsPromotionAdjustmentsApplicator.php

@@ -82,4 +90,13 @@ private function addAdjustment(PromotionInterface $promotion, OrderItemUnitInter

        $unit->addAdjustment($adjustment);
    }
+
+    private function calculate(int $itemTotal, int $minimumPrice, int $promotionAmount): int


And now, I'm 100% sure it should be separated service

i had different service to handle calculations, but @GSadee said its a BC break so i decided to leave it in private methods

Looking at your PR and #13325 it appears to me, that some extraction of this logic would be beneficial. Even in your PR it is present twice and has exactly same reason to exist and change

I agree that extraction would be recommended, but in what way to avoid breaking a BC promise?

...Sylius/Component/Core/spec/Promotion/Action/PercentageDiscountPromotionActionCommandSpec.php

src/Sylius/Component/Core/spec/Promotion/Action/UnitFixedDiscountPromotionActionCommandSpec.php

GSadee · 2021-11-30T14:07:18Z

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php

+            $splitPromotion = array_merge($distributionData['distributedPromotion'], $splitPromotion);
+        }
+
+


Suggested change

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php

src/Sylius/Component/Core/Promotion/Action/DiscountPromotionActionCommand.php

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php

src/Sylius/Component/Core/Distributor/MinimumPriceDistributor.php

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php

lchrusciel · 2021-12-07T12:34:00Z

src/Sylius/Component/Core/Distributor/MinimumPriceDistributor.php

+        }
+
+        return array_values(array_map(
+            function (array $processedOrderItem): int { return $processedOrderItem['promotion']; },


probably we could use arrow functions here and below

yeah, we could use it here but it could be done in other PR as part of refactor.

GSadee · 2021-12-08T13:31:28Z

UPGRADE-1.11.md

@@ -16,6 +16,11 @@ Please note that those priorities are being executed in ascending order. You can
 Be aware that if those priorities were customized, this would lead to problems. 
 You should check and adjust priorities on your application.

+### Minimum price & Promotions
+
+We added MinimumPrice to channelPricings entity, this price should be taken into account when customizing any promotions in Sylius.


Suggested change

We added MinimumPrice to channelPricings entity, this price should be taken into account when customizing any promotions in Sylius.

We added minimum price to the ChannelPricing entity, this price should be taken into account when customizing any promotions in Sylius.

GSadee · 2021-12-08T13:41:40Z

Thank you, @SirDomin! 🥇

SirDomin requested a review from a team as a code owner November 12, 2021 00:18

probot-autolabeler bot added Admin AdminBundle related issues and PRs. API APIs related issues and PRs. labels Nov 12, 2021

SirDomin force-pushed the handle-minimum-price-cart branch 8 times, most recently from f920173 to 1607091 Compare November 17, 2021 12:38

AdamKasp reviewed Nov 17, 2021

View reviewed changes

src/Sylius/Component/Core/Promotion/Action/UnitDiscountPromotionActionCommand.php Outdated Show resolved Hide resolved

GSadee requested changes Nov 18, 2021

View reviewed changes

GSadee reviewed Nov 19, 2021

View reviewed changes

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature Outdated Show resolved Hide resolved

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature Outdated Show resolved Hide resolved

CoderMaggie reviewed Nov 21, 2021

View reviewed changes

...res/promotion/applying_promotion_coupon/applying_promotion_coupon_with_minimum_price.feature Outdated Show resolved Hide resolved

AdamKasp approved these changes Nov 22, 2021

View reviewed changes

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature Show resolved Hide resolved

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature Outdated Show resolved Hide resolved

SirDomin force-pushed the handle-minimum-price-cart branch 4 times, most recently from 1dcd250 to 9696f75 Compare November 22, 2021 15:40

GSadee reviewed Nov 24, 2021

View reviewed changes

SirDomin force-pushed the handle-minimum-price-cart branch 3 times, most recently from 582b879 to 735aea2 Compare November 24, 2021 15:20

lchrusciel reviewed Nov 24, 2021

View reviewed changes

SirDomin force-pushed the handle-minimum-price-cart branch 4 times, most recently from aaaa365 to 0837e4d Compare November 24, 2021 23:02

SirDomin added 8 commits November 30, 2021 11:29

apply cart promotions based on minimum price

5e2014b

[Behat] add more specific behats

8e6afc0

split promotion amounts

8552151

[Behat] merge scenarios

b50ec02

[Behat] add more specific behats with distribution

44baa05

[Spec] fix spec, add 0 default value in migration

9d25e52

add default option to doctrine

c594ae3

remove reference from distributeWithMinimumPrice

1dae4dd

SirDomin force-pushed the handle-minimum-price-cart branch 2 times, most recently from a5f3dac to 0b4ba14 Compare November 30, 2021 11:13

[MinimumPrice] redistribute evenly after handling minimum price

4cf2f27

SirDomin force-pushed the handle-minimum-price-cart branch from 0b4ba14 to 4cf2f27 Compare November 30, 2021 12:19

GSadee reviewed Nov 30, 2021

View reviewed changes

lchrusciel requested changes Nov 30, 2021

View reviewed changes

src/Sylius/Component/Core/Promotion/Action/DiscountPromotionActionCommand.php Outdated Show resolved Hide resolved

lchrusciel reviewed Dec 1, 2021

View reviewed changes

src/Sylius/Component/Core/Promotion/Action/DiscountPromotionActionCommand.php Outdated Show resolved Hide resolved

src/Sylius/Component/Core/Promotion/Action/FixedDiscountPromotionActionCommand.php Outdated Show resolved Hide resolved

SirDomin force-pushed the handle-minimum-price-cart branch from 729d98b to cf9c86c Compare December 6, 2021 13:28

[MinimumPrice] Distribute recursively after applying minimum price

d064dd0

SirDomin force-pushed the handle-minimum-price-cart branch from cf9c86c to d064dd0 Compare December 6, 2021 14:23

lchrusciel and others added 4 commits December 7, 2021 10:40

[Promotion] Minimum price distributor initial implementation

2fb1f89

[MinimumPrice] clean existing classess, add percentage support

0ee76a5

[Behat] move reduntant steps to private methods

adaa2fe

[UPGRADE] add info to upgrade file

5a5021f

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Dec 7, 2021

[Spec] fix steps for latest changes

05916ee

lchrusciel reviewed Dec 7, 2021

View reviewed changes

[BC] keep previous services implementation in Commands

5a6c9d0

SirDomin force-pushed the handle-minimum-price-cart branch from db86ccc to 5a6c9d0 Compare December 8, 2021 01:51

GSadee approved these changes Dec 8, 2021

View reviewed changes

GSadee merged commit 57f9ac0 into Sylius:master Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MinimumPrice] Handle minimum price cart #13291

[MinimumPrice] Handle minimum price cart #13291

SirDomin commented Nov 12, 2021

AdamKasp commented Nov 17, 2021

lchrusciel Nov 24, 2021

SirDomin Nov 24, 2021

lchrusciel Nov 25, 2021

lchrusciel Nov 24, 2021

lchrusciel Nov 24, 2021

SirDomin Nov 24, 2021

lchrusciel Nov 25, 2021

GSadee Nov 25, 2021

GSadee Nov 30, 2021

lchrusciel Dec 7, 2021

SirDomin Dec 8, 2021

GSadee Dec 8, 2021

GSadee commented Dec 8, 2021


		$minimumPrice = $variant->getChannelPricingForChannel($channel)->getMinimumPrice();

		$adjustment->setAmount($this->calculate($unit->getTotal(), $minimumPrice, -$amount));

		$splitPromotion = array_merge($distributionData['distributedPromotion'], $splitPromotion);
		}

	We added MinimumPrice to channelPricings entity, this price should be taken into account when customizing any promotions in Sylius.
	We added minimum price to the ChannelPricing entity, this price should be taken into account when customizing any promotions in Sylius.

[MinimumPrice] Handle minimum price cart #13291

[MinimumPrice] Handle minimum price cart #13291

Conversation

SirDomin commented Nov 12, 2021

AdamKasp commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee commented Dec 8, 2021