[Woo POS] Allow item list switch between Products and Coupons #15366
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request disables the coupons feature flag by hardcoding its value to false. It adds functionality to toggle between product and coupon item types by introducing a new enum and asynchronous methods in the point-of-sale controllers and aggregate model. The UI is updated with a new SwiftUI view for displaying coupon details and a toggle interface in the item list view. Additionally, several data-fetching methods have been updated to return paged results and to throw errors, and corresponding test mocks and service protocols have been modified accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ILV as ItemListView
participant PM as PointOfSaleAggregateModel
participant PIC as PointOfSaleItemsController
participant DS as DataService
U->>ILV: Tap "Products"/"Coupons" button
ILV->>PM: toggleItemType()
PM->>PIC: toggleItemType()
PIC->>PIC: Toggle itemType (.products ↔ .coupons)
PIC->>DS: fetchItems(pageNumber, based on itemType)
DS-->>PIC: Return paged items
PIC-->>PM: Updated item list
PM-->>ILV: Refresh UI with new items
sequenceDiagram
participant U as User
participant IL as ItemList
participant PM as PointOfSaleAggregateModel
participant Cart as CartManager
U->>IL: Tap coupon button (CouponCardView)
IL->>PM: addToCart(coupon)
PM->>Cart: Process coupon addition to cart
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
|
Fetching from storage can fail, also we can reuse the error handling from items controller
Now that we have dummy UI updates we should keep the flag off in trunk
iamgabrielma
added
feature: coupons
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
WooCommerce/Classes/POS/Presentation/Item Selector/CouponCardView.swift (1)
4-17: Simple CouponCardView implementation needs enhancementThe implementation is functional but quite basic. Consider the following improvements:
- Add proper styling to match the app's design language
- Display more user-friendly coupon information (e.g., code, amount, description) rather than just technical IDs
- Include accessibility labels and hints for VoiceOver support
struct CouponCardView: View { private let coupon: POSCoupon init(coupon: POSCoupon) { self.coupon = coupon } var body: some View { - HStack { - Text(coupon.id.uuidString) - Text(coupon.couponID.description) - } + VStack(alignment: .leading, spacing: 4) { + Text(coupon.code ?? "Coupon") + .font(.headline) + HStack { + Text("Discount: ") + .font(.subheadline) + .foregroundColor(.secondary) + Text(coupon.formattedAmount ?? "Unknown amount") + .font(.subheadline) + } + if let description = coupon.description, !description.isEmpty { + Text(description) + .font(.caption) + .foregroundColor(.secondary) + .lineLimit(2) + } + } + .padding() + .accessibilityLabel("\(coupon.code ?? "Coupon"), \(coupon.formattedAmount ?? "Unknown amount")") } }WooCommerce/Classes/POS/Presentation/ItemListView.swift (1)
163-167: Function implementation is correct but lacks state tracking.The function correctly calls the model's
toggleItemType()method asynchronously, but there's no way to track or update the UI state based on the current item type.Consider enhancing the implementation to track the current state:
func toggleItemType() { Task { + // Update local state before or after the toggle + currentItemType = currentItemType == .product ? .coupon : .product await posModel.toggleItemType() } }Or better yet, retrieve the current state from the model:
func toggleItemType() { Task { await posModel.toggleItemType() + // Update local state after the toggle + currentItemType = await posModel.itemsController.currentItemType } }WooCommerce/Classes/POS/Presentation/Item Selector/ItemList.swift (1)
119-124:❓ Verification inconclusive
Good implementation for coupon handling, but missing analytics tracking.
The implementation allows users to add coupons to the cart, but unlike the product and variation cases, it doesn't track this action for analytics.
case let .coupon(coupon): Button(action: { posModel.addToCart(item) + analytics.track(event: .PointOfSale.addItemToCart(type: .coupon)) }, label: { CouponCardView(coupon: coupon) })Make sure the
PointOfSale.addItemToCartevent supports a.coupontype.
🏁 Script executed:
#!/bin/bash # Check if the addItemToCart analytics event supports coupon type rg "addItemToCart.*coupon" --type swiftLength of output: 41
Attention: Missing Analytics Tracking for Coupons
The coupon-handling code correctly adds the coupon to the cart, but it lacks the corresponding analytics tracking call. Please add the following tracking call after adding the coupon to the cart:
case let .coupon(coupon): Button(action: { posModel.addToCart(item) + analytics.track(event: .PointOfSale.addItemToCart(type: .coupon)) }, label: { CouponCardView(coupon: coupon) })Before merging, verify manually that the
PointOfSale.addItemToCartevent in the analytics system supports a.coupontype. Our automated search did not return conclusive evidence of coupon support, so please review the analytics event definitions to ensure compatibility.WooCommerce/Classes/POS/Utils/PreviewHelpers.swift (1)
94-97: Empty implementation of toggleItemType()While the method satisfies the protocol requirement, it currently lacks implementation. This is acceptable for preview purposes but consider adding a basic implementation to toggle between mock products and coupons for more comprehensive previews.
func toggleItemType() async { - // + // For preview purposes, we could toggle between mock products and coupons + // by updating the itemsViewState with different mock data }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Experiments/Experiments/DefaultFeatureFlagService.swift(1 hunks)WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift(5 hunks)WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift(2 hunks)WooCommerce/Classes/POS/Presentation/Item Selector/CouponCardView.swift(1 hunks)WooCommerce/Classes/POS/Presentation/Item Selector/ItemList.swift(1 hunks)WooCommerce/Classes/POS/Presentation/ItemListView.swift(4 hunks)WooCommerce/Classes/POS/Utils/PreviewHelpers.swift(2 hunks)WooCommerce/WooCommerce.xcodeproj/project.pbxproj(4 hunks)WooCommerce/WooCommerceTests/POS/Mocks/MockPOSItemProvider.swift(1 hunks)Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift(3 hunks)Yosemite/Yosemite/PointOfSale/PointOfSaleItemServiceProtocol.swift(1 hunks)
🔇 Additional comments (18)
Experiments/Experiments/DefaultFeatureFlagService.swift (1)
75-75: Temporary Feature Flag Disablement ConfirmedYou've hardcoded the
enableCouponsInPointOfSaleflag tofalseas mentioned in the PR description. This effectively prevents the new coupon functionality from being accessible in all build configurations during development.Remember to revisit this when the coupon feature is ready for testing, so it can be conditionally enabled again.
WooCommerce/Classes/POS/Presentation/Item Selector/CouponCardView.swift (1)
13-14:❓ Verification inconclusive
Replace raw identifiers with user-friendly information
Displaying raw UUIDs and numerical IDs doesn't provide a good user experience. Users need to see meaningful information like coupon code, discount amount, and expiration date.
- Text(coupon.id.uuidString) - Text(coupon.couponID.description) + Text(coupon.code ?? "Coupon") + Text(coupon.formattedAmount ?? "")
🏁 Script executed:
#!/bin/bash # Check what properties POSCoupon has that would be more useful to display grep -A 20 "struct POSCoupon" $(find . -name "*.swift" | grep -v "build" | grep -v "DerivedData")Length of output: 69698
Action Required: Verify Display Fields for Coupon Information
The updated code replaces raw identifiers with more user-friendly properties. However, our verification script did not yield output for the
POSCouponstructure, so it's unclear ifcoupon.codeandcoupon.formattedAmountare valid properties to display. Please manually verify that the coupon model exposes these properties (or suitable alternatives like coupon code, discount amount, or expiration date) before merging.
- Confirm that the
couponinstance has acodeproperty for displaying the coupon code.- Confirm that a property like
formattedAmount(or another equivalent) is available for rendering the discount amount or other meaningful details.- If these properties are not present, update the diff accordingly with the correct user-friendly fields.
- Text(coupon.id.uuidString) - Text(coupon.couponID.description) + Text(coupon.code ?? "Coupon") + Text(coupon.formattedAmount ?? "")WooCommerce/WooCommerce.xcodeproj/project.pbxproj (4)
1593-1593: LGTM: New file added to build sources.The file reference for
CouponCardView.swifthas been properly added to the build sources.
4756-4756: LGTM: File reference properly configured.The file reference for
CouponCardView.swiftis correctly set up with the proper file type and path.
7518-7518: LGTM: File correctly added to Item Selector group.
CouponCardView.swifthas been properly added to the Item Selector group, which aligns with the PR objective of allowing toggling between products and coupons.
16212-16212: LGTM: Build phase entry correct.The file has been correctly added to the compilation phase.
WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift (1)
95-98: Clean implementation of the toggle functionality.The
@MainActorattribute is correctly applied to ensure UI updates happen on the main thread. This implementation follows the established pattern in this file for other asynchronous methods.Yosemite/Yosemite/PointOfSale/PointOfSaleItemServiceProtocol.swift (1)
61-61:✅ Verification successful
Good change to the return type for better consistency.
Changing the return type from
[POSItem]toPagedItems<POSItem>aligns with other methods in this protocol. Adding error handling withthrowsis also a good practice.Make sure all implementations of this protocol have been updated to match this new signature.
🏁 Script executed:
#!/bin/bash # Check for all implementations of providePointOfSaleCoupons rg "func providePointOfSaleCoupons\(\)" --type swiftLength of output: 577
Review Verified: All implementations match the updated signature.
The changes in the return type and error handling have been confirmed across the codebase:
Yosemite/Yosemite/PointOfSale/PointOfSaleItemServiceProtocol.swiftYosemite/Yosemite/PointOfSale/PointOfSaleItemService.swiftWooCommerce/WooCommerceTests/POS/Mocks/MockPOSItemProvider.swiftWooCommerce/Classes/POS/Utils/PreviewHelpers.swiftThe method signature
func providePointOfSaleCoupons() throws -> PagedItems<POSItem>is consistently applied, and no further changes are needed.WooCommerce/Classes/POS/Presentation/ItemListView.swift (2)
4-4: Appropriate import for the new feature.The import of
Yosemite.POSCouponis necessary for the coupon functionality being added.
20-22: Good use of feature flag to conditionally enable the coupons feature.This approach allows for controlled rollout of the feature and easy disabling if issues are found.
WooCommerce/Classes/POS/Utils/PreviewHelpers.swift (1)
60-61: Method signature update looks goodThe update to return
PagedItems<POSItem>and to throw errors aligns with the changes in the main implementation. This ensures preview consistency with the actual service.WooCommerce/WooCommerceTests/POS/Mocks/MockPOSItemProvider.swift (1)
49-50: Method signature update is correctThe updated method signature properly aligns with the protocol changes, ensuring test mocks remain consistent with the actual implementation.
Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift (2)
15-15: Good addition of storage failure error caseAdding the
storageFailurecase enhances error handling capabilities and provides more specific error information.
103-119: Improved error handling and paging supportThe updated method now properly throws errors when storage is unavailable and returns a paged result structure. The implementation is more robust with explicit error handling.
WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift (4)
9-12: Good addition of POSItemType enumThe enum provides a clean way to manage different item types in the controller, making the toggle functionality more maintainable.
24-24: Protocol extension with toggleItemType methodThe protocol extension appropriately adds the toggle functionality requirement to the interface.
35-35: Default item type initializationGood default initialization to
.productsto ensure the controller starts with the expected state.
53-56: Implementation of toggleItemType is clean and effectiveThe implementation efficiently toggles between item types and reloads the view with the new type of items.
WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift
Outdated
Show resolved
Hide resolved
|
@iamgabrielma This got away from my radar. I'll review this PR tomorrow! |
staskus
left a comment
staskus
left a comment
There was a problem hiding this comment.
Thanks!
I think this is important first step so I'll delay approving it since we could structure it differently based on our discussions. 👍
WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift
Outdated
Show resolved
Hide resolved
| private let paginationTracker: AsyncPaginationTracker | ||
| private var childPaginationTrackers: [POSItem: AsyncPaginationTracker] = [:] | ||
| private let itemProvider: PointOfSaleItemServiceProtocol | ||
| private var itemType: POSItemType = .products |
There was a problem hiding this comment.
Based on a discussion p1742206171389629/1741916795.290219-slack-C070SJRA8DP, we could also consider how ItemListState could be used to represent the Products | Coupons data structure.
Maybe something like this? 🤔
The current selection could be tracked on a state or separately.
|
Version |
|
Sounds good! I've spent some time today experimenting with a new approach from what I've gathered from our conversation in slack: trunk...experiment/pos-coupon-service This is not ready for review yet, but just a draft so we can sync. Let me know what do you think in general:
There's also a blocker which I haven't figured out yet as I ran out of time: The app crashes when we switch between products/coupons, I'm guessing because miss-referencing storage when we switch services, I'll continue tomorrow with this. |
|
Thanks, @iamgabrielma!
Sounds good! If you prefer, you could make PR with just of creation of the service and controller, which could be reviewed while we figure out the rest.
Another option would be to have an
I can imagine. Without actually doing it and seeing how it all works it's hard to advice something tangible. I'm not 100% sure what's the most comfortable way to represent it all. |
I think some storage code only expects to be called from the main actor. It's enough to make I pushed the fix into the branch. |
|
Overall, I think the approach is good 👍 Since we switch between Then we could move on to the required changes to the item state to support our needs. I don't want to put too many changes in one place. We may not even need WDYT? |
…e to use current controller
It's unnecessary in this PR as we do not handle rendering from the new coupon service, neither adding to cart
|
That sounds good, thanks for the tips @staskus ! This PR is updated, and ready for review. I've limited the scope to:
Screen.Recording.2025-03-25.at.11.33.53.mov |
| self.siteID = siteID | ||
| self.currencyFormatter = CurrencyFormatter(currencySettings: currencySettings) | ||
| self.productsRemote = ProductsRemote(network: network) | ||
| self.variationRemote = ProductVariationsRemote(network: network) |
There was a problem hiding this comment.
I've kept this as it is for now, but we will need to split the PointOfSaleItemServiceProtocol, as coupons do not need to know about some of these dependencies.
There was a problem hiding this comment.
No need for splitting.
A separate PointOfSaleCouponServiceProtocol is enough. We don't need the ability to interchangeably use PointOfSaleCouponService and PointOfSaleItemService. So we can safely remove all those methods and have:
public protocol PointOfSaleCouponServiceProtocol {
func providePointOfSaleCoupons(pageNumber: Int) async throws -> PagedItems<POSItem>
}
There was a problem hiding this comment.
Gotcha! I'll take a look into this in the next PR as I start to implement those.
staskus
left a comment
staskus
left a comment
There was a problem hiding this comment.
🚢 Looks good! Lets goooo
|
|
||
| @available(iOS 17.0, *) | ||
| @Observable final class PointOfSaleCouponsController: PointOfSaleItemsControllerProtocol { | ||
| let itemType: ItemType = .coupons |
There was a problem hiding this comment.
Is ItemType supposed to be used in Controllers?
There was a problem hiding this comment.
It doesn't seem necessary, nope. Removed! 249f75f
| case .variableParentProduct: | ||
| return nil | ||
| case .coupon: | ||
| debugPrint("Not implemented. TODO: Make POSCoupon POSOrderable") |
There was a problem hiding this comment.
Note: May not need to be POSOrderable, since for coupons it's enough to pass code to the API.
| self.siteID = siteID | ||
| self.currencyFormatter = CurrencyFormatter(currencySettings: currencySettings) | ||
| self.productsRemote = ProductsRemote(network: network) | ||
| self.variationRemote = ProductVariationsRemote(network: network) |
There was a problem hiding this comment.
No need for splitting.
A separate PointOfSaleCouponServiceProtocol is enough. We don't need the ability to interchangeably use PointOfSaleCouponService and PointOfSaleItemService. So we can safely remove all those methods and have:
public protocol PointOfSaleCouponServiceProtocol {
func providePointOfSaleCoupons(pageNumber: Int) async throws -> PagedItems<POSItem>
}
…on-list # Conflicts: # WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift # WooCommerce/Classes/POS/Presentation/ItemListView.swift # Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift
iamgabrielma
deleted the
task/15346-pos-allow-switch-product-vs-coupon-list
branch
Closes: #15346
Description
This PR hooks the item provider with the UI, allowing the item list to either display a list of Products/Variations or Coupons on tapping a button, so it provides a basic interface to access those coupons and add them to the cart (future PR) from the dashboard.
Screen.Recording.2025-03-14.at.10.24.18.mov
Changes
POSItemTypePOSItemTypestate.[POSItem]toPagedItems<POSItem>so these can be listed.Testing
enableCouponsInPointOfSaleflag totrueRELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: