Skip to content
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

[Woo POS] Always create a new order when checking out #15427

Merged
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -5,6 +5,7 @@
-----

- [*] 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].

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,19 @@ 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))
}
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
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
90 changes: 14 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,17 @@ 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)
}

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