[Woo POS] Ensure auto-draft order removal on exit POS #15975

iamgabrielma · 2025-08-05T08:49:03Z

Description

This PR addresses .autodraft orders not being deleted from storage when we exit POS after checkout, but before completing a payment.

The draft order is created in local storage when we perform POSOrderService.syncOrder() as a side effect of fetching the order remotely and then persisting it to storage temporarily in the background, when we're processing a card payment.

This should be cleared after collecting a payment and starting a new order, however it will remain visible if we just exit POS without clearing it explicitly (we only clear the in-memory order, not the autodraft created in local storage), which will remain until we pull to refresh in the order list.

Changes

The fix is simply to trigger the order deletion when we exit POS if the order is set to .autoDraft status
In order to support this without breaking modularity (DI StoresManager into POS), we:
- Create a OrderStoreMethods and OrderStoreMethodsProtocol for shared functionality around dispatching order actions.
- Create a separate POSOrderManagementService service to handle the order actions, abstracted from the app action dispatcher
- Delete the order through this service in the POS order controller.

Steps to reproduce / Testing information

Using a physical device, enter POS and connect the card reader (physical reader is necessary)
Add some products to the order and checkout
Before collecting payment, exit POS and navigate to Orders
If you are fast enough, you'll see the draft disappear when the order view is loaded, otherwise you should not see the draft order.
On a new order, confirm that can be completed via card payment, and we can start to a new order normally.

autodraft.orders.autoremoved.mp4

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

wpmobilebot · 2025-08-05T08:58:10Z

📲 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	`pr15975-dcc23c6`
	Version	`23.3`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`dcc23c6`
	Installation URL	34b5jhau5jqdo

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

wpmobilebot · 2025-08-22T09:03:23Z

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

wpmobilebot · 2025-09-05T03:31:39Z

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

iangmaia · 2025-09-19T08:20:45Z

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

…al-on-exit-POS

…al-on-exit-POS # Conflicts: # WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift

iamgabrielma · 2025-10-01T03:37:19Z

WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift

+
+    func waitForClearOrder() async {
+        while !clearOrderWasCalled {
+            try? await Task.sleep(nanoseconds: 1_000_000)


A question/feedback about this one: I've always though that using Task.sleep would be considered not the best practice, however we seem to use it scarcely in this test suite as well as other POS tests to handle testing async operations in mocks.

My first approach was just to add the waiting time to the tests itself, but updating the mock instead seemed cleaner, since the SUT is the aggregate model, and not the order controller.

I've also tried to use the confirmation API, but I got nowhere, so to avoid too many additional changes I decided that should be fine to add a small waiting time to account for the clearOrder func being now async.

Let me know if you have any thoughts about it.

Generally, any sort of timeout in test can cause flakiness and (by design) makes tests slower. I would encourage us to find other ways to test without them if possible.

I will check if I have any ideas for this case!

Yes, it can be tested without timeouts, but I agree it can be tricky with Swift Testing when mixing synchronous and asynchronous code.

First, we want to know when clearOrder was called. Since clearOrder is async, we need some mechanism to let us know when something happens asynchronously. The easiest one to use in mocks is a callback.

Second, we need to wait for tests until our async condition is met. You're right, we cannot use the confirmation API. I jumped on it many times. It can only be used if the method we're testing is async. In our case, neither sut.startNewCart()nor sut.pointOfSaleClosed() are async. You were very close to the solution based on the code, in these cases we need to use withCheckedContinuation().

Therefore, in our mocks we should have:

var clearOrderWasCalled: Bool = false var onClearOrderCalled: () -> Void = { } func clearOrder() async { clearOrderWasCalled = true onClearOrderCalled() }

in our tests we need to have:

sut.addToCart(makePurchasableItem()) await withCheckedContinuation { continuation in orderController.onClearOrderCalled = { continuation.resume() } // When sut.startNewCart() } // Then #expect(orderController.clearOrderWasCalled == true)

That's it! Here's the commit: https://github.com/woocommerce/woocommerce-ios/compare/task/WOOMOB-88-ensure-autodraft-order-removal-on-exit-POS...task/WOOMOB-88-ensure-autodraft-order-removal-on-exit-POS-with-async-tests?expand=1

That's very helpful, appreciated 🙇 . It's definitely tricky to wrap my head around about the flow here: enters the continuation block -> set the callback -> start new cart (which spawns async task) -> suspend continuation -> execute clear order -> trigger callback -> resume the continuation

staskus

Thank you for working on it.

There's one more case to handle, a few places to clean up (I think adaptors can and should be avoided), timeouts to remove.

I was also wondering if we could leave this behavior within the payment code. E.g. cleaning the order if payment was canceled. Not sure if it's the option but if that would be possible, I think that would be the cleanest solution hiding any complexity from the client of the payment code (POS).

staskus · 2025-10-01T15:15:48Z

Modules/Sources/Yosemite/Tools/POS/POSOrderManagementService.swift

+import struct Combine.AnyPublisher
+import struct NetworkingCore.JetpackSite
+
+public protocol POSOrderManagementServiceProtocol {


Should we use POSOrderService instead? I found it a bit counter-intuitive to encounter a different kind of order service or is there a good reason?

Great point. I don't have a good reason to keep it separate. It also makes more more sense to keep the order deletion action in the POSOrderService along with the other order-related functions. Updated on 040be92

staskus · 2025-10-01T15:26:18Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

        removeAllItemsFromCart()
-        orderController.clearOrder()
+        Task {
+            await orderController.clearOrder()


The draft order is created in local storage <...> as a side effect of fetching the order remotely and then persisting it to storage temporarily in the background, when we're processing a card payment.

Since the order is put into storage as a site effect when processing a card payment, could it also be cleared at the same level? My concern is that we may be solving the issue at the wrong abstraction level. While some code outside the POS puts the order into storage, the POS itself clears the storage. If something changes in the outside code, we may continue to delete the order without actually needing to do it.

What are your thoughts on that, would that be doable?

staskus · 2025-10-01T15:38:04Z

WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift

    }

-    func clearOrder() {
+    func clearOrder() async {


We should also clear the order when coming back to the cart, because we create a new one every time we check out.

Simulator.Screen.Recording.-.iPad.Air.11-inch.M3.-.2025-10-01.at.18.36.00.mov

staskus · 2025-10-01T15:44:15Z

WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift

    }
 }
+
+private struct POSOrderManagementServiceAdaptor: POSOrderManagementServiceProtocol {


This code can be removed, no need to use the adaptors or store dispatching. This is only required for dependencies that cannot be injected in any other way, and we prefer to use them through the Environment.

For our case:

POSOrderManagementService relies on OrderStoreMethodsProtocol

POSOrderManagementService can be created within POSTabCoordinator (like PointOfSaleCouponService), injecting methods

POSOrderManagementService can be passed to entry point view, which can then be passed to PointOfSaleOrderController.

staskus · 2025-10-01T16:09:27Z

WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift

+
+    func waitForClearOrder() async {
+        while !clearOrderWasCalled {
+            try? await Task.sleep(nanoseconds: 1_000_000)


Yes, it can be tested without timeouts, but I agree it can be tricky with Swift Testing when mixing synchronous and asynchronous code.

First, we want to know when clearOrder was called. Since clearOrder is async, we need some mechanism to let us know when something happens asynchronously. The easiest one to use in mocks is a callback.

Second, we need to wait for tests until our async condition is met. You're right, we cannot use the confirmation API. I jumped on it many times. It can only be used if the method we're testing is async. In our case, neither sut.startNewCart()nor sut.pointOfSaleClosed() are async. You were very close to the solution based on the code, in these cases we need to use withCheckedContinuation().

Therefore, in our mocks we should have:

var clearOrderWasCalled: Bool = false var onClearOrderCalled: () -> Void = { } func clearOrder() async { clearOrderWasCalled = true onClearOrderCalled() }

in our tests we need to have:

sut.addToCart(makePurchasableItem()) await withCheckedContinuation { continuation in orderController.onClearOrderCalled = { continuation.resume() } // When sut.startNewCart() } // Then #expect(orderController.clearOrderWasCalled == true)

That's it! Here's the commit: https://github.com/woocommerce/woocommerce-ios/compare/task/WOOMOB-88-ensure-autodraft-order-removal-on-exit-POS...task/WOOMOB-88-ensure-autodraft-order-removal-on-exit-POS-with-async-tests?expand=1

staskus · 2025-10-01T16:31:31Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

        removeAllItemsFromCart()
-        orderController.clearOrder()
+        Task {
+            await orderController.clearOrder()


Check if we don't need to capture orderController weakly here. It likely doesn't cause memory leaks but we can check just to be safe. I remember it did for some other calls that didn't complete.

wpmobilebot · 2025-10-17T04:51:58Z

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

check and delete auto-drafts on exit POS

2f295b0

iamgabrielma added type: task An internally driven task. feature: order creation All tasks related to creating an order feature: POS Bug labels Aug 5, 2025

iamgabrielma added this to the 23.0 milestone Aug 5, 2025

update release notes

7ea9b21

iamgabrielma added 3 commits August 5, 2025 16:43

add tests

4c4ed83

add delay for testing the dispatch

f4d7c91

make clearOrder() async

fc81997

iamgabrielma modified the milestones: 23.0, 23.1 Aug 6, 2025

wpmobilebot modified the milestones: 23.1, 23.2 Aug 22, 2025

wpmobilebot modified the milestones: 23.2, 23.3 Sep 5, 2025

iangmaia modified the milestones: 23.3, 23.4 Sep 19, 2025

iamgabrielma removed this from the 23.4 milestone Sep 29, 2025

iamgabrielma added 7 commits September 30, 2025 09:49

Merge branch 'trunk' into task/WOOMOB-88-ensure-autodraft-order-remov…

f88493c

…al-on-exit-POS

inject stores to dispatch order deletion

dad2062

Merge branch 'trunk' into task/WOOMOB-88-ensure-autodraft-order-remov…

c8a032a

…al-on-exit-POS # Conflicts: # WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift

temp DI stores

b07e138

make OrderStoreMethodsProtocol to share delete func between app and pos

a54af56

update tests

b96908b

periphery ignores

f1ed6e5

iamgabrielma marked this pull request as ready for review September 30, 2025 13:49

iamgabrielma added this to the 23.4 milestone Sep 30, 2025

iamgabrielma requested a review from staskus September 30, 2025 14:08

handle clear order being async in tests

b259f51

iamgabrielma commented Oct 1, 2025

View reviewed changes

staskus reviewed Oct 1, 2025

View reviewed changes

iamgabrielma modified the milestones: 23.4, 23.5 Oct 3, 2025

iamgabrielma added 3 commits October 3, 2025 14:32

update tests to get rid of timeout

bfcb359

Move deleteOrder to existing POSOrderService

040be92

lint

dcc23c6

wpmobilebot modified the milestones: 23.5, 23.6 Oct 17, 2025

[Woo POS] Ensure auto-draft order removal on exit POS #15975

Are you sure you want to change the base?

[Woo POS] Ensure auto-draft order removal on exit POS #15975

Uh oh!

Conversation

iamgabrielma commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Steps to reproduce / Testing information

Uh oh!

wpmobilebot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Aug 22, 2025

Uh oh!

wpmobilebot commented Sep 5, 2025

Uh oh!

iangmaia commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamgabrielma commented Aug 5, 2025 •

edited

Loading

wpmobilebot commented Aug 5, 2025 •

edited

Loading

iamgabrielma Oct 3, 2025 •

edited

Loading