[CartPromotion][CatalogPromotion] Receiving discount only on non discounted products #13349

GSadee · 2021-12-01T14:27:51Z

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

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

…discount only on non discounted products

…iving discount only on non discounted products

…ging the name of field

…ounted products for unit cart promotions

… discounted products for fixed order cart promotions

…nt and minimum prices

AdamKasp · 2021-12-09T17:39:05Z

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

@@ -27,7 +27,7 @@ public function __construct(ProportionalIntegerDistributorInterface $proportiona
        $this->proportionalIntegerDistributor = $proportionalIntegerDistributor;
    }

-    public function distribute(array $orderItems, int $amount, ChannelInterface $channel): array
+    public function distribute(array $orderItems, int $amount, ChannelInterface $channel, bool $appliesOnDiscounted): array


is it BC break? we shouldn't add an argument without default value to a public function.

That would be a BC Break, but this code is only on master branch which means it is not used by anyone 💃 We're safe 🚀

AdamKasp · 2021-12-09T17:44:42Z

src/Sylius/Component/Core/Model/Order.php

@@ -460,4 +460,18 @@ public function setCustomerIp(?string $customerIp): void
    {
        $this->customerIp = $customerIp;
    }
+
+    public function getNonDiscountedItemsTotal(): int
+    {


I am not sure but can we use here array_reduce instead of foreach?

Zales0123 · 2021-12-09T18:12:10Z

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

+    /**
+     * @Then /^the product "([^"]+)" should have total price ("[^"]+") in the cart$/
+     */
+    public function theProductShouldHaveTotalPriceInTheCart(string $productName, int $totalPrice): void


If we use a transformer, we could just call $product->getName() in line 426 and remove exception from the end of the function

Zales0123 · 2021-12-09T18:20:01Z

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

@@ -27,7 +27,7 @@ public function __construct(ProportionalIntegerDistributorInterface $proportiona
        $this->proportionalIntegerDistributor = $proportionalIntegerDistributor;
    }

-    public function distribute(array $orderItems, int $amount, ChannelInterface $channel): array
+    public function distribute(array $orderItems, int $amount, ChannelInterface $channel, bool $appliesOnDiscounted): array


That would be a BC Break, but this code is only on master branch which means it is not used by anyone 💃 We're safe 🚀

Zales0123 · 2021-12-09T18:22:20Z

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

+    private function getTotalPrice(OrderItemInterface $orderItem, bool $appliesOnDiscounted, ChannelInterface $channel): int
+    {
+        $variant = $orderItem->getVariant();
+        if ($appliesOnDiscounted === false && !empty($variant->getAppliedPromotionsForChannel($channel))) {


!$appilesOnDiscounted?

Also, maybe we could have hasAppliedPromotionsForChannel($channel): bool on the variant? It would seem more natural this way

Zales0123 · 2021-12-09T18:24:31Z

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

+                $distributedAmounts[] = 0;
+            } else {
+                $distributedAmounts[] = (int) round(($element * $amount) / $total, 0, \PHP_ROUND_HALF_DOWN);
+            }


I would extract it to the private function or make it shorter:

$distributedAmounts[] = ($element === 0) ? 0 : (int) round(($element * $amount) / $total, 0, \PHP_ROUND_HALF_DOWN);

Zales0123 · 2021-12-09T18:25:17Z

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

+            }
+        }
+
+        if(array_sum($distributedAmounts) === 0) {


Missing space after if 😱

Zales0123 · 2021-12-09T18:30:31Z

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

        if ($this->minimumPriceDistributor !== null) {
-            $splitPromotion = $this->minimumPriceDistributor->distribute($subject->getItems()->toArray(), $promotionAmount, $subject->getChannel());
+            $splitPromotion = $this->minimumPriceDistributor->distribute($subject->getItems()->toArray(), $promotionAmount, $subject->getChannel(), $promotion->getAppliesToDiscounted());
        } else {
            $itemsTotal = [];
            foreach ($subject->getItems() as $orderItem) {
+                if ($promotion->getAppliesToDiscounted()) {
+                    $itemsTotal[] = $orderItem->getTotal();
+
+                    continue;
+                }
+
+                $variant = $orderItem->getVariant();
+                if (!empty($variant->getAppliedPromotionsForChannel($subject->getChannel()))) {
+                    $itemsTotal[] = 0;
+
+                    continue;
+                }
+
                $itemsTotal[] = $orderItem->getTotal();
            }


My soul hurts, as this part of code is identical to this one 😢

Zales0123 · 2021-12-09T18:32:43Z

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

@@ -107,4 +111,23 @@ private function calculate(int $unitTotal, ?int $minimumPrice, int $promotionAmo

        return $promotionAmount;
    }
+
+    private function canPromotionBeApplied(OrderItemUnitInterface $unit, PromotionInterface $promotion): bool


Shouldn't we make it protected? It's a solid part of logic, that would not be easily replaceable (as is the private function in the extendable class)

lchrusciel · 2021-12-09T14:28:12Z

...romotion/receiving_discount/receiving_discounts_with_product_minimum_price_specified.feature

+    Scenario: Distributing discount proportionally between different products when one has minimum price specified and promotion does not apply on discounted products
+        Given this promotion does not apply on discounted products
+        And it gives "$27" discount to every order
+        And there is a catalog promotion "Fixed T-Shirt sale" that reduces price by fixed "$2.50" in the "United States" channel and applies on "PHP Mug" product


Suggested change

And there is a catalog promotion "Fixed T-Shirt sale" that reduces price by fixed "$2.50" in the "United States" channel and applies on "PHP Mug" product

And there is a catalog promotion "Fixed Mug sale" that reduces price by fixed "$2.50" in the "United States" channel and applies on "PHP Mug" product

AdamKasp · 2021-12-09T20:14:29Z

Thanks, Grzegorz! 🥇

GSadee added Feature New feature proposals. Behat Issues and PRs aimed at improving Behat usage. Shop ShopBundle related issues and PRs. API APIs related issues and PRs. labels Dec 1, 2021

GSadee requested a review from a team as a code owner December 1, 2021 14:27

probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Dec 1, 2021

GSadee force-pushed the cart-promotions-vs-catalog-promotions branch 3 times, most recently from e74c157 to f3ee1bb Compare December 5, 2021 20:59

SirDomin force-pushed the cart-promotions-vs-catalog-promotions branch from 640d16e to 9e6efd5 Compare December 8, 2021 02:47

SirDomin changed the title ~~[WIP][CartPromotion][CatalogPromotion] Receiving discount only on non discounted products~~ [CartPromotion][CatalogPromotion] Receiving discount only on non discounted products Dec 8, 2021

GSadee commented Dec 8, 2021

View reviewed changes

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

GSadee and others added 6 commits December 8, 2021 16:35

[CartPromotion][CatalogPromotion][Behat] Add scenarios for receiving …

4413793

…discount only on non discounted products

[CartPromotion][CatalogPromotion][Behat] Implement scenarios for rece…

a65561e

…iving discount only on non discounted products

[CartPromotion][CatalogPromotion][Behat] Fixes after rebase with chan…

3556f47

…ging the name of field

[CartPromotion][CatalogPromotion] Receiving discount only on non disc…

1f1e798

…ounted products for unit cart promotions

[WIP][CartPromotion][CatalogPromotion] Receiving discount only on non…

372365b

… discounted products for fixed order cart promotions

[CartPromotion] handle fixed and percentage order discount

e735927

SirDomin force-pushed the cart-promotions-vs-catalog-promotions branch from 9e6efd5 to 4227f91 Compare December 9, 2021 08:27

[MinimumPrice][ApplyOnDiscounted] merge solutions for apply on discou…

9ce4cb8

…nt and minimum prices

SirDomin force-pushed the cart-promotions-vs-catalog-promotions branch from 4227f91 to 9ce4cb8 Compare December 9, 2021 08:47

AdamKasp reviewed Dec 9, 2021

View reviewed changes

Zales0123 reviewed Dec 9, 2021

View reviewed changes

lchrusciel reviewed Dec 9, 2021

View reviewed changes

AdamKasp approved these changes Dec 9, 2021

View reviewed changes

AdamKasp merged commit 3ff3cf7 into Sylius:master Dec 9, 2021

GSadee deleted the cart-promotions-vs-catalog-promotions branch December 10, 2021 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CartPromotion][CatalogPromotion] Receiving discount only on non discounted products #13349

[CartPromotion][CatalogPromotion] Receiving discount only on non discounted products #13349

GSadee commented Dec 1, 2021

AdamKasp Dec 9, 2021

Zales0123 Dec 9, 2021

AdamKasp Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

Zales0123 Dec 9, 2021

lchrusciel Dec 9, 2021

AdamKasp commented Dec 9, 2021

	And there is a catalog promotion "Fixed T-Shirt sale" that reduces price by fixed "$2.50" in the "United States" channel and applies on "PHP Mug" product
	And there is a catalog promotion "Fixed Mug sale" that reduces price by fixed "$2.50" in the "United States" channel and applies on "PHP Mug" product

[CartPromotion][CatalogPromotion] Receiving discount only on non discounted products #13349

[CartPromotion][CatalogPromotion] Receiving discount only on non discounted products #13349

Conversation

GSadee commented Dec 1, 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

AdamKasp commented Dec 9, 2021