refactor: Lazy load next strategy #2684
Conversation
WalkthroughThis update introduces a comprehensive refactor and modernization of the commission calculation system in the codebase. The legacy strategy and formula-based commission logic is replaced with dedicated, encapsulated classes such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Container
participant OrderCommission
participant OrderLineItemCommission
participant ProductCommission
Client->>Controller: Request commission calculation
Controller->>Container: Get OrderCommission/ProductCommission
Container-->>Controller: Returns service instance
Controller->>OrderCommission: set_order(order)
Controller->>OrderCommission: calculate()
OrderCommission->>OrderLineItemCommission: calculate() for each item
OrderLineItemCommission-->>OrderCommission: Returns commission data
OrderCommission-->>Controller: Returns aggregated commission data
Controller-->>Client: Respond with commission results
Possibly related PRs
Suggested labels
Poem
✨ 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. 🪧 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 (
|
mrabbani
changed the base branch from
develop
to
feat/adjust-commision-for-refund
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (41)
includes/Order/Admin/Hooks.php (1)
168-176: 🛠️ Refactor suggestion
Log the root cause instead of silently swallowing the exception
If
OrderCommissionthrows, the catch block falls back to0without emitting any diagnostics, making operational issues almost impossible to triage.
For consistency with the latercommission_meta_box()implementation (wheredokan_log()is used), please add at least a warning-level log entry here.- } catch ( Exception $e ) { - $commission = 0; + } catch ( Exception $e ) { + dokan_log( + sprintf( + 'Failed to calculate admin commission for order #%d : %s', + $order->get_id(), + $e->getMessage() + ) + ); + $commission = 0; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { $order_commission = dokan_get_container()->get( OrderCommission::class ); $order_commission->set_order( $order ); $order_commission->get(); $commission = $order_commission->get_admin_commission(); } catch ( Exception $e ) { dokan_log( sprintf( 'Failed to calculate admin commission for order #%d : %s', $order->get_id(), $e->getMessage() ) ); $commission = 0; }includes/Commission/Strategies/Vendor.php (2)
49-57:
⚠️ Potential issue
set_next()should return the next strategy, not$thisReturning
$thisbreaks fluent-chaining semantics:$vendor->set_next()->set_next(); // will repeatedly call Vendor::set_next(), never GlobalStrategy::set_next()Change the return to
$this->next:- return $this; + return $this->next;This aligns with the documented
@inheritDocreturn type (AbstractStrategy).
87-93: 🛠️ Refactor suggestion
Method name, docblock and return value are mis-aligned
- The method is called
set_settings()but returns the$settingsobject. Either rename back toget_settings()or keep the current name and drop the return.- Docblock declares
@return voidwhile a value is returned.- Adding a return type-hint (
: voidor: VendorSetting) will let static analysers catch accidental misuse.Example fix if the intent is “setter only”:
- public function set_settings() { + public function set_settings(): void { $settings = new \WeDevs\Dokan\Commission\Settings\Vendor( $this->vendor_id ); $this->settings = $settings->get(); - return $settings; }includes/Commission/Settings/Product.php (1)
69-74: 🛠️ Refactor suggestion
⚠️ Potential issueFiltered settings ignored & variable shadowing introduces bugs
$settings = apply_filters( ... )correctly lets extensions mutate the payload, but the next three lines still read from the original$settingarray.
➔ Any change done by the filter is silently discarded.
$settingsis reused later for theSettingobject, shadowing the filtered array and making the code harder to follow.- $settings = apply_filters( 'dokan_product_commission_settings_before_save', $setting, $this->product ); - - $commission_percentage = $setting['percentage'] ?? ''; - $commission_type = $setting['type'] ?? ''; - $additional_flat = $setting['flat'] ?? ''; + $settings = apply_filters( 'dokan_product_commission_settings_before_save', $setting, $this->product ); + + // read from the filtered array + $commission_percentage = $settings['percentage'] ?? ''; + $commission_type = $settings['type'] ?? ''; + $additional_flat = $settings['flat'] ?? '';
- Consider renaming the first
$settingsvariable (e.g.$filtered) or the laterSettinginstance (e.g.$commission) to eliminate shadowing.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$settings = apply_filters( 'dokan_product_commission_settings_before_save', $setting, $this->product ); - $commission_percentage = $setting['percentage'] ?? ''; - $commission_type = $setting['type'] ?? ''; - $additional_flat = $setting['flat'] ?? ''; + // read from the filtered array + $commission_percentage = $settings['percentage'] ?? ''; + $commission_type = $settings['type'] ?? ''; + $additional_flat = $settings['flat'] ?? '';includes/Commission/AbstractCommissionCalculator.php (1)
10-10:
⚠️ Potential issueFatal error: property redeclaration
Commission(the parent) already declares
protected Setting $settings;Redeclaring the same property in the child class triggers a fatal error in PHP >= 7.4:
Cannot redeclare property WeDevs\Dokan\Commission\AbstractCommissionCalculator::$settings
- protected Setting $settings;Simply delete this line—the inherited property and its getters/setters are sufficient.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.includes/Analytics/Reports/Orders/Stats/DataStore.php (1)
158-173: 💡 Verification agent
🧩 Analysis chain
Redundant variables & dead legacy code
The array now exclusively uses
$order_commissionvalues. Earlier variables such as$vendor_earning,$admin_earning,$gateway_fee, etc., set above, are no longer referenced. Keeping them:• wastes CPU cycles on every order update
• risks future confusion (“which value is authoritative?”)Recommend deleting lines 110-128 and any now-unused variables.
Run this to confirm no remaining usages:
🏁 Script executed:
#!/bin/bash rg -n "(vendor_earning|admin_earning|gateway_fee|shipping_fee_recipient)" includes/Analytics/Reports/Orders/Stats/DataStore.phpLength of output: 1035
Remove dead legacy variables in DataStore.php
The pre-array assignments for
$vendor_earning,$admin_earning,$gateway_fee,$gateway_fee_provider, and$shipping_fee_recipientare no longer referenced by the final data payload. They can be safely deleted to eliminate dead code and avoid confusion.• File:
includes/Analytics/Reports/Orders/Stats/DataStore.php
• Delete lines 110–128 (all assignments to those five variables)- $vendor_earning = 0; - $admin_earning = 0; - $gateway_fee = 0; - $gateway_fee_provider = ''; - $shipping_fee_recipient = ''; - … - $vendor_earning = (float) dokan()->commission->get_earning_by_order( $order ); - $admin_earning = (float) dokan()->commission->get_earning_by_order( $order, 'admin' ); - $gateway_fee = $order->get_meta( 'dokan_gateway_fee' ); - $gateway_fee_provider = $order->get_meta( 'dokan_gateway_fee_paid_by' ); - $shipping_fee_recipient = $order->get_meta( 'shipping_fee_recipient' ); - … - $shipping_fee_recipient = $parent_order->get_meta( 'shipping_fee_recipient' );Committable suggestion skipped: line range outside the PR's diff.
tests/php/src/Coupon/CouponTest.php (1)
177-194:
⚠️ Potential issueEmail restriction test case has incorrect expected discount.
The test case for email restrictions (
email-10) expects a discount of 0, but the comment indicates "Customer email is matched, discount 10". Since the customer email is matched and the coupon has a fixed_product discount of 10, the expected discount should likely be 30 (10 per item × 3 items) rather than 0.Correct the expected discount value to match the scenario description:
- 'discount' => 0, // Customer email is matched, discount 10 + 'discount' => 30, // Customer email is matched, discount 10 per item for 3 items📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.[ 'coupons' => [ [ 'code' => 'email-10', 'status' => 'publish', 'meta' => [ 'discount_type' => 'fixed_product', 'coupon_amount' => 10, 'customer_email' => [ 'customer@gmail.com' ], ], ], ], 'order_items' => $order_items, ], [ - 'discount' => 0, // Customer email is matched, discount 10 + 'discount' => 30, // Customer email is matched, discount 10 per item for 3 items ],includes/Commission/Strategies/Product.php (1)
41-53: 🛠️ Refactor suggestion
Return the actual next strategy to respect Chain-of-Responsibility semantics
set_next()builds the fallback strategy but currently returns$this.
Returning$this->nextis the common convention and allows fluent chaining with the concrete successor.- return $this; + return $this->next;tests/php/src/Commission/CommissionCalculatorTest.php (2)
49-60: 🛠️ Refactor suggestion
Same precision concern for the refund assertions
Swap to
assertEqualsWithDelta()(orassertSameonround()) to avoid flaky tests.
32-38: 🛠️ Refactor suggestion
Use
assertEqualsWithDelta()for floating-point comparisonsDirect equality on floats is brittle due to rounding errors.
- $this->assertEquals( 67.792, $comm->get_admin_commission() ); + $this->assertEqualsWithDelta( 67.792, $comm->get_admin_commission(), 0.0001 );Repeat for every float assertion below (
test_calculate_for_refund).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$comm = $calc->set_subtotal( 880 )->set_quantity( 10 ) ->set_discount( $coupon_info ) ->set_settings( $settings ) ->calculate(); $this->assertEqualsWithDelta( 67.792, $comm->get_admin_commission(), 0.0001 ); }includes/Commission/CouponInfo.php (4)
91-97: 🛠️ Refactor suggestion
Inconsistent keyword:
'percentage'vs'percent'
admin_shared_coupon_typedefault is'percent', but the check expects'percentage'.
Pick one spelling to avoid falling through to the fixed-amount branch.- if ( 'percentage' === $coupon_info['admin_shared_coupon_type'] ) { + if ( 'percent' === $coupon_info['admin_shared_coupon_type'] ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( 'percent' === $coupon_info['admin_shared_coupon_type'] ) { $admin = ( $coupon_info['discount'] * $coupon_info['admin_shared_coupon_amount'] ) / 100; $vendor = $coupon_info['discount'] - $admin; } else { $admin = min( $coupon_info['discount'], $coupon_info['admin_shared_coupon_amount'] ); $vendor = $coupon_info['discount'] - $admin; }
8-9:
⚠️ Potential issueProperties should be initialised to avoid “uninitialised typed property” errors
Typed properties without a default value trigger a fatal error the first time they are accessed.
Give them an explicitnulldefault.- protected ?float $vendor_discount; - protected ?float $admin_discount; + protected ?float $vendor_discount = null; + protected ?float $admin_discount = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.protected ?float $vendor_discount = null; protected ?float $admin_discount = null;
21-25:
⚠️ Potential issueEnsure cached discounts are reset when
set_info()is called
set_info()overwrites$this->infobut does not invalidate$vendor_discount/$admin_discount.
If the instance is reused with new coupon data, stale discount values will be returned.public function set_info( array $info ): CouponInfo { $this->info = $info; + + // Invalidate cached amounts + $this->vendor_discount = null; + $this->admin_discount = null; return $this; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public function set_info( array $info ): CouponInfo { $this->info = $info; // Invalidate cached amounts $this->vendor_discount = null; $this->admin_discount = null; return $this; }
72-76: 💡 Verification agent
🧩 Analysis chain
Default
coupon_commissions_typevalue does not match the switch casesThe fallback value
'vendor'is never handled in theswitch, so the discount would silently be0.
Use one of the recognised keys (probably'default'or'from_vendor').- 'coupon_commissions_type' => 'vendor', + 'coupon_commissions_type' => 'default',Also applies to: 82-89
🏁 Script executed:
#!/bin/bash # Display default config and the switch block handling coupon_commissions_type sed -n '68,140p' includes/Commission/CouponInfo.phpLength of output: 1464
Fix default
coupon_commissions_typeto a recognised keyThe default value
'vendor'isn’t handled in the switch, so neither admin nor vendor ever receive the discount. Change it to one of the cases (e.g.'default').• File:
includes/Commission/CouponInfo.php
• Lines: 72–76 (also apply at 82–89)- 'coupon_commissions_type' => 'vendor', + 'coupon_commissions_type' => 'default',includes/Commission/Strategies/OrderItem.php (2)
44-48:
⚠️ Potential issueValidate constructor arguments to prevent fatal errors
$order_itemdefaults to an empty string, yet it is dereferenced immediately.
Either make the parameter mandatory or guard against invalid input.-public function __construct( $order_item = '', $vendor_id = 0 ) { - $this->order_item = $order_item; +public function __construct( \WC_Order_Item_Product $order_item, int $vendor_id = 0 ) { + $this->order_item = $order_item;Add an early check:
if ( ! $order_item instanceof \WC_Order_Item_Product ) { throw new \InvalidArgumentException( 'OrderItem strategy expects a WC_Order_Item_Product instance' ); }
22-25: 🛠️ Refactor suggestion
Strengthen typing for
$order_itempropertyUsing a loosely-typed property invites runtime fatals if a non-
WC_Order_Item_Productslips through.
Declare the property and add a native type-hint.- protected $order_item; + protected \WC_Order_Item_Product $order_item;includes/Commission/ProductCommission.php (2)
71-75:
⚠️ Potential issueHandle missing or invalid product gracefully
wc_get_product()may returnfalse.
Add a guard before dereferencing to avoid fatal errors.$product = wc_get_product( $this->product_id ); -if ( $product ) { - $this->total_amount = floatval( $product->get_price() ); -} +if ( ! $product ) { + throw new DokanException( esc_html__( 'Invalid product ID.', 'dokan-lite' ) ); +} +$this->total_amount = floatval( $product->get_price() );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( empty( $this->total_amount ) && ! empty( $this->product_id ) ) { $product = wc_get_product( $this->product_id ); if ( ! $product ) { throw new DokanException( esc_html__( 'Invalid product ID.', 'dokan-lite' ) ); } $this->total_amount = floatval( $product->get_price() ); }
20-24:
⚠️ Potential issueInitialise nullable typed properties
Accessing an uninitialised typed property throws a fatal error in PHP 8+.
Assignnulldefaults.- protected ?int $product_id; - protected ?int $category_id; - protected ?int $vendor_id; + protected ?int $product_id = null; + protected ?int $category_id = null; + protected ?int $vendor_id = null;tests/php/src/Commission/OrderCommission.php (2)
36-49:
⚠️ Potential issueExisting categories are ignored – breaks deterministic index → ID mapping
$ids[]is only populated when the category does not already exist.
If a test run (or another suite) has pre-createdCategory_1 … Category_10, the loop skips all inserts, leaving$this->category_idsempty.
Subsequent look-ups with$this->category_ids[$index]will throw an “Undefined offset” notice and the test will fail.foreach ( $categories as $category_name ) { - if ( ! term_exists( $category_name, 'product_cat' ) ) { - $term = wp_insert_term( … ); - $ids[] = $term['term_id']; - } + $term_id = term_exists( $category_name, 'product_cat' ); + if ( ! $term_id ) { + $term = wp_insert_term( … ); + $term_id = $term['term_id']; + } elseif ( is_array( $term_id ) ) { + $term_id = $term_id['term_id']; + } + + $ids[] = $term_id; }This guarantees that the index order (0 →
Category_1, 1 →Category_2, …) is preserved regardless of pre-existing data.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.foreach ( $categories as $category_name ) { $term_id = term_exists( $category_name, 'product_cat' ); if ( ! $term_id ) { $term = wp_insert_term( $category_name, // the term 'product_cat', // the taxonomy [ 'description' => 'Description for ' . $category_name, 'slug' => sanitize_title( $category_name ), ] ); $term_id = $term['term_id']; } elseif ( is_array( $term_id ) ) { $term_id = $term_id['term_id']; } $ids[] = $term_id; }
341-349: 💡 Verification agent
🧩 Analysis chain
Mutating
$expectedinside the helper has no effectThe function adjusts
$expectedto add shipping/tax values, but $expected is not returned and therefore the caller still uses the unmodified array when asserting.
This silently makes the test succeed only if shipping/tax recipients are always vendor (so the numbers cancel).Solution: either:
- Remove the mutation and expect the data provider to include the correct totals, or
- Return the patched
$expectedand use it in the caller.Option 1 shown below:
- 'admin' === dokan()->fees->get_shipping_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_cost : $expected['vendor_earning'] += $shipping_cost; - … - 'admin' === dokan()->fees->get_shipping_tax_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_tax_cost : $expected['vendor_earning'] += $shipping_tax_cost; +// No need to patch $expected – provider must contain the final numbers.Run the tests after patching to ensure the expected values are still correct.
If they fail, update the data-provider numbers accordingly.
🏁 Script executed:
#!/bin/bash # Display the context around lines 341-349 to verify the helper function and check for return statements. sed -n '330,380p' tests/php/src/Commission/OrderCommission.phpLength of output: 2141
🏁 Script executed:
#!/bin/bash # Locate the OrderCommission class file and display its get(), get_admin_total_earning, and get_vendor_total_earning methods. fd OrderCommission.php # Assuming the file is under src/Commission FILE_PATH=$(fd OrderCommission.php | head -n1) echo "Found at: $FILE_PATH" # Display relevant method implementations sed -n '1,200p' "$FILE_PATH" grep -R "function get(" -n "$FILE_PATH" grep -R "get_admin_total_earning" -n "$FILE_PATH" grep -R "get_vendor_total_earning" -n "$FILE_PATH"Length of output: 6889
Remove ineffective
$expectedmutations in the helperThe shipping/tax adjustment lines in
create_order_with_coupon(tests/php/src/Commission/OrderCommission.php) mutate$expectedbut never return it—so they have no effect on the values actually asserted.• File:
tests/php/src/Commission/OrderCommission.php
• Lines: ~340–348Suggested change:
- 'admin' === dokan()->fees->get_shipping_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_cost : $expected['vendor_earning'] += $shipping_cost; - 'admin' === dokan()->fees->get_tax_fee_recipient( $order->get_id() ) ? $expected['admin_commission'] += $tax_cost : $expected['vendor_earning'] += $tax_cost; - 'admin' === dokan()->fees->get_shipping_tax_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_tax_cost : $expected['vendor_earning'] += $shipping_tax_cost; + // Removed – data provider must supply the final admin_commission and vendor_earning.Please remove these lines (or alternatively return the patched
$expected) and then run the full test suite to confirm correctness across all fee-recipient scenarios.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$shipping_cost = wc_format_decimal( floatval( $order->get_shipping_total() ) ) - $order->get_total_shipping_refunded(); - 'admin' === dokan()->fees->get_shipping_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_cost : $expected['vendor_earning'] += $shipping_cost; - 'admin' === dokan()->fees->get_tax_fee_recipient( $order->get_id() ) ? $expected['admin_commission'] += $tax_cost : $expected['vendor_earning'] += $tax_cost; - 'admin' === dokan()->fees->get_shipping_tax_fee_recipient( $order ) ? $expected['admin_commission'] += $shipping_tax_cost : $expected['vendor_earning'] += $shipping_tax_cost; + // Removed – data provider must supply the final admin_commission and vendor_earning. $tax_cost = ( ( $order->get_total_tax() - $order->get_total_tax_refunded() ) - ( $order->get_shipping_tax() - dokan()->fees->get_total_shipping_tax_refunded( $order ) ) ); $shipping_tax_cost = ( $order->get_shipping_tax() - dokan()->fees->get_total_shipping_tax_refunded( $order ) );includes/Commission/Formula/AbstractFormula.php (1)
24-29:
⚠️ Potential issueUninitialised typed property
$percentwill throw on first access
Unlike$fixedand$combinedFixed,$percentlacks a default. Accessing it before initialisation triggers aTyped property must not be accessed before initializationerror in PHP 7.4+.- protected float $percent; + /** Percentage commission figure (0-100). */ + protected float $percent = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** Percentage commission figure (0-100). */ protected float $percent = 0; protected float $fixed = 0; protected float $combinedFixed = 0;includes/Commission/Calculator.php (2)
94-103: 🛠️ Refactor suggestion
⚠️ Potential issuePrevent negative admin commission / inflated vendor earnings
Scenario:
If($percentage + $flat + $combine_flat) < $admin_discount,$raw_admin_commissionbecomes negative.
min( $raw_admin_commission, $net_amount )will then return the negative number, causing:$vendor_earning = $net_amount - $admin_commission; // ➜ larger than $net_amountThis is financially incorrect and may credit vendors more than the customer paid.
- $admin_commission = min( $raw_admin_commission, $net_amount ); + $admin_commission = max( 0, min( $raw_admin_commission, $net_amount ) );Consider adding unit tests for this edge case.
5-15:
⚠️ Potential issueAdd missing
usestatement forCouponInfo
Calculatordefines a?CouponInfo $discountproperty (l.14) but never imports the class.
UnlessCouponInfolives in the same namespace, PHP will raise a fatal error the first time this file is loaded.use WeDevs\Dokan\Commission\Model\Setting; +use WeDevs\Dokan\Commission\CouponInfo;includes/Commission/OrderLineItemCommission.php (1)
5-10:
⚠️ Potential issueImport
CouponInfoto avoid runtime error
calculate()instantiatesnew CouponInfo(...)(l.138) but the class is not imported.
Add the missingusestatement:use WeDevs\Dokan\Vendor\Coupon; +use WeDevs\Dokan\Commission\CouponInfo;includes/Commission/RecalculateCommissions.php (2)
16-23:
⚠️ Potential issueEarly
returndisables the whole constructor
return;on l.18 exits before any hooks are registered, effectively turning the class into a no-op.
If this is intentional for staging it should be documented and the unused code removed; otherwise, delete thereturnso recalculation logic runs.
231-235:
⚠️ Potential issueMissing import for
WC_Subscription
instanceof WC_Subscription(l.233) is used but the class is not imported or fully-qualified.
Add:use WC_Order_Refund; +use WC_Subscription;to avoid a fatal error when subscriptions are active.
includes/Vendor/DokanOrderLineItemCouponInfo.php (4)
161-165:
⚠️ Potential issueSetter allows only
int, but getter casts tofloat
set_per_qty_amount( int $per_qty_amount )conflicts with
get_per_qty_amount(): float.If fractions ( e.g. 3.33 USD / qty ) are ever stored, the current signature will silently truncate.
- public function set_per_qty_amount( int $per_qty_amount ): self { + public function set_per_qty_amount( float $per_qty_amount ): self {
253-256: 🛠️ Refactor suggestion
Getter mutates state – surprising side-effect
get_admin_shared_coupon_type()changes$this->admin_shared_coupon_typewhen subsidy is unsupported.
A getter should never mutate; compute the “resolved” value locally instead.- if ( ! $this->is_subsidy_supported() && $this->admin_shared_coupon_type === 'default') { - $this->admin_shared_coupon_type = 'vendor'; - } - - return $this->admin_shared_coupon_type; + $type = $this->admin_shared_coupon_type; + if ( ! $this->is_subsidy_supported() && $type === 'default' ) { + $type = 'vendor'; + } + return $type;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$type = $this->admin_shared_coupon_type; if ( ! $this->is_subsidy_supported() && $type === 'default' ) { $type = 'vendor'; } return $type;
7-20: 🛠️ Refactor suggestion
Inconsistent & loosely-typed property declarations
A few scalar properties that represent monetary values are declared without scalar type-hints (e.g.
''instead of0.0).
•$per_qty_amountand$admin_shared_coupon_amountare monetary amounts but are initialised as strings/ints.
•$coupon_commissions_typeand$admin_shared_coupon_typeare meant to be enums, yet any string can be assigned.Strong typing will avoid accidental assignment/usage bugs and improve static analysis.
- private $per_qty_amount = 0; + private float $per_qty_amount = 0.0; - private $admin_shared_coupon_amount = ''; + private float $admin_shared_coupon_amount = 0.0;Also add
stringtype-hints for the enum-like properties.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private $discount = 0; private $coupon_code = ''; private float $per_qty_amount = 0.0; private $quantity = 0; private $admin_coupons_enabled_for_vendor = ''; /** * Expected types are 'from_vendor' or 'from_admin' or 'shared' or '' * @var string Coupon commission type */ private $coupon_commissions_type = ''; private $admin_shared_coupon_type = ''; private float $admin_shared_coupon_amount = 0.0; private bool $subsidy_supported = true;
299-306:
⚠️ Potential issueVendor discount logic missing
Doc-block states vendor gets full discount only for
from_vendor,default, or empty types, but the method unconditionally returns full discount (unless a filter overrides). Implement baseline logic before the filter to avoid incorrect payouts when the type isfrom_admin/shared.- $discount = $this->get_discount(); + $discount = $this->get_discount(); + + // Baseline split + if ( $this->coupon_commissions_type === 'from_admin' ) { + $discount = 0.0; + } elseif ( $this->coupon_commissions_type === 'shared' ) { + $discount = $discount / 2; // or pull ratio from settings + }includes/Order/RefundHandler.php (2)
209-213: 🛠️ Refactor suggestion
Default refund reason never used
Current logic overwrites the refund reason only when one already exists – the opposite of the intent.
- if ( $refund_reason ) { - $refund_reason = __( 'Refunded by Dokan', 'dokan-lite' ); - } + if ( empty( $refund_reason ) ) { + $refund_reason = __( 'Refunded by Dokan', 'dokan-lite' ); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ( empty( $refund_reason ) ) { $refund_reason = __( 'Refunded by Dokan', 'dokan-lite' ); } $wpdb->insert(
189-195:
⚠️ Potential issueFatal error – missing parentheses on
get_id
$refund_order->get_idis a property access;WC_Order_Refundexposes it as a method.- $refund_order->get_id, $vendor_refund_amount + $refund_order->get_id(), $vendor_refund_amountincludes/Vendor/Coupon.php (1)
153-157:
⚠️ Potential issue
WC()->cartmay be null
save_coupon_data_to_cart_itemassumes a session Cart context. When coupons are applied programmatically (e.g. REST API, admin),WC()->cartcan benull, triggering fatal errors.Guard the call:
$cart = WC()->cart; if ( ! $cart ) { return; }includes/Commission/Strategies/AbstractStrategy.php (2)
101-108: 🛠️ Refactor suggestion
Saving settings only when the current strategy is
OrderItemStrategyloses data
save_settings_to_order_item()skips saving when the eligible strategy lives deeper in the chain.
Typical entry point calls this on the root strategy, so order-item settings found in a child strategy (e.g., Product) are silently discarded.Consider:
- $settings = $this->get_settings(); - if ( ! $settings || ! $this instanceof OrderItemStrategy ) { + $eligible = $this->get_eligible_strategy(); + if ( ! $eligible instanceof OrderItemStrategy ) { return; } - $this->get_order_item_setting_saver( $order_item )->save( $settings->to_array() ); + $this->get_order_item_setting_saver( $order_item ) + ->save( $eligible->settings->to_array() );Committable suggestion skipped: line range outside the PR's diff.
12-18: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid calling an overridable method from the constructor
__construct()invokes the abstractset_settings()before the concrete subclass is fully constructed.
If a subclass relies on its own properties to build theSettingobject, those properties will still be un-initialized whenset_settings()is executed, leading to subtle bugs or fatal errors.Recommended pattern:
- public function __construct() { - $this->set_settings(); - } + final public function __construct() { + $this->init_settings(); + } + + private function init_settings(): void { + $this->settings = $this->create_settings(); + } + + /** + * Sub-classes MUST implement this instead of `set_settings()`. + */ + abstract protected function create_settings(): Setting;This prevents override-before-construct while still delegating the actual instantiation to the child class.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.protected ?AbstractStrategy $next = null; protected ?Setting $settings; final public function __construct() { $this->init_settings(); } private function init_settings(): void { $this->settings = $this->create_settings(); } /** * Sub-classes MUST implement this instead of `set_settings()`. */ abstract protected function create_settings(): Setting;tests/php/src/Commission/OrderCommissionTest.php (1)
32-42: 🛠️ Refactor suggestion
Floating-point assertions should tolerate precision error
assertEquals()performs a loose comparison but still fails on tiny float discrepancies.
UseassertEqualsWithDelta()(PHPUnit ≥ 7.5) orassertSame( round(), … )to avoid brittle tests.Example:
- $this->assertEquals( $expected['total'], $order->get_total(), 'Order total should be equal to expected' ); + $this->assertEqualsWithDelta( $expected['total'], $order->get_total(), 0.01, 'Order total mismatch' );Repeat for other monetary comparisons.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$this->assertEqualsWithDelta( $expected['total'], $order->get_total(), 0.01, 'Order total mismatch' ); $order_commission = dokan_get_container()->get( \WeDevs\Dokan\Commission\OrderCommission::class ); $order_commission->set_order( $order ); $order_commission->calculate(); $this->assertEquals( $expected['commission'], $order_commission->get_admin_commission(), 'Admin commission should be equal to expected' ); $this->assertEquals( $expected['net_commission'], $order_commission->get_admin_net_commission(), 'Admin net commission should be equal to expected' ); $this->assertEquals( $expected['vendor_earnings'], $order_commission->get_vendor_earning(), 'Vendor earnings should be equal to expected' ); $this->assertEquals( $expected['vendor_net_earnings'], $order_commission->get_vendor_net_earning(), 'Vendor net earnings should be equal to expected' );includes/Commission.php (1)
541-571: 🛠️ Refactor suggestion
Dead parameters & missing validation in
get_commission()
$auto_saveand$override_total_amount_by_product_priceare no longer used – either implement or deprecate them formally to avoid API confusion.WC_Order_Factory::get_order_item()may returnfalse; accessing methods on a non-object will fatal.Suggested guard:
$order_item = WC_Order_Factory::get_order_item( $order_item_id ); - try { + if ( ! $order_item ) { + return new WP_Error( 'invalid_order_item', __( 'Invalid order item ID', 'dokan-lite' ) ); + } + try {Also document the updated behaviour in the docblock.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public function get_commission( $args = [], $auto_save = false, $override_total_amount_by_product_price = true ) { $order_item_id = ! empty( $args['order_item_id'] ) ? $args['order_item_id'] : ''; $total_amount = ! empty( $args['total_amount'] ) ? $args['total_amount'] : 0; $product_id = ! empty( $args['product_id'] ) ? $args['product_id'] : 0; $vendor_id = ! empty( $args['vendor_id'] ) ? $args['vendor_id'] : ''; $category_id = ! empty( $args['category_id'] ) ? $args['category_id'] : 0; if ( $order_item_id ) { $order_item = WC_Order_Factory::get_order_item( $order_item_id ); if ( ! $order_item ) { return new WP_Error( 'invalid_order_item', __( 'Invalid order item ID', 'dokan-lite' ) ); } try { $line_item_commission = dokan_get_container()->get( OrderLineItemCommission::class ); $line_item_commission->set_order( $order_item->get_order() ); $line_item_commission->set_item( $order_item ); return $line_item_commission->calculate(); } catch ( \Exception $e ) { dokan_log( $e->getMessage() ); } } else { try { $product_commission = dokan_get_container()->get( ProductCommission::class ); $product_commission->set_product_id( $product_id ); $product_commission->set_category_id( $category_id ); $product_commission->set_total_amount( $total_amount ); $product_commission->set_vendor_id( $vendor_id ); return $product_commission->calculate(); } catch ( \Exception $e ) { dokan_log( $e->getMessage() ); } } }includes/Commission/Model/Setting.php (1)
62-63:
⚠️ Potential issueInitialize
$sourceto prevent fatal “uninitialised typed property” errors
$sourceis a non-nullable typed property declared without a default value.
Any read access (e.g.to_array()orget_source()) beforeset_source()is invoked will raiseError: Typed property … must not be accessed before initialization.- protected string $source; + /** + * Holds the origin of the applied commission settings. + * + * @since 3.14.0 + * @var string + */ + protected string $source = '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Holds the origin of the applied commission settings. * * @since 3.14.0 * @var string */ protected string $source = '';includes/Commission/Model/Commission.php (1)
8-20:
⚠️ Potential issueTyped properties need default values (same fatal-error risk as above)
Every typed property is accessed before guaranteed initialisation through the
??operator, which still triggers a fatal error in PHP 7.4+.- protected float $admin_net_commission; - protected float $vendor_net_earning; - protected float $admin_discount; - protected float $vendor_discount; + protected float $admin_net_commission = 0.0; + protected float $vendor_net_earning = 0.0; + protected float $admin_discount = 0.0; + protected float $vendor_discount = 0.0;For
$settings, either:- protected Setting $settings; + protected ?Setting $settings = null;and make
get_settings()throw if stillnull, or document thatset_settings()must be called first.
Failing to address this will cause fatal errors in production.includes/Commission/OrderCommission.php (2)
19-19:
⚠️ Potential issueDefault-initialise
$orderReading
$this->orderanywhere (e.g. incalculate()) beforeset_order()is executed will crash for the same typed-property reason.- private ?WC_Order $order; + private ?WC_Order $order = null;
324-333:
⚠️ Potential issueUndefined variable
$processing_fee– should be$gateway_feeThe condition references an undeclared variable, so the branch will never execute and may trigger a notice.
- if ( ! empty( $processing_fee ) && empty( $gateway_fee_paid_by ) ) { + if ( ! empty( $gateway_fee ) && empty( $gateway_fee_paid_by ) ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private function get_dokan_gateway_fee() { $gateway_fee = $this->order->get_meta( 'dokan_gateway_fee', true ); $gateway_fee_paid_by = $this->order->get_meta( 'dokan_gateway_fee_paid_by', true ); if ( ! empty( $gateway_fee ) && empty( $gateway_fee_paid_by ) ) { /** * @since 3.7.15 dokan_gateway_fee_paid_by meta key returns empty value if gateway fee is paid admin */ $gateway_fee_paid_by = $this->order->get_payment_method() === 'dokan-stripe-connect' ? 'admin' : 'seller'; }
mrabbani
merged commit 463545c
into
feat/adjust-commision-for-refund
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
Bug Fixes
Refactor
Tests
Chores
Documentation