-
Notifications
You must be signed in to change notification settings - Fork 118
[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 13 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,44 @@ | ||
import Foundation | ||
import Networking | ||
import protocol Storage.StorageManagerType | ||
import struct Combine.AnyPublisher | ||
import struct NetworkingCore.JetpackSite | ||
|
||
public protocol POSOrderManagementServiceProtocol { | ||
/// Deletes an order permanently or moves it to trash | ||
/// - Parameters: | ||
/// - siteID: The site ID where the order belongs | ||
/// - order: The order to delete | ||
/// - deletePermanently: Whether to delete permanently or move to trash | ||
/// - onCompletion: Completion handler with the result | ||
func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result<Order, Error>) -> Void) | ||
} | ||
/// periphery: ignore | ||
public final class POSOrderManagementService: POSOrderManagementServiceProtocol { | ||
private let orderStoreMethods: OrderStoreMethodsProtocol | ||
|
||
public convenience init?(siteID: Int64, | ||
credentials: Credentials?, | ||
selectedSite: AnyPublisher<JetpackSite?, Never>, | ||
appPasswordSupportState: AnyPublisher<Bool, Never>, | ||
storageManager: StorageManagerType) { | ||
guard let credentials else { | ||
DDLogError("⛔️ Could not create POSOrderManagementService due to not finding credentials") | ||
return nil | ||
} | ||
let network = AlamofireNetwork(credentials: credentials, | ||
selectedSite: selectedSite, | ||
appPasswordSupportState: appPasswordSupportState) | ||
let remote = OrdersRemote(network: network) | ||
self.init(storageManager: storageManager, remote: remote) | ||
} | ||
|
||
public init(storageManager: StorageManagerType, remote: OrdersRemote) { | ||
self.orderStoreMethods = OrderStoreMethods(storageManager: storageManager, remote: remote) | ||
} | ||
|
||
// MARK: - Protocol conformance | ||
public func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result<Order, Error>) -> Void) { | ||
orderStoreMethods.deleteOrder(siteID: siteID, order: order, deletePermanently: deletePermanently, onCompletion: onCompletion) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import Yosemite | |
import protocol Experiments.FeatureFlagService | ||
import enum Experiments.FeatureFlag | ||
import protocol Storage.StorageManagerType | ||
|
||
final class POSServiceLocatorAdaptor: POSDependencyProviding { | ||
var analytics: POSAnalyticsProviding { | ||
POSAnalyticsAdaptor() | ||
|
@@ -29,6 +30,10 @@ final class POSServiceLocatorAdaptor: POSDependencyProviding { | |
var externalViews: POSExternalViewProviding { | ||
POSExternalViewAdaptor() | ||
} | ||
|
||
var orderManagement: POSOrderManagementServiceProtocol { | ||
POSOrderManagementServiceAdaptor() | ||
} | ||
} | ||
|
||
// MARK: - Individual Service Adaptors | ||
|
@@ -117,3 +122,12 @@ private struct POSExternalViewAdaptor: POSExternalViewProviding { | |
)) | ||
} | ||
} | ||
|
||
private struct POSOrderManagementServiceAdaptor: POSOrderManagementServiceProtocol { | ||
|
||
func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result<Order, Error>) -> Void) { | ||
let action = OrderAction.deleteOrder(siteID: siteID, order: order, deletePermanently: deletePermanently, onCompletion: onCompletion) | ||
Task { @MainActor in | ||
ServiceLocator.stores.dispatch(action) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import enum WooFoundation.CurrencyCode | |
import protocol WooFoundation.Analytics | ||
import enum Alamofire.AFError | ||
import class Yosemite.OrderTotalsCalculator | ||
import protocol Yosemite.POSOrderManagementServiceProtocol | ||
|
||
enum SyncOrderState { | ||
case newOrder | ||
|
@@ -36,7 +37,7 @@ protocol PointOfSaleOrderControllerProtocol { | |
@discardableResult | ||
func syncOrder(for cart: Cart, retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error> | ||
func sendReceipt(recipientEmail: String) async throws | ||
func clearOrder() | ||
func clearOrder() async | ||
func collectCashPayment(changeDueAmount: String?) async throws | ||
} | ||
|
||
|
@@ -45,11 +46,13 @@ protocol PointOfSaleOrderControllerProtocol { | |
receiptSender: POSReceiptSending, | ||
currencySettingsProvider: POSCurrencySettingsProviding, | ||
analytics: POSAnalyticsProviding, | ||
orderManagement: POSOrderManagementServiceProtocol, | ||
celebration: PaymentCaptureCelebrationProtocol = PaymentCaptureCelebration()) { | ||
self.orderService = orderService | ||
self.receiptSender = receiptSender | ||
self.currencySettingsProvider = currencySettingsProvider | ||
self.analytics = analytics | ||
self.orderManagement = orderManagement | ||
self.celebration = celebration | ||
} | ||
|
||
|
@@ -58,6 +61,7 @@ protocol PointOfSaleOrderControllerProtocol { | |
private let currencySettingsProvider: POSCurrencySettingsProviding | ||
private let celebration: PaymentCaptureCelebrationProtocol | ||
private let analytics: POSAnalyticsProviding | ||
private let orderManagement: POSOrderManagementServiceProtocol | ||
|
||
private(set) var orderState: PointOfSaleInternalOrderState = .idle | ||
private var order: Order? = nil | ||
|
@@ -114,11 +118,24 @@ protocol PointOfSaleOrderControllerProtocol { | |
try await receiptSender.sendReceipt(orderID: order.orderID, recipientEmail: recipientEmail) | ||
} | ||
|
||
func clearOrder() { | ||
func clearOrder() async { | ||
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. 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 |
||
await clearAutoDraftIfNeeded(for: order) | ||
order = nil | ||
orderState = .idle | ||
} | ||
|
||
private func clearAutoDraftIfNeeded(for order: Order?) async { | ||
guard let order, order.status == .autoDraft else { return } | ||
|
||
await withCheckedContinuation { continuation in | ||
Task { @MainActor in | ||
orderManagement.deleteOrder(siteID: order.siteID, order: order, deletePermanently: true) { _ in | ||
continuation.resume() | ||
} | ||
} | ||
} | ||
} | ||
|
||
private func celebrate() { | ||
celebration.celebrate() | ||
} | ||
|
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.
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?Uh oh!
There was an error while loading. Please reload this page.
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.
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