[Woo POS] Coupons: Add Coupon(s) to Cart (dummy UI) #15407
Conversation
Evolved Cart structure to contain Items and Coupons to resemble Order that contains both OrderItems and CouponLineItems. Abstracted both types of items under CartItem which shares an id for now. Created helper add, remove, removeAll, isEmpty methods to more easily switch from current functionality
staskus
force-pushed
the
task/15330-woo-pos-coupons-add-to-cart
branch
from
c8677ed to
fac9b49
Compare
|
|
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis pull request refactors the cart management in the POS module. It replaces the old array‑based cart system with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CartView
participant OrderController
participant AsyncService
User->>CartView: Initiate order sync
CartView->>OrderController: syncOrder(for: Cart)
OrderController->>OrderController: Map items from Cart (products & coupons)
OrderController->>AsyncService: Process order asynchronously
AsyncService-->>OrderController: Return Result (SyncOrderState)
OrderController-->>CartView: Deliver sync result
sequenceDiagram
participant User
participant CartView
participant CouponRowView
participant Cart
User->>CartView: Request coupon removal
CartView->>CouponRowView: onItemRemoveTapped invoked
CouponRowView->>Cart: Remove coupon item
Cart-->>CartView: Updated cart state
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
WooCommerce/Classes/POS/Presentation/CouponRowView.swift (1)
3-48: Well-structured SwiftUI view implementation.The CouponRowView follows good SwiftUI practices with clean layout structure, proper styling, and accessibility support through @ScaledMetric. The optional removal callback is a nice touch for component reusability.
Consider adding a documentation comment above the struct definition to explain the purpose and usage context of this view.
WooCommerce/WooCommerce.xcodeproj/project.pbxproj (1)
27-33: New File Reference for CouponRowView.swift Added
This hunk adds a new build file entry forCouponRowView.swiftalongside existing files. The file reference and associated settings appear consistent with project conventions.WooCommerce/Classes/POS/Models/Cart.swift (1)
46-55: Consider a more protocol-oriented approach for item removal.The current implementation uses type checking with
switchandcase _ as Type, which works but is less idiomatic in Swift. Consider a more protocol-oriented approach where each cart item type knows which collection it belongs to.One alternative approach could be:
-mutating func remove(_ cartItem: any CartItem) { - switch cartItem { - case _ as CartProductItem: - items.removeAll { $0.id == cartItem.id } - case _ as CartCouponItem: - coupons.removeAll { $0.id == cartItem.id } - default: - break - } -} +protocol CartItemCollection { + var collectionType: CartItemCollectionType { get } +} + +enum CartItemCollectionType { + case product + case coupon +} + +extension CartProductItem: CartItemCollection { + var collectionType: CartItemCollectionType { .product } +} + +extension CartCouponItem: CartItemCollection { + var collectionType: CartItemCollectionType { .coupon } +} + +mutating func remove(_ cartItem: any CartItem & CartItemCollection) { + switch cartItem.collectionType { + case .product: + items.removeAll { $0.id == cartItem.id } + case .coupon: + coupons.removeAll { $0.id == cartItem.id } + } +}WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift (5)
35-35: Enhance test coverage
You're verifying the cart isn't empty, which is correct. Consider also asserting the specific items expected in the cart for tighter validation.
107-107: Confirm cart's content
This assertion ensures the cart isn't empty. It might be helpful to add item-level checks to confirm correct items are present.
143-143: Naming clarity
Accessing the first cart item is fine. For added clarity, consider naming itfirstCartItem.
738-738: Empty cart usage
Passing.init()tosyncOrdermeans syncing an empty cart. Consider adding a variant to test non‑empty carts for better coverage.
763-766: Onboarding view model initialization
Defining a specific plugin state for the onboarding flow is helpful. Consider testing other states to fully validate the onboarding logic.WooCommerce/Classes/POS/Presentation/CartView.swift (2)
50-63: Coupon row integration
Separating coupons from regular items improves UI clarity. Consider adding unit tests and verifying scenarios such as removing multiple coupons or adding duplicates.
76-77: Chained animations
Having separate animation calls for items and coupons is a clean approach. Monitor performance on large or rapidly changing carts.WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift (2)
118-119: Removal logic
Delegating removal tocart.removekeeps responsibilities well-defined. Consider logging if the item isn't found.
149-149: Customer interaction tracking
Checkingcart.isEmptyto track a new interaction is sensible. Also consider how returning shoppers or partial coupon usage might fit in.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift(2 hunks)WooCommerce/Classes/POS/Models/Cart.swift(1 hunks)WooCommerce/Classes/POS/Models/CartItem.swift(0 hunks)WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift(5 hunks)WooCommerce/Classes/POS/Presentation/CartView.swift(6 hunks)WooCommerce/Classes/POS/Presentation/CouponRowView.swift(1 hunks)WooCommerce/Classes/POS/Presentation/ItemListView.swift(0 hunks)WooCommerce/Classes/POS/Presentation/ItemRowView.swift(3 hunks)WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift(1 hunks)WooCommerce/Classes/POS/ViewHelpers/CartViewHelper.swift(1 hunks)WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingViewModel.swift(1 hunks)WooCommerce/WooCommerce.xcodeproj/project.pbxproj(8 hunks)WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift(17 hunks)WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleAggregateModel.swift(1 hunks)WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift(1 hunks)WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift(32 hunks)WooCommerce/WooCommerceTests/POS/ViewHelpers/CartViewHelperTests.swift(1 hunks)Yosemite/Yosemite/PointOfSale/POSCoupon.swift(1 hunks)Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift(1 hunks)
💤 Files with no reviewable changes (2)
- WooCommerce/Classes/POS/Presentation/ItemListView.swift
- WooCommerce/Classes/POS/Models/CartItem.swift
🧰 Additional context used
🧬 Code Definitions (5)
WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift (2)
WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift (1)
syncOrder(65-101)WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift (1)
syncOrder(23-37)
WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift (1)
WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleAggregateModel.swift (2)
addToCart(52-52)remove(54-54)
WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift (2)
WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift (1)
syncOrder(15-17)WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift (1)
syncOrder(65-101)
WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift (2)
WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift (1)
syncOrder(15-17)WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift (1)
syncOrder(23-37)
WooCommerce/Classes/POS/Presentation/CartView.swift (1)
WooCommerce/Classes/POS/ViewHelpers/CartViewHelper.swift (1)
itemsInCartLabel(4-11)
🔇 Additional comments (54)
Yosemite/Yosemite/PointOfSale/POSCoupon.swift (1)
3-8: Good refactoring from numeric ID to string code for coupons.The change from
couponID: Int64tocode: Stringbetter represents how coupons typically work in real-world scenarios, where customers enter alphanumeric codes rather than referencing numeric IDs. The new initializer is clean and properly supports this data model change.WooCommerce/Classes/POS/Presentation/CouponRowView.swift (2)
50-58: Good use of constants for styling consistency.Extracting dimensions and styles into a Constants enum improves maintainability and ensures consistent styling across the view.
60-65: Preview aids in development and testing.The inclusion of a preview is helpful for development and ensures the component renders as expected.
Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift (1)
128-128: Properly updated to match POSCoupon struct changes.This change correctly adapts the coupon mapping to use the new
codeproperty instead of the removedcouponID, maintaining consistency with the POSCoupon struct modifications.WooCommerce/Classes/POS/Utils/PointOfSalePreviewOrderController.swift (1)
15-15: Method signature updated for consistency with the new Cart model.The change from accepting an array of
CartItemto a singleCartobject aligns with the broader refactoring in the PR, creating consistency across controllers. This matches the implementation in PointOfSaleOrderController shown in the relevant code snippets.WooCommerce/WooCommerce.xcodeproj/project.pbxproj (7)
1633-1639: Updated Cart File Reference and Consistency Check
In this hunk, the reference for the cart file has been updated from the oldCartItem.swifttoCart.swift. Verify that all build file entries reflecting the cart module now uniformly useCart.swiftand that no outdated references remain.
3260-3266: PBXFileReference Updates Include CouponRowView.swift
The file references for Swift source files (including the newly addedCouponRowView.swift) are now listed. The settings such aslastKnownFileType,path, andsourceTreeare correctly set.
4810-4816: Cart.swift Reference Updated in File References
This hunk reflects the updated file reference forCart.swiftwithin the project’s file reference section. Please confirm the path and grouping align with the intended project structure.
7244-7250: CouponRowView.swift Inclusion in Group Listings
Here,CouponRowView.swifthas been added to the group listing alongside other view components. This ensures that the new coupon UI element is incorporated into the project’s navigation.
9932-9938: Cart.swift Reference Replaced in Group Lists
The reference toCart.swiftis now present in the group lists, replacing the older file naming. It is advisable to search the project for any lingering references toCartItem.swiftto ensure consistency.
16084-16090: CouponRowView.swift Added in Source Listings
This hunk confirms thatCouponRowView.swiftis now referenced in the source lists, ensuring the UI component is properly integrated into multiple parts of the project.
16582-16588: Final Group Listing Update with Cart.swift Reference
The final hunk shows thatCart.swiftappears correctly within the group listings. It’s recommended to verify that these changes do not affect build settings and that the integration tests pass as expected.WooCommerce/Classes/POS/ViewHelpers/CartViewHelper.swift (1)
21-22: Updated method signature and implementation to support Cart modelThis change updates the
shouldShowClearCartButtonmethod to accept aCartobject instead of an array ofCartItemand modifies the implementation to use!cart.isEmptyinstead of checking if the array is not empty. This aligns with the new Cart model structure that supports both product and coupon items.WooCommerce/WooCommerceTests/POS/ViewHelpers/CartViewHelperTests.swift (4)
76-78: Tests updated to use Cart model instead of CartItem arrayThe test cases have been updated to use the new
Cartmodel with.init()for empty cart creation instead of an empty array of cart items. This properly reflects the changes in the model structure.
81-82: Tests updated to use Cart with CartProductItemThe test now correctly initializes a Cart with CartProductItem using
.init(items: [makeItem()])instead of passing an array directly, maintaining the same test logic with the new data structure.
85-86: Tests updated to use Cart with CartProductItemSimilar to the previous test, this test has been correctly updated to use the new Cart model with CartProductItem while maintaining the same test conditions.
90-96: Updated makeItem helper to return CartProductItemThe
makeItem()helper function now returns aCartProductIteminstead ofCartItem, consistent with the new Cart model that separates products and coupons. The implementation creates a valid CartProductItem with all required fields.WooCommerce/Classes/POS/Presentation/ItemRowView.swift (4)
4-4: Updated cartItem property type from CartItem to CartProductItemThe property type has been changed to use the more specific
CartProductItemtype instead of the genericCartItem. This aligns with the new Cart model that separates products and coupons as distinct item types.
14-14: Updated initializer parameter type to CartProductItemThe initializer has been updated to accept a
CartProductIteminstead of aCartItem, consistent with the property type change. This ensures type safety when creating an ItemRowView instance.
97-102: Updated preview to use CartProductItemThe SwiftUI preview has been updated to use
CartProductIteminstead ofCartItem, maintaining consistency with the view's updated requirements.
107-112: Updated second preview to use CartProductItemThis preview has also been correctly updated to use
CartProductIteminstead ofCartItem, ensuring all previews are compatible with the view's updated requirements.WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingViewModel.swift (2)
48-48: Added useCase parameter to initializer for better testabilityA new
useCaseparameter with a default value has been added to the initializer, enabling dependency injection. This improves testability by allowing mock use cases to be provided during testing.
52-52: Updated implementation to use injected useCaseThe implementation now uses the injected
useCaseparameter instead of creating a new instance, which is consistent with the parameter addition and maintains the same behavior for existing code.WooCommerce/Classes/POS/Models/Cart.swift (4)
5-8: Good structure for the Cart model.The Cart struct is well-designed with separate arrays for product items and coupons, providing a clear separation of concerns.
10-12: Appropriate protocol definition.The CartItem protocol correctly extends Identifiable and requires an id property, establishing a good foundation for polymorphism.
14-20: Well-structured item models.Both CartProductItem and CartCouponItem are appropriately designed with immutable properties and proper conformance to the CartItem protocol.
Also applies to: 22-25
57-65: Clear implementation of utility methods.The removeAll method and isEmpty computed property are well-implemented and provide the expected functionality.
WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleOrderController.swift (2)
19-19: Appropriate type update for the new Cart model.The spyCartProducts type has been correctly updated to use CartProductItem to align with the new Cart model structure.
24-28: Properly adapted syncOrder method.The syncOrder method signature and implementation have been correctly updated to work with the new Cart structure.
WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleOrderControllerTests.swift (2)
23-23: Test methods properly updated to use the new Cart structure.All syncOrder method calls have been correctly updated to create a Cart instance and pass it to the method.
Also applies to: 38-43, 56-64, 83-83, 102-104, 133-134, 172-173, 211-212, 252-252, 311-312, 314-314, 336-336, 339-340, 357-357, 389-390, 404-404, 432-432
456-467: Helper method correctly adjusted for the new model.The makeItem function has been properly updated to return CartProductItem and construct it with the required parameters.
WooCommerce/WooCommerceTests/POS/Mocks/MockPointOfSaleAggregateModel.swift (2)
50-50: Properly updated cart property type.The cart property has been correctly changed from an array of CartItem to a Cart instance.
54-54: Improved type flexibility with 'any CartItem'.Using
any CartItemfor the parameter type provides better flexibility for handling different item types.WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift (9)
18-18: Mock analytics usage
Using a separateMockAnalyticsProvider()ensures that the test environment won't send real analytics data. Approving.
30-30: Duplicate
51-51: Duplicate
124-124: Well-defined ordering test
Verifying item IDs match the reversed order is clear and robust. This helps ensure new items are properly prepended.
140-140: Logical correctness
Ensuring exactly two items in the cart aligns with the test setup. Looks correct.
147-147: Accurate final count
Asserting the cart length is 1 is consistent with the earlier removal step.
148-148: Meaningful assertion
Checking thetitleensures the correct item remains. Good coverage.
164-164: Proper cart count
Matches the intended scenario in this test.
239-239: Cart item extraction
Ensures there's at least one item in the cart before proceeding. Good defensive check.WooCommerce/Classes/POS/Controllers/PointOfSaleOrderController.swift (2)
30-30: Updated function signature
Switching from an array ofCartItemto aCartstreamlines the model in line with the rest of the cart refactor.
66-68: Refined synchronization
Mappingcart.itemsintoPOSCartItemis straightforward. Consider ensuring item quantities remain non‑negative to prevent unexpected behavior.Do you want me to check for negative quantity references elsewhere in the codebase?
WooCommerce/Classes/POS/Presentation/CartView.swift (5)
14-14: Shadow condition
Switching fromposModel.cart.isNotEmptyto!posModel.cart.isEmptyis logically sound. Coupling it withoffSetPosition < 0correctly applies the shadow only when scrolled.
25-25: Item count usage
UsingposModel.cart.items.countclarifies that the label depends specifically on product items rather than coupons or other data.
46-46: New cart emptiness check
if !posModel.cart.isEmptymatches the updated cart structure. No concerns here.
100-100: Scrolling to the first item
Scrolling uponcart.items.first?.idchanges is helpful, but watch for potential timing issues if the cart updates frequently.Would you like me to check concurrency or race conditions around frequent cart mutations?
287-287: Analytics tracking
UsingposModel.cart.items.countensures accurate event data for the number of items in the cart.WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift (5)
9-9: New import
ImportingPOSCouponaligns with adding coupons to the cart.
31-31: Updated protocol
Switching from[CartItem]toCartfosters centralized logic and clearer data flow.
33-33: Generic cart item removal
Acceptingany CartItemallows for multiple item types. Consider whether a more descriptive method name likeremoveItemcould be clearer.Would you like me to propose an alternative naming convention?
57-57: Initialize cart
Initializingcartwith.init()at construction avoids null references and is clean.
115-115: Cart addition
Replacing item conversion logic withcart.add(item)is a welcome simplification.
staskus
force-pushed
the
task/15330-woo-pos-coupons-add-to-cart
branch
from
8e6f33c to
61e3423
Compare
| import enum Yosemite.POSItem | ||
|
|
||
| struct Cart { | ||
| var items: [CartProductItem] = [] |
There was a problem hiding this comment.
For the Cart struct, should we update the name of items to products since we're starting to be more specific about the type?
There was a problem hiding this comment.
Good question. I'm confused about this as well 😀 I chose this name only because we refer to products as items within the Order object. But then it's a how to call the type. Maybe I should rename CardProductItem to CartItem again, and remove the protocol CartItem since its value is questionable. Or call this CartItemProtocol 🤔
| if posModel.cart.coupons.isNotEmpty { | ||
| ForEach(posModel.cart.coupons, id: \.id) { couponItem in | ||
| CouponRowView(couponItem: couponItem, | ||
| onItemRemoveTapped: posModel.orderStage == .building ? { | ||
| posModel.remove(cartItem: couponItem) | ||
| } : nil) | ||
| .id(couponItem.id) | ||
| .transition(.opacity) | ||
| } | ||
|
|
||
| Spacer(minLength: 64) | ||
| } |
There was a problem hiding this comment.
Should we make this section more explicit? Something like CouponsCartSectionView and declare a shouldRenderCouponsSection variable? I have no strong opinion about it, mostly for clarity when scanning the file and in case we forget about the feature flag or the internal comment.
There was a problem hiding this comment.
👍 Of course, I can. For now it's all dummy UI since I don't know how it will look. However, we can make the code tidy.
| private var coordinateSpace: CoordinateSpace = .named(Constants.scrollViewCoordinateSpaceIdentifier) | ||
| private var shouldApplyHeaderBottomShadow: Bool { | ||
| posModel.cart.isNotEmpty && offSetPosition < 0 | ||
| !posModel.cart.isEmpty && offSetPosition < 0 |
There was a problem hiding this comment.
Just curious: Do you find ! + isEmpty more readable than .isNotEmpty? Or the change is just because we reach for cart.isEmpty as a product AND coupons check?
There was a problem hiding this comment.
Just curious: Do you find ! + isEmpty more readable than .isNotEmpty
No. There's no difference for me. I can declare .isNotEmpty on aCart struct, and keep .isNotEmpty.
| init( | ||
| fixedState: CardPresentPaymentOnboardingState, | ||
| fixedUserIsAdministrator: Bool = false, | ||
| useCase: CardPresentPaymentsOnboardingUseCaseProtocol = CardPresentPaymentsOnboardingUseCase(), |
|
Changes:
|
Closes: #15330
Description
Adding functionality to support Coupons in the Cart with a dummy UI. Applying coupons to a cart is outside the scope.
The result - adding coupon to cart
cart.add(POSItem.coupon(.init(id: UUID(), code: "coupon")))is reflected in the Cart UI.New Structs
The largest change, similar to what we may need to do with the item list, is to support Coupons next to Items (Products) in the Cart.
I decided to use the structure of the
Order, sinceCartis more or less a reflection of it.Usage
I updated the code to reference
Cartinstead ofCartItem. For most of the UI, I kept usingcart.items.ItemRowViewheavily relies on product logic so I use specificCartProductItemfor it. For some UI, such as 'Clear' button, I check if the wholecart.isEmptyrather than a specific type.The updated unit tests confirmed that with these changes everything continued working as before.
Dummy UI
Created a dummy
CouponRowViewwhich is a simplified version ofItemRowViewand I show it inCartViewif any coupons exist. This change shouldn't affect production.Additional changes
Fixed
PointOfSaleAggregateModelTestssince they were crashing due to unmocked dependencies that were expected to be run in the main thread.Steps to reproduce
These changes shouldn't change anything, and POS should work as before.
However, you can test adding coupons by manually adding coupon in
addToCartmethod inPointOfSaleAggregateModel, for example:Testing information
Ran smoke POS tests of basically cart and order creation functionality to confirm that these changes do not cause regressions.
Screenshots
Add.Coupon.mp4
RELEASE-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: