Fix orderTotalChanged error for 8dp vs 2dp amounts

[joshheald]

Description

A recent change #15957 means that we now get 8dp precision on numbers in the Order when we create or update an order. That change broke the card payment process, which performs a check comparing the initial order total (when the merchant taps collect payment) and the most recent total when we take the payment.

Because this was using a string comparison, after the change we started comparing values like “22.56” with “22.56000000”, and the comparison would always fail. This resulted in an error showing for card payments, stating that the order total had changed since the payment started.

Note that the API always rounds the total to 2dp and pads it to 8dp – this is different for tax values, which were the values prompting the previous change.

To fix, we now convert the totals to a NSDecimalNumber for comparison, which matches our treatment elsewhere in the app.

We’ll discuss the merits of the 8dp change elsewhere, but even if we revert it, this is a beneficial improvement.

Steps to reproduce

Set up a product with a price and tax rate which would result in more than 2dp of precision. For example – $6 with a 9.62% tax rate (prices entered exclusive of tax) would result in a price of $6.5772, but rounded that is $6.58

1. Launch the app and open POS or the Orders tab
2. Create the order with the product set up above
3. Collect a card payment for the order (TTP or Card Reader are both fine.)

The payment should proceed without showing an error mentioning that the "Order total has changed since the beginning of payment."

Screenshots

payment-fix-for-mismatching-totals.-.01.mp4

---

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

[wpmobilebot]

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

	App Name	WooCommerce iOS Prototype
	Build Number	pr15980-f124f9f
	Version	22.9
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	f124f9f
	Installation URL	4h77k8dqktkng

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

[staskus]

I tested this flow with different tax rates and amounts. I agree it's overall better than comparing the strings.

I also checked the code to make sure there aren't corner cases where this would fail. I think we had problems currencyFormatter when we formatted user input with different separators but it works fine with API strings.

---

I tried to look for number formatting where it fails. I wonder if it's a valid scenario were API returns 23.67 but we have a local value 23.66999999, since in that case it would still fail?

---

I tested it also within unit tests, feel free to add to CollectOrderPaymentUseCaseTests:

    func test_collectPayment_succeeds_when_order_total_precision_differs_between_initial_and_retrieved_order() throws {
        // Given an order with 2 decimal place precision
        let initialOrder = Order.fake().copy(siteID: defaultSiteID, orderID: defaultOrderID, total: "22.56")

        // And the retrieved order has 8 decimal place precision (same value, different formatting)
        let retrievedOrder = Order.fake().copy(siteID: defaultSiteID, orderID: defaultOrderID, total: "22.56000000")

        setUpUseCase(order: initialOrder)

        // Mock the order retrieval to return the 8dp version
        stores.whenReceivingAction(ofType: OrderAction.self) { action in
            switch action {
            case .retrieveOrderRemotely(_, _, let completion):
                completion(.success(retrievedOrder))
            default:
                break
            }
        }

        let paymentMethod = PaymentMethod.cardPresent(details: .fake())
        let intent = PaymentIntent.fake().copy(charges: [.fake().copy(paymentMethod: paymentMethod)])
        let capturedPaymentData = CardPresentCapturedPaymentData(paymentMethod: paymentMethod, receiptParameters: .fake())
        mockSuccessfulCardPresentPaymentActions(intent: intent, capturedPaymentData: capturedPaymentData)

        // When collecting payment
        var paymentCompleted = false
        var paymentFailed = false

        waitFor { promise in
            self.useCase.collectPayment(
                using: .bluetoothScan,
                channel: .storeManagement,
                onFailure: { error in
                    paymentFailed = true
                    // This should not happen with the decimal comparison fix
                    XCTFail("Payment should not fail due to precision mismatch. Error: \(error)")
                    promise(())
                },
                onCancel: {
                    XCTFail("Payment should not be canceled")
                    promise(())
                },
                onPaymentCompletion: {
                    paymentCompleted = true
                    promise(())
                },
                onCompleted: {}
            )
            self.mockPreflightController.completeConnection(reader: MockCardReader.wisePad3(), gatewayID: Mocks.paymentGatewayAccount)
        }

        // Then payment should succeed (with decimal comparison, "22.56" equals "22.56000000")
        XCTAssertTrue(paymentCompleted)
        XCTAssertFalse(paymentFailed)
    }

[joshheald]

@staskus thanks for the review

I tried to look for number formatting where it fails. I wonder if it's a valid scenario were API returns 23.67 but we have a local value 23.66999999, since in that case it would still fail?

The local value comes from the API as well, previously, it's not calculated locally. The API currently only returns rounded 2dp totals, padded to 8dp with 0s, so this doesn't happen... but you're right that it would fail if the API actually returned 8dp of precision for total.

This is where it all starts to get a bit silly... 8dp doesn't appear to ever do anything useful for us. I don't understand how it really helps with the tax calculation.

1. When preparing the order response, the total is passed through wc_format_decimal
2. That passes the value from get_total to number_format, passing our 8 dp value, or 2 by default.
3. get_total appears to return a 2dp string (a guess) and so number_format just pads it with zeroes.

None of this is mentioned in comments that I've seen, or documented.

For all examples and fields I've looked at, including the tax fields, the API returns 2dp of precision, padded to 8dp with zeroes, even when the calculation would return more precision.

I'll set this down more clearly in P2 and we can discuss, because it affects both platforms. I still think this PR is one we should merge... but the 8dp one should probably be reverted.

[joshheald]

I think the reason 8dp makes a difference is that the line_items.price values are returned to 8dp of precision. This happens whether or not we specify the dp parameter.

The app adds the line items by id and quantity one at a time as the order's built – totals are correct here even without specifying 8dp.

The totals could break (before we specified 8dp) when the app makes a final POST request sending back the order details, including line_items with total and subtotal specified. IIRC we do that in order to facilitate discounts and app-side tax overrides.

Specifying these amounts then causes the tax to be calculated on 5.50 rather than 5.5045869999999999, which is what the API sent back in the price field and what we used in the app.

It's a bit of a mess, tbh...

[dangermattic]

	1 Warning
⚠️	This PR is assigned to the milestone 23.0. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

[staskus]

It's a bit of a mess, tbh...

It definitely is... Thanks for diving deeper and explaining.