Total price is wrong when product has price options in config with currency conversion

[ameotoko]

Pretty funny but serious bug. If you have a store config with a currency conversion, and a product with customer defined price options (i.e. those that add surcharge when selected in frontend) – the total price is calculated incorrectly.

Reproducible in Isotope Demo.

Details

1. Set up a config store where Price factor is not 1
2. Create an attribute of type "Radio button menu" or "Select menu"
3. Check "Defined by customer"
4. Create options in Options Manager:
   • one default option with "Price surcharge" +0.00
   • one additional option with "Price surcharge" +50.00

5. Add this attribute to the product type configuration and set the product price to 100.00

The default result in frontend (without currency conversion) is: product is displayed with total price of 100.00 and two options – 0.00 and 50.00. When you select 2nd option, price changes to 150.00.

Now switch to the new config. From the screenshot on the left you would expect the total price of "Premium" to be 153.20, but you can see it was miscalculated on the right screenshot.

I understand, that this was probably easy to miss when testing on Isotope Demo, because Price factors are close to 1 there. I'm building a shop with a conversion like 0.0175 (something like 1 to 57), and I now get ridiculous prices for surcharged options, like 529.212,00 instead of 48.789,00, so it is finally noticeable.

[ameotoko]

If you debug Isotope\Frontend::addOptionsPrice() while switching to the 2nd option, you can see what's happening.

The converted surcharge is being retrieved in line 729, and then added to the unconverted product price in line 733:

core/system/modules/isotope/library/Isotope/Frontend.php

Lines 726 to 735 in 175a07f

	/** @var AttributeOption $objOption */
	foreach ($objOptions as $objOption) {
	if (\in_array($objOption->getLanguageId(), $value)) {
	$amount = $objOption->getAmount($fltPrice, 0);
	$objTax = $objSource->getRelated('tax_class');
	if ($objOption->isPercentage() || !$objTax instanceof TaxClass) {
	$fltAmount += $amount;
	continue;
	}

The already wrong result is then converted one more time in Isotope\Isotope::calculatePrice():

core/system/modules/isotope/library/Isotope/Isotope.php

Lines 252 to 262 in 175a07f

	if ($objConfig->priceMultiplier != 1) {
	switch ($objConfig->priceCalculateMode) {
	case 'mul':
	$fltPrice = $fltPrice * $objConfig->priceCalculateFactor;
	break;
	case 'div':
	$fltPrice = $fltPrice / $objConfig->priceCalculateFactor;
	break;
	}
	}

[aschempp]

can you please check if e271209 fixes the problem?

[ameotoko]

Yes, this logic fixes it. However, I fixed it in a different place in my project:

core/system/modules/isotope/library/Isotope/Model/AttributeOption.php

Lines 183 to 190 in b44de9a

	public function getAmount($fltPrice, $intTaxClass)
	{
	if ($this->isPercentage()) {
	return $fltPrice / 100 * $this->getPercentage();
	}
	return Isotope::calculatePrice($this->price, $this, 'price', $intTaxClass);
	}

    public function getAmount($fltPrice, $intTaxClass)
    {
        if ($this->isPercentage()) {
            return $fltPrice / 100 * $this->getPercentage();
        }


---     return Isotope::calculatePrice($this->price, $this, 'price', $intTaxClass);
+++     return $this->price;
    }

For two reasons:

1. This is the one place where I could not understand the logic – why does it return percented amount based on unconverted price, and relative amount based on converted one?
2. The test $objOption->isPercentage() you applied in e271209 is already present in getAmount() method inside the AttributeOption class. I had the feeling that the fix was supposed to be there originally, but got mixed up or somehow forgotten. Besides,

   core/system/modules/isotope/library/Isotope/Frontend.php

   Line 733 in e271209

   $amount = $objOption->isPercentage() ? $objOption->getAmount($fltPrice, 0) : $objOption->price;

   is the only occurence of AttributeOption::getAmount() ever being used in the core, so might as well just do it there.

What do you think?

[aschempp]

I understand, but I'm not sure that's a good idea. People might use that method since it is public API. Also, all other getAmount() methods return a calculated price, and the method description says so as well.