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

Cart rules mustn't be auto added automatically in some cases #15691

Merged
merged 4 commits into from Oct 10, 2019

Conversation

@PierreRambaud
Copy link
Contributor

PierreRambaud commented Sep 25, 2019

Questions Answers
Branch? 1.7.6.x
Description? Cart rules mustn't be auto added automatically during getTotalCalculationCartRules. The calculation here make the function recursive and going to an infinite loop 🤦‍♂
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15574
How to test? Follow ticket instruction.

This change is Reviewable

@PierreRambaud PierreRambaud requested a review from PrestaShop/prestashop-core-developers as a code owner Sep 25, 2019
@PierreRambaud PierreRambaud added the WIP label Sep 25, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 25, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 25, 2019

I have exactly the same fears than for #12878

And consequently I suggest the same strategy: #12878 (comment)

We could create a new function, used ONLY in a single situation. Our situation. So it means: creating a duplicate of Product:: getProducts(), and this duplicate function would be called ONLY for the situation we want to fix. This way, we make sure fixing this usecase cannot modify the behavior for another usecase and thus we highly reduce the possibility of introducing unexpected side effects.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 25, 2019

And Travis is not happy either

@PierreRambaud

This comment has been minimized.

Copy link
Contributor Author

PierreRambaud commented Sep 25, 2019

I have exactly the same fears than for #12878

And consequently I suggest the same strategy: #12878 (comment)

We could create a new function, used ONLY in a single situation. Our situation. So it means: creating a duplicate of Product:: getProducts(), and this duplicate function would be called ONLY for the situation we want to fix. This way, we make sure fixing this usecase cannot modify the behavior for another usecase and thus we highly reduce the possibility of introducing unexpected side effects.

This is exactly what I'm thinking. I really don't know why it works, and don't know what I break 😅
But I'm still not in favor of creating a new function, and because we want to fix how cart rules are calculated :/

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 25, 2019

Last commit that impacted this area of code is #6509

It's 3 years ago. It does not make sense if this is a regression of 1.7.6 😐

@PierreRambaud

This comment has been minimized.

Copy link
Contributor Author

PierreRambaud commented Sep 25, 2019

Last commit that impacted this area of code is #6509

It's 3 years ago. It does not make sense if this is a regression of 1.7.6

Nothing make sens in the legacy code 🤦‍♂

@PierreRambaud PierreRambaud removed the WIP label Sep 26, 2019
@PierreRambaud PierreRambaud changed the title Do not autoadd cart rules when using getTotalCalculationCartRules method Cart rules calculations shouldn't be done when using Cart::BOTH_WITHOUT_SHIPPING Sep 26, 2019
@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15574 branch from 9ef7d80 to 0da2d4f Oct 7, 2019
@Progi1984 Progi1984 added this to the 1.7.6.2 milestone Oct 8, 2019
@matks matks dismissed their stale review Oct 8, 2019

Travis is failing

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 8, 2019

Travis indicates this breaks some integration tests related to cart rules calculations 😭

On a side note, I have checked and it seems that this fix, combined with #12878, results in #11172 being fixed, and this is validated by the behat scenario #15452

So if we merge this fix, then it seems #12878 can maybe be merged.

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix/15574 branch from c65e119 to d8f1f50 Oct 8, 2019
@PierreRambaud PierreRambaud changed the title Cart rules calculations shouldn't be done when using Cart::BOTH_WITHOUT_SHIPPING Cart rules mustn't be auto added automatically in some cases Oct 8, 2019
@matks
matks approved these changes Oct 9, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 10, 2019

Thank you @PierreRambaud

@matks matks merged commit cd433be into PrestaShop:1.7.6.x Oct 10, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud PierreRambaud deleted the PierreRambaud:fix/15574 branch Oct 10, 2019
@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Oct 10, 2019

Thank you very very very much @PierreRambaud and @matks

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