diff --git a/Modules/Sources/Yosemite/Stores/Helpers/OrderStoreMethods.swift b/Modules/Sources/Yosemite/Stores/Helpers/OrderStoreMethods.swift new file mode 100644 index 00000000000..16b924221fb --- /dev/null +++ b/Modules/Sources/Yosemite/Stores/Helpers/OrderStoreMethods.swift @@ -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) -> 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) -> 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) + } +} diff --git a/Modules/Sources/Yosemite/Stores/OrderStore.swift b/Modules/Sources/Yosemite/Stores/OrderStore.swift index a4093adbfaa..db86604dea9 100644 --- a/Modules/Sources/Yosemite/Stores/OrderStore.swift +++ b/Modules/Sources/Yosemite/Stores/OrderStore.swift @@ -6,11 +6,23 @@ import Storage // MARK: - OrderStore // +/// periphery: ignore public class OrderStore: Store { private let remote: OrdersRemote + private let methods: OrderStoreMethods + + init(dispatcher: Dispatcher, + storageManager: StorageManagerType, + network: Network, + remote: OrdersRemote) { + self.remote = remote + self.methods = OrderStoreMethods(storageManager: storageManager, remote: remote) + super.init(dispatcher: dispatcher, storageManager: storageManager, network: network) + } public override init(dispatcher: Dispatcher, storageManager: StorageManagerType, network: Network) { self.remote = OrdersRemote(network: network) + self.methods = OrderStoreMethods(storageManager: storageManager, remote: self.remote) super.init(dispatcher: dispatcher, storageManager: storageManager, network: network) } @@ -85,7 +97,7 @@ public class OrderStore: Store { case let .markOrderAsPaidLocally(siteID, orderID, datePaid, onCompletion): markOrderAsPaidLocally(siteID: siteID, orderID: orderID, datePaid: datePaid, onCompletion: onCompletion) case let .deleteOrder(siteID, order, deletePermanently, onCompletion): - deleteOrder(siteID: siteID, order: order, deletePermanently: deletePermanently, onCompletion: onCompletion) + methods.deleteOrder(siteID: siteID, order: order, deletePermanently: deletePermanently, onCompletion: onCompletion) case let .observeInsertedOrders(siteID, completion): observeInsertedOrders(siteID: siteID, completion: completion) case let .checkIfStoreHasOrders(siteID, completion): @@ -97,6 +109,7 @@ public class OrderStore: Store { // MARK: - Services! // +/// periphery: ignore private extension OrderStore { /// Nukes all of the Stored Orders. diff --git a/Modules/Sources/Yosemite/Tools/POS/POSOrderService.swift b/Modules/Sources/Yosemite/Tools/POS/POSOrderService.swift index 270e2436dbb..e49c1370310 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSOrderService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSOrderService.swift @@ -4,6 +4,7 @@ import class WooFoundation.CurrencyFormatter import enum WooFoundation.CurrencyCode import struct Combine.AnyPublisher import struct NetworkingCore.JetpackSite +import protocol Storage.StorageManagerType public protocol POSOrderServiceProtocol { /// Syncs order based on the cart. @@ -13,16 +14,19 @@ public protocol POSOrderServiceProtocol { func syncOrder(cart: POSCart, currency: CurrencyCode) async throws -> Order func updatePOSOrder(orderID: Int64, recipientEmail: String) async throws func markOrderAsCompletedWithCashPayment(order: Order, changeDueAmount: String?) async throws + func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result) -> Void) } public final class POSOrderService: POSOrderServiceProtocol { private let siteID: Int64 private let ordersRemote: POSOrdersRemoteProtocol + private let orderStoreMethods: OrderStoreMethodsProtocol public convenience init?(siteID: Int64, credentials: Credentials?, selectedSite: AnyPublisher, - appPasswordSupportState: AnyPublisher) { + appPasswordSupportState: AnyPublisher, + storageManager: StorageManagerType) { guard let credentials else { DDLogError("⛔️ Could not create POSOrderService due to not finding credentials") return nil @@ -30,14 +34,18 @@ public final class POSOrderService: POSOrderServiceProtocol { let network = AlamofireNetwork(credentials: credentials, selectedSite: selectedSite, appPasswordSupportState: appPasswordSupportState) + let remote = OrdersRemote(network: network) self.init(siteID: siteID, - ordersRemote: OrdersRemote(network: network)) + ordersRemote: remote, + orderStoreMethods: OrderStoreMethods(storageManager: storageManager, remote: remote)) } - public init(siteID: Int64, - ordersRemote: POSOrdersRemoteProtocol) { + internal init(siteID: Int64, + ordersRemote: POSOrdersRemoteProtocol, + orderStoreMethods: OrderStoreMethodsProtocol) { self.siteID = siteID self.ordersRemote = ordersRemote + self.orderStoreMethods = orderStoreMethods } // MARK: - Protocol conformance @@ -81,6 +89,10 @@ public final class POSOrderService: POSOrderServiceProtocol { throw POSOrderServiceError.updateOrderFailed } } + + public func deleteOrder(siteID: Int64, order: Order, deletePermanently: Bool, onCompletion: @escaping (Result) -> Void) { + orderStoreMethods.deleteOrder(siteID: siteID, order: order, deletePermanently: deletePermanently, onCompletion: onCompletion) + } } private extension Order { diff --git a/Modules/Tests/YosemiteTests/Mocks/MockOrderStoreMethods.swift b/Modules/Tests/YosemiteTests/Mocks/MockOrderStoreMethods.swift new file mode 100644 index 00000000000..e5b083d3713 --- /dev/null +++ b/Modules/Tests/YosemiteTests/Mocks/MockOrderStoreMethods.swift @@ -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 = .success(Order.fake()) + + func deleteOrder(siteID: Int64, + order: Order, + deletePermanently: Bool, + onCompletion: @escaping (Result) -> Void) { + deleteOrderCalled = true + deletedOrder = order + deletedSiteID = siteID + deletedPermanently = deletePermanently + onCompletion(deleteOrderResult) + } +} diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSOrderServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSOrderServiceTests.swift index e7e09015375..ee041508b31 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSOrderServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSOrderServiceTests.swift @@ -5,11 +5,14 @@ import Testing struct POSOrderServiceTests { let sut: POSOrderService let mockOrdersRemote: MockPOSOrdersRemote + let mockOrderStoreMethods: MockOrderStoreMethods init() { let mockOrdersRemote = MockPOSOrdersRemote() + let mockOrderStoreMethods = MockOrderStoreMethods() self.mockOrdersRemote = mockOrdersRemote - self.sut = POSOrderService(siteID: 123, ordersRemote: mockOrdersRemote) + self.mockOrderStoreMethods = mockOrderStoreMethods + self.sut = POSOrderService(siteID: 123, ordersRemote: mockOrdersRemote, orderStoreMethods: mockOrderStoreMethods) } @Test diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 558975895ff..a7d7c793e8c 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -4,6 +4,7 @@ 23.4 ----- - [*] Order details: Fixes broken country selector navigation and "Non editable banner" width. [https://github.com/woocommerce/woocommerce-ios/pull/16171] +- [*] Point of Sale: Remove temporary orders from storage on exiting POS mode [https://github.com/woocommerce/woocommerce-ios/pull/15975] 23.3 ----- diff --git a/WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift b/WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift index 25fa3d2f068..a0bd64e3f74 100644 --- a/WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift +++ b/WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift @@ -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() diff --git a/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift b/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift index fdc704ad42a..f73800adbff 100644 --- a/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift +++ b/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift @@ -36,7 +36,7 @@ protocol PointOfSaleOrderControllerProtocol { @discardableResult func syncOrder(for cart: Cart, retryHandler: @escaping () async -> Void) async -> Result func sendReceipt(recipientEmail: String) async throws - func clearOrder() + func clearOrder() async func collectCashPayment(changeDueAmount: String?) async throws } @@ -114,11 +114,24 @@ protocol PointOfSaleOrderControllerProtocol { try await receiptSender.sendReceipt(orderID: order.orderID, recipientEmail: recipientEmail) } - func clearOrder() { + func clearOrder() async { + 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 + orderService.deleteOrder(siteID: order.siteID, order: order, deletePermanently: true) { _ in + continuation.resume() + } + } + } + } + private func celebrate() { celebration.celebrate() } diff --git a/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift b/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift index 03cddc67d67..592d5f7dc1c 100644 --- a/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift +++ b/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift @@ -172,7 +172,9 @@ extension PointOfSaleAggregateModel { func startNewCart() { removeAllItemsFromCart() - orderController.clearOrder() + Task { + await orderController.clearOrder() + } 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. diff --git a/WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift b/WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift index c1c67969475..e40a1082b14 100644 --- a/WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift +++ b/WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift @@ -146,7 +146,8 @@ private extension POSTabCoordinator { let orderService = POSOrderService(siteID: siteID, credentials: credentials, selectedSite: defaultSitePublisher, - appPasswordSupportState: isAppPasswordSupported), + appPasswordSupportState: isAppPasswordSupported, + storageManager: storageManager), #available(iOS 17.0, *) { let posView = PointOfSaleEntryPointView( siteID: siteID, diff --git a/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift b/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift index 56ef3ff4281..938b552e7b0 100644 --- a/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift +++ b/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift @@ -22,7 +22,7 @@ class PointOfSalePreviewOrderController: PointOfSaleOrderControllerProtocol { func sendReceipt(recipientEmail: String) async throws { } - func clearOrder() { } + func clearOrder() async { } func collectCashPayment(changeDueAmount: String?) async throws {} } diff --git a/WooCommerce/Classes/POS/Utils/PreviewHelpers.swift b/WooCommerce/Classes/POS/Utils/PreviewHelpers.swift index 30a1cff82d5..e8f7802ed63 100644 --- a/WooCommerce/Classes/POS/Utils/PreviewHelpers.swift +++ b/WooCommerce/Classes/POS/Utils/PreviewHelpers.swift @@ -448,6 +448,8 @@ final class POSOrderServicePreview: POSOrderServiceProtocol { func updatePOSOrder(orderID: Int64, recipientEmail: String) async throws {} func markOrderAsCompletedWithCashPayment(order: Yosemite.Order, changeDueAmount: String?) async throws {} + + func deleteOrder(siteID: Int64, order: Yosemite.Order, deletePermanently: Bool, onCompletion: @escaping (Result) -> Void) {} } final class POSReceiptServicePreview: POSReceiptServiceProtocol { diff --git a/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift b/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift index 9ffd927fbdd..c97b7c50702 100644 --- a/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift +++ b/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift @@ -14,6 +14,7 @@ import protocol WooFoundation.Analytics import enum Networking.DotcomError import enum Networking.NetworkError +@MainActor struct PointOfSaleOrderControllerTests { let mockOrderService = MockPOSOrderService() let mockReceiptSender = MockPOSReceiptSender() @@ -595,6 +596,67 @@ struct PointOfSaleOrderControllerTests { } } + @MainActor + @Test func clearOrder_when_no_order_then_does_not_dispatch_delete_action() async throws { + // Given + let sut = PointOfSaleOrderController(orderService: mockOrderService, + receiptSender: mockReceiptSender, + currencySettingsProvider: MockCurrencySettingsProvider(), + analytics: MockPOSAnalytics()) + + // When + await sut.clearOrder() + + // Then + #expect(mockOrderService.deleteOrderWasCalled == false) + #expect(sut.orderState == .idle) + } + + @MainActor + @Test func clearOrder_when_autodraft_order_then_dispatches_delete_action() async throws { + // Given + let sut = PointOfSaleOrderController(orderService: mockOrderService, + receiptSender: mockReceiptSender, + currencySettingsProvider: MockCurrencySettingsProvider(), + analytics: MockPOSAnalytics()) + + let fakeOrderItem = OrderItem.fake().copy(quantity: 1) + let fakeCartItem = makeItem(orderItemsToMatch: [fakeOrderItem]) + let autoDraftOrder = Order.fake().copy(status: .autoDraft) + mockOrderService.orderToReturn = autoDraftOrder + + await sut.syncOrder(for: .init(purchasableItems: [fakeCartItem]), retryHandler: { }) + + // When + await sut.clearOrder() + + // Then + #expect(mockOrderService.deleteOrderWasCalled == true) + #expect(mockOrderService.deletedOrder?.orderID == autoDraftOrder.orderID) + #expect(mockOrderService.deletedOrder?.status == .autoDraft) + #expect(sut.orderState == .idle) + } + + @MainActor + @Test func clearOrder_when_completed_order_then_does_not_dispatch_delete_action() async throws { + // Given + let sut = PointOfSaleOrderController(orderService: mockOrderService, + receiptSender: mockReceiptSender, + currencySettingsProvider: MockCurrencySettingsProvider(), + analytics: MockPOSAnalytics()) + + let completedOrder = Order.fake().copy(status: .completed) + mockOrderService.orderToReturn = completedOrder + await sut.syncOrder(for: .init(purchasableItems: [makeItem()]), retryHandler: {}) + + // When + await sut.clearOrder() + + // Then + #expect(mockOrderService.deleteOrderWasCalled == false) + #expect(sut.orderState == .idle) + } + @MainActor struct AnalyticsTests { private let analytics = MockPOSAnalytics() diff --git a/WooCommerce/WooCommerceTests/POS/Mocks/MockPOSOrderService.swift b/WooCommerce/WooCommerceTests/POS/Mocks/MockPOSOrderService.swift index 6c33349ebed..012b7c8b540 100644 --- a/WooCommerce/WooCommerceTests/POS/Mocks/MockPOSOrderService.swift +++ b/WooCommerce/WooCommerceTests/POS/Mocks/MockPOSOrderService.swift @@ -14,6 +14,10 @@ class MockPOSOrderService: POSOrderServiceProtocol { var spySyncOrderCurrency: CurrencyCode? var spyCashPaymentChangeDueAmount: String? + var deleteOrderWasCalled = false + var deletedOrder: Order? + var deleteOrderResult: Result = .success(Order.fake()) + func syncOrder(cart: Yosemite.POSCart, currency: CurrencyCode) async throws -> Yosemite.Order { syncOrderWasCalled = true @@ -60,6 +64,12 @@ class MockPOSOrderService: POSOrderServiceProtocol { throw error } } + + func deleteOrder(siteID: Int64, order: Yosemite.Order, deletePermanently: Bool, onCompletion: @escaping (Result) -> Void) { + deleteOrderWasCalled = true + deletedOrder = order + onCompletion(deleteOrderResult) + } } enum MockPOSOrderServiceError: Error { diff --git a/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift b/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift index 33bb4bd1e30..b79606a5945 100644 --- a/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift +++ b/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift @@ -33,8 +33,11 @@ final class MockPointOfSaleOrderController: PointOfSaleOrderControllerProtocol { } var clearOrderWasCalled: Bool = false - func clearOrder() { + var onClearOrderCalled: () -> Void = {} + + func clearOrder() async { clearOrderWasCalled = true + onClearOrderCalled() } var sendReceiptErrorToThrow: Error? diff --git a/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift b/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift index 4907c11771d..23ff8bc208d 100644 --- a/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift +++ b/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift @@ -213,11 +213,16 @@ struct PointOfSaleAggregateModelTests { cardPresentPaymentService: cardPresentPaymentService, orderController: orderController, ) - sut.addToCart(makePurchasableItem()) - // When - sut.startNewCart() + await withCheckedContinuation { continuation in + orderController.onClearOrderCalled = { + continuation.resume() + } + + // When + sut.startNewCart() + } // Then #expect(orderController.clearOrderWasCalled == true) @@ -322,11 +327,16 @@ struct PointOfSaleAggregateModelTests { itemsController: itemsController, cardPresentPaymentService: cardPresentPaymentService, orderController: orderController) - sut.addToCart(makePurchasableItem()) - // When - sut.pointOfSaleClosed() + await withCheckedContinuation { continuation in + orderController.onClearOrderCalled = { + continuation.resume() + } + + // When + sut.pointOfSaleClosed() + } // Then #expect(orderController.clearOrderWasCalled == true)