Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decimal and thousand separator for category commission. #2440

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

Aunshon
Copy link
Collaborator

@Aunshon Aunshon commented Nov 18, 2024

All Submissions:

  • My code follow the WordPress' coding standards
  • My code satisfies feature requirements
  • My code is tested
  • My code passes the PHPCS tests
  • My code has proper inline documentation
  • I've included related pull request(s) (optional)
  • I've included developer documentation (optional)
  • I've added proper labels to this pull request

Changes proposed in this Pull Request:

Fixed

Changelog entry

No change log needed.

Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.

Before Changes

Describe the issue before changes with screenshots(s).

After Changes

Describe the issue after changes with screenshot(s).

Feature Video (optional)

Link of detailed video if this PR is for a feature.

PR Self Review Checklist:

  • Code is not following code style guidelines
  • Bad naming: make sure you would understand your code if you read it a few months from now.
  • KISS: Keep it simple, Sweetie (not stupid!).
  • DRY: Don't Repeat Yourself.
  • Code that is not readable: too many nested 'if's are a bad sign.
  • Performance issues
  • Complicated constructions that need refactoring or comments: code should almost always be self-explanatory.
  • Grammar errors.

FOR PR REVIEWER ONLY:

As a reviewer, your feedback should be focused on the idea, not the person. Seek to understand, be respectful, and focus on constructive dialog.

As a contributor, your responsibility is to learn from suggestions and iterate your pull request should it be needed based on feedback. Seek to collaborate and produce the best possible contribution to the greater whole.

  • Correct — Does the change do what it’s supposed to? ie: code 100% fulfilling the requirements?
  • Secure — Would a nefarious party find some way to exploit this change? ie: everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities?
  • Readable — Will your future self be able to understand this change months down the road?
  • Elegant — Does the change fit aesthetically within the overall style and architecture?

Summary by CodeRabbit

  • New Features

    • Improved display of commission values with consistent currency formatting.
    • Enhanced input handling with debouncing for better performance during rapid input changes.
    • Added validation for percentage values to ensure they are within the range of 0 to 100.
    • User input for admin commission now includes min and max restrictions directly in the form.
    • Robust initialization and validation of commission-related properties.
    • New setting introduced to delete all Dokan plugin data upon uninstallation.
    • Enhanced validation logic for required fields in settings management, including new commission types.
    • Expanded commission update logic to incorporate new commission types (Flat and Percentage).
  • Bug Fixes

    • Refined logic for validating and unformatting input values, especially for percentage inputs.
    • Enhanced error handling in settings management for withdrawal limits and commission structures.
    • Adjusted layout and styling for improved responsiveness on the settings page.
    • Minor corrections in property names and removal of unnecessary console logs for cleaner debugging.
  • Chores

    • Updated comments and documentation to clarify new logic in commission calculations.

@Aunshon Aunshon self-assigned this Nov 18, 2024
Copy link
Contributor

coderabbitai bot commented Nov 18, 2024

Walkthrough

The changes in CategoryBasedCommission.vue enhance commission value handling through new formatting and validation functions. The :value bindings for commission inputs now utilize the formatValue method for consistent display. Debouncing is implemented for commissinItemHandler and handleAllCategoryInput methods to optimize performance during rapid inputs. New utility methods for value conversion and validation are introduced, and the initialization logic for commission data in AdminCommission.vue is improved to ensure proper handling of commission-related properties. Additional updates to validation and error handling are made across various components and settings.

Changes

File Path Change Summary
src/admin/components/Commission/CategoryBasedCommission.vue - Added methods: formatValue(value), unFormatValue(value), validatePercentage(percentage)
- Updated method signatures for commissinItemHandler and handleAllCategoryInput to include debouncing.
- Enhanced validation and unformatting logic for input values, particularly for percentages.
includes/Product/Hooks.php - Updated save_per_product_commission_options method to validate commission range.
- Updated add_per_product_commission_options method to include min and max attributes for input fields.
assets/src/js/setup-wizard/commission/AdminCommission.vue - Improved initialization of this.commission during the created lifecycle hook to ensure correct handling of commission data.
- Updated fixedCOmmissionhandler method to handle empty strings for commission values.
includes/Admin/Settings.php - Updated commission_category_based_values field to include required attribute.
- Enhanced validation in set_withdraw_limit_value_validation to prevent negative limits.
- Added add_dokan_data_clear_setting method for data management on uninstallation.
src/admin/components/CombineInput.vue - Removed console log statement and corrected property name fixexName.
src/admin/pages/Settings.vue - Expanded validation logic for required fields and commission types, particularly for 'combine' and 'fixed'.
- Adjusted layout and styling for improved responsiveness.
src/admin/pages/VendorSingle.vue - Adjusted handling of admin_commission_type in transformer method, removing default 'flat' assignment.
- Removed formatting for admin_additional_fee and admin_commission properties.
- Updated sendEmail and fetch methods for improved logic.
src/admin/components/Fields.vue - Updated template structure with new classes for error messages, enhancing layout and spacing.
includes/Commission.php - Deprecated multiple methods related to commission calculations.
- Updated get_commission method to include additional logic for handling empty $total_amount and $order_item_id.
includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php - Expanded commission handling to include new commission types (Flat, Percentage) in the task and update_global_settings methods.
includes/Upgrade/Upgrades/V_3_14_0.php - Enhanced update_global_commission_type method to accommodate new commission types with updated logic for handling Flat and Percentage.

Possibly related PRs

Suggested labels

QA approved, :+1: Dev Review Done, Needs: Testing

Suggested reviewers

  • shohag121
  • mrabbani

Poem

🐰 In the fields where numbers play,
Formatting values, bright as day!
Debouncing inputs, swift and neat,
Commission handling, can't be beat!
With methods new, we hop along,
Making sure our values are never wrong! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/admin/components/Commission/CategoryBasedCommission.vue (2)

258-258: Fix typo in method name

The method name has a typo: 'commissinItemHandler' should be 'commissionItemHandler'

-            commissinItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {
+            commissionItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {

367-393: LGTM: Well-structured utility methods with proper validation

The utility methods are well-implemented with appropriate handling of edge cases. However, consider adding try-catch blocks around the accounting.js calls to handle potential formatting errors gracefully.

             formatValue: ( value ) => {
                 if ( value === '' ) {
                     return value;
                 }
-
-                return accounting.formatNumber( value, dokan.currency.precision, dokan.currency.thousand, dokan.currency.decimal );
+                try {
+                    return accounting.formatNumber( value, dokan.currency.precision, dokan.currency.thousand, dokan.currency.decimal );
+                } catch (error) {
+                    console.error('Error formatting value:', error);
+                    return value;
+                }
             },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 329da76 and cce9ddc.

📒 Files selected for processing (1)
  • src/admin/components/Commission/CategoryBasedCommission.vue (8 hunks)
🔇 Additional comments (3)
src/admin/components/Commission/CategoryBasedCommission.vue (3)

35-35: LGTM: Consistent value formatting implementation

The value formatting has been consistently implemented across all commission input fields, ensuring uniform display of decimal and thousand separators.

Also applies to: 52-52, 82-82, 99-100


111-112: LGTM: Added debounce for performance optimization

The addition of debouncing will help optimize performance by reducing the frequency of commission value updates during rapid input changes.


Line range hint 258-285: LGTM: Input handlers properly handle value formatting and validation

The implementation correctly:

  • Validates percentage values (0-100 range)
  • Unformats values before processing
  • Uses debouncing to optimize performance

Also applies to: 287-297

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
includes/Product/Hooks.php (1)

494-494: Consider adding the step attribute for decimal precision control.

The min/max validation is good. Consider adding step="0.01" to allow for precise decimal commission values while maintaining the HTML5 validation.

-                    <input id="admin_commission" class="input-text wc_input_price" min="0" max="100" type="text" name="_per_product_admin_commission" value="<?php echo wc_format_localized_price( $admin_commission ); ?>">
+                    <input id="admin_commission" class="input-text wc_input_price" min="0" max="100" step="0.01" type="text" name="_per_product_admin_commission" value="<?php echo wc_format_localized_price( $admin_commission ); ?>">
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cce9ddc and 7fd3311.

📒 Files selected for processing (1)
  • includes/Product/Hooks.php (2 hunks)

@@ -539,7 +539,7 @@
update_post_meta( $post_id, '_per_product_admin_commission_type', $commission_type );
}

if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore
if ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve commission validation logic.

The current validation is embedded within the condition making it less readable. Consider separating the validation logic and adding numeric validation.

-        if ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore
-            $admin_commission = ( '' === $_POST['_per_product_admin_commission'] ) ? '' : sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
+        if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore
+            $commission = sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
+            
+            if ( '' === $commission ) {
+                $admin_commission = '';
+            } elseif ( is_numeric( $commission ) && $commission >= 0 && $commission <= 100 ) {
+                $admin_commission = $commission;
+            }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ( isset( $_POST['_per_product_admin_commission'] ) && 0 <= $_POST['_per_product_admin_commission'] && 100 >= $_POST['_per_product_admin_commission'] ) { // phpcs:ignore
if ( isset( $_POST['_per_product_admin_commission'] ) ) { // phpcs:ignore
$commission = sanitize_text_field( $_POST['_per_product_admin_commission'] ); // phpcs:ignore
if ( '' === $commission ) {
$admin_commission = '';
} elseif ( is_numeric( $commission ) && $commission >= 0 && $commission <= 100 ) {
$admin_commission = $commission;
}

@shashwatahalder01

This comment was marked as resolved.

@shashwatahalder01

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/admin/components/Commission/CategoryBasedCommission.vue (1)

111-111: Fix typo in method name and approve debouncing implementation

The debouncing implementation looks good and will help optimize performance during rapid inputs. However, there's a typo in the method name.

-    commissinItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {
+    commissionItemHandler: Debounce( function( value, commission_type, term_id, oldValue = '' ) {

Also applies to: 260-287, 289-299

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ccd8380 and 69b9aee.

📒 Files selected for processing (1)
  • src/admin/components/Commission/CategoryBasedCommission.vue (10 hunks)
🔇 Additional comments (4)
src/admin/components/Commission/CategoryBasedCommission.vue (4)

35-35: LGTM: Consistent value formatting across all commission inputs

The value bindings now properly utilize the formatValue method, ensuring consistent decimal and thousand separator formatting across percentage and flat commission inputs.

Also applies to: 52-52, 82-82, 99-99


261-266: LGTM: Proper validation of percentage values

The handlers now correctly validate percentage values within the 0-100 range and properly handle empty values, which should resolve the reported issue with percentage values greater than 10 not being stored.

Also applies to: 290-293


164-167: LGTM: Proper initialization of commission items

The initialization logic now ensures that commission.items is always an object, which should prevent the reported issue of null values being stored during setup.


369-395: LGTM: Well-implemented formatting and validation utilities

The new utility methods properly handle number formatting and validation while gracefully handling edge cases.

Let's verify the currency settings are properly initialized:

@StalinDurjo

This comment was marked as resolved.

@StalinDurjo

This comment was marked as resolved.

@StalinDurjo

This comment was marked as resolved.

@shashwatahalder01

This comment was marked as resolved.

@Aunshon

This comment was marked as resolved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/admin/pages/Settings.vue (1)

1122-1122: Consider using CSS Grid or Flexbox for better responsiveness

The current approach uses fixed widths and media queries. Consider using modern CSS layout techniques:

-width: 100%;
+display: grid;
+grid-template-columns: minmax(0, 1fr);

This would provide better flexibility and maintainability across different screen sizes.

Also applies to: 1155-1155, 1171-1171

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 69b9aee and 85c3dec.

📒 Files selected for processing (4)
  • includes/Admin/Settings.php (1 hunks)
  • src/admin/components/CombineInput.vue (0 hunks)
  • src/admin/components/Commission/CategoryBasedCommission.vue (11 hunks)
  • src/admin/pages/Settings.vue (4 hunks)
💤 Files with no reviewable changes (1)
  • src/admin/components/CombineInput.vue
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/admin/components/Commission/CategoryBasedCommission.vue
🔇 Additional comments (3)
src/admin/pages/Settings.vue (2)

436-438: LGTM: Commission type validation enhancement

The code correctly removes the category-based commission validation error when the commission type is flat, percentage, combine, or fixed.


447-465: Verify the category-based commission validation logic

The validation for category-based commission values is complex and might have edge cases. Please ensure:

  1. All commission types are properly handled
  2. The validation works correctly when switching between commission types
  3. Error states are properly cleared when switching commission types
✅ Verification successful

Based on the search results, I can now provide a comprehensive verification of the category-based commission validation logic. The code shows:

  1. Extensive test coverage for all commission types and scenarios in tests/pw/tests/api/calculation.spec.ts
  2. Proper validation handling in src/admin/pages/Settings.vue that checks:
    • Commission type switching
    • All required fields for category-based commission
    • Both percentage and flat rate values
  3. Clear error state management through arrayRemove and push operations
  4. Proper data structure validation ensuring commission_category_based_values has the correct shape

Category-based commission validation is properly implemented

The validation logic correctly:

  • Handles switching between commission types by clearing irrelevant errors
  • Validates both flat and percentage values for category-based commissions
  • Maintains proper error states through array operations
  • Ensures the data structure has required fields (all, flat, percentage)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential edge cases in commission type handling

# Search for commission type related code
rg -A 5 "commission_type.*category_based"

# Search for commission validation related code
rg -A 10 "commission_category_based_values"

Length of output: 63476

includes/Admin/Settings.php (1)

541-547: LGTM! Field configuration aligns with commission handling requirements.

The changes to the commission_category_based_values field configuration look good:

  • Added required validation
  • Proper conditional display based on commission type
  • Integration with pro features via dokan_pro_commission flag

Let's verify the commission type dependency:

✅ Verification successful

Let me generate additional scripts to verify the commission type dependency more thoroughly:


Based on the verification results, I can now provide the final response:

Commission type dependency is properly implemented and validated

The changes to the commission_category_based_values field configuration are correctly implemented with proper dependencies:

  • The field is only shown when commission_type is set to 'category_based'
  • Proper sanitization and validation are in place for commission values
  • The commission type relationship is consistently handled across the codebase
  • Category-based commission values are properly stored and processed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify commission type handling and related fields

# Check for commission type references and validations
rg -A 5 "commission_type.*category_based"

# Look for any commission value formatting/validation logic
ast-grep --pattern 'function [^{]*commission[^{]*\{'

Length of output: 16308


Script:

#!/bin/bash
# Check for commission type field definition and its relationship with category commission
rg -A 10 "commission_type.*=.*\[" includes/

# Look for any validation or sanitization logic related to commission values
rg -A 5 "sanitize.*commission|validate.*commission" includes/

# Search for any commission type conditional logic
ast-grep --pattern 'if ($_ === "category_based") { $$$  }'

Length of output: 17916

@jamil-mahmud

This comment was marked as resolved.

@shashwatahalder01

This comment was marked as resolved.

@shashwatahalder01

This comment was marked as resolved.

@StalinDurjo

This comment was marked as resolved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
includes/Commission.php (2)

585-592: Consider improving the documentation for future maintainability.

While the documentation explains the edge case well, referencing a specific GitHub comment URL might make the documentation brittle if the URL becomes inaccessible. Consider including the key points from the GitHub comment directly in the documentation.

-         * @see https://github.com/getdokan/dokan/pull/2440#issuecomment-2488159960
+         * This handles edge cases such as:
+         * 1. When a 100% coupon is applied, making the total_amount zero
+         * 2. When calculating commission for a new product without an order

Line range hint 593-597: Add validation for product price.

The current implementation could be improved by adding explicit validation for the product price. Consider handling edge cases like negative prices or invalid product objects.

         if ( ! empty( $product_id ) && empty( $total_amount ) && empty( $order_item_id ) ) {
             $product = dokan()->product->get( $product_id );
 
-            // If product price is empty the setting the price as 0
-            $total_amount = $product && $product->get_price() && ! empty( $product->get_price() ) ? $product->get_price() : 0;
+            // Validate product and ensure price is non-negative
+            $price = $product && $product->get_price() ? $product->get_price() : 0;
+            $total_amount = max( 0, (float) $price );
         }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 00bda24 and dcb8376.

📒 Files selected for processing (1)
  • includes/Commission.php (1 hunks)

@shashwatahalder01

This comment was marked as off-topic.

@shashwatahalder01

This comment was marked as resolved.

@Aunshon
Copy link
Collaborator Author

Aunshon commented Nov 22, 2024

#2440 (comment)

@shashwatahalder01 vai this issue is already exists. this is not related for commission restructure.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dcb8376 and f26fd56.

📒 Files selected for processing (2)
  • includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php (4 hunks)
  • includes/Upgrade/Upgrades/V_3_14_0.php (2 hunks)
🔇 Additional comments (2)
includes/Upgrade/Upgrades/V_3_14_0.php (2)

7-8: LGTM: New commission formula imports

The imports for Flat and Percentage commission types are properly organized with existing commission formula imports.


21-35: Verify upgrade process safety and impact

This upgrade affects critical commission calculations. Please ensure:

  1. The upgrade process is atomic (all or nothing)
  2. There's a rollback mechanism in case of failure
  3. Existing commission calculations aren't adversely affected
✅ Verification successful

Let me gather more specific information about the upgrade process and rollback mechanisms.


Let me check one more aspect about the background process implementation and error handling.


Based on the gathered information, I can now provide a final response:

Commission upgrade process appears to be safely implemented

The upgrade process implementation shows several safety mechanisms:

  1. Uses WordPress background processing framework for handling large datasets
  2. Implements atomic operations per entity (global settings, vendor settings, product settings)
  3. Has proper status tracking through WordPress options
  4. Processes data in batches to prevent timeouts
  5. Each task returns boolean status for tracking success/failure

The commission type migration is handled systematically:

  • Global settings are updated with proper value preservation
  • Vendor commission settings are migrated individually
  • Product-specific commissions are processed separately
  • All calculations maintain numerical precision using WooCommerce's decimal handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential issues in commission calculations across the codebase

# Look for commission calculation references
echo "Checking commission calculation references..."
rg -l "admin_percentage|additional_fee|commission_type" --type php

# Check for existing error handling in commission calculations
echo "Checking error handling in commission calculations..."
ast-grep --pattern 'try { $$$ } catch' 

# Look for number format handling in commission calculations
echo "Checking number format handling..."
rg "thousands_sep|decimal_point|number_format" --type php

Length of output: 7229


Script:

#!/bin/bash
# Check upgrade and rollback mechanisms

# Look for rollback mechanisms in the upgrade process
echo "Checking rollback mechanisms..."
rg -A 5 "rollback|revert" includes/Upgrade/

# Check atomic transaction handling in the upgrade
echo "Checking atomic transaction handling..."
rg -A 5 "begin_transaction|commit|rollback" includes/Upgrade/

# Check background process handling for V_3_14_0
echo "Checking background process implementation..."
cat includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php

# Check error handling in the upgrade process
echo "Checking upgrade error handling..."
rg -A 5 "try|catch|Exception" includes/Upgrade/Upgrades/V_3_14_0.php

Length of output: 5962


Script:

#!/bin/bash
# Check background process base class and error handling

# Check the background process abstract class
echo "Checking background process base implementation..."
cat includes/Abstracts/DokanBackgroundProcesses.php

# Check for error logging mechanisms
echo "Checking error logging mechanisms..."
rg -A 5 "dokan_log|error_log|WC_Logger" includes/Upgrade/

# Check if there are any upgrade status tracking mechanisms
echo "Checking upgrade status tracking..."
rg -A 5 "update_option.*upgrade|get_option.*upgrade" includes/Upgrade/

Length of output: 5344

Comment on lines +21 to +24
$options = get_option( 'dokan_selling', [] );

$commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE;
$admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for commission values

The retrieved values from options should be validated and sanitized before use. Consider adding type casting and validation for admin_percentage.

- $admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0;
+ $admin_percentage = isset( $options['admin_percentage'] ) ? floatval( $options['admin_percentage'] ) : 0;
+ if ( $admin_percentage < 0 ) {
+     $admin_percentage = 0;
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$options = get_option( 'dokan_selling', [] );
$commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE;
$admin_percentage = isset( $options['admin_percentage'] ) ? $options['admin_percentage'] : 0;
$options = get_option( 'dokan_selling', [] );
$commission_type = isset( $options['commission_type'] ) ? $options['commission_type'] : Fixed::SOURCE;
$admin_percentage = isset( $options['admin_percentage'] ) ? floatval( $options['admin_percentage'] ) : 0;
if ( $admin_percentage < 0 ) {
$admin_percentage = 0;
}

Comment on lines +29 to +35
if ( Flat::SOURCE === $commission_type ) {
$options['admin_percentage'] = 0;
$options['additional_fee'] = $admin_percentage;
} elseif ( Percentage::SOURCE === $commission_type ) {
$options['admin_percentage'] = $admin_percentage;
$options['additional_fee'] = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and number format conversion

The commission value updates need proper error handling and should consider number format conversion given the PR's objective to fix decimal/thousand separator issues.

 if ( Flat::SOURCE === $commission_type ) {
+    try {
+        // Convert any localized number format to standard format
+        $admin_percentage = str_replace( 
+            [get_option('thousands_sep'), get_option('decimal_point')],
+            ['', '.'],
+            $admin_percentage
+        );
         $options['admin_percentage'] = 0;
         $options['additional_fee'] = $admin_percentage;
+    } catch ( Exception $e ) {
+        error_log( 'Dokan commission migration error: ' . $e->getMessage() );
+        return false;
+    }
 } elseif ( Percentage::SOURCE === $commission_type ) {
+    try {
         $options['admin_percentage'] = $admin_percentage;
         $options['additional_fee'] = 0;
+    } catch ( Exception $e ) {
+        error_log( 'Dokan commission migration error: ' . $e->getMessage() );
+        return false;
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ( Flat::SOURCE === $commission_type ) {
$options['admin_percentage'] = 0;
$options['additional_fee'] = $admin_percentage;
} elseif ( Percentage::SOURCE === $commission_type ) {
$options['admin_percentage'] = $admin_percentage;
$options['additional_fee'] = 0;
}
if ( Flat::SOURCE === $commission_type ) {
try {
// Convert any localized number format to standard format
$admin_percentage = str_replace(
[get_option('thousands_sep'), get_option('decimal_sep')],
['', '.'],
$admin_percentage
);
if (!is_numeric($admin_percentage)) {
throw new Exception('Invalid number format for admin percentage');
}
$options['admin_percentage'] = 0;
$options['additional_fee'] = (float) $admin_percentage;
} catch ( Exception $e ) {
error_log( 'Dokan commission migration error: ' . $e->getMessage() );
return false;
}
} elseif ( Percentage::SOURCE === $commission_type ) {
try {
// Convert any localized number format to standard format
$admin_percentage = str_replace(
[get_option('thousands_sep'), get_option('decimal_sep')],
['', '.'],
$admin_percentage
);
if (!is_numeric($admin_percentage)) {
throw new Exception('Invalid number format for admin percentage');
}
$options['admin_percentage'] = (float) $admin_percentage;
$options['additional_fee'] = 0;
} catch ( Exception $e ) {
error_log( 'Dokan commission migration error: ' . $e->getMessage() );
return false;
}
}

Comment on lines +66 to +79
$category_commission_item = [
'flat' => $admin_additional_fee,
'percentage' => $commission,
];

if ( Flat::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = 0;
$category_commission_item['flat'] = $commission;
} elseif ( Percentage::SOURCE === $commission_type ) {
$category_commission_item['percentage'] = $commission;
$category_commission_item['flat'] = 0;
}

$category_commission['items'][ $term->term_id ] = $category_commission_item;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential Logical Inconsistency in Commission Assignments

In the update_global_settings method, there seems to be a logical inconsistency when assigning commission values based on the commission type. Initially, $category_commission_item is defined as:

$category_commission_item = [
    'flat'       => $admin_additional_fee,
    'percentage' => $commission,
];

However, within the conditional blocks:

if ( Flat::SOURCE === $commission_type ) {
    $category_commission_item['percentage'] = 0;
    $category_commission_item['flat'] = $commission;
} elseif ( Percentage::SOURCE === $commission_type ) {
    $category_commission_item['percentage'] = $commission;
    $category_commission_item['flat'] = 0;
}

In the Flat case, you set 'flat' to $commission, which might actually represent a percentage value, and in the initial definition, 'flat' was set to $admin_additional_fee. This could lead to incorrect commission calculations.

Suggested Fix:

Ensure that the correct variables are assigned to 'flat' and 'percentage' based on the commission type. Here’s the corrected assignment:

if ( Flat::SOURCE === $commission_type ) {
    $category_commission_item['percentage'] = 0;
-   $category_commission_item['flat'] = $commission;
+   $category_commission_item['flat'] = $admin_additional_fee;
} elseif ( Percentage::SOURCE === $commission_type ) {
    $category_commission_item['percentage'] = $commission;
    $category_commission_item['flat'] = 0;
}

This change ensures that 'flat' uses $admin_additional_fee when the commission type is Flat.

Comment on lines 108 to 114
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
$flat = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect Commission Value Assignments in update_vendors_settings

In the update_vendors_settings method, there is a logical error when adjusting commission values based on the old commission type:

if ( Flat::SOURCE === $commission_type_old ) {
    $percentage = 0;
    $flat       = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
    $flat = 0;
}

Setting $flat = $percentage; after $percentage = 0; results in $flat being zero. This likely nullifies the intended flat commission value.

Suggested Fix:

Assign the correct commission value to $flat or $percentage based on the old commission type:

if ( Flat::SOURCE === $commission_type_old ) {
    $percentage = 0;
-   $flat       = $percentage;
+   $flat       = $commission->get_flat();
} elseif ( Percentage::SOURCE === $commission_type_old ) {
    $flat       = 0;
+   $percentage = $commission->get_percentage();
}

This ensures that the original commission values are correctly assigned.

Comment on lines 148 to 154
if ( Flat::SOURCE === $commission_type_old ) {
$percentage = 0;
$flat = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$flat = 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Same Issue in update_products_settings Method

The update_products_settings method contains a similar logical error:

if ( Flat::SOURCE === $commission_type_old ) {
    $percentage = 0;
    $flat       = $percentage;
} elseif ( Percentage::SOURCE === $commission_type_old ) {
    $flat = 0;
}

Again, setting $flat = $percentage; after $percentage = 0; results in both values being zero, which may not be intended.

Suggested Fix:

Apply the same correction to ensure accurate commission value assignments:

if ( Flat::SOURCE === $commission_type_old ) {
    $percentage = 0;
-   $flat       = $percentage;
+   $flat       = $commission->get_flat();
} elseif ( Percentage::SOURCE === $commission_type_old ) {
    $flat       = 0;
+   $percentage = $commission->get_percentage();
}

This correction maintains the integrity of the commission settings based on the old commission type.

@shashwatahalder01
Copy link
Contributor

Scenario:
The flat commission feature doesn't function correctly with the new implementation for vendor & product-specific settings.
previous issue
After running the upgrader, flat commission is saved as a percentage value in fixed commission for product commission.
the flat commission is not saved for vendor commission.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f26fd56 and 27fa0e1.

📒 Files selected for processing (1)
  • includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php (4 hunks)
🔇 Additional comments (4)
includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php (4)

7-8: LGTM: New imports align with existing patterns

The addition of Flat and Percentage commission formula imports is consistent with the existing codebase structure.


102-113: LGTM: Vendor commission type conversion logic is sound

The implementation correctly handles the conversion between commission types:

  • Preserves original values
  • Sets appropriate zero values based on commission type
  • Properly migrates between Flat and Percentage types

66-79: ⚠️ Potential issue

Critical: Commission Value Assignment Inconsistency

There's a logical error in the commission value assignments that could lead to incorrect calculations:

  1. Initially, $category_commission_item['flat'] is set to $admin_additional_fee
  2. Then for Flat::SOURCE, it's overwritten with $commission

This could result in losing the correct flat fee value.

Apply this fix:

 $category_commission_item = [
     'flat'       => $admin_additional_fee,
     'percentage' => $commission,
 ];

 if ( Flat::SOURCE === $commission_type ) {
     $category_commission_item['percentage'] = 0;
-    $category_commission_item['flat'] = $commission;
+    $category_commission_item['flat'] = $admin_additional_fee;
 } elseif ( Percentage::SOURCE === $commission_type ) {
     $category_commission_item['percentage'] = $commission;
     $category_commission_item['flat'] = 0;
 }

Likely invalid or redundant comment.


Line range hint 1-161: Verify the impact on existing commission data

Given the critical nature of commission calculations and the identified issues, we should verify the upgrade process thoroughly.

✅ Verification successful

Let me gather more information about the commission-related code to better understand the impact.


Let me gather more specific information about the commission upgrade process.


Let me check one more specific aspect about the commission type changes.


Based on the analysis of the code, I can now provide a definitive response about the commission type changes:

Commission type migration is properly handled and safe

The upgrade script correctly handles the commission type migration by:

  1. Preserving all commission values while changing the type to Fixed:

    • For flat commissions: Sets percentage to 0 and moves the flat amount correctly
    • For percentage commissions: Keeps the percentage and sets flat to 0
  2. Properly migrating commission data at all levels:

    • Global settings
    • Category-based commissions
    • Vendor-specific commissions
    • Product-specific commissions
  3. Maintaining data integrity through:

    • Proper validation before changes
    • Atomic updates using WordPress options API
    • Background processing for large datasets

The changes are backward compatible and won't affect existing commission calculations as the script properly maps old commission types to the new format while preserving the actual commission values.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for existing commission data patterns to understand potential impact

# Check for terms with flat commission type
echo "Terms with flat commission type:"
rg -l "per_category_admin_commission_type.*flat" 

# Check for terms with percentage commission type
echo "Terms with percentage commission type:"
rg -l "per_category_admin_commission_type.*percentage"

# Look for potential commission calculation implementations
echo "Commission calculation implementations:"
ast-grep --pattern 'function $_($$commission) {
  $$$
  return $$result;
}'

Length of output: 436


Script:

#!/bin/bash
# Let's check for commission-related files and their implementations

# Find commission-related files
echo "Commission-related files:"
fd -e php commission -i

# Check commission calculation patterns
echo -e "\nCommission calculation patterns:"
rg -i "calculate.*commission" -A 5

# Check commission type usage
echo -e "\nCommission type usage:"
rg -i "commission.*type" -A 2

# Check commission-related database operations
echo -e "\nCommission database operations:"
rg -i "(get|update|save)_commission" -A 2

Length of output: 146227


Script:

#!/bin/bash
# Check for commission type changes and their impact
echo "Commission type changes:"
rg -A 5 "commission_type.*=.*Fixed::SOURCE" 

# Check for commission calculation changes
echo -e "\nCommission calculation changes:"
rg -A 5 "calculate_commission"

# Check for commission data migrations
echo -e "\nCommission data migrations:"
rg -A 5 "update.*commission.*settings"

# Check for commission-related tests
echo -e "\nCommission-related tests:"
fd -e php test.*commission -x cat {}

Length of output: 119651


Script:

#!/bin/bash
# Check for commission type changes in the upgrade script
echo "Commission type changes in upgrade script:"
rg -A 5 "commission_type.*=.*Fixed::SOURCE" includes/Upgrade/Upgrades/BackgroundProcesses/V_3_14_0_UpdateCommissions.php

# Check for commission type usage in other files
echo -e "\nCommission type usage in other files:"
rg -A 5 "commission_type" --type php --glob '!*test*' --glob '!*upgrade*'

# Check for commission calculation methods
echo -e "\nCommission calculation methods:"
rg -A 5 "calculate_commission" --type php --glob '!*test*'

Length of output: 38003

Comment on lines +144 to +149
if ( Flat::SOURCE === $commission_type_old ) {
$commission->set_flat( $commission->get_percentage() );
$commission->set_percentage( 0 );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_flat( 0 );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Incorrect Value Assignment in Product Commission Update

The current implementation incorrectly uses the percentage value as the flat fee when converting from Flat type. This could lead to incorrect commission calculations.

Apply this fix:

 if ( Flat::SOURCE === $commission_type_old ) {
-    $commission->set_flat( $commission->get_percentage() );
+    $commission->set_flat( $commission->get_flat() );
     $commission->set_percentage( 0 );
 } elseif ( Percentage::SOURCE === $commission_type_old ) {
     $commission->set_flat( 0 );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ( Flat::SOURCE === $commission_type_old ) {
$commission->set_flat( $commission->get_percentage() );
$commission->set_percentage( 0 );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_flat( 0 );
}
if ( Flat::SOURCE === $commission_type_old ) {
$commission->set_flat( $commission->get_flat() );
$commission->set_percentage( 0 );
} elseif ( Percentage::SOURCE === $commission_type_old ) {
$commission->set_flat( 0 );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants