refactor: Improve coupon validation with extensible filter hook #2509
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![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 (2)
includes/Order/Hooks.php (2)
423-432: Fix indentation inconsistencies.The code uses a mix of tabs and spaces for indentation. Maintain consistent indentation using spaces.
- foreach ( $discount->get_items() as $item ) { - if ( ! isset( $item->product ) || ! $item->product instanceof \WC_Product ) { - continue; - } - - $item_id = $item->product->get_id(); - - $available_vendors[] = (int) dokan_get_vendor_by_product( $item_id, true ); - $available_products[] = $item_id; - } + foreach ( $discount->get_items() as $item ) { + if ( ! isset( $item->product ) || ! $item->product instanceof \WC_Product ) { + continue; + } + + $item_id = $item->product->get_id(); + + $available_vendors[] = (int) dokan_get_vendor_by_product( $item_id, true ); + $available_products[] = $item_id; + }
440-453: Enhance filter documentation and consider early returns.
- Replace
DOKAN_SINCEplaceholder in the filter documentation- Consider restructuring with early returns for better readability
/** * Filter the validity of a coupon. * - * @since DOKAN_SINCE + * @since 3.9.0 * * @param boolean $valid The validity of the coupon. * @param \WC_Coupon $coupon The coupon object. * @param array $available_vendors List of available vendors. * @param array $available_products List of available products. * @param \WC_Discounts $discount The discount object, which contains the order details. */ - if ( apply_filters( 'dokan_coupon_is_valid', $valid, $coupon, $available_vendors, $available_products, $discount ) ) { - return true; - } - - return $valid; + return apply_filters( 'dokan_coupon_is_valid', $valid, $coupon, $available_vendors, $available_products, $discount );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (2)
includes/Order/Hooks.php (2)
49-49: LGTM! Hook registration is correct.The renamed method is properly hooked into WooCommerce's coupon validation filter with appropriate priority and argument count.
434-438: LGTM! Multiple vendor validation is well implemented.The validation logic for fixed cart coupons with multiple vendors is clear and throws an appropriate exception.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Order/Hooks.php (1)
419-432: Add input parameter validation.Consider adding early validation for the input parameters to ensure they are of the expected types. This would make the code more robust and prevent potential issues.
public function ensure_is_coupon_valid( $valid, $coupon, $discount ) { + if ( ! $coupon instanceof \WC_Coupon ) { + return $valid; + } + + if ( ! $discount instanceof \WC_Discounts ) { + return $valid; + } + $available_vendors = []; $available_products = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (4)
includes/Order/Hooks.php (4)
49-49: LGTM! Hook registration updated correctly.The hook registration has been properly updated to use the new method name while maintaining the same priority and argument count for backward compatibility.
410-417: Update the @SInCE version tag.The PHPDoc block still contains the placeholder "DOKAN_SINCE". Please replace it with the actual version number.
-@since DOKAN_SINCE Refactored to make it more flexible, and added filter +@since 3.9.0 Refactored to make it more flexible, and added filter
440-450: Update the filter hook documentation.The filter hook documentation still contains the placeholder "DOKAN_SINCE". Please replace it with the actual version number.
-@since DOKAN_SINCE +@since 3.9.0
419-454: LGTM! Well-structured implementation with improved extensibility.The refactored implementation is clean and maintainable with:
- Clear variable names
- Proper separation of concerns
- New filter hook for extensibility
- Improved type hints for better IDE support
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
includes/Order/Hooks.php (3)
413-420: Update the@sinceversion tag.Replace the
DOKAN_SINCEplaceholder with the actual version number.-@since DOKAN_SINCE Refactored to make it more flexible, and added filter +@since 3.9.0 Refactored to make it more flexible, and added filter
422-435: Consider restructuring with early returns for better readability.The method could be more readable with early returns and better variable initialization.
public function ensure_is_coupon_valid( bool $valid, WC_Coupon $coupon, WC_Discounts $discounts ): bool { + if (!$valid) { + return false; + } + $available_vendors = []; $available_products = []; foreach ( $discounts->get_items() as $item ) { if ( ! isset( $item->product ) || ! $item->product instanceof WC_Product ) { continue; } $item_id = $item->product->get_id(); $available_vendors[] = (int) dokan_get_vendor_by_product( $item_id, true ); $available_products[] = $item_id; }
443-456: Update the filter hook documentation version.The
@sincetag in the filter documentation still contains the placeholder version./** * Filter the validity of a coupon. * - * @since DOKAN_SINCE + * @since 3.9.0 * * @param boolean $valid The validity of the coupon.Additionally, consider adding code examples in the documentation to show how to use the new filter hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
includes/Order/Hooks.php (2)
6-7: LGTM! Proper type imports added.The new imports for
WC_CouponandWC_Discountsare correctly added, improving type safety.
52-52: LGTM! Hook registration updated to match renamed method.The hook registration is correctly updated to use the new method name while maintaining the same priority and parameter count.
mralaminahamed
added
Needs: Testing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
includes/Order/Hooks.php (3)
426-435: Consider adding early return for empty items.The loop could be optimized by adding an early return if there are no items to process.
+ if ( empty( $discounts->get_items() ) ) { + return $valid; + } + foreach ( $discounts->get_items() as $item ) {
443-454: Enhance filter documentation with specific array types.The array parameter types in the filter documentation could be more specific about their structure.
* @param boolean $valid The validity of the coupon. * @param WC_Coupon $coupon The coupon object. - * @param array $available_vendors List of available vendors. - * @param array $available_products List of available products. + * @param int[] $available_vendors Array of vendor IDs from cart items. + * @param int[] $available_products Array of product IDs from cart items. * @param WC_Discounts $discounts The discount object, which contains the order details.
422-454: LGTM! Well-structured coupon validation logic.The refactored implementation:
- Correctly handles vendor-specific validation
- Properly validates fixed cart coupons
- Introduces extensibility through the new filter hook
The new filter hook
dokan_coupon_is_validprovides a clean extension point for custom validation rules, aligning well with the PR's objective of improving extensibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
🔇 Additional comments (2)
includes/Order/Hooks.php (2)
6-7: LGTM! Clean updates to use statements and hook registration.The new use statements are properly added and the hook registration is correctly updated to match the renamed method.
Also applies to: 52-52
413-420: Replace DOKAN_SINCE placeholder with actual version.The PHPDoc block still contains the placeholder "DOKAN_SINCE" which needs to be replaced with the actual version number.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
includes/Order/Hooks.php (1)
455-463: Refactor duplicate error code.Both exception throws use the same error code
108. Consider defining this as a constant to avoid magic numbers and ensure consistency.+ private const ERROR_CODE_AMOUNT_RESTRICTION = 108; - ), 108 + ), self::ERROR_CODE_AMOUNT_RESTRICTION - ), 108 + ), self::ERROR_CODE_AMOUNT_RESTRICTIONAlso applies to: 478-486
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 459-459:
Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.🪛 GitHub Actions: Inspections
[error] Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Order/Hooks.php
[failure] 459-459:
Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
[failure] 482-482:
Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
🪛 GitHub Actions: Inspections
includes/Order/Hooks.php
[error] Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
includes/Order/Hooks.php (3)
6-7: LGTM! Proper type imports added.The addition of explicit type imports for
WC_CouponandWC_Discountsimproves code clarity and type safety.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
413-422: Update DOKAN_SINCE placeholder with actual version number.The docblock still contains the placeholder
DOKAN_SINCE. This should be replaced with the actual version number (e.g., '3.9.0') as suggested in the previous review.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
426-435: LGTM! Clean and efficient item processing.The loop efficiently processes items while properly type-checking the product instances.
🧰 Tools
🪛 GitHub Actions: Inspections
[error] Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Order/Hooks.php (1)
422-500: Well-structured refactoring with improved extensibility.The refactoring successfully:
- Separates concerns between validation types
- Adds proper type safety
- Introduces extension points via filters
- Maintains backward compatibility
Consider adding integration tests to verify the behavior of the new filter hooks with various coupon scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (6)
includes/Order/Hooks.php (6)
6-7: LGTM! Clean import additions.The new imports for
WC_CouponandWC_Discountsare properly added to support the type hints in the refactored method.
413-422: Replace DOKAN_SINCE placeholder with actual version.The PHPDoc block still contains the placeholder
DOKAN_SINCE. This should be replaced with the actual version number where this change is being introduced.
422-422: LGTM! Improved method signature.The method signature changes are excellent:
- Descriptive rename from
ensure_vendor_coupontoensure_coupon_is_valid- Added type hints for parameters and return type
- Return type declaration matches the PHPDoc
443-463: LGTM! Well-structured minimum amount validation.The minimum amount validation is well-implemented with:
- Clear filter hook naming
- Comprehensive docblock
- Proper error handling
466-486: LGTM! Well-structured maximum amount validation.The maximum amount validation mirrors the minimum amount implementation with consistent structure and error handling.
489-500: LGTM! Flexible validation hook.The
dokan_coupon_is_validfilter provides excellent extensibility by exposing all relevant data:
- Coupon object
- Available vendors
- Available products
- Discounts object
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
includes/Order/Hooks.php (3)
461-461: Document the error code constant.The hardcoded error code
108should be defined as a constant with a descriptive name to improve maintainability.+ const ERROR_CODE_AMOUNT_RESTRICTION = 108; ... - 108 + self::ERROR_CODE_AMOUNT_RESTRICTIONAlso applies to: 483-483
448-448: Update @SInCE tags in filter documentation.Replace
DOKAN_SINCEwith the actual version number in the filter documentation blocks:
- dokan_coupon_validate_minimum_amount
- dokan_coupon_validate_maximum_amount
- dokan_coupon_is_valid
Also applies to: 470-470, 490-490
443-462: Well-structured filter implementation.The addition of new filter hooks improves extensibility:
dokan_coupon_validate_minimum_amountdokan_coupon_validate_maximum_amountdokan_coupon_is_validThis allows for custom validation rules while maintaining clean separation of concerns.
Consider documenting common use cases for these filters in the developer documentation to help plugin developers understand when and how to use them effectively.
Also applies to: 465-484, 487-498
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (4)
includes/Order/Hooks.php (4)
6-7: LGTM! Import statements are properly organized.The new imports for
WC_CouponandWC_Discountsalign well with the type hints in the refactored method.
413-422: Update the @SInCE version number.The
@since DOKAN_SINCEplaceholder needs to be replaced with the actual version number.
458-458: Fix text domain inconsistency.The text domain should be 'dokan-lite' instead of 'dokan' for consistency.
Also applies to: 480-480
426-435: LGTM! Improved item validation logic.The refactored code properly validates items using type checking and collects vendor and product information efficiently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Order/Hooks.php (1)
426-429: Add early return for invalid items.Consider adding an early return if no valid items are found to improve performance and readability.
foreach ( $discounts->get_items() as $item ) { if ( ! isset( $item->product ) || ! $item->product instanceof WC_Product ) { - continue; + return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
includes/Order/Hooks.php (3)
6-7: LGTM! Clean import additions and hook registration update.The new imports and hook registration changes align well with the refactoring objectives.
Also applies to: 52-52
413-420: Update version placeholder in PHPDoc.The
@since DOKAN_SINCEplaceholder needs to be replaced with the actual version number.
443-454: LGTM! Well-documented filter hook.The new filter hook
dokan_coupon_is_validprovides comprehensive context with all necessary parameters for third-party extensions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
includes/Order/Hooks.php (2)
425-431: Fix code alignment and add null checks.
- Fix alignment issues flagged by PHPCS:
- $available_vendors = []; + $available_vendors = []; - $available_vendors[] = (int) dokan_get_vendor_by_product( $item->product->get_id(), true ); + $available_vendors[] = (int) dokan_get_vendor_by_product( $item->product->get_id(), true );
- Consider adding a null check for
dokan_get_vendor_by_product()return value to handle cases where a product might not have an associated vendor.🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 430-430:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
439-448: Update filter documentation version.The
@since DOKAN_SINCEtag in the filter documentation needs to be updated with the actual version number.Additionally, consider adding a code example in the documentation to show how to use this filter, as it's a new extension point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Order/Hooks.php(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Order/Hooks.php
[warning] 423-423:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
[warning] 430-430:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
includes/Order/Hooks.php (3)
6-7: LGTM! Well-organized use statements.The new use statements for
WC_CouponandWC_Discountsare properly added and organized.
413-420: Update DOKAN_SINCE placeholder.The PHPDoc block still contains the placeholder
DOKAN_SINCE. This should be replaced with the actual version number where this change is being introduced.
422-448: Verify backward compatibility of the coupon validation changes.The refactored coupon validation logic might impact existing implementations. Let's verify there are no breaking changes:
✅ Verification successful
Coupon validation changes enhance functionality without breaking compatibility
The changes introduce better handling of fixed cart coupons with multiple vendors and provide improved extensibility through the
dokan_coupon_is_validfilter. No active implementations of the old method were found that could be impacted by these changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing implementations of the old method name rg "ensure_vendor_coupon" -l # Search for existing filter usage rg "add_filter.*dokan_coupon_is_valid" -lLength of output: 109
Script:
#!/bin/bash # Check content of files mentioning the old method echo "=== Content from whats-new.php ===" rg -A 5 -B 5 "ensure_vendor_coupon" templates/whats-new.php echo -e "\n=== Content from CHANGELOG.md ===" rg -A 5 -B 5 "ensure_vendor_coupon" CHANGELOG.md # Search for broader coupon-related filters echo -e "\n=== Coupon-related filters ===" rg "apply_filters.*dokan.*coupon" -A 2 # Search for coupon-related hooks echo -e "\n=== Coupon-related actions ===" rg "do_action.*dokan.*coupon" -A 2Length of output: 2523
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 423-423:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
[warning] 430-430:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
All Submissions:
Changes proposed in this Pull Request:
This PR refactors the coupon validation logic in Dokan by:
ensure_vendor_coupon()toensure_is_coupon_valid()for better claritydokan_coupon_is_validthat provides more flexibility for extending coupon validationRelated Pull Request(s)
Closes
How to test the changes in this PR:
dokan_coupon_is_validfilterChangelog entry
dokan_coupon_is_validfilter hook for custom coupon validation rulesBefore Changes:
After Changes:
PR Self Review Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
dokan_coupon_is_validfor custom coupon validation.Refactor
ensure_vendor_coupontoensure_coupon_is_valid.