-
Notifications
You must be signed in to change notification settings - Fork 121
POS cash payment: prefill order amount and update analytics tracking #16058
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
Conversation
…the merchant taps on cash payment from the checkout screen. Move the existing `cash_payment_tapped` event to be tracked when marking cash payment as completed.
…te button enabled state based on input & loading state, and remove validation error.
Generated by 🚫 Danger |
|
|
iamgabrielma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Works well, thanks for adding the additional test cases! 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're updating this file and testing the cash payment, we could also update the warnings for iOS17 in lines 65 and 108 regarding onChange deprecation 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed in 0dff142. Hopefully we can reset build warnings to 0 and new warnings are detected in CI in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw @iamgabrielma while checking your PR #16002 from the latest merge conflicts, the release notes entry seemed to get into 23.1 instead of 23.2. Not sure what impact it has though, maybe just for Peacock beta testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, appreciated, I did move them here then it seems messed it up when merging trunk into the branch. Thanks, I'll address it 🙇
| guard validateAmountOnSubmit() else { | ||
| return | ||
| } | ||
| ServiceLocator.analytics.track(.pointOfSaleCashPaymentTapped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily for this PR, but I wonder if we should expose the analytics prop in PointOfSaleAggregateModel, so the POS Views that depend on it can use this dependency for analytics, rather than relying on calling the shared instance of service locator. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reviewing Povilas' POS modularization PRs in the last HACK Week, he updated all the ServiceLocator analytics usage with an Analytics environment variable from the top level view for all POS views to access. I hope his work gets merged in soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll bring some of these things to the trunk, without the POS module itself, so it could be used within the code now.
| return false | ||
| } | ||
| return true | ||
| return inputDecimal >= orderDecimal && !isLoading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't find this function specially readable, perhaps just by switching the order a bit and extracting logic from the guard makes it clearer? Could be entirely personal so feel free to ignore (untested!):
func isPaymentButtonEnabled(orderTotal: String,
textFieldAmountInput: String,
isLoading: Bool) -> Bool {
if isLoading { return false }
let sanitizedInput = textFieldAmountInput.isNotEmpty ? textFieldAmountInput : "0"
guard let orderDecimal = parseCurrency(orderTotal),
let inputDecimal = parseCurrency(sanitizedInput) else {
return false
}
return inputDecimal >= orderDecimal
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for suggesting for better readability, updated in 7c576c0.
# Conflicts: # WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift
|
Thanks for the review! I responded to the comments and registered/updated the affected Tracks events. Merging shortly. |

For WOOMOB-1017
Just one review is required.
Description
This PR enhanced the POS cash payment UX to match Android with updated analytics tracking. The cash payment input field is now pre-filled with the order total with keyboard focused for easy replacement when merchants start typing. The payment button state is properly managed based on the entered amount and loading state.
The changes remove the validation error message since the button state now provides clear visual feedback about payment readiness. Added test coverage for the currency parsing functionality to ensure robust handling of various input formats.
Steps to reproduce
Notes:
FormattableAmountTextFieldcomponent separately.checkout_cash_payment_tappedshould be trackedcash_payment_tappedandcash_payment_failedare trackedcash_payment_tappedandcash_collect_payment_successare trackedTesting information
I tested on iPad Pro 11in 3rd generation, iOS 18.5.
Screenshots
ScreenRecording_08-29-2025.16-07-20_1.MP4
RELEASE-NOTES.txtif necessary.