Fix commission upgrader

[Aunshon]

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

Closes

• Closes

How to test the changes in this Pull Request:

• Steps or issue link

Changelog entry

Title

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

Before Changes

Describe the issue before changes with screenshots(s).

After Changes

Describe the issue after changes with screenshot(s).

Feature Video (optional)

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

PR Self Review Checklist:

• 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

  • Updated handling of commission values for categories, products, and vendors to improve data representation.
  • Transitioned commission values from numeric to string formats for better clarity in commission types.
  • Enhanced admin notice to emphasize the importance of backing up the database before upgrades.

• Bug Fixes

  • Resolved issues with commission value assignments, ensuring empty strings are used instead of numeric zeros for specific commission types.

[coderabbitai]

Walkthrough

The pull request introduces changes to three classes within the WeDevs\Dokan\Commission\Upugrader namespace: Update_Category_Commission, Update_Product_Commission, and Update_Vendor_Commission. The modifications primarily focus on the process_single_* methods, where the handling of commission values is adjusted. Specifically, the assignment of percentage and flat values is altered to use empty strings ('') instead of 0 for certain commission types, transitioning from numeric to string representations for these values. Additionally, the AdminNotice class is updated to include a cautionary note about backing up the database before upgrades.

Changes

File	Change Summary
includes/Commission/Upugrader/Update_Category_Commission.php, includes/Commission/Upugrader/Update_Product_Commission.php, includes/Commission/Upugrader/Update_Vendor_Commission.php	Modified process_single_category, process_single_product, and process_single_vendor methods to handle commission values by setting percentage and flat to '' instead of 0 for specific commission types.
includes/Upgrade/AdminNotice.php	Updated the show_notice method to enhance the description, emphasizing the importance of backing up the database before running upgrades.

Possibly related PRs

• Fix/revenue reports #2423: The changes in this PR also involve handling commission types and modifying how commission values are stored, specifically transitioning from numeric values to strings, which aligns with the changes made in the Update_Category_Commission class.
• Fix admin fixed commission setting validation #2437: This PR focuses on validating commission percentage settings, which is related to the handling of commission values in the Update_Category_Commission class, particularly in ensuring that valid data types are used.
• Fix decimal and thousand separator for category commission. #2440: This PR enhances the handling of commission values, including validation and formatting, which is directly related to the changes made in the Update_Category_Commission class regarding how commission values are processed and stored.
• Enhance : Admin Notice Show gloablly & extand API #2446: The modifications in this PR include enhancements to admin notices, which may relate to how commission updates are communicated to users, especially if those updates involve changes to commission handling as seen in the main PR.
• Add notice for version compatibility. #2450: This PR introduces a notice for version compatibility, which could be relevant in the context of ensuring that users are aware of changes in commission handling and related functionalities introduced in the main PR.

Suggested labels

QA approved, :+1: Dev Review Done

Suggested reviewers

• mrabbani

🐰 In the land of code where changes bloom,
Commissions shift, dispelling the gloom.
From zeros to strings, a new way to play,
For vendors and products, a brighter day!
With careful updates, we hop and we cheer,
For clearer values, our path is now clear! 🐇✨

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 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.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)

includes/Commission/Upugrader/Update_Vendor_Commission.php (1)

146-150: Consider adding data validation before save

While the error handling is good, consider adding validation before saving to ensure data integrity:

1. Validate that empty string values are acceptable
2. Consider logging the before/after values for audit purposes

 if ( Flat::SOURCE === $commission_type_old ) {
+    // Log old values for audit
+    $this->log_commission_change($vendor_id, 'flat', $percentage, '');
     $commission->set_percentage( '' );
     $commission->set_flat( $percentage );
 } elseif ( Percentage::SOURCE === $commission_type_old ) {
     $commission->set_percentage( $percentage );
+    // Log old values for audit
+    $this->log_commission_change($vendor_id, 'percentage', $commission->get_flat(), '');
     $commission->set_flat( '' );
 }

includes/Commission/Upugrader/Update_Category_Commission.php (2)

Line range hint 204-207: Add validation before updating WordPress option

The direct update of WordPress options without validation is risky:

+// Validate commission values before update
+if (!$this->validate_commission_values($category_commission)) {
+    error_log("Invalid commission values for term_id: $term_id");
+    return;
+}
 $dokan_selling['commission_category_based_values'] = $category_commission;
-update_option( 'dokan_selling', $dokan_selling );
+$updated = update_option( 'dokan_selling', $dokan_selling );
+if (!$updated) {
+    error_log("Failed to update dokan_selling option for term_id: $term_id");
+}

---

Line range hint 1-1: Consider implementing a shared Commission Value Handler

The commission value changes are implemented similarly across products, vendors, and categories. Consider creating a shared service to handle commission value conversions:

1. Implement consistent validation
2. Centralize error handling
3. Provide audit logging
4. Ensure type safety

Example implementation structure:

namespace WeDevs\Dokan\Commission\Handler;

class CommissionValueHandler {
    public function convertToFixed($oldType, $percentage, $flat) {
        // Shared conversion logic
    }
    
    public function validate($commission) {
        // Shared validation
    }
    
    public function log($entity, $oldValues, $newValues) {
        // Shared logging
    }
}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 025eb4a and 7bb6ad8.

📒 Files selected for processing (3)

• includes/Commission/Upugrader/Update_Category_Commission.php (1 hunks)
• includes/Commission/Upugrader/Update_Product_Commission.php (1 hunks)
• includes/Commission/Upugrader/Update_Vendor_Commission.php (1 hunks)

🔇 Additional comments (1)

includes/Commission/Upugrader/Update_Product_Commission.php (1)

190-198: Verify data type handling in database and UI layers

The change from numeric 0 to empty string '' for commission values needs careful consideration:

1. Ensure database schema accepts empty strings for these fields
2. Verify UI components handle empty string values correctly
3. Confirm that downstream calculations handle empty strings appropriately

✅ Verification successful

Let me analyze the results and generate a final response.

The search results show that empty strings are widely used and handled properly throughout the codebase:

1. Database layer: No direct database schema found, but the code shows consistent use of empty strings as valid values in commission settings and calculations.

2. UI components handle empty strings:

   • CategoryBasedCommission.vue, VendorCommissionFields.vue, and other components explicitly handle empty string values
   • Form validation and formatting functions properly process empty strings
   • UI components use proper type coercion and validation

3. Calculations:

   • Commission calculation code in Commission/ directory shows empty strings are valid and handled
   • Test files demonstrate empty strings are expected values in commission calculations
   • Type conversion and validation functions handle empty string cases

Empty string handling is consistent and safe

The codebase shows that empty strings are the standard way to represent unset commission values, with proper handling across database, UI and calculation layers.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check database schema and related code for commission value handling
echo "Checking database schema..."
rg -A 5 "CREATE TABLE.*dokan.*commission" 

echo "Checking value handling in calculations..."
rg -A 5 "(percentage|flat).*=.*''" 

echo "Checking UI value handling..."
rg -A 5 "render.*commission.*(percentage|flat)"

Length of output: 112214

[coderabbitai]

⚠️ Potential issue

Add error handling and validation for option updates

The current implementation lacks error handling and validation when updating WordPress options:

1. Add validation before updating options
2. Implement error handling
3. Consider adding logging for audit purposes

 if ( Flat::SOURCE === $commission_type ) {
+    try {
         $category_commission_item['percentage'] = '';
         $category_commission_item['flat'] = $commission;
+    } catch (\Exception $e) {
+        error_log("Failed to update category commission for term_id: $term_id. Error: " . $e->getMessage());
+        return;
+    }
 } elseif ( Percentage::SOURCE === $commission_type ) {
+    try {
         $category_commission_item['percentage'] = $commission;
         $category_commission_item['flat'] = '';
+    } catch (\Exception $e) {
+        error_log("Failed to update category commission for term_id: $term_id. Error: " . $e->getMessage());
+        return;
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

includes/Upgrade/AdminNotice.php (2)

40-40: Improve message formatting for better readability.

The addition of the database backup warning is crucial, especially given the commission data structure changes. However, consider restructuring the message for better clarity.

Consider this format:

-            'description'       => __( 'Updating your Dokan data is required to continue functional operations. Kindly backup your database before running upgrade for safety.', 'dokan-lite' ),
+            'description'       => __( 'Updating your Dokan data is required to continue functional operations. IMPORTANT: Please backup your database before proceeding with the upgrade process.', 'dokan-lite' ),

The revised message:

• Maintains the essential warning
• Uses "IMPORTANT" to draw attention
• Uses more formal language ("Please" instead of "Kindly")
• Clarifies the timing ("before proceeding" instead of "before running")

---

40-40: Maintain consistency in backup warning messages.

The backup warning appears in both the description and confirmation dialog with slightly different wording. Consider using consistent language to reinforce the importance of the backup step.

If you implement the previous suggestion for the description, consider updating the confirmation message as well:

-                    'confirm_message' => __( 'It is strongly recommended that you backup your database before proceeding. Are you sure you wish to run the updater now?', 'dokan-lite' ),
+                    'confirm_message' => __( 'IMPORTANT: Please backup your database before proceeding with the upgrade process. Are you sure you wish to run the updater now?', 'dokan-lite' ),

Also applies to: 51-51

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7bb6ad8 and ef4710d.

📒 Files selected for processing (1)

• includes/Upgrade/AdminNotice.php (1 hunks)

🔇 Additional comments (1)

includes/Upgrade/AdminNotice.php (1)

Line range hint 71-91: LGTM: Robust error handling implementation.

The error handling in the upgrade process is comprehensive, covering:

• Permission validation
• Process state verification
• Upgrade requirement checks
• Proper exception handling with appropriate status codes

[Aunshon]

Commission updater data mapping.