-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixed Custom option price calculation is wrong with multi currency when option price type is percentage #19608
Conversation
…en option price type is percentage.
Hi @emiprotech. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
Hi @emiprotech , thanks for the contribution! Have you tested the fixed option price with multicurrency after the fix? I wonder if we need to add currency conversion here:
|
@sivaschenko, |
@emiprotech would you like to continue the progress on this pull request? |
@sivaschenko Yes, Please continue the progress on this pull request. |
Hi @emiprotech , I am closing this PR now due to inactivity. |
Hi @emiprotech, thank you for your contribution! |
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @emiprotech thanks for the updates, please take a look at my code review notes
/** | ||
* Option type percent | ||
*/ | ||
protected static $typePercent = 'percent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pleare reuse \Magento\Catalog\Model\Product\Option\Value::TYPE_PERCENT instead of introducing this property
@@ -160,6 +165,7 @@ public function hasOptions() | |||
*/ | |||
protected function _getPriceConfiguration($option) | |||
{ | |||
$option->getPriceType() == self::$typePercent ? $optionPrice = $option->getPrice(true) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to simplify the code here to be easier to read, like:
$optionPrice = $option->getPrice(true);
If ($option->getPriceType() != Value::TYPE_PERCENT) {
$optionPrice = $this->pricingHelper->currency($optionPrice, false, false);
}
Hi @sivaschenko |
Hi @sivaschenko, thank you for the review. |
Hi @emiprotech, thank you for your contribution! |
…i currency when option price type is percentage #19608
Description (*)
We have set multi currency custom option with percentage price type now it is working with multi currency switcher.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)