Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
22.1
-----
- [*] Order Creation: Fixed an issue where order recalculation would stop working after canceling a confirmation with unsaved changes [https://github.com/woocommerce/woocommerce-ios/pull/15392].
- [internal] POS: Always create a new order during the checkout [https://github.com/woocommerce/woocommerce-ios/pull/15427].
- [internal] Improve data fetching in Order Details, to avoid I/O performance on the main thread. [https://github.com/woocommerce/woocommerce-ios/pull/14999]

22.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import protocol WooFoundation.Analytics

enum SyncOrderState {
case newOrder
case orderUpdated
case orderNotChanged
}

Expand Down Expand Up @@ -75,27 +74,20 @@ protocol PointOfSaleOrderControllerProtocol {
}

orderState = .syncing
let isNewOrder = order == nil

do {
let syncedOrder = try await orderService.syncOrder(cart: posCart,
order: order,
currency: storeCurrency)
self.order = syncedOrder
orderState = .loaded(totals(for: syncedOrder), syncedOrder)
if isNewOrder {
analytics.track(.orderCreationSuccess)
return .success(.newOrder)
} else {
return .success(.orderUpdated)
}
analytics.track(.orderCreationSuccess)
return .success(.newOrder)
} catch {
if isNewOrder {
analytics.track(event: WooAnalyticsEvent.Orders.orderCreationFailed(
usesGiftCard: false,
errorContext: String(describing: error),
errorDescription: error.localizedDescription))
}
self.order = nil
analytics.track(event: WooAnalyticsEvent.Orders.orderCreationFailed(
usesGiftCard: false,
errorContext: String(describing: error),
errorDescription: error.localizedDescription))
setOrderStateToError(error, retryHandler: retryHandler)
return .failure(SyncOrderStateError.syncFailure)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,13 @@ private extension PointOfSaleAggregateModel {
}
}

// Tracks when the order is created or updated successfully
// Tracks when the order is created successfully
// pdfdoF-6hn#comment-7625-p2
func trackOrderSyncState(_ result: Result<SyncOrderState, Error>) {
switch result {
case .success(let syncState):
switch syncState {
case .newOrder, .orderUpdated:
case .newOrder:
collectOrderPaymentAnalyticsTracker.trackOrderSyncSuccess()
default:
break
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,11 +299,10 @@ struct PointOfSaleOrderControllerTests {
}

@available(iOS 17.0, *)
@Test func syncOrder_when_updating_existing_order_returns_orderUpdated_result() async throws {
@Test func syncOrder_when_updating_existing_order_returns_newOrder_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

Expand All @@ -316,9 +315,9 @@ struct PointOfSaleOrderControllerTests {

// Then
if case .success(let state) = result {
#expect(state == .orderUpdated)
#expect(state == .newOrder)
} else {
#expect(Bool(false), "Expected success result with order updated")
#expect(Bool(false), "Expected success result with new order")
}
}

Expand Down Expand Up @@ -524,6 +523,51 @@ struct PointOfSaleOrderControllerTests {
])
}

@available(iOS 17.0, *)
@Test func syncOrder_when_fails_sets_order_to_nil() async throws {
// Given
let sut = PointOfSaleOrderController(orderService: mockOrderService,
receiptService: mockReceiptService)

// First create a successful order
let orderItem = OrderItem.fake().copy(quantity: 1)
let fakeOrder = Order.fake().copy(items: [orderItem])
let cartItem = makeItem(orderItemsToMatch: [orderItem])
mockOrderService.orderToReturn = fakeOrder

// Initial sync succeeds
let initialResult = await sut.syncOrder(for: .init(items: [cartItem]), retryHandler: {})
switch initialResult {
case .success(.newOrder):
break
default:
#expect(Bool(false), "Expected success with new order, got \(initialResult)")
}

// Then simulate a failure
mockOrderService.errorToReturn = SyncOrderStateError.syncFailure
let failureResult = await sut.syncOrder(for: .init(items: [cartItem, cartItem]), retryHandler: {})
switch failureResult {
case .failure(SyncOrderStateError.syncFailure):
break
default:
#expect(Bool(false), "Expected sync failure, got \(failureResult)")
}

// When - try syncing with the same cart again
mockOrderService.errorToReturn = nil
mockOrderService.orderToReturn = fakeOrder // Restore mock to return success
let subsequentResult = await sut.syncOrder(for: .init(items: [cartItem]), retryHandler: {})

// Then - should be treated as new order since previous order was cleared
switch subsequentResult {
case .success(.newOrder):
break
default:
#expect(Bool(false), "Expected new order after failure cleared previous order, got \(subsequentResult)")
}
}

struct AnalyticsTests {
private let analytics: WooAnalytics
private let analyticsProvider = MockAnalyticsProvider()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ class MockPOSOrderService: POSOrderServiceProtocol {
var spySyncOrderCurrency: CurrencyCode?

func syncOrder(cart: Yosemite.POSCart,
order: Yosemite.Order?,
currency: CurrencyCode) async throws -> Yosemite.Order {
syncOrderWasCalled = true
spySyncOrderCurrency = currency
Expand Down
92 changes: 16 additions & 76 deletions Yosemite/Yosemite/Tools/POS/POSOrderService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ public protocol POSOrderServiceProtocol {
/// Syncs order based on the cart.
/// - Parameters:
/// - cart: Cart with different types of items and quantities.
/// - order: Optional latest remotely synced order. Nil when syncing order for the first time.
/// - Returns: Order from the remote sync.
func syncOrder(cart: POSCart, order: Order?, currency: CurrencyCode) async throws -> Order
func syncOrder(cart: POSCart, currency: CurrencyCode) async throws -> Order
func updatePOSOrder(order: Order, recipientEmail: String) async throws
}

Expand All @@ -36,19 +35,14 @@ public final class POSOrderService: POSOrderServiceProtocol {
// MARK: - Protocol conformance

public func syncOrder(cart: POSCart,
order posOrder: Order?,
currency: CurrencyCode) async throws -> Order {
let initialOrder: Order = posOrder ?? OrderFactory.newOrder(currency: currency)
.copy(siteID: siteID,
status: .autoDraft)
let order = updateOrder(initialOrder, cart: cart).sanitizingLocalItems()
let syncedOrder: Order
if posOrder != nil {
syncedOrder = try await ordersRemote.updatePOSOrder(siteID: siteID, order: order, fields: [.items, .couponLines])
} else {
syncedOrder = try await ordersRemote.createPOSOrder(siteID: siteID, order: order, fields: [.items, .status, .currency, .couponLines])
}
return syncedOrder
let order = OrderFactory
.newOrder(currency: currency)
.copy(siteID: siteID, status: .autoDraft)
.addItems(cart.items)
.addCoupons(cart.coupons)

return try await ordersRemote.createPOSOrder(siteID: siteID, order: order, fields: [.items, .status, .currency, .couponLines])
}

public func updatePOSOrder(order: Order, recipientEmail: String) async throws {
Expand All @@ -66,73 +60,19 @@ public final class POSOrderService: POSOrderServiceProtocol {
}
}

private struct POSOrderSyncProductType: OrderSyncProductTypeProtocol, Hashable {
let productID: Int64
let price: String
// Not used in POS but have to be included for the app usage.
let productType: ProductType
let bundledItems: [ProductBundleItem]

init(productID: Int64, price: String, productType: ProductType, bundledItems: [ProductBundleItem] = []) {
self.productID = productID
self.price = price
self.productType = productType
self.bundledItems = bundledItems
}
}

private extension POSOrderService {
func updateOrder(_ order: Order, cart: POSCart) -> Order {
// Removes all existing items by setting quantity to 0.
let itemsToRemove = order.items.compactMap {
Self.removalProductInput(item: $0)
}

// Adds items from the latest cart grouping by item.
let itemsToAdd = cart.items.createGroupedOrderSyncProductInputs().values
let itemsToSync = itemsToRemove + itemsToAdd

var order = ProductInputTransformer.updateMultipleItems(with: itemsToSync, on: order, shouldUpdateOrDeleteZeroQuantities: .update)
order = updateCoupons(cart.coupons, on: order)

return order
private extension Order {
func addItems(_ cartItems: [POSCartItem]) -> Order {
let itemsToAdd = Array(cartItems.createGroupedOrderSyncProductInputs().values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to call sanitizingLocalItems on these, or is that only relevant for updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨 I missed it.

Great observation. We do need it. As the comment in the sanitizingLocalItems says, not applying it breaks the calculations if prices_include_tax is true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, glad I spotted it then! I wasn't sure because it could have been something only required on update... though, our products are all added as an update, aren't they? Rather than in the initial POST request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though, our products are all added as an update, aren't they? Rather than in the initial POST request?

No. We add them as initial request when creating an order. createGroupedOrderSyncProductInputs turns products into order items. sanitize makes sure totals and subtotals are cleared and don't influence backend calculations.

Copy link
Contributor

@joshheald joshheald Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense! Yeah, I'm all for just sending up an ID and a quantity as our input. Factors in to the line/cart-level discount/taxes discussion as well.

Ideally, if we ever allow merchants to add line-level discounts like they can in IPP, we could make a change in the API that lets us specify a discount directly, and whether that's before or after tax.

return ProductInputTransformer
.updateMultipleItems(with: itemsToAdd, on: self, shouldUpdateOrDeleteZeroQuantities: .update)
.sanitizingLocalItems()
}

func updateCoupons(_ coupons: [POSCoupon], on order: Order) -> Order {
// Get coupon codes from cart
let cartCouponCodes = Set(coupons.map { $0.code })

// Keep existing coupons that are still in the cart
let remainingCoupons = order.coupons.filter { orderCoupon in
cartCouponCodes.contains(orderCoupon.code)
}

// Find new coupons that need to be added (in cart but not in order)
let existingCouponCodes = Set(order.coupons.map { $0.code })
func addCoupons(_ coupons: [POSCoupon]) -> Order {
let newCoupons = coupons
.filter { !existingCouponCodes.contains($0.code) }
.map { OrderFactory.newOrderCouponLine(code: $0.code) }

// Update order with remaining + new coupons
return order.copy(coupons: remainingCoupons + newCoupons)
}

/// Creates a new `OrderSyncProductInput` type meant to remove an existing item from `OrderSynchronizer`
///
static func removalProductInput(item: OrderItem) -> OrderSyncProductInput? {
let productForRemoval = POSProductForRemoval(productID: item.productID)
// Return a new input with the new quantity but with the same item id to properly reference the update.
return OrderSyncProductInput(id: item.itemID,
product: .product(productForRemoval),
quantity: 0)
}

/// A simplified product struct, intended to contain only the `productID`
struct POSProductForRemoval: OrderSyncProductTypeProtocol {
var price: String = ""
var productID: Int64
var productType: ProductType = .simple
var bundledItems: [ProductBundleItem] = []
return self.copy(coupons: newCoupons)
}
}

Expand Down
Loading