Skip to content

[Woo POS] Coupons: Disallow adding duplicate coupons to cart #15551

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
May 6, 2025
Merged

[Woo POS] Coupons: Disallow adding duplicate coupons to cart #15551

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
11913c3
Return early if same coupon ID is already in cart
iamgabrielma Apr 25, 2025
b3a53cb
extract sut to be reusable within cart struct
iamgabrielma Apr 25, 2025
60de14f
Disallow tracking add to cart event when is disallowed
iamgabrielma Apr 25, 2025
72e42f7
lint
iamgabrielma Apr 25, 2025
6868fe0
Merge branch 'trunk' into task/woomob-259-disallow-adding-same-coupon…
iamgabrielma May 2, 2025
ff28aea
Delete unused makeSUT
iamgabrielma May 2, 2025
936f2b5
restore using analytics shared dependency for CartTests
iamgabrielma May 2, 2025
3bfe659
Remove unused CouponsItemActionHandler
iamgabrielma May 6, 2025
68096bc
remove early return duplication
iamgabrielma May 6, 2025
3c28116
Add POSItemActionHandlerTests
iamgabrielma May 6, 2025
a7c5977
extract MockPOSSearchHistoryService
iamgabrielma May 6, 2025
d004acc
use specific handler type when testing search
iamgabrielma May 6, 2025
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
4 changes: 3 additions & 1 deletion WooCommerce/Classes/POS/Models/Cart.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ extension Cart {
case .variableParentProduct:
return
case .coupon(let coupon):
let couponItem = Cart.CouponItem(id: UUID(), code: coupon.code, summary: coupon.summary)
guard !coupons.contains(where: { $0.id == coupon.id }) else { return }
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 this check here if we skip the duplicate in the action handler?

Copy link
Contributor Author

@iamgabrielma iamgabrielma May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We don't anymore, I've removed this on 68096bc and added initial tests for the action handler on a7c5977


let couponItem = Cart.CouponItem(id: coupon.id, code: coupon.code, summary: coupon.summary)
coupons.insert(couponItem, at: coupons.startIndex)
}
}
Expand Down
22 changes: 22 additions & 0 deletions WooCommerce/Classes/POS/Presentation/Item Selector/POSItemActionHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,28 @@ final class StandardPOSItemActionHandler: POSItemActionHandler {
}
}

@available(iOS 17.0, *)
final class CouponsItemActionHandler: POSItemActionHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer needed, can be removed 👍

Copy link
Contributor Author

@iamgabrielma iamgabrielma May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I didn't see that we stopped using CouponsItemActionHandler since I worked on this bit. Removing it introduces a regression thought, as we treat products and coupons equally when dealing with track events, meaning: We will track the add_to_cart event despite coupons not being added to cart when are duplicated:

🔵 Tracked pos_coupon_added_to_cart, properties: [site_url: https://indiemelon.mystagingwebsite.com, blog_id: -1, was_ecommerce_trial: false, plan: , store_id: c5bd46cc-1804-4f7b-badb-bb98c449127f, is_wpcom_store: false]
🔵 Tracked pos_coupon_added_to_cart, properties: [store_id: c5bd46cc-1804-4f7b-badb-bb98c449127f, plan: , is_wpcom_store: false, blog_id: -1, was_ecommerce_trial: false, site_url: https://indiemelon.mystagingwebsite.com]

I've added a small helper function to handle this case in POSItemActionHandler, as this will happen both when we add coupons via list and via search.

I'll leave this one without merging for now in case you want to take a look, in a nutshell when adding coupons from either list or search, the track event should be logged just once.

private let posModel: PointOfSaleAggregateModelProtocol
private let analytics: Analytics
private let itemListType: ItemListType

init(posModel: PointOfSaleAggregateModelProtocol, itemListType: ItemListType, analytics: Analytics = ServiceLocator.analytics) {
self.posModel = posModel
self.itemListType = itemListType
self.analytics = analytics
}

func handleTap(_ item: POSItem) {
let alreadyExists = posModel.cart.coupons.contains(where: { $0.id == item.id })
if alreadyExists {
return
}
posModel.addToCart(item)
trackTapAnalytics(for: item, itemListType: itemListType, using: analytics)
}
}

/// Handler for handling taps on search result items, saving the search term
@available(iOS 17.0, *)
final class SearchResultItemActionHandler: POSItemActionHandler {
Expand Down
17 changes: 5 additions & 12 deletions WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,14 +243,7 @@ struct PointOfSaleAggregateModelTests {
}

@available(iOS 17.0, *)
@Test(.disabled(
"""
This test doesn't currently work; analytics extensions are not thread-safe,
and using the MainActor means the assert happens too early. I don't think
we want the addToCart to be async, but that would be one way to fix it.
"""))
func addToCart_tracks_analytics_event() async throws {
// Given
@Test func addToCart_when_attempt_to_add_duplicated_coupon_then_does_not_add_it_to_cart() {
let sut = PointOfSaleAggregateModel(itemsController: MockPointOfSaleItemsController(),
purchasableItemsSearchController: MockPointOfSalePurchasableItemsSearchController(),
couponsController: MockPointOfSaleCouponsController(),
Expand All @@ -261,14 +254,14 @@ struct PointOfSaleAggregateModelTests {
collectOrderPaymentAnalyticsTracker: MockPOSCollectOrderPaymentAnalyticsTracker(),
searchHistoryService: MockPOSSearchHistoryService(),
popularItemsController: MockPointOfSalePopularItemsController())
let item = makePurchasableItem()
let coupon = makeCouponItem(code: "DISCOUNT!")

// When
sut.addToCart(item)
sut.addToCart(coupon)
sut.addToCart(coupon)

// Then
let event = try #require(analyticsProvider.receivedEvents.first)
#expect(event == "item_added_to_cart")
#expect(sut.cart.coupons.count == 1)
}
}

Expand Down