Fix several commission issues #2502
Conversation
…roduct edit page the earning suggestion shows the wrong value.
…fferent decimal and thousand separator, on change of price the earning suggestion updates to wrong decimal and thousand separator.
… suggestion should display zero, but it still shows the previous value. Fixed: When using a decimal comma separator, the decimal value doesn’t work for simple products, and variable products display as zero. (both regular price & discounted price) Fixed: Fatal Error on negative value for regular price: If a negative value is given for price (both simple and variable), there is a fatal error.
WalkthroughThis pull request introduces comprehensive improvements across multiple files in the Dokan plugin, focusing on product pricing, commission calculations, and code quality. The changes span JavaScript and PHP files, enhancing functionality in the product editor, commission handling, and product category management. Key modifications include adding new methods for money formatting, improving type handling, fixing method naming, and updating test coverage for various product and commission scenarios. Changes
Assessment against linked issues
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
🧹 Nitpick comments (8)
includes/ProductCategory/Helper.php (2)
68-68: Improve error handling for empty product terms
Retrieving terms throughget_product_termscentralizes logic, but consider logging or gracefully handling cases when terms unexpectedly return empty to improve debugging.
309-327: Centralize product term retrieval
The newget_product_termsmethod consolidates term retrieval. This is beneficial for maintainability. However, ensure that no legacy code duplicates the retrieval approach.includes/Commission.php (2)
593-596: Clarify decimal separator comment
The note about decimal comma usage is helpful. Consider adding a test scenario for it or referencing a known issue or solution to help maintainers quickly identify the context.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
602-602: Guard against empty product object
When retrieving$product, ensure$productis valid before calling$product->get_price(). This is partially handled, but a more explicit check and potential warning could help.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
assets/src/js/product-editor.js (4)
962-962: Safeguard against undefined or non-numeric values.
When passingpriceAmountinto the AJAX request, consider parsing or validating it to ensure you’re not sending unexpected strings.
988-990: Typographical nitpick.
The method name is spelledunormatMoney; consider renaming it tounformatMoneyfor clarity.
995-997: Use descriptive variable names.
vandorPriceis likely meant to bevendorPrice. Renaming helps avoid confusion in the future.
1000-1000: Potentially refine negative price handling.
Blocking form submission if the vendor price is negative may be appropriate, but confirm whether partial refunds or other scenarios could yield negative amounts in legitimate cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
assets/src/js/product-editor.js(3 hunks)includes/Commission.php(3 hunks)includes/ProductCategory/Helper.php(4 hunks)includes/REST/CommissionControllerV1.php(1 hunks)templates/products/edit-product-single.php(1 hunks)tests/php/src/Commission/CommissionTest.php(24 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
assets/src/js/product-editor.js
[error] 968-968: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 GitHub Actions: Inspections
includes/Commission.php
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
🔇 Additional comments (35)
includes/REST/CommissionControllerV1.php (1)
136-138: Confirm usage of new boolean parameters
Two additional boolean parameters (false, false) are being passed todokan()->commission->get_commission. Ensure these align with the new logic inCommissionand handle default operations as intended.Would you like me to generate a quick script to search for calls to
get_commissionacross the codebase to confirm consistent usage of these new parameters?includes/ProductCategory/Helper.php (3)
5-5: Confirm WC_Product import usage
The import ofWC_Productis a good addition. Validate that no naming conflicts exist and that the class references remain consistent throughout the file.
262-287: Validate the new product type checks
When$productis not an instance ofWC_Product, the code attemptswc_get_product. Confirm that this logic covers all product scenarios, including edge cases like deleted or non-purchasable items.
301-301: No direct issues
Line 301 is introducing usage ofget_product_terms; looks good and consistent with the new approach.includes/Commission.php (3)
177-177: Type consistency improved withfloatval
Switching from(float)tofloatval()is fine. Verify no downstream float-cast behavior changes.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
565-570: Ensure backward compatibility of new parameter
The addition of$override_total_amount_by_product_priceinget_commissionextends functionality. Confirm that legacy calls still work correctly when the third parameter is not passed.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
598-598: Override total amount check
$override_total_amount_by_product_pricelogic ensures a fallback to product price. Double-check for scenarios where$product->get_price()might be zero or missing due to discount edge cases.🧰 Tools
🪛 GitHub Actions: Inspections
[error] Variable "$orderItemId" is not in valid snake_case format, should be "$order_item_id"
templates/products/edit-product-single.php (1)
295-295: Enhance user message for vendor earnings
The inline string “You Earn” is now complemented by a properly formatted price display. This is a good approach. Optionally, consider localizing more robustly, ensuring all currency formatting is consistent sitewide.assets/src/js/product-editor.js (4)
952-953: Consider validating the price input before assigning.
Currently,priceAmountis set to eithersale_priceorproduct_price, which may be uninitialized or non-numeric. Adding an explicit check or parse step, in addition to the default of0, can provide more clarity and avoid unintended values.
978-986: Ensure consistent decimal precision across your application.
formatMoneyspecifies a precision of 4. Verify that this aligns with the rest of your pricing logic and currency settings elsewhere in the code.
998-999: Validate unformatted vendor price.
Before or after unformatting the vendor price, consider verifying numeric correctness.
969-969:⚠️ Potential issueAvoid using the global
isNaNfor type checks.
According to static analysis,isNaNperforms type coercions that can lead to unexpected results. UseNumber.isNaNwith a numeric cast instead.Below is an example fix:
-if ( ! isNaN( response ) ) { +if ( ! Number.isNaN( Number( response ) ) ) { earning_suggestion.html( Dokan_Editor.formatMoney( response ) ); }Likely invalid or redundant comment.
tests/php/src/Commission/CommissionTest.php (23)
116-132: Global commission configuration looks good.
No issues found in this logic; the fixed commission scenario is handled properly for smaller amounts.
Line range hint
199-230: Global settings updates appear consistent.
This chunk correctly sets single-quoted keys in$commission_global_options.
Line range hint
258-292: Category-based logic is consistent.
No issues here; code is cohesive and aligns with the test scenario.
314-332: Improved reliability for global strategy.
Implementation is consistent with the test plan described in the docblock.
354-362: Flat commission approach validated.
Code is straightforward and tested thoroughly for the flat-based setting.
383-392: Percentage fallback correctly configured.
Logic remains consistent with preceding test scenarios; no concerns.
416-425: Commission with vendor-specific percentage looks good.
The data set is tested accurately.
449-458: Combine commission approach.
No anomalies found in these global settings for the combine scenario.
482-491: Robust coverage of product-level combination commissions.
All references to array keys align with the single-quoted style.
515-524: Zero-price scenario gracefully handled.
Confirmed that the code properly covers edge cases for $0 amounts.
544-561: Commission distribution verified.
Handling of category_4 is in line with the existing logic.
584-601: Category-based commissions applied consistently.
These lines appropriately handle the same scenario with zero total price.
611-618: Properly reintegrates item-based settings.
No issues discovered with reassigning category IDs.
640-657: Category refinement is correct.
No concerns with the logic or formatting.
680-697: Ensures fallback to zero commission on null totals.
The code block is consistent with the overall approach.
707-714: Vendor-based category notices handled.
All changes appear correct and consistent with earlier patterns.
736-753: Global fee logic remains uniform.
The coverage for$settings['global_setting']is adequate.
763-770: Flat commission coverage extended.
No further issues detected.
792-809: Percentage commission coverage extended.
Implementation parallels existing tests.
819-826: Confirmed zero-value percentage logic.
No issues uncovered in this segment.
1041-1063: Commission settings storage looks correct.
The calls to$product_setting->save,$vendor_setting->save, and$global_setting->saveaccurately reflect the data arrays.
1136-1202: Data provider for variable products.
This new data provider method elegantly supplies consistent test parameters.
1204-1274: Variable product commission test.
Testing multiple variations under a single product covers more edge cases, improving reliability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/admin/components/CombineInput.vue (1)
100-101: DefineformatPositiveValueas a standard function instead of an arrow functionDefining methods as arrow functions can cause issues with
thisbinding in Vue components. It's recommended to use standard function definitions within themethodsobject to ensure proper context.Apply this diff to redefine the method:
- formatPositiveValue: ( value ) => { + formatPositiveValue( value ) {assets/src/js/setup-wizard/commission/AdminCommission.vue (1)
79-85: Improve commission value validation and type handling.While converting empty strings to 0 is reasonable, the method could benefit from more robust value validation.
Consider this enhanced implementation:
fixedCOmmissionhandler(data) { - if (data.fixed === '') { - data.fixed = 0; - } - if (data.percentage === '') { - data.percentage = 0; - } + // Convert to numbers and ensure non-negative values + data.fixed = data.fixed === '' ? 0 : Math.max(0, Number(data.fixed)); + data.percentage = data.percentage === '' ? 0 : Math.max(0, Math.min(100, Number(data.percentage))); this.fixedCommission = data; },This ensures:
- Explicit type conversion using
Number()- Non-negative commission values
- Percentage values between 0 and 100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/src/js/setup-wizard/commission/AdminCommission.vue(1 hunks)src/admin/components/CombineInput.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
🔇 Additional comments (3)
src/admin/components/CombineInput.vue (2)
12-13: Ensure proper synchronization after replacingv-modelwith:valueandv-on:inputReplacing
v-modelwith:valueandv-on:inputchanges the way data is synchronized between the input and the component's state. Ensure that theonInputmethod correctly emits updated values and that any two-way binding or reactivity is preserved.
34-35: Verify data binding consistency forfixedinputSimilar to the
percentageinput, confirm that replacingv-modelwith:valueandv-on:inputmaintains proper synchronization of thefixedvalue. Ensure that user input updates the component's state and is emitted correctly.assets/src/js/setup-wizard/commission/AdminCommission.vue (1)
79-85: Verify commission calculations with product editor integration.Given the PR's focus on fixing commission issues and its relation to product pricing changes, ensure comprehensive testing of commission calculations.
Please verify:
- Integration with the new money formatting methods in product editor
- Correct handling of commission calculations for both fixed and percentage types
- Edge cases with minimum/maximum commission values
✅ Verification successful
✓ Commission value handling is well-tested and follows established patterns
The code change follows the same pattern used throughout the codebase for handling empty commission values, with comprehensive test coverage across unit, integration, and API tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files and commission calculation logic rg -l "commission|pricing" "tests/" # Look for money formatting usage ast-grep --pattern 'formatMoney($_)'Length of output: 1432
Script:
#!/bin/bash # Search for AdminCommission related tests rg -l "AdminCommission" "tests/" # Look for test cases handling empty/zero commission values rg "commission.*0|commission.*empty" "tests/" -A 5 # Search for Vue component test files fd -e spec.js -e spec.ts -e test.js -e test.tsLength of output: 70249
| validateNegative( value ) { | ||
| if ( Number( value ) < 0 ) { | ||
| value = ''; | ||
| } | ||
|
|
||
| return value; | ||
| }, |
There was a problem hiding this comment.
Handle non-numeric and NaN values in validateNegative method
The validateNegative method uses Number(value), which can return NaN for invalid inputs. Comparing NaN with numbers results in false, so the condition Number(value) < 0 won't catch NaN values. This could lead to unexpected behavior.
Apply this diff to handle NaN values explicitly:
validateNegative( value ) {
+ let num = Number( value );
+ if ( isNaN( num ) || num < 0 ) {
+ value = '';
+ } else {
+ value = num;
+ }
return value;
},📝 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.
| validateNegative( value ) { | |
| if ( Number( value ) < 0 ) { | |
| value = ''; | |
| } | |
| return value; | |
| }, | |
| validateNegative( value ) { | |
| let num = Number( value ); | |
| if ( isNaN( num ) || num < 0 ) { | |
| value = ''; | |
| } else { | |
| value = num; | |
| } | |
| return value; | |
| }, |
| onInput: Debounce( function( currentValue, type ) { | ||
| let self = this; | ||
| let data = { | ||
| fixed: self.fixed ? accounting.unformat(self.fixed, dokan.currency.decimal) : '', | ||
| percentage: self.percentage ? accounting.unformat(self.percentage, dokan.currency.decimal): '' | ||
| }; | ||
| let data = JSON.parse( JSON.stringify( { ...self.value }) ); | ||
| data[ type ] = '' !== currentValue ? String( currentValue ) : ''; | ||
|
|
||
| data.fixed = '' !== data.fixed ? self.validateNegative( accounting.unformat( data.fixed, dokan.currency.decimal ) ) : ''; | ||
| data.percentage = '' !== data.percentage ? self.validateNegative( self.validatePercentage( accounting.unformat( data.percentage, dokan.currency.decimal ) ) ) : ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using JSON.parse(JSON.stringify(...)) for cloning objects
Using JSON.parse(JSON.stringify(...)) to clone objects can lead to issues with properties that cannot be serialized (like functions) and can be inefficient. It's better to use a more appropriate cloning method.
Apply this diff to simplify the cloning process:
- let data = JSON.parse( JSON.stringify( { ...self.value } ) );
+ let data = { ...self.value };If a deep clone is necessary, consider using a utility like lodash.cloneDeep or a similar function.
📝 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.
| onInput: Debounce( function( currentValue, type ) { | |
| let self = this; | |
| let data = { | |
| fixed: self.fixed ? accounting.unformat(self.fixed, dokan.currency.decimal) : '', | |
| percentage: self.percentage ? accounting.unformat(self.percentage, dokan.currency.decimal): '' | |
| }; | |
| let data = JSON.parse( JSON.stringify( { ...self.value }) ); | |
| data[ type ] = '' !== currentValue ? String( currentValue ) : ''; | |
| data.fixed = '' !== data.fixed ? self.validateNegative( accounting.unformat( data.fixed, dokan.currency.decimal ) ) : ''; | |
| data.percentage = '' !== data.percentage ? self.validateNegative( self.validatePercentage( accounting.unformat( data.percentage, dokan.currency.decimal ) ) ) : ''; | |
| onInput: Debounce( function( currentValue, type ) { | |
| let self = this; | |
| let data = { ...self.value }; | |
| data[ type ] = '' !== currentValue ? String( currentValue ) : ''; | |
| data.fixed = '' !== data.fixed ? self.validateNegative( accounting.unformat( data.fixed, dokan.currency.decimal ) ) : ''; | |
| data.percentage = '' !== data.percentage ? self.validateNegative( self.validatePercentage( accounting.unformat( data.percentage, dokan.currency.decimal ) ) ) : ''; |
| data.fixed = 0; | ||
| } | ||
| if (data.percentage === '') { | ||
| data.percentage = this.fixedCommission.percentage ?? 0; | ||
| data.percentage = 0; | ||
| } | ||
|
|
||
| this.fixedCommission = data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix method naming convention.
The method name fixedCOmmissionhandler has inconsistent capitalization. Following Vue.js naming conventions, method names should use camelCase.
Rename the method to fixedCommissionHandler:
- fixedCOmmissionhandler(data) {
+ fixedCommissionHandler(data) {Don't forget to update the template where this method is referenced:
- v-on:change="fixedCOmmissionhandler"
+ v-on:change="fixedCommissionHandler"Committable suggestion skipped: line range outside the PR's diff.
Aunshon
added
Needs: Testing
All Submissions:
Changes proposed in this Pull Request:
Fixed some bugs of commissions as mentioned in the issue.
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry