Merge branch 'task/collect-payment-success-event-properties' into task/pos-remaining-mvp-events

iamgabrielma · iamgabrielma · commit 050f2fa83f50 · 2025-02-18T14:18:48.000+01:00
# Conflicts:
#	WooCommerce/Classes/POS/Analytics/POSCollectOrderPaymentAnalytics.swift
#	WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift
diff --git a/WooCommerce/Classes/POS/Analytics/POSCollectOrderPaymentAnalytics.swift b/WooCommerce/Classes/POS/Analytics/POSCollectOrderPaymentAnalytics.swift
@@ -9,6 +9,7 @@ final class POSCollectOrderPaymentAnalytics: CollectOrderPaymentAnalyticsTrackin
     private var cardReaderReady: Double = 0
     private var cardReaderTapped: Double = 0
     private var checkoutTapCount: Int = 0
+    private var hasTrackedProcessingPayment = false
 
     private let analytics: Analytics
 
@@ -39,6 +40,9 @@ final class POSCollectOrderPaymentAnalytics: CollectOrderPaymentAnalyticsTrackin
             millisecondsSinceCardTapped: elapsedTimeSinceCardTapped,
             checkoutTapCount: checkoutTapCount
         ))
+
+        resetCheckoutTapCountTracker()
+        resetProcessingPaymentTracking()
     }
 
     func trackSuccessfulCashPayment() {
@@ -58,6 +62,8 @@ final class POSCollectOrderPaymentAnalytics: CollectOrderPaymentAnalyticsTrackin
     func trackReceiptPrintFailed(error: any Error) { }
 
     func trackCustomerInteractionStarted() {
+        // Any action that is considered as user starting an iteraction resets any ongoing counter
+        resetAllCountersOnInteractionStarted()
         analytics.track(.pointOfSaleInteractionWithCustomerStarted)
         customerInteractionStarted = Date().timeIntervalSince1970
     }
@@ -73,8 +79,13 @@ final class POSCollectOrderPaymentAnalytics: CollectOrderPaymentAnalyticsTrackin
         trackElapsedTimeFromOrderCreationToCardReady()
     }
 
+    // The Stripe SDK returns multiple `.processing` events, but we want to capture the first one in the stream only.
+    // This flag is reset as soon as the payment has been successful
     func trackCardReaderTapped() {
-        cardReaderTapped = trackCurrentTime()
+        if !hasTrackedProcessingPayment {
+            hasTrackedProcessingPayment = true
+            cardReaderTapped = trackCurrentTime()
+        }
     }
 
     func trackCheckoutTapped() {
@@ -101,6 +112,18 @@ private extension POSCollectOrderPaymentAnalytics {
         let end = Date().timeIntervalSince1970
         return floor((end - start) * 1000)
     }
+
+    private func resetProcessingPaymentTracking() {
+        hasTrackedProcessingPayment = false
+    }
+
+    private func resetAllCountersOnInteractionStarted() {
+        orderCreated = 0
+        cardReaderReady = 0
+        cardReaderTapped = 0
+        resetCheckoutTapCountTracker()
+        resetProcessingPaymentTracking()
+    }
 }
 
 // Protocol conformance. These events are not needed for IPP, only for POS.
diff --git a/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift b/WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift
@@ -13,10 +13,21 @@ import class WooFoundation.CurrencySettings
 import enum WooFoundation.CurrencyCode
 import protocol WooFoundation.Analytics
 
+enum SyncOrderState {
+    case newOrder
+    case orderUpdated
+    case orderNotChanged
+}
+
+enum SyncOrderStateError: Error {
+    case syncFailure
+}
+
 protocol PointOfSaleOrderControllerProtocol {
     var orderState: PointOfSaleInternalOrderState { get }
 
-    func syncOrder(for cartProducts: [CartItem], retryHandler: @escaping () async -> Void) async
+    @discardableResult
+    func syncOrder(for cartProducts: [CartItem], retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error>
     func sendReceipt(recipientEmail: String) async throws
     func clearOrder()
     func collectCashPayment() async throws
@@ -51,16 +62,15 @@ protocol PointOfSaleOrderControllerProtocol {
     private(set) var orderState: PointOfSaleInternalOrderState = .idle
     private var order: Order? = nil
 
-    @MainActor
+    @MainActor @discardableResult
     func syncOrder(for cartItems: [CartItem],
-                   retryHandler: @escaping () async -> Void) async {
+                   retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error> {
         let posCartItems = cartItems.map {
             POSCartItem(item: $0.item, quantity: Decimal($0.quantity))
         }
 
-        guard !orderState.isSyncing,
-              !posCartItems.matches(order: order) else {
-            return
+        guard !orderState.isSyncing, !posCartItems.matches(order: order) else {
+            return .success(.orderNotChanged)
         }
 
         orderState = .syncing
@@ -74,6 +84,9 @@ protocol PointOfSaleOrderControllerProtocol {
             orderState = .loaded(totals(for: syncedOrder), syncedOrder)
             if isNewOrder {
                 analytics.track(.orderCreationSuccess)
+                return .success(.newOrder)
+            } else {
+                return .success(.orderUpdated)
             }
         } catch {
             if isNewOrder {
@@ -83,6 +96,7 @@ protocol PointOfSaleOrderControllerProtocol {
                     errorDescription: error.localizedDescription))
             }
             setOrderStateToError(error, retryHandler: retryHandler)
+            return .failure(SyncOrderStateError.syncFailure)
         }
     }
 
diff --git a/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift b/WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift
@@ -227,7 +227,6 @@ extension PointOfSaleAggregateModel {
         }
         do {
             try await collectPayment(for: order)
-            collectOrderPaymentAnalyticsTracker.resetCheckoutTapCountTracker()
         } catch {
             DDLogError("Error taking payment: \(error)")
         }
@@ -253,6 +252,7 @@ extension PointOfSaleAggregateModel {
     private func cashPaymentSuccess() {
         paymentState = .cash(.paymentSuccess)
         collectOrderPaymentAnalyticsTracker.trackSuccessfulCashPayment()
+        // TODO: Move to trackSuccessfulCashPayment() on #15151
         collectOrderPaymentAnalyticsTracker.resetCheckoutTapCountTracker()
     }
 
@@ -452,10 +452,12 @@ extension PointOfSaleAggregateModel {
     func checkOut() async {
         collectOrderPaymentAnalyticsTracker.trackCheckoutTapped()
         orderStage = .finalizing
-        await orderController.syncOrder(for: cart, retryHandler: { [weak self] in
+        let syncOrderResult = await orderController.syncOrder(for: cart, retryHandler: { [weak self] in
             await self?.checkOut()
         })
-        collectOrderPaymentAnalyticsTracker.trackOrderCreationSuccess()
+        if case .success(.newOrder) = syncOrderResult {
+            collectOrderPaymentAnalyticsTracker.trackOrderCreationSuccess()
+        }
         await startPaymentWhenCardReaderConnected()
     }
 }
diff --git a/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift b/WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift
@@ -12,8 +12,9 @@ class PointOfSalePreviewOrderController: PointOfSaleOrderControllerProtocol {
         OrderFactory.newOrder(currency: .USD)
     )
 
-    func syncOrder(for cartProducts: [CartItem],
-                   retryHandler: @escaping () async -> Void) async { }
+    func syncOrder(for cartProducts: [CartItem], retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error> {
+        return .success(.newOrder)
+    }
 
     func sendReceipt(recipientEmail: String) async throws { }
 
diff --git a/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift b/WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift
@@ -276,6 +276,94 @@ struct PointOfSaleOrderControllerTests {
         #expect(mockPaymentCelebration.celebrationWasCalled == true)
     }
 
+    @available(iOS 17.0, *)
+    @Test func syncOrder_when_successful_returns_newOrder_result() async throws {
+        // Given
+        let sut = PointOfSaleOrderController(orderService: mockOrderService,
+                                             receiptService: mockReceiptService)
+        let fakeOrderItem = OrderItem.fake().copy(quantity: 1)
+        let fakeOrder = Order.fake()
+        let fakeCartItem = makeItem(orderItemsToMatch: [fakeOrderItem])
+        mockOrderService.orderToReturn = fakeOrder
+
+        // When
+        let result = await sut.syncOrder(for: [fakeCartItem], retryHandler: { })
+
+        // Then
+        if case .success(let state) = result {
+            #expect(state == .newOrder)
+        } else {
+            #expect(Bool(false), "Expected success result with new order")
+        }
+    }
+
+    @available(iOS 17.0, *)
+    @Test func syncOrder_when_updating_existing_order_returns_orderUpdated_result() async throws {
+        // Given
+        let sut = PointOfSaleOrderController(orderService: mockOrderService,
+                                             receiptService: mockReceiptService)
+        let fakeOrderItem = OrderItem.fake().copy(quantity: 1)
+        let fakeOrder = Order.fake()
+        mockOrderService.orderToReturn = fakeOrder
+
+        // When
+        // 1. Initial order
+        _ = await sut.syncOrder(for: [makeItem()], retryHandler: {})
+
+        // 2. Sync existing order
+        let result = await sut.syncOrder(for: [makeItem(), makeItem()], retryHandler: {})
+
+        // Then
+        if case .success(let state) = result {
+            #expect(state == .orderUpdated)
+        } else {
+            #expect(Bool(false), "Expected success result with order updated")
+        }
+    }
+
+    @available(iOS 17.0, *)
+    @Test func syncOrder_when_cart_matching_order_then_returns_orderNotChanged_result() async throws {
+        // Given
+        let sut = PointOfSaleOrderController(orderService: mockOrderService,
+                                             receiptService: mockReceiptService)
+        let orderItem = OrderItem.fake().copy(quantity: 1)
+        let fakeOrder = Order.fake().copy(items: [orderItem])
+        let cartItem = makeItem(orderItemsToMatch: [orderItem])
+        mockOrderService.orderToReturn = fakeOrder
+
+        // When
+        // 1. Initial order
+        _ = await sut.syncOrder(for: [cartItem], retryHandler: {})
+
+        // 2. Syncing existing order with same cart should not update order
+        let result = await sut.syncOrder(for: [cartItem], retryHandler: {})
+
+        // Then
+        if case .success(let state) = result {
+            #expect(state == .orderNotChanged)
+        } else {
+            #expect(Bool(false), "Expected success result with no changes to existing order")
+        }
+    }
+
+    @available(iOS 17.0, *)
+    @Test func syncOrder_when_orderService_fails_then_returns_syncOrderState_failure() async throws {
+        let sut = PointOfSaleOrderController(orderService: mockOrderService,
+                                             receiptService: mockReceiptService)
+        let cartItem = makeItem(quantity: 1)
+
+        // When
+        mockOrderService.orderToReturn = nil
+        let result = await sut.syncOrder(for: [cartItem], retryHandler: {})
+
+        // Then
+        if case .failure(let error) = result {
+            #expect(error as? SyncOrderStateError == .syncFailure)
+        } else {
+            #expect(Bool(false), "Expected sync failure but got \(result)")
+        }
+    }
+
     struct AnalyticsTests {
         private let analytics: WooAnalytics
         private let analyticsProvider = MockAnalyticsProvider()
diff --git a/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift b/WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift
@@ -18,18 +18,22 @@ final class MockPointOfSaleOrderController: PointOfSaleOrderControllerProtocol {
     var syncOrderWasCalled: Bool = false
     var spyCartProducts: [CartItem]?
     var spyRetryHandler: (() async -> Void)?
+    var syncOrderResultToReturn: Result<SyncOrderState, Error> = .success(.newOrder)
+
+    @discardableResult
     func syncOrder(for cartProducts: [CartItem],
-                   retryHandler: @escaping () async -> Void) async {
+                   retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error> {
         syncOrderWasCalled = true
         spyCartProducts = cartProducts
         spyRetryHandler = retryHandler
 
         guard let orderStateToReturn else {
             orderState = .syncing
-            return
+            return syncOrderResultToReturn
         }
 
         orderState = orderStateToReturn
+        return syncOrderResultToReturn
     }
 
     var clearOrderWasCalled: Bool = false

Original file line number	Diff line number	Diff line change
`@@ -227,7 +227,6 @@ extension PointOfSaleAggregateModel {`
`227`	`227`	`}`
`228`	`228`	`do {`
`229`	`229`	`try await collectPayment(for: order)`
`230`		`- collectOrderPaymentAnalyticsTracker.resetCheckoutTapCountTracker()`
`231`	`230`	`} catch {`
`232`	`231`	`DDLogError("Error taking payment: \(error)")`
`233`	`232`	`}`
`@@ -253,6 +252,7 @@ extension PointOfSaleAggregateModel {`
`253`	`252`	`private func cashPaymentSuccess() {`
`254`	`253`	`paymentState = .cash(.paymentSuccess)`
`255`	`254`	`collectOrderPaymentAnalyticsTracker.trackSuccessfulCashPayment()`
	`255`	`+ // TODO: Move to trackSuccessfulCashPayment() on #15151`
`256`	`256`	`collectOrderPaymentAnalyticsTracker.resetCheckoutTapCountTracker()`
`257`	`257`	`}`
`258`	`258`
`@@ -452,10 +452,12 @@ extension PointOfSaleAggregateModel {`
`452`	`452`	`func checkOut() async {`
`453`	`453`	`collectOrderPaymentAnalyticsTracker.trackCheckoutTapped()`
`454`	`454`	`orderStage = .finalizing`
`455`		`- await orderController.syncOrder(for: cart, retryHandler: { [weak self] in`
	`455`	`+ let syncOrderResult = await orderController.syncOrder(for: cart, retryHandler: { [weak self] in`
`456`	`456`	`await self?.checkOut()`
`457`	`457`	`})`
`458`		`- collectOrderPaymentAnalyticsTracker.trackOrderCreationSuccess()`
	`458`	`+ if case .success(.newOrder) = syncOrderResult {`
	`459`	`+ collectOrderPaymentAnalyticsTracker.trackOrderCreationSuccess()`
	`460`	`+ }`
`459`	`461`	`await startPaymentWhenCardReaderConnected()`
`460`	`462`	`}`
`461`	`463`	`}`
Original file line number	Diff line number	Diff line change
`@@ -12,8 +12,9 @@ class PointOfSalePreviewOrderController: PointOfSaleOrderControllerProtocol {`
`12`	`12`	`OrderFactory.newOrder(currency: .USD)`
`13`	`13`	`)`
`14`	`14`
`15`		`- func syncOrder(for cartProducts: [CartItem],`
`16`		`- retryHandler: @escaping () async -> Void) async { }`
	`15`	`+ func syncOrder(for cartProducts: [CartItem], retryHandler: @escaping () async -> Void) async -> Result<SyncOrderState, Error> {`
	`16`	`+ return .success(.newOrder)`
	`17`	`+ }`
`17`	`18`
`18`	`19`	`func sendReceipt(recipientEmail: String) async throws { }`
`19`	`20`