Enhancement/commission restructure #2027
Conversation
# Conflicts: # package-lock.json
Disabling preflight.
```js
corePlugins: {
preflight: false,
},
```
…sion-restructore # Conflicts: # package-lock.json # src/admin/components/Fields.vue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (8)
src/admin/components/CombineInput.vue (1)
113-113: Remove empty<style>tag.The empty
<style>tag can be safely removed as it does not introduce any styling changes.-<style scoped lang="less"> -</style>src/admin/components/Fields/WithdrawCharges.vue (1)
86-96: LGTM, but consider removing commented-out lines and adding error handling.The code changes are approved. The
chargeChangeHandlermethod correctly handles the case when thefixedandpercentagevalues are not numbers by assigning them the corresponding values fromfixedCommission.Consider removing the commented-out lines if they are no longer needed:
-// let positiveValue = this.unFormatValue(data); -// let formatedData = this.formatPositiveValue( positiveValue );Also, consider adding error handling for the case when
fixedCommissionis undefined to avoid potential errors:+if (!fixedCommission) { + console.error('fixedCommission is undefined'); + return; +} if (isNaN( data.fixed )) { data.fixed = fixedCommission.fixed ?? ''; } if (isNaN( data.percentage )) { data.percentage = fixedCommission.percentage ?? ''; }tests/pw/pages/storesPage.ts (1)
154-220: LGTM! The changes simplify and enhance theeditVendorfunction.The key improvements include:
- Updating the function signature to accept
sellerIdinstead of searching for the vendor by store name, which simplifies the navigation logic and makes the function more efficient.- Removing the extensive handling of user information, including billing and shipping addresses, which streamlines the function and focuses on essential vendor information updates.
- Adding a section for handling commission types, which enhances the vendor management capabilities by allowing more detailed financial arrangements.
- Simplifying the overall structure of the function, which improves readability and maintainability.
The function follows good practices such as:
- Using appropriate selectors and methods to interact with the page elements.
- Using environment variables to conditionally execute certain code blocks.
- Using TypeScript types to define the input parameters.
- Using async/await syntax to handle asynchronous operations.
- Using descriptive variable and parameter names.
- Using a consistent indentation, naming convention, spacing, and line length style.
Consider extracting repeated code blocks into separate methods to further improve readability and maintainability. For example, the code blocks for updating the vendor's address, social profiles, and payment options could be extracted into separate methods.
async updateVendorAddress(vendor: vendor) { await this.clearAndType(vendors.editVendor.street1, vendor.vendorInfo.street1); await this.clearAndType(vendors.editVendor.street2, vendor.vendorInfo.street2); await this.clearAndType(vendors.editVendor.city, vendor.vendorInfo.city); await this.clearAndType(vendors.editVendor.zipCode, vendor.vendorInfo.zipCode); await this.click(vendors.editVendor.country); await this.clearAndType(vendors.editVendor.countryInput, vendor.vendorInfo.country); await this.press(data.key.enter); await this.click(vendors.editVendor.state); await this.clearAndType(vendors.editVendor.stateInput, vendor.vendorInfo.state); await this.press(data.key.enter); } async updateVendorSocialProfiles(vendor: vendor) { await this.clearAndType(vendors.editVendor.facebook, vendor.vendorInfo.socialProfileUrls.facebook); await this.clearAndType(vendors.editVendor.flickr, vendor.vendorInfo.socialProfileUrls.flickr); await this.clearAndType(vendors.editVendor.twitter, vendor.vendorInfo.socialProfileUrls.twitter); await this.clearAndType(vendors.editVendor.youtube, vendor.vendorInfo.socialProfileUrls.youtube); await this.clearAndType(vendors.editVendor.linkedin, vendor.vendorInfo.socialProfileUrls.linkedin); await this.clearAndType(vendors.editVendor.pinterest, vendor.vendorInfo.socialProfileUrls.pinterest); await this.clearAndType(vendors.editVendor.instagram, vendor.vendorInfo.socialProfileUrls.instagram); } async updateVendorPaymentOptions(vendor: vendor) { await this.clearAndType(vendors.editVendor.accountName, vendor.vendorInfo.payment.bankAccountName); await this.clearAndType(vendors.editVendor.accountNumber, vendor.vendorInfo.payment.bankAccountNumber); await this.selectByValue(vendors.editVendor.accountType, vendor.vendorInfo.payment.bankAccountType); await this.clearAndType(vendors.editVendor.bankName, vendor.vendorInfo.payment.bankName); await this.clearAndType(vendors.editVendor.bankAddress, vendor.vendorInfo.payment.bankAddress); await this.clearAndType(vendors.editVendor.routingNumber, vendor.vendorInfo.payment.bankRoutingNumber); await this.clearAndType(vendors.editVendor.iban, vendor.vendorInfo.payment.bankIban); await this.clearAndType(vendors.editVendor.swift, vendor.vendorInfo.payment.bankSwiftCode); await this.clearAndType(vendors.editVendor.payPalEmail, vendor.vendorInfo.payment.email()); }Then, you can call these methods from the
editVendorfunction:async editVendor(sellerId: string, vendor: vendor) { // ... await this.updateVendorAddress(vendor); await this.updateVendorSocialProfiles(vendor); await this.updateVendorPaymentOptions(vendor); // ... }tests/pw/utils/helpers.ts (3)
179-201: LGTM, but remove the commentedconsole.log.The code changes are approved. The function correctly calculates the sales metrics from the line items.
Please remove the commented out
console.logstatement at line 197 before merging to production:-// console.log('subtotal:', subtotal, 'subtotalTax:', subtotalTax, 'subtotalCommission:', subtotalCommission, 'subtotalEarning:', subtotalEarning);
264-288: LGTM, but remove the debugging code.The code changes are approved. The function correctly calculates the total discount amount based on the provided subtotal, coupon lines, and the sequential application flag.
Please remove the debugging code block at line range 273-278 before merging to production:
-// only for debuging -if (!coupon.discount_type) { - //get coupon details from single order - const couponMeta = JSON.parse(coupon.meta_data[0].value); - coupon.discount_type = couponMeta[2]; - coupon.nominal_amount = couponMeta[3]; -}
291-308: LGTM, but remove the commentedconsole.log.The code changes are approved. The function correctly calculates the commission or earning amount based on the provided details and adjusts the values based on the fee recipients and gateway fee giver.
Please remove the commented out
console.logstatement at line 304 before merging to production:-// console.log('productcommissionOrEarning', productcommissionOrEarning, 'productTax', productTax, 'shippingTax', shippingTax, 'shippingFee', shippingFee);tests/pw/feature-map/feature-map.yml (1)
242-257: Excellent work on expanding the commission management capabilities! Please ensure test coverage for the new features.The new
Commissionsection under theadminkey introduces a comprehensive set of features for managing commissions across various contexts such as the setup wizard, selling settings, vendors, products, and subscriptions. The feature names are clear, consistent, and cover key commission settings scenarios.These changes significantly enhance the functionality and flexibility of the commission system. Great job!
To maintain a high quality test suite, please make sure to add corresponding tests for each new feature introduced. This will help ensure the reliability and maintainability of the commission management functionality.
tests/pw/utils/dbData.ts (1)
71-90: LGTM! The changes introduce a more flexible commission system.The code changes are approved. The modifications significantly enhance the commission logic by:
- Moving from a percentage-based system to a fixed commission type.
- Allowing for differentiated commission rates based on categories through the
commission_category_based_valuesobject.- Utilizing the
CATEGORY_IDenvironment variable to set item-specific rates.These changes provide a more granular approach to commission management.
Consider adding more categories to the
commission_category_based_valuesobject to fully leverage the new structure. You can uncomment and modify the commented-out code for additional categories:items: { [CATEGORY_ID]: { percentage: '5', flat: '5', }, + '27': { + percentage: '8', + flat: '10', + }, + '42': { + percentage: '3', + flat: '7', + }, },This will allow you to set different commission rates for multiple categories, enhancing the flexibility of the commission system.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- dokan.php (2 hunks)
- src/admin/components/CombineInput.vue (4 hunks)
- src/admin/components/Fields/WithdrawCharges.vue (2 hunks)
- tests/pw/feature-map/feature-map.yml (3 hunks)
- tests/pw/pages/basePage.ts (1 hunks)
- tests/pw/pages/productsPage.ts (8 hunks)
- tests/pw/pages/selectors.ts (5 hunks)
- tests/pw/pages/settingsPage.ts (2 hunks)
- tests/pw/pages/storesPage.ts (2 hunks)
- tests/pw/pages/vendorBookingPage.ts (1 hunks)
- tests/pw/utils/apiUtils.ts (1 hunks)
- tests/pw/utils/dbData.ts (5 hunks)
- tests/pw/utils/helpers.ts (1 hunks)
- tests/pw/utils/pwMatchers.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/pw/utils/apiUtils.ts
Additional comments not posted (26)
tests/pw/utils/pwMatchers.ts (1)
57-85: LGTM!The
toBeApproximatelycustom matcher function is well-implemented and enhances the testing framework by providing a more flexible way to assert numerical equality with tolerance. The function includes useful features such as colored console output, warning messages, and table output of logs. It follows the naming conventions, code structure, and formatting of the codebase. The function is properly exported and added to thecustomExpectobject.Great job!
src/admin/components/CombineInput.vue (5)
2-39: Address past review comments to improve accessibility and localization.The changes to the template structure and styling are approved as they improve the responsiveness and alignment of the input fields.
However, please address the following past review comments:
- Make the input fields translatable.
- Add
aria-labeloraria-labelledbyattributes to the input fields to improve accessibility.
67-68: LGTM!The change to the default value of the
valueprop is approved.
74-75: LGTM!The change to the initialization of the
fixedandpercentagedata properties is approved.
96-96: LGTM!The change to the debounce timing is approved as it improves the responsiveness of the component without introducing performance issues.
98-100: LGTM!The addition of the
NaNcheck in theformatPositiveValuemethod is approved as it improves the input validation and consistency of the component.src/admin/components/Fields/WithdrawCharges.vue (1)
61-66: LGTM!The code changes are approved. The
formatPositiveValuemethod correctly formats the fixed and percentage values using theaccountinglibrary based on the specified currency precision and formatting options from thedokanglobal object. The method name accurately describes its functionality.tests/pw/pages/vendorBookingPage.ts (1)
29-35: LGTM!The changes to click on sub-menu elements instead of the main elements for both the general settings and booking costs sections are approved. This adjustment aligns with the updated UI structure and potentially enhances the organization and user experience.
dokan.php (2)
54-54: LGTM!The code changes are approved. The addition of the
$feesproperty and the corresponding documentation update are valid enhancements to theWeDevs_Dokanclass.
411-411: LGTM!The code changes are approved. The instantiation of the
WeDevs\Dokan\Feesclass and its assignment to the$this->container['fees']key is a valid addition to theinit_classesmethod, ensuring that the fees management component is properly initialized.tests/pw/utils/helpers.ts (5)
174-177: LGTM!The code changes are approved. The function correctly calculates the total tax for line items using the provided tax rate.
203-215: LGTM!The code changes are approved. The function correctly calculates the subtotal and total tax for line items using the provided tax rate.
217-230: LGTM!The code changes are approved. The function correctly calculates the subtotal with discount, subtotal without discount, and total tax for line items using the provided tax rate.
233-248: LGTM!The code changes are approved. The function correctly calculates the total admin commission and total vendor earning from the line items metadata.
251-254: LGTM!The code changes are approved. The function correctly calculates the tax amount using the provided rate and rounds it to two decimal places.
tests/pw/pages/settingsPage.ts (2)
114-116: LGTM!The code changes to update the commission settings are well-structured and follow good practices. The changes correctly update the commission type, percentage, and fixed amounts based on the provided
sellingobject.
144-145: LGTM!The code changes to verify the saved commission settings are a good practice to ensure that the settings are updated as expected.
tests/pw/pages/productsPage.ts (4)
54-54: LGTM!The code change is approved. It correctly updates the selector to use the submenu for general product settings.
60-60: LGTM!The code change is approved. It correctly updates the selector to use the submenu for inventory settings.
100-100: LGTM!The code changes are approved. They correctly update the selectors to use the submenus for attributes and variations.
Also applies to: 116-117
170-170: LGTM!The code changes are approved. They correctly update the selectors to use the submenus for attributes and variations, consistent with the changes made in
addVariableProduct.Also applies to: 187-188
tests/pw/utils/dbData.ts (2)
1336-1336: LGTM!The code change is approved. The removal of the line break in the
contentproperty improves the readability without altering the functionality.
1375-1375: LGTM!The code change is approved. Removing the line break in the
setup_wizard_message_without_htmlproperty improves the readability of the message without affecting its content.tests/pw/pages/basePage.ts (1)
317-321: LGTM!The new
selectAndWaitForResponsemethod is a great addition to theBasePageclass. It enhances the functionality by allowing complex interactions with web elements that depend on network responses, improving the robustness of automated testing scenarios. The implementation follows the existing coding style and conventions, and the method name and parameters are clear and descriptive. Well done!tests/pw/pages/selectors.ts (2)
686-696: LGTM!The new selectors for the commission settings look good. They follow a consistent naming convention, use appropriate locators, and provide a clear way to interact with the commission fields. The code changes are approved.
3096-3108: LGTM!The addition of a separate
commissionobject within theproductobject to group the commission-related selectors is a good approach. It improves the organization and readability of the selectors. The selectors themselves are consistent with the previously added commission selectors. The code changes are approved.
| } | ||
| }, | ||
| }, | ||
| data() { | ||
| return { | ||
| fixed: this.formatPositiveValue( this.value.fixed ) ?? 0, | ||
| percentage: this.formatPositiveValue( this.value.percentage ) ?? 0 | ||
| fixed: this.formatPositiveValue( this.value.fixed ) ?? '', |
There was a problem hiding this comment.
this.value.fixed.trime() === '' ? this.formatPositiveValue( this.value.fixed ) : ''
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (4)
src/admin/components/Commission/CategoryBasedCommission.vue (4)
176-203: Optimize the category rearrangement logic.The
getCatgroiesmethod rearranges the fetched categories based on their parent-child relationship. However, it iterates over the categories multiple times, which can be inefficient for large datasets. Consider optimizing the logic to minimize the number of iterations.
220-231: Optimize the category filtering logic.The
getChildrenmethod filters the categories array on each invocation to find the child categories of a given parent category. This can be inefficient if the method is called frequently. Consider optimizing the logic by pre-computing the parent-child relationships or using a more efficient data structure.
253-277: Simplify the commission update logic.The
commissinItemHandlermethod performs multiple operations to update the commission values and emit the change event. Consider simplifying the logic by extracting the common parts into separate methods and using more concise syntax.
302-344: Use a simpler deep equality check.The
isEqualmethod performs a deep equality check between two values. However, the implementation is quite verbose and can be simplified. Consider using a simpler deep equality check function or a third-party library like Lodash'sisEqualfunction.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- assets/src/js/setup-wizard/commission/AdminCommission.vue (1 hunks)
- includes/Admin/SetupWizard.php (5 hunks)
- src/admin/components/CombineInput.vue (4 hunks)
- src/admin/components/Commission/CategoryBasedCommission.vue (1 hunks)
Additional comments not posted (15)
assets/src/js/setup-wizard/commission/AdminCommission.vue (7)
6-8: LGTM!The dropdown menu is correctly bound to the
selectedCommissiondata property usingv-model, and the options are dynamically generated using thev-fordirective.
11-24: LGTM!The conditional rendering of the
CategoryBasedCommissionandCombineInputcomponents based on the selected commission type usingv-ifandv-else-ifdirectives is implemented correctly.
26-29: LGTM!The hidden input fields are correctly bound to the corresponding data properties to store the selected commission type and values.
51-65: LGTM!The
createdlifecycle hook correctly retrieves the commission data from the hidden DOM element and initializes the component's data properties.
68-70: LGTM!The
onCategoryUpdatemethod correctly updates thecommissiondata property when theCategoryBasedCommissioncomponent emits a change event.
87-89: No styles to review.The style section is empty, so there are no styles to review for this component.
1-89: Component review summaryThe
AdminCommissioncomponent is generally well-structured and follows Vue.js best practices. The template section correctly implements the conditional rendering of child components and binds the data properties to the appropriate elements. The script section handles the component's logic, including data initialization and event handling, effectively.However, one issue was identified in the
fixedCOmmissionhandlermethod regarding the handling of empty input values. It is recommended to address this issue by improving the user experience and providing meaningful feedback to the user when invalid or empty values are entered.Overall, the component is in good shape, but addressing the identified issue will further enhance its functionality and usability.
src/admin/components/CombineInput.vue (5)
2-39: Improved template structure and responsiveness!The redesigned template structure using flexbox enhances the overall layout, responsiveness, and alignment of the input fields. The addition of
refattributes and responsive design classes further improves the component's accessibility and user experience across different screen sizes.
67-68: Verify the impact of thevalueprop change.The default value of the
valueprop has been modified from numbers to empty strings. Please ensure that this change does not adversely affect the component's initialization and communication with its parent component.
74-75: Verify the impact of the data property changes.The
fixedandpercentagedata properties now initialize as empty strings instead of zeros. Please ensure that this change does not introduce any issues with input handling and validation.
96-96: Improved responsiveness with reduced debounce timing!The reduction of the debounce timing from 1600ms to 500ms enhances the component's responsiveness to input changes. This ensures that the parent component receives the updated values more quickly, leading to a better user experience.
99-101: Enhanced input validation with empty string check!The inclusion of a check for empty strings in the
formatPositiveValuemethod improves the input validation process. By returning the empty string directly, the component avoids unnecessary formatting and ensures that empty inputs are handled correctly.includes/Admin/SetupWizard.php (3)
507-527: LGTM!The function retrieves the necessary commission-related options and prepares the arguments for rendering the commission setup template correctly. The logic is sound and the implementation looks good.
554-569: Looks good!The function properly handles saving the commission options submitted via the setup wizard form. It performs necessary sanitization of user inputs, updates the options, triggers an action hook for extensibility, and redirects to the next step. The code looks secure and follows best practices.
194-199: The new "Commission" step looks good!The addition of the "Commission" step to the setup wizard is implemented correctly. It follows the existing pattern of defining the step's name, view, and handler functions. This change enhances the setup wizard by including the commission setup functionality.
| fixedCOmmissionhandler(data) { | ||
| if (data.fixed === '') { | ||
| data.fixed = this.fixedCommission.fixed ?? 0; | ||
| } | ||
| if (data.percentage === '') { | ||
| data.percentage = this.fixedCommission.percentage ?? 0; | ||
| } | ||
|
|
||
| this.fixedCommission = data; | ||
| }, |
There was a problem hiding this comment.
Improve the handling of empty input values.
The fixedCOmmissionhandler method currently falls back to the previous values or 0 when the input values are empty. However, this behavior may not be intuitive for users.
Consider the following suggestions to improve the user experience:
- Display a validation message to the user when the input values are empty, prompting them to enter valid values.
- Use a more meaningful default value instead of 0, such as the minimum allowed commission value.
- Disable the form submission until valid values are entered.
Additionally, ensure that the handling of empty input values is consistent across the component and aligns with the overall user experience and business requirements.
| <div class='relative'> | ||
| <div class='d-xs:hidden md:flex bg-gray-100 min-h-[3rem] text-gray-500 border-[0.957434px] border-b-0 items-center'> | ||
| <div class='w-1/2 pl-3 flex h-[3rem] items-center border-r-[0.957434px]'> | ||
| <p class='text-xs'>{{ __( 'Category', 'dokan-lite' ) }}</p> |
There was a problem hiding this comment.
Use the correct text domain for localization.
The __() function call should use the correct text domain for localization. Replace 'dokan-lite' with the appropriate text domain for your plugin.
|
|
||
| <div class='flex w-1/2'> | ||
| <div class='w-1/2 mr-20'> | ||
| <p class='text-xs text-center'>{{ __( 'Percentage', 'dokan-lite' ) }}</p> |
There was a problem hiding this comment.
Use the correct text domain for localization.
The __() function calls should use the correct text domain for localization. Replace 'dokan-lite' with the appropriate text domain for your plugin.
Also applies to: 13-13
| <button type='button' class='p-1 d-xs:pl-1 md:pl-4 bg-transparent bg-co border-none cursor-pointer' @click='()=>allCategroyEnabled = !allCategroyEnabled'> | ||
| <i class="far" :class='! allCategroyEnabled ? "fa-minus-square text-black" : "fa-plus-square text-[#F05025]"'></i> | ||
| </button> | ||
| <p class='d-xs:text-[8px] sm:text-[14px] !m-0'>{{__( 'All Categories', 'dokan-lite' )}}</p> |
There was a problem hiding this comment.
Use the correct text domain for localization.
The __() function calls should use the correct text domain for localization. Replace 'dokan-lite' with the appropriate text domain for your plugin.
Also applies to: 39-39, 42-42, 45-45, 83-83, 86-86, 89-89
| getCurrencySymbol() { | ||
| return window.dokan.currency.symbol ? window.dokan.currency.symbol : ''; | ||
| }, |
There was a problem hiding this comment.
Avoid relying on global variables.
The component relies on the global dokan object to access the currency symbol. Consider passing the currency symbol as a prop to the component to make it more reusable and testable.
| dokan.api.get('/products/multistep-categories').then( data => { | ||
| if ( 'object' === typeof data) { | ||
| this.categories = data | ||
|
|
||
| this.renderCategories = Object.values( this.getCatgroies() ); | ||
|
|
||
| if (this.commission.items && Object.values( this.commission.items ).length) { | ||
| this.allCategroyEnabled = false; | ||
| } | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Handle API errors and loading state.
The component fetches category data from an API endpoint in the created hook. However, it doesn't handle potential API errors or loading state. Consider adding error handling and displaying a loading indicator while the data is being fetched.
| catRowClick( item, index, ) { | ||
| if ( this.openRows.includes( Number( item.term_id ) ) ) { | ||
| // Remove the parent. | ||
| let index = this.openRows.indexOf(Number( item.term_id )); | ||
| this.openRows.splice(index, 1); | ||
|
|
||
| this.getChildren( item.term_id ).forEach( child => { | ||
| let index = this.openRows.indexOf(Number( child )); | ||
| -1 !== index ? this.openRows.splice(index, 1) : ""; | ||
| } ); | ||
| } else { | ||
| this.openRows.push( Number( item.term_id ) ); | ||
| } |
There was a problem hiding this comment.
Avoid directly modifying arrays in methods.
The catRowClick method directly modifies the openRows array. It's generally recommended to avoid directly modifying arrays in methods and instead use Vue's reactivity system. Consider using this.$set or splice to update the array.
| handleAllCategoryInput( value, commission_type, oldValue = '' ) { | ||
| if (isNaN( value )) { | ||
| value = oldValue | ||
| } | ||
| this.$set( this.commission.all, commission_type, value ); | ||
| this.$set(this.commission, 'items', {}); | ||
|
|
||
| this.emitComponentChange( JSON.parse( JSON.stringify( this.commission ) ) ) | ||
| }, |
There was a problem hiding this comment.
Avoid directly modifying objects in methods.
The handleAllCategoryInput method directly modifies the commission object. It's generally recommended to avoid directly modifying objects in methods and instead use Vue's reactivity system. Consider using this.$set to update the object properties.
There was a problem hiding this comment.
Actionable comments posted: 28
🧹 Outside diff range and nitpick comments (19)
tests/pw/utils/pwMatchers.ts (2)
57-85: Approve new matcher with suggestions for improvement.The new
toBeApproximatelymatcher is a valuable addition for comparing floating-point numbers within a specified tolerance. However, consider the following improvements:
- The function is quite long. Consider splitting it into smaller, more focused functions for better maintainability.
- The use of
console.logfor warnings might not be ideal in all testing environments. Consider using a more flexible logging mechanism.- The default tolerance of 0 might be too strict for floating-point comparisons. Consider setting a small non-zero default, e.g., 1e-10.
Here's a suggested refactoring to address these points:
const formatMessage = (color: string, message: string) => `${color}${message}\x1b[0m`; const logWarning = (expected: number, received: number, log?: any) => { if (Array.isArray(log) && log.length > 0 && typeof log[0] === 'object') { console.table(log); } console.log(formatMessage('\x1b[33m', `Test failed deep equality check: expected ${expected} and received ${received}`)); }; export const customMatchers = { // ... other matchers ... toBeApproximately(received: number, expected: number, tolerance: number = 1e-10, log?: any) { const diff = helpers.roundToTwo(Math.abs(received - expected)); const pass = diff <= tolerance; if (pass && diff > 0) { logWarning(expected, received, log); } return { message: () => pass ? 'passed' : formatMessage('\x1b[91m', `Difference between expected ${expected} and received ${received} value is more than the tolerance of ${tolerance}`), pass, }; }, // ... other matchers ... };This refactoring improves readability, separates concerns, and sets a more appropriate default tolerance for floating-point comparisons.
Line range hint
1-92: Summary: New matcher enhances testing capabilities for commission calculations.The introduction of the
toBeApproximatelymatcher aligns well with the PR objectives, particularly in enhancing the ability to test commission-related calculations where precise floating-point comparisons are necessary. This addition will be valuable for ensuring accurate commission management and display, as outlined in the PR summary.While the implementation is generally sound, consider the suggested refactoring to improve code maintainability and testing flexibility. These improvements will contribute to the overall robustness of the testing framework, supporting the broader goals of enhancing commission structure management in the Dokan plugin.
tests/pw/tests/e2e/euCompliance.spec.ts (2)
14-15: LGTM! Consider adding a fallback value for VENDOR_ID.The addition of the
VENDOR_IDconstant from the environment variables is a good practice. It allows for flexible testing across different vendor IDs without hardcoding values.Consider adding a fallback value to ensure the tests don't fail if the environment variable is not set:
const { VENDOR_ID = 'default_vendor_id' } = process.env;This will use 'default_vendor_id' if
VENDOR_IDis not set in the environment.
Line range hint
1-165: Address skipped tests and improve test coverage.The test suite provides comprehensive coverage for EU Compliance features across admin, vendor, and customer roles. However, there are several skipped tests related to product EU compliance data.
Review and update the skipped tests:
- Lines 128-138: Vendor product EU compliance data tests
- Line 163: Customer product EU compliance data view test
Consider adding tests for:
- Commission calculations with EU compliance data
- Interaction between EU compliance settings and the new commission API (
dokan/v1/commission)- Validation of EU compliance data input
Ensure test coverage for the new features mentioned in the PR, such as:
- Display of commission amounts in product lists
- Earning suggestions for vendors
- Backward compatibility for commission types
Would you like assistance in implementing these additional tests or updating the skipped ones?
tests/pw/utils/dbUtils.ts (1)
208-214: NewgetChildOrderIdsmethod enhances order managementThe addition of the
getChildOrderIdsmethod is a valuable enhancement that aligns well with the PR objectives, particularly in addressing issue #2676 for improved order details handling. The implementation is concise and follows good practices, using parameterized queries and efficient result mapping.Consider adding error handling to manage cases where the query fails or returns no results, improving the method's robustness.
Here's a suggested improvement for error handling:
async getChildOrderIds(orderId: string): Promise<string[]> { try { const query = `SELECT id FROM ${dbPrefix}_wc_orders WHERE parent_order_id = ?;`; const res = await dbUtils.dbQuery(query, [orderId]); if (res.length === 0) { console.log(`No child orders found for parent order ID: ${orderId}`); return []; } return res.map((row: { id: number }) => row.id.toString()); } catch (error) { console.error(`Error fetching child order IDs for parent order ${orderId}:`, error); throw error; } }This modification adds error handling and logging, which can be helpful for debugging and maintaining the application.
tests/pw/tests/api/_env.setup.ts (3)
127-148: Consider uncommenting or removing unused shipping methods.The implementation for setting up shipping methods is thorough, including the deletion of previous zones and creation of a new one. The conditional logic for DOKAN_PRO shipping methods is a good practice for maintaining flexibility.
However, there are two commented-out shipping methods:
// await apiUtils.addShippingZoneMethod(zoneId, payloads.addShippingMethodFreeShipping); // await apiUtils.addShippingZoneMethod(zoneId, payloads.addShippingMethodLocalPickup);Consider either uncommenting these if they should be part of the setup, or removing them if they're no longer needed. This will improve code clarity and prevent potential confusion in the future.
161-175: LGTM: Categories and attributes setup is well-implemented.The implementation for setting up categories and attributes is thorough and well-structured. It ensures a clean slate by deleting previous entries before creating new ones, which is a good practice.
One minor suggestion for improvement:
Consider adding error handling for the API calls, especially when creating the category and attribute. This would make the test more robust and provide clearer feedback if something goes wrong during the setup process. For example:
try { const [, categoryId] = await apiUtils.createCategory(payloads.createCategory); helpers.createEnvVar('CATEGORY_ID', categoryId); } catch (error) { console.error('Failed to create category:', error); throw error; }This approach would provide more detailed information if the setup fails, making it easier to debug issues in the future.
123-175: Overall, the new test setups are well-implemented with room for minor improvements.The additions to the "setup user settings" section in
tests/pw/tests/api/_env.setup.tsare generally well-structured and cover important aspects of the Dokan plugin setup. Here's a summary of the review:
- The tax rate setup is concise and well-implemented.
- The shipping methods setup is thorough, but there are some commented-out methods that should be addressed.
- The basic payments setup is good, but could benefit from a more flexible, configuration-driven approach.
- The categories and attributes setup is well-structured, but could be improved with additional error handling.
To enhance the overall quality and maintainability of the test setups, consider:
- Addressing commented-out code by either implementing or removing it.
- Implementing a more flexible, configuration-driven approach for setting up payment gateways.
- Adding error handling to critical setup operations.
- Ensuring consistent use of environment variables like
DOKAN_PROacross all relevant setups.These improvements will make the test setups more robust, flexible, and easier to maintain as the plugin evolves.
tests/pw/pages/basePage.ts (1)
331-335: LGTM! Consider adding a comment for consistency.The new
selectAndWaitForResponsemethod is a valuable addition to theBasePageclass. It effectively combines selecting an option and waiting for a response, which is useful for handling asynchronous operations triggered by select changes.For consistency with other methods in the class, consider adding a brief comment describing the method's purpose, like this:
// select option and wait for response async selectAndWaitForResponse(subUrl: string, selector: string, value: string, code = 200): Promise<Response> { // ... (existing implementation) }tests/pw/pages/productsPage.ts (6)
Line range hint
23-31: Consider using a slug generator for the category slug.While the current implementation works, using the same value for both the category name and slug might not always produce the desired result, especially for multi-word category names or names with special characters.
Consider implementing a slug generator function to create a URL-friendly version of the category name. For example:
import { slugify } from '@utils/helpers'; // Inside the method const categorySlug = slugify(categoryName); await this.fill(productsAdmin.category.slug, categorySlug);This approach ensures that the slug is always URL-friendly, regardless of the category name input.
Line range hint
33-50: Consider adding error handling for attribute and term addition.The method handles the happy path well, but it doesn't account for potential errors that might occur during the attribute or term addition process.
Consider wrapping the attribute and term addition logic in try-catch blocks to handle potential errors gracefully. For example:
async addAttribute(attribute: product['attribute']) { try { await this.goIfNotThere(data.subUrls.backend.wc.addNewAttributes); // ... existing attribute addition logic ... // Add new terms for (const attributeTerm of attribute.attributeTerms) { try { await this.fill(productsAdmin.attribute.attributeTerm, attributeTerm); // ... existing term addition logic ... } catch (error) { console.error(`Failed to add term ${attributeTerm}:`, error); // Optionally, you could throw the error here if you want to stop the process } } } catch (error) { console.error('Failed to add attribute:', error); throw error; // Re-throw the error if you want calling code to handle it } }This approach allows for more robust error handling and logging, which can be helpful for debugging and maintaining the test suite.
Line range hint
52-85: EnhanceaddSimpleProductmethod for better robustness and flexibility.While the method works well for adding a simple product, there are a few improvements that could make it more robust and flexible:
- Error handling: Consider adding try-catch blocks to handle potential errors during the product addition process.
- Flexibility: The method could be more flexible by allowing optional parameters for additional product details.
- Validation: Add more comprehensive validation checks after saving the product.
Here's a suggested refactor:
async addSimpleProduct(product: product['simple'], additionalDetails?: Partial<product['simple']>): Promise<void> { try { await this.goIfNotThere(data.subUrls.backend.wc.addNewProducts); // Product basic info await this.type(productsAdmin.product.productName, product.productName()); await this.selectByValue(productsAdmin.product.productType, product.productType); await this.click(productsAdmin.product.subMenus.general); await this.type(productsAdmin.product.regularPrice, product.regularPrice()); await this.click(productsAdmin.product.category(product.category)); // Handle additional details if provided if (additionalDetails) { if (additionalDetails.stockStatus) { await this.click(productsAdmin.product.subMenus.inventory); await this.selectByValue(productsAdmin.product.stockStatus, additionalDetails.stockStatus); } // Add more optional fields here... } // Vendor Store Name await this.select2ByText(productsAdmin.product.storeName, productsAdmin.product.storeNameInput, product.storeName); await this.scrollToTop(); // Publish or save as draft await this.handleProductStatus(product.status); // Validate product creation await this.validateProductCreation(product); } catch (error) { console.error('Failed to add simple product:', error); throw error; } } private async handleProductStatus(status: string): Promise<void> { switch (status) { case 'publish': await this.clickAndWaitForResponseAndLoadState(data.subUrls.ajax, productsAdmin.product.publish); await this.toContainText(productsAdmin.product.updatedSuccessMessage, data.product.publishSuccessMessage); break; case 'draft': await this.clickAndWaitForResponseAndLoadState(data.subUrls.post, productsAdmin.product.saveDraft, 302); await this.toContainText(productsAdmin.product.updatedSuccessMessage, data.product.draftUpdateSuccessMessage); break; case 'pending': await this.click(productsAdmin.product.editStatus); await this.selectByValue(productsAdmin.product.status, data.product.status.pending); await this.clickAndWaitForResponseAndLoadState(data.subUrls.post, productsAdmin.product.saveDraft, 302); await this.toContainText(productsAdmin.product.updatedSuccessMessage, data.product.pendingProductUpdateSuccessMessage); break; default: throw new Error(`Unsupported product status: ${status}`); } } private async validateProductCreation(product: product['simple']): Promise<void> { await this.toHaveValue(productsAdmin.product.productName, product.productName()); await this.toHaveValue(productsAdmin.product.regularPrice, product.regularPrice()); // Add more validation checks as needed }This refactored version provides better error handling, allows for additional product details, and separates concerns into smaller, more manageable methods.
Line range hint
87-136: RefactoraddVariableProductfor improved structure and error handling.The
addVariableProductmethod is comprehensive but could benefit from improved structure and error handling. Here are some suggestions:
- Break down the method into smaller, more focused helper methods.
- Add error handling with try-catch blocks.
- Improve type safety by using more specific types for the
productparameter.Here's a suggested refactor:
async addVariableProduct(product: product['variable']): Promise<void> { try { await this.goIfNotThere(data.subUrls.backend.wc.addNewProducts); await this.addBasicProductInfo(product); await this.addProductAttributes(product.attribute); await this.addProductVariations(product.variations); await this.setProductCategory(product.category); await this.setVendorStore(product.storeName); await this.publishProduct(); await this.validateVariableProduct(product); } catch (error) { console.error('Failed to add variable product:', error); throw error; } } private async addBasicProductInfo(product: product['variable']): Promise<void> { await this.type(productsAdmin.product.productName, product.productName()); await this.selectByValue(productsAdmin.product.productType, product.productType); } private async addProductAttributes(attribute: string): Promise<void> { await this.click(productsAdmin.product.subMenus.attributes); await this.selectAndAddAttribute(attribute); await this.clickAndWaitForResponse(data.subUrls.backend.wc.taxonomyTerms, productsAdmin.product.selectAll); await this.check(productsAdmin.product.usedForVariations); await this.clickAndWaitForResponse(data.subUrls.ajax, productsAdmin.product.saveAttributes); } private async selectAndAddAttribute(attribute: string): Promise<void> { if (await this.isVisibleLocator(productsAdmin.product.customProductAttribute)) { await this.selectByValue(productsAdmin.product.customProductAttribute, `pa_${attribute}`); await this.click(productsAdmin.product.addAttribute); } else { await this.clickAndWaitForResponse(data.subUrls.backend.wc.searchAttribute, productsAdmin.product.addExistingAttribute); await this.typeAndWaitForResponse(data.subUrls.backend.wc.term, productsAdmin.product.addExistingAttributeInput, attribute); await this.pressAndWaitForResponse(data.subUrls.ajax, data.key.enter); } } private async addProductVariations(variations: product['variable']['variations']): Promise<void> { await this.click(productsAdmin.product.subMenus.variations); await this.clickAndAcceptAndWaitForResponse(data.subUrls.ajax, productsAdmin.product.subMenus.generateVariations); await this.fillAlert('100'); await this.selectByValue(productsAdmin.product.addVariations, variations.variableRegularPrice); } private async setProductCategory(category: string): Promise<void> { await this.click(productsAdmin.product.category(category)); } private async setVendorStore(storeName: string): Promise<void> { await this.select2ByText(productsAdmin.product.storeName, productsAdmin.product.storeNameInput, storeName); await this.scrollToTop(); } private async publishProduct(): Promise<void> { await this.clickAndWaitForResponseAndLoadState(data.subUrls.post, productsAdmin.product.publish, 302); await this.toContainText(productsAdmin.product.updatedSuccessMessage, data.product.publishSuccessMessage); } private async validateVariableProduct(product: product['variable']): Promise<void> { await this.toHaveValue(productsAdmin.product.productName, product.productName()); await this.toHaveSelectedValue(productsAdmin.product.productType, product.productType); // Add more specific validations for variable products }This refactored version breaks down the complex method into smaller, more manageable pieces. It also adds error handling and improves type safety. The helper methods make the code more readable and easier to maintain.
Line range hint
138-1022: Consider refactoring for improved code reuse, error handling, and consistency.The
ProductsPageclass contains a wealth of functionality for managing products. However, there are several areas where the code could be improved:
Code Reuse: Many methods share similar patterns. Consider extracting common functionality into shared helper methods.
Error Handling: Implement consistent error handling across all methods. This could involve creating a custom error handling method or using a try-catch wrapper.
Type Safety: Enhance type safety by using more specific TypeScript types and interfaces for method parameters and return values.
Method Length: Some methods are quite long. Consider breaking them down into smaller, more focused methods.
Consistency: Ensure consistent naming conventions and coding styles across all methods.
Documentation: Add JSDoc comments to methods to improve code readability and maintainability.
Testing: Consider adding unit tests for individual methods to ensure reliability.
Here's an example of how you might refactor a portion of the class to address these points:
/** * Represents a page for managing products. */ export class ProductsPage extends AdminPage { constructor(page: Page) { super(page); } /** * Adds a product of the specified type. * @param product - The product to add * @param type - The type of product */ async addProduct(product: Product, type: ProductType): Promise<void> { try { await this.goIfNotThere(data.subUrls.backend.wc.addNewProducts); await this.addBasicProductInfo(product, type); switch (type) { case 'simple': await this.addSimpleProductDetails(product as SimpleProduct); break; case 'variable': await this.addVariableProductDetails(product as VariableProduct); break; // Add cases for other product types } await this.setProductCategory(product.category); await this.setVendorStore(product.storeName); await this.publishProduct(); await this.validateProduct(product, type); } catch (error) { this.handleError('Failed to add product', error); } } private async addBasicProductInfo(product: Product, type: ProductType): Promise<void> { await this.type(productsAdmin.product.productName, product.productName()); await this.selectByValue(productsAdmin.product.productType, type); } private async addSimpleProductDetails(product: SimpleProduct): Promise<void> { await this.click(productsAdmin.product.subMenus.general); await this.type(productsAdmin.product.regularPrice, product.regularPrice()); // Add other simple product specific details } private async addVariableProductDetails(product: VariableProduct): Promise<void> { await this.addProductAttributes(product.attributes); await this.addProductVariations(product.variations); // Add other variable product specific details } // ... Other helper methods ... private handleError(message: string, error: any): void { console.error(message, error); // You could also implement custom error handling logic here throw error; } }This refactored structure promotes code reuse, improves error handling, and enhances overall code organization. Apply similar principles throughout the class to improve its maintainability and readability.
Line range hint
1-1022: Overall assessment: Comprehensive but could benefit from refactoring.The
ProductsPageclass provides a robust and extensive set of methods for managing products in an e-commerce system. It covers a wide range of product types and operations, which is commendable. However, there are several areas where the code could be improved:
- Code structure: Consider breaking down larger methods into smaller, more focused ones to improve readability and maintainability.
- Error handling: Implement consistent error handling across all methods.
- Code reuse: Look for opportunities to extract common functionality into shared helper methods.
- Type safety: Enhance the use of TypeScript types for improved code reliability.
- Documentation: Add JSDoc comments to methods for better code understanding.
- Testing: Consider implementing unit tests for individual methods.
These improvements would enhance the overall quality of the code, making it more maintainable, reliable, and easier to extend in the future. Despite these suggestions, the current implementation is functional and provides a solid foundation for product management functionality.
tests/pw/utils/apiUtils.ts (1)
Line range hint
899-922: LGTM! Consider enhancing error handling.The new
createCustomermethod is well-implemented and consistent with other methods in the class. It properly handles the case where a customer already exists by updating the customer instead.Consider enhancing the error handling to be more specific:
if (responseBody.code) { - expect(response.status()).toBe(400); + if (responseBody.code === 'registration-error-email-exists') { + console.log('Customer with this email already exists. Updating instead.'); + } else { + throw new Error(`Unexpected error: ${responseBody.code}`); + } // get customer id if already exists customerId = await this.getCustomerId(payload.username, auth);This change would provide more clarity on the specific error encountered and handle unexpected errors more robustly.
tests/pw/utils/helpers.ts (1)
280-280: Correct typo in commentThere's a typo in the comment at line 280:
- // only for debuging + // Only for debuggingtests/pw/utils/payloads.ts (1)
630-641: Consider removing or uncommenting the per-product commission settingsThe per-product commission settings in the
createDiscountProductpayload are currently commented out. If these settings are intended to be part of the payload, please uncomment them to ensure they are applied. If they are no longer needed, consider removing them to keep the code clean.tests/pw/pages/selectors.ts (1)
2972-2988: Address the TODO comment and approve submenu selectors.The TODO comment suggests grouping locators for better organization. The submenu selectors are correctly defined.
Would you like me to help organize these locators or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- dokan.php (2 hunks)
- includes/Vendor/Manager.php (1 hunks)
- includes/functions.php (9 hunks)
- tests/pw/feature-map/feature-map.yml (1 hunks)
- tests/pw/pages/basePage.ts (1 hunks)
- tests/pw/pages/productsPage.ts (8 hunks)
- tests/pw/pages/selectors.ts (5 hunks)
- tests/pw/pages/settingsPage.ts (2 hunks)
- tests/pw/pages/vendorBookingPage.ts (1 hunks)
- tests/pw/tests/api/_env.setup.ts (1 hunks)
- tests/pw/tests/e2e/euCompliance.spec.ts (2 hunks)
- tests/pw/utils/apiUtils.ts (1 hunks)
- tests/pw/utils/dbData.ts (5 hunks)
- tests/pw/utils/dbUtils.ts (3 hunks)
- tests/pw/utils/helpers.ts (1 hunks)
- tests/pw/utils/interfaces.ts (5 hunks)
- tests/pw/utils/payloads.ts (6 hunks)
- tests/pw/utils/pwMatchers.ts (2 hunks)
- tests/pw/utils/testData.ts (6 hunks)
🧰 Additional context used
🔇 Additional comments (37)
tests/pw/utils/pwMatchers.ts (2)
2-2: LGTM: Import statement for helpers is correct and necessary.The import statement for
helpersis correctly added and is required for the newtoBeApproximatelymatcher function.
92-92: LGTM: customExpect object correctly updated.The
customExpectobject is correctly updated to include the newtoBeApproximatelymatcher, making it available for use in tests.tests/pw/tests/e2e/euCompliance.spec.ts (1)
101-101: LGTM! Verify VENDOR_ID usage across the test suite.The update to use
VENDOR_IDin theadmin.editVendormethod call is a good improvement. It makes the test more flexible and easier to maintain.To ensure consistency, let's verify that
VENDOR_IDis used appropriately throughout the test suite:This script will help identify any remaining hardcoded vendor IDs and confirm that
VENDOR_IDis used consistently across the test suite.tests/pw/utils/dbUtils.ts (3)
146-146: Improved query parameter handling increateRefundRequestThe modification to use a single array for query parameters is a positive change. It enhances code readability and maintainability while adhering to best practices for parameterized queries. This approach helps prevent SQL injection and aligns with the PR's objectives of improving code quality.
Line range hint
1-215: Summary of changes in dbUtils.tsThe modifications in this file align well with the PR objectives of enhancing the commission structure and improving order management. The changes include:
- Improved query parameter handling in
createRefundRequest.- Addition of the
getChildOrderIdsmethod for better suborder management.- Removal of the
getSellingInfomethod as part of the commission restructure.These changes contribute to the overall goal of improving the Dokan plugin's functionality and user experience. The code quality is good, with some suggestions for further improvement in error handling. Ensure that the removal of
getSellingInfodoesn't negatively impact existing functionality.
Line range hint
1-215: Verify impact of removinggetSellingInfomethodThe
getSellingInfomethod has been removed as part of the commission restructure. While this aligns with the PR objectives, it's crucial to ensure that this removal doesn't negatively impact existing functionality.Please run the following script to check for any remaining references to the
getSellingInfomethod:If any references are found, please ensure that they have been properly updated or removed to maintain consistency with the new commission structure.
tests/pw/tests/api/_env.setup.ts (1)
123-125: LGTM: Tax rate setup looks good.The implementation for setting up the tax rate is concise and uses the appropriate method from
apiUtils. The use of predefined payloads (payloads.enableTaxandpayloads.createTaxRate) promotes consistency and maintainability.tests/pw/pages/vendorBookingPage.ts (3)
Line range hint
1-385: Summary: UI selector updates maintain test compatibility.The changes in this file are limited to two selector updates within the
adminAddBookingProductmethod. These updates reflect a reorganization of the admin UI for booking products, specifically the introduction of asubMenusstructure. The changes are consistent with the PR objectives of redesigning the commission user interface.While the updates are minimal, they are crucial for maintaining the compatibility of these tests with the new UI structure. The rest of the
BookingPageclass remains unchanged, preserving the existing test coverage for various booking-related functionalities.To ensure comprehensive updates across the codebase:
- Verify that all relevant selectors in other files have been updated to reflect the new UI structure.
- Consider adding a test case that specifically validates the new UI structure for booking products in the admin interface.
- Update any relevant documentation or comments that might reference the old selector structure.
Please run the following script to identify any other files that might need similar updates:
#!/bin/bash # Search for files containing bookingProductsAdmin selectors rg "bookingProductsAdmin\.product" --type typescript -lReview the output to ensure all relevant files have been updated to reflect the new UI structure.
35-35: LGTM. Ensure UI structure consistency.The selector update from
bookingProductsAdmin.product.bookingCoststobookingProductsAdmin.product.subMenus.bookingCostsis consistent with the previous change and aligns with the PR objectives of redesigning the commission user interface.To ensure all relevant selectors have been updated, please run the following script:
#!/bin/bash # Search for other potential selectors that might need updating rg "bookingProductsAdmin\.product\.[a-zA-Z]+" --type typescriptThis will help identify any other selectors that might need to be updated to the new
subMenusstructure.
29-29: LGTM. Verify UI structure change.The selector update from
bookingProductsAdmin.product.generaltobookingProductsAdmin.product.subMenus.generalaligns with the PR objectives of redesigning the commission user interface. This change likely reflects a reorganization of the admin UI for booking products.To ensure consistency across the codebase, please run the following script to check for any other occurrences of the old selector that might need updating:
✅ Verification successful
Selector Update Verified
The update from
bookingProductsAdmin.product.generaltobookingProductsAdmin.product.subMenus.generalhas been applied consistently across the codebase. No other occurrences of the old selector were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of the old selector rg "bookingProductsAdmin\.product\.general" --type typescriptLength of output: 100
Script:
#!/bin/bash # Search for other occurrences of the old selector in TypeScript files rg "bookingProductsAdmin\.product\.general" --glob "*.ts" --glob "*.tsx"Length of output: 74
dokan.php (3)
53-54: LGTM: Class property documentation updatedThe class property documentation has been updated to include the new
Feesclass and improve the formatting of theCommissionclass property. This change aligns with the PR objectives of enhancing the commission structure.
411-411: LGTM: New Fees class instance addedA new instance of
WeDevs\Dokan\Feeshas been added to the container. This addition is consistent with the PR objectives of enhancing the commission structure and management.
Line range hint
1-624: Overall assessment: Changes look goodThe modifications to
dokan.phpare minimal and focused on introducing the new Fees functionality. These changes align well with the PR objectives of enhancing the commission structure. The newFeesclass is properly integrated into the existing plugin architecture without introducing any apparent issues or conflicts.tests/pw/feature-map/feature-map.yml (3)
331-347: LGTM: Comprehensive commission management features addedThe new 'Commission' page features provide a robust set of capabilities for administrators to manage commissions across various contexts, including:
- Setup wizard settings
- General Dokan selling settings
- Vendor-specific settings
- Product-specific settings
- Dokan subscription product settings
This enhancement aligns well with the PR objectives of improving commission management and transparency.
331-347: Verify: Removal of vendor filtering by approval statusThe ability for admins to filter vendors by pending and approved status has been removed. While this might simplify the vendor management interface, it could potentially make it more difficult for admins to quickly identify vendors in specific approval states.
Could you confirm if this removal is intentional and aligns with the overall goals of the PR? If so, are there alternative ways for admins to efficiently manage vendors in different approval states?
Line range hint
1-347: Summary of changes in feature mapThe updates to the feature map align well with the PR objectives, particularly:
- The addition of a comprehensive 'Commission' page with various settings for admins.
- Retention of withdrawal management features for admins.
However, there's one point that needs clarification:
- The removal of vendor filtering by approval status on the 'Stores' page.
Overall, these changes seem to enhance the commission management capabilities of the Dokan plugin, which is the main focus of this PR.
tests/pw/pages/basePage.ts (2)
331-335: Excellent addition enhancing BasePage capabilities.This new method
selectAndWaitForResponseis a valuable addition to theBasePageclass. It enhances the class's capabilities for handling select elements that trigger network requests, which is a common scenario in modern web applications.Some potential use cases for this method include:
- Testing dropdown menus that load data dynamically (e.g., cascading selects).
- Verifying that selecting an option triggers the correct API call.
- Ensuring that the response to a select change meets expected criteria (via the
codeparameter).The method's implementation is consistent with the class's existing patterns, making it easy for developers familiar with the class to adopt and use.
Line range hint
1-1190: Well-integrated addition to BasePage class.The addition of the
selectAndWaitForResponsemethod to theBasePageclass is a well-integrated and valuable enhancement. This new method complements existing functionality, particularly methods likeselectByValueAndWaitForResponse, by providing a more generic way to handle select elements that trigger network requests.The change is localized and doesn't affect the rest of the file, maintaining the overall structure and integrity of the
BasePageclass. This addition improves the class's capability to handle asynchronous operations triggered by select changes, which is likely to be useful in various testing scenarios.Overall, this is a positive change that enhances the utility of the
BasePageclass without introducing any breaking changes or inconsistencies.tests/pw/pages/productsPage.ts (1)
Line range hint
1-21: LGTM: Imports and class definition look good.The imports and class definition are well-structured. The class extends
AdminPage, which suggests good use of inheritance for shared functionality.tests/pw/utils/apiUtils.ts (2)
Line range hint
924-933: LGTM! Efficient implementation for bulk deletion.The updated
deleteAllCustomersmethod is well-implemented. It efficiently handles bulk deletion of customers using a single batch operation, and includes helpful logging when no customers exist.
Line range hint
1-933: Overall, excellent additions and changes to the ApiUtils class.The new
createCustomermethod and the updates todeleteAllCustomersare well-implemented and integrate seamlessly with the existing codebase. They follow the established patterns and coding style of theApiUtilsclass, maintaining consistency and readability.These changes enhance the functionality of the class while preserving its structure and design principles. Good job on maintaining code quality and consistency in this large and complex utility class.
tests/pw/utils/interfaces.ts (4)
415-416: Addition of 'commission' field to 'productInfo' is appropriateIncluding the
commissionfield in theproductInfointerface allows for product-specific commission settings, enhancing the flexibility of commission configurations.
653-653: Inclusion of 'commission' field in 'dokanSetupWizard' interface aligns with commission restructuringThis update consolidates commission-related fields into a single
commissionobject, improving data organization.
758-759: Adding 'commission' field to 'vendorInfo' interface enhances data encapsulationIncluding the
commissionfield allows for vendor-specific commission settings, which is beneficial for managing different commission rates per vendor.
1536-1536: Updating 'commission' in 'dokanSettings.selling' improves clarityBy consolidating commission settings into the
commissionobject within thedokanSettings.sellinginterface, the configuration becomes more structured and easier to maintain.tests/pw/utils/dbData.ts (4)
1-1: LGTM!The addition of
CATEGORY_IDto the destructuredprocess.envconstants enhances the configurability of the application.
71-91: Commission settings updated correctlyThe updates to the commission configuration, including setting
commission_typeto'fixed', addingadditional_fee, and definingcommission_category_based_values, are implemented correctly. The use ofCATEGORY_IDincommission_category_based_valuesallows for category-specific commission rates.
1336-1336: Widget content updated appropriatelyThe
contentofwidgetBlock['4']has been updated to display recent comments without avatars, dates, or excerpts. The HTML structure and JSON attributes are correctly formatted.
1378-1378:setup_wizard_message_without_htmlupdated appropriatelyThe
setup_wizard_message_without_htmlnow reflects the updated message without HTML tags, maintaining consistency with the HTML version.tests/pw/utils/testData.ts (5)
563-571: Commission settings added toproductInfoare correctly structuredThe commission configuration within
productInfois well-defined and aligns with the expected data structure.
899-899: Confirm ifcommissionFixedvalue of'0'is intentionalIn the
commissionsettings fordokanSetupWizard,commissionFixedis set to'0'. Please confirm if setting the fixed commission to zero is intended.
1277-1285: Commission settings added tovendorInfoare appropriately configuredThe commission data within
vendorInfois properly structured and consistent with other configurations.
2113-2121: Commission settings indokanSettings.sellingare correctly implementedThe addition of commission settings under
sellingindokanSettingsis accurate and follows the expected format.
883-883: Verify the definition ofCATEGORY_IDThe
categoryfield is set toCATEGORY_ID, which suggests it should be a predefined constant. Please ensure thatCATEGORY_IDis defined elsewhere in the test data.You can run the following script to check if
CATEGORY_IDis defined:includes/functions.php (1)
994-995: Ensure proper placement of new columns in the admin product listYou're adding
'admin_commission'and'author'columns. Verify that this placement doesn't conflict with existing columns and that the columns appear in the desired order.Run the following script to check the columns:
tests/pw/utils/payloads.ts (2)
4605-4649:⚠️ Potential issueReview commission types in product commission settings
In the
productCommissionOnlyPercent,productCommissionOnlyFixed, andproductCommissionBothPercentageAndFixedarrays, the_per_product_admin_commission_typeis set to'fixed'in all cases. Please ensure that thevaluefield for_per_product_admin_commission_typeaccurately reflects the intended commission type:
- For percentage-only commissions, set
value: 'percentage'.- For fixed-only commissions, keep
value: 'fixed'.- For both percentage and fixed commissions, verify if the commission system supports combining both types, or consider defining them separately.
This will prevent any misconfiguration in the product commission settings.
Run the following script to check the assignments of
_per_product_admin_commission_type:#!/bin/bash # Description: Check the commission types assigned to `_per_product_admin_commission_type`. # Expected: Correct commission types corresponding to the intended commission structures. rg --type typescript '_per_product_admin_commission_type' --json | jq '.data.lines[].text' -r
4587-4603: Verify default values invendorwiseCommissionThe
vendorwiseCommissionobject hasadmin_commission_typeset to'fixed'withadmin_commissionandadmin_additional_feeboth set to'5'. Please confirm that these default values align with the intended commission structure. Additionally, ensure that theadmin_category_commissionvalues match the expected rates for both theallanditemsfields.Run the following script to verify the usage of
vendorwiseCommissionand check for consistency:✅ Verification successful
Default Values in
vendorwiseCommissionAre Verified and ConsistentThe default values for
vendorwiseCommissionalign with the intended commission structure:
admin_commission_type:'fixed'admin_commission:'5'admin_additional_fee:'5'admin_category_commission:
all:{ flat: '7', percentage: '7' }items:{ '27': { flat: '7', percentage: '7' } }All usages of
vendorwiseCommissionin the codebase, primarily within test files, consistently build upon these defaults to test various scenarios. No discrepancies or unintended overrides were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of commission values in `vendorwiseCommission` across the codebase. # Search for `vendorwiseCommission` assignments rg --type typescript 'vendorwiseCommission' --json | jq '.data.lines[].text' -rLength of output: 128
Script:
#!/bin/bash # Description: Verify the consistency of commission values in `vendorwiseCommission` across TypeScript files. # Search for `vendorwiseCommission` assignments in .ts and .tsx files rg 'vendorwiseCommission' --glob '*.ts' --glob '*.tsx' --json | jq '.data.lines[].text' -rLength of output: 1960
Script:
#!/bin/bash # Description: Search for all occurrences of `vendorwiseCommission` in TypeScript files. # Search for `vendorwiseCommission` assignments in .ts and .tsx files rg 'vendorwiseCommission' --glob '*.ts' --glob '*.tsx'Length of output: 4266
| if ( isset( $data['admin_additional_fee'] ) && ( is_numeric( wc_format_decimal( $data['admin_additional_fee'] ) ) || '' === $data['admin_additional_fee'] ) ) { | ||
| $vendor->update_meta( 'dokan_admin_additional_fee', wc_format_decimal( $data['admin_additional_fee'] ) ); | ||
| } |
There was a problem hiding this comment.
Improve validation for 'admin_additional_fee'.
The current condition allows empty strings, which might not be intended. It's advisable to ensure that only numeric values are processed to maintain data integrity.
Apply this diff to enhance validation:
if ( isset( $data['admin_additional_fee'] ) && ( is_numeric( wc_format_decimal( $data['admin_additional_fee'] ) ) || '' === $data['admin_additional_fee'] ) ) {
$vendor->update_meta( 'dokan_admin_additional_fee', wc_format_decimal( $data['admin_additional_fee'] ) );
}Change to:
if ( isset( $data['admin_additional_fee'] ) && is_numeric( wc_format_decimal( $data['admin_additional_fee'] ) ) ) {
$vendor->update_meta( 'dokan_admin_additional_fee', wc_format_decimal( $data['admin_additional_fee'] ) );
}📝 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( $data['admin_additional_fee'] ) && ( is_numeric( wc_format_decimal( $data['admin_additional_fee'] ) ) || '' === $data['admin_additional_fee'] ) ) { | |
| $vendor->update_meta( 'dokan_admin_additional_fee', wc_format_decimal( $data['admin_additional_fee'] ) ); | |
| } | |
| if ( isset( $data['admin_additional_fee'] ) && is_numeric( wc_format_decimal( $data['admin_additional_fee'] ) ) ) { | |
| $vendor->update_meta( 'dokan_admin_additional_fee', wc_format_decimal( $data['admin_additional_fee'] ) ); | |
| } |
| if ( isset( $data['admin_category_commission'] ) ) { | ||
| $vendor->update_meta( 'admin_category_commission', wc_clean( $data['admin_category_commission'] ) ); | ||
| } |
There was a problem hiding this comment.
Add validation for 'admin_category_commission'.
The input for 'admin_category_commission' is being sanitized but not validated. To prevent potential issues, ensure the data is of the expected type before updating metadata.
Apply this diff to include validation:
if ( isset( $data['admin_category_commission'] ) ) {
+ // Check if the input is numeric
+ if ( is_numeric( $data['admin_category_commission'] ) ) {
$vendor->update_meta( 'admin_category_commission', wc_format_decimal( $data['admin_category_commission'] ) );
+ } else {
+ // Handle invalid input appropriately
+ }
}📝 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( $data['admin_category_commission'] ) ) { | |
| $vendor->update_meta( 'admin_category_commission', wc_clean( $data['admin_category_commission'] ) ); | |
| } | |
| if ( isset( $data['admin_category_commission'] ) ) { | |
| // Check if the input is numeric | |
| if ( is_numeric( $data['admin_category_commission'] ) ) { | |
| $vendor->update_meta( 'admin_category_commission', wc_format_decimal( $data['admin_category_commission'] ) ); | |
| } else { | |
| // Handle invalid input appropriately | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
tests/pw/pages/basePage.ts (1)
328-332: Approve the new method with a minor suggestion.The new
selectAndWaitForResponsemethod is a valuable addition to theBasePageclass. It follows the established pattern of similar methods and provides a useful way to select an option while waiting for a specific response.For consistency with other methods in the class, consider adding a comment describing the method's functionality, similar to the comments present for other methods. This would improve the overall documentation of the class.
tests/pw/pages/selectors.ts (3)
Line range hint
6-3845: Consider breaking down the large selector object into smaller modules.The
selectorobject is extremely large and contains selectors for admin, vendor, and customer interfaces. To improve maintainability and readability, consider splitting this into separate files or modules for each major section (admin, vendor, customer). This would make it easier to navigate and maintain the code.You could create separate files like
adminSelectors.ts,vendorSelectors.ts, andcustomerSelectors.ts, and then import and combine them in this main file:import { adminSelectors } from './adminSelectors'; import { vendorSelectors } from './vendorSelectors'; import { customerSelectors } from './customerSelectors'; export const selector = { admin: adminSelectors, vendor: vendorSelectors, customer: customerSelectors, };This approach would make the codebase more modular and easier to maintain.
2972-2988: Address the TODO comment for grouping locators.There's a TODO comment suggesting to group locators for better organization. This is a good suggestion that should be implemented.
Would you like me to provide a suggestion on how to group these locators or create a GitHub issue to track this task?
Line range hint
1-3845: Overall recommendation: Refactor for improved maintainabilityWhile the
selectors.tsfile is well-structured and functional, its large size and some instances of code duplication make it a candidate for refactoring. Here are the key recommendations:
- Split the large selector object into separate files for admin, vendor, and customer interfaces.
- Create shared objects for commonly used selectors (like commission-related selectors) to reduce duplication.
- Address TODO comments, particularly the one about grouping locators.
- Reorganize related selectors (like product variations and commission) into their own objects for better organization.
Implementing these changes will significantly improve the maintainability and readability of the code, making it easier for developers to work with and update in the future.
Consider implementing a modular architecture for your selectors. This could involve creating a base
Selectorclass or interface, and then extending it for specific pages or components. This approach would provide a more structured and scalable way to manage your selectors as your application grows.tests/pw/tests/api/calculation.spec.ts (2)
288-288: Typo in Test DescriptionThe word "receipient" in the test description is misspelled. It should be "recipient".
Apply this diff to correct the typo:
- test.describe.serial('fee receipient calculation test', () => { + test.describe.serial('fee recipient calculation test', () => {
303-373: Consistent Typo in Test NamesThe word "receipient" is consistently misspelled in the test names within this section. Please correct it to "recipient" for clarity and professionalism.
Apply this diff to correct the typos:
- test('fee receipient: shippingFee=seller, shippingTax=seller, productTax=seller', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=seller, shippingTax=seller, productTax=seller', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=seller, shippingTax=seller, productTax=admin', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=seller, shippingTax=seller, productTax=admin', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=seller, shippingTax=admin, productTax=seller', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=seller, shippingTax=admin, productTax=seller', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=seller, shippingTax=admin, productTax=admin', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=seller, shippingTax=admin, productTax=admin', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=admin, shippingTax=seller, productTax=seller', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=admin, shippingTax=seller, productTax=seller', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=admin, shippingTax=seller, productTax=admin', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=admin, shippingTax=seller, productTax=admin', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=admin, shippingTax=admin, productTax=seller', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=admin, shippingTax=admin, productTax=seller', { tag: ['@lite'] }, async () => { - test('fee receipient: shippingFee=admin, shippingTax=admin, productTax=admin', { tag: ['@lite'] }, async () => { + test('fee recipient: shippingFee=admin, shippingTax=admin, productTax=admin', { tag: ['@lite'] }, async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- tests/pw/pages/basePage.ts (2 hunks)
- tests/pw/pages/selectors.ts (6 hunks)
- tests/pw/tests/api/calculation.spec.ts (1 hunks)
- tests/pw/utils/testData.ts (7 hunks)
🧰 Additional context used
🔇 Additional comments (5)
tests/pw/utils/testData.ts (4)
541-550: LGTM! Commission structure aligns with PR objectives.The new
commissionobject inproductInfoprovides a comprehensive structure for managing different commission types, including fixed, percentage-based, and category-based commissions. This aligns well with the PR objectives to enhance commission management and improve the display of commission-related information.
875-883: LGTM! Consistent commission structure in setup wizard.The
commissionobject added to thedokanSetupWizardsection maintains consistency with the structure used inproductInfo. This addition allows for comprehensive commission setup during the initial wizard, which aligns with the PR's objective of redesigning the commission UI across various settings.
2092-2100: LGTM! Consistent commission structure in Dokan settings.The
commissionobject added todokanSettings.sellingmaintains consistency with the commission structures used elsewhere in the codebase. This addition provides a centralized way to manage commission settings, which aligns with the PR's objective of enhancing commission management and improving consistency across the plugin.
1142-1142: LGTM! New API endpoint for multi-step categories.The addition of the
multistepCategoriesAPI endpoint tosubUrls.api.dokansuggests new functionality for handling multi-step categories. This aligns with the PR objectives of improving the management of commission-related information and enhancing the overall functionality of the plugin.tests/pw/pages/selectors.ts (1)
Line range hint
1-4: File structure and organization looks good.The file starts with a clear structure, exporting a
selectorobject that contains all the selectors. This organization makes it easy to import and use these selectors in test files.
Fix price decimal consistency
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (1)
templates/orders/listing.php (1)
81-91: Improved earnings display with currency consistency and precision.The changes enhance the display of order earnings by ensuring currency consistency and increasing precision. This aligns well with the PR objectives of improving commission-related information display.
Suggestions for consideration:
- Consider adding a comment explaining why two additional decimal places are added. This will help future maintainers understand the reasoning behind this specific adjustment.
- You might want to make the number of additional decimal places configurable, allowing for flexibility in different contexts or regions where such precision might not be necessary.
Would you like me to propose a code snippet for adding a comment and making the decimal places configurable?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- includes/REST/CommissionControllerV1.php (1 hunks)
- src/admin/components/CombineInput.vue (4 hunks)
- src/admin/components/Commission/CategoryBasedCommission.vue (1 hunks)
- src/admin/components/Currency.vue (1 hunks)
- src/admin/components/VendorCommissionFields.vue (1 hunks)
- templates/orders/commission-meta-box-html.php (1 hunks)
- templates/orders/listing.php (1 hunks)
- templates/products/edit-product-single.php (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/admin/components/Currency.vue (1)
11-11:⚠️ Potential issueConsider the implications of increased precision and add error handling.
The change increases the currency precision by 2 decimal places, which could provide more accurate price representations. However, this modification raises a few concerns:
- The increased precision might not be appropriate for all currencies and could potentially confuse users.
- There's no check to ensure
dokan.currency.precisionexists before adding to it.Consider the following improvements:
- Add a check for the existence of
dokan.currency.precision:- return accounting.formatMoney( value, {...dokan.currency, precision: dokan.currency.precision+ 2} ); + return accounting.formatMoney( value, {...dokan.currency, precision: (dokan.currency.precision || 0) + 2} );
- Make the precision increase configurable:
- return accounting.formatMoney( value, {...dokan.currency, precision: dokan.currency.precision+ 2} ); + const PRECISION_INCREASE = 2; // Consider making this a prop or a constant + return accounting.formatMoney( value, {...dokan.currency, precision: (dokan.currency.precision || 0) + PRECISION_INCREASE} );
- Ensure this precision increase is consistent with other parts of the application dealing with currency formatting.
To verify the consistency of currency precision across the codebase, run the following script:
This will help identify if the precision increase is an isolated change or part of a broader update to currency handling in the application.
templates/orders/listing.php (1)
Line range hint
1-91: Verify the impact of increased decimal precision on the UI.The changes to the earnings display enhance precision and ensure currency consistency, which aligns well with the PR objectives. The rest of the order listing functionality remains intact.
To ensure a smooth user experience:
- Please verify that the increased number of decimal places doesn't cause any layout issues, especially on mobile devices or with long numbers.
- Consider adding a test case to check if the earnings are correctly displayed with the new precision across different currencies and order amounts.
To assist in verifying the UI impact, you can run the following command to check for potential overflow issues in the CSS:
This will help identify any CSS rules that might need adjustment to accommodate the increased precision in earnings display.
src/admin/components/CombineInput.vue (3)
43-43: Confirm the correctness of the debounce import and usageYou've switched the debounce function import to:
import Debounce from "debounce";Please verify that the
Debouncefunction imported from the "debounce" package matches the expected API and is compatible with its usage in theonInputmethod.
67-68: Ensure empty string defaults do not cause unintended behaviorYou've changed the default values of
fixedandpercentagefrom0to empty strings'':value: { type: Object, default: { fixed: '', percentage: '' } },And initialized data properties accordingly:
fixed: this.formatPositiveValue( this.value.fixed ) ?? '', percentage: this.formatPositiveValue( this.value.percentage ) ?? ''Please verify that all computations and validations that rely on these properties can handle empty strings without causing errors or unexpected behavior.
Also applies to: 74-75
96-96: Reevaluate the debounce delay reductionThe debounce delay for the
onInputmethod has been reduced from1600msto500ms:}, 500 ),While a shorter delay improves responsiveness, it might lead to more frequent function calls, which could impact performance on slower devices. Consider whether
500msis the optimal balance between responsiveness and performance.templates/orders/commission-meta-box-html.php (1)
77-77:⚠️ Potential issueEnsure
$classvariable is definedThe
$classvariable used in$row_classmay not be defined, which could lead to an undefined variable notice. Initialize$classbefore usage.+$class = isset( $class ) ? $class : ''; $row_class = apply_filters( 'woocommerce_admin_html_order_item_class', ! empty( $class ) ? $class : '', $item, $order );Likely invalid or redundant comment.
templates/products/edit-product-single.php (1)
244-251: Ensure Proper Sanitization of Data Passed to TemplateThe
$dataarray is being populated with product category information and a'from'parameter before being passed to the template partproducts/dokan-category-header-ui. Please verify that all elements within$data, especially those derived from user inputs or database queries, are properly sanitized and escaped within the template to prevent potential security vulnerabilities like Cross-Site Scripting (XSS).
| if ( value === '' ) { | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance input validation in formatPositiveValue
The formatPositiveValue method now checks for empty strings, but it doesn't handle non-numeric inputs, which could lead to errors when formatting.
Consider adding validation to ensure that value is a valid number before formatting:
formatPositiveValue: ( value ) => {
if ( value === '' ) {
return value;
}
+ if ( isNaN( Number( value ) ) ) {
+ return '';
+ }
return accounting.formatNumber( value, dokan.currency.precision, dokan.currency.thousand, dokan.currency.decimal );
},This change ensures that non-numeric inputs are handled gracefully.
📝 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 ( value === '' ) { | |
| return value; | |
| } | |
| if ( value === '' ) { | |
| return value; | |
| } | |
| if ( isNaN( Number( value ) ) ) { | |
| return ''; | |
| } | |
| fixedCOmmissionhandler(data) { | ||
| if (isNaN( data.fixed )) { | ||
| data.fixed = this.fixedCommission.fixed ?? ''; | ||
| } | ||
| if (isNaN( data.percentage )) { | ||
| data.percentage = this.fixedCommission.percentage ?? ''; | ||
| } | ||
| this.fixedCommission = data; | ||
|
|
||
| this.commissionUpdated(); | ||
| }, | ||
|
|
||
| onCategoryUpdate(data) { | ||
| this.categoryCommission = data; | ||
|
|
||
| this.commissionUpdated(); | ||
| }, | ||
|
|
||
| commissionUpdated() { | ||
| this.vendorInfo.admin_additional_fee = this.fixedCommission.fixed ?? ''; | ||
| this.vendorInfo.admin_commission = this.fixedCommission.percentage ?? ''; | ||
| this.vendorInfo.admin_commission_type = this.selectedCommission; | ||
| this.vendorInfo.admin_category_commission = JSON.parse( JSON.stringify( this.categoryCommission ) ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Mutating Props Directly; Use Events to Notify Parent Components
Directly modifying the vendorInfo prop within the child component can lead to unexpected behavior, as props are intended to be immutable in Vue.js components. Instead, emit an event to inform the parent component of the changes, allowing it to update vendorInfo accordingly.
Apply this diff to emit an event with the updated commission data:
methods: {
fixedCOmmissionhandler(data) {
if (isNaN(data.fixed)) {
data.fixed = this.fixedCommission.fixed ?? '';
}
if (isNaN(data.percentage)) {
data.percentage = this.fixedCommission.percentage ?? '';
}
this.fixedCommission = data;
this.commissionUpdated();
},
onCategoryUpdate(data) {
this.categoryCommission = data;
this.commissionUpdated();
},
- commissionUpdated() {
- this.vendorInfo.admin_additional_fee = this.fixedCommission.fixed ?? '';
- this.vendorInfo.admin_commission = this.fixedCommission.percentage ?? '';
- this.vendorInfo.admin_commission_type = this.selectedCommission;
- this.vendorInfo.admin_category_commission = JSON.parse(JSON.stringify(this.categoryCommission));
- }
+ commissionUpdated() {
+ this.$emit('update-vendor-info', {
+ admin_additional_fee: this.fixedCommission.fixed ?? '',
+ admin_commission: this.fixedCommission.percentage ?? '',
+ admin_commission_type: this.selectedCommission,
+ admin_category_commission: JSON.parse(JSON.stringify(this.categoryCommission)),
+ });
+ }
}In the parent component, listen for the update-vendor-info event and update the vendorInfo object accordingly:
<vendor-commission-fields
:vendor-info="vendorInfo"
@update-vendor-info="handleVendorInfoUpdate"
/>methods: {
handleVendorInfoUpdate(updatedInfo) {
this.vendorInfo = {
...this.vendorInfo,
...updatedInfo,
};
},
},| 'product_id' => [ | ||
| 'description' => __( 'Products price', 'dokan-lite' ), | ||
| 'type' => 'integer', | ||
| 'default' => 0, | ||
| 'required' => true, | ||
| 'sanitize_callback' => 'absint', | ||
| ], |
There was a problem hiding this comment.
Correct the description of the product_id parameter.
The description for the product_id parameter currently reads 'Products price', which is incorrect. The product_id parameter should represent the ID of the product, not the product's price. Please update the description to accurately reflect this.
Apply this diff to correct the description:
- 'description' => __( 'Products price', 'dokan-lite' ),
+ 'description' => __( 'Product ID', 'dokan-lite' ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'product_id' => [ | |
| 'description' => __( 'Products price', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'default' => 0, | |
| 'required' => true, | |
| 'sanitize_callback' => 'absint', | |
| ], | |
| 'product_id' => [ | |
| 'description' => __( 'Product ID', 'dokan-lite' ), | |
| 'type' => 'integer', | |
| 'default' => 0, | |
| 'required' => true, | |
| 'sanitize_callback' => 'absint', | |
| ], |
| public function get_permissions_check() { | ||
| // phpcs:ignore WordPress.WP.Capabilities.Unknown | ||
| return current_user_can( 'dokandar' ) || current_user_can( 'manage_options' ); | ||
| } |
There was a problem hiding this comment.
Add the $request parameter to the permission callback function.
The get_permissions_check method should accept a WP_REST_Request parameter to comply with the expected signature of permission callbacks in the WordPress REST API. This allows for more advanced permission checks based on the request data.
Apply this diff to update the method signature:
- public function get_permissions_check() {
+ public function get_permissions_check( $request ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function get_permissions_check() { | |
| // phpcs:ignore WordPress.WP.Capabilities.Unknown | |
| return current_user_can( 'dokandar' ) || current_user_can( 'manage_options' ); | |
| } | |
| public function get_permissions_check( $request ) { | |
| // phpcs:ignore WordPress.WP.Capabilities.Unknown | |
| return current_user_can( 'dokandar' ) || current_user_can( 'manage_options' ); | |
| } |
| * @package dokan | ||
| */ | ||
|
|
||
| $total_commission = 0 > $total_commission ? 0 : $total_commission; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify total commission calculation using max function
To enhance readability, consider using the max function to ensure $total_commission is not negative.
-$total_commission = 0 > $total_commission ? 0 : $total_commission;
+$total_commission = max( 0, $total_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.
| $total_commission = 0 > $total_commission ? 0 : $total_commission; | |
| $total_commission = max( 0, $total_commission ); |
| $shipping_fee_refunded += (float) $refund->get_shipping_total(); | ||
| $product_tax_fee_refunded += (float) $refund->get_shipping_tax(); | ||
| $shipping_tax_fee_refunded += (float) $refund->get_cart_tax(); |
There was a problem hiding this comment.
Correct refund calculations for tax fees
Similarly, in the refund calculations, $product_tax_fee_refunded and $shipping_tax_fee_refunded are assigned incorrectly. Swap their assignments to reflect the correct tax types.
foreach ( $order->get_refunds() as $refund ) {
$shipping_fee_refunded += (float) $refund->get_shipping_total();
- $product_tax_fee_refunded += (float) $refund->get_shipping_tax();
- $shipping_tax_fee_refunded += (float) $refund->get_cart_tax();
+ $product_tax_fee_refunded += (float) $refund->get_cart_tax();
+ $shipping_tax_fee_refunded += (float) $refund->get_shipping_tax();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $shipping_fee_refunded += (float) $refund->get_shipping_total(); | |
| $product_tax_fee_refunded += (float) $refund->get_shipping_tax(); | |
| $shipping_tax_fee_refunded += (float) $refund->get_cart_tax(); | |
| $shipping_fee_refunded += (float) $refund->get_shipping_total(); | |
| $product_tax_fee_refunded += (float) $refund->get_cart_tax(); | |
| $shipping_tax_fee_refunded += (float) $refund->get_shipping_tax(); |
| $percentage = isset( $commission_data->get_parameters()['percentage'] ) ? $commission_data->get_parameters()['percentage'] : 0; | ||
| $flat = isset( $commission_data->get_parameters()['flat'] ) ? $commission_data->get_parameters()['flat'] : 0; | ||
| $commission_type_html = $all_commission_types[ $commission_type ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize by storing commission parameters in a variable
To avoid multiple method calls and improve performance, store the parameters returned by $commission_data->get_parameters() in a variable.
+$parameters = $commission_data->get_parameters();
-$percentage = isset( $commission_data->get_parameters()['percentage'] ) ? $commission_data->get_parameters()['percentage'] : 0;
-$flat = isset( $commission_data->get_parameters()['flat'] ) ? $commission_data->get_parameters()['flat'] : 0;
+$percentage = isset( $parameters['percentage'] ) ? $parameters['percentage'] : 0;
+$flat = isset( $parameters['flat'] ) ? $parameters['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.
| $percentage = isset( $commission_data->get_parameters()['percentage'] ) ? $commission_data->get_parameters()['percentage'] : 0; | |
| $flat = isset( $commission_data->get_parameters()['flat'] ) ? $commission_data->get_parameters()['flat'] : 0; | |
| $commission_type_html = $all_commission_types[ $commission_type ]; | |
| $parameters = $commission_data->get_parameters(); | |
| $percentage = isset( $parameters['percentage'] ) ? $parameters['percentage'] : 0; | |
| $flat = isset( $parameters['flat'] ) ? $parameters['flat'] : 0; | |
| $commission_type_html = $all_commission_types[ $commission_type ]; |
| $original_commission = $commission_data->get_admin_commission(); | ||
|
|
||
| if ( $order->get_total_refunded_for_item( $item_id ) ) { | ||
| $commission_refunded = ( $order->get_total_refunded_for_item( $item_id ) / $amount ) * $original_commission; |
There was a problem hiding this comment.
Prevent division by zero in commission refund calculation
There is a potential division by zero error if $amount is zero. Ensure $amount is not zero before performing the calculation.
if ( $order->get_total_refunded_for_item( $item_id ) ) {
+ if ( $amount != 0 ) {
$commission_refunded = ( $order->get_total_refunded_for_item( $item_id ) / $amount ) * $original_commission;
+ } else {
+ $commission_refunded = 0;
+ }
}Committable suggestion was skipped due to low confidence.
| getCatgroies() { | ||
| const result = []; | ||
| const categoryMap = {}; | ||
|
|
||
| // First, create a map of categories using term_id as the key | ||
| for (const term_id in this.categories) { | ||
| categoryMap[term_id] = this.categories[term_id]; | ||
| } | ||
|
|
||
| // Iterate through the categories to rearrange them | ||
| for (const term_id in categoryMap) { | ||
| const category = categoryMap[term_id]; | ||
|
|
||
| // If the category has a parent (parent_id is not 0), find its parent and insert it after the parent | ||
| if (category.parent_id !== '0') { | ||
| const parent = categoryMap[category.parent_id]; | ||
| const parentIndex = result.indexOf(parent); | ||
|
|
||
| // Insert the child category right after its parent | ||
| result.splice(parentIndex + 1, 0, category); | ||
| } else { | ||
| // If it's a top-level category (parent_id is 0), add it to the result | ||
| result.push(category); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| }, | ||
|
|
There was a problem hiding this comment.
Correct spelling mistakes in method and variable names
There are multiple instances where 'commission' is misspelled as 'comission' or 'commissin', and 'categories' is misspelled as 'catgroies' in method names and variable names. Consistent and correct naming improves code readability and maintainability.
Please apply the following corrections:
- Rename the method
getCatgroiestogetCategories:
- getCatgroies() {
+ getCategories() {And update all references to this method:
- this.renderCategories = Object.values( this.getCatgroies() );
+ this.renderCategories = Object.values( this.getCategories() );- Correct
comission_typetocommission_typein method definitions and usages:
- getCommissionValue( comission_type, term_id ) {
+ getCommissionValue( commission_type, term_id ) {- return this.commission.items[term_id][comission_type];
+ return this.commission.items[term_id][commission_type];- Rename
commissinItemHandlertocommissionItemHandlerand correct parameter names:
- commissinItemHandler( value, commission_type, term_id, oldValue = '' ) {
+ commissionItemHandler( value, commission_type, term_id, oldValue = '' ) {- Update all instances where 'comission' is used to 'commission' throughout the code.
Also applies to: 248-255, 256-280
| catRowClick( item, index, ) { | ||
| if ( this.openRows.includes( Number( item.term_id ) ) ) { | ||
| // Remove the parent. | ||
| let index = this.openRows.indexOf(Number( item.term_id )); | ||
| this.openRows.splice(index, 1); | ||
|
|
||
| this.getChildren( item.term_id ).forEach( child => { | ||
| let index = this.openRows.indexOf(Number( child )); | ||
| -1 !== index ? this.openRows.splice(index, 1) : ""; | ||
| } ); | ||
| } else { |
There was a problem hiding this comment.
Avoid variable shadowing of index parameter
In the catRowClick method, a local variable index is declared at line 211, which shadows the index parameter of the method. This can lead to confusion and potential bugs. Please rename the local variable to avoid shadowing.
Suggested change:
- let index = this.openRows.indexOf(Number( item.term_id ));
+ let openRowIndex = this.openRows.indexOf(Number( item.term_id ));And update the subsequent usage:
- this.openRows.splice(index, 1);
+ this.openRows.splice(openRowIndex, 1);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- includes/Product/Hooks.php (3 hunks)
🔇 Additional comments (2)
includes/Product/Hooks.php (2)
492-495: Duplicate comment: Ensure proper sanitization and escaping.The previous review comment regarding sanitization and escaping of output in
add_per_product_commission_optionsis still applicable. Please ensure that all output is properly escaped to prevent XSS vulnerabilities.Refer to the previous suggestion:
- <input id="admin_commission" class="input-text wc_input_price" type="text" name="_per_product_admin_commission" value="<?php echo $admin_commission; ?>"> + <input id="admin_commission" class="input-text wc_input_price" type="text" name="_per_product_admin_commission" value="<?php echo esc_attr( $admin_commission ); ?>"> - <input class="input-text wc_input_price" type="text" name="_per_product_admin_additional_fee" value="<?php echo wc_format_localized_price( $additional_fee ); ?>"> + <input class="input-text wc_input_price" type="text" name="_per_product_admin_additional_fee" value="<?php echo esc_attr( wc_format_localized_price( $additional_fee ) ); ?>">
541-545: Duplicate comment: Sanitize and format input data properly.The previous review comment about using
wc_format_decimalfor proper sanitization and formatting insave_per_product_commission_optionsis still valid. Please ensure that numerical inputs are correctly processed.Refer to the previous suggestion:
- $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore + $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : wc_format_decimal( sanitize_text_field( $_POST['_per_product_admin_commission'] ) ); - $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ); // phpcs:ignore + $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : wc_format_decimal( sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ) );
| if ( isset( $_POST['_per_product_admin_commission_type'] ) ) { // phpcs:ignore | ||
| $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage'; // phpcs:ignore | ||
| update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type ); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary // phpcs:ignore comments.
The // phpcs:ignore comments are used to bypass coding standards checks. It's better to resolve the issues reported by PHPCS rather than ignoring them.
Apply this diff to remove the comments:
if ( isset( $_POST['_per_product_admin_commission_type'] ) ) {
- $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage'; // phpcs:ignore
+ $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage';
update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type );
}Committable suggestion was skipped due to low confidence.
| class="woocommerce-help-tip" | ||
| tabindex="0" for="admin_commission" | ||
| aria-label="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | ||
| data-tip="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | ||
| ></span> |
There was a problem hiding this comment.
Correct the use of for attribute and esc_attr_e function.
- The
forattribute is not valid on<span>elements; it's intended for<label>elements. Consider removing it or placing it on the appropriate element. - The function
esc_attr_eechoes the escaped string, which is not suitable within attribute contexts that already have an echo statement. Useesc_attr__instead.
Apply this diff to fix the issues:
<span
class="woocommerce-help-tip"
- tabindex="0" for="admin_commission"
- aria-label="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>"
- data-tip="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>"
+ tabindex="0"
+ aria-label="<?php echo esc_attr__( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>"
+ data-tip="<?php echo esc_attr__( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>"
></span>📝 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.
| class="woocommerce-help-tip" | |
| tabindex="0" for="admin_commission" | |
| aria-label="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | |
| data-tip="<?php esc_attr_e( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | |
| ></span> | |
| class="woocommerce-help-tip" | |
| tabindex="0" | |
| aria-label="<?php echo esc_attr__( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | |
| data-tip="<?php echo esc_attr__( 'When the value is 0, no commissions will be deducted from this vendor.', 'dokan-lite' ); ?>" | |
| ></span> |
| add_action( 'woocommerce_product_options_advanced', array( $this, 'add_per_product_commission_options' ), 15 ); | ||
| add_action( 'woocommerce_process_product_meta_simple', array( $this, 'save_per_product_commission_options' ), 15 ); | ||
| add_action( 'woocommerce_process_product_meta_variable', array( $this, 'save_per_product_commission_options' ), 15 ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistency in array syntax.
The code uses both short array syntax [] and traditional array syntax array(). For consistency and modern PHP practices, it's recommended to use short array syntax throughout the codebase.
Apply this diff to update the array syntax:
- add_action( 'woocommerce_product_options_advanced', array( $this, 'add_per_product_commission_options' ), 15 );
- add_action( 'woocommerce_process_product_meta_simple', array( $this, 'save_per_product_commission_options' ), 15 );
- add_action( 'woocommerce_process_product_meta_variable', array( $this, 'save_per_product_commission_options' ), 15 );
+ add_action( 'woocommerce_product_options_advanced', [ $this, 'add_per_product_commission_options' ], 15 );
+ add_action( 'woocommerce_process_product_meta_simple', [ $this, 'save_per_product_commission_options' ], 15 );
+ add_action( 'woocommerce_process_product_meta_variable', [ $this, 'save_per_product_commission_options' ], 15 );📝 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.
| add_action( 'woocommerce_product_options_advanced', array( $this, 'add_per_product_commission_options' ), 15 ); | |
| add_action( 'woocommerce_process_product_meta_simple', array( $this, 'save_per_product_commission_options' ), 15 ); | |
| add_action( 'woocommerce_process_product_meta_variable', array( $this, 'save_per_product_commission_options' ), 15 ); | |
| add_action( 'woocommerce_product_options_advanced', [ $this, 'add_per_product_commission_options' ], 15 ); | |
| add_action( 'woocommerce_process_product_meta_simple', [ $this, 'save_per_product_commission_options' ], 15 ); | |
| add_action( 'woocommerce_process_product_meta_variable', [ $this, 'save_per_product_commission_options' ], 15 ); |
| $product = wc_get_product( get_the_ID() ); | ||
| $admin_commission = $product->get_meta( '_per_product_admin_commission' ); | ||
| $additional_fee = $product->get_meta( '_per_product_admin_additional_fee' ); |
There was a problem hiding this comment.
Add a null check for the product object.
The $product object may be false if the product is not found. Adding a null check ensures that the code doesn't throw errors if $product is invalid.
Apply this diff to add the null check:
$admin_commission = $product->get_meta( '_per_product_admin_commission' );
$additional_fee = $product->get_meta( '_per_product_admin_additional_fee' );
+ if ( ! $product ) {
+ return;
+ }📝 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.
| $product = wc_get_product( get_the_ID() ); | |
| $admin_commission = $product->get_meta( '_per_product_admin_commission' ); | |
| $additional_fee = $product->get_meta( '_per_product_admin_additional_fee' ); | |
| $product = wc_get_product( get_the_ID() ); | |
| if ( ! $product ) { | |
| return; | |
| } | |
| $admin_commission = $product->get_meta( '_per_product_admin_commission' ); | |
| $additional_fee = $product->get_meta( '_per_product_admin_additional_fee' ); |
…tore # Conflicts: # dokan.php # includes/Upgrade/Upgrades.php # src/admin/components/Fields.vue # src/admin/pages/Settings.vue # tests/pw/utils/dbData.ts # tests/pw/utils/testData.ts
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (21)
includes/Upgrade/Upgrades.php (1)
47-47: Document the breaking changes in upgrade notes.Since this is a major version upgrade (4.0.0) involving commission restructuring, it's important to document the changes and migration steps.
Consider adding a code comment above the version entry to reference the related PR and breaking changes:
+ // PR #2027: Commission restructure - Breaking changes in commission calculation and storage '4.0.0' => Upgrades\V_4_0_0::class,webpack.config.js (2)
57-59: Consider standardizing the file organization pattern.The new commission-related entry points follow different organizational patterns:
- Setup wizard component:
assets/src/js/setup-wizard/commission/- Category commission component:
src/admin/components/Commission/This inconsistency could make it harder for developers to locate and maintain these files.
Consider moving all admin-related components under
src/admin/components/for better maintainability.
57-59: Add missing styles entry point for setup wizard commission.While you've added the JavaScript entry point for the setup wizard commission component, there's no corresponding styles entry point. This is inconsistent with other components in the configuration that have both JS and styles entries.
Consider adding a styles entry point:
'dokan-setup-wizard-commission': './assets/src/js/setup-wizard/commission/index.js', + 'dokan-setup-wizard-commission-style': '/assets/src/less/setup-wizard/commission.less', // Category commission component styles. 'dokan-category-commission': '/src/admin/components/Commission/index.js',tests/pw/utils/dbUtils.ts (1)
193-200: Consider enhancing error handling and documentation.The implementation aligns well with the PR objectives for improving suborder visibility. However, consider the following improvements:
- Add JSDoc documentation explaining the method's purpose and parameters
- Add error handling for invalid order IDs
- Add input validation for the orderId parameter
- Ensure consistent type handling (number vs string)
Consider applying these enhancements:
+/** + * Retrieves the IDs of child orders associated with a parent order. + * @param orderId - The ID of the parent order + * @returns Promise<string[]> Array of child order IDs + * @throws {Error} If the order ID is invalid or database query fails + */ async getChildOrderIds(orderId: string): Promise<string[]> { + if (!orderId || isNaN(parseInt(orderId))) { + throw new Error('Invalid order ID provided'); + } + const query = `SELECT id FROM ${dbPrefix}_wc_orders WHERE parent_order_id = ?;`; const res = await dbUtils.dbQuery(query, [orderId]); - const ids = res.map((row: { id: number }) => row.id); + const ids = res.map((row: { id: number }) => row.id.toString()); return ids; },templates/products/products-listing-row.php (1)
225-234: Consider adding an action hook for extensibility.To maintain consistency with other columns and allow for extensibility, consider adding an action hook after the earnings column, similar to other columns.
Add this after line 232:
} else { echo '<span class="na">–</span>'; } + + /** + * Fire an action to add extra content after product earning column in product listing table + * + * @since 3.11.5 + * + * @param \WC_Product $product Current product. + */ + do_action( 'dokan_product_list_table_after_column_content_earning', $product ); ?>tests/pw/tests/api/_env.setup.ts (1)
124-176: Consider enhancing test setup for commission scenarios.While the reorganization of test setup is good, given that this PR focuses on commission restructuring, consider adding specific test setup cases for:
- Different commission scenarios (product-specific, vendor-specific, global rates)
- Commission calculations with various tax and shipping configurations
- Setup for testing commission display in product lists and order details
This would better align the test setup with the PR objectives of enhancing commission management and display.
includes/Product/Hooks.php (1)
504-514: Move inline styles to a proper CSS file.The inline styles should be moved to a dedicated CSS file for better maintainability and caching.
Create a new CSS file (e.g.,
assets/css/product-commission.css) and move these styles there:.commission .wrapper input { width: 60px; float: none; } span.combine-commission-description { margin-left: 5px; font-size: 13px; font-style: italic; }templates/products/edit-product-single.php (1)
376-376: Consider removing unnecessary wrapper divThe action hook is wrapped in an empty form-group div which doesn't serve any purpose. Consider removing the wrapper div unless it's needed for styling or structure.
-<div class="dokan-form-group"> <?php do_action( 'dokan_product_edit_after_pricing', $post, $post_id ); ?> -</div>tests/pw/pages/settingsPage.ts (1)
113-116: Consider expanding commission test coverage.The current test coverage focuses on basic commission settings. Consider adding test cases for:
- Product-specific commission rates
- Vendor-specific commission rates
- Commission rate priorities
- Complex scenarios with multiple commission types
This would better align with the PR objectives of implementing a layered commission structure.
Would you like me to help create additional test cases for these scenarios?
tests/pw/utils/dbData.ts (1)
71-94: LGTM! Consider adding documentation for the commission structure.The implementation aligns well with the PR objectives, introducing a flexible commission structure that supports both fixed and category-based rates. The structure allows for product-specific and category-specific commission rates as requested in Issue #2577.
Consider adding code comments or documentation to explain:
- The priority order between different commission types
- The meaning and usage of the
allcategory vs specific category IDs- The relationship between percentage and flat values
tests/pw/feature-map/feature-map.yml (1)
331-347: Consider adding test cases for commission calculation validation.While the current test cases cover the commission configuration scenarios, consider adding test cases to validate the actual commission calculations:
- Verify commission calculations for different commission types
- Test commission calculations with discounts/coupons
- Validate commission splits for multi-vendor orders
- Test commission calculations for refunds/cancellations
includes/Assets.php (2)
590-592: LGTM! Consider adding error handling.The code correctly retrieves commission settings for the current vendor. However, consider adding error handling for cases where the vendor object or commission settings might be null.
-$vendor = dokan()->vendor->get( dokan_get_current_user_id() ); -$commision_settings = $vendor->get_commission_settings(); +$vendor = dokan()->vendor->get( dokan_get_current_user_id() ); +$commision_settings = $vendor ? $vendor->get_commission_settings() : null; +if ( ! $commision_settings ) { + $commision_settings = dokan()->commission->get_default_commission_settings(); +}
1216-1230: Add proper version constant and improve documentation.The new method is well-structured but needs proper version documentation.
- Replace
DOKAN_SINCEwith the actual version number where this method was introduced- Add param and return type hints in the documentation
/** * Admin vue localized scripts * - * @since DOKAN_SINCE + * @since 3.7.0 * + * @return array{commission_types: array<string, string>} */ private function get_vue_admin_localized_scripts() {tests/pw/pages/productsPage.ts (4)
Line range hint
1-13: Consider organizing selectors using a dedicated constants file.The current approach of declaring selectors at the top of the file could be improved by moving them to a dedicated constants file, following the separation of concerns principle. This would make the code more maintainable and reusable.
60-62: Fix indentation for better readability.The indentation of this block is inconsistent with the surrounding code. Maintaining consistent indentation improves code readability and helps prevent errors.
- await this.click(productsAdmin.product.subMenus.inventory); - await this.selectByValue(productsAdmin.product.stockStatus, data.product.stockStatus.outOfStock); + await this.click(productsAdmin.product.subMenus.inventory); + await this.selectByValue(productsAdmin.product.stockStatus, data.product.stockStatus.outOfStock);
Line range hint
1017-1048: Enhance error handling in filterProducts method.The filterProducts method could benefit from better error handling and type safety:
- Add error handling for invalid filter types
- Use TypeScript enums for filter types
- Add input validation
+type FilterType = 'by-date' | 'by-category' | 'by-type' | 'by-other'; async filterProducts(filterBy: string, value: string): Promise<void> { + if (!this.isValidFilterType(filterBy)) { + throw new Error(`Invalid filter type: ${filterBy}`); + } + if (!value) { + throw new Error('Filter value is required'); + } await this.goIfNotThere(data.subUrls.frontend.vDashboard.products); switch (filterBy) { // ... existing cases ... default: - break; + throw new Error(`Unhandled filter type: ${filterBy}`); }
Line range hint
1-1124: Overall code quality assessment.The test suite provides comprehensive coverage of product management functionality. However, consider the following improvements:
- Add JSDoc comments for better documentation
- Implement consistent error handling across all methods
- Consider breaking down the large class into smaller, focused classes
- Add more type safety using TypeScript features
tests/pw/utils/apiUtils.ts (1)
899-899: Remove unnecessary empty line.The empty line at 899 appears to be unnecessary and can be removed to maintain consistent code formatting.
-tests/pw/pages/selectors.ts (1)
3013-3029: Consider grouping submenu locators for better organization.The TODO comment suggests grouping locators for better organization. Consider creating a dedicated submenu object that groups related locators together for improved maintainability and readability.
Would you like me to help organize these locators or open a GitHub issue to track this task?
src/admin/components/Fields.vue (1)
687-696: Optimize Deep Cloning in 'watchCategoryCommission' Computed PropertyThe
watchCategoryCommissioncomputed property usesJSON.parse(JSON.stringify(...))to deep clone data, which may be inefficient for large objects.Consider using a more efficient deep cloning method like
_.cloneDeepfrom Lodash for better performance:watchCategoryCommission() { - let data = JSON.parse( JSON.stringify( this.fieldValue[this.fieldData.name] ) ); + let data = _.cloneDeep( this.fieldValue[this.fieldData.name] ); if ( window._.isEmpty( data ) ) { return {}; } return data; },tests/pw/utils/payloads.ts (1)
4593-4609: Consider removingadmin_category_commissionwhenadmin_commission_typeis'fixed'In the
vendorwiseCommissionobject,admin_commission_typeis set to'fixed', butadmin_category_commissionis also provided. Since the commission type is'fixed', the category-based commission settings inadmin_category_commissionmay be ignored. To prevent confusion or unintended behavior, consider removingadmin_category_commissionwhen using a fixed commission type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
includes/Assets.php(6 hunks)includes/Product/Hooks.php(2 hunks)includes/Upgrade/Upgrades.php(1 hunks)src/admin/components/Fields.vue(7 hunks)src/admin/pages/Settings.vue(6 hunks)templates/products/edit-product-single.php(2 hunks)templates/products/products-listing-row.php(1 hunks)tests/pw/feature-map/feature-map.yml(1 hunks)tests/pw/pages/basePage.ts(2 hunks)tests/pw/pages/productsPage.ts(8 hunks)tests/pw/pages/selectors.ts(6 hunks)tests/pw/pages/settingsPage.ts(2 hunks)tests/pw/pages/vendorAuctionsPage.ts(1 hunks)tests/pw/tests/api/_env.setup.ts(1 hunks)tests/pw/utils/apiUtils.ts(1 hunks)tests/pw/utils/dbData.ts(4 hunks)tests/pw/utils/dbUtils.ts(3 hunks)tests/pw/utils/interfaces.ts(5 hunks)tests/pw/utils/payloads.ts(6 hunks)webpack.config.js(1 hunks)
🔇 Additional comments (36)
includes/Upgrade/Upgrades.php (2)
47-47: Verify upgrade path compatibility.
The version jump from 3.13.0 to 4.0.0 is significant. We need to ensure that sites running older versions can safely upgrade through all intermediate versions.
Let's check the upgrade path:
#!/bin/bash
# Check for any gaps in version numbers that might affect the upgrade path
echo "Analyzing version sequence..."
rg "'(\d+\.\d+\.?\d*)'.*=>.*class" | sort -V
# Look for version-specific checks in the upgrade process
echo "Checking version-specific upgrade handling..."
rg "version_compare.*4\.0\.0" 47-47: Verify the existence and implementation of V_4_0_0 upgrade class.
The addition of version 4.0.0 represents a major version upgrade, which typically indicates breaking changes. Given that this is part of the commission restructure enhancement, it's crucial to ensure proper data migration.
Let's verify the upgrade class implementation:
Consider the following recommendations for the upgrade process:
- Ensure backward compatibility for existing commission data
- Include rollback procedures in case of upgrade failures
- Add detailed logging for tracking the migration progress
✅ Verification successful
V_4_0_0 upgrade class is properly implemented with commission migration support
The upgrade class exists and includes comprehensive commission migration functionality:
- Global commission type update method
- Category commission types migration
- Vendor-specific commission updates
- Product-level commission migration
- Background processing support for large-scale updates
The implementation follows the proper upgrade pattern and includes necessary data migration for the commission restructure enhancement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the V_4_0_0 upgrade class exists and contains commission-related migrations
echo "Checking for V_4_0_0 upgrade class..."
fd "V_4_0_0.php" --type f
# Look for commission-related migration code in the upgrade class
echo "Checking commission migration implementation..."
rg -A 10 "class V_4_0_0"
Length of output: 2828
Script:
#!/bin/bash
# Let's check for the V_4_0_0 class implementation in the upgrades directory
echo "Searching for V_4_0_0 class file..."
fd -e php -t f "V_4_0_0" includes/Upgrade/
# If the file exists, let's examine its contents
echo "Checking implementation details..."
rg -l "class V_4_0_0" includes/Upgrade/
# Let's also look for any commission-related migration code
echo "Searching for commission-related migrations..."
rg -A 10 "commission" includes/Upgrade/V_4_0_0.php || true
# Check the Upgrades namespace structure
echo "Verifying namespace structure..."
rg "namespace.*Upgrades" includes/Upgrade/
Length of output: 4801
Script:
#!/bin/bash
# Let's examine the V_4_0_0 class implementation and its commission update process
echo "Checking V_4_0_0 class implementation..."
rg -A 10 "class V_4_0_0" includes/Upgrade/Upgrades/V_4_0_0.php
# Check the background process implementation
echo "Examining commission update background process..."
rg -A 10 "class V_4_0_0_UpdateCommissions" includes/Upgrade/Upgrades/BackgroundProcesses/V_4_0_0_UpdateCommissions.php
# Look for any commission-related methods in the upgrade class
echo "Searching for commission-related methods..."
rg "commission" includes/Upgrade/Upgrades/V_4_0_0.php
Length of output: 1825
webpack.config.js (1)
57-59: Verify the commission components' dependencies.
Since these components are part of a major commission structure update, let's verify their dependencies are properly configured.
✅ Verification successful
Let me gather more information about the commission components to verify their dependencies thoroughly.
Let me check one final aspect to verify the component initialization and entry points.
Dependencies are properly configured for commission components
The verification shows that:
- Components have correct Vue imports and proper exports
- Entry point (index.js) correctly initializes the Vue application
- AdminCommission properly imports CategoryBasedCommission
- No circular dependencies detected
- Component registration and hierarchy is well-structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential missing dependencies in the commission components
# Test 1: Check for Vue component dependencies
echo "Checking Vue component dependencies..."
rg -l "import.*from.*vue" ./assets/src/js/setup-wizard/commission/ ./src/admin/components/Commission/
# Test 2: Check for commission-related API imports
echo "Checking API dependencies..."
rg -l "import.*from.*api" ./assets/src/js/setup-wizard/commission/ ./src/admin/components/Commission/
# Test 3: Verify if components are properly exported
echo "Verifying component exports..."
rg -l "export default" ./assets/src/js/setup-wizard/commission/ ./src/admin/components/Commission/
Length of output: 764
Script:
#!/bin/bash
# Check the actual imports and component structure
# Check imports in AdminCommission.vue
echo "Checking imports in AdminCommission.vue:"
rg "import" ./assets/src/js/setup-wizard/commission/AdminCommission.vue -A 1
# Check imports in CategoryBasedCommission.vue
echo -e "\nChecking imports in CategoryBasedCommission.vue:"
rg "import" ./src/admin/components/Commission/CategoryBasedCommission.vue -A 1
# Check if there are any shared utilities or common dependencies
echo -e "\nChecking for shared utilities or dependencies:"
rg "import.*from.*@/utils|import.*from.*@/api" ./assets/src/js/setup-wizard/commission/ ./src/admin/components/Commission/ -A 1
# Check component registration and usage
echo -e "\nChecking component registration and usage:"
rg "components:|Vue.component" ./assets/src/js/setup-wizard/commission/ ./src/admin/components/Commission/ -A 3
Length of output: 1698
Script:
#!/bin/bash
# Check entry points and component initialization
# Check the setup wizard entry point
echo "Checking setup wizard commission entry point:"
cat ./assets/src/js/setup-wizard/commission/index.js
# Check if CategoryBasedCommission is properly exported
echo -e "\nChecking CategoryBasedCommission exports:"
rg "export" ./src/admin/components/Commission/CategoryBasedCommission.vue
# Check for any potential circular dependencies
echo -e "\nChecking for potential circular dependencies:"
rg -l "AdminCommission" ./src/admin/components/Commission/
rg -l "CategoryBasedCommission" ./assets/src/js/setup-wizard/commission/
Length of output: 1071
tests/pw/pages/vendorAuctionsPage.ts (1)
29-29: LGTM! Verify selector consistency across test files.
The selector update from direct auction reference to submenu structure correctly adapts to UI changes.
Let's verify the consistency of this selector change across other test files:
✅ Verification successful
Let me gather more context about the auction selectors to ensure a thorough verification.
Selector change is consistent with codebase patterns
The verification shows that:
- The
product.subMenuspattern is consistently used across multiple test files (booking, products, commission pages) - The auction-related selectors in
vendorAuctionsPage.tsfollow a clear pattern with both admin and vendor contexts - No outdated direct auction selectors were found that need updating
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct auction selectors that might need updating
# and verify the consistent usage of submenu structure across test files
# Search for direct auction selectors
echo "Checking for potential direct auction selectors that might need updating..."
rg -l "product\.auction[^s]" "tests/pw/pages/"
# Verify consistent usage of submenu structure
echo "Verifying consistent usage of submenu structure..."
rg "product\.subMenus\.auction" "tests/pw/pages/"
Length of output: 515
Script:
#!/bin/bash
# Get more context around the auction selector usage
echo "Checking auction selector usage with context..."
rg -B 2 -A 2 "product\.auction" "tests/pw/pages/"
# Check if there are any other similar submenu patterns
echo "Checking other submenu patterns..."
rg "product\.subMenus\." "tests/pw/pages/"
Length of output: 4155
tests/pw/utils/dbUtils.ts (3)
4-4: LGTM!
The added empty line improves code organization by separating imports from the rest of the code.
131-131: LGTM!
The formatting change maintains code functionality while improving readability.
1-1: Verify the impact of removing getSellingInfo method.
The AI summary indicates that the getSellingInfo method has been removed. Let's verify that all references to this method have been updated accordingly.
✅ Verification successful
Removal of getSellingInfo appears to be part of a larger commission system restructuring
Based on the git history, we can see that getSellingInfo was previously used to fetch commission and fee recipient information, and has been systematically removed. The lack of current SQL queries or replacement patterns, combined with the PR's objective of commission system restructuring, suggests this is an intentional removal as part of the system redesign.
- The only remaining references are in test files (
tests/pw/utils/payloads.tsandtests/pw/pages/selectors.ts) and appear to be unrelated to the actual method (they're just string content) - Git history shows consistent removal of the method and its usages
- No new commission/fee-related database queries were found that could indicate improper removal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to getSellingInfo
rg "getSellingInfo" --type ts
# Search for potential commission-related method calls that might need updates
rg "commission.*info|selling.*info" --type ts
Length of output: 709
Script:
#!/bin/bash
# Let's check for any historical references to getSellingInfo in git history
git log -p | rg "getSellingInfo"
# Let's also check for any database-related selling or commission queries
rg -A 3 "SELECT.*(?i)(selling|commission)" --type ts
Length of output: 1806
Script:
#!/bin/bash
# Let's check for any current references to commission or fee-related database queries
# that might have replaced getSellingInfo
ast-grep --pattern 'const $_: Promise<$_> = await $_.$_'
# Also check for any SQL queries related to commission or fees
rg "SELECT.*FROM.*(?i)(commission|fee)" --type ts
Length of output: 110
templates/products/products-listing-row.php (1)
225-234: LGTM! Well-structured implementation with proper security measures.
The new earnings column is implemented securely and consistently with the existing table structure. The code follows WordPress security best practices with proper escaping.
Let's verify the commission calculation method implementation:
✅ Verification successful
The commission calculation implementation is properly integrated and well-tested
The get_earning_by_product method is correctly implemented in includes/Commission.php and is well-tested as evidenced by comprehensive test cases in tests/php/src/Commission/CommissionTest.php. The method:
- Handles both product objects and IDs
- Includes proper validation
- Supports different contexts ('seller' and 'admin')
- Has filter hooks for extensibility (
dokan_get_earning_by_product) - Is consistently used across multiple templates
The implementation aligns with the PR objectives and is being used correctly in the products listing row template.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the commission calculation implementation
# Look for the implementation of get_earning_by_product method
ast-grep --pattern 'function get_earning_by_product($product) {
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach to find the commission calculation method
# First, let's search for files containing 'Commission' class
fd -e php | xargs rg -l "class.*Commission"
# Then search for the method name with broader context
rg -A 10 "get_earning_by_product"
Length of output: 10366
tests/pw/tests/api/_env.setup.ts (3)
151-160: LGTM! Payment gateway setup matches previous recommendations.
The basic payment gateway setup looks good, and the commented-out gateways are properly conditioned on DOKAN_PRO. This aligns with previous review recommendations about payment gateway setup.
162-176: Good consolidation of category and attribute setup.
The consolidation of category and attribute setup into a single test case is a good improvement:
- Reduces test execution time by avoiding multiple setup/teardown cycles
- Maintains atomic cleanup of both categories and attributes
- Provides a clearer setup structure for product-related tests
This is particularly relevant for commission testing as it ensures a clean slate for product categories that might have different commission rates.
141-143: Verify if commented-out shipping methods are required.
The FreeShipping and LocalPickup methods are commented out. Given that this PR involves commission restructuring, these shipping methods might be needed for comprehensive commission testing scenarios.
✅ Verification successful
Let me check the actual usage of these shipping methods in the referenced files to understand if they are required for commission testing.
Let me check if these shipping methods are specifically used in any commission-related tests to make a final determination.
The commented shipping methods can be safely left commented out
The evidence shows that while FreeShipping and LocalPickup methods are defined in the codebase, they are not referenced in any commission-related test files. The commission tests (commission.spec.ts, calculation.spec.ts, etc.) don't depend on these shipping methods. The existing FlatRate shipping method along with Dokan-specific shipping methods (TableRate and DistanceRate) appear sufficient for commission testing scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these shipping methods are referenced in commission-related tests
rg -l "FreeShipping|LocalPickup" tests/
Length of output: 162
Script:
#!/bin/bash
# Check the context of FreeShipping and LocalPickup usage
rg -C 3 "FreeShipping|LocalPickup" tests/pw/tests/api/_env.setup.ts tests/pw/tests/e2e/_env.setup.ts
Length of output: 2614
Script:
#!/bin/bash
# Search for commission-related test files and their content
rg -l "commission" tests/pw/tests/
# Then check if these files reference the shipping methods
rg -C 3 "FreeShipping|LocalPickup" $(rg -l "commission" tests/pw/tests/)
Length of output: 591
includes/Product/Hooks.php (1)
470-516: Verify commission field integration with templates.
Let's ensure the commission fields are properly integrated with Dokan templates and WooCommerce hooks.
✅ Verification successful
Let me gather more information about the commission field integration.
Based on the comprehensive verification results, I can now provide the final response:
Commission field implementation is properly integrated and functional ✓
The verification shows:
- Commission fields are correctly integrated with WooCommerce product options via
woocommerce_product_options_advancedhook - The commission data is properly saved and retrieved using product meta
- The commission template (commission-meta-box-html.php) correctly displays and handles commission calculations
- The implementation includes proper handling of commission types, rates, and additional fees
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for commission field usage in templates and potential conflicts
# Test 1: Check if commission fields are used in templates
echo "Checking commission field usage in templates..."
rg -l "admin_commission|per_product_admin_commission" templates/
# Test 2: Check for potential conflicts with WooCommerce hooks
echo "Checking for potential hook conflicts..."
rg -A 2 "woocommerce_product_options_advanced"
Length of output: 806
Script:
#!/bin/bash
# Check the implementation details of commission-related functionality
# Check commission meta box template content
echo "Checking commission meta box template content..."
cat templates/orders/commission-meta-box-html.php
# Check save_per_product_commission_options implementation
echo -e "\nChecking save commission options implementation..."
ast-grep --pattern 'function save_per_product_commission_options($$$)'
# Check for any other commission-related hooks
echo -e "\nChecking other commission-related hooks..."
rg -A 2 "_per_product_admin_commission" --type php
Length of output: 20836
templates/products/edit-product-single.php (1)
276-284: Add error handling for category data retrieval
While the code structure is good, consider adding error handling for the Helper::get_saved_products_category() call to gracefully handle potential failures.
$data = Helper::get_saved_products_category( $post_id );
+if ( is_wp_error( $data ) ) {
+ echo '<div class="dokan-alert dokan-alert-danger">';
+ echo esc_html( $data->get_error_message() );
+ echo '</div>';
+ $data = array();
+}
$data['from'] = 'edit_product';tests/pw/pages/settingsPage.ts (2)
114-116: LGTM: Commission settings implementation aligns with restructuring goals.
The implementation correctly handles the new commission structure by setting the commission type and values (percentage and fixed) separately.
144-145:
Fix incorrect assertion for fixed commission value.
The assertion for the fixed commission value is using the percentage field selector, which will lead to incorrect validation.
Apply this fix as mentioned in the previous review:
await this.toHaveValue(settingsAdmin.selling.percentage, selling.commission.commissionPercentage);
-await this.toHaveValue(settingsAdmin.selling.percentage, selling.commission.commissionFixed);
+await this.toHaveValue(settingsAdmin.selling.fixed, selling.commission.commissionFixed);tests/pw/utils/dbData.ts (1)
1389-1389: LGTM! Good addition of a plain text version.
The plain text version of the setup wizard message is a good addition, useful for scenarios where HTML rendering isn't available or desired.
tests/pw/feature-map/feature-map.yml (1)
331-347: LGTM! Well-structured commission feature test cases.
The new Commission page section comprehensively covers all key commission management scenarios including setup wizard, selling settings, vendor-specific, product-specific, and subscription product settings. The test cases are well-organized and consistent with the overall feature map structure.
includes/Assets.php (2)
603-604: LGTM! Commission data properly localized.
The code correctly adds commission percentage and type to the localized data, making it available for frontend scripts to handle commission-related functionality.
216-220: LGTM! Route properly configured.
The new route for single vendor view is correctly configured and follows the existing route pattern conventions.
tests/pw/pages/basePage.ts (2)
235-235: LGTM! Code readability improvement.
The condensed implementation maintains the same functionality while improving readability.
339-344: LGTM! Well-implemented select dropdown handler.
The new method follows established patterns and enhances test coverage for commission-related dropdown interactions. It properly handles concurrent operations and response validation.
tests/pw/utils/apiUtils.ts (1)
Line range hint 82-82: Duplicate comment: Address the TODO comment regarding maxPageLimit.
The maxPageLimit parameter is not fully handled. Ensure that the function respects this limit by stopping requests when the limit is reached.
src/admin/pages/Settings.vue (2)
97-99: 'Back to Top' button implemented correctly
The 'Back to Top' button is effectively implemented with a click handler that smoothly scrolls to the top. Good job on enhancing user navigation.
636-638: Proper cleanup of event listeners in beforeDestroy
Removing the resize event listener in the beforeDestroy hook prevents potential memory leaks. Well done on following best practices.
tests/pw/utils/interfaces.ts (4)
653-653: Addition of 'commission' field to 'dokanSetupWizard' interface
The commission field has been correctly added to the dokanSetupWizard interface.
758-759: Addition of 'commission' field to 'vendorInfo' interface
The commission field has been correctly added to the vendorInfo interface.
1540-1540: Addition of 'commission' field to 'dokanSettings.selling' interface
The commission field has been correctly added to the dokanSettings.selling interface.
2018-2024: Previous suggestion regarding 'commission' interface is still applicable
The previous review comment about enhancing the commission interface for greater flexibility and type safety remains valid.
src/admin/components/Fields.vue (7)
Line range hint 75-96: Well-Implemented 'commission_fixed' Field Type
The new template for 'commission_fixed' correctly integrates the CombineInput component. Value bindings for fixed and percentage fees are appropriately handled, and the commissionUpdated method ensures that changes are reflected in the parent component. Error messages are displayed correctly based on validation results.
99-118: Effective Integration of 'category_based_commission' Field Type
The addition of the 'category_based_commission' template introduces the CategoryBasedCommission component seamlessly. The component utilizes watchCategoryCommission for value binding and communicates changes through the onCategoryUpdate method. This enhances the flexibility of commission settings based on product categories.
120-137: Dynamic Rendering of Commission Fields for Pro Version
The conditional rendering of custom commission fields when fieldData.dokan_pro_commission is 'yes' is handled efficiently. Iterating over commissionFieldComponents with v-for allows for modular and extendable code, accommodating additional commission components in the future.
512-513: Correct Importation of New Components
The CombineInput and CategoryBasedCommission components are imported correctly from their respective paths. This ensures that the components are available for use within the template.
524-525: Accurate Registration of Components
The newly imported components CombineInput and CategoryBasedCommission are properly registered within the components object. This enables their usage within the template sections of the Vue component.
552-552: Extensibility Through Hook Application
The use of dokan.hooks.applyFilters to initialize commissionFieldComponents enhances the extensibility of the commission fields. This approach allows other developers to add or modify commission field components through hooks.
960-962: Correct State Update in 'onCategoryUpdate' Method
The onCategoryUpdate method effectively updates the fieldValue with the new category commission data received from the component, ensuring that the state remains in sync.
tests/pw/utils/payloads.ts (1)
4626-4639: Verify the correctness of commission values
In the productCommissionOnlyFixed array, _per_product_admin_commission is set to '0', and _per_product_admin_additional_fee is '10'. Typically, for a fixed commission, _per_product_admin_commission represents the percentage commission, and _per_product_admin_additional_fee represents the fixed amount. Ensure that setting the percentage commission to '0' is intentional and that the fixed fee is correctly applied.
Run the following script to check for any unintended zero percentage commissions:
|
|
||
| <span class="wrapper"> | ||
| <input type="hidden" value="fixed" name="_per_product_admin_commission_type"> | ||
| <input id="admin_commission" class="input-text wc_input_price" type="text" name="_per_product_admin_commission" value="<?php echo $admin_commission; ?>"> |
There was a problem hiding this comment.
Properly escape commission values.
The commission values should be properly escaped to prevent potential XSS vulnerabilities.
Apply this diff to fix the escaping:
- <input id="admin_commission" class="input-text wc_input_price" type="text" name="_per_product_admin_commission" value="<?php echo $admin_commission; ?>">
+ <input id="admin_commission" class="input-text wc_input_price" type="text" name="_per_product_admin_commission" value="<?php echo esc_attr( $admin_commission ); ?>">
- <input class="input-text wc_input_price" type="text" name="_per_product_admin_additional_fee" value="<?php echo wc_format_localized_price( $additional_fee ); ?>">
+ <input class="input-text wc_input_price" type="text" name="_per_product_admin_additional_fee" value="<?php echo esc_attr( wc_format_localized_price( $additional_fee ) ); ?>">Also applies to: 497-497
| public static function save_per_product_commission_options( $post_id ) { | ||
| if ( ! current_user_can( 'manage_woocommerce' ) ) { | ||
| return; | ||
| } | ||
|
|
||
| $commission_type = ''; | ||
| $admin_commission = ''; | ||
| $additional_fee = ''; | ||
|
|
||
| if ( isset( $_POST['_per_product_admin_commission_type'] ) ) { // phpcs:ignore | ||
| $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage'; // phpcs:ignore | ||
| update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type ); | ||
| } | ||
|
|
||
| if ( isset( $_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_additional_fee'] ) ) { // phpcs:ignore | ||
| $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ); // phpcs:ignore | ||
| } | ||
|
|
||
| update_post_meta( $post_id, '_per_product_admin_commission', wc_format_decimal( $admin_commission ) ); | ||
| update_post_meta( $post_id, '_per_product_admin_additional_fee', wc_format_decimal( $additional_fee ) ); | ||
| } |
There was a problem hiding this comment.
Add numeric validation for commission values.
The commission values should be validated as numeric values before saving to prevent invalid data.
Apply this diff to add validation:
if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore
- $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
+ $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] );
+ if ( '' !== $admin_commission && ! is_numeric( $admin_commission ) ) {
+ return;
+ }
}
if ( isset( $_POST['_per_product_admin_additional_fee'] ) ) { // phpcs:ignore
- $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ); // phpcs:ignore
+ $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] );
+ if ( '' !== $additional_fee && ! is_numeric( $additional_fee ) ) {
+ return;
+ }
}Additionally, consider removing the static keyword as this method doesn't need to be static.
📝 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 static function save_per_product_commission_options( $post_id ) { | |
| if ( ! current_user_can( 'manage_woocommerce' ) ) { | |
| return; | |
| } | |
| $commission_type = ''; | |
| $admin_commission = ''; | |
| $additional_fee = ''; | |
| if ( isset( $_POST['_per_product_admin_commission_type'] ) ) { // phpcs:ignore | |
| $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage'; // phpcs:ignore | |
| update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type ); | |
| } | |
| if ( isset( $_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_additional_fee'] ) ) { // phpcs:ignore | |
| $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ); // phpcs:ignore | |
| } | |
| update_post_meta( $post_id, '_per_product_admin_commission', wc_format_decimal( $admin_commission ) ); | |
| update_post_meta( $post_id, '_per_product_admin_additional_fee', wc_format_decimal( $additional_fee ) ); | |
| } | |
| public static function save_per_product_commission_options( $post_id ) { | |
| if ( ! current_user_can( 'manage_woocommerce' ) ) { | |
| return; | |
| } | |
| $commission_type = ''; | |
| $admin_commission = ''; | |
| $additional_fee = ''; | |
| if ( isset( $_POST['_per_product_admin_commission_type'] ) ) { // phpcs:ignore | |
| $commission_type = ! empty( $_POST['_per_product_admin_commission_type'] ) ? sanitize_text_field( $_POST['_per_product_admin_commission_type'] ) : 'percentage'; // phpcs:ignore | |
| update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type ); | |
| } | |
| if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore | |
| $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] ); | |
| if ( '' !== $admin_commission && ! is_numeric( $admin_commission ) ) { | |
| return; | |
| } | |
| } | |
| if ( isset( $_POST['_per_product_admin_additional_fee'] ) ) { // phpcs:ignore | |
| $additional_fee = ( '' === $_POST['_per_product_admin_additional_fee'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_additional_fee'] ); | |
| if ( '' !== $additional_fee && ! is_numeric( $additional_fee ) ) { | |
| return; | |
| } | |
| } | |
| update_post_meta( $post_id, '_per_product_admin_commission', wc_format_decimal( $admin_commission ) ); | |
| update_post_meta( $post_id, '_per_product_admin_additional_fee', wc_format_decimal( $additional_fee ) ); | |
| } |
| <label for="_regular_price" class="form-label"><?php esc_html_e( 'Price', 'dokan-lite' ); ?> | ||
| <span | ||
| class="vendor-earning simple-product" | ||
| data-commission="<?php echo esc_attr( dokan()->commission->get_earning_by_product( $post_id ) ); ?>" | ||
| data-product-id="<?php echo esc_attr( $post_id ); ?>"> | ||
| ( <?php esc_html_e( ' You Earn : ', 'dokan-lite' ); ?><?php echo esc_html( get_woocommerce_currency_symbol() ); ?> | ||
| <span class="vendor-price"> | ||
| <?php | ||
| echo wp_kses_post( | ||
| wc_price( | ||
| dokan()->commission->get_earning_by_product( $post_id ), | ||
| [ | ||
| 'currency' => get_woocommerce_currency_symbol(), | ||
| 'decimals' => wc_get_price_decimals() + 2, | ||
| ] | ||
| ) | ||
| ); | ||
| ?> | ||
| </span> | ||
| ) | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve vendor earnings display formatting
- Remove unnecessary space before the colon in the earnings text
- Simplify the nested span structure
- Use standard decimal places as per previous review
- <span
- class="vendor-earning simple-product"
- data-commission="<?php echo esc_attr( dokan()->commission->get_earning_by_product( $post_id ) ); ?>"
- data-product-id="<?php echo esc_attr( $post_id ); ?>">
- ( <?php esc_html_e( ' You Earn : ', 'dokan-lite' ); ?><?php echo esc_html( get_woocommerce_currency_symbol() ); ?>
- <span class="vendor-price">
- <?php
- echo wp_kses_post(
- wc_price(
- dokan()->commission->get_earning_by_product( $post_id ),
- [
- 'currency' => get_woocommerce_currency_symbol(),
- 'decimals' => wc_get_price_decimals() + 2,
- ]
- )
- );
- ?>
- </span>
- )
- </span>
+ <span class="vendor-earning simple-product"
+ data-commission="<?php echo esc_attr( dokan()->commission->get_earning_by_product( $post_id ) ); ?>"
+ data-product-id="<?php echo esc_attr( $post_id ); ?>">
+ (<?php esc_html_e( 'You Earn:', 'dokan-lite' ); ?>
+ <?php
+ echo wp_kses_post(
+ wc_price(
+ dokan()->commission->get_earning_by_product( $post_id ),
+ ['currency' => get_woocommerce_currency_symbol()]
+ )
+ );
+ ?>)
+ </span>📝 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.
| <label for="_regular_price" class="form-label"><?php esc_html_e( 'Price', 'dokan-lite' ); ?> | |
| <span | |
| class="vendor-earning simple-product" | |
| data-commission="<?php echo esc_attr( dokan()->commission->get_earning_by_product( $post_id ) ); ?>" | |
| data-product-id="<?php echo esc_attr( $post_id ); ?>"> | |
| ( <?php esc_html_e( ' You Earn : ', 'dokan-lite' ); ?><?php echo esc_html( get_woocommerce_currency_symbol() ); ?> | |
| <span class="vendor-price"> | |
| <?php | |
| echo wp_kses_post( | |
| wc_price( | |
| dokan()->commission->get_earning_by_product( $post_id ), | |
| [ | |
| 'currency' => get_woocommerce_currency_symbol(), | |
| 'decimals' => wc_get_price_decimals() + 2, | |
| ] | |
| ) | |
| ); | |
| ?> | |
| </span> | |
| ) | |
| </span> | |
| </label> | |
| <label for="_regular_price" class="form-label"><?php esc_html_e( 'Price', 'dokan-lite' ); ?> | |
| <span class="vendor-earning simple-product" | |
| data-commission="<?php echo esc_attr( dokan()->commission->get_earning_by_product( $post_id ) ); ?>" | |
| data-product-id="<?php echo esc_attr( $post_id ); ?>"> | |
| (<?php esc_html_e( 'You Earn:', 'dokan-lite' ); ?> | |
| <?php | |
| echo wp_kses_post( | |
| wc_price( | |
| dokan()->commission->get_earning_by_product( $post_id ), | |
| ['currency' => get_woocommerce_currency_symbol()] | |
| ) | |
| ); | |
| ?>) | |
| </span> | |
| </label> |
| <label v-if="screenWidth < 753" for='dokan-setting-show'><i :class="showMenu || screenWidth >= 753 ? 'fas fa-chevron-down' : 'fas fa-chevron-up'"></i></label> | ||
| <input v-if="screenWidth < 753" class='!hidden' id='dokan-setting-show' checked :value='showMenu' @click='() => showMenu = !showMenu' type='checkbox'> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional logic and use v-model for two-way binding
The conditional logic within :class can be simplified since screenWidth >= 753 will always be false when screenWidth < 753 (as per the v-if condition). Also, using v-model on the checkbox input simplifies the code and ensures proper two-way data binding.
Consider revising the code as follows:
<label v-if="screenWidth < 753" for='dokan-setting-show'>
- <i :class="showMenu || screenWidth >= 753 ? 'fas fa-chevron-down' : 'fas fa-chevron-up'"></i>
+ <i :class="showMenu ? 'fas fa-chevron-down' : 'fas fa-chevron-up'"></i>
</label>
<input v-if="screenWidth < 753" class='!hidden' id='dokan-setting-show' type='checkbox'
- checked :value='showMenu' @click='() => showMenu = !showMenu'>
+ v-model='showMenu'>📝 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.
| <label v-if="screenWidth < 753" for='dokan-setting-show'><i :class="showMenu || screenWidth >= 753 ? 'fas fa-chevron-down' : 'fas fa-chevron-up'"></i></label> | |
| <input v-if="screenWidth < 753" class='!hidden' id='dokan-setting-show' checked :value='showMenu' @click='() => showMenu = !showMenu' type='checkbox'> | |
| <label v-if="screenWidth < 753" for='dokan-setting-show'> | |
| <i :class="showMenu ? 'fas fa-chevron-down' : 'fas fa-chevron-up'"></i> | |
| </label> | |
| <input v-if="screenWidth < 753" class='!hidden' id='dokan-setting-show' type='checkbox' v-model='showMenu'> |
| showMenu: false, | ||
| screenWidth: window.document.documentElement.clientWidth |
There was a problem hiding this comment.
Avoid accessing window in data() initialization
Accessing window.document.documentElement.clientWidth during the data() initialization can cause issues, especially in server-side rendering environments where window is undefined. It's better to initialize screenWidth in the mounted() hook.
Apply this diff to fix the issue:
data () {
return {
// ... other data properties
showMenu: false,
- screenWidth: window.document.documentElement.clientWidth
+ screenWidth: 0
}
},
mounted() {
+ this.screenWidth = window.document.documentElement.clientWidth;
// ... existing mounted code
},Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| commissionUpdated( data ) { | ||
| if (isNaN( data.fixed )) { | ||
| data.fixed = this.fieldValue[this.fieldData.fields.fixed_fee.name] ?? ''; | ||
| } | ||
| if (isNaN( data.percentage )) { | ||
| data.percentage = this.fieldValue[this.fieldData.fields.percent_fee.name] ?? ''; | ||
| } | ||
|
|
||
| this.fieldValue[this.fieldData.fields.percent_fee.name] = data.percentage; | ||
| this.fieldValue[this.fieldData.fields.fixed_fee.name] = data.fixed; |
There was a problem hiding this comment.
Handle NaN and Empty Values Correctly in 'commissionUpdated'
In the commissionUpdated method, checks for isNaN( data.fixed ) might not handle null, undefined, or empty strings properly, since isNaN( '' ) returns false.
Update the conditionals to accurately detect invalid numbers:
commissionUpdated( data ) {
- if (isNaN( data.fixed )) {
+ if ( !data.fixed || isNaN( Number( data.fixed ) )) {
data.fixed = this.fieldValue[this.fieldData.fields.fixed_fee.name] ?? '';
}
- if (isNaN( data.percentage )) {
+ if ( !data.percentage || isNaN( Number( data.percentage ) )) {
data.percentage = this.fieldValue[this.fieldData.fields.percent_fee.name] ?? '';
}
this.fieldValue[this.fieldData.fields.percent_fee.name] = data.percentage;
this.fieldValue[this.fieldData.fields.fixed_fee.name] = data.fixed;
},📝 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.
| commissionUpdated( data ) { | |
| if (isNaN( data.fixed )) { | |
| data.fixed = this.fieldValue[this.fieldData.fields.fixed_fee.name] ?? ''; | |
| } | |
| if (isNaN( data.percentage )) { | |
| data.percentage = this.fieldValue[this.fieldData.fields.percent_fee.name] ?? ''; | |
| } | |
| this.fieldValue[this.fieldData.fields.percent_fee.name] = data.percentage; | |
| this.fieldValue[this.fieldData.fields.fixed_fee.name] = data.fixed; | |
| commissionUpdated( data ) { | |
| if ( !data.fixed || isNaN( Number( data.fixed ) )) { | |
| data.fixed = this.fieldValue[this.fieldData.fields.fixed_fee.name] ?? ''; | |
| } | |
| if ( !data.percentage || isNaN( Number( data.percentage ) )) { | |
| data.percentage = this.fieldValue[this.fieldData.fields.percent_fee.name] ?? ''; | |
| } | |
| this.fieldValue[this.fieldData.fields.percent_fee.name] = data.percentage; | |
| this.fieldValue[this.fieldData.fields.fixed_fee.name] = data.fixed; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
includes/DependencyManagement/Providers/ServiceProvider.php (1)
39-39: Add documentation for the new Fees service.The addition of the Fees service follows the correct registration pattern. However, please add documentation (preferably PHPDoc) explaining:
- The purpose of the Fees service
- Its relationship with the Commission service
- Any specific initialization requirements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
includes/DependencyManagement/Providers/ServiceProvider.php(1 hunks)
🔇 Additional comments (1)
includes/DependencyManagement/Providers/ServiceProvider.php (1)
38-40: Verify service initialization dependencies.
The placement of the 'fees' service after 'commission' suggests a potential dependency. Let's verify the initialization order and any dependencies between these services.
✅ Verification successful
Let me try another verification approach to check for any dependencies between these services.
Based on the results, I can now provide a final response:
The service initialization order is correct and intentional.
The results show that the Commission class is actually dependent on the Fees service, not the other way around. The Commission class contains multiple deprecated methods that delegate to the Fees service (e.g., calculate_gateway_fee, get_shipping_fee_recipient, etc.). The Fees class only uses Commission-related classes for calculation strategies but doesn't have a direct dependency on the Commission service instance.
- The Fees class imports calculation strategy classes:
Calculator,GlobalStrategy,OrderItem,Product, andVendorfrom the Commission namespace - The Commission class heavily uses
dokan()->feesmethods, indicating it's the dependent service - Several methods in Commission are marked as deprecated in favor of their Fees counterparts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dependencies between Fees and Commission services
# Look for Commission class usage in Fees class
echo "Checking Fees class dependencies..."
rg -l "use.*\\Commission" "includes/Fees.php" || echo "No direct Commission import in Fees.php"
# Look for service initialization order requirements
echo "Checking service initialization..."
ast-grep --pattern 'class Fees {
$$$
public function __construct($commission) {
$$$
}
}'
Length of output: 482
Script:
#!/bin/bash
# Check for any dependencies between Commission and Fees classes
# Search for Commission class references in Fees class with fixed regex
echo "Checking Fees class for Commission references..."
rg -i "commission" "includes/Fees.php"
# Look for Fees class implementation and constructor
echo -e "\nChecking Fees class implementation..."
rg -A 5 "class Fees" "includes/Fees.php"
# Check if Commission class uses Fees
echo -e "\nChecking Commission class for Fees references..."
rg -i "fees" "includes/Commission.php"
# Check service provider initialization logic
echo -e "\nChecking service provider initialization..."
rg -A 10 "register_services" "includes/DependencyManagement/Providers/ServiceProvider.php"
Length of output: 3851
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/pw/utils/dbUtils.ts (2)
131-131: Consider improving test data management increateRefundRequestWhile the formatting changes look good, there are opportunities to improve the test utility:
- Consider extracting hardcoded test values to constants or test fixtures
- Add enum or constants for status (0, 1) and method values
- Define TypeScript interfaces for the response body structure
Line range hint
1-200: Improve overall code structure and documentationConsider these structural improvements:
- Add JSDoc comments to document the purpose and parameters of each method
- Implement consistent error handling across all database methods
- Define TypeScript interfaces for common parameter types
- Consider grouping related methods (e.g., order-related, user-related) into separate classes or modules
tests/pw/feature-map/feature-map.yml (1)
334-350: Consider adding more comprehensive test scenarios.The commission feature map could be enhanced with additional test scenarios:
Vendor features:
- Vendor can view earned commission
- Vendor can view commission history
- Vendor can export commission reports
Customer features:
- Customer can view product commission split (if applicable)
Admin features:
- Admin can view commission history/logs
- Admin can export commission reports
- Admin can validate commission calculations
- Admin can override commission rates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
templates/products/edit-product-single.php(2 hunks)tests/pw/feature-map/feature-map.yml(1 hunks)tests/pw/pages/basePage.ts(2 hunks)tests/pw/pages/productsPage.ts(8 hunks)tests/pw/pages/selectors.ts(6 hunks)tests/pw/pages/settingsPage.ts(2 hunks)tests/pw/pages/vendorAuctionsPage.ts(1 hunks)tests/pw/utils/dbData.ts(3 hunks)tests/pw/utils/dbUtils.ts(3 hunks)tests/pw/utils/interfaces.ts(5 hunks)tests/pw/utils/payloads.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- templates/products/edit-product-single.php
- tests/pw/pages/productsPage.ts
- tests/pw/pages/settingsPage.ts
- tests/pw/pages/vendorAuctionsPage.ts
- tests/pw/utils/dbData.ts
🔇 Additional comments (8)
tests/pw/utils/dbUtils.ts (1)
1-1: Verify the impact of removing getSellingInfo
The AI summary indicates that the getSellingInfo method has been removed. Let's verify that all its functionality has been properly migrated to the new commission structure.
tests/pw/utils/interfaces.ts (1)
419-419: LGTM! Consistent interface usage.
The commission interface is consistently applied across multiple interfaces (productInfo, vendor, dokanSetupWizard, dokanSettings), aligning with the PR objectives for commission restructuring.
Also applies to: 656-656, 761-761, 1543-1543
tests/pw/feature-map/feature-map.yml (1)
334-350: LGTM! Well-structured commission feature definitions.
The new Commission page section properly defines test scenarios for various commission management capabilities across different contexts (setup wizard, selling settings, vendor, product, subscription).
tests/pw/pages/basePage.ts (2)
235-235: LGTM! Improved code readability.
The condensed implementation maintains the same functionality while being more concise and readable.
339-344: LGTM! Well-implemented select and wait pattern.
The new method follows the established patterns in the codebase and properly handles asynchronous operations for dropdown selections that trigger API calls.
This addition is particularly relevant for the commission management features mentioned in the PR objectives, as it will help test dropdown interactions in the new commission UI.
tests/pw/pages/selectors.ts (3)
2068-2076: Reuse commission selectors from shared object.
These commission selectors in the selling section are identical to those defined earlier. They should reuse the shared commission selector object.
2623-2633: Reuse commission selectors from shared object.
The commission selectors in the setup wizard section are also duplicated. They should reuse the shared commission selector object.
3153-3165: Reuse commission selectors from shared object.
The commission selectors in the product commission section are duplicated again. They should reuse the shared commission selector object.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
tests/pw/tests/e2e/vendorSettings.spec.ts (2)
93-94: Good practice: Cleaning up catalog settings after test.The addition of cleanup code after the catalog settings test helps prevent test interference and maintains a clean test environment.
Consider moving the cleanup to an
afterEachhook or a shared cleanup utility to ensure it runs even if the test fails:const cleanupCatalogSettings = async () => { await dbUtils.updateOptionValue(dbData.dokan.optionName.selling, { catalog_mode_hide_add_to_cart_button: 'off', catalog_mode_hide_product_price: 'off' }); };
Line range hint
119-121: Good practice: Cleaning up min-max settings after test.Similar to the catalog settings, this cleanup helps maintain test isolation.
Consider applying the same cleanup pattern suggestion here:
const cleanupMinMaxSettings = async () => { await dbUtils.updateOptionValue(dbData.dokan.optionName.selling, { enable_min_max_quantity: 'off', enable_min_max_amount: 'off' }); };tests/pw/pages/commissionPage.ts (3)
19-35: RefactoraddCommissionmethod to eliminate code duplicationThe
addCommissionmethod contains repetitive code blocks for handling different commission types and categories. Refactoring can improve readability and maintainability.Consider extracting common logic into helper functions and simplifying conditional structures. This will reduce nesting and make the code more scalable for future enhancements.
38-54: RefactorassertCommissionmethod to eliminate code duplicationSimilar to
addCommission, theassertCommissionmethod has repetitive code when asserting commission values. Refactoring can enhance maintainability.Extract shared assertions into helper methods and streamline conditional checks to reduce redundancy.
105-159: Unify commission setting methods across entitiesThe methods
setCommissionForVendor,setCommissionForProduct, andsetCommissionToDokanSubscriptionProductperform similar operations for setting commissions on different entities. Consolidating these methods can reduce code duplication.Consider creating a generic method that accepts parameters to handle commission settings for various entities. This approach will improve scalability and ease future maintenance.
tests/pw/utils/testData.ts (3)
574-583: Consider refactoring repeatedcommissionobjects to improve maintainability.The
commissionobject is defined multiple times with similar structures throughout the code. To reduce duplication and enhance maintainability, consider creating a sharedcommissiontemplate or function that can be reused wherever needed.
1297-1305: Consider refactoring repeatedcommissionobjects invendorInfo.Similar to previous instances, the
commissionobject here has the same structure. Refactoring these into a shared configuration can reduce redundancy and simplify future updates.
2137-2145: Consolidatecommissionsettings indokanSettingsto avoid duplication.The
commissionobject withindokanSettings.sellingmirrors other commission definitions. Centralizing this configuration can enhance code clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
tests/pw/pages/commissionPage.ts(1 hunks)tests/pw/pages/settingsPage.ts(2 hunks)tests/pw/tests/e2e/coupons.spec.ts(0 hunks)tests/pw/tests/e2e/vendorSettings.spec.ts(1 hunks)tests/pw/utils/dbData.ts(4 hunks)tests/pw/utils/testData.ts(7 hunks)
💤 Files with no reviewable changes (1)
- tests/pw/tests/e2e/coupons.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pw/pages/settingsPage.ts
- tests/pw/utils/dbData.ts
🧰 Additional context used
🪛 Biome
tests/pw/pages/commissionPage.ts
[error] 14-16: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (4)
tests/pw/utils/testData.ts (4)
26-26: Verify that CATEGORY_ID is properly set in the environment variables.
The CATEGORY_ID is imported from process.env. Ensure that this environment variable is defined and correctly set in your testing environment to prevent any runtime issues.
889-897: Typo in property name: specficCategory should be specificCategory.
The property specficCategory appears to have a typo. It should be specificCategory for consistency and correctness.
908-916: Ensure consistency in commission settings.
In the dokanSetupWizard, the commissionFixed is set to '0', whereas in other instances it has a value like '10'. Please verify that this is intentional and consistent with the overall commission logic.
1182-1182: Confirm the implementation of the new API endpoint multistepCategories.
The addition of multistepCategories under data.api.dokan introduces a new API endpoint. Ensure that corresponding handlers and tests are implemented to support this endpoint properly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/pw/utils/schemas.ts (1)
2125-2125: LGTM! Consider adding validation for percentage format.The change to support both string and number types for commission values is good and aligns with the commission restructuring objectives. This provides flexibility to handle both fixed amounts and percentage-based commissions.
Consider adding regex validation for percentage strings to ensure they follow a consistent format:
-commission: z.string().or(z.number()), +commission: z.number().or( + z.string().regex(/^\d+(\.\d{1,2})?%?$/, + 'Commission percentage must be a valid number with up to 2 decimal places, optionally followed by %') +),tests/pw/utils/helpers.ts (3)
99-99: AlignpriceStringWithDecimalparameter type withpriceStringFor consistency and improved type safety, consider restricting the
choiceparameter inpriceStringWithDecimalto'US' | 'ES', similar topriceString.Apply this diff to update the function signature:
-priceStringWithDecimal: (num: number, choice: string): string => (choice === 'US' ? Number(num).toLocaleString('es-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 }) : Number(num).toLocaleString('es-ES', { minimumFractionDigits: 2, maximumFractionDigits: 2 })), +priceStringWithDecimal: (num: number, choice: 'US' | 'ES'): string => (choice === 'US' ? Number(num).toLocaleString('es-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 }) : Number(num).toLocaleString('es-ES', { minimumFractionDigits: 2, maximumFractionDigits: 2 })),
302-319: Define interfaces forgatewayDetailsandfeeRecipientparametersThe parameters
gatewayDetailsandfeeRecipientare typed asany, which reduces type safety. Defining proper interfaces enhances code readability and helps catch errors at compile time.Consider adding interfaces like:
interface GatewayDetails { gatewayFee: number; gatewayFeeGiver: string; } interface FeeRecipient { taxFeeRecipient: string; shippingTaxFeeRecipient: string; shippingFeeRecipient: string; }Update the function signature accordingly:
- commissionOrEarning(type: string, productcommissionOrEarning: number = 0, productTax: number = 0, shippingTax: number = 0, shippingFee: number = 0, gatewayDetails: any, feeRecipient: any) { + commissionOrEarning( + type: string, + productCommissionOrEarning: number = 0, + productTax: number = 0, + shippingTax: number = 0, + shippingFee: number = 0, + gatewayDetails: GatewayDetails, + feeRecipient: FeeRecipient + ) {
302-319: Improve parameter naming for clarityThe parameter
productcommissionOrEarningis hard to read due to the lack of camelCase formatting. Renaming it toproductCommissionOrEarningimproves readability and adheres to naming conventions.Apply this diff to rename the parameter:
- commissionOrEarning(type: string, productcommissionOrEarning: number = 0, productTax: number = 0, shippingTax: number = 0, shippingFee: number = 0, gatewayDetails: any, feeRecipient: any) { + commissionOrEarning(type: string, productCommissionOrEarning: number = 0, productTax: number = 0, shippingTax: number = 0, shippingFee: number = 0, gatewayDetails: any, feeRecipient: any) { // Inside the function, update usage: - let commissionOrEarning = Number(productcommissionOrEarning) - Number(gatewayDetails.gatewayFee) + Number(productTax) + Number(shippingTax) + Number(shippingFee); + let commissionOrEarning = Number(productCommissionOrEarning) - Number(gatewayDetails.gatewayFee) + Number(productTax) + Number(shippingTax) + Number(shippingFee);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
tests/pw/pages/settingsPage.ts(3 hunks)tests/pw/utils/helpers.ts(2 hunks)tests/pw/utils/schemas.ts(1 hunks)tests/pw/utils/testData.ts(7 hunks)
🔇 Additional comments (13)
tests/pw/pages/settingsPage.ts (3)
6-6: LGTM: Helper import added for price formatting.
The addition of the helpers import is appropriate for the new price formatting requirements.
115-117: LGTM: Commission settings implementation aligns with new structure.
The implementation correctly handles the new commission structure with separate fields for type, percentage, and fixed amounts, aligning with the commission restructure objectives from PR #2027.
145-146: LGTM: Assertions updated with proper price formatting.
The assertions now correctly use the price formatting helpers for both percentage and fixed commission values. This addresses the past review comments about assertion fixes.
tests/pw/utils/testData.ts (6)
867-898: LGTM! Global commission structure provides comprehensive configuration options.
The implementation supports multiple commission scenarios with appropriate defaults and configuration options for fixed, all-category, and specific-category commission types.
1295-1305: LGTM! Vendor commission structure maintains consistency.
The vendor-specific commission configuration maintains structural consistency with global and product commission settings, ensuring uniform commission handling across different contexts.
908-916: LGTM! Setup wizard commission structure provides appropriate defaults.
The commission configuration in the setup wizard offers a balanced default setup while maintaining structural consistency with the rest of the system.
1182-1182: LGTM! New API endpoint added for commission categories.
The addition of the multistepCategories endpoint supports the enhanced commission management functionality.
2137-2145: LGTM! Settings commission structure ensures consistent configuration.
The commission structure in Dokan settings maintains consistency with other commission configurations throughout the application.
#!/bin/bash
# Description: Verify consistent commission structure usage across the application
# Expected: The same commission structure should be used consistently
# Search for all commission structure implementations
rg -A 10 "commission.*?{[\s\S]*?commissionType.*?commissionPercentage.*?commissionFixed.*?commissionCategory"
# Search for any divergent commission implementations
rg -A 5 "commission.*?{[\s\S]*?(?!commissionType|commissionPercentage|commissionFixed|commissionCategory)"574-583: LGTM! Product commission structure aligns with requirements.
The commission structure provides comprehensive control over product-level commission settings, supporting both fixed and category-based commission types with appropriate configuration options.
tests/pw/utils/helpers.ts (4)
185-188: Duplicate Comment
262-264: Duplicate Comment
282-289: Duplicate Comment
96-96: Verify all usages of priceString after updating the choice parameter type
The choice parameter in priceString has been restricted to 'US' | 'ES', enhancing type safety. Ensure that all calls to priceString in the codebase pass 'US' or 'ES' to prevent TypeScript errors.
Run the following script to find any calls to priceString with a choice parameter other than 'US' or 'ES':
✅ Verification successful
All priceString calls comply with the updated type definition
After verification, all calls to priceString in the codebase consistently use 'ES' as the choice parameter, which matches the restricted type 'US' | 'ES'. No TypeScript errors will occur as all usages are type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to 'priceString' with invalid 'choice' arguments.
rg --type ts --pcre2 'priceString\([^,]+,\s*(?[\'"])[^)]+\)'
Length of output: 210
Script:
#!/bin/bash
# Let's first find all usages of priceString function
rg "priceString\(" --type ts -A 1
Length of output: 1617
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (8)
tests/pw/utils/helpers.ts (3)
96-99: Consider consolidating price formatting logicThe
priceStringandpriceStringWithDecimalmethods share similar logic but with different formatting options. Consider refactoring to reduce code duplication.- priceString: (num: number, choice: 'US' | 'ES'): string => (choice === 'US' ? Number(num).toLocaleString('es-US') : Number(num).toLocaleString('es-ES')), - priceStringWithDecimal: (num: number, choice: string): string => (choice === 'US' ? Number(num).toLocaleString('es-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 }) : Number(num).toLocaleString('es-ES', { minimumFractionDigits: 2, maximumFractionDigits: 2 })), + formatPrice: (num: number, choice: 'US' | 'ES', options?: Intl.NumberFormatOptions): string => { + const locale = choice === 'US' ? 'es-US' : 'es-ES'; + return Number(num).toLocaleString(locale, options); + }, + priceString: (num: number, choice: 'US' | 'ES'): string => helpers.formatPrice(num, choice), + priceStringWithDecimal: (num: number, choice: 'US' | 'ES'): string => + helpers.formatPrice(num, choice, { minimumFractionDigits: 2, maximumFractionDigits: 2 }),
208-209: Remove commented console.log statementRemove the commented debug statement as it adds noise to the codebase.
- // console.log('subtotal:', subtotal, 'subtotalTax:', subtotalTax, 'subtotalCommission:', subtotalCommission, 'subtotalEarning:', subtotalEarning);
315-315: Remove commented console.log statementRemove the commented debug statement to improve code cleanliness.
- // console.log('productCommissionOrEarning', productCommissionOrEarning, 'productTax', productTax, 'shippingTax', shippingTax, 'shippingFee', shippingFee);tests/pw/utils/calculationHelpers.ts (3)
9-10: InitializeapiUtilswithin the function to avoid potential side effectsCurrently,
apiUtilsis declared at the module level but initialized within theassertOrderCalculationfunction. This could lead to unexpected behavior, especially in concurrent executions or when the utility needs to be reused in different contexts. It's advisable to both declare and initializeapiUtilswithin the function scope to encapsulate its usage and prevent potential side effects.Apply this diff to adjust the scope:
- let apiUtils: ApiUtils; async function assertOrderCalculation(order: any) { - apiUtils = new ApiUtils(await request.newContext()); + const apiUtils = new ApiUtils(await request.newContext());
14-14: Remove commented-out console statementsCommented-out code can clutter the codebase and may lead to confusion. If these logs are no longer needed, it's better to remove them. If logging is required for debugging, consider implementing a proper logging mechanism with configurable log levels.
Remove or handle the following lines:
- // console.log(orderResponseBody); - // console.table(orderLog);Also applies to: 89-89
130-130: Use a configurable logging mechanism instead ofconsole.tableDirectly using
console.tablemay not be appropriate in all environments and can produce unwanted output. Consider using a logging library that allows you to set log levels and formats, ensuring that verbose logs like tables are only displayed in appropriate contexts (e.g., during development or debugging).Modify the logging approach:
- console.table(orderLog); + // Use a logging library or conditional logging based on environment + if (process.env.NODE_ENV === 'development') { + console.table(orderLog); + }tests/pw/tests/api/calculation.spec.ts (2)
29-33: Consider removing the skipped debug testThe test
'calculation (debug)'at lines 29-33 is currently skipped and appears to be for debugging purposes. If it's no longer needed, consider removing it to clean up the test suite.
594-594: Remove commented-out debugging codeThe commented-out
console.logstatement at line 594 should be removed to keep the codebase clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
tests/pw/tests/api/calculation.spec.ts(1 hunks)tests/pw/utils/calculationHelpers.ts(1 hunks)tests/pw/utils/helpers.ts(3 hunks)
🔇 Additional comments (6)
tests/pw/utils/helpers.ts (1)
185-188: LGTM! Tax calculation implementation is correct
The implementation correctly calculates the total tax for line items by applying the tax rate to each item's subtotal.
tests/pw/utils/calculationHelpers.ts (1)
26-33: Ensure safe number conversions and handle potential NaN values
When converting values to numbers, there's a risk of getting NaN if the source is undefined or not a valid number string. This can lead to unexpected behavior in calculations. Consider validating inputs or providing default values to ensure robustness.
Run the following script to check for potential NaN values:
tests/pw/tests/api/calculation.spec.ts (4)
261-261: Avoid multiple calls to payloads.createProduct() to maintain consistent product data
In line 261, payloads.createProduct() is called multiple times, which may lead to inconsistent product data if createProduct() generates random values. Store the result in a variable to ensure consistency.
272-272: Avoid multiple calls to payloads.createProduct() to maintain consistent product data
Similarly, at line 272, multiple calls to payloads.createProduct() can cause inconsistencies. Reuse a single instance of the product payload.
283-283: Ensure consistent product data by reusing createProduct() result
Again, at line 283, to prevent inconsistencies, store the result of createProduct() in a variable and reuse it within this test case.
570-571: Possible incorrect store update
At line 571, you're updating VENDOR_ID with data fetched for VENDOR2_ID, which may unintentionally overwrite the store settings for the wrong vendor.
| if (!coupon.discount_type) { | ||
| //get coupon details from single order | ||
| const couponMeta = JSON.parse(coupon.meta_data[0].value); | ||
| coupon.discount_type = couponMeta[2]; | ||
| coupon.nominal_amount = couponMeta[3]; | ||
| } |
There was a problem hiding this comment.
Add null checks for coupon meta_data access
The code accesses coupon.meta_data[0].value without verifying that meta_data exists and contains elements.
- const couponMeta = JSON.parse(coupon.meta_data[0].value);
+ if (!coupon.meta_data?.length) {
+ throw new Error('Coupon metadata is missing');
+ }
+ const couponMeta = JSON.parse(coupon.meta_data[0].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.
| if (!coupon.discount_type) { | |
| //get coupon details from single order | |
| const couponMeta = JSON.parse(coupon.meta_data[0].value); | |
| coupon.discount_type = couponMeta[2]; | |
| coupon.nominal_amount = couponMeta[3]; | |
| } | |
| if (!coupon.discount_type) { | |
| //get coupon details from single order | |
| if (!coupon.meta_data?.length) { | |
| throw new Error('Coupon metadata is missing'); | |
| } | |
| const couponMeta = JSON.parse(coupon.meta_data[0].value); | |
| coupon.discount_type = couponMeta[2]; | |
| coupon.nominal_amount = couponMeta[3]; | |
| } |
| for (const { price, quantity, meta_data } of lineItems) { | ||
| const commissionRate = meta_data.find((item: { key: string }) => item.key === '_dokan_commission_rate').value; | ||
| const additionalFee = meta_data.find((item: { key: string }) => item.key === '_dokan_additional_fee').value; | ||
|
|
||
| default: | ||
| break; | ||
| const lineItemTotal = price * quantity; | ||
| const commission = this.percentage(Number(lineItemTotal), Number(commissionRate)) + Number(additionalFee); | ||
| subtotal += lineItemTotal; | ||
| subtotalTax += this.tax(lineItemTotal, taxRate); | ||
| subtotalCommission += commission; | ||
| subtotalEarning += lineItemTotal - commission; |
There was a problem hiding this comment.
Add null checks for meta_data access
The code accesses meta_data properties without verifying their existence, which could lead to runtime errors.
- const commissionRate = meta_data.find((item: { key: string }) => item.key === '_dokan_commission_rate').value;
- const additionalFee = meta_data.find((item: { key: string }) => item.key === '_dokan_additional_fee').value;
+ const commissionRateMeta = meta_data.find((item: { key: string }) => item.key === '_dokan_commission_rate');
+ const additionalFeeMeta = meta_data.find((item: { key: string }) => item.key === '_dokan_additional_fee');
+ const commissionRate = commissionRateMeta?.value ?? 0;
+ const additionalFee = additionalFeeMeta?.value ?? 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.
| for (const { price, quantity, meta_data } of lineItems) { | |
| const commissionRate = meta_data.find((item: { key: string }) => item.key === '_dokan_commission_rate').value; | |
| const additionalFee = meta_data.find((item: { key: string }) => item.key === '_dokan_additional_fee').value; | |
| default: | |
| break; | |
| const lineItemTotal = price * quantity; | |
| const commission = this.percentage(Number(lineItemTotal), Number(commissionRate)) + Number(additionalFee); | |
| subtotal += lineItemTotal; | |
| subtotalTax += this.tax(lineItemTotal, taxRate); | |
| subtotalCommission += commission; | |
| subtotalEarning += lineItemTotal - commission; | |
| for (const { price, quantity, meta_data } of lineItems) { | |
| const commissionRateMeta = meta_data.find((item: { key: string }) => item.key === '_dokan_commission_rate'); | |
| const additionalFeeMeta = meta_data.find((item: { key: string }) => item.key === '_dokan_additional_fee'); | |
| const commissionRate = commissionRateMeta?.value ?? 0; | |
| const additionalFee = additionalFeeMeta?.value ?? 0; | |
| const lineItemTotal = price * quantity; | |
| const commission = this.percentage(Number(lineItemTotal), Number(commissionRate)) + Number(additionalFee); | |
| subtotal += lineItemTotal; | |
| subtotalTax += this.tax(lineItemTotal, taxRate); | |
| subtotalCommission += commission; | |
| subtotalEarning += lineItemTotal - commission; |
| async function assertOrderCalculation(order: any) { | ||
| apiUtils = new ApiUtils(await request.newContext()); | ||
|
|
||
| const [orderResponse, orderResponseBody, orderId] = order; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parameter typing and naming in assertOrderCalculation
The parameter order: any is an array that is destructured into [orderResponse, orderResponseBody, orderId]. Passing an array of mixed types as a single parameter reduces code clarity and type safety. Consider passing these variables as separate, well-typed parameters to enhance readability and maintainability.
Apply this diff to refactor the function signature:
- async function assertOrderCalculation(order: any) {
- const [orderResponse, orderResponseBody, orderId] = order;
+ async function assertOrderCalculation(orderResponse: Response, orderResponseBody: any, orderId: string) {Committable suggestion skipped: line range outside the PR's diff.
| async function assertData(orderId: string, calculatedData: number[], receivedData: number[]) { | ||
| const [calculatedSubTotal, calculatedOrderTotal, calculatedVendorEarning, calculatedAdminCommission, gatewayFee, shippingFee, calculatedShippingTax, calculatedProductTax, calculatedTotalTax, calculatedDiscount, calculatedDiscountTax] = calculatedData; | ||
| const [receivedSubtotal, receivedOrderTotal, receivedVendorEarning, receivedAdminCommission, receivedGatewayFee, receivedShippingFee, receivedShippingTax, receivedProductTax, receivedTotalTax, receivedDiscountTotal, receivedDiscountTax] = receivedData; | ||
|
|
||
| const orderLog = [ | ||
| [`OID: ${orderId}`, 'SubTotal', 'OrderTotal', 'Earning', 'Commission', 'GatewayFee', 'ShippingFee', 'ShippingTax', 'ProductTax', 'TotalTax', 'Discount', 'DiscountTax'], | ||
| ['Expected', calculatedSubTotal, calculatedOrderTotal, calculatedVendorEarning, calculatedAdminCommission, gatewayFee, shippingFee, calculatedShippingTax, calculatedProductTax, calculatedTotalTax, calculatedDiscount, calculatedDiscountTax], | ||
| ['Received', receivedSubtotal, receivedOrderTotal, receivedVendorEarning, receivedAdminCommission, receivedGatewayFee, receivedShippingFee, receivedShippingTax, receivedProductTax, receivedTotalTax, receivedDiscountTotal, receivedDiscountTax], | ||
| ]; | ||
|
|
||
| // console.table(orderLog); | ||
|
|
||
| expect(receivedSubtotal).toBeApproximately(calculatedSubTotal, 0.01, orderLog); | ||
| expect(receivedOrderTotal).toBeApproximately(calculatedOrderTotal, 0.01, orderLog); | ||
| expect(receivedVendorEarning).toBeApproximately(calculatedVendorEarning, 0.01, orderLog); | ||
| expect(receivedAdminCommission).toBeApproximately(calculatedAdminCommission, 0.01, orderLog); | ||
| expect(receivedProductTax).toBeApproximately(calculatedProductTax, 0.01, orderLog); | ||
| expect(receivedShippingTax).toBeApproximately(calculatedShippingTax, 0.01, orderLog); | ||
| expect(receivedTotalTax).toBeApproximately(calculatedTotalTax, 0.01, orderLog); | ||
| expect(receivedDiscountTotal).toBeApproximately(calculatedDiscount, 0.01, orderLog); | ||
| expect(receivedDiscountTax).toBeApproximately(calculatedDiscountTax, 0.01, orderLog); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify data handling in assertData by using descriptive objects
Using arrays for data storage and access in assertions can lead to confusion and errors due to reliance on index positions. Refactor to use objects with clear property names to improve test reliability and code comprehension.
Implement the refactoring as shown:
async function assertData(orderId: string, calculatedData: any, receivedData: any) {
- const [calculatedSubTotal, calculatedOrderTotal, ...] = calculatedData;
- const [receivedSubtotal, receivedOrderTotal, ...] = receivedData;
+ const {
+ calculatedSubtotal,
+ calculatedOrderTotal,
+ calculatedVendorEarning,
+ // ... other calculated properties
+ } = calculatedData;
+ const {
+ receivedSubtotal,
+ receivedOrderTotal,
+ receivedVendorEarning,
+ // ... other received properties
+ } = receivedData;
// ... perform assertions using property names
}This approach enhances clarity and makes the assertions easier to follow.
Committable suggestion skipped: line range outside the PR's diff.
| async function getReceivedData(orderId: string, parameters: any): Promise<number[]> { | ||
| const [orderTotal, gatewayDetails, shippingFee, shippingTax, productTax, totalTax, lineItems, feeRecipient, , discountTotal, discountTax] = parameters; | ||
| let receivedData = []; | ||
| if (DOKAN_PRO) { | ||
| const orderLog = await apiUtils.getSingleOrderLog(String(orderId)); | ||
| const subTotal = helpers.roundToTwo(orderLog.order_total - orderLog.shipping_total - orderLog.shipping_total_tax - orderLog.tax_total + orderLog.dokan_gateway_fee); | ||
| receivedData = [subTotal, orderLog.order_total, orderLog.vendor_earning, orderLog.commission, orderLog.dokan_gateway_fee, orderLog.shipping_total, orderLog.shipping_total_tax, orderLog.tax_total, totalTax, discountTotal, discountTax].map(Number); | ||
| } else { | ||
| let [adminCommission, vendorEarning] = helpers.lineItemsToCommissionAndEarning(lineItems); | ||
| adminCommission = helpers.commissionOrEarning('commission', adminCommission!, productTax, shippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| vendorEarning = helpers.commissionOrEarning('earning', vendorEarning!, productTax, shippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| const subTotal = helpers.roundToTwo(orderTotal - shippingFee - totalTax + gatewayDetails.gatewayFee); | ||
| receivedData = [subTotal, orderTotal, vendorEarning, adminCommission, gatewayDetails.gatewayFee, shippingFee, shippingTax, productTax, totalTax, discountTotal, discountTax].map(Number); | ||
| } | ||
| return receivedData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor getReceivedData to return an object for clarity
Returning data as an object rather than an array enhances code readability and reduces errors associated with index-based access. This change will make the codebase more maintainable and easier to understand.
Modify the return statement as follows:
async function getReceivedData(orderId: string, parameters: any): Promise<any> {
// ...
- return receivedData;
+ return {
+ subTotal,
+ orderTotal,
+ vendorEarning,
+ adminCommission,
+ gatewayFee: gatewayDetails.gatewayFee,
+ shippingFee,
+ shippingTax,
+ productTax,
+ totalTax,
+ discountTotal,
+ discountTax,
+ };
}Adjust the consuming code to access these values by their property names.
Committable suggestion skipped: line range outside the PR's diff.
| let [adminCommission, vendorEarning] = helpers.lineItemsToCommissionAndEarning(lineItems); | ||
| adminCommission = helpers.commissionOrEarning('commission', adminCommission!, productTax, shippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| vendorEarning = helpers.commissionOrEarning('earning', vendorEarning!, productTax, shippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| const subTotal = helpers.roundToTwo(orderTotal - shippingFee - totalTax + gatewayDetails.gatewayFee); | ||
| receivedData = [subTotal, orderTotal, vendorEarning, adminCommission, gatewayDetails.gatewayFee, shippingFee, shippingTax, productTax, totalTax, discountTotal, discountTax].map(Number); | ||
| } |
There was a problem hiding this comment.
Handle possible undefined values when using non-null assertions
The use of non-null assertion operators (!) assumes that the values are not undefined or null, which can lead to runtime errors if the assumption is incorrect. Ensure that the values returned from helpers.lineItemsToCommissionAndEarning(lineItems) are always defined.
Consider adding checks or handling cases where these values might be undefined:
let [adminCommission, vendorEarning] = helpers.lineItemsToCommissionAndEarning(lineItems);
+ if (adminCommission === undefined || vendorEarning === undefined) {
+ throw new Error('Failed to retrieve commission and earning from line items');
+ }
adminCommission = helpers.commissionOrEarning('commission', adminCommission, productTax, shippingTax, shippingFee, gatewayDetails, feeRecipient);Committable suggestion skipped: line range outside the PR's diff.
| async function getCalculatedData(parameters: any): Promise<number[]> { | ||
| const [, gatewayDetails, shippingFee, , , , lineItems, feeRecipient, taxRate, , , couponLines, applySequentially] = parameters; | ||
|
|
||
| const [calculatedSubtotal, calculatedProductTax, calculatedSubtotalCommission, calculatedSubtotalEarning] = helpers.lineItemsToSalesMetrics(lineItems, taxRate); | ||
| const calculatedShippingTax = helpers.tax(shippingFee, taxRate); | ||
| const calculatedSubtotalWithoutDiscount = helpers.lineItemsToSubtotalWithoutDiscount(lineItems); | ||
| const calculatedDiscount = helpers.calculateDiscount(calculatedSubtotalWithoutDiscount, couponLines, applySequentially); | ||
| const calculatedDiscountTax = helpers.tax(calculatedDiscount, taxRate); | ||
| const calculatedTotalTax = helpers.roundToTwo(calculatedProductTax! + calculatedShippingTax); | ||
| const calculatedOrderTotal = helpers.roundToTwo(calculatedSubtotal! + calculatedTotalTax + shippingFee); | ||
| const calculatedAdminCommission = helpers.commissionOrEarning('commission', calculatedSubtotalCommission, calculatedProductTax, calculatedShippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| const calculatedVendorEarning = helpers.commissionOrEarning('earning', calculatedSubtotalEarning, calculatedProductTax, calculatedShippingTax, shippingFee, gatewayDetails, feeRecipient); | ||
| const calculatedData = [calculatedSubtotal, calculatedOrderTotal, calculatedVendorEarning, calculatedAdminCommission, gatewayDetails.gatewayFee, shippingFee, calculatedShippingTax, calculatedProductTax, calculatedTotalTax, calculatedDiscount, calculatedDiscountTax].map(Number); | ||
| return calculatedData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use objects for parameters and return values in getCalculatedData
Passing and returning arrays with multiple values reduces code clarity and increases the risk of errors due to incorrect index usage. Refactoring to use objects will enhance readability and maintainability.
Apply this diff to adjust the function:
async function getCalculatedData(parameters: any): Promise<any> {
- const [, gatewayDetails, shippingFee, , , , lineItems, feeRecipient, taxRate, , , couponLines, applySequentially] = parameters;
+ const {
+ gatewayDetails,
+ shippingFee,
+ lineItems,
+ feeRecipient,
+ taxRate,
+ couponLines,
+ applySequentially,
+ } = parameters;
// ... perform calculations
- return calculatedData;
+ return {
+ calculatedSubtotal,
+ calculatedOrderTotal,
+ calculatedVendorEarning,
+ calculatedAdminCommission,
+ gatewayFee: gatewayDetails.gatewayFee,
+ shippingFee,
+ calculatedShippingTax,
+ calculatedProductTax,
+ calculatedTotalTax,
+ calculatedDiscount,
+ calculatedDiscountTax,
+ };
}Update any functions that consume the returned data to use property names instead of array indices.
Committable suggestion skipped: line range outside the PR's diff.
| async function getParameters(orderId: string, orderResponseBody: any): Promise<(number | object)[]> { | ||
| const [, singleOrderResponseBody] = await apiUtils.getSingleOrder(orderId); | ||
|
|
||
| const taxRate = Number(orderResponseBody.tax_lines[0]?.rate_percent ?? 0); | ||
| const discountTotal = Number(orderResponseBody.discount_total ?? 0); | ||
| const discountTax = Number(orderResponseBody.discount_tax ?? 0); | ||
| const shippingFee = Number(orderResponseBody.shipping_total ?? 0); | ||
| const shippingTax = Number(orderResponseBody.shipping_tax ?? 0); | ||
| const productTax = Number(orderResponseBody.cart_tax ?? 0); | ||
| const totalTax = Number(orderResponseBody.total_tax ?? 0); | ||
| const orderTotal = Number(orderResponseBody.total); | ||
| const lineItems = orderResponseBody.line_items; | ||
| const couponLines = orderResponseBody.coupon_lines; | ||
|
|
||
| const gatewayDetails = helpers.getGatewayDetails(singleOrderResponseBody); | ||
| const feeRecipient = await apiUtils.getFeeRecipient(singleOrderResponseBody); | ||
|
|
||
| let applySequentially = await apiUtils.getSingleWcSettingOptions('general', 'woocommerce_calc_discounts_sequentially'); | ||
| applySequentially = applySequentially.value === 'yes'; | ||
|
|
||
| return [orderTotal, gatewayDetails, shippingFee, shippingTax, productTax, totalTax, lineItems, feeRecipient, taxRate, discountTotal, discountTax, couponLines, applySequentially]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor getParameters to return an object instead of an array
Returning an array of mixed numbers and objects makes the code hard to understand and prone to index-related errors. Using an object with descriptive property names will improve readability and ease of maintenance.
Apply this diff to modify the return structure:
async function getParameters(orderId: string, orderResponseBody: any): Promise<any> {
// ...
- return [orderTotal, gatewayDetails, shippingFee, shippingTax, productTax, totalTax, lineItems, feeRecipient, taxRate, discountTotal, discountTax, couponLines, applySequentially];
+ return {
+ orderTotal,
+ gatewayDetails,
+ shippingFee,
+ shippingTax,
+ productTax,
+ totalTax,
+ lineItems,
+ feeRecipient,
+ taxRate,
+ discountTotal,
+ discountTax,
+ couponLines,
+ applySequentially,
+ };
}Ensure that subsequent functions that consume parameters are updated to access properties by name.
Committable suggestion skipped: line range outside the PR's diff.
All Submissions:
Changes proposed in this Pull Request:
Related PR
Resources
Videos
How to test the changes in this Pull Request:
deduct discount from.enhancement/commission-restructorebranch check all commissions refund it and check commissions.dokan/v1/commissionChangelog entry
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation