Fix: exclude POS cash orders from vendor withdrawable balance - #3079
Conversation
📝 WalkthroughWalkthroughReplaces hardcoded COD-exclusion checks with computed flags passed through two new filters— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![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
🤖 Fix all issues with AI agents
In `@includes/Order/Hooks.php`:
- Around line 128-138: Replace the placeholder `@since` DOKAN_SINCE with the
actual release version for this change, and update the docblock to generalize
COD-specific language: change the overall description to "Filter whether the
order should be excluded from vendor withdrawal balance" (already present) and
update the param descriptions for $should_exclude_cod_payment and
$exclude_cod_payment to say "Whether to exclude the order from the vendor
balance" (or similar generic wording) instead of referencing COD; keep the
WC_Order $order and int $order_id descriptions unchanged and ensure the param
names in the docblock match the actual parameter names used by the filter/hook
in Hooks.php.
🧹 Nitpick comments (1)
includes/Order/Hooks.php (1)
139-149: Normalize the filter result to a boolean.This avoids surprises if a hook returns a non-boolean truthy value.
Suggested change
-$should_exclude_cod_payment = apply_filters( +$should_exclude_cod_payment = (bool) apply_filters( 'dokan_order_should_exclude_from_vendor_balance', $should_exclude_cod_payment, $order, $order_id, $new_status, $exclude_cod_payment );
Shamim-97
added
Needs: Testing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@includes/Order/RefundHandler.php`:
- Line 90: Remove the trailing end-of-line whitespace from the docblock line
containing "@return bool" in the RefundHandler class (file
includes/Order/RefundHandler.php) so PHPCS no longer flags the EOL whitespace;
locate the docblock above the relevant method in the RefundHandler class and
delete the extra space character at the end of the "@return bool" line.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
includes/Order/RefundHandler.php (3)
210-210: Pre-existing bug: Missing method call parentheses.This line appears to have a pre-existing bug -
$refund_order->get_idshould be$refund_order->get_id(). While not introduced by this PR, it will cause a runtime error when this logging code is executed.🐛 Suggested fix
- $refund_order->get_id, $vendor_refund_amount + $refund_order->get_id(), $vendor_refund_amount
225-227: Pre-existing logic issue: Condition appears inverted.This pre-existing code overwrites the refund reason with a generic message when a reason already exists. The condition likely should be negated (
! $refund_reason) to set the default only when no reason is provided.🐛 Suggested fix
- if ( $refund_reason ) { + if ( ! $refund_reason ) { $refund_reason = __( 'Refunded by Dokan', 'dokan-lite' ); }
98-128: Address the misleading comment about filter name consistency.The implementation correctly uses two distinct filters for different contexts:
dokan_order_should_exclude_from_vendor_balance(Hooks.php) for order status updatesdokan_order_refund_should_exclude_from_vendor_balance(RefundHandler.php) for refund handlingHowever, the comment at line 109 states "Use the exact same filter name for consistency across the whole system" which is misleading since the filter name is actually different. Using separate filter names for these distinct contexts is appropriate design. Clarify the comment to reflect that this is a refund-specific filter with additional context parameters like
$refund_order.The implementation itself properly enables extensibility for wePOS and other plugins to override exclusion logic via the filter hook.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/Order/RefundHandler.php (1)
231-233: Pre-existing bug: Inverted condition overwrites valid refund reasons.This logic appears to be inverted. Currently, if
$refund_reasonis truthy (has a value), it gets overwritten with the default message. The condition should be negated to only set the default when the reason is empty.🐛 Proposed fix
- if ( $refund_reason ) { + if ( ! $refund_reason ) { $refund_reason = __( 'Refunded by Dokan', 'dokan-lite' ); }
🤖 Fix all issues with AI agents
In `@includes/Order/RefundHandler.php`:
- Around line 108-128: The docblock for the apply_filters call is wrong:
$exclude_cod_option is documented as string but is actually a bool (set via 'on'
=== dokan_get_option(...)); update the `@param` type for $exclude_cod_option to
bool and replace the placeholder `@since` DOKAN_SINCE with the real release
version, keeping the rest of the docblock and the apply_filters call
(dokan_order_refund_should_exclude_from_vendor_balance,
$should_exclude_cod_payment, $order, $order_id, $new_status,
$exclude_cod_option, $refund_order) unchanged so the signature and filter
behavior remain consistent.
| /** | ||
| * Apply the filter so other plugins (like wePOS) can override this. | ||
| * Use the exact same filter name for consistency across the whole system. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * @param bool $should_exclude_cod_payment Whether to exclude the payment. | ||
| * @param WC_Order $order The main WooCommerce order object. | ||
| * @param int $order_id The ID of the main order. | ||
| * @param string $new_status The new status of the order. | ||
| * @param string $exclude_cod_option The value of the 'exclude COD' setting. | ||
| * @param WC_Order $refund_order The specific refund order object. | ||
| */ | ||
| $should_exclude_cod_payment = apply_filters( | ||
| 'dokan_order_refund_should_exclude_from_vendor_balance', | ||
| $should_exclude_cod_payment, | ||
| $order, | ||
| $order_id, | ||
| $new_status, | ||
| $exclude_cod_option, | ||
| $refund_order, | ||
| ); |
There was a problem hiding this comment.
Docblock type mismatch for $exclude_cod_option parameter.
Line 117 documents $exclude_cod_option as string, but the variable is actually a bool (the result of 'on' === dokan_get_option(...)).
Also, ensure the @since DOKAN_SINCE placeholder is replaced with the actual version number before release.
📝 Proposed fix
* `@since` DOKAN_SINCE
* `@param` bool $should_exclude_cod_payment Whether to exclude the payment.
* `@param` WC_Order $order The main WooCommerce order object.
* `@param` int $order_id The ID of the main order.
* `@param` string $new_status The new status of the order.
- * `@param` string $exclude_cod_option The value of the 'exclude COD' setting.
+ * `@param` bool $exclude_cod_option Whether the 'exclude COD' setting is enabled.
* `@param` WC_Order $refund_order The specific refund order object.
*/🤖 Prompt for AI Agents
In `@includes/Order/RefundHandler.php` around lines 108 - 128, The docblock for
the apply_filters call is wrong: $exclude_cod_option is documented as string but
is actually a bool (set via 'on' === dokan_get_option(...)); update the `@param`
type for $exclude_cod_option to bool and replace the placeholder `@since`
DOKAN_SINCE with the real release version, keeping the rest of the docblock and
the apply_filters call (dokan_order_refund_should_exclude_from_vendor_balance,
$should_exclude_cod_payment, $order, $order_id, $new_status,
$exclude_cod_option, $refund_order) unchanged so the signature and filter
behavior remain consistent.
dev-shahed
added
🎉 QA Approved
MdAsifHossainNadim
added
Dev Review Done
and removed
Needs: Dev Review
3 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.