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

Currency conversions result in incorrect values #6077

Open
lat9 opened this issue Dec 10, 2023 · 7 comments
Open

Currency conversions result in incorrect values #6077

lat9 opened this issue Dec 10, 2023 · 7 comments
Assignees

Comments

@lat9
Copy link
Contributor

lat9 commented Dec 10, 2023

Using master branch as of 2023-12-09 7:56PM Eastern Time, with the demo products, running PHP 8.2.

  • Site's default currency is USD, not displaying prices with tax.
  • Set the GBP conversion ratio to 0.83509400.
  • Using the "Currencies" sidebox, choose GB Pound as the selected currency.
  • View "Die Hard With a Vengeance Linked" (products_id = 12). The single unit price properly shows as £33.40 (US$39.99, 33.39540906 unrounded).
  • Add 4 of these to the cart; the cart total displays as £133.58 instead of £133.60.
  • That miscalculation is subsequently carried over into the associated order's Sub-total.

No logs are generated, just that incorrect

@lat9
Copy link
Contributor Author

lat9 commented Dec 10, 2023

Update: It's not just the quantity of four. Using the same settings for the original post and dropping the product quantity to 1, with a US$5.00 flat-rate shipping (£4.18, using the same conversion ratio), the order's total calculation is off by a penny as displayed on the checkout_payment page:
image

@lat9 lat9 changed the title Currency conversions for product quantities > 1 result in incorrect values Currency conversions result in incorrect values Dec 10, 2023
@brittainmark
Copy link
Contributor

Add 4 of these to the cart; the cart total displays as £133.58 instead of £133.60.

This looks like the conversion is done on the total USD cost 39.99 4 0.83509400 = 133.58
same thing for the total. £37.5708

So technically the figures are correct, even though they do not add up.

Rather than straight rounding, you could use PHP_ROUND_UP when you round to always round to the next penny. This would work for the total, but could cause a penny plus on the total.
Additionally, you could display the unit price to more decimal places so that the item price is correct.
Finally, you could add a note to explain that currency conversion may cause rounding errors.

OR

Have to change how the calculation is done. i.e. convert unit price and round, then apply multiples.
For the total, each figure would have to be rounded before they are summed and not use the total from orders_total table

OR

Display all the items in their default currency and only round the total with PHP_ROUND_UP set so you don't lose a cent.

I expect you have already worked all this out. Just wanted to clarify.

@lat9
Copy link
Contributor Author

lat9 commented Dec 10, 2023

Yep, I saw that processing. What I'm grappling with is the possibility that there are stores that have products that are unit-priced at less than 0.01; for example, a hardware store that sells nails at 0.0625 per nail with a minimum of 100. For that pricing, you'd want qty * price to be 'currency-converted' and then tax applied.

P.S. Since we're dealing with store's profit and loss, the calculations are not technically correct since they neither make sense nor do they add up!

@brittainmark
Copy link
Contributor

That been the case. Should you stick with the store base currency and just display the total converted to the guest currency?

@lat9
Copy link
Contributor Author

lat9 commented Dec 10, 2023

That been the case. Should you stick with the store base currency and just display the total converted to the guest currency?

Could you elaborate on your comment? I don't understand the suggestion.

@brittainmark
Copy link
Contributor

brittainmark commented Dec 11, 2023

Display all the figures in the base currency USD. an just display an extra line of field displaying the final total in GBP.
image

This way the accounts are in the local currency and the only change is for the currency conversion on the total.
Don't know if that is a good idea or not.

@lat9 lat9 added the 2.0.x label Jan 8, 2024
@lat9 lat9 self-assigned this Jan 8, 2024
@lat9
Copy link
Contributor Author

lat9 commented Jan 17, 2024

TL;DR

This issue has its basis in:

  1. Various pricing elements for a site, whether products or orders, are stored in the database in the site's default currency.
  2. The shopping_cart class rounds prices stored in its totals based on the currently-selected currency.

I was able to coerce the shopping_cart page's products' table to reflect the correct total by changing this section of the page's header_php.php

// $ppe is product price each, before one-time charges added
$ppe = $products[$i]['final_price'];
$ppe = zen_round(zen_add_tax($ppe, zen_get_tax_rate($products[$i]['tax_class_id'])), $currencies->get_decimal_places($_SESSION['currency']));
// $ppt is product price total, before one-time charges added
$ppt = $ppe * $products[$i]['quantity'];
$productsPriceEach = $currencies->format($ppe) . ($products[$i]['onetime_charges'] != 0 ? '<br>' . $currencies->display_price($products[$i]['onetime_charges'], zen_get_tax_rate($products[$i]['tax_class_id']), 1) : '');
$productsPriceTotal = $currencies->format($ppt) . ($products[$i]['onetime_charges'] != 0 ? '<br>' . $currencies->display_price($products[$i]['onetime_charges'], zen_get_tax_rate($products[$i]['tax_class_id']), 1) : '');

to

    $tax_rate = zen_get_tax_rate($products[$i]['tax_class_id']);

    // $ppe is product price each, before one-time charges added
    $ppe = $products[$i]['final_price'];
    $ppe = zen_round(zen_add_tax($ppe, $tax_rate), $currencies->get_decimal_places($_SESSION['currency']));

    // -----
    // If pricing is currently being displayed in a currency *other than* the site's
    // default, convert the per-item pricing now so that any quantity > 1 will be
    // multiplying based on that currency-converted value.
    //
    if ($_SESSION['currency'] !== DEFAULT_CURRENCY) {
        $ppe = $currencies->value($ppe);
    }

    // $ppt is product price total, before one-time charges added
    $ppt = $ppe * $products[$i]['quantity'];

    // -----
    // NOTE: Telling the currencies' formatter not to convert to the current currency
    // in use, since any conversion was performed when determining the product's
    // per-item price.
    //
    $productsPriceEach = $currencies->format($ppe, false) . ($products[$i]['onetime_charges'] != 0 ? '<br>' . $currencies->display_price($products[$i]['onetime_charges'], $tax_rate, 1) : '');
    $productsPriceTotal = $currencies->format($ppt, false) . ($products[$i]['onetime_charges'] != 0 ? '<br>' . $currencies->display_price($products[$i]['onetime_charges'], $tax_rate, 1) : '');

That's only a small part of the equation, though, as the shopping-cart class' total calculation is still off.

The products' total summation (i.e. an order's subtotal) are performed using the site's default currency and once a product's default-currency price is multiplied by the in-cart quantity, the rounding that occurs in that multiplication leads to the errant value for the cart's selected currency.

So far, I've found the following modules to require change for these calculations:

  1. /includes/classes/order.php
  2. /includes/classes/shopping_cart.php
  3. /includes/modules/pages/shopping_cart/header_php.php
  4. /includes/templates/*/templates/tpl_account_history_info_default.php (since, for whatever reason, those calculations are left to the templating rather than being performed in the page's header_php.php)
  5. /includes/template/*/templates/tpl_checkout_confirmation_default.php (ditto)
  6. /admin/orders.php

That said, the correction for this issue will be extremely complicated since it requires changes for the templating as well.

@lat9 lat9 removed the 2.0.x label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants