Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Aug 5, 2025

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.

A recent change (bdbc520) 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”`. 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 this 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.
@joshheald joshheald added this to the 23.0 milestone Aug 5, 2025
@joshheald joshheald added feature: mobile payments Related to mobile payments / card present payments / Woo Payments. Bug labels Aug 5, 2025
@joshheald joshheald requested a review from staskus August 5, 2025 14:08
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 5, 2025

App Icon📲 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 NameWooCommerce iOS Prototype
Build Numberpr15980-f124f9f
Version22.9
Bundle IDcom.automattic.alpha.woocommerce
Commitf124f9f
Installation URL4h77k8dqktkng
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

joshheald commented Aug 6, 2025

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
Copy link
Collaborator

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
Copy link
Contributor

staskus commented Aug 6, 2025

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

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

@joshheald joshheald merged commit fbe4080 into trunk Aug 6, 2025
13 checks passed
@joshheald joshheald deleted the task/fix-card-payments-failing-with-order-total-changed-error branch August 6, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug feature: mobile payments Related to mobile payments / card present payments / Woo Payments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants