Feat/vendor coupon distribution #2493
Conversation
WalkthroughThis pull request introduces enhancements to coupon management within the Dokan plugin. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
kzamanbd
added
Needs: Testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
includes/Vendor/Coupon.php (2)
153-192: Child order coupon sync is correct; watch for partial usage
Applying the coupon to sub-orders is handled well. However, if a coupon has usage limits or partial application complexities, verifying that the parent’s partial usage is reflected correctly in child orders is key.
223-232: Consider re-checking final amounts after child coupon removal
When removing a coupon from sub-orders, you remove the coupon code and save. Ensure the child orders recalculate totals so that subsequent item manipulations (like partial refunds) remain consistent.includes/Vendor/Hooks.php (1)
20-22: Store theCouponinstance for flexibility
Instead of creatingnew Coupon()inline, consider assigning it to a property if you plan to reference it later or unhook any of its methods. Otherwise, this is perfectly fine for a straightforward solution.includes/Order/Manager.php (2)
839-845: Mapping coupon codes
Extracting coupon codes viaarray_mapis succinct. Make sure the new array only contains unique codes if the parent uses repeated codes.
864-877: Create new coupon items – consider discount tax
CreatingWC_Order_Item_Couponobjects is solid. Just confirm whether any discount tax or shipping discount portion needs to be tracked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
includes/Order/Manager.php(4 hunks)includes/Vendor/Coupon.php(1 hunks)includes/Vendor/Hooks.php(1 hunks)
🔇 Additional comments (11)
includes/Vendor/Coupon.php (6)
17-23: Constructor hooks look good, but consider null checks for safety
All add_action and add_filter calls appear correct. As a best practice, you might want to confirm that the relevant WooCommerce classes and objects are loaded before hooking your methods, especially if any third-party integrations could be disabled.
35-56: Gracefully handle potential null order
The method retrieves an order via wc_get_order($data->get_order_id()) but does not verify if it succeeds. Consider checking for a null or invalid order object before proceeding to avoid calling methods on a potentially null variable.
68-79: Concurrency note on adding/removing filters
Removing and re-adding the woocommerce_coupon_get_apply_quantity filter works in a single-threaded context but could cause race conditions if multiple coupon calculations occurred in parallel. WooCommerce typically runs in single-threaded PHP processes, so this may be acceptable.
91-100: Ensure proper context for $discounts->get_object()
You correctly branch for WC_Cart vs WC_Order. Just ensure that if a new context (like a subscription renewal or an invoice scenario) triggers this hook, your code can handle or ignore it gracefully.
203-216: Avoid reapplication of coupons
You skip applying the coupon if it already exists in $used_coupons. This is correct. Just confirm you handle usage-limit coupons properly in the parent or child to avoid over-applying discounts.
272-286: Cart item removal logic looks good
Removing coupon info from each cart item is properly structured, updating $cart_contents with the revised metadata. Looks good for typical store workflows.
includes/Order/Manager.php (5)
11-11: Import statement is appropriately placed
use WeDevs\Dokan\Vendor\Coupon; is consistent with existing imports. This is fine.
632-632: Calling create_coupons is a good approach
This centralizes coupon logic in one method, ensuring the logic is maintainable. Verify that your error handling in create_coupons gracefully recovers if any coupon application fails.
827-829: Skip if no coupons
Early return on empty $parent_coupons is optimal for performance. This is a clean pattern to avoid overhead.
847-848: Initialize arrays
Defining $order_items and $used_coupons_data is straightforward. This chunk is trivial but correct.
849-862: Summing coupon discounts
Accumulating coupon amounts into $used_coupons_data is correct. If usage-limited coupons are partially redeemed, confirm you’re not exceeding the coupon’s usage limit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
includes/Vendor/Coupon.php (3)
68-79: Performance consideration: cloned discount object
Cloning theWC_Discountsinstance for applying coupons ($discounts_clone->apply_coupon( $coupon )) can be CPU- and memory-intensive in larger stores. Although it ensures the original discount state remains unaltered, make sure you truly need a clone. If performance is a concern, see if there's a more efficient strategy (e.g., partial or lazy cloning).
224-232: Check for empty child orders
When removing coupons from child orders, ensure$sub_ordersis not empty or null before iterating. This is typically safe but adding a quick check can prevent edge-case warnings if no child orders exist.+ if ( empty( $sub_orders ) ) { + return; + }
234-266: Ensure consistent discount distribution for multiple coupons
The loop adjusts each coupon’s discount once the total exceeds the product price ($limit_reached). If multiple coupons are applied, ensure that subsequent coupons are zeroed out in a user-friendly manner. This logic is correct, but consider clarifying it in comments or documentation to avoid future confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Vendor/Coupon.php(1 hunks)
🔇 Additional comments (6)
includes/Vendor/Coupon.php (6)
17-23: Validate existence of valid order/cart objects in hooks
Inside the constructor, multiple hooks (e.g., woocommerce_checkout_create_order_line_item) assume valid cart or order objects. It might be safer to add extra checks where these hooks are called, especially in methods like add_coupon_info_to_order_item, to avoid potential null references if an order or cart fails to load.
91-100: Validate order existence before meta updates
Similar to the removal logic, ensure $order is a legit WC_Order object before passing it to save_coupon_data_to_order_item(). This avoids potential null reference issues when the order lookup fails.
127-136: Existing safe handling of division by zero
Good job using a ternary operator to avoid a division-by-zero error when $item_object['quantity'] is zero. This ensures safe fallback to 0.
256-260: Well-handled negative discount scenario
This logic ensures that discount does not exceed the total product price by capping coupon discounts at zero. This addresses the negative discount edge-case thoroughly.
277-287: Maintain consistency between cart and order data
When removing coupon info from cart items, confirm that the resulting cart accurately reflects the removal in all subsequent order summaries. The code looks synchronized, but consider verifying or testing large cart scenarios to ensure correctness.
181-181:
Guard against division by zero for order item quantities
Here, for $order_item->get_quantity(), no check is performed before dividing $discount_amount. If $order_item->get_quantity() returns 0 (or unexpectedly negative), this raises a potential division-by-zero error.
- 'per_qty_amount' => $discount_amount / $order_item->get_quantity(),
+ 'per_qty_amount' => $order_item->get_quantity() > 0
+ ? ( $discount_amount / $order_item->get_quantity() )
+ : 0,Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Vendor/Coupon.php (1)
275-289: Minor concurrency nitpick
If multiple coupons are removed in parallel (e.g., from different browser sessions), there may be a slight chance of race conditions overwriting each other’s changes. Consider verifying final cart state if concurrency is a serious concern in your environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Vendor/Coupon.php(1 hunks)
🔇 Additional comments (5)
includes/Vendor/Coupon.php (5)
15-16: Constant naming is consistent and clear
This naming is easy to reference and concisely communicates that the meta key is specific to Dokan coupons.
35-40: Great job guarding against invalid or missing $order
This prevents potential fatal errors when the order doesn’t exist or has an invalid ID.
70-81: Verify concurrency with hooking/unhooking filters
Removing and re-adding filters in the same request is standard for preventing recursion. However, if concurrency or nested calls occur in high-traffic scenarios, confirm that no filters remain unintentionally unregistered or re-registered out of sequence.
245-264: Exceeding discount logic is properly handled
The use of $limit_reached and max() is an effective safeguard against negative discount amounts and ensures the discount does not surpass the product price.
183-183:
Prevent potential division by zero
If $order_item->get_quantity() is zero, the expression can cause a division by zero error.
Proposed fix:
- 'per_qty_amount' => $discount_amount / $order_item->get_quantity(),
+ 'per_qty_amount' => $order_item->get_quantity() > 0
+ ? ( $discount_amount / $order_item->get_quantity() )
+ : 0,Likely invalid or redundant comment.
mrabbani
added
👍 Dev Review Done
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
includes/Vendor/Coupon.php (5)
17-23: Constructor Hooks & Priorities
Registering actions and filters in the constructor is an effective approach. If further customization or interoperability is needed (e.g., with other plugins hooking into the same events), consider exposing configurable hook priorities or toggles.
95-114: Consider Splitting Logic for Maintainability
Withinsave_item_coupon_discount, the flow differs significantly based on whether the discount applies to a cart or an order. If these paths continue to grow, consider extracting them into separate classes to simplify maintenance.
156-206: Reducing Code Duplication
The logic for filtering coupon codes, calculating discounts, and setting_dokan_coupon_infois largely repeated across cart and order items. If feasible, consider extracting a shared helper function or class method to maintain DRY principles.
208-246: Sub-Order Synchronization
Applying and removing coupons from child orders ensures consistent discount handling. If any advanced scenarios arise (e.g., partial refunds, order cancellations), ensure they also propagate appropriately to child orders. Currently, the approach appears solid.Would you like help adding fallback logic for scenarios like partial refund or order cancellation to keep sub-orders fully in sync?
248-302: Discount Capping Logic
The code caps total discount at the product price by computing$remain_discountand adjusting subsequent coupons to zero. This logic is correct if the intended behavior is to ignore extra coupon amounts. For a more equitable cost-sharing across coupons, consider distributing the remainder among all relevant coupons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Vendor/Coupon.php(1 hunks)
🔇 Additional comments (4)
includes/Vendor/Coupon.php (4)
1-3: Namespace and PSR-4 Compliance Looks Good
The namespace WeDevs\Dokan\Vendor and file structure appear to follow recommended PSR-4 conventions and WordPress best practices. No immediate concerns here.
25-58: Robust Null Checks are in Place
The method ensures $data is an instance of WC_Order_Item_Coupon and performs a null check on $order before proceeding. This addresses prior concerns about possible null returns from wc_get_order. Implementation is solid.
60-93: Coupon Interception Approach
Cloning $discounts to preserve state before reapplying the coupon is a clever strategy. However, be mindful that subsequent logic on $discounts_clone should not affect the main $discounts object once reattached. Make sure any shared references are handled carefully if the logic becomes more complex in the future.
116-154: Division-by-Zero Guard
The check ($item_object['quantity'] > 0 ? ... : 0) correctly prevents division by zero. This aligns with prior recommendations.
create simple test case for coupon apply
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/php/src/Coupon/CouponTest.php (4)
16-42: Method naming convention.
PHPUnit typically expectssetUp()instead ofset_up(). Using the standardpublic function setUp(): voidcan improve IDE and toolchain compatibility.-public function set_up() { +public function setUp(): void {
44-61: Comprehensive order data setup.
Theget_order_data()method provides a clear sample order. Consider adding more variations (e.g., multiple line items, different prices) for extended coverage.
92-106: Testing coupon with all products.
The discount assertion is valid, and suborder counting is verified. You may want to test shipping cost calculations if relevant for the final billing.
148-177: Testing coupon usage limit.
This test correctly checks that a coupon with ausage_limitof 1 applies only once. Consider verifying behavior across multiple customers or sessions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/php/src/Coupon/CouponTest.php(1 hunks)
🔇 Additional comments (9)
tests/php/src/Coupon/CouponTest.php (9)
3-4: Namespace usage aligns with plugin structure.
No issues found.
5-7: Imports and base test class.
TheExceptionandDokanTestCaseclasses are correctly imported and used.
8-11: Descriptive test group annotation.
The@group coupon-discountannotation is a good way to group and run related tests selectively.
14-15: Use of typed properties.
Declaring typed properties (e.g.,public int $product_id1;) helps ensure type safety and clarity.
67-85: Data provider for all products.
The data provider covers a basic percent discount scenario. Consider adding edge cases such as zero or negative discount values for thorough testing.
112-127: Data provider for specific products.
The array for fixed product discount is sufficient for basic coverage. Consider adding a scenario with multiple product IDs for more realistic coverage.
135-147: Testing specific product coupons.
The test properly verifies the discount for included product IDs. You might add an additional test for multiple coupons applied simultaneously if needed.
178-199: Test with minimum amount requirement.
This ensures the coupon isn't applied if the order doesn't meet the minimum. Consider adding an edge case where the order total is exactly the minimum amount (like 200) to verify boundary conditions.
200-220: Email restriction enforcement.
The test confirms that the coupon is not applied for unmatched emails. Good approach. Optionally, add a test for multiple allowed email addresses to confirm partial matches.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/php/src/Coupon/CouponTest.php (5)
21-46: Consider extracting test data setup into helper methods.The test data setup logic could be more maintainable by extracting the creation of sellers, products, categories, and customers into separate helper methods.
Example refactor:
public function data_provider(): array { - $this->seller_id1 = $this->factory()->seller->create(); - $this->seller_id2 = $this->factory()->seller->create(); - $this->category_id1 = $this->factory()->term->create( [ 'taxonomy' => 'product_cat' ] ); + $this->setupSellers(); + $this->setupCategories(); + $this->setupProducts(); + $this->setupCustomers();
47-61: Fix indentation inconsistency.The code uses a mix of tabs and spaces for indentation. Consider using spaces consistently throughout the file.
66-238: Consider using constants for test values.The test data contains magic numbers for prices, discounts, and other values. Consider defining these as class constants to improve maintainability and make the test's intent clearer.
Example:
+ private const PRODUCT_1_PRICE = 50; + private const PRODUCT_2_PRICE = 30; + private const PERCENTAGE_DISCOUNT = 20; + private const FIXED_DISCOUNT = 5; + private const MINIMUM_AMOUNT = 150; return [ 'discount_type_percentage' => [ [ 'coupons' => [ [ 'meta' => [ 'discount_type' => 'percent', - 'coupon_amount' => 20, + 'coupon_amount' => self::PERCENTAGE_DISCOUNT,
246-285: Enhance test assertions and error messages.Consider the following improvements:
- Add descriptive messages to assertions to make test failures more informative
- Add assertions for free shipping status where applicable (e.g., 'single vendor order' case)
Example enhancement:
- $this->assertEquals( $expected['discount'], $order->get_discount_total() ); + $this->assertEquals( + $expected['discount'], + $order->get_discount_total(), + sprintf( + 'Expected discount of %s but got %s for coupon scenario', + $expected['discount'], + $order->get_discount_total() + ) + ); + if ( isset( $expected['free_shipping'] ) ) { + $this->assertTrue( + $order->get_shipping_total() === '0', + 'Free shipping was not applied correctly' + ); + }
66-238: Consider adding test cases for additional scenarios.The test coverage could be enhanced by adding test cases for:
- Multiple coupons applied to the same order
- Invalid coupon scenarios (expired, usage limit reached)
- Individual use coupons
- Usage limits (per user, per coupon)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/php/src/Coupon/CouponTest.php(1 hunks)
🔇 Additional comments (1)
tests/php/src/Coupon/CouponTest.php (1)
1-15: Well-structured test class setup!The class follows PHPUnit best practices with proper namespace, test group annotation, and typed properties.
kzamanbd
added
Needs: Dev Review
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Couponclass for enhanced coupon management in WooCommerce.Bug Fixes
Refactor
Hooksclass to integrate coupon distribution capabilities.Tests