woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
301 stars 109 forks source link

[Collect Payment] Change is incorrectly calculated for values over $999.99 #13829

Open joshheald opened 2 months ago

joshheald commented 2 months ago

Description

When collecting a cash payment, we help merchants to calculate the change required.

If the amount is greater than 999, this calculation doesn't work correctly.

Context: p1725289245731479/1725288827.336759-slack-C6H8C3G23

Screenshots

IMG_6234 IMG_6235

dangermattic commented 2 months ago

Thanks for reporting! 👍

joshheald commented 2 months ago

The issue is converting an already formatted price back to decimal, rather than one we get from the API.

Most of the app formats currency according to the site settings, which are pretty freeform. I've set my test site to USD, with some quirky choices: CleanShot 2024-09-03 at 16 03 02@2x

The only currency setting which affects the API output for prices is the number of decimals – that's included in the API response itself.

However, the thousands/decimal separators and currency symbol are not included in the price strings. Of course, this could depend on the environment...

When calculating the change, we use CurrencyFormatter.convertToDecimal(_:locale:) passing the formattedTotal, not the API total string.

formattedTotal has previously been formatted using CurrencyFormatter.formatAmount(_:with:locale:numberOfDecimals:).

That adds in all the store-specific formatting, which could be.... anything really. After it's been formatted, we could apply the reverse rules as long as they haven't changed in the meantime, but to do that in a function which can also handle the API strings would be tricky. Unfortunately, that's what we're assuming, because we use the same function for API amount strings and these... enter convertToDecimal.

/// Returns a decimal value from a given string.
/// - Parameters:
///   - stringValue: the string received from the API
///   - locale: the locale that the currency string is based on.
///
public func convertToDecimal(_ stringValue: String, locale: Locale = .current) -> NSDecimalNumber? {

    let latinValue = stringValue.applyingTransform(StringTransform.toLatin, reverse: false) ?? stringValue

    // NSDecimalNumber use by default the local decimal separator to evaluate a decimal amount.
    // We substitute the current decimal separator with the locale decimal separator.
    let localeDecimalSeparator = locale.decimalSeparator ?? currencySettings.decimalSeparator
    var newStringValue = latinValue.replacingOccurrences(of: ",", with: localeDecimalSeparator)
    newStringValue = newStringValue.replacingOccurrences(of: ".", with: localeDecimalSeparator)

    // Removes the currency symbol, if any.
    let currencyCode = currencySettings.currencyCode
    let unit = currencySettings.symbol(from: currencyCode)
    newStringValue = newStringValue.replacingOccurrences(of: unit, with: "")

    let decimalValue = NSDecimalNumber(string: newStringValue, locale: locale)

    guard decimalValue != NSDecimalNumber.notANumber else {
        DDLogError("Error: string input is not a number: \(stringValue)")
        return nil
    }

    return decimalValue
}

This is fun... note the localeDecimalSeparator area of the code.

The exact problem we've noticed here is that when commas are used as thousand separators, we convert both thousand separators and decimal separators to the locale's decimal separator. In the screenshots above, there isn't a decimal, so instead of 3950, the formatter outputs 3. If there were decimals as well, it would give NaN, and return nil along with tracking a String is input is not a number error.

In every call to convertToDecimal, we're using the default current locale.

From what I can tell with a quick check, the API doesn't change its behaviour around numbers as the site's locale changes. It always respects the number of decimals, but never changes the decimal separator, nor includes the thousands separator or currency symbol.

joshheald commented 2 months ago

Option 1 – don't try to convert arbitrary strings

NumberFormatter would be a good choice here, but it is a little picky – e.g. if you tell it to use numberStyle: .currency, it'll return NaN for a string that doesn't include a currency symbol, or an unexpected currency symbol.

Given that this is usually used for converting API amount strings to decimal numbers, I think we should make that more obvious, and stop using it for converting formatted currency amounts to decimal numbers... which is something we should perhaps not do at all.

Then, we can use NumberFormatter with a decimal type, and a fixed locale for converting from the API.

This may also fix other areas of the app:

Option 2 – make convertToDecimal more powerful but more complicated

Alternatively, we could change the implementation of convertToDecimal to make it try a few times to get the number...

For example, try each of these approaches in this order, returning early as soon as it gets a number:

  1. use NumberFormatter in numberStyle = .decimal mode
  2. use NumberFormatter in numberStyle = .currency mode with the current device locale
  3. try .currency mode again, but set the store's currency symbol
  4. try manual normalization of the string, stripping out the currency code and thousands separator defined in the store settings, and converting the decimal from the store settings decimal point to the locale decimal point, then try NumberFormatter one more time.

Importantly, don't assume that , or . mean anything in particular!

joshheald commented 2 months ago

Option 3 – a little of this, a little of that

Since this logic has existed since version 3.4 of the app, I'm gonna go steady on this.

I'll change as many of the calls to convertToDecimal to use unformatted currency strings as possible, which should fix most of the issues, and then we can separately look at making this function do what people expect it to.

staskus commented 2 months ago

Option 3 – a little of this, a little of that

I agree, it doesn't sound to me that we get around the complexity, especially given different rules of locales, languages, currencies, and Woo itself.

In one of the payment apps I worked with, amounts were always represented by a type (Money) that would encapsulate all this complexity. It at least ensured common usage across the app, with a single way to convert to and from the money.

joshheald commented 2 months ago

Yep, that would be much better! I've got the symptom fix done: #13854. We can work more on the convertToDecimal fixes later, but I think it might be safest to make a replacement option, deprecate that function, then get each team to adopt the new one where it's complex to switch.