Fix formatting issues for discount and cash payment input values when the device and store currency settings are different

[staskus]

Closes: #13961

Description

Edit Discount View displays a different discount than originally added when using amounts larger than a thousand and store and device separator settings differ. Additionally, I found the same issue affects the Cash Payment view as well.

The issue comes from a broken PriceFieldFormatter.formatAmount method. We use it in two cases:

1. When a user inputs an amount into a text field
2. When reopening the view with a preset amount

When a user inputs an amount into a text field, the amount is likely inputted using the device decimal separator. The formatAmount method anticipates it and replaces the device decimal separator with the store decimal separator. It works as expected when the values are inputted since only one separator can be added and it's interpreted as a decimal separator.

When the text field is initialized with an already existing value, the value is formatted in store settings. If the value is larger than a thousand, we pass value with thousands and decimal separator: 1,000.55. However, if device settings uses different separators, PriceFieldFormatter.formatAmount fails to format the value as expected, and 'Edit Discount' or 'Cash Payment' views are initialized with wrong values.

Solution

1. Use PriceFieldFormatter.formatAmount only for user input. For this reason, I renamed it to formatUserInput and added a comment.
2. Create a separate PriceFieldFormatter.formatAmount that accepts Decimal. We expect to use this value when presetting the value to the text field. This method reuses the same formatting logic as formatUserInput but doesn't have to perform any sanitizing of the text field.
3. When presetting values for 'Edit Discount', 'Shipping', 'Card Payment', 'Custom Amount', turn the values into Decimal and use PriceFieldFormatter.formatAmount. We can convert values to Decimal using currencyFormatter.convertToDecimal since the values are formatted for API (more info [Order payments] Fix Change and Discount calculations for numbers including a grouping separator #13854).
4. Made an additional fix for 'Add Discount' view to use PriceFieldFormatter.amountDecimal value rather than Decimal when formatting the final price string. When we don't provide locale to Decimal, it fails to parse the string that uses , decimal separator. Since at this place 'Add Discount' tries to convert user input to Decimal, it's safer to use PriceFieldFormatter which relies on the same formatter to convert between the value and decimal.

Steps to reproduce

Original Issue - Edit Discount View

1. Store Settings, configure Thousands Separator to ,, Decimal Separator to .. (Default US Settings)
2. Device Settings (Settings -> Language & Region) set Number Format to (1234 567,89) (Lithuania Region Settings)
3. Open Woo app
4. Orders -> New Order
5. Add Product
6. Expand Product Row -> Select Discount
7. Add a discount larger than 1000 with a decimal separator (1000.54)
8. Add to close the discount view
9. Select the same discount again
10. Confirm the view is reopened with the value inputted into the text field

Video of an issue

Original.Issue.-.Edit.Discount.View.MP4

Related Issue - Cash Payment

1. Same setup as in Original Issue - Edit Discount View
2. Open Woo app
3. Orders -> New Order
4. Add Custom amount larger than 1000 with a decimal separator (1000.54)
5. Collect Payment
6. Cash
7. Confirm the view is opened with the correct value inputted into the text field

Video of an issue

Related.Issue.-.Cash.Payment.MP4

Semi-related issue - Formatting Discount Value

1. Store Settings, configure Thousands Separator to ., Decimal Separator to , (Reverse US Settings)
2. Device Settings (Settings -> Language & Region) set Number Format to (1234 567,89) (Lithuania Region Settings)
3. Add Product
4. Expand Product Row -> Select Discount
5. Input discount with a decimal separator
6. Confirm that the formatting value changes when changing fraction digits (10.44 -> 10.56...)

Video of an issue

Related.Issue.-.Formatting.Discount.Value.MP4

Testing information

This PR took a long time to test and find a setup where I could not encounter more critical issues. Input fields I tested:

• 'Order -> Add Custom Amount'
• 'Order -> Product -> Discount'
• 'Order -> Product -> Shipping'
• 'Order -> Collect Payment -> Cash'

The changes affect the same text fields I was fixing in an unrelated PR Fix Custom Amount input field is occasionally not responsive.

Verified given views with multiple combinations of Store Settings and Device Settings.

Store Settings:

• Thousands Separator . , and a random value
• Decimal Separator . , and a random value

Numbers:

• Positive / Negative

Updated unit tests to cover broken scenarios.

Device Settings:
All the combinations that iOS support:

Videos

Discount Line

Simulator.Screen.Recording.-.iPhone.15.-.2024-09-16.at.17.24.35.mp4

Cash Payment

Cash.Payment.-.Fixed.mp4

Random Store Separator for Discounts

Random.separator.-.Discount.-.Fixed.mp4

Custom Amount

Simulator.Screen.Recording.-.iPhone.15.-.2024-09-16.at.17.25.12.mp4

Shipping

Shipping.-.Not.Affected.mp4

---

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

• The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
• Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
• Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

[joshheald]

@staskus Thanks for improving this – I'm unlikely to have time to review until Thursday with my AFK and beta testing to do this afternoon, so I hope @iamgabrielma can do the review.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr13979-2eec1d4
	Version	20.4
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	2eec1d4
	App Center Build	WooCommerce - Prototype Builds #10902

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[iamgabrielma]

Thanks for working on this @staskus, it works great and I was unable to break it during testing 🎉

✅ Cash payment
✅ Formatting discount
✅ Discount line
✅ Custom amount
✅ Shipping

Specifically the Cash payment issue is a big one that we missed, but also easily missed since US-stores don't generally use values in the thousands. This one and the assumption of the values are inputted since only one separator can be added and it's interpreted as a decimal separator we made at some point when initially implementing this logic are very good points regarding not making assumptions too early on how features are used, or can bite us later.

I found some additional issues when testing, related to handling negative amounts, but these are unrelated to the changes in this PR and will log them on the side (for example, merchants can tap on "collect payment" with a negative-valued order, just to fail later on during the collection flow as cannot be collected).

I left some questions, just so we can log separate issues to be tackled at some point 👍

[iamgabrielma]

Question: Would it be worth to extract this bit into a separate function so we can log (maybe also test) if the conversion from String -> NSDecimal -> Decimal fails at some point?

[staskus]

Good question.

We can always log a failure to convert. Even within the currency formatter itself, so we would see something in the logs if users report issues.

In this instance, the conversion to Decimal can fail if CashPaymentTenderViewModel is initialized with total: String in an unexpected format. And technically this value is completely unconstrained. Anything can be passed through a total: String. It would be impossible to unit test this. A safer approach would be to always pass these values as Decimal rather than String, and only turn between Decimal and String in two cases:

1. When the value comes from the API
2. When the user inputs the value

I'll check what it would mean passing Decimal to CashPaymentTenderViewModel and if a conversion in a different place would be safer. However, I imagine a better solution would require a larger refactoring.

[staskus]

Okay, so CurrencyFormatter already logs an error:

DDLogError("Error: string input is not a number: \(stringValue)")

I checked the chain. total value comes from Order, which should be supported by CurrencyFormatter. Failure can only be unexpected.

[iamgabrielma]

Question (not for this PR): Should we allow this property to be of an optional type? As we're moving the responsibility of handling nil-values to the components who consume the price formatter, rather than dealing with issues directly here on an centralised manner.

[staskus]

I don't have a quick answer to this. Looking specifically at PriceFieldFormatter, amountDecimal being optional probably makes sense, since amount can also be empty.

The other option I imagine is to return 0 and maybe log the failure? I'll check how this method is used. If all the callers just turn this value into 0, we could also centralize it in PriceFieldFormatter.

[staskus]

I removed optionality from PriceFieldFormatter decimalValue

[iamgabrielma]

nit:

Suggested change

	func test_formatUserInput_upports_numbers_with_decimal_and_thousands_separator_uses_custom_separators() {
	func test_formatUserInput_supports_numbers_with_decimal_and_thousands_separator_uses_custom_separators() {

[staskus]

Fixed

[iamgabrielma]

I was curious to see if this fails if we assert against €1000,50 since we pass numberOfDecimals: 2 and it fails (expected since we're not validating this after the fact), but maybe we want to add an additional test to check if cuts down to the expected number of capped decimals if the user input happens to be more:

    func test_formatUserInput_when_decimals_more_than_expected_then_returns_the_max_numberOfDecimals_allowed() {
        // Given
        let customSettings = CurrencySettings(currencyCode: .EUR,
                                              currencyPosition: .left,
                                              thousandSeparator: ".",
                                              decimalSeparator: ",",
                                              numberOfDecimals: 2)

        let priceFieldFormatter = PriceFieldFormatter(locale: Locale(identifier: "lt-LT"), storeCurrencySettings: customSettings)

        // When
        _ = priceFieldFormatter.formatUserInput("1000,5123")

        // Then
        XCTAssertEqual(priceFieldFormatter.formattedAmount, "€1000,51")

[staskus]

Thanks for the suggestion, added.