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 display voucher conversion between currencies #14263

Merged
merged 2 commits into from Jul 2, 2019

Conversation

@tomlev
Copy link
Member

commented Jun 18, 2019

Questions Answers
Branch? develop
Description? Fixes voucher display when discount is in other currency than current one
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9914
How to test? see ticket

This change is Reviewable

@tomlev tomlev requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 18, 2019

@tomlev tomlev changed the title Fix voucher conversion between currencies Fix display voucher conversion between currencies Jun 18, 2019

@matks
Copy link
Contributor

left a comment

I would like us to put all of this logic inside private methods 😅 but I'm not going to bother too much. Added a few questions/suggestions

$value = 0;
} else {
// convert to default currency
$value /= $currencyFrom->conversion_rate;

This comment has been minimized.

Copy link
@matks

matks Jun 18, 2019

Contributor

I'm worried about rounding issues when I see this 😐 . Am I too paranoid ?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 18, 2019

Contributor

You're not

This comment has been minimized.

Copy link
@tomlev

tomlev Jun 19, 2019

Author Member

unfortunately I've no tool to handle it correctly, nor guidelines to know how to round it.
following call $this->priceFormatter->convertAndFormat($value); will round correctly if the precision is large enough, which will be the case if the conversion rate is not 1000000+ ^^

) {
$cartRowCheapest = $cartRow;
// Discount (%) on the cheapest product
if ($cartRule->reduction_product == -1) {

This comment has been minimized.

Copy link
@matks

matks Jun 18, 2019

Contributor

Can we improve a little bit the code and replace this -1 by something that has meaning, like either a variable or a constant ?

This comment has been minimized.

Copy link
@tomlev

tomlev Jun 19, 2019

Author Member

this part was just refactored, I'm not sure to find quickly what these values are meant to be

$amount = $cartRow->applyPercentageDiscount($cartRule->reduction_percent);
$cartRuleData->addDiscountApplied($amount);
// Discount (%) on the selection of products
if ($cartRule->reduction_product == -2) {

This comment has been minimized.

Copy link
@matks

matks Jun 18, 2019

Contributor

Can we improve a little bit the code and replace this -2 by something that has meaning, like either a variable or a constant ?

@matks matks added this to the 1.7.7.0 milestone Jun 24, 2019

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@tomlev I still have the same issue with this PR

capture d'écran_1699

@matks

matks approved these changes Jul 2, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Thank you @tomlev

@matks matks merged commit db4ad89 into PrestaShop:develop Jul 2, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.