Skip to content

[Woo POS] [Receipts] Set order metadata with the formatted change due amount string when a cash payment is made #15559

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

jaclync
Copy link
Contributor

@jaclync jaclync commented Apr 28, 2025

Closes: WOOMOB-378

Just one reviewer is required.

⚠️ for author: potential merge conflicts with #15557.

Description

This pull request sends the change amount from each cash payment in POS in the order's metadata _cash_change_amount field. The most important changes involve adding functionality for encoding and passing the cash payment change due amount, updating relevant protocols and controllers, and adding comprehensive test coverage. There was a small refactoring to replace OrderAction.updateOrder in PointOfSaleOrderController with a new POSOrderServiceProtocol.markOrderAsCompletedWithCashPayment, so that we don't have to change many of the existing OrderAction.updateOrder use cases for the new parameter.

POS Cash Payment Enhancements:

  • Added a new optional parameter cashPaymentChangeDueAmount to the updateOrder and updatePOSOrder methods in OrdersRemote to encode and pass cash payment change amounts.
  • Updated POSOrdersRemoteProtocol to include the cashPaymentChangeDueAmount parameter in the updatePOSOrder method.
  • Updated PointOfSaleOrderControllerProtocol and its implementations to handle the changeDueAmount parameter when collecting cash payments. The core order update logic was moved to POSOrderServiceProtocol.markOrderAsCompletedWithCashPayment so that order update actions can call the async/await OrdersRemote.updatePOSOrder with the cash payment change due amount parameter there.

UI and View Model Updates:

  • Modified PointOfSaleCollectCashView to calculate and pass the change due amount using a helper method.
  • Enhanced CollectCashViewHelper with methods to compute and format the change due amount.

Test Coverage:

  • Added unit tests in OrdersRemoteTests to verify encoding of cash payment change amounts and ensure no metadata is included when the parameter is absent.
  • Updated PointOfSaleOrderControllerTests to validate the behavior of collectCashPayment with and without a change due amount, and to ensure proper error handling and analytics tracking.
  • Added unit tests in POSOrderServiceTests for the new function markOrderAsCompletedWithCashPayment with the core order update logic with change due amount.

Steps to reproduce

No change due amount

  • Switch to a store eligible for POS and has at least one eligible product, if needed
  • Go to Menu tab > Point of Sale
  • Add product(s) to cart and check out
  • Tap Cash payment and enter the exact order amount, then mark payment as complete --> should hear a cha-ching
  • In an API testing tool (I personally used Postman), check the API response of the newly completed order --> the meta_data value should have an entry with key _cash_change_amount with the zero formatted change due amount as its value. In the response, payment_method should be cod and payment_method_title should be the localized version of "Pay in Person", just like before

Non-zero change due amount

  • Tap to create a new order in POS
  • Add product(s) to cart and check out
  • Tap Cash payment and enter a higher amount than the order amount, note the change due amount, then mark payment as complete --> should hear a cha-ching
  • In an API testing tool (I personally used Postman), check the API response of the newly completed order --> the meta_data value should have an entry with key _cash_change_amount with the formatted change due amount as its value. The amount string should be formatted with the store currency settings but does not include the currency symbol. In the response, payment_method should be cod and payment_method_title should be the localized version of "Pay in Person", just like before

Screenshots

Example request body:

{
  "payment_method_title" : "Pay in Person",
  "payment_method" : "cod",
  "status" : "completed",
  "meta_data" : [
    {
      "id" : 0,
      "key" : "_cash_change_amount",
      "value" : "5"
    }
  ]
}

  • 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.

jaclync added 4 commits April 28, 2025 15:15
…tOfSaleOrderController` to `POSOrderService` to avoid having to update `OrderAction.updateOrder` that implies a lot more unnecessary changes from existing usage.
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 28, 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 Number29790
VersionPR #15559
Bundle IDcom.automattic.alpha.woocommerce
Commit04e6921
Installation URL2m9ur1pimqmkg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync marked this pull request as ready for review April 28, 2025 08:27
@jaclync jaclync changed the title Feat/woomob 378 set formatted cash change due amount for pos cash payment [Woo POS] [Receipts] Set order metadata with the formatted change due amount string when a cash payment is made Apr 28, 2025
@jaclync jaclync added type: task An internally driven task. feature: point of sale labels Apr 28, 2025
@jaclync jaclync added this to the 22.3 milestone Apr 28, 2025
@joshheald joshheald self-assigned this Apr 28, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

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

Works well; a question inline about whether we should send the change even when it's 0

Comment on lines -136 to -152
let fieldsToUpdate: [OrderUpdateField] = [
.status,
.paymentMethodID,
.paymentMethodTitle]
let updatedOrder = order.copy(status: .completed,
paymentMethodID: PaymentGateway.Constants.cashOnDeliveryGatewayID,
paymentMethodTitle: Localization.cashPaymentMethodTitle)

let _ = try await withCheckedThrowingContinuation { continuation in
let action = OrderAction.updateOrder(siteID: siteID,
order: updatedOrder,
giftCard: nil,
fields: fieldsToUpdate,
onCompletion: { [weak self] result in
guard let self = self else { return }
switch result {
case .success:
self.celebrate()
case .failure:
analytics.track(.pointOfSaleCashPaymentFailed)
}
continuation.resume(with: result)
})
stores.dispatch(action)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tidying up, this should definitely be in the service

Comment on lines +306 to +307
func collectCashPayment(changeDueAmount: String?) async throws {
try await orderController.collectCashPayment(changeDueAmount: changeDueAmount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this optional? There's always a change amount for cash payments, it's just that sometimes it's $0. There's potentially value in specifically knowing that the app calculated the change as zero... are there other reasons not to send it in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, it's optional not only for 0 change due amount but also for unexpected scenarios like when it cannot parse the currency and input text:

guard let orderDecimal = parseCurrency(orderTotal),
let inputDecimal = parseCurrency(textFieldAmountInput) else {

I agree that 0 change due amount can be shown on the receipt as well. The zero case was skipped before just because it was following the logic where the UI doesn't show a message for 0 change due amount. From Perplexity:

If the change due is zero, many POS systems will omit displaying or printing the change due amount, often configurable by the user or store settings.

I also checked a few receipt examples and a bunch of them do show 0 change due amount. I believe the best UX is for the app to set the order metadata for all >= 0 change due amount, and the backend can decide whether to show it or not (we will show it by default for Basic POS) like based on a POS configuration in the future.

Updated in bcf0589 and ready for another check!

Screenshot 2025-04-29 at 10 05 37 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍 thanks!

Another thought (sorry, should have come up with this yesterday):

Why send the formatted value? The API doesn't send us formatted values, it sends a currency code and an unformatted string for the amount. Perhaps we should reply in the same way?

We don't really support multicurrency well, but we shouldn't make it harder for ourselves – relying on the currency set on the order is reasonable. We can throw an error in the app if someone tries to give change in a different currency to the order – I don't think that'll even come up yet though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thought, better late than never! I was sending the formatted value just so that the backend can just display it right away. I just checked the order API response including other metadata like from WooPayments, the amounts are not formatted. Let me try making it work in core first, then I will update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the currency symbol in 7111d68. Can be tested with woocommerce/woocommerce#57734 that displays the formatted change due amount.

@wpmobilebot wpmobilebot modified the milestones: 22.3, 22.4 May 2, 2025
@wpmobilebot
Copy link
Contributor

Version 22.3 has now entered code-freeze, so the milestone of this PR has been updated to 22.4.

@jaclync
Copy link
Contributor Author

jaclync commented May 6, 2025

@joshheald I updated the PR to send the cash payment change due amount without the currency symbol, if you'd like to take another look. This can be tested with woocommerce/woocommerce#57734 that displays the formatted change due amount with the order currency symbol.

@jaclync jaclync enabled auto-merge May 13, 2025 01:27
@jaclync jaclync merged commit 4940372 into trunk May 13, 2025
13 checks passed
@jaclync jaclync deleted the feat/WOOMOB-378-set-formatted-cash-change-due-amount-for-pos-cash-payment branch May 13, 2025 01:45
@jaclync
Copy link
Contributor Author

jaclync commented May 13, 2025

Merging as the core work woocommerce/woocommerce#57734 was merged.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: point of sale type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants