enhance: Add admin earning calculations based on order type checks.#3112
Conversation
… commission and analytics.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
mrabbani
left a comment
There was a problem hiding this comment.
Code Review
CRITICAL: Double-counting get_total_admin_fees() in get_admin_total_earning()
Location: includes/Commission/OrderCommission.php:501
get_admin_commission() already returns get_admin_net_commission() + get_total_admin_fees(). The new implementation adds get_total_admin_fees() again:
// Current get_admin_commission():
return $this->get_admin_net_commission() + $this->get_total_admin_fees();
// New get_admin_total_earning():
return $this->get_admin_net_earning() + $this->get_admin_commission() + $this->get_total_admin_fees();
// Expands to: admin_net_earning + (admin_net_commission + total_admin_fees) + total_admin_fees ← DOUBLEShould likely be:
return $this->get_admin_net_earning() + $this->get_admin_commission();Also, this method is marked @deprecated 4.0.0 but is now being given new behavior. The @deprecated tag should be removed if this is the correct method going forward.
ERROR: Copy-paste docblocks on Commission model
Location: includes/Commission/Model/Commission.php
Both $admin_net_commission and $admin_net_earning have the same docblock: "Vendor Earning without subsidy." — these should describe admin properties:
$admin_net_commission→ "Admin's net commission amount."$admin_net_earning→ "Admin's net earning from admin-type orders (subscriptions, advertisements, etc.)."
ERROR: CommissionInterface not updated
Location: includes/Commission/Contracts/CommissionInterface.php
The Commission model now has get_admin_net_earning() / set_admin_net_earning(), but CommissionInterface was not updated. Add get_admin_net_earning(): float to the interface contract.
WARNING: @since DOKAN_SINCE placeholder
Location: includes/Analytics/Reports/OrderType.php
Both new methods (get_admin_earning_order_types, is_admin_order_type) use @since DOKAN_SINCE — replace with the actual version before merge.
WARNING: Missing @since tags on new Commission model methods
set_admin_net_earning() and get_admin_net_earning() in includes/Commission/Model/Commission.php are missing @since annotations.
PR Template
- "Changes proposed" section is empty
- "How to test" has no actual steps
- "Changelog entry" is placeholder text
- "Closes #" is empty
Code ReviewBase: CRITICAL:
|
| File | Location |
|---|---|
includes/Analytics/Reports/OrderType.php |
get_admin_earning_order_types(), is_admin_order_type() |
includes/Commission/Contracts/CommissionInterface.php |
get_admin_net_earning() |
includes/Commission/Model/Commission.php |
set_admin_net_earning(), get_admin_net_earning() |
WARNING: is_admin_order_type() param type mismatch
Location: includes/Analytics/Reports/OrderType.php:280
public function is_admin_order_type( $order ): bool {Docblock says @param \WC_Order $order but the parameter has no type hint and get_type() accepts \WC_Abstract_Order. Should be type-hinted as \WC_Abstract_Order for consistency and safety.
PR Checklist
| Item | Status |
|---|---|
@since placeholders |
❌ Need real version numbers |
| Test instructions | ❌ "How to test" section is empty |
| Changelog entry | ❌ Placeholder text |
| Closes # | ❌ Empty |
| Changes proposed | ❌ Empty |
- Restore get_total_admin_fees() in get_admin_total_earning() formula to prevent silent exclusion of shipping, tax, gateway and order fees - Fix calculate_for_refund() to route refund amounts to admin_net_earning for admin-type orders (subscriptions, advertisements), matching the branching logic in calculate() - Replace direct `new OrderType()` with DI container resolution - Add admin_net_earning to get_data() return array - Add \WC_Abstract_Order type hint to is_admin_order_type() parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3641c8c to
98a9483
Compare
✅ Dev Review DoneAll previously reported issues have been addressed in commit
Remaining before merge:
Good to go ✅ |
…-upgrader' into enhance/update-admin-earnings-data-based-on-order-type
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: