-
Notifications
You must be signed in to change notification settings - Fork 111
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
[Woo POS] [Cash & Receipts] Validate cash amount #14811
base: task/14602-handle-cash-payment-success
Are you sure you want to change the base?
[Woo POS] [Cash & Receipts] Validate cash amount #14811
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
@@ -112,6 +117,46 @@ struct PointOfSaleCollectCashView: View { | |||
} | |||
} | |||
|
|||
private extension PointOfSaleCollectCashView { | |||
func parseCurrency(_ amountString: String) -> NSDecimalNumber? { |
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've attempted to implement this from zero using Decimal
but dealing with currency was anything but fun, so I've relied on our own CurrencyFormatter
implementation. That said we seem to carry our own downsides (eg: #13829 ) so I'm not sure if there's a better way we could use to handle formatting here, since we use the same currency formatter in other places in POS 🤔
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.
Yes, understandable. It's okay to use what we currently have. We can make improvements internally within the formatters.
The largest issues I've seen were when converting amounts back and forth when creating and then editing Order fields again. It's not the case with this POS cash payment so it should be fine.
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.
Works well, thanks!
I suggest moving cash calculations into a separate helper and to write unit tests.
@@ -112,6 +117,46 @@ struct PointOfSaleCollectCashView: View { | |||
} | |||
} | |||
|
|||
private extension PointOfSaleCollectCashView { | |||
func parseCurrency(_ amountString: String) -> NSDecimalNumber? { |
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.
Yes, understandable. It's okay to use what we currently have. We can make improvements internally within the formatters.
The largest issues I've seen were when converting amounts back and forth when creating and then editing Order fields again. It's not the case with this POS cash payment so it should be fine.
currencyFormatter.convertToDecimal(amountString, locale: .current) | ||
} | ||
|
||
private func formatAsCurrency(_ amount: NSDecimalNumber) -> String { |
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.
All these methods could be unit tested (with mocked currency formatter). We have calculations happening within them. These methods would likely need to change into pure functions within a helper and then View would just set appropriate labels based on the result.
Closes: #14749
#14602 must be merged first
Description
This PR adds validation to the cash amount entered by a merchant during the cash collection flow.
Screen.Recording.2025-01-07.at.19.19.53.mov
Testing
.acceptCashForPointOfSale
feature flag.
or,
for decimals should behave the same.You will see some additional space at the bottom of the success view, as well as the floating button to connect the reader. This is unrelated to this PR and will be addressed in the parent PR (#14602) or in a follow-up
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: