-
Notifications
You must be signed in to change notification settings - Fork 119
[Woo POS] Ensure auto-draft order removal on exit POS #15975
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
2f295b0
7ea9b21
4c4ed83
f4d7c91
fc81997
f88493c
dad2062
c8a032a
b07e138
a54af56
b96908b
f1ed6e5
b259f51
bfcb359
040be92
dcc23c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import Foundation | ||
| import Networking | ||
| import Storage | ||
|
|
||
| /// OrderStoreMethods extracts functionality of OrderStore that needs be reused within Yosemite | ||
| /// OrderStoreMethods is intentionally internal not to be exposed outside the module | ||
| /// | ||
| /// periphery: ignore | ||
| internal protocol OrderStoreMethodsProtocol { | ||
| func deleteOrder(siteID: Int64, | ||
| order: Order, | ||
| deletePermanently: Bool, | ||
| onCompletion: @escaping (Result<Order, Error>) -> Void) | ||
| } | ||
|
|
||
| internal class OrderStoreMethods: OrderStoreMethodsProtocol { | ||
| private let remote: OrdersRemote | ||
| private let storageManager: StorageManagerType | ||
|
|
||
| init( | ||
| storageManager: StorageManagerType, | ||
| remote: OrdersRemote | ||
| ) { | ||
| self.remote = remote | ||
| self.storageManager = storageManager | ||
| } | ||
|
|
||
| /// Deletes a given order. | ||
| /// Extracted from OrderStore.deleteOrder() implementation. | ||
| /// | ||
| func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result<Order, Error>) -> Void) { | ||
| // Optimistically delete the order from storage | ||
| deleteStoredOrder(siteID: siteID, orderID: order.orderID) | ||
|
|
||
| remote.deleteOrder(for: siteID, orderID: order.orderID, force: deletePermanently) { [weak self] result in | ||
| switch result { | ||
| case .success: | ||
| onCompletion(result) | ||
| case .failure: | ||
| // Revert optimistic deletion unless the order is an auto-draft (shouldn't be stored) | ||
| guard order.status != .autoDraft else { | ||
| return onCompletion(result) | ||
| } | ||
| self?.upsertStoredOrdersInBackground(readOnlyOrders: [order], onCompletion: { | ||
| onCompletion(result) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Storage Methods | ||
|
|
||
| private extension OrderStoreMethods { | ||
| /// Deletes any Storage.Order with the specified OrderID | ||
| /// Extracted from OrderStore.deleteStoredOrder() | ||
| /// | ||
| func deleteStoredOrder(siteID: Int64, orderID: Int64, onCompletion: (() -> Void)? = nil) { | ||
| storageManager.performAndSave({ storage in | ||
| guard let order = storage.loadOrder(siteID: siteID, orderID: orderID) else { | ||
| return | ||
| } | ||
| storage.deleteObject(order) | ||
| }, completion: onCompletion, on: .main) | ||
| } | ||
|
|
||
| /// Updates (OR Inserts) the specified ReadOnly Order Entities *in a background thread*. | ||
| /// Extracted from OrderStore.upsertStoredOrdersInBackground() | ||
| /// | ||
| func upsertStoredOrdersInBackground(readOnlyOrders: [Networking.Order], | ||
| removeAllStoredOrders: Bool = false, | ||
| onCompletion: (() -> Void)? = nil) { | ||
| storageManager.performAndSave({ [weak self] derivedStorage in | ||
| guard let self else { return } | ||
| if removeAllStoredOrders { | ||
| derivedStorage.deleteAllObjects(ofType: Storage.Order.self) | ||
| } | ||
| upsertStoredOrders(readOnlyOrders: readOnlyOrders, in: derivedStorage) | ||
| }, completion: onCompletion, on: .main) | ||
| } | ||
|
|
||
| /// Updates (OR Inserts) the specified ReadOnly Order Entities into the Storage Layer. | ||
| /// Extracted from OrderStore.upsertStoredOrders() | ||
| /// | ||
| func upsertStoredOrders(readOnlyOrders: [Networking.Order], | ||
| insertingSearchResults: Bool = false, | ||
| in storage: StorageType) { | ||
| let useCase = OrdersUpsertUseCase(storage: storage) | ||
| useCase.upsert(readOnlyOrders, insertingSearchResults: insertingSearchResults) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import Foundation | ||
| @testable import Yosemite | ||
| import Networking | ||
|
|
||
| final class MockOrderStoreMethods: OrderStoreMethodsProtocol { | ||
| var deleteOrderCalled = false | ||
| var deletedOrder: Order? | ||
| var deletedSiteID: Int64? | ||
| var deletedPermanently: Bool? | ||
| var deleteOrderResult: Result<Order, Error> = .success(Order.fake()) | ||
|
|
||
| func deleteOrder(siteID: Int64, | ||
| order: Order, | ||
| deletePermanently: Bool, | ||
| onCompletion: @escaping (Result<Order, Error>) -> Void) { | ||
| deleteOrderCalled = true | ||
| deletedOrder = order | ||
| deletedSiteID = siteID | ||
| deletedPermanently = deletePermanently | ||
| onCompletion(deleteOrderResult) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,7 +172,9 @@ extension PointOfSaleAggregateModel { | |
|
|
||
| func startNewCart() { | ||
| removeAllItemsFromCart() | ||
| orderController.clearOrder() | ||
| Task { | ||
| await orderController.clearOrder() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| setStateForEditing() | ||
| viewStateCoordinator.reset() | ||
| } | ||
|
|
@@ -613,7 +615,9 @@ extension PointOfSaleAggregateModel { | |
|
|
||
| // Before exiting Point of Sale, we warn the merchant about losing their in-progress order. | ||
| // We need to clear it down as any accidental retention can cause issues especially when reconnecting card readers. | ||
| orderController.clearOrder() | ||
| Task { | ||
| await orderController.clearOrder() | ||
| } | ||
|
|
||
| // Ideally, we could rely on the POS being deallocated to cancel all these. Since we have memory leak issues, | ||
| // cancelling them explicitly helps reduce the risk of user-visible bugs while we work on the memory leaks. | ||
|
|
||
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.
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