Fix decimal and thousand separator for category commission. #2440
Conversation
WalkthroughThe changes in Changes
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: 0
🧹 Outside diff range and nitpick comments (2)
src/admin/components/Commission/CategoryBasedCommission.vue (2)
258-258: Fix typo in method nameThe method name has a typo: 'commissinItemHandler' should be 'commissionItemHandler'
- commissinItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) { + commissionItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {
367-393: LGTM: Well-structured utility methods with proper validationThe utility methods are well-implemented with appropriate handling of edge cases. However, consider adding try-catch blocks around the accounting.js calls to handle potential formatting errors gracefully.
formatValue: ( value ) => { if ( value === '' ) { return value; } - - return accounting.formatNumber( value, dokan.currency.precision, dokan.currency.thousand, dokan.currency.decimal ); + try { + return accounting.formatNumber( value, dokan.currency.precision, dokan.currency.thousand, dokan.currency.decimal ); + } catch (error) { + console.error('Error formatting value:', error); + return value; + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/admin/components/Commission/CategoryBasedCommission.vue(8 hunks)
🔇 Additional comments (3)
src/admin/components/Commission/CategoryBasedCommission.vue (3)
35-35: LGTM: Consistent value formatting implementation
The value formatting has been consistently implemented across all commission input fields, ensuring uniform display of decimal and thousand separators.
Also applies to: 52-52, 82-82, 99-100
111-112: LGTM: Added debounce for performance optimization
The addition of debouncing will help optimize performance by reducing the frequency of commission value updates during rapid input changes.
Line range hint 258-285: LGTM: Input handlers properly handle value formatting and validation
The implementation correctly:
- Validates percentage values (0-100 range)
- Unformats values before processing
- Uses debouncing to optimize performance
Also applies to: 287-297
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
includes/Product/Hooks.php (1)
494-494: Consider adding thestepattribute for decimal precision control.The min/max validation is good. Consider adding
step="0.01"to allow for precise decimal commission values while maintaining the HTML5 validation.- <input id="admin_commission" class="input-text wc_input_price" min="0" max="100" type="text" name="_per_product_admin_commission" value="<?php echo wc_format_localized_price( $admin_commission ); ?>"> + <input id="admin_commission" class="input-text wc_input_price" min="0" max="100" step="0.01" type="text" name="_per_product_admin_commission" value="<?php echo wc_format_localized_price( $admin_commission ); ?>">
includes/Product/Hooks.php
Outdated
| @@ -539,7 +539,7 @@ | |||
| update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type ); | |||
| } | |||
|
|
|||
| if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore | |||
| if ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve commission validation logic.
The current validation is embedded within the condition making it less readable. Consider separating the validation logic and adding numeric validation.
- if ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore
- $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
+ if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore
+ $commission = sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
+
+ if ( '' === $commission ) {
+ $admin_commission = '';
+ } elseif ( is_numeric( $commission ) && $commission >= 0 && $commission <= 100 ) {
+ $admin_commission = $commission;
+ }📝 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 ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore | |
| if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore | |
| $commission = sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore | |
| if ( '' === $commission ) { | |
| $admin_commission = ''; | |
| } elseif ( is_numeric( $commission ) && $commission >= 0 && $commission <= 100 ) { | |
| $admin_commission = $commission; | |
| } |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/admin/components/Commission/CategoryBasedCommission.vue (1)
111-111: Fix typo in method name and approve debouncing implementationThe debouncing implementation looks good and will help optimize performance during rapid inputs. However, there's a typo in the method name.
- commissinItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) { + commissionItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {Also applies to: 260-287, 289-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/admin/components/Commission/CategoryBasedCommission.vue(10 hunks)
🔇 Additional comments (4)
src/admin/components/Commission/CategoryBasedCommission.vue (4)
35-35: LGTM: Consistent value formatting across all commission inputs
The value bindings now properly utilize the formatValue method, ensuring consistent decimal and thousand separator formatting across percentage and flat commission inputs.
Also applies to: 52-52, 82-82, 99-99
261-266: LGTM: Proper validation of percentage values
The handlers now correctly validate percentage values within the 0-100 range and properly handle empty values, which should resolve the reported issue with percentage values greater than 10 not being stored.
Also applies to: 290-293
164-167: LGTM: Proper initialization of commission items
The initialization logic now ensures that commission.items is always an object, which should prevent the reported issue of null values being stored during setup.
369-395: LGTM: Well-implemented formatting and validation utilities
The new utility methods properly handle number formatting and validation while gracefully handling edge cases.
Let's verify the currency settings are properly initialized:
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/admin/pages/Settings.vue (1)
1122-1122: Consider using CSS Grid or Flexbox for better responsivenessThe current approach uses fixed widths and media queries. Consider using modern CSS layout techniques:
-width: 100%; +display: grid; +grid-template-columns: minmax(0, 1fr);This would provide better flexibility and maintainability across different screen sizes.
Also applies to: 1155-1155, 1171-1171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
includes/Admin/Settings.php(1 hunks)src/admin/components/CombineInput.vue(0 hunks)src/admin/components/Commission/CategoryBasedCommission.vue(11 hunks)src/admin/pages/Settings.vue(4 hunks)
💤 Files with no reviewable changes (1)
- src/admin/components/CombineInput.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/admin/components/Commission/CategoryBasedCommission.vue
🔇 Additional comments (3)
src/admin/pages/Settings.vue (2)
436-438: LGTM: Commission type validation enhancement
The code correctly removes the category-based commission validation error when the commission type is flat, percentage, combine, or fixed.
447-465: Verify the category-based commission validation logic
The validation for category-based commission values is complex and might have edge cases. Please ensure:
- All commission types are properly handled
- The validation works correctly when switching between commission types
- Error states are properly cleared when switching commission types
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the category-based commission validation logic. The code shows:
- Extensive test coverage for all commission types and scenarios in
tests/pw/tests/api/calculation.spec.ts - Proper validation handling in
src/admin/pages/Settings.vuethat checks:- Commission type switching
- All required fields for category-based commission
- Both percentage and flat rate values
- Clear error state management through
arrayRemoveandpushoperations - Proper data structure validation ensuring
commission_category_based_valueshas the correct shape
Category-based commission validation is properly implemented
The validation logic correctly:
- Handles switching between commission types by clearing irrelevant errors
- Validates both flat and percentage values for category-based commissions
- Maintains proper error states through array operations
- Ensures the data structure has required fields (all, flat, percentage)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in commission type handling
# Search for commission type related code
rg -A 5 "commission_type.*category_based"
# Search for commission validation related code
rg -A 10 "commission_category_based_values"
Length of output: 63476
includes/Admin/Settings.php (1)
541-547: LGTM! Field configuration aligns with commission handling requirements.
The changes to the commission_category_based_values field configuration look good:
- Added required validation
- Proper conditional display based on commission type
- Integration with pro features via
dokan_pro_commissionflag
Let's verify the commission type dependency:
✅ Verification successful
Let me generate additional scripts to verify the commission type dependency more thoroughly:
Based on the verification results, I can now provide the final response:
Commission type dependency is properly implemented and validated
The changes to the commission_category_based_values field configuration are correctly implemented with proper dependencies:
- The field is only shown when
commission_typeis set to 'category_based' - Proper sanitization and validation are in place for commission values
- The commission type relationship is consistently handled across the codebase
- Category-based commission values are properly stored and processed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify commission type handling and related fields
# Check for commission type references and validations
rg -A 5 "commission_type.*category_based"
# Look for any commission value formatting/validation logic
ast-grep --pattern 'function [^{]*commission[^{]*\{'
Length of output: 16308
Script:
#!/bin/bash
# Check for commission type field definition and its relationship with category commission
rg -A 10 "commission_type.*=.*\[" includes/
# Look for any validation or sanitization logic related to commission values
rg -A 5 "sanitize.*commission|validate.*commission" includes/
# Search for any commission type conditional logic
ast-grep --pattern 'if ($_ === "category_based") { $$$ }'
Length of output: 17916
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
includes/Commission.php (2)
585-592: Consider improving the documentation for future maintainability.While the documentation explains the edge case well, referencing a specific GitHub comment URL might make the documentation brittle if the URL becomes inaccessible. Consider including the key points from the GitHub comment directly in the documentation.
- * @see https://github.com/getdokan/dokan/pull/2440#issuecomment-2488159960 + * This handles edge cases such as: + * 1. When a 100% coupon is applied, making the total_amount zero + * 2. When calculating commission for a new product without an order
Line range hint
593-597: Add validation for product price.The current implementation could be improved by adding explicit validation for the product price. Consider handling edge cases like negative prices or invalid product objects.
if ( ! empty( $product_id ) && empty( $total_amount ) && empty( $order_item_id ) ) { $product = dokan()->product->get( $product_id ); - // If product price is empty the setting the price as 0 - $total_amount = $product && $product->get_price() && ! empty( $product->get_price() ) ? $product->get_price() : 0; + // Validate product and ensure price is non-negative + $price = $product && $product->get_price() ? $product->get_price() : 0; + $total_amount = max( 0, (float) $price ); }
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
|
@shashwatahalder01 vai this issue is already exists. this is not related for commission restructure. |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php(4 hunks)includes/Upgrade/Upgrades/V_3_14_0.php(2 hunks)
🔇 Additional comments (2)
includes/Upgrade/Upgrades/V_3_14_0.php (2)
7-8: LGTM: New commission formula imports
The imports for Flat and Percentage commission types are properly organized with existing commission formula imports.
21-35: Verify upgrade process safety and impact
This upgrade affects critical commission calculations. Please ensure:
- The upgrade process is atomic (all or nothing)
- There's a rollback mechanism in case of failure
- Existing commission calculations aren't adversely affected
✅ Verification successful
Let me gather more specific information about the upgrade process and rollback mechanisms.
Let me check one more aspect about the background process implementation and error handling.
Based on the gathered information, I can now provide a final response:
Commission upgrade process appears to be safely implemented
The upgrade process implementation shows several safety mechanisms:
- Uses WordPress background processing framework for handling large datasets
- Implements atomic operations per entity (global settings, vendor settings, product settings)
- Has proper status tracking through WordPress options
- Processes data in batches to prevent timeouts
- Each task returns boolean status for tracking success/failure
The commission type migration is handled systematically:
- Global settings are updated with proper value preservation
- Vendor commission settings are migrated individually
- Product-specific commissions are processed separately
- All calculations maintain numerical precision using WooCommerce's decimal handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential issues in commission calculations across the codebase
# Look for commission calculation references
echo "Checking commission calculation references..."
rg -l "admin_percentage|additional_fee|commission_type" --type php
# Check for existing error handling in commission calculations
echo "Checking error handling in commission calculations..."
ast-grep --pattern 'try { $$$ } catch'
# Look for number format handling in commission calculations
echo "Checking number format handling..."
rg "thousands_sep|decimal_point|number_format" --type php
Length of output: 7229
Script:
#!/bin/bash
# Check upgrade and rollback mechanisms
# Look for rollback mechanisms in the upgrade process
echo "Checking rollback mechanisms..."
rg -A 5 "rollback|revert" includes/Upgrade/
# Check atomic transaction handling in the upgrade
echo "Checking atomic transaction handling..."
rg -A 5 "begin_transaction|commit|rollback" includes/Upgrade/
# Check background process handling for V_3_14_0
echo "Checking background process implementation..."
cat includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php
# Check error handling in the upgrade process
echo "Checking upgrade error handling..."
rg -A 5 "try|catch|Exception" includes/Upgrade/Upgrades/V_3_14_0.php
Length of output: 5962
Script:
#!/bin/bash
# Check background process base class and error handling
# Check the background process abstract class
echo "Checking background process base implementation..."
cat includes/Abstracts/DokanBackgroundProcesses.php
# Check for error logging mechanisms
echo "Checking error logging mechanisms..."
rg -A 5 "dokan_log|error_log|WC_Logger" includes/Upgrade/
# Check if there are any upgrade status tracking mechanisms
echo "Checking upgrade status tracking..."
rg -A 5 "update_option.*upgrade|get_option.*upgrade" includes/Upgrade/
Length of output: 5344
| $options = get_option( 'dokan_selling', [] ); | ||
|
|
||
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | ||
| $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for commission values
The retrieved values from options should be validated and sanitized before use. Consider adding type casting and validation for admin_percentage.
- $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0;
+ $admin_percentage = isset( $options['admin_percentage'] ) ? floatval( $options['admin_percentage'] ) : 0;
+ if ( $admin_percentage < 0 ) {
+ $admin_percentage = 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.
| $options = get_option( 'dokan_selling', [] ); | |
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | |
| $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0; | |
| $options = get_option( 'dokan_selling', [] ); | |
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | |
| $admin_percentage = isset( $options['admin_percentage'] ) ? floatval( $options['admin_percentage'] ) : 0; | |
| if ( $admin_percentage < 0 ) { | |
| $admin_percentage = 0; | |
| } |
| if ( Flat::SOURCE === $commission_type ) { | ||
| $options['admin_percentage'] = 0; | ||
| $options['additional_fee'] = $admin_percentage; | ||
| } elseif ( Percentage::SOURCE === $commission_type ) { | ||
| $options['admin_percentage'] = $admin_percentage; | ||
| $options['additional_fee'] = 0; | ||
| } |
There was a problem hiding this comment.
Add error handling and number format conversion
The commission value updates need proper error handling and should consider number format conversion given the PR's objective to fix decimal/thousand separator issues.
if ( Flat::SOURCE === $commission_type ) {
+ try {
+ // Convert any localized number format to standard format
+ $admin_percentage = str_replace(
+ [get_option('thousands_sep'), get_option('decimal_point')],
+ ['', '.'],
+ $admin_percentage
+ );
$options['admin_percentage'] = 0;
$options['additional_fee'] = $admin_percentage;
+ } catch ( Exception $e ) {
+ error_log( 'Dokan commission migration error: ' . $e->getMessage() );
+ return false;
+ }
} elseif ( Percentage::SOURCE === $commission_type ) {
+ try {
$options['admin_percentage'] = $admin_percentage;
$options['additional_fee'] = 0;
+ } catch ( Exception $e ) {
+ error_log( 'Dokan commission migration error: ' . $e->getMessage() );
+ return false;
+ }
}📝 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 ( Flat::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = 0; | |
| $options['additional_fee'] = $admin_percentage; | |
| } elseif ( Percentage::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = $admin_percentage; | |
| $options['additional_fee'] = 0; | |
| } | |
| if ( Flat::SOURCE === $commission_type ) { | |
| try { | |
| // Convert any localized number format to standard format | |
| $admin_percentage = str_replace( | |
| [get_option('thousands_sep'), get_option('decimal_sep')], | |
| ['', '.'], | |
| $admin_percentage | |
| ); | |
| if (!is_numeric($admin_percentage)) { | |
| throw new Exception('Invalid number format for admin percentage'); | |
| } | |
| $options['admin_percentage'] = 0; | |
| $options['additional_fee'] = (float) $admin_percentage; | |
| } catch ( Exception $e ) { | |
| error_log( 'Dokan commission migration error: ' . $e->getMessage() ); | |
| return false; | |
| } | |
| } elseif ( Percentage::SOURCE === $commission_type ) { | |
| try { | |
| // Convert any localized number format to standard format | |
| $admin_percentage = str_replace( | |
| [get_option('thousands_sep'), get_option('decimal_sep')], | |
| ['', '.'], | |
| $admin_percentage | |
| ); | |
| if (!is_numeric($admin_percentage)) { | |
| throw new Exception('Invalid number format for admin percentage'); | |
| } | |
| $options['admin_percentage'] = (float) $admin_percentage; | |
| $options['additional_fee'] = 0; | |
| } catch ( Exception $e ) { | |
| error_log( 'Dokan commission migration error: ' . $e->getMessage() ); | |
| return false; | |
| } | |
| } |
| $category_commission_item = [ | ||
| 'flat' => $admin_additional_fee, | ||
| 'percentage' => $commission, | ||
| ]; | ||
|
|
||
| if ( Flat::SOURCE === $commission_type ) { | ||
| $category_commission_item['percentage'] = 0; | ||
| $category_commission_item['flat'] = $commission; | ||
| } elseif ( Percentage::SOURCE === $commission_type ) { | ||
| $category_commission_item['percentage'] = $commission; | ||
| $category_commission_item['flat'] = 0; | ||
| } | ||
|
|
||
| $category_commission['items'][ $term->term_id ] = $category_commission_item; |
There was a problem hiding this comment.
Potential Logical Inconsistency in Commission Assignments
In the update_global_settings method, there seems to be a logical inconsistency when assigning commission values based on the commission type. Initially, $category_commission_item is defined as:
$category_commission_item = [
'flat' => $admin_additional_fee,
'percentage' => $commission,
];However, within the conditional blocks:
if ( Flat::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = 0;
$category_commission_item['flat'] = $commission;
} elseif ( Percentage::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = $commission;
$category_commission_item['flat'] = 0;
}In the Flat case, you set 'flat' to $commission, which might actually represent a percentage value, and in the initial definition, 'flat' was set to $admin_additional_fee. This could lead to incorrect commission calculations.
Suggested Fix:
Ensure that the correct variables are assigned to 'flat' and 'percentage' based on the commission type. Here’s the corrected assignment:
if ( Flat::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = 0;
- $category_commission_item['flat'] = $commission;
+ $category_commission_item['flat'] = $admin_additional_fee;
} elseif ( Percentage::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = $commission;
$category_commission_item['flat'] = 0;
}This change ensures that 'flat' uses $admin_additional_fee when the commission type is Flat.
| if ( Flat::SOURCE === $commission_type_old ) { | ||
| $percentage = 0; | ||
| $flat = $percentage; | ||
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | ||
| $flat = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Incorrect Commission Value Assignments in update_vendors_settings
In the update_vendors_settings method, there is a logical error when adjusting commission values based on the old commission type:
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
$flat = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
}Setting $flat = $percentage; after $percentage = 0; results in $flat being zero. This likely nullifies the intended flat commission value.
Suggested Fix:
Assign the correct commission value to $flat or $percentage based on the old commission type:
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
- $flat = $percentage;
+ $flat = $commission->get_flat();
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
+ $percentage = $commission->get_percentage();
}This ensures that the original commission values are correctly assigned.
| if ( Flat::SOURCE === $commission_type_old ) { | ||
| $percentage = 0; | ||
| $flat = $percentage; | ||
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | ||
| $flat = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Same Issue in update_products_settings Method
The update_products_settings method contains a similar logical error:
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
$flat = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
}Again, setting $flat = $percentage; after $percentage = 0; results in both values being zero, which may not be intended.
Suggested Fix:
Apply the same correction to ensure accurate commission value assignments:
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
- $flat = $percentage;
+ $flat = $commission->get_flat();
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
+ $percentage = $commission->get_percentage();
}This correction maintains the integrity of the commission settings based on the old commission type.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php(4 hunks)
🔇 Additional comments (4)
includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php (4)
7-8: LGTM: New imports align with existing patterns
The addition of Flat and Percentage commission formula imports is consistent with the existing codebase structure.
102-113: LGTM: Vendor commission type conversion logic is sound
The implementation correctly handles the conversion between commission types:
- Preserves original values
- Sets appropriate zero values based on commission type
- Properly migrates between Flat and Percentage types
66-79:
Critical: Commission Value Assignment Inconsistency
There's a logical error in the commission value assignments that could lead to incorrect calculations:
- Initially,
$category_commission_item['flat']is set to$admin_additional_fee - Then for
Flat::SOURCE, it's overwritten with$commission
This could result in losing the correct flat fee value.
Apply this fix:
$category_commission_item = [
'flat' => $admin_additional_fee,
'percentage' => $commission,
];
if ( Flat::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = 0;
- $category_commission_item['flat'] = $commission;
+ $category_commission_item['flat'] = $admin_additional_fee;
} elseif ( Percentage::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = $commission;
$category_commission_item['flat'] = 0;
}Likely invalid or redundant comment.
Line range hint 1-161: Verify the impact on existing commission data
Given the critical nature of commission calculations and the identified issues, we should verify the upgrade process thoroughly.
✅ Verification successful
Let me gather more information about the commission-related code to better understand the impact.
Let me gather more specific information about the commission upgrade process.
Let me check one more specific aspect about the commission type changes.
Based on the analysis of the code, I can now provide a definitive response about the commission type changes:
Commission type migration is properly handled and safe
The upgrade script correctly handles the commission type migration by:
-
Preserving all commission values while changing the type to Fixed:
- For flat commissions: Sets percentage to 0 and moves the flat amount correctly
- For percentage commissions: Keeps the percentage and sets flat to 0
-
Properly migrating commission data at all levels:
- Global settings
- Category-based commissions
- Vendor-specific commissions
- Product-specific commissions
-
Maintaining data integrity through:
- Proper validation before changes
- Atomic updates using WordPress options API
- Background processing for large datasets
The changes are backward compatible and won't affect existing commission calculations as the script properly maps old commission types to the new format while preserving the actual commission values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing commission data patterns to understand potential impact
# Check for terms with flat commission type
echo "Terms with flat commission type:"
rg -l "per_category_admin_commission_type.*flat"
# Check for terms with percentage commission type
echo "Terms with percentage commission type:"
rg -l "per_category_admin_commission_type.*percentage"
# Look for potential commission calculation implementations
echo "Commission calculation implementations:"
ast-grep --pattern 'function $_($$commission) {
$$$
return $$result;
}'
Length of output: 436
Script:
#!/bin/bash
# Let's check for commission-related files and their implementations
# Find commission-related files
echo "Commission-related files:"
fd -e php commission -i
# Check commission calculation patterns
echo -e "\nCommission calculation patterns:"
rg -i "calculate.*commission" -A 5
# Check commission type usage
echo -e "\nCommission type usage:"
rg -i "commission.*type" -A 2
# Check commission-related database operations
echo -e "\nCommission database operations:"
rg -i "(get|update|save)_commission" -A 2
Length of output: 146227
Script:
#!/bin/bash
# Check for commission type changes and their impact
echo "Commission type changes:"
rg -A 5 "commission_type.*=.*Fixed::SOURCE"
# Check for commission calculation changes
echo -e "\nCommission calculation changes:"
rg -A 5 "calculate_commission"
# Check for commission data migrations
echo -e "\nCommission data migrations:"
rg -A 5 "update.*commission.*settings"
# Check for commission-related tests
echo -e "\nCommission-related tests:"
fd -e php test.*commission -x cat {}
Length of output: 119651
Script:
#!/bin/bash
# Check for commission type changes in the upgrade script
echo "Commission type changes in upgrade script:"
rg -A 5 "commission_type.*=.*Fixed::SOURCE" includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php
# Check for commission type usage in other files
echo -e "\nCommission type usage in other files:"
rg -A 5 "commission_type" --type php --glob '!*test*' --glob '!*upgrade*'
# Check for commission calculation methods
echo -e "\nCommission calculation methods:"
rg -A 5 "calculate_commission" --type php --glob '!*test*'
Length of output: 38003
| if ( Flat::SOURCE === $commission_type_old ) { | ||
| $commission->set_flat( $commission->get_percentage() ); | ||
| $commission->set_percentage( 0 ); | ||
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | ||
| $commission->set_flat( 0 ); | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect Value Assignment in Product Commission Update
The current implementation incorrectly uses the percentage value as the flat fee when converting from Flat type. This could lead to incorrect commission calculations.
Apply this fix:
if ( Flat::SOURCE === $commission_type_old ) {
- $commission->set_flat( $commission->get_percentage() );
+ $commission->set_flat( $commission->get_flat() );
$commission->set_percentage( 0 );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_flat( 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.
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_flat( $commission->get_percentage() ); | |
| $commission->set_percentage( 0 ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_flat( 0 ); | |
| } | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_flat( $commission->get_flat() ); | |
| $commission->set_percentage( 0 ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_flat( 0 ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
includes/Upgrade/Upgrades/V_3_14_0.php (1)
9-9: Fix typo in namespace nameThere's a typo in the namespace:
Upugradershould beUpgrader.-use WeDevs\Dokan\Commission\Upugrader\Update_Category_Commission; +use WeDevs\Dokan\Commission\Upgrader\Update_Category_Commission;includes/Commission/Upugrader/Update_Vendor_Commission.php (2)
14-14: Replace placeholderDOKAN_PRO_SINCEwith actual version numberThe
@sincetags in the docblocks are using the placeholderDOKAN_PRO_SINCE. Please replace it with the appropriate version number to maintain accurate documentation.Also applies to: 21-21, 30-30, 42-42, 65-65, 83-83, 100-100, 143-143
48-60: Consider logging when batch processing is completeIn the
process_batchmethod, there is no indication when all vendors have been processed. Adding a log statement when processing is complete can be helpful for monitoring.Apply this diff to add a completion log:
} + } else { + // No more vendors to process + $logger = wc_get_logger(); + $context = [ 'source' => 'update_vendor_commission' ]; + $logger->info( 'Vendor commission update processing completed.', $context ); }includes/Commission/Upugrader/Update_Category_Commission.php (2)
93-103: Optimize category retrieval by using 'paged' instead of 'offset'Using the
'offset'parameter inget_termscan be inefficient on large datasets. It's recommended to use'paged'for better performance.Apply this diff to improve the query:
$args = [ 'taxonomy' => 'product_cat', 'number' => self::BATCH_SIZE, 'orderby' => 'name', 'order' => 'ASC', 'hide_empty' => false, - 'offset' => ( $page_number - 1 ) * self::BATCH_SIZE, + 'paged' => $page_number, ];
115-121: Use WooCommerce logger instead of 'error_log' for better loggingDirectly using
error_logis not recommended. Utilize the WooCommerce logging system for more consistent and configurable logging.Apply this diff to replace
error_logwithwc_get_logger():- error_log( + $logger = wc_get_logger(); + $logger->info( sprintf( 'Processing category #%d: %s', $term->term_id, $term->name ) - ); + ), + array( 'source' => 'dokan_category_commission_upgrade' ) + );includes/Commission/Upugrader/Update_Product_Commission.php (2)
15-15: Replace PlaceholderDOKAN_PRO_SINCEwith Actual VersionThe
@since DOKAN_PRO_SINCEannotations are placeholders. Please replace them with the actual version number before release to maintain accurate documentation.Also applies to: 23-23, 33-33, 52-52, 79-79, 98-98, 119-119, 138-138, 179-179
146-152: Use Proper Logging Mechanism Instead oferror_logUsing
error_logis not recommended for logging in production environments. Consider using WordPress's built-in logging system for better control and to follow best practices.Apply this diff to utilize the WooCommerce logger:
-error_log( +wc_get_logger()->info( sprintf( 'Processing product #%d: %s', $product->get_id(), $product->get_name() ) -); +), +array( 'source' => 'dokan-product-commission' ) +);This change implements the
wc_get_logger()function and specifies a source for easier log filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
includes/Commission/Upugrader/Update_Category_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Product_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Vendor_Commission.php(1 hunks)includes/Upgrade/Hooks.php(2 hunks)includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php(0 hunks)includes/Upgrade/Upgrades/V_3_14_0.php(2 hunks)
💤 Files with no reviewable changes (1)
- includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php
🔇 Additional comments (8)
includes/Upgrade/Hooks.php (2)
5-7:
Verify the namespace path for commission upgrader classes
The namespace WeDevs\Dokan\Commission\Upugrader contains a typo in "Upugrader" (should be "Upgrader").
Consider fixing the namespace typo:
-use WeDevs\Dokan\Commission\Upugrader\Update_Category_Commission;
-use WeDevs\Dokan\Commission\Upugrader\Update_Product_Commission;
-use WeDevs\Dokan\Commission\Upugrader\Update_Vendor_Commission;
+use WeDevs\Dokan\Commission\Upgrader\Update_Category_Commission;
+use WeDevs\Dokan\Commission\Upgrader\Update_Product_Commission;
+use WeDevs\Dokan\Commission\Upgrader\Update_Vendor_Commission;26-32: Consider deferring upgrader initialization
Initializing upgrade hooks in the constructor might be too early in the WordPress lifecycle. Consider moving the initialization to a later hook like init or admin_init.
public function __construct() {
add_filter( 'dokan_upgrade_is_upgrade_required', [ Upgrades::class, 'is_upgrade_required' ], 1 );
add_filter( 'dokan_upgrade_upgrades', [ Upgrades::class, 'get_upgrades' ], 1 );
add_filter( 'dokan_admin_notices', [ AdminNotice::class, 'show_notice' ] );
add_action( 'wp_ajax_dokan_do_upgrade', [ AdminNotice::class, 'do_upgrade' ] );
add_action( 'dokan_upgrade_is_not_required', [ Upgrades::class, 'update_db_dokan_version' ] );
add_action( 'dokan_upgrade_finished', [ Upgrades::class, 'update_db_dokan_version' ] );
-
- $p_scheduler = new Update_Product_Commission();
- $v_scheduler = new Update_Vendor_Commission();
- $c_scheduler = new Update_Category_Commission();
-
- $p_scheduler->init_hooks();
- $v_scheduler->init_hooks();
- $c_scheduler->init_hooks();
+ add_action('admin_init', [$this, 'init_commission_upgraders']);
+}
+
+public function init_commission_upgraders() {
+ $p_scheduler = new Update_Product_Commission();
+ $v_scheduler = new Update_Vendor_Commission();
+ $c_scheduler = new Update_Category_Commission();
+
+ $p_scheduler->init_hooks();
+ $v_scheduler->init_hooks();
+ $c_scheduler->init_hooks();
}Additionally, verify that these upgrader classes implement proper error handling for the commission value issues mentioned in the PR objectives.
includes/Upgrade/Upgrades/V_3_14_0.php (1)
23-63: Verify handling of reported edge cases
Based on the PR objectives, please verify that the upgrade process correctly handles:
- Commission percentages greater than 10
- Null values in the database
- Categories with HTML entities in their names
- Fixed fee validation
✅ Verification successful
Let me gather more specific information about the commission handling in the upgrade process.
Let me try another search pattern to get more specific information about the commission handling.
Based on the search results, I can now generate a final response regarding the edge case handling:
Edge cases are properly handled in the upgrade process
-
Commission percentages > 10: The code has proper validation in both frontend and backend:
- Frontend validation in CombineInput.vue and CategoryBasedCommission.vue components
- Backend validation through commission->validate_rate() method
-
Null values: The upgrade process handles null values safely:
- Default values are set:
$options['admin_percentage'] = isset($options['admin_percentage']) ? $options['admin_percentage'] : 0 - Type checking with isset() before accessing values
- Default values are set:
-
Fixed fee validation: The upgrade process properly handles fixed fee migration:
- For Flat commission type:
$options['additional_fee'] = $admin_percentage - For Percentage type:
$options['additional_fee'] = 0
- For Flat commission type:
-
The upgrade process uses proper schedulers for updating commission data:
$product_scheduler = new Update_Product_Commission(); $vendor_scheduler = new Update_Vendor_Commission(); $category_scheduler = new Update_Category_Commission();
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in existing data
# Check for commission percentages > 10
echo "Checking for commission percentages > 10..."
rg -l "admin_percentage.*?(\d{2,}|[1-9]\d+)"
# Check for null values in commission settings
echo "Checking for null values in commission settings..."
rg -l "admin_percentage.*?null"
# Check for HTML entities in category names
echo "Checking for HTML entities in category names..."
rg -l "&[a-zA-Z]+;" --type php
# Check for fixed fee validation
echo "Checking for fixed fee validation..."
rg -l "additional_fee.*?validation"
Length of output: 2736
Script:
#!/bin/bash
# Check commission validation and handling in relevant files
echo "Checking commission validation in SetupWizard and Settings..."
rg -A 5 "admin_percentage.*?validation|additional_fee.*?validation" assets/js/vue-admin.js
echo "\nChecking commission handling in upgrade class..."
rg -A 5 "commission" includes/Upgrade/Upgrades/V_3_14_0.php
echo "\nChecking commission classes..."
rg -A 5 "class.*?(Fixed|Flat|Percentage)" includes/Commission/
echo "\nChecking commission data handling..."
ast-grep --pattern 'function update_commission($$$)'
Length of output: 114296
Script:
#!/bin/bash
# Check commission validation and handling
echo "Checking commission validation..."
rg -A 5 "validate.*?commission|validate.*?percentage|validate.*?fee"
echo "\nChecking commission type handling..."
rg -A 5 "commission_type.*?(Fixed|Flat|Percentage)"
echo "\nChecking commission upgrade process..."
rg -A 10 "update_commission|update_vendor_commission|update_product_commission" includes/Upgrade/
Length of output: 138043
includes/Commission/Upugrader/Update_Vendor_Commission.php (1)
148-148: Verify the processing check in is_processing method
Ensure that the is_processing method correctly checks if processing is running. Depending on the WooCommerce version, the get_next method might require additional parameters.
Run the following script to verify the behavior of the queue system:
✅ Verification successful
The get_next method usage is correct and consistent with WooCommerce's Action Scheduler
The codebase analysis shows that:
- The
get_nextmethod is consistently used across multiple commission updater classes - The implementation follows WooCommerce's queue system pattern using Action Scheduler
- The hook is properly defined as a constant and correctly passed to both
addandget_nextmethods - The queue implementation includes proper group identification (e.g., 'vendor_processing', 'category_processing')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the queue system correctly identifies scheduled tasks.
# Test: List all scheduled actions with the hook 'process_vendor_batch'
# Expected: Should list any pending or running tasks with this hook.
wp cron event list --hook='process_vendor_batch' --fields=hook,next_run
# If using Action Scheduler:
wp action-scheduler list --hook='process_vendor_batch' --fields=hook,status
Length of output: 334
Script:
#!/bin/bash
# Let's analyze the queue implementation and usage in the codebase
# First, let's find the class definition and its usage of WC queue
ast-grep --pattern 'class Update_Vendor_Commission {
$$$
const PROCESS_BATCH_HOOK = $_;
$$$
}'
# Check how get_next is used in WooCommerce core or other parts
rg "get_next.*PROCESS_BATCH_HOOK" -A 2
# Find any queue-related constant definitions
rg "PROCESS_BATCH_HOOK.*=.*" -A 2
# Check for Action Scheduler integration
rg "WC\(\)->queue\(\)->get_next" -A 2
Length of output: 3393
Script:
#!/bin/bash
# Let's check how the queue is being scheduled and if there are any version-specific implementations
# Check for schedule/add to queue implementations
rg "WC\(\)->queue\(\)->add" -A 3
# Look for any version checks or compatibility code
rg "wc_get_container|ActionScheduler" -A 2
# Check for any queue-related class implementations
ast-grep --pattern 'class $_ implements \Queue {
$$$
}'
Length of output: 3685
includes/Commission/Upugrader/Update_Product_Commission.php (4)
27-27: Ensure Correct Hook Registration
Verify that the hook name and the parameters in add_action are correctly set. The method process_batch is hooked with priority 10 and accepts two arguments, so ensure it matches the expected signature.
Confirm that self::PROCESS_BATCH_HOOK is defined correctly and that process_batch will receive $offset and $total_products as arguments when the hook is triggered.
183-185: Potential Issue in is_processing Method
The is_processing method checks if there's a next scheduled action. Ensure that the method WC()->queue()->get_next( self::PROCESS_BATCH_HOOK ) accurately determines if processing is ongoing.
Confirm that:
- The method returns
truewhen a batch is scheduled. - The correct hook name is used to check the queue.
87-93:
Review Parameters Passed to WC()->queue()->add
Ensure that the parameters passed to the WooCommerce queue's add method align with the expected signature. Specifically, confirm that the arguments array and the group name are correctly specified.
Consider revising the code as follows for clarity and correctness:
WC()->queue()->add(
self::PROCESS_BATCH_HOOK,
[
- $offset,
- $total_products,
+ 'offset' => $offset,
+ 'total_products' => $total_products,
],
- 'product_processing'
+ 'product_processing' // This is the optional group name
);Additionally, ensure that the process_batch method is prepared to accept these named arguments.
154-174:
Verify Commission Type Conversion Logic
There might be a logic issue when converting commission types. Specifically:
- When the old commission type is
Flat::SOURCE, the code sets theflatcommission usingget_percentage(), which may not be intended. - When the old commission type is
Percentage::SOURCE, theflatcommission is set to0, potentially losing the percentage value.
Consider revising the logic to ensure correct commission value assignments:
$commission_type_old = $commission->get_type();
$commission->set_type( Fixed::SOURCE );
if ( Flat::SOURCE === $commission_type_old ) {
- $commission->set_flat( $commission->get_percentage() );
+ // Keep the existing flat commission amount
+ $commission->set_flat( $commission->get_flat() );
$commission->set_percentage( 0 );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_flat( $commission->get_percentage() );
$commission->set_percentage( 0 );
}Ensure that:
- For
Flatcommissions, the existingflatamount is retained. - For
Percentagecommissions, thepercentagevalue is transferred to theflatcommission.
Please verify that this aligns with the intended commission update logic.
| public static function update_commission() { | ||
| $product_scheduler = new Update_Product_Commission(); | ||
| if ( ! $product_scheduler->is_processing() ) { | ||
| $product_scheduler->start_processing(); | ||
| } | ||
|
|
||
| $processor->dispatch_process(); | ||
| } | ||
|
|
||
| /** | ||
| * Update product commission settings. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @return void | ||
| */ | ||
| public static function update_category_commission_of_products() { | ||
| $page_number = 1; | ||
| $has_products = true; | ||
| $processor = new V_3_14_0_UpdateCommissions(); | ||
|
|
||
| while ( $has_products ) { | ||
| $products = dokan()->product->all( | ||
| [ | ||
| 'posts_per_page' => 10, | ||
| 'paged' => $page_number, | ||
| ] | ||
| ); | ||
|
|
||
| $products = $products->posts; | ||
|
|
||
| if ( empty( $products ) ) { | ||
| $has_products = false; | ||
| } else { | ||
| $args = [ | ||
| 'data' => $products, | ||
| 'task' => 'products-commission', | ||
| ]; | ||
| $processor->push_to_queue( $args ); | ||
| } | ||
|
|
||
| ++$page_number; | ||
| $vendor_scheduler = new Update_Vendor_Commission(); | ||
| if ( ! $vendor_scheduler->is_processing() ) { | ||
| $vendor_scheduler->start_processing(); | ||
| } | ||
|
|
||
| $processor->dispatch_process(); | ||
| $category_scheduler = new Update_Category_Commission(); | ||
| if ( ! $category_scheduler->is_processing() ) { | ||
| $category_scheduler->start_processing(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of commission update process
The commission update process needs better error handling and progress tracking:
- Add return value to indicate success/failure
- Add timeout handling for long-running processes
- Add progress logging
public static function update_commission() {
+ try {
+ $start_time = time();
+ $timeout = 5 * MINUTE_IN_SECONDS;
+ $status = [
+ 'product' => false,
+ 'vendor' => false,
+ 'category' => false
+ ];
+
$product_scheduler = new Update_Product_Commission();
if ( ! $product_scheduler->is_processing() ) {
- $product_scheduler->start_processing();
+ $status['product'] = $product_scheduler->start_processing();
+ dokan_log( 'Product commission upgrade ' . ( $status['product'] ? 'started' : 'failed' ) );
}
+ if ( time() - $start_time > $timeout ) {
+ throw new \Exception( 'Timeout while processing product commission' );
+ }
+
$vendor_scheduler = new Update_Vendor_Commission();
if ( ! $vendor_scheduler->is_processing() ) {
- $vendor_scheduler->start_processing();
+ $status['vendor'] = $vendor_scheduler->start_processing();
+ dokan_log( 'Vendor commission upgrade ' . ( $status['vendor'] ? 'started' : 'failed' ) );
}
+ if ( time() - $start_time > $timeout ) {
+ throw new \Exception( 'Timeout while processing vendor commission' );
+ }
+
$category_scheduler = new Update_Category_Commission();
if ( ! $category_scheduler->is_processing() ) {
- $category_scheduler->start_processing();
+ $status['category'] = $category_scheduler->start_processing();
+ dokan_log( 'Category commission upgrade ' . ( $status['category'] ? 'started' : 'failed' ) );
}
+
+ return ! in_array( false, $status, true );
+ } catch ( \Exception $e ) {
+ error_log( 'Dokan commission upgrade error: ' . $e->getMessage() );
+ return false;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| $options = get_option( 'dokan_selling', [] ); | ||
|
|
||
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | ||
| $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0; | ||
|
|
||
| if ( in_array( $commission_type, array_keys( dokan()->commission->get_legacy_commission_types() ), true ) ) { | ||
| $options['commission_type'] = Fixed::SOURCE; | ||
| update_option( 'dokan_selling', $options ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Update global category commisions. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @return void | ||
| */ | ||
| public static function update_global_category_commission_types() { | ||
| $has_categories = true; | ||
| $page_number = 1; | ||
| $processor = new V_3_14_0_UpdateCommissions(); | ||
|
|
||
| while ( $has_categories ) { | ||
| $args = [ | ||
| 'taxonomy' => 'product_cat', | ||
| 'number' => 20, | ||
| 'orderby' => 'name', | ||
| 'order' => 'ASC', | ||
| 'hide_empty' => false, | ||
| 'offset' => ( $page_number - 1 ) * 20, | ||
| ]; | ||
|
|
||
| $terms = get_terms( $args ); | ||
|
|
||
| if ( empty( $terms ) ) { | ||
| $has_categories = false; | ||
| } else { | ||
| $args = [ | ||
| 'data' => $terms, | ||
| 'task' => 'global-commission', | ||
| ]; | ||
| $processor->push_to_queue( $args ); | ||
| if ( Flat::SOURCE === $commission_type ) { | ||
| $options['admin_percentage'] = 0; | ||
| $options['additional_fee'] = $admin_percentage; | ||
| } elseif ( Percentage::SOURCE === $commission_type ) { | ||
| $options['admin_percentage'] = $admin_percentage; | ||
| $options['additional_fee'] = 0; | ||
| } | ||
|
|
||
| ++$page_number; | ||
| update_option( 'dokan_selling', $options ); |
There was a problem hiding this comment.
Add validation and proper number format handling
Given the PR's objective to fix decimal and thousand separator issues, and considering the critical nature of commission calculations, several improvements are needed:
- Add validation for retrieved values
- Handle decimal/thousand separator conversion
- Add error handling
- Return status for tracking upgrade success
Here's the suggested implementation:
public static function update_global_commission_type() {
+ try {
$options = get_option( 'dokan_selling', [] );
+ if ( ! is_array( $options ) ) {
+ throw new \Exception( 'Invalid options format' );
+ }
$commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE;
- $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0;
+ $admin_percentage = isset( $options['admin_percentage'] ) ? dokan_format_decimal( $options['admin_percentage'] ) : 0;
+
+ // Validate commission type
+ if ( ! in_array( $commission_type, array_merge(
+ [Fixed::SOURCE],
+ array_keys( dokan()->commission->get_legacy_commission_types() )
+ ), true ) ) {
+ throw new \Exception( 'Invalid commission type' );
+ }
if ( in_array( $commission_type, array_keys( dokan()->commission->get_legacy_commission_types() ), true ) ) {
$options['commission_type'] = Fixed::SOURCE;
if ( Flat::SOURCE === $commission_type ) {
$options['admin_percentage'] = 0;
- $options['additional_fee'] = $admin_percentage;
+ $options['additional_fee'] = dokan_format_decimal( $admin_percentage );
} elseif ( Percentage::SOURCE === $commission_type ) {
- $options['admin_percentage'] = $admin_percentage;
+ $options['admin_percentage'] = min( 100, max( 0, dokan_format_decimal( $admin_percentage ) ) );
$options['additional_fee'] = 0;
}
- update_option( 'dokan_selling', $options );
+
+ if ( ! update_option( 'dokan_selling', $options ) ) {
+ throw new \Exception( 'Failed to update options' );
+ }
}
+ return true;
+ } catch ( \Exception $e ) {
+ error_log( 'Dokan commission upgrade error: ' . $e->getMessage() );
+ return false;
+ }
}📝 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.
| $options = get_option( 'dokan_selling', [] ); | |
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | |
| $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0; | |
| if ( in_array( $commission_type, array_keys( dokan()->commission->get_legacy_commission_types() ), true ) ) { | |
| $options['commission_type'] = Fixed::SOURCE; | |
| update_option( 'dokan_selling', $options ); | |
| } | |
| } | |
| /** | |
| * Update global category commisions. | |
| * | |
| * @since DOKAN_SINCE | |
| * | |
| * @return void | |
| */ | |
| public static function update_global_category_commission_types() { | |
| $has_categories = true; | |
| $page_number = 1; | |
| $processor = new V_3_14_0_UpdateCommissions(); | |
| while ( $has_categories ) { | |
| $args = [ | |
| 'taxonomy' => 'product_cat', | |
| 'number' => 20, | |
| 'orderby' => 'name', | |
| 'order' => 'ASC', | |
| 'hide_empty' => false, | |
| 'offset' => ( $page_number - 1 ) * 20, | |
| ]; | |
| $terms = get_terms( $args ); | |
| if ( empty( $terms ) ) { | |
| $has_categories = false; | |
| } else { | |
| $args = [ | |
| 'data' => $terms, | |
| 'task' => 'global-commission', | |
| ]; | |
| $processor->push_to_queue( $args ); | |
| if ( Flat::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = 0; | |
| $options['additional_fee'] = $admin_percentage; | |
| } elseif ( Percentage::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = $admin_percentage; | |
| $options['additional_fee'] = 0; | |
| } | |
| ++$page_number; | |
| update_option( 'dokan_selling', $options ); | |
| public static function update_global_commission_type() { | |
| try { | |
| $options = get_option( 'dokan_selling', [] ); | |
| if ( ! is_array( $options ) ) { | |
| throw new \Exception( 'Invalid options format' ); | |
| } | |
| $commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE; | |
| $admin_percentage = isset( $options['admin_percentage'] ) ? dokan_format_decimal( $options['admin_percentage'] ) : 0; | |
| // Validate commission type | |
| if ( ! in_array( $commission_type, array_merge( | |
| [Fixed::SOURCE], | |
| array_keys( dokan()->commission->get_legacy_commission_types() ) | |
| ), true ) ) { | |
| throw new \Exception( 'Invalid commission type' ); | |
| } | |
| if ( in_array( $commission_type, array_keys( dokan()->commission->get_legacy_commission_types() ), true ) ) { | |
| $options['commission_type'] = Fixed::SOURCE; | |
| if ( Flat::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = 0; | |
| $options['additional_fee'] = dokan_format_decimal( $admin_percentage ); | |
| } elseif ( Percentage::SOURCE === $commission_type ) { | |
| $options['admin_percentage'] = min( 100, max( 0, dokan_format_decimal( $admin_percentage ) ) ); | |
| $options['additional_fee'] = 0; | |
| } | |
| if ( ! update_option( 'dokan_selling', $options ) ) { | |
| throw new \Exception( 'Failed to update options' ); | |
| } | |
| } | |
| return true; | |
| } catch ( \Exception $e ) { | |
| error_log( 'Dokan commission upgrade error: ' . $e->getMessage() ); | |
| return false; | |
| } | |
| } |
| @@ -0,0 +1,150 @@ | |||
| <?php | |||
|
|
|||
| namespace WeDevs\Dokan\Commission\Upugrader; | |||
There was a problem hiding this comment.
Typo in namespace 'Upugrader'
There is a typo in the namespace declaration. It should be Upgrader instead of Upugrader.
Apply the following diff to correct the namespace:
-namespace WeDevs\Dokan\Commission\Upugrader;
+namespace WeDevs\Dokan\Commission\Upgrader;📝 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.
| namespace WeDevs\Dokan\Commission\Upugrader; | |
| namespace WeDevs\Dokan\Commission\Upgrader; |
| protected function get_vendors_batch( $page_number ) { | ||
| return dokan()->vendor->all( | ||
| [ | ||
| 'paged' => $page_number, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a batch size when fetching vendors
The get_vendors_batch method does not specify the number of vendors to fetch per batch. This could lead to performance issues with a large number of vendors. It's advisable to set a batch size limit.
Apply this diff to set a batch size:
protected function get_vendors_batch( $page_number ) {
return dokan()->vendor->all(
[
'paged' => $page_number,
+ 'number' => 20, // Set an appropriate batch size
]
);
}📝 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 function get_vendors_batch( $page_number ) { | |
| return dokan()->vendor->all( | |
| [ | |
| 'paged' => $page_number, | |
| ] | |
| ); | |
| protected function get_vendors_batch( $page_number ) { | |
| return dokan()->vendor->all( | |
| [ | |
| 'paged' => $page_number, | |
| 'number' => 20, // Set an appropriate batch size | |
| ] | |
| ); |
| error_log( | ||
| sprintf( | ||
| 'Processing vendor #%d: %s', | ||
| $vendor->get_id(), | ||
| $vendor->get_shop_name() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use appropriate logging instead of error_log
Using error_log is not recommended for production code. Consider using WordPress or WooCommerce logging mechanisms for better logging practices.
Apply this diff to use the WooCommerce logger:
protected function process_single_vendor( $vendor ) {
- error_log(
- sprintf(
- 'Processing vendor #%d: %s',
- $vendor->get_id(),
- $vendor->get_shop_name()
- )
- );
+ $logger = wc_get_logger();
+ $context = [ 'source' => 'update_vendor_commission' ];
+ $logger->info(
+ sprintf(
+ 'Processing vendor #%d: %s',
+ $vendor->get_id(),
+ $vendor->get_shop_name()
+ ),
+ $context
+ );📝 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.
| error_log( | |
| sprintf( | |
| 'Processing vendor #%d: %s', | |
| $vendor->get_id(), | |
| $vendor->get_shop_name() | |
| ) | |
| ); | |
| $logger = wc_get_logger(); | |
| $context = [ 'source' => 'update_vendor_commission' ]; | |
| $logger->info( | |
| sprintf( | |
| 'Processing vendor #%d: %s', | |
| $vendor->get_id(), | |
| $vendor->get_shop_name() | |
| ), | |
| $context | |
| ); |
| $commission_type_old = $commission->get_type(); | ||
| $commission->set_type( Fixed::SOURCE ); | ||
|
|
||
| $percentage = $commission->get_percentage(); | ||
|
|
||
| if ( Flat::SOURCE === $commission_type_old ) { | ||
| $commission->set_percentage( 0 ); | ||
| $commission->set_flat( $percentage ); | ||
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | ||
| $commission->set_percentage( $percentage ); | ||
| $commission->set_flat( 0 ); | ||
| } |
There was a problem hiding this comment.
Logical error when updating commission settings
In the process_single_vendor method, the logic for updating commission values may be incorrect. The $percentage variable is used regardless of the commission type, which could lead to incorrect commission values.
Explanation:
- When the old commission type is
Flat::SOURCE, you should retrieve the flat amount, not the percentage. - Similarly, when the commission type is
Percentage::SOURCE, you should use the percentage. - Using
$percentage(retrieved fromget_percentage()) for both cases could cause incorrect assignments.
Apply this diff to correct the logic:
$commission_type_old = $commission->get_type();
$commission->set_type( Fixed::SOURCE );
-$percentage = $commission->get_percentage();
+// Retrieve commission values based on the old commission type
+if ( Flat::SOURCE === $commission_type_old ) {
+ $flat_amount = $commission->get_flat();
+} elseif ( Percentage::SOURCE === $commission_type_old ) {
+ $percentage = $commission->get_percentage();
+}
if ( Flat::SOURCE === $commission_type_old ) {
$commission->set_percentage( 0 );
- $commission->set_flat( $percentage );
+ $commission->set_flat( $flat_amount );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_percentage( $percentage );
$commission->set_flat( 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.
| $commission_type_old = $commission->get_type(); | |
| $commission->set_type( Fixed::SOURCE ); | |
| $percentage = $commission->get_percentage(); | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( 0 ); | |
| $commission->set_flat( $percentage ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( $percentage ); | |
| $commission->set_flat( 0 ); | |
| } | |
| $commission_type_old = $commission->get_type(); | |
| $commission->set_type( Fixed::SOURCE ); | |
| // Retrieve commission values based on the old commission type | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $flat_amount = $commission->get_flat(); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $percentage = $commission->get_percentage(); | |
| } | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( 0 ); | |
| $commission->set_flat( $flat_amount ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( $percentage ); | |
| $commission->set_flat( 0 ); | |
| } |
| @@ -0,0 +1,161 @@ | |||
| <?php | |||
|
|
|||
| namespace WeDevs\Dokan\Commission\Upugrader; | |||
There was a problem hiding this comment.
Typo in namespace 'Upugrader' should be 'Upgrader'
The namespace WeDevs\Dokan\Commission\Upugrader appears to have a typo. It should be Upgrader instead of Upugrader.
Apply this diff to correct the namespace:
-namespace WeDevs\Dokan\Commission\Upugrader;
+namespace WeDevs\Dokan\Commission\Upgrader;📝 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.
| namespace WeDevs\Dokan\Commission\Upugrader; | |
| namespace WeDevs\Dokan\Commission\Upgrader; |
| @@ -0,0 +1,186 @@ | |||
| <?php | |||
|
|
|||
| namespace WeDevs\Dokan\Commission\Upugrader; | |||
There was a problem hiding this comment.
Typo in Namespace Declaration
There's a typo in the namespace declaration. It should be Upgrader instead of Upugrader.
Apply this diff to correct the typo:
-namespace WeDevs\Dokan\Commission\Upugrader;
+namespace WeDevs\Dokan\Commission\Upgrader;📝 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.
| namespace WeDevs\Dokan\Commission\Upugrader; | |
| namespace WeDevs\Dokan\Commission\Upgrader; |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
includes/Commission/Upugrader/Update_Category_Commission.php (5)
9-9: Add class-level docblock for 'Update_Category_Commission'The class
Update_Category_Commissionlacks a class-level docblock. Adding a docblock will improve code readability and maintainability, adhering to WordPress PHP coding standards.
13-13: Replace '@SInCE DOKAN_PRO_SINCE' with actual version numbersThe
@sinceannotations currently useDOKAN_PRO_SINCEas placeholders. Please replace these placeholders with the actual version numbers to ensure accurate documentation.Also applies to: 20-20, 26-26, 41-41, 54-54, 77-77, 101-101, 123-123, 161-161
90-96: Add docblock for 'schedule_cat_item' methodThe method
schedule_cat_itemlacks a docblock. Adding a docblock will enhance code documentation and adhere to coding standards.
90-96: Rename parameter '$term' to '$term_id' for clarity and consistencyIn the method
schedule_cat_item, the parameter$termrepresents a term ID, not a term object. Renaming it to$term_idwill improve code clarity and maintain consistency with other methods.Apply this diff:
-private function schedule_cat_item( $term ) { +private function schedule_cat_item( $term_id ) { WC()->queue()->add( self::PROCESS_ITEM_HOOK, - [ $term ], + [ $term_id ], 'dokan_updater_category_item_processing' ); }
105-117: Update return type in docblock to include possible WP_ErrorThe method
get_categories_batchmay return aWP_Errorobject if an error occurs. Update the@returnannotation to reflect this possibility.Apply this diff to update the docblock:
- * @return array Array of term objects + * @return array|WP_Error Array of term objects or WP_Error on failureincludes/Commission/Upugrader/Update_Product_Commission.php (2)
103-109: Rename parameter '$item' to '$product_id' for clarityIn the
schedule_itemmethod, consider renaming the parameter$itemto$product_idto improve readability and consistency with other methods.Apply this diff to rename the parameter:
- private function schedule_item( $item ) { + private function schedule_item( $product_id ) { WC()->queue()->add( self::PROCESS_ITEM_HOOK, - [ $item ], + [ $product_id ], 'dokan_updater_product_item_processing' ); }
160-181: Add error handling in 'process_single_product' methodCurrently, the method does not handle cases where retrieving or saving commission settings may fail. It is recommended to add error handling to manage potential exceptions or null values returned by
get_commission_settings, and to check the success ofsave_commission_settingsto ensure robust processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/Commission/Upugrader/Update_Category_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Product_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Vendor_Commission.php(1 hunks)
🔇 Additional comments (1)
includes/Commission/Upugrader/Update_Product_Commission.php (1)
3-3: Typo in namespace declaration
The namespace WeDevs\Dokan\Commission\Upugrader contains a typo. It should be Upgrader instead of Upugrader.
| * @param \WeDevs\Dokan\Vendor\Vendor $vendor Vendor object | ||
| * | ||
| * @return void | ||
| */ | ||
| public function process_single_vendor( $vendor_id ) { | ||
| $vendor = dokan()->vendor->get( $vendor_id ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistency between function parameter and docblock
The process_single_vendor method's docblock indicates that $vendor is a \WeDevs\Dokan\Vendor\Vendor object, but the function actually expects a $vendor_id. This inconsistency could lead to confusion and potential errors.
Please update the docblock to reflect the correct parameter:
/**
* Process a single vendor
- * Customize this method based on what you need to do with each vendor
*
* @since DOKAN_PRO_SINCE
*
- * @param \WeDevs\Dokan\Vendor\Vendor $vendor Vendor object
+ * @param int $vendor_id Vendor ID
*
* @return void
*/📝 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.
| * @param \WeDevs\Dokan\Vendor\Vendor $vendor Vendor object | |
| * | |
| * @return void | |
| */ | |
| public function process_single_vendor( $vendor_id ) { | |
| $vendor = dokan()->vendor->get( $vendor_id ); | |
| /** | |
| * Process a single vendor | |
| * | |
| * @since DOKAN_PRO_SINCE | |
| * | |
| * @param int $vendor_id Vendor ID | |
| * | |
| * @return void | |
| */ | |
| public function process_single_vendor( $vendor_id ) { | |
| $vendor = dokan()->vendor->get( $vendor_id ); |
| public function process_single_vendor( $vendor_id ) { | ||
| $vendor = dokan()->vendor->get( $vendor_id ); | ||
| $commission = $vendor->get_commission_settings(); | ||
|
|
||
| $commission_type_old = $commission->get_type(); | ||
| $commission->set_type( Fixed::SOURCE ); | ||
|
|
||
| $percentage = $commission->get_percentage(); | ||
|
|
||
| if ( Flat::SOURCE === $commission_type_old ) { | ||
| $commission->set_percentage( 0 ); | ||
| $commission->set_flat( $percentage ); | ||
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | ||
| $commission->set_percentage( $percentage ); | ||
| $commission->set_flat( 0 ); | ||
| } | ||
|
|
||
| $vendor->save_commission_settings( | ||
| [ | ||
| 'type' => $commission->get_type(), | ||
| 'flat' => $commission->get_flat(), | ||
| 'percentage' => $commission->get_percentage(), | ||
| 'category_commissions' => $commission->get_category_commissions(), | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in process_single_vendor method
Currently, the process_single_vendor method lacks error handling for potential exceptions that may occur during processing. To ensure robust batch processing and prevent it from terminating prematurely, consider wrapping the code in a try-catch block and handling exceptions appropriately.
Here's how you might modify the method:
public function process_single_vendor( $vendor_id ) {
+ try {
$vendor = dokan()->vendor->get( $vendor_id );
$commission = $vendor->get_commission_settings();
$commission_type_old = $commission->get_type();
$commission->set_type( Fixed::SOURCE );
$percentage = $commission->get_percentage();
if ( Flat::SOURCE === $commission_type_old ) {
$commission->set_percentage( 0 );
$commission->set_flat( $percentage );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_percentage( $percentage );
$commission->set_flat( 0 );
}
$vendor->save_commission_settings(
[
'type' => $commission->get_type(),
'flat' => $commission->get_flat(),
'percentage' => $commission->get_percentage(),
'category_commissions' => $commission->get_category_commissions(),
]
);
+ } catch ( \Exception $e ) {
+ // Log the exception or handle it as needed
+ $logger = wc_get_logger();
+ $context = [ 'source' => 'update_vendor_commission' ];
+ $logger->error(
+ sprintf( 'Error processing vendor #%d: %s', $vendor_id, $e->getMessage() ),
+ $context
+ );
+ }
}📝 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 process_single_vendor( $vendor_id ) { | |
| $vendor = dokan()->vendor->get( $vendor_id ); | |
| $commission = $vendor->get_commission_settings(); | |
| $commission_type_old = $commission->get_type(); | |
| $commission->set_type( Fixed::SOURCE ); | |
| $percentage = $commission->get_percentage(); | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( 0 ); | |
| $commission->set_flat( $percentage ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( $percentage ); | |
| $commission->set_flat( 0 ); | |
| } | |
| $vendor->save_commission_settings( | |
| [ | |
| 'type' => $commission->get_type(), | |
| 'flat' => $commission->get_flat(), | |
| 'percentage' => $commission->get_percentage(), | |
| 'category_commissions' => $commission->get_category_commissions(), | |
| ] | |
| ); | |
| } | |
| public function process_single_vendor( $vendor_id ) { | |
| try { | |
| $vendor = dokan()->vendor->get( $vendor_id ); | |
| $commission = $vendor->get_commission_settings(); | |
| $commission_type_old = $commission->get_type(); | |
| $commission->set_type( Fixed::SOURCE ); | |
| $percentage = $commission->get_percentage(); | |
| if ( Flat::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( 0 ); | |
| $commission->set_flat( $percentage ); | |
| } elseif ( Percentage::SOURCE === $commission_type_old ) { | |
| $commission->set_percentage( $percentage ); | |
| $commission->set_flat( 0 ); | |
| } | |
| $vendor->save_commission_settings( | |
| [ | |
| 'type' => $commission->get_type(), | |
| 'flat' => $commission->get_flat(), | |
| 'percentage' => $commission->get_percentage(), | |
| 'category_commissions' => $commission->get_category_commissions(), | |
| ] | |
| ); | |
| } catch ( \Exception $e ) { | |
| // Log the exception or handle it as needed | |
| $logger = wc_get_logger(); | |
| $context = [ 'source' => 'update_vendor_commission' ]; | |
| $logger->error( | |
| sprintf( 'Error processing vendor #%d: %s', $vendor_id, $e->getMessage() ), | |
| $context | |
| ); | |
| } | |
| } |
| * @param int $term Category term object | ||
| * | ||
| * @return void | ||
| */ | ||
| public function process_single_category( $term_id ) { |
There was a problem hiding this comment.
Inconsistent parameter name and description in docblock
The docblock parameter is $term, described as 'Category term object', but the method process_single_category accepts $term_id, which is an integer term ID. Please update the docblock to match the method signature and clarify the parameter's purpose.
Apply this diff to correct the docblock:
- * @param int $term Category term object
+ * @param int $term_id Term ID to process📝 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.
| * @param int $term Category term object | |
| * | |
| * @return void | |
| */ | |
| public function process_single_category( $term_id ) { | |
| * @param int $term_id Term ID to process | |
| * | |
| * @return void | |
| */ | |
| public function process_single_category( $term_id ) { |
| /** | ||
| * The batch size for processing products | ||
| * | ||
| * @since DOKAN_PRO_SINCE | ||
| */ |
There was a problem hiding this comment.
Replace '@SInCE DOKAN_PRO_SINCE' placeholders with actual version numbers in docblocks
The @since DOKAN_PRO_SINCE annotations in the docblocks are placeholders and should be replaced with the actual version numbers where these constants and methods are introduced.
Also applies to: 19-23, 26-30, 37-43, 56-65, 83-92, 111-119, 132-138, 150-159, 183-189
| protected function get_total_products() { | ||
| $args = [ | ||
| 'status' => 'publish', | ||
| 'limit' => -1, | ||
| 'return' => 'ids', | ||
| ]; | ||
|
|
||
| $products = wc_get_products( $args ); | ||
| return count( $products ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize total product count retrieval
Retrieving all product IDs to count the total number of products can be inefficient for large datasets. Consider using the 'paginate' parameter in wc_get_products to get the total product count without loading all product IDs into memory.
Refactor the get_total_products method as follows:
protected function get_total_products() {
$args = [
'status' => 'publish',
'limit' => 1,
'paginate' => true,
'return' => 'ids',
];
$products = wc_get_products( $args );
return $products->total;
}| /** | ||
| * Process a single product | ||
| * Customize this method based on what you need to do with each product | ||
| * | ||
| * @since DOKAN_PRO_SINCE | ||
| * | ||
| * @param int $product | ||
| * | ||
| * @return void | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent parameter name in docblock
The docblock parameter name @param int $product does not match the method parameter $product_id. Please update the docblock to reflect the correct parameter name.
Apply this diff to fix the inconsistency:
/**
* Process a single product
* Customize this method based on what you need to do with each product
*
* @since DOKAN_PRO_SINCE
*
- * @param int $product
+ * @param int $product_id
*
* @return void
*/📝 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.
| /** | |
| * Process a single product | |
| * Customize this method based on what you need to do with each product | |
| * | |
| * @since DOKAN_PRO_SINCE | |
| * | |
| * @param int $product | |
| * | |
| * @return void | |
| */ | |
| /** | |
| * Process a single product | |
| * Customize this method based on what you need to do with each product | |
| * | |
| * @since DOKAN_PRO_SINCE | |
| * | |
| * @param int $product_id | |
| * | |
| * @return void | |
| */ |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
includes/Commission/Upugrader/Update_Vendor_Commission.php (2)
20-20: Consider increasing the batch size for better performanceA batch size of 10 might be too small for large-scale processing. Consider increasing it to 50-100 for better performance while maintaining memory efficiency.
- const BATCH_SIZE = 10; + const BATCH_SIZE = 50;
81-81: Document the reason for PHPCS ignoreThe
phpcs:ignorecomment should include the specific rule being ignored and why it's necessary.- public function process_batch( $page_number, $max_pages ) { // phpcs:ignore + public function process_batch( $page_number, $max_pages ) { // phpcs:ignore WordPress.WP.CronInterval.ChangeDetected -- Batch processing requires custom intervalsincludes/Commission/Upugrader/Update_Product_Commission.php (1)
57-57: Inconsistent@sincetags in docblocksThe
@sincetags use bothDOKAN_SINCEandDOKAN_PRO_SINCE. Please ensure consistency across the file by using the appropriate tag. If this class is part of Dokan Pro, consider using@since DOKAN_PRO_SINCEconsistently.Also applies to: 122-122
includes/Commission/Upugrader/Update_Category_Commission.php (5)
63-63: Correct the comment: 'products' should be 'categories'The comment at line 63 incorrectly mentions 'products' instead of 'categories'. Update it to reflect that it retrieves the total number of categories.
Apply this diff to correct the comment:
- // Get total number of products + // Get total number of categories
86-88: Inconsistent parameter names in docblocksThe docblocks for
process_batch,schedule_next_batch, andget_categories_batchrefer to the parameter$page_number, but the method signatures use$offset. Please update the docblocks to match the actual parameter names for clarity.Apply these diffs to correct the docblocks:
For
process_batch:- * @param int $page_number Current page number + * @param int $offset Current offsetFor
schedule_next_batch:- * @param int $page_number Next page number to process + * @param int $offset Next offset to processFor
get_categories_batch:- * @param int $page_number Page number to fetch + * @param int $offset Offset to fetchAlso applies to: 106-108, 140-142
160-163: Correct the @return type in docblockThe
category_countmethod should return an integer or aWP_Error, but the current @return annotation lists multiple types that are not applicable. Please update it to accurately reflect the actual return types.Apply this diff to correct the @return annotation:
- * @return int[]|string|string[]|\WP_Error|\WP_Term[] + * @return int|\WP_Error
13-13: Ensure consistent use of @SInCE tagsThe
@sincetags in the docblocks use bothDOKAN_PRO_SINCEandDOKAN_SINCE. For consistency and clarity, please standardize the@sincetags throughout the file.Apply this diff to standardize the
@sincetags:- * @since DOKAN_SINCE + * @since DOKAN_PRO_SINCEAlso applies to: 28-28, 43-43, 58-58, 84-84, 104-104, 121-121, 138-138, 160-160, 177-177, 215-215
123-127: Clarify parameter naming for consistencyIn the
schedule_cat_itemmethod, the parameter is named$term, but it represents a term ID. For clarity, consider renaming the parameter to$term_idand updating the docblock accordingly.Apply this diff to update the parameter name:
/** - * @param $term + * @param int $term_id Term ID to schedule * * @return void */ - private function schedule_cat_item( $term ) { + private function schedule_cat_item( $term_id ) {Also, update the method calls to pass
$term_id:- $this->schedule_cat_item( $category->term_id ); + $this->schedule_cat_item( $term_id );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/Commission/Upugrader/Update_Category_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Product_Commission.php(1 hunks)includes/Commission/Upugrader/Update_Vendor_Commission.php(1 hunks)
🔇 Additional comments (5)
includes/Commission/Upugrader/Update_Vendor_Commission.php (4)
3-3: Typo in namespace 'Upugrader'
This issue was previously identified. The namespace should be Upgrader instead of Upugrader.
140-151: Fix commission conversion logic
The previously identified issue with commission value conversion is still present. The code incorrectly uses the percentage value for both flat and percentage commission types.
199-202: LGTM!
The implementation correctly checks both batch and item processing queues.
135-160: Verify commission updates don't affect existing orders
Please ensure that the commission updates only affect future orders and don't modify commissions for existing orders.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information to verify the concern about commission updates affecting existing orders.
Commission updates are safely isolated to future orders
The code review concern can be marked as verified safe because:
- The commission settings are stored separately from order data
- Existing order commissions are stored as order item meta (
_dokan_commission_rate,_dokan_commission_type,_dokan_additional_fee) - The vendor commission update code in
Update_Vendor_Commission.phponly modifies vendor settings and doesn't touch any order meta data - There is no code that retroactively updates commissions on existing orders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any modifications to existing order commissions
# Search for order commission updates
rg -A 5 "update.*commission" --type php
# Check for order meta modifications
rg -A 5 "_dokan_commission" --type php
Length of output: 8158
includes/Commission/Upugrader/Update_Product_Commission.php (1)
64-64:
Question about adding 50 to $total_products
Adding 50 to the total number of products may lead to scheduling more batches than necessary, potentially processing non-existent products. Is this adjustment intentional? It might be masking an issue with the product count calculation.
| $counts = dokan_get_seller_status_count(); | ||
| $total_items = $counts['total']; | ||
| $max_pages = ceil( $total_items / self::BATCH_SIZE ); | ||
| $max_pages = $max_pages + 5; |
There was a problem hiding this comment.
Remove arbitrary page padding
Adding 5 extra pages without clear justification could lead to unnecessary processing cycles. If this is needed for edge cases, please document the reason.
- $max_pages = $max_pages + 5;📝 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.
| $max_pages = $max_pages + 5; |
| private function log_vendor_update( $vendor_id, $success, $error_message = '' ) { | ||
| $log_key = 'dokan_commission_upgrade_vendor_' . $vendor_id; | ||
| update_option( | ||
| $log_key, [ | ||
| 'status' => $success ? 'success' : 'error', | ||
| 'error_message' => $error_message, | ||
| 'timestamp' => current_time( 'mysql' ), | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use WC_Logger instead of options table for logging
Storing logs in the options table can lead to performance issues with large numbers of vendors. Consider using WooCommerce's logging system instead.
- private function log_vendor_update( $vendor_id, $success, $error_message = '' ) {
- $log_key = 'dokan_commission_upgrade_vendor_' . $vendor_id;
- update_option(
- $log_key, [
- 'status' => $success ? 'success' : 'error',
- 'error_message' => $error_message,
- 'timestamp' => current_time( 'mysql' ),
- ]
- );
+ private function log_vendor_update( $vendor_id, $success, $error_message = '' ) {
+ $logger = wc_get_logger();
+ $context = [
+ 'source' => 'dokan_commission_upgrade',
+ 'vendor_id' => $vendor_id
+ ];
+ if ($success) {
+ $logger->info(
+ sprintf('Successfully processed vendor #%d', $vendor_id),
+ $context
+ );
+ } else {
+ $logger->error(
+ sprintf('Failed to process vendor #%d: %s', $vendor_id, $error_message),
+ $context
+ );
+ }
+ }📝 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 log_vendor_update( $vendor_id, $success, $error_message = '' ) { | |
| $log_key = 'dokan_commission_upgrade_vendor_' . $vendor_id; | |
| update_option( | |
| $log_key, [ | |
| 'status' => $success ? 'success' : 'error', | |
| 'error_message' => $error_message, | |
| 'timestamp' => current_time( 'mysql' ), | |
| ] | |
| ); | |
| } | |
| private function log_vendor_update( $vendor_id, $success, $error_message = '' ) { | |
| $logger = wc_get_logger(); | |
| $context = [ | |
| 'source' => 'dokan_commission_upgrade', | |
| 'vendor_id' => $vendor_id | |
| ]; | |
| if ($success) { | |
| $logger->info( | |
| sprintf('Successfully processed vendor #%d', $vendor_id), | |
| $context | |
| ); | |
| } else { | |
| $logger->error( | |
| sprintf('Failed to process vendor #%d: %s', $vendor_id, $error_message), | |
| $context | |
| ); | |
| } | |
| } |
| $args = [ | ||
| 'status' => 'publish', | ||
| 'limit' => self::BATCH_SIZE, |
There was a problem hiding this comment.
Inconsistent product statuses between total count and batch retrieval
In get_total_products(), the products are fetched with 'status' => 'any', whereas in get_products_batch(), 'status' => 'publish'. This inconsistency may cause the total product count to be inaccurate, leading to scheduling extra batches or missing some products during processing.
Apply this diff to ensure consistent product statuses:
protected function get_total_products() {
$args = [
- 'status' => 'any',
+ 'status' => 'publish',
'limit' => -1,
'return' => 'ids',
];Also applies to: 165-167
| return; | ||
| } | ||
|
|
||
| $total = $total + 50; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary addition of 50 to total category count
Adding a fixed number to the total category count at line 70 may result in scheduling empty batches and inefficient processing. Consider removing this addition or adjusting it to accurately reflect the total number of categories.
Apply this diff to remove the unnecessary addition:
- $total = $total + 50;📝 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.
| $total = $total + 50; |
shashwatahalder01
added
QA approved
All Submissions:
Changes proposed in this Pull Request:
Fixed
Changelog entry
No change log needed.
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
Chores