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

Jump to bottom

Do not reconvert discounts #20598

Merged
merged 1 commit into from Aug 21, 2020
Merged

Do not reconvert discounts #20598

merged 1 commit into from Aug 21, 2020

Conversation

atomiix
Copy link
Contributor

@atomiix atomiix commented Aug 14, 2020 •
edited

Questions Answers
Branch? 1.7.7.x
Description? In the FO, when adding a discount created with a different currency than the one used in the FO, the amount of the reduction could be wrong. It seems that we were converting an already converted amount to the current currency. This PR fixes it.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #20594
How to test? See #20594, Also, be sure that #9914 is still fixed

This change is Reviewable

@atomiix atomiix requested a review from a team as a code owner August 14, 2020 13:14
@prestonBot prestonBot added 1.7.7.x Branch Bug Type: Bug labels Aug 14, 2020
@atomiix atomiix linked an issue Aug 14, 2020 that may be closed by this pull request
Wrong cart rules discount on FO when using alternative currency #20594
Closed
@atomiix atomiix added this to the 1.7.7.0 milestone Aug 14, 2020
matks
matks approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love a PR that removes more code than it adds ❤️

jolelievre
jolelievre reviewed Aug 17, 2020
View reviewed changes
src/Adapter/Presenter/Cart/CartPresenter.php
@@ -570,19 +570,6 @@ private function getTemplateVarVouchers(Cart $cart)
} else {
$freeShippingOnly = false;
$totalCartVoucherReduction = $this->includeTaxes() ? $cartVoucher['value_real'] : $cartVoucher['value_tax_exc'];
$currencyFrom = new \Currency($cartVoucher['reduction_currency']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you should remove it all 🤔 But maybe rather check if the currencies ids match, if they don't then you need to perform the conversion You can also use Tools::convertPriceFull which (I believe) manages all these checks and conversion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried many combinations with different currencies, different discount currencies, different kind of discounts and each time it worked well without performing any other conversion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does it mean $cart->getCartRules() already performs the conversion?
If this is something we changed we might need to mark this as a BC (if developers expect it to return not converted values)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's a recent change (introduced in 177) by this PR #14263 So I guess we just need to ensure that it doesn't create a regression related to this issue #9914

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it in the how to test section 😉

@jolelievre jolelievre added the Waiting for QA Status: action required, waiting for test feedback label Aug 17, 2020
@ngodefroy ngodefroy self-assigned this Aug 20, 2020
@ngodefroy
Copy link

@atomiix I created a 20AED amount cart rule here:
Capture d’écran 2020-08-20 à 14 09 00

I then applied this cart rule on the product below but the price of the discount should be 4,60 € instead of 5,52 €
Capture d’écran 2020-08-20 à 14 06 14

Currency rate AED
Capture d’écran 2020-08-20 à 14 05 47

@ngodefroy ngodefroy added Waiting for dev Status: action required, waiting for tech feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 20, 2020
@atomiix
Copy link
Contributor Author

atomiix commented Aug 20, 2020

@ngodefroy I think you forgot the tax: 20 / 4.35 = 4.60, 4.60 * 1.20 = 5.52

@atomiix atomiix added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for dev Status: action required, waiting for tech feedback labels Aug 20, 2020
@ngodefroy
Copy link

@atomiix OK for me, I misunderstood the "tax excluded" box, thank you.

@ngodefroy ngodefroy added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 20, 2020
@atomiix atomiix merged commit f4c8e12 into PrestaShop:1.7.7.x Aug 21, 2020
@atomiix atomiix deleted the fix/20594 branch August 21, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@matks matks matks approved these changes

@jolelievre jolelievre jolelievre approved these changes

Assignees

@ngodefroy ngodefroy

Labels
1.7.7.x Branch Bug Type: Bug QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
1.7.7.0
Development

Successfully merging this pull request may close these issues.

Wrong cart rules discount on FO when using alternative currency
6 participants
@atomiix @ngodefroy @PierreRambaud @matks @jolelievre @prestonBot