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

Merged
merged 14 commits into from
Nov 26, 2024
Merged
7 changes: 6 additions & 1 deletion assets/src/js/setup-wizard/commission/AdminCommission.vue
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@
this.fixedCommission.fixed = commission.additional_fee ? Number( commission.additional_fee ) : 0;
this.fixedCommission.percentage = commission.admin_percentage ? Number( commission.admin_percentage ) : 0;
this.selectedCommission = commission.commission_type ? String(commission.commission_type) : 'fixed';
this.commission = commission.commission_category_based_values;
let commission_category_based_values = commission.commission_category_based_values;

commission_category_based_values.all = ! commission_category_based_values.all || Array.isArray( commission_category_based_values.all ) ? {} : commission_category_based_values.all;
commission_category_based_values.items = ! commission_category_based_values.items || Array.isArray( commission_category_based_values.items ) ? {} : commission_category_based_values.items;

this.commission = commission_category_based_values;
Comment on lines +64 to +69
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

Strengthen data validation and initialization.

The current initialization logic has several potential issues:

  1. Loose comparisons might not catch all edge cases
  2. No validation for commission percentage ranges
  3. No handling for decimal/thousand separators

Consider applying these improvements:

-            let commission_category_based_values = commission.commission_category_based_values;
+            let commission_category_based_values = commission.commission_category_based_values || {};
 
-            commission_category_based_values.all = ! commission_category_based_values.all || Array.isArray( commission_category_based_values.all ) ? {} : commission_category_based_values.all;
-            commission_category_based_values.items = ! commission_category_based_values.items || Array.isArray( commission_category_based_values.items ) ? {} : commission_category_based_values.items;
+            // Ensure proper object initialization
+            commission_category_based_values.all = (typeof commission_category_based_values.all === 'object' && !Array.isArray(commission_category_based_values.all)) 
+                ? commission_category_based_values.all 
+                : {};
+            commission_category_based_values.items = (typeof commission_category_based_values.items === 'object' && !Array.isArray(commission_category_based_values.items)) 
+                ? commission_category_based_values.items 
+                : {};
+
+            // Validate commission percentages
+            Object.keys(commission_category_based_values.all).forEach(key => {
+                const value = parseFloat(commission_category_based_values.all[key]);
+                if (isNaN(value) || value < 0 || value > 100) {
+                    commission_category_based_values.all[key] = 0;
+                }
+            });

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

},

methods: {
Expand Down
11 changes: 6 additions & 5 deletions includes/Admin/Settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -538,12 +538,13 @@ public function get_settings_fields() {
],
],
'commission_category_based_values' => [
'name' => 'commission_category_based_values',
'type' => 'category_based_commission',
'name' => 'commission_category_based_values',
'type' => 'category_based_commission',
'dokan_pro_commission' => 'yes',
'label' => __( 'Admin Commission', 'dokan-lite' ),
'desc' => __( 'Amount you will get from each sale', 'dokan-lite' ),
'show_if' => [
'label' => __( 'Admin Commission', 'dokan-lite' ),
'desc' => __( 'Amount you will get from each sale', 'dokan-lite' ),
'required' => 'yes',
'show_if' => [
'commission_type' => [
'equal' => 'category_based',
],
Expand Down
10 changes: 9 additions & 1 deletion includes/Commission.php
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,15 @@ public function get_commission( $args = [], $auto_save = false ) {
$category_id = $category_id ? $category_id : 0;
}

if ( ! empty( $product_id ) && empty( $total_amount ) ) {
/**
* If the $total_amount is empty and $order_item_id is empty then we will calculate the commission based on the product price.
* There is a case where the $total_amount is empty and $order_item_id is empty but the $product_id is not empty
* In this case, we will calculate the commission based on the product price.
* Also there is an issue when 100% coupon is applied see the below link for more details
*
* @see https://github.com/getdokan/dokan/pull/2440#issuecomment-2488159960
*/
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
Expand Down
161 changes: 161 additions & 0 deletions includes/Commission/Upugrader/Update_Category_Commission.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php

namespace WeDevs\Dokan\Commission\Upugrader;
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

Typo in namespace 'Upugrader' should be 'Upgrader'

The namespace WeDevs\Dokan\Commission\Upugrader appears to have a typo. It should be Upgrader instead of Upugrader.

Apply this diff to correct the namespace:

-namespace WeDevs\Dokan\Commission\Upugrader;
+namespace WeDevs\Dokan\Commission\Upgrader;
📝 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
namespace WeDevs\Dokan\Commission\Upugrader;
namespace WeDevs\Dokan\Commission\Upgrader;


use WeDevs\Dokan\Commission\Formula\Flat;
use WeDevs\Dokan\Commission\Formula\Percentage;
use WP_Term;

class Update_Category_Commission {
/**
* The batch size for processing categories
*
* @since DOKAN_PRO_SINCE
*/
const BATCH_SIZE = 20;

/**
* The hook name for processing batches
*
* @since DOKAN_PRO_SINCE
*/
const PROCESS_BATCH_HOOK = 'process_category_batch';

/**
* Initialize the processor
*/
public function init_hooks() {
add_action( self::PROCESS_BATCH_HOOK, [ $this, 'process_batch' ], 10, 1 );
}

/**
* Start the batch processing
*
* @since DOKAN_PRO_SINCE
*
* @return void
*/
public function start_processing() {
// Schedule the first batch with page 1
$this->schedule_next_batch( 1 );
}

/**
* Process a batch of categories
*
* @since DOKAN_PRO_SINCE
*
* @param int $page_number Current page number
*
* @return void
*/
public function process_batch( $page_number ) {
// Get categories for this batch
$categories = $this->get_categories_batch( $page_number );

if ( ! empty( $categories ) && ! is_wp_error( $categories ) ) {
foreach ( $categories as $category ) {
$this->process_single_category( $category );
}

// Schedule next batch since we have categories in this batch
$this->schedule_next_batch( $page_number + 1 );
}
}

/**
* Schedule the next batch of categories
*
* @since DOKAN_PRO_SINCE
*
* @param int $page_number Next page number to process
*
* @return void
*/
protected function schedule_next_batch( $page_number ) {
WC()->queue()->add(
self::PROCESS_BATCH_HOOK,
[ $page_number ],
'category_processing'
);
}

/**
* Get a batch of categories.
*
* @since DOKAN_PRO_SINCE
*
* @param int $page_number Page number to fetch
*
* @return array Array of term objects
*/
protected function get_categories_batch( $page_number ) {
$args = [
'taxonomy' => 'product_cat',
'number' => self::BATCH_SIZE,
'orderby' => 'name',
'order' => 'ASC',
'hide_empty' => false,
'offset' => ( $page_number - 1 ) * self::BATCH_SIZE,
];

return get_terms( $args );
}

/**
* Process a single category.
*
* @since DOKAN_PRO_SINCE
*
* @param WP_Term $term Category term object
*
* @return void
*/
protected function process_single_category( $term ) {
error_log(
sprintf(
'Processing category #%d: %s',
$term->term_id,
$term->name
)
);

$dokan_selling = get_option( 'dokan_selling', [] );
$category_commission = dokan_get_option( 'commission_category_based_values', 'dokan_selling', [] );

$commission_type = get_term_meta( $term->term_id, 'per_category_admin_commission_type', true );
$admin_additional_fee = get_term_meta( $term->term_id, 'per_category_admin_additional_fee', true );
$commission = get_term_meta( $term->term_id, 'per_category_admin_commission', true );

if ( ! empty( $commission_type ) ) {
$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;
}

$dokan_selling['commission_category_based_values'] = $category_commission;
update_option( 'dokan_selling', $dokan_selling );
}

/**
* Check if processing is currently running.
*
* @since DOKAN_PRO_SINCE
*
* @return bool
*/
public function is_processing() {
return WC()->queue()->get_next( self::PROCESS_BATCH_HOOK ) !== null;
}
}
186 changes: 186 additions & 0 deletions includes/Commission/Upugrader/Update_Product_Commission.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
<?php

namespace WeDevs\Dokan\Commission\Upugrader;
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

Typo in Namespace Declaration

There's a typo in the namespace declaration. It should be Upgrader instead of Upugrader.

Apply this diff to correct the typo:

-namespace WeDevs\Dokan\Commission\Upugrader;
+namespace WeDevs\Dokan\Commission\Upgrader;
📝 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
namespace WeDevs\Dokan\Commission\Upugrader;
namespace WeDevs\Dokan\Commission\Upgrader;


use WC_Product;
use WeDevs\Dokan\Commission\Formula\Fixed;
use WeDevs\Dokan\Commission\Formula\Flat;
use WeDevs\Dokan\Commission\Formula\Percentage;

class Update_Product_Commission {

/**
* The batch size for processing products
*
* @since DOKAN_PRO_SINCE
*/
Comment on lines +12 to +16
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

Replace '@SInCE DOKAN_PRO_SINCE' placeholders with actual version numbers in docblocks

The @since DOKAN_PRO_SINCE annotations in the docblocks are placeholders and should be replaced with the actual version numbers where these constants and methods are introduced.

Also applies to: 19-23, 26-30, 37-43, 56-65, 83-92, 111-119, 132-138, 150-159, 183-189

const BATCH_SIZE = 10;

/**
* The hook name for processing batches
*
* @since DOKAN_PRO_SINCE
*/
const PROCESS_BATCH_HOOK = 'process_product_batch';

public function init_hooks() {
add_action( self::PROCESS_BATCH_HOOK, [ $this, 'process_batch' ], 10, 2 );
}

/**
* Start the batch processing
*
* @since DOKAN_PRO_SINCE
*
* @return void
*/
public function start_processing() {
// Get total number of products
$total_products = $this->get_total_products();

if ( $total_products === 0 ) {
return;
}

// Schedule the first batch
$this->schedule_next_batch( 0, $total_products );
}

/**
* Process a batch of products
*
* @since DOKAN_PRO_SINCE
*
* @param int $offset Current offset
* @param int $total_products Total number of products
*
* @return void
*/
public function process_batch( $offset, $total_products ) {
// Get products for this batch
$products = $this->get_products_batch( $offset );

foreach ( $products as $product ) {
$this->process_single_product( $product );
}

// Calculate next offset
$next_offset = $offset + self::BATCH_SIZE;

// If there are more products to process, schedule the next batch
if ( $next_offset < $total_products ) {
$this->schedule_next_batch( $next_offset, $total_products );
}
}

/**
* Schedule the next batch of products
*
* @since DOKAN_PRO_SINCE
*
* @param int $offset Current offset
* @param int $total_products Total number of products
*
* @return void
*/
protected function schedule_next_batch( $offset, $total_products ) {
WC()->queue()->add(
self::PROCESS_BATCH_HOOK, [
$offset,
$total_products,
], 'product_processing'
);
}

/**
* Get a batch of products
*
* @since DOKAN_PRO_SINCE
*
* @param int $offset Current offset
*
* @return WC_Product[] Array of product objects
*/
protected function get_products_batch( $offset ) {
$args = [
'status' => 'publish',
'limit' => self::BATCH_SIZE,
Comment on lines +146 to +148
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

Inconsistent product statuses between total count and batch retrieval

In get_total_products(), the products are fetched with 'status' => 'any', whereas in get_products_batch(), 'status' => 'publish'. This inconsistency may cause the total product count to be inaccurate, leading to scheduling extra batches or missing some products during processing.

Apply this diff to ensure consistent product statuses:

protected function get_total_products() {
    $args = [
-        'status' => 'any',
+        'status' => 'publish',
        'limit'  => -1,
        'return' => 'ids',
    ];

Also applies to: 165-167

'offset' => $offset,
'orderby' => 'ID',
'order' => 'ASC',
];

return wc_get_products( $args );
}

/**
* Get total number of products
*
* @since DOKAN_PRO_SINCE
*
* @return int
*/
protected function get_total_products() {
$args = [
'status' => 'publish',
'limit' => -1,
'return' => 'ids',
];

$products = wc_get_products( $args );
return count( $products );
}
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

Optimize total product count retrieval

Retrieving all product IDs to count the total number of products can be inefficient for large datasets. Consider using the 'paginate' parameter in wc_get_products to get the total product count without loading all product IDs into memory.

Refactor the get_total_products method as follows:

protected function get_total_products() {
    $args = [
        'status'   => 'publish',
        'limit'    => 1,
        'paginate' => true,
        'return'   => 'ids',
    ];

    $products = wc_get_products( $args );
    return $products->total;
}


/**
* Process a single product
* Customize this method based on what you need to do with each product
*
* @since DOKAN_PRO_SINCE
*
* @param WC_Product $product
*
* @return void
*/
protected function process_single_product( $product ) {
// Example processing - customize this based on your needs
error_log(
sprintf(
'Processing product #%d: %s',
$product->get_id(),
$product->get_name()
)
);

$commission = dokan()->product->get_commission_settings( $product->get_id() );

$commission_type_old = $commission->get_type();
$commission->set_type( Fixed::SOURCE );

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 );
}

dokan()->product->save_commission_settings(
$product->get_id(),
[
'type' => $commission->get_type(),
'percentage' => $commission->get_percentage(),
'flat' => $commission->get_flat(),
]
);
}

/**
* Check if processing is currently running
*
* @since DOKAN_PRO_SINCE
*
* @return bool
*/
public function is_processing() {
return WC()->queue()->get_next( self::PROCESS_BATCH_HOOK ) !== null;
}
}
Loading
Loading