-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix decimal and thousand separator for category commission. #2440
base: develop
Are you sure you want to change the base?
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
includes/Product/Hooks.php (1)
494-494
: Consider adding thestep
attribute 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.vue
that checks:- Commission type switching
- All required fields for category-based commission
- Both percentage and flat rate values
- Clear error state management through
arrayRemove
andpush
operations - Proper data structure validation ensuring
commission_category_based_values
has 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_commission
flag
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_type
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Scenario: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ); | |
} |
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