Skip to content
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

Fix multiple cart rules calculation #16724

Merged
merged 5 commits into from Jan 8, 2020

Conversation

@atomiix
Copy link
Contributor

atomiix commented Dec 6, 2019

Questions Answers
Branch? develop
Description? This fix discounts calculation with multiple cart rules
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16673
How to test? See #16673

This change is Reviewable

@atomiix atomiix added this to the 1.7.7.0 milestone Dec 6, 2019
@atomiix atomiix requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 6, 2019
@atomiix atomiix force-pushed the atomiix:cart-rules-calculation branch from c844763 to fea810f Dec 9, 2019
Copy link
Contributor

jolelievre left a comment

Is the issue #11722 still fixed with your modification? Is there a behat test dedicated to this previous issue?

$initialShipping = $this->getFees()->getInitialShippingFees();
$finalShipping = $this->getFees()->getFinalShippingFees();

if (null !== $initialShipping && null !== $finalShipping) {

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 9, 2019

Contributor

I'm not sure I get this part Part of the shipping is added, the other one is subsided.. Which one is what? Maybe this justifies a comment

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 9, 2019

Contributor

Since you compute the whole discount separately, is it still useful to applyFlatDiscount on rows during the computation process?

This comment has been minimized.

Copy link
@atomiix

atomiix Dec 10, 2019

Author Contributor

I'm not sure I get this part Part of the shipping is added, the other one is subsided.. Which one is what? Maybe this justifies a comment

You're right, I'll add a comment explaining this part!

Since you compute the whole discount separately, is it still useful to applyFlatDiscount on rows during the computation process?

I think it's not useful but I'm pretty sure that getting rid of it will introduce a BC break?

Edit: Actually, it's needed because otherwise you can't keep track of an already applied discount. Imagine 2 discounts of 50% each with 2 different priorities. If we do not keep track of the already applied discount the cart total will be 0€ instead of 50% of 50% of the amount.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 12, 2019

Contributor

Ok for applyFlatDiscount

For the shipping part I didn't get it at first but with your comment it seems logical, but I think we could simplify this an remove the comment with this:

if (null !== $initialShipping && null !== $finalShipping) {
    $shippingDiscount = (new AmountImmutable())->add($initialShipping)->sub($finalShipping);
    $totalWithoutDiscount = $totalWithoutDiscount->sub($shippingDiscount);
}

or to avoid the intermediate variables:

if (null !== $this->getFees()->getInitialShippingFees() && null !== $this->getFees()->getFinalShippingFees()) {
    $shippingDiscount = (new AmountImmutable())
                         ->add($this->getFees()->getInitialShippingFees())
                         ->sub($this->getFees()->getFinalShippingFees())
    ;
    $totalWithoutDiscount = $totalWithoutDiscount->sub($shippingDiscount);
}

What do you think?

This comment has been minimized.

Copy link
@atomiix

atomiix Dec 17, 2019

Author Contributor

Just updated the code 😉

@atomiix

This comment has been minimized.

Copy link
Contributor Author

atomiix commented Dec 10, 2019

Is the issue #11722 still fixed with your modification? Is there a behat test dedicated to this previous issue?

Yes the issue is still fixed with my modifications. The behat tests I added handle that case too.

@atomiix atomiix force-pushed the atomiix:cart-rules-calculation branch from 8b26e87 to 2ab9486 Dec 17, 2019
->add($this->getFees()->getInitialShippingFees())
->sub($this->getFees()->getFinalShippingFees())
;
$totalWithoutDiscount = $totalWithoutDiscount->add($shippingDiscount);

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 17, 2019

Contributor

Shouldn't it be a substraction here since it's a discount?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 17, 2019

Contributor

Or maybe it should be added to $amount ? To have the addition of product discount plus shipping discount?
By the way does $totalWithoutDiscount already contains shipping fees?

This comment has been minimized.

Copy link
@atomiix

atomiix Dec 17, 2019

Author Contributor

Actually it's the rest of the shipping discount so it should be added. Maybe I should rename the $shippingDiscountto better reflect what this variable is?

This comment has been minimized.

Copy link
@atomiix

atomiix Dec 17, 2019

Author Contributor

As seen with you on Slack, I changed the variable name to better understand that part

@atomiix atomiix force-pushed the atomiix:cart-rules-calculation branch from 2ab9486 to 9917dc8 Dec 17, 2019
@@ -150,10 +150,11 @@ public function getTotal($ignoreProcessedFlag = false)

$amount = new AmountImmutable();
foreach ($this->cartRows as $cartRow) {
$rowPrice = $cartRow->getFinalTotalPrice();
$rowPrice = $cartRow->getInitialTotalPrice();

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 17, 2019

Contributor

I think you can use getRowTotalWithoutDiscount here instead of the loop

src/Core/Cart/Calculator.php Show resolved Hide resolved
@atomiix atomiix force-pushed the atomiix:cart-rules-calculation branch from 9917dc8 to 102e45a Dec 18, 2019
Copy link
Contributor

jolelievre left a comment

Nice job @atomiix

$shippingFees = $this->fees->getFinalShippingFees();
$amount = $this->getRowTotalWithoutDiscount();
$amount = $amount->sub($this->getDiscountTotal());
$shippingFees = $this->fees->getInitialShippingFees();

This comment has been minimized.

Copy link
@arouiadib

arouiadib Jan 5, 2020

Contributor

Hi Thomas, could you please explain why using getInitialShippingFees() here instead of getFinalShippingFees? using getFinalShippingFees makes avoiding #9586 (at least).

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

khouloudbelguith commented Jan 6, 2020

Hi,

We have issues could be fixed with this PR:
-#16673 (comment)
-#9586 (comment) (regression compared to PS1.7.6.2)

Thanks!

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jan 8, 2020
@Progi1984 Progi1984 merged commit 3cfa99d into PrestaShop:develop Jan 8, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Jan 8, 2020

Thanks @atomiix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.