Fix admin fixed commission setting validation #2437
Conversation
Restrict admin commission percentage from 0 to 100
WalkthroughThe changes involve significant updates to the Changes
Suggested reviewers
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 (5)
src/admin/components/CombineInput.vue (3)
81-88: LGTM! Consider enhancing validation robustness.The validation logic correctly implements the 0-100 range restriction for percentage values. However, there's room for improvement:
Consider these enhancements:
+const MIN_PERCENTAGE = 0; +const MAX_PERCENTAGE = 100; watch: { value: { handler(newVal, oldVal) { let percentage = newVal.percentage; - if ( ! percentage || '' === percentage || Number( percentage ) < 0 || Number( percentage ) > 100 ) { + if ( ! percentage || + '' === percentage || + isNaN(Number(percentage)) || + Number( percentage ) < MIN_PERCENTAGE || + Number( percentage ) > MAX_PERCENTAGE ) { percentage = oldVal.percentage; }
95-98: Consider adding error handling for number formatting.The use of
accounting.unformatis good for consistent number handling, but consider adding error handling:onInput: Debounce( function() { let self = this; + try { let data = { fixed: self.fixed ? accounting.unformat(self.fixed, dokan.currency.decimal) : '', percentage: self.percentage ? accounting.unformat(self.percentage, dokan.currency.decimal): '' }; this.$emit('change', data); + } catch (error) { + console.error('Error formatting input values:', error); + // Emit error event or handle gracefully + this.$emit('error', error); + } }, 500 ),
Line range hint
53-56: Fix typo in prop name 'fixexName'.There's a typo in the prop name that should be corrected for better code maintainability:
-fixexName: { +fixedName: { type: String, default: 'fixed-val-name' },Remember to update any corresponding template bindings:
-:name="fixexName" +:name="fixedName"includes/Admin/SetupWizard.php (1)
557-564: LGTM! The commission validation looks good.The added validation ensures that the admin commission percentage stays within the valid range of 0-100%. This fixes the issue mentioned in the PR objectives.
However, consider adding a notice when an invalid value is provided and defaulted to 0.
$dokan_commission_percentage = isset( $_POST['dokan_commission_percentage'] ) ? (float) wc_format_decimal( sanitize_text_field( wp_unslash( $_POST['dokan_commission_percentage'] ) ) ) : 0; if ( $dokan_commission_percentage < 0 || $dokan_commission_percentage > 100 ) { + add_settings_error( + 'dokan_commission', + 'dokan_commission_invalid', + __( 'Commission percentage must be between 0 and 100. Value has been set to 0.', 'dokan-lite' ), + 'error' + ); $dokan_commission_percentage = 0; }includes/Admin/Settings.php (1)
73-83: Consider enhancing the validation feedback.While the validation logic is correct, consider these improvements:
- Add an admin notice when reverting to the saved value
- Use
number_formatfor consistent decimal handlingpublic function validate_fixed_price_values( $option_values, $option_name ) { $clickable_types = [ 'flat', 'fixed' ]; if ( 'dokan_selling' === $option_name && isset( $option_values['commission_type'] ) && in_array( $option_values['commission_type'], $clickable_types, true ) ) { - $admin_percentage = (float) $option_values['admin_percentage']; + $admin_percentage = number_format( (float) $option_values['admin_percentage'], 2, '.', '' ); $saved_admin_percentage = dokan_get_option( 'admin_percentage', 'dokan_selling', '' ); if ( $admin_percentage < 0 || $admin_percentage > 100 ) { + add_settings_error( + 'admin_percentage', + 'invalid_percentage', + sprintf( __( 'Commission percentage must be between 0-100. Reverting to previous value: %s%%', 'dokan-lite' ), $saved_admin_percentage ), + 'error' + ); $option_values['admin_percentage'] = $saved_admin_percentage; } } return $option_values; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/Admin/Settings.php(2 hunks)includes/Admin/SetupWizard.php(2 hunks)src/admin/components/CombineInput.vue(1 hunks)
🔇 Additional comments (3)
includes/Admin/Settings.php (3)
33-33: LGTM: Filter hook addition is correct.
The new filter hook for commission validation is properly added with appropriate priority.
64-72: LGTM: Method documentation is complete.
The method documentation properly describes the purpose, parameters, and return type.
73-83: Verify commission validation integration.
Let's verify the integration with other commission-related functionality.
✅ Verification successful
Let me gather more specific information about the commission validation implementation.
Let me check one more aspect to verify the commission validation integration.
Based on the gathered information, I can now provide the final response:
Commission validation is properly integrated and functioning as expected
The validation implementation is correctly integrated into the settings workflow:
- The
validate_fixed_price_valuesmethod is hooked intodokan_save_settings_valuefilter with proper priority - The validation is specifically targeted for fixed/flat commission types
- The implementation is consistent with the commission settings structure across the codebase
- Test cases confirm the expected behavior for various commission scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other commission validation implementations and usages
# Check for other commission validation logic
echo "Checking for other commission validation implementations..."
rg -g '!vendor' -l "admin_percentage|commission.*validation"
# Check for filter usage
echo "Checking for filter usage..."
rg -g '!vendor' "dokan_save_settings_value"
Length of output: 1272
Script:
#!/bin/bash
# Check the implementation in Commission Settings classes
echo "Checking Commission Settings implementations..."
rg -A 10 "class.*Setting" includes/Commission/Settings/
# Check for other admin_percentage validations
echo "Checking for other admin_percentage validations..."
rg -A 5 "admin_percentage.*validation|validate.*admin_percentage" --type php
# Check the actual usage of validate_fixed_price_values
echo "Checking validate_fixed_price_values usage..."
ast-grep --pattern 'validate_fixed_price_values($$$)'
Length of output: 3944
Script:
#!/bin/bash
# Check the implementation of commission types and their validation
echo "Checking commission type implementations..."
rg -A 10 "commission_type.*flat|commission_type.*fixed" --type php
# Check for any other validation hooks
echo "Checking related validation hooks..."
rg -A 5 "add_filter.*dokan_save_settings|add_filter.*dokan_settings" --type php
# Check the actual settings saving implementation
echo "Checking settings save implementation..."
rg -A 10 "function.*save_settings" includes/Admin/
Length of output: 15941
Restrict admin commission percentage from 0 to 100
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/admin/components/CombineInput.vue (4)
Line range hint
39-42: Fix typo in prop namefixexNameThe prop name contains a typo:
fixexNameshould befixedName. This typo is inconsistent with the component's naming conventions and could cause confusion.Apply this fix:
- fixexName: { + fixedName: { type: String, default: 'fixed-val-name' },
84-84: Remove debug console.log statementDebug logging should not be included in production code.
- console.log(newPercentage, oldPercentage);
86-88: Simplify validation conditionThe validation condition can be simplified by leveraging the
validatePercentagemethod that's already doing part of this validation.- if ( ! newPercentage || '' === newPercentage || Number( newPercentage ) < 0 || Number( newPercentage ) > 100 ) { + if ( ! newPercentage || newPercentage === '' ) { newPercentage = oldPercentage; }
97-103: Enhance percentage validation robustnessWhile the current validation handles the range check, it could be more robust by handling non-numeric inputs and undefined/null values.
validatePercentage( percentage ) { + if (percentage === undefined || percentage === null) { + return ''; + } + const numValue = Number(percentage); + if (isNaN(numValue)) { + return ''; + } - if ( Number( percentage ) < 0 || Number( percentage ) > 100 ) { + if ( numValue < 0 || numValue > 100 ) { percentage = ''; } return percentage; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/admin/components/CombineInput.vue(1 hunks)
🔇 Additional comments (2)
src/admin/components/CombineInput.vue (2)
105-109: LGTM! Well-implemented input handling
The input handler correctly implements:
- Proper debouncing for performance optimization
- Consistent currency formatting using accounting.unformat
- Clean data structure emission
97-103: Verify commission validation coverage
While the validation logic here is correct, we should verify that commission percentage validation is consistently applied across all entry points.
Also applies to: 105-109
✅ Verification successful
Let me gather more information with proper file type specifications and additional context.
Based on the comprehensive search results, I can now provide a final response:
Commission validation is properly implemented across the system
The validation of commission percentage between 0-100 is consistently enforced through multiple layers:
- UI validation in
CombineInput.vue - Backend validation through
dokan()->commission->validate_rate()used across all commission formulas (Fixed, Flat, Percentage, Combine, CategoryBased) - Setup wizard validation in
includes/Admin/SetupWizard.phpenforces the same range - Commission settings are properly saved and validated through dedicated Settings classes for Global, Vendor, Product, and OrderItem contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other commission percentage validation implementations
echo "Searching for commission percentage validation patterns..."
rg -l "commission|percentage" --type php --type vue
# Search for direct database updates to commission settings
echo "Checking for direct commission updates..."
rg -l "update_option.*commission" --type php
Length of output: 392
Script:
#!/bin/bash
# Search for commission-related code with proper file extensions
echo "Searching for commission validation patterns..."
rg -l "commission|percentage" -g "*.php" -g "*.vue"
# Get context around commission validation
echo "Checking commission validation implementations..."
rg -B 2 -A 2 "validatePercentage|validate.*commission|check.*percentage" -g "*.php" -g "*.vue"
# Check database operations
echo "Checking database operations related to commission..."
rg -B 2 -A 2 "(update|save|set).*commission" -g "*.php" -g "*.vue"
Length of output: 65352
Restrict admin commission percentage from 0 to 100
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style