-
Notifications
You must be signed in to change notification settings - Fork 200
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
Enhancement: Make analytics reports compatible with Dokan #2318
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 (3)
tests/php/src/Analytics/Reports/Stock/ProductQueryFilterTest.php (2)
13-33
: LGTM! Consider adding an assertion for the actual product IDs.The test method is correctly testing the scenario where the stock reports are fetched with a seller filter. However, consider adding an assertion to check that the response data contains the exact product IDs created for
seller_id1
.foreach ( $data as $item ) { $prod = wc_get_product( $item['id'] ); } $this->assertCount( count( $seller1_prod_ids ), $data ); +$this->assertEqualsCanonicalizing( $seller1_prod_ids, array_column( $data, 'id' ) );
35-53
: LGTM! Consider adding an assertion for the actual product IDs.The test method is correctly testing the scenario where the stock reports are fetched without a seller filter. However, consider adding an assertion to check that the response data contains the exact product IDs created for both sellers.
foreach ( $data as $item ) { $prod = wc_get_product( $item['id'] ); } $this->assertCount( count( $seller1_prod_ids ) + count( $seller2_prod_ids ), $data ); +$this->assertEqualsCanonicalizing( array_merge( $seller1_prod_ids, $seller2_prod_ids ), array_column( $data, 'id' ) );
includes/Analytics/Reports/BaseQueryFilter.php (1)
168-178
: Consider additional sanitization for the 'seller' GET parameter.The method retrieves the seller ID from the 'sellers' GET parameter and casts it to an integer, which is an improvement from the previous version. However, consider using more rigorous sanitization measures to ensure the value is safe to use in queries.
For example, you could use
absint
to ensure the value is a non-negative integer:-return (int) ( wp_unslash( $_GET['sellers'] ?? 0 ) ); // phpcs:ignore +return absint( wp_unslash( $_GET['sellers'] ?? 0 ) ); // phpcs:ignore
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- includes/Analytics/Reports/BaseQueryFilter.php (1 hunks)
- includes/Analytics/Reports/DataStoreModifier.php (1 hunks)
- includes/Analytics/Reports/Products/Stats/WcDataStore.php (1 hunks)
- includes/Analytics/Reports/Stock/QueryFilter.php (1 hunks)
- includes/Analytics/Reports/Stock/Stats/WcDataStore.php (1 hunks)
- includes/DependencyManagement/Providers/AnalyticsServiceProvider.php (1 hunks)
- tests/php/src/Analytics/Reports/Stock/ProductQueryFilterTest.php (1 hunks)
- tests/php/src/Analytics/Reports/Stock/Stats/QueryFilterTest.php (1 hunks)
Additional comments not posted (24)
includes/Analytics/Reports/Products/Stats/WcDataStore.php (1)
18-26
: LGTM! The method is well-structured and follows best practices.The
initialize_queries()
method is overriding the parent class's method to customize the query behavior. The use of theWcSqlQuery
class instead of theSqlQuery
class is justified in the comments, explaining the need for applying specific filters to the queries.The method follows best practices by:
- Using the
get_db_table_name()
method to get the database table name, which is good for maintainability.- Using the
add_sql_clause()
method to add clauses to the queries, which is a clean and readable way of building the queries.- Using the
clear_all_clauses()
method to clear all clauses before initializing the queries, which ensures that the queries are built from scratch.The method is well-structured and follows the Single Responsibility Principle (SRP) by focusing on initializing the queries.
includes/Analytics/Reports/Stock/QueryFilter.php (2)
12-14
: LGTM!The
register_hooks()
method is correctly registering thecheck_wc_analytics_reports_stock_path
filter hook with the appropriate priority and number of arguments.
39-52
: LGTM!The
add_author_clause()
method is correctly modifying the SQL WHERE clause to filter by seller ID. It has proper documentation and the logic is sound.tests/php/src/Analytics/Reports/Stock/Stats/QueryFilterTest.php (2)
12-30
: LGTM!The test method is well-structured, follows the AAA pattern, and correctly tests the stock stats report for all sellers. The test method name is descriptive and follows the naming convention.
32-52
: LGTM!The test method is well-structured, follows the AAA pattern, and correctly tests the stock stats report filtered by a seller. The test method name is descriptive and follows the naming convention.
includes/Analytics/Reports/DataStoreModifier.php (3)
10-10
: LGTM!The
DataStoreModifier
class follows SOLID principles and has a clear purpose.
19-21
: LGTM!The
register_hooks
method correctly registers the filter to modify WooCommerce data stores.
35-57
: LGTM!The
modify_wc_products_stats_datastore
method follows the Open-Closed Principle and has a clear purpose. The use ofisset
to avoid undefined index notices is a good practice. The docblock provides a clear explanation of the modifications and their reason.includes/DependencyManagement/Providers/AnalyticsServiceProvider.php (4)
11-31
: LGTM!The
AnalyticsServiceProvider
class is well-structured and follows the expected conventions for a service provider in a dependency injection system. The class definition,TAGS
constant, and$services
property are all properly defined.
41-43
: LGTM!The
provides
method is implemented correctly. It checks if the given service alias exists in the$services
property orTAGS
constant and returns a boolean accordingly. The use of strict comparison within_array
is a good practice.
48-58
: LGTM!The
register
method is implemented correctly. It loops through the$services
property and registers each service with the container using a shared definition. It also adds the tags defined in theTAGS
constant to each service definition. This approach is readable, maintainable, and allows for flexibility in retrieving services from the container.
60-64
: LGTM!The
add_tags
method is a well-defined private helper method. It accepts aDefinitionInterface
parameter and a$tags
parameter, ensuring type safety. The method correctly loops through the$tags
and adds each tag to the given service definition using theaddTag
method. This implementation is clean and follows the expected behavior.includes/Analytics/Reports/BaseQueryFilter.php (6)
87-89
: LGTM!The method correctly indicates that the query should always be filtered by seller ID.
96-104
: LGTM!The method correctly retrieves the order types to include in WooCommerce analytics queries based on the user role.
111-119
: LGTM!The method correctly retrieves the refund types to include in WooCommerce analytics queries based on the user role.
121-123
: LGTM!The method correctly retrieves the name of the Dokan table.
130-138
: LGTM!The method correctly retrieves the non-refund order types to include in WooCommerce analytics queries based on the user role.
147-161
: LGTM!The method correctly adds a where clause to filter the query by the seller ID when available. The SQL condition is properly prepared using
$wpdb->prepare
to prevent SQL injection.includes/Analytics/Reports/Stock/Stats/WcDataStore.php (6)
14-57
: LGTM!The
get_data
method is well-implemented and effectively uses transients for caching stock counts. The use of seller-specific transient keys ensures that the cached data is specific to each seller. The method handles the case when the transient is not set and retrieves the data from the database. The use of thewc_get_product_stock_status_options
function ensures compatibility with WooCommerce.
64-101
: LGTM!The
get_low_stock_count
method is well-implemented and correctly retrieves the count of products with low stock. The method handles the case when the low stock amount meta value is not set by using the default low stock amount option. The SQL query is well-structured and uses prepared statements to prevent SQL injection vulnerabilities. The use of theget_seller_where_query
method ensures that the low stock count is specific to each seller.
109-125
: LGTM!The
get_count
method is well-implemented and correctly retrieves the count of products with a specific stock status. The SQL query is simple and uses prepared statements to prevent SQL injection vulnerabilities. The use of theget_seller_where_query
method ensures that the count is specific to each seller.
132-145
: LGTM!The
get_product_count
method is well-implemented and correctly retrieves the total count of products for the store. The method uses theWP_Query
class, which is a standard WordPress approach. It correctly handles the case when a seller ID is available by adding the seller-specific condition to the query arguments. The use ofintval
ensures that the returned count is an integer value.
147-149
: LGTM!The
get_seller_id
method is concise and well-implemented. It uses the Dokan container to retrieve the seller ID from theWeDevs\Dokan\Analytics\Reports\Stock\QueryFilter
class, which is a good practice for dependency injection. The method casts the seller ID to an integer using(int)
to ensure that it returns an integer value.
151-165
: LGTM!The
get_seller_where_query
method is well-implemented and correctly generates the seller-specific WHERE clause for SQL queries. It retrieves the seller ID using theget_seller_id
method and generates a prepared SQL statement to add the seller-specific condition to the WHERE clause. The use of prepared statements ensures that the SQL query is secure and prevents SQL injection vulnerabilities. The method returns an empty string if no seller ID is available, which allows the queries to work correctly without the seller-specific condition.
/** | ||
* Undocumented function | ||
* | ||
* @param [type] $result | ||
* @param [type] $server | ||
* @param WP_REST_Request $request | ||
* @return mixed | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add proper documentation for the check_wc_analytics_reports_stock_path()
method.
The method is missing proper documentation. Please add a clear description of what the method does, the purpose of its parameters, and the expected return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo
- Run migration to resync the analytics Data.
- What will be happen if the dokan stats table is not created but trying to sync the order.
- Add
admin_shipping_fee
andseller_shipping_fee
cols in thedokan_order_stats
table.
*/ | ||
public function register_hooks(): void { | ||
add_action( 'woocommerce_analytics_update_order_stats', [ $this, 'sync_dokan_order' ] ); | ||
add_action( 'woocommerce_before_delete_order', [ $this, 'delete_order' ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete stats?
Yes
- Report will be mismatch with the
dokan_vendor_balance
.
No
- Report will not be synced as per WC stats data.
* @param string $handling Optional. The handling mode for the clause. Defaults to an empty string. | ||
* @return string The SQL clause. | ||
*/ | ||
protected function get_sql_clause( $type, $handling = '' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default params should be a valid value
includes/Install/Installer.php
Outdated
@@ -536,4 +537,26 @@ private static function parse_update_notice( $content, $new_version ) { | |||
|
|||
return wp_kses_post( $upgrade_notice ); | |||
} | |||
|
|||
protected function create_dokan_order_stats_table() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be happened for exist Dokan User?
$customer_id = absint( CustomersDataStore::get_existing_customer_id_from_order( $order ) ); | ||
|
||
// Delete the order. | ||
$wpdb->delete( self::get_db_table_name(), array( 'order_id' => $order_id ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be happened if table does not exist for existing Dokan?
); | ||
|
||
// Update or add the information to the DB. | ||
$result = $wpdb->replace( $table_name, $data, $format ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be happened if table does not exist for existing Dokan?
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (3)
includes/Analytics/Reports/Orders/Stats/DataStore.php (1)
65-65
: Remove commented-out code if not neededThe line setting
$this->date_column_name
is commented out. If this code is no longer necessary, consider removing it to keep the code clean. If it's required, please uncomment it and ensure it functions as intended.includes/Install/Installer.php (2)
542-542
: Improve the clarity of the comment.The comment on line 542 has grammatical errors and could be clearer. Consider rephrasing it for better understanding:
-// Following imported here because this method could be called from the others file. +// The following is included here because this method could be called from other files.
548-549
: Ensure consistent data type declarations forbigint
columns.For consistency with other table definitions, consider specifying the display width for
bigint
columns:-`order_id` bigint UNSIGNED NOT NULL, -`seller_id` bigint UNSIGNED NOT NULL DEFAULT '0', +`order_id` bigint(20) UNSIGNED NOT NULL, +`seller_id` bigint(20) UNSIGNED NOT NULL DEFAULT '0',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- includes/Analytics/Reports/Orders/Stats/DataStore.php (1 hunks)
- includes/Analytics/Reports/WcSqlQuery.php (1 hunks)
- includes/Install/Installer.php (2 hunks)
- includes/Order/MiscHooks.php (0 hunks)
💤 Files with no reviewable changes (1)
- includes/Order/MiscHooks.php
🔇 Additional comments (4)
includes/Analytics/Reports/WcSqlQuery.php (4)
3-5
: LGTM: Namespace and use statement are correctly defined.The namespace
WeDevs\Dokan\Analytics\Reports
is properly defined and follows PSR-4 autoloading standards. The use statement correctly imports the parent classSqlQuery
from the WooCommerce Admin API Reports.
7-7
: LGTM: Class definition and inheritance are correct.The
WcSqlQuery
class is properly defined and correctly extends theSqlQuery
class from WooCommerce Admin API Reports. The class name follows the appropriate naming convention.
8-15
: LGTM: Method documentation is clear and follows PHPDoc standards.The documentation for the
get_sql_clause
method is well-written and informative. It clearly explains the purpose of changing the default value of the$handling
parameter and provides accurate descriptions of the parameters and return value. The documentation follows the PHPDoc standard, which enhances code readability and maintainability.
16-18
: LGTM: Method implementation is correct and addresses the previous review comment.The
get_sql_clause
method is correctly overridden and implements the intended change in the default value of the$handling
parameter. The implementation is concise and properly delegates to the parent class's method.This change addresses the previous review comment about default parameters having valid values. The new default value 'filtered' is a valid and meaningful value for this context.
* Max allowed Attemption to create table. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo in comment: "Attemption" should be "Attempts"
In the comment, the word "Attemption" is incorrect. Please correct it to "Attempts" for proper grammar.
* Attemption count to create order table if DB throws error. | ||
* | ||
* @var integer | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo in comment: "Attemption" should be "Attempt"
Similarly, in this comment, "Attemption" should be "Attempt" to correct the grammatical error.
if ( $wpdb->last_error | ||
&& str_contains( (string) $wpdb->last_error, "{$table_name}' doesn't exist" ) | ||
&& self::MAX_ATTEMPT > self::$attempt_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace str_contains
with strpos
for PHP compatibility
The str_contains
function is only available in PHP 8 and above. To maintain compatibility with earlier PHP versions (e.g., PHP 7.x), consider using strpos
instead.
Apply this diff to fix the compatibility issue:
-&& str_contains( (string) $wpdb->last_error, "{$table_name}' doesn't exist" )
+&& false !== strpos( (string) $wpdb->last_error, "{$table_name}' doesn't exist" )
📝 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 ( $wpdb->last_error | |
&& str_contains( (string) $wpdb->last_error, "{$table_name}' doesn't exist" ) | |
&& self::MAX_ATTEMPT > self::$attempt_count | |
if ( $wpdb->last_error | |
&& false !== strpos( (string) $wpdb->last_error, "{$table_name}' doesn't exist" ) | |
&& self::MAX_ATTEMPT > self::$attempt_count |
includes/Install/Installer.php
Outdated
$sql = "CREATE TABLE IF NOT EXISTS `{$wpdb->prefix}dokan_order_stats` ( | ||
`order_id` bigint UNSIGNED NOT NULL, | ||
`seller_id` bigint UNSIGNED NOT NULL DEFAULT '0', | ||
`order_type` tinyint(1) NOT NULL DEFAULT '0' COMMENT '0 = WC Order, 1 = Dokan Single Vendor Order, 2 = Dokan Suborder, 3 = Refund of Dokan Parent Order, 4 = Refund of Dokan Suborder, 5 = 4 = Refund of Dokan Single Order', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the typographical error in the order_type
column comment.
The comment for the order_type
column contains a duplicate numbering and should be corrected to accurately reflect the order types:
-`order_type` tinyint(1) NOT NULL DEFAULT '0' COMMENT '0 = WC Order, 1 = Dokan Single Vendor Order, 2 = Dokan Suborder, 3 = Refund of Dokan Parent Order, 4 = Refund of Dokan Suborder, 5 = 4 = Refund of Dokan Single Order',
+`order_type` tinyint(1) NOT NULL DEFAULT '0' COMMENT '0 = WC Order, 1 = Dokan Single Vendor Order, 2 = Dokan Suborder, 3 = Refund of Dokan Parent Order, 4 = Refund of Dokan Suborder, 5 = Refund of Dokan Single Order',
📝 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.
`order_type` tinyint(1) NOT NULL DEFAULT '0' COMMENT '0 = WC Order, 1 = Dokan Single Vendor Order, 2 = Dokan Suborder, 3 = Refund of Dokan Parent Order, 4 = Refund of Dokan Suborder, 5 = 4 = Refund of Dokan Single Order', | |
`order_type` tinyint(1) NOT NULL DEFAULT '0' COMMENT '0 = WC Order, 1 = Dokan Single Vendor Order, 2 = Dokan Suborder, 3 = Refund of Dokan Parent Order, 4 = Refund of Dokan Suborder, 5 = Refund of Dokan Single Order', |
`seller_earning` double NOT NULL DEFAULT '0', | ||
`seller_gateway_fee` double NOT NULL DEFAULT '0', | ||
`seller_shipping_fee` double NOT NULL DEFAULT '0', | ||
`seller_discount` double NOT NULL DEFAULT '0', | ||
`admin_commission` double NOT NULL DEFAULT '0', | ||
`admin_gateway_fee` double NOT NULL DEFAULT '0', | ||
`admin_shipping_fee` double NOT NULL DEFAULT '0', | ||
`admin_discount` double NOT NULL DEFAULT '0', | ||
`admin_subsidy` double NOT NULL DEFAULT '0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use DECIMAL
data type for monetary values instead of double
.
Storing monetary values as double
can lead to precision and rounding errors due to floating-point representation. It's recommended to use DECIMAL(19,4)
for monetary amounts to ensure accuracy and consistency.
Apply this diff to update the data types:
-`seller_earning` double NOT NULL DEFAULT '0',
-`seller_gateway_fee` double NOT NULL DEFAULT '0',
-`seller_shipping_fee` double NOT NULL DEFAULT '0',
-`seller_discount` double NOT NULL DEFAULT '0',
-`admin_commission` double NOT NULL DEFAULT '0',
-`admin_gateway_fee` double NOT NULL DEFAULT '0',
-`admin_shipping_fee` double NOT NULL DEFAULT '0',
-`admin_discount` double NOT NULL DEFAULT '0',
-`admin_subsidy` double NOT NULL DEFAULT '0',
+`seller_earning` decimal(19,4) NOT NULL DEFAULT '0',
+`seller_gateway_fee` decimal(19,4) NOT NULL DEFAULT '0',
+`seller_shipping_fee` decimal(19,4) NOT NULL DEFAULT '0',
+`seller_discount` decimal(19,4) NOT NULL DEFAULT '0',
+`admin_commission` decimal(19,4) NOT NULL DEFAULT '0',
+`admin_gateway_fee` decimal(19,4) NOT NULL DEFAULT '0',
+`admin_shipping_fee` decimal(19,4) NOT NULL DEFAULT '0',
+`admin_discount` decimal(19,4) NOT NULL DEFAULT '0',
+`admin_subsidy` decimal(19,4) NOT NULL DEFAULT '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.
`seller_earning` double NOT NULL DEFAULT '0', | |
`seller_gateway_fee` double NOT NULL DEFAULT '0', | |
`seller_shipping_fee` double NOT NULL DEFAULT '0', | |
`seller_discount` double NOT NULL DEFAULT '0', | |
`admin_commission` double NOT NULL DEFAULT '0', | |
`admin_gateway_fee` double NOT NULL DEFAULT '0', | |
`admin_shipping_fee` double NOT NULL DEFAULT '0', | |
`admin_discount` double NOT NULL DEFAULT '0', | |
`admin_subsidy` double NOT NULL DEFAULT '0', | |
`seller_earning` decimal(19,4) NOT NULL DEFAULT '0', | |
`seller_gateway_fee` decimal(19,4) NOT NULL DEFAULT '0', | |
`seller_shipping_fee` decimal(19,4) NOT NULL DEFAULT '0', | |
`seller_discount` decimal(19,4) NOT NULL DEFAULT '0', | |
`admin_commission` decimal(19,4) NOT NULL DEFAULT '0', | |
`admin_gateway_fee` decimal(19,4) NOT NULL DEFAULT '0', | |
`admin_shipping_fee` decimal(19,4) NOT NULL DEFAULT '0', | |
`admin_discount` decimal(19,4) NOT NULL DEFAULT '0', | |
`admin_subsidy` decimal(19,4) NOT NULL DEFAULT '0', |
There was a problem hiding this 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 (2)
includes/Upgrade/Upgrades/V_3_13_0.php (1)
10-16
: Add method documentation for clarity.While the code style is good, adding documentation would improve readability and maintainability. Consider adding a PHPDoc block to explain the purpose of the method, any parameters it might accept in the future, and what it returns.
Here's a suggested documentation block:
/** * Resyncs WooCommerce order stats to synchronize Dokan stats. * * This method performs two main actions: * 1. Creates a Dokan order stats table. * 2. Synchronizes WooCommerce order statistics. * * @since 3.13.0 * * @return void */ public static function resync_wc_order_stats_to_sync_dokan_stats() { // ... (existing code) }includes/Upgrade/Manager.php (1)
35-35
: Improved boolean conversion using explicit castThe change from
! !
to(bool)
is a good improvement. It makes the intention clearer and is more aligned with modern PHP coding practices. This modification enhances code readability without altering the function's behavior.Consider adding a brief comment explaining why we're casting to boolean here, e.g.:
// Ensure a boolean return value regardless of what get_option() returns return (bool) get_option( $this->is_upgrading_db_key, false );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- includes/Upgrade/Manager.php (1 hunks)
- includes/Upgrade/Upgrades.php (1 hunks)
- includes/Upgrade/Upgrades/V_3_13_0.php (1 hunks)
🔇 Additional comments (3)
includes/Upgrade/Upgrades/V_3_13_0.php (2)
1-8
: LGTM: File structure and namespace are well-defined.The file structure, namespace definition, and use statements are correctly implemented, adhering to PSR-4 autoloading standards.
9-9
: Class definition looks good. Verify naming convention.The class
V_3_13_0
is correctly defined and extendsDokanUpgrader
. The naming convention seems to follow a version number format, which is likely used for upgrade scripts.To ensure consistency, let's verify if this naming convention is used for other upgrade classes:
✅ Verification successful
Naming convention is consistent.
The class
V_3_13_0
follows the established version number format for upgrade scripts, consistent with other classes in theincludes/Upgrade/Upgrades
directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar class naming patterns in the Upgrades directory # Test: Search for class definitions in the Upgrades directory rg --type php -g 'V_*.php' -g '!V_3_13_0.php' --no-filename -N '^class V_\d+_\d+_\d+ extends DokanUpgrader' includes/Upgrade/UpgradesLength of output: 1209
includes/Upgrade/Upgrades.php (1)
46-46
: LGTM! Verify the existence of theV_3_13_0
class.The new entry for version 3.13.0 is correctly added and follows the established pattern. This change will enable the upgrade process for users updating to version 3.13.0 of the Dokan Lite plugin.
To ensure the upgrade process will work as expected, please verify that the
V_3_13_0
class exists:✅ Verification successful
Verification Successful:
V_3_13_0
class exists and is properly implemented.The
V_3_13_0
class has been successfully located inincludes/Upgrade/Upgrades/V_3_13_0.php
and contains the necessary implementation to handle the upgrade process for version 3.13.0. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the V_3_13_0 class file # Test: Check if the V_3_13_0.php file exists in the expected directory fd -t f "V_3_13_0.php" includes/Upgrade/Upgrades # If the file exists, display its content for further verification if [ $? -eq 0 ]; then echo "File found. Displaying content:" cat $(fd -t f "V_3_13_0.php" includes/Upgrade/Upgrades) else echo "Error: V_3_13_0.php file not found in the expected directory." fiLength of output: 774
public static function resync_wc_order_stats_to_sync_dokan_stats() { | ||
// Create Dokan order stats table. | ||
( new Installer() )->create_dokan_order_stats_table(); | ||
|
||
// Sync the WC order stats. | ||
$import = ReportsSync::regenerate_report_data( null, false ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider error handling, performance, and transactional integrity.
The resync_wc_order_stats_to_sync_dokan_stats
method implementation is straightforward, but there are a few areas for improvement:
- Error Handling: Add try-catch blocks to handle potential exceptions and log errors.
- Performance: For large databases, this operation could be time-consuming. Consider adding a way to process this in batches or as a background job.
- Transactional Integrity: Wrap the operations in a database transaction to ensure atomicity.
Here's a suggested refactor to address these concerns:
public static function resync_wc_order_stats_to_sync_dokan_stats() {
global $wpdb;
try {
$wpdb->query('START TRANSACTION');
// Create Dokan order stats table.
(new Installer())->create_dokan_order_stats_table();
// Sync the WC order stats.
$import = ReportsSync::regenerate_report_data(null, false);
$wpdb->query('COMMIT');
error_log('Successfully resynced WC order stats to Dokan stats.');
} catch (\Exception $e) {
$wpdb->query('ROLLBACK');
error_log('Failed to resync WC order stats to Dokan stats: ' . $e->getMessage());
}
}
Additionally, consider implementing a batching mechanism or background processing for better performance with large datasets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
docs/container.md (3)
10-15
: Consider using an absolute link for better maintainability.The relative link to the ServiceProvider class might break if the documentation structure changes.
Consider updating line 15 to use an absolute link:
-You can see the already registered service providers inside the `boot` method of the [ServiceProvider](./../includes/DependencyManagement/Providers/ServiceProvider.php#L46) class. +You can see the already registered service providers inside the `boot` method of the [ServiceProvider](https://github.com/weDevsOfficial/dokan/blob/develop/includes/DependencyManagement/Providers/ServiceProvider.php#L46) class.
67-101
: Update method name to follow PHP naming conventions.The
share_with_implements_tags
method name doesn't follow PHP naming conventions.Consider updating line 100 to use camelCase:
-$this->getContainer()->share_with_implements_tags(ServiceClass::class); +$this->getContainer()->shareWithImplementsTags(ServiceClass::class);
117-121
: Add context for service overriding.While the example is clear, it would be helpful to provide more context about when and why one might want to override a service.
Consider adding a brief explanation before the code example, such as:
Service overriding allows you to replace an existing service implementation with a new one. This can be useful for testing, extending functionality, or adapting the system to different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docs/container.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
docs/container.md
8-8: null
Link fragments should be valid(MD051, link-fragments)
🔇 Additional comments (2)
docs/container.md (2)
103-115
: LGTM!This section provides clear and concise examples of how to retrieve services from the container.
123-129
: LGTM!This section provides a clear example of how to check if a service is registered. The link to the League Container documentation is a helpful resource for users who need more detailed information.
### Register Services in the Service Provider | ||
|
||
1. **Step 1:** Register the services inside the `register` method of your service provider. | ||
2. **Step 2:** Implement a `provides` method that returns `true` or `false` when the container invokes it with a service name. | ||
|
||
```php | ||
namespace WeDevs\Dokan\DependencyManagement\Providers; | ||
|
||
use WeDevs\Dokan\DependencyManagement\BaseServiceProvider; | ||
|
||
class SomeServiceProvider extends BaseServiceProvider | ||
{ | ||
/** | ||
* The provides method lets the container know | ||
* which services are provided by this provider. | ||
* The alias must be added to this array or it will | ||
* be ignored. | ||
*/ | ||
public function provides(string $id): bool | ||
{ | ||
$services = [ | ||
'key', | ||
Some\Controller::class, | ||
Some\Model::class, | ||
Some\Request::class, | ||
]; | ||
|
||
return in_array($id, $services); | ||
} | ||
|
||
/** | ||
* The register method defines services in the container. | ||
* Services must have an alias in the `provides` method | ||
* or they will be ignored. | ||
*/ | ||
public function register(): void | ||
{ | ||
$this->getContainer()->add('key', 'value'); | ||
|
||
$this->getContainer() | ||
->add(Some\Controller::class) | ||
->addArgument(Some\Request::class) | ||
->addArgument(Some\Model::class); | ||
|
||
$this->getContainer()->add(Some\Request::class); | ||
$this->getContainer()->add(Some\Model::class); | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider a more dynamic approach for the provides
method.
The current implementation of the provides
method uses a hardcoded array of services, which might become outdated as the codebase evolves.
Consider implementing a more dynamic approach, such as using reflection to automatically detect and register services. Here's an example:
public function provides(string $id): bool
{
$reflection = new \ReflectionClass($this);
$methods = $reflection->getMethods(\ReflectionMethod::IS_PUBLIC);
$services = array_filter($methods, function($method) {
return strpos($method->getName(), 'provide') === 0;
});
$serviceNames = array_map(function($method) {
return lcfirst(substr($method->getName(), 7));
}, $services);
return in_array($id, $serviceNames);
}
This approach automatically detects methods starting with "provide" and uses them to determine the available services.
## Container Documentation | ||
|
||
- [Register Service Provider](#register-service-provider) | ||
- [Register Services in the Service Provider](#register-services-in-the-service-provider) | ||
- [Add Services to the Container](#add-services-to-the-container) | ||
- [Get Service from the Container](#get-service-from-the-container) | ||
- [Override the Existing Service](#override-the-existing-service) | ||
- [Check if a Service is Registered](#check-service-is-registered-or-not) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the invalid link fragment in the table of contents.
The link fragment for "Check if a Service is Registered" doesn't match the corresponding section heading.
Please update line 8 as follows:
-- [Check if a Service is Registered](#check-service-is-registered-or-not)
++ [Check if a Service is Registered](#check-if-a-service-is-registered)
📝 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.
## Container Documentation | |
- [Register Service Provider](#register-service-provider) | |
- [Register Services in the Service Provider](#register-services-in-the-service-provider) | |
- [Add Services to the Container](#add-services-to-the-container) | |
- [Get Service from the Container](#get-service-from-the-container) | |
- [Override the Existing Service](#override-the-existing-service) | |
- [Check if a Service is Registered](#check-service-is-registered-or-not) | |
## Container Documentation | |
- [Register Service Provider](#register-service-provider) | |
- [Register Services in the Service Provider](#register-services-in-the-service-provider) | |
- [Add Services to the Container](#add-services-to-the-container) | |
- [Get Service from the Container](#get-service-from-the-container) | |
- [Override the Existing Service](#override-the-existing-service) | |
- [Check if a Service is Registered](#check-if-a-service-is-registered) |
🧰 Tools
🪛 Markdownlint
8-8: null
Link fragments should be valid(MD051, link-fragments)
@mrabbani , bhai kindly resolve the merge conflict |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- includes/Analytics/Reports/Orders/Stats/DataStore.php (1 hunks)
- includes/Analytics/Reports/Orders/Stats/ScheduleListener.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
includes/Analytics/Reports/Orders/Stats/DataStore.php (1)
161-176
: Match data formats with placeholder types in database operationsEnsure that the format specifiers in the
$format
array correctly correspond to the data types in the$data
array. Mismatches can lead to incorrect data being stored or database errors.Run the following script to verify the data formats:
✅ Verification successful
Format specifiers correctly match the data types in the
$data
array.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check that the data types match the format specifiers in the $format array. # Test: Ensure each %d corresponds to an integer and each %f to a float in the $data array. # Extract the keys from the $data array. data_keys=( 'order_id' 'seller_id' 'order_type' 'seller_earning' 'seller_gateway_fee' 'seller_shipping_fee' 'seller_discount' 'admin_commission' 'admin_gateway_fee' 'admin_shipping_fee' 'admin_discount' 'admin_subsidy' ) # Print the keys with their expected data types based on the $format array. echo "Data keys and expected types:" for i in "${!data_keys[@]}"; do format="${formats[$i]}" key="${data_keys[$i]}" if [ "$i" -le 2 ]; then type="integer (%d)" else type="float (%f)" fi echo "- $key: $type" doneLength of output: 2310
Script:
#!/bin/bash # Description: Extract and verify that the format specifiers in the $format array match the data types in the $data array. # Extract the $format array from DataStore.php using ast-grep formats=$(ast-grep --pattern $'array(\n\s*\'.*?\'\n)' includes/Analytics/Reports/Orders/Stats/DataStore.php | grep -oP '\'.*?\'') # Define the data keys as per the original script data_keys=( 'order_id' 'seller_id' 'order_type' 'seller_earning' 'seller_gateway_fee' 'seller_shipping_fee' 'seller_discount' 'admin_commission' 'admin_gateway_fee' 'admin_shipping_fee' 'admin_discount' 'admin_subsidy' ) # Convert formats to an array IFS=$'\n' read -d '' -r -a format_array <<< "$(echo "$formats" | tr -d '\n' | grep -oP '\%[df]')" # Print the keys with their expected data types based on the $format array. echo "Data keys and expected types based on \$format array:" for i in "${!data_keys[@]}"; do format="${format_array[$i]}" key="${data_keys[$i]}" if [ "$format" == "%d" ]; then type="integer (%d)" elif [ "$format" == "%f" ]; then type="float (%f)" else type="unknown ($format)" fi echo "- $key: $type" done # Optionally, list any discrepancies if formats and data types do not match expected patterns.Length of output: 94
Script:
#!/bin/bash # Description: Extract and verify that the format specifiers in the $format array match the data types in the $data array. # Extract the $format array from DataStore.php using ast-grep formats=$(ast-grep --pattern $'array(\n\s*[\'"%df]+\n' includes/Analytics/Reports/Orders/Stats/DataStore.php | grep -oP '%[df]') # Check if formats were successfully extracted if [ -z "$formats" ]; then echo "Error: Failed to extract format specifiers from DataStore.php." exit 1 fi # Define the data keys as per the original script data_keys=( 'order_id' 'seller_id' 'order_type' 'seller_earning' 'seller_gateway_fee' 'seller_shipping_fee' 'seller_discount' 'admin_commission' 'admin_gateway_fee' 'admin_shipping_fee' 'admin_discount' 'admin_subsidy' ) # Convert formats to an array IFS=$'\n' read -r -d '' -a format_array <<< "$(echo "$formats")" # Check if the number of formats matches the number of data keys if [ "${#format_array[@]}" -ne "${#data_keys[@]}" ]; then echo "Error: The number of format specifiers does not match the number of data keys." exit 1 fi # Print the keys with their expected data types based on the $format array. echo "Data keys and expected types based on \$format array:" for i in "${!data_keys[@]}"; do format="${format_array[$i]}" key="${data_keys[$i]}" if [ "$format" == "%d" ]; then type="integer (%d)" elif [ "$format" == "%f" ]; then type="float (%f)" else type="unknown ($format)" fi echo "- $key: $type" done # Optionally, list any discrepancies if formats and data types do not match expected patterns.Length of output: 291
Script:
#!/bin/bash # Description: Extract and verify that the format specifiers in the $format array match the data types in the $data array. # Debugging: Indicate the start of the script echo "Starting format specifier extraction and verification..." # Extract the $format array from DataStore.php using ast-grep # The pattern targets array elements that are exactly '%d' or '%f' formats=$(ast-grep --pattern 'array\(\s*(["'\'']%[df]["'\'']\s*,?\s*)+\)' includes/Analytics/Reports/Orders/Stats/DataStore.php | grep -oP '(?<=["'\''])(%[df])(?=["'\''])') # Debugging: Show the extracted formats echo "Extracted format specifiers:" echo "$formats" # Check if formats were successfully extracted if [ -z "$formats" ]; then echo "Error: Failed to extract format specifiers from DataStore.php." exit 1 fi # Define the data keys as per the original script data_keys=( 'order_id' 'seller_id' 'order_type' 'seller_earning' 'seller_gateway_fee' 'seller_shipping_fee' 'seller_discount' 'admin_commission' 'admin_gateway_fee' 'admin_shipping_fee' 'admin_discount' 'admin_subsidy' ) # Convert formats to an array IFS=$'\n' read -r -d '' -a format_array <<< "$(echo "$formats")" # Debugging: Show the format array length and contents echo "Number of format specifiers extracted: ${#format_array[@]}" echo "Format specifiers array:" for format in "${format_array[@]}"; do echo "$format" done # Check if the number of formats matches the number of data keys if [ "${#format_array[@]}" -ne "${#data_keys[@]}" ]; then echo "Error: The number of format specifiers (${#format_array[@]}) does not match the number of data keys (${#data_keys[@]})." exit 1 fi # Print the keys with their expected data types based on the $format array. echo "Data keys and expected types based on \$format array:" for i in "${!data_keys[@]}"; do format="${format_array[$i]}" key="${data_keys[$i]}" if [ "$format" == "%d" ]; then type="integer (%d)" elif [ "$format" == "%f" ]; then type="float (%f)" else type="unknown ($format)" fi echo "- $key: $type" done # Optionally, list any discrepancies if formats and data types do not match expected patterns. # (Assuming additional validation logic can be added here if needed)Length of output: 537
Script:
#!/bin/bash # Description: Extract and verify that the format specifiers in the $format array match the data types in the $data array. # Debugging: Indicate the start of the script echo "Starting format specifier extraction and verification..." # Locate the lines defining the $format array in DataStore.php format_lines=$(grep -A 20 "\$format\s*=\s*array(" includes/Analytics/Reports/Orders/Stats/DataStore.php) # Debugging: Show the extracted format lines echo "Extracted format array lines:" echo "$format_lines" # Check if format_lines were successfully extracted if [ -z "$format_lines" ]; then echo "Error: Failed to locate the \$format array in DataStore.php." exit 1 fi # Extract the format specifiers (%d and %f) using sed formats=$(echo "$format_lines" | sed -n "s/.*'\(%[df]\)'.*/\1/p") # Debugging: Show the extracted formats echo "Extracted format specifiers:" echo "$formats" # Check if formats were successfully extracted if [ -z "$formats" ]; then echo "Error: Failed to extract format specifiers from the \$format array." exit 1 fi # Define the data keys as per the original script data_keys=( 'order_id' 'seller_id' 'order_type' 'seller_earning' 'seller_gateway_fee' 'seller_shipping_fee' 'seller_discount' 'admin_commission' 'admin_gateway_fee' 'admin_shipping_fee' 'admin_discount' 'admin_subsidy' ) # Convert formats to an array IFS=$'\n' read -r -d '' -a format_array <<< "$(echo "$formats")" # Debugging: Show the format array length and contents echo "Number of format specifiers extracted: ${#format_array[@]}" echo "Format specifiers array:" for format in "${format_array[@]}"; do echo "$format" done # Check if the number of formats matches the number of data keys if [ "${#format_array[@]}" -ne "${#data_keys[@]}" ]; then echo "Error: The number of format specifiers (${#format_array[@]}) does not match the number of data keys (${#data_keys[@]})." exit 1 fi # Print the keys with their expected data types based on the $format array. echo "Data keys and expected types based on \$format array:" for i in "${!data_keys[@]}"; do format="${format_array[$i]}" key="${data_keys[$i]}" if [ "$format" == "%d" ]; then type="integer (%d)" elif [ "$format" == "%f" ]; then type="float (%f)" else type="unknown ($format)" fi echo "- $key: $type" done # Optionally, list any discrepancies if formats and data types do not match expected patterns. # (Assuming additional validation logic can be added here if needed)Length of output: 5731
public function delete_order( $order_id ) { | ||
return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::delete_order( $order_id ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unnecessary return
statement in delete_order
method.
Similar to the sync_dokan_order
method, the delete_order
method should not return a value. Action hook callbacks are expected to return void
.
Apply this diff to correct the method:
-public function delete_order( $order_id ) {
- return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::delete_order( $order_id );
+public function delete_order( $order_id ): void {
+ \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::delete_order( $order_id );
}
📝 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 delete_order( $order_id ) { | |
return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::delete_order( $order_id ); | |
} | |
public function delete_order( $order_id ): void { | |
\WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::delete_order( $order_id ); | |
} |
public function sync_dokan_order( $order_id ) { | ||
return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::sync_order( $order_id ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unnecessary return
statement in sync_dokan_order
method.
The sync_dokan_order
method is intended to return void
as per the docblock, but it currently includes a return
statement. Since this method is an action hook callback, it should not return any value.
Apply this diff to remove the return
statement:
-public function sync_dokan_order( $order_id ) {
- return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::sync_order( $order_id );
+public function sync_dokan_order( $order_id ): void {
+ \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::sync_order( $order_id );
}
📝 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 sync_dokan_order( $order_id ) { | |
return \WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::sync_order( $order_id ); | |
} | |
public function sync_dokan_order( $order_id ): void { | |
\WeDevs\Dokan\Analytics\Reports\Orders\Stats\DataStore::sync_order( $order_id ); | |
} |
public function get_data( $args ) { | ||
throw new Exception( 'Not supported by Dokan' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider returning a WP_Error
instead of throwing an exception
In the get_data
method, throwing an exception may disrupt the normal flow of the application and lead to unhandled exceptions. Instead, consider returning a WP_Error
object to handle the unsupported operation gracefully within the WordPress environment.
Apply this diff to modify the method:
public function get_data( $args ) {
- throw new Exception( 'Not supported by Dokan' );
+ return new \WP_Error( 'not_supported', __( 'Not supported by Dokan', 'dokan' ) );
}
📝 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_data( $args ) { | |
throw new Exception( 'Not supported by Dokan' ); | |
} | |
public function get_data( $args ) { | |
return new \WP_Error( 'not_supported', __( 'Not supported by Dokan', 'dokan' ) ); | |
} |
if ( ! $order->get_id() || ! $order->get_date_created() ) { | ||
dokan_log( 'Dokan Analytics Order not found: ' . $order->get_id() ); | ||
|
||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure order ID is valid before logging
When the order ID is missing, logging Dokan Analytics Order not found:
without a valid ID may result in incomplete log entries. Verify that the order ID is available before including it in the log message to maintain clarity in your logs.
Apply this diff to handle the logging:
if ( ! $order->get_id() || ! $order->get_date_created() ) {
- dokan_log( 'Dokan Analytics Order not found: ' . $order->get_id() );
+ dokan_log( 'Dokan Analytics Order not found: ' . ( $order->get_id() ? $order->get_id() : 'unknown' ) );
return -1;
}
📝 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 ( ! $order->get_id() || ! $order->get_date_created() ) { | |
dokan_log( 'Dokan Analytics Order not found: ' . $order->get_id() ); | |
return -1; | |
} | |
if ( ! $order->get_id() || ! $order->get_date_created() ) { | |
dokan_log( 'Dokan Analytics Order not found: ' . ( $order->get_id() ? $order->get_id() : 'unknown' ) ); | |
return -1; | |
} |
$order = wc_get_order( $order_id ); | ||
$customer_id = absint( CustomersDataStore::get_existing_customer_id_from_order( $order ) ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for order before accessing properties
The wc_get_order( $order_id )
function may return false
if the order does not exist. Accessing methods on a non-object will cause a fatal error. Ensure that $order
is a valid object before proceeding to use it.
Apply this diff to handle the null case:
$order = wc_get_order( $order_id );
+if ( ! $order ) {
+ return;
+}
$customer_id = absint( CustomersDataStore::get_existing_customer_id_from_order( $order ) );
📝 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.
$order = wc_get_order( $order_id ); | |
$customer_id = absint( CustomersDataStore::get_existing_customer_id_from_order( $order ) ); | |
$order = wc_get_order( $order_id ); | |
if ( ! $order ) { | |
return; | |
} | |
$customer_id = absint( CustomersDataStore::get_existing_customer_id_from_order( $order ) ); |
/** | ||
* Dynamically sets the date column name based on configuration | ||
*/ | ||
public function __construct() { | ||
// $this->date_column_name = get_option( 'woocommerce_date_type', 'date_paid' ); | ||
parent::__construct(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Uncomment or remove code related to date column name initialization
The constructor mentions dynamically setting the date column name based on configuration, but the actual code is commented out. To avoid confusion and ensure the code reflects its intended functionality, either uncomment the code or remove the comment if the feature is not needed.
If you intend to set the date column name dynamically, apply this diff:
public function __construct() {
- // $this->date_column_name = get_option( 'woocommerce_date_type', 'date_paid' );
+ $this->date_column_name = get_option( 'woocommerce_date_type', 'date_paid' );
parent::__construct();
}
If not, consider updating the comment to reflect the current implementation.
📝 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.
/** | |
* Dynamically sets the date column name based on configuration | |
*/ | |
public function __construct() { | |
// $this->date_column_name = get_option( 'woocommerce_date_type', 'date_paid' ); | |
parent::__construct(); | |
} | |
/** | |
* Dynamically sets the date column name based on configuration | |
*/ | |
public function __construct() { | |
$this->date_column_name = get_option( 'woocommerce_date_type', 'date_paid' ); | |
parent::__construct(); | |
} |
$parent_order = wc_get_order( $order->get_parent_id() ); | ||
$vendor_id = $parent_order->get_meta( '_dokan_vendor_id' ); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for parent order before accessing metadata
When retrieving the parent order using wc_get_order( $order->get_parent_id() )
, there is a possibility that the parent order does not exist, and the function returns false
. Accessing methods on false
will lead to a fatal error. Include a check to ensure $parent_order
is valid before accessing its metadata.
Apply this diff to prevent errors:
$parent_order = wc_get_order( $order->get_parent_id() );
+if ( ! $parent_order ) {
+ return 0;
+}
$vendor_id = $parent_order->get_meta( '_dokan_vendor_id' );
Committable suggestion was skipped due to low confidence.
array( | ||
'order_id' => $order->get_id(), | ||
'seller_id' => (int) self::get_vendor_id_from_order( $order ), | ||
'order_type' => (int) ( ( new OrderType() )->get_type( $order ) ), | ||
// Seller Data | ||
'seller_earning' => $vendor_earning, | ||
'seller_gateway_fee' => $gateway_fee_provider === 'seller' ? $gateway_fee : '0', | ||
'seller_shipping_fee' => $shipping_fee_recipient === 'seller' ? $shipping_fee : '0', | ||
'seller_discount' => $order->get_meta( '_seller_discount' ), | ||
// Admin Data | ||
'admin_commission' => $admin_earning, | ||
'admin_gateway_fee' => $gateway_fee_provider !== 'seller' ? $gateway_fee : '0', | ||
'admin_shipping_fee' => $shipping_fee_recipient !== 'seller' ? $shipping_fee : '0', | ||
'admin_discount' => $order->get_meta( '_admin_discount' ), | ||
'admin_subsidy' => $order->get_meta( '_admin_subsidy' ), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure consistent data types for numeric values
In the $data
array, some numeric fields are assigned string values like '0'
. For consistency and to prevent potential database issues, use numeric values (integer or float) instead of strings.
Apply this diff to cast values to appropriate numeric types:
'seller_gateway_fee' => $gateway_fee_provider === 'seller' ? (float) $gateway_fee : 0.0,
'seller_shipping_fee' => $shipping_fee_recipient === 'seller' ? (float) $shipping_fee : 0.0,
'seller_discount' => (float) $order->get_meta( '_seller_discount' ),
// Admin Data
'admin_commission' => $admin_earning,
'admin_gateway_fee' => $gateway_fee_provider !== 'seller' ? (float) $gateway_fee : 0.0,
'admin_shipping_fee' => $shipping_fee_recipient !== 'seller' ? (float) $shipping_fee : 0.0,
'admin_discount' => (float) $order->get_meta( '_admin_discount' ),
'admin_subsidy' => (float) $order->get_meta( '_admin_subsidy' ),
📝 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.
array( | |
'order_id' => $order->get_id(), | |
'seller_id' => (int) self::get_vendor_id_from_order( $order ), | |
'order_type' => (int) ( ( new OrderType() )->get_type( $order ) ), | |
// Seller Data | |
'seller_earning' => $vendor_earning, | |
'seller_gateway_fee' => $gateway_fee_provider === 'seller' ? $gateway_fee : '0', | |
'seller_shipping_fee' => $shipping_fee_recipient === 'seller' ? $shipping_fee : '0', | |
'seller_discount' => $order->get_meta( '_seller_discount' ), | |
// Admin Data | |
'admin_commission' => $admin_earning, | |
'admin_gateway_fee' => $gateway_fee_provider !== 'seller' ? $gateway_fee : '0', | |
'admin_shipping_fee' => $shipping_fee_recipient !== 'seller' ? $shipping_fee : '0', | |
'admin_discount' => $order->get_meta( '_admin_discount' ), | |
'admin_subsidy' => $order->get_meta( '_admin_subsidy' ), | |
), | |
array( | |
'order_id' => $order->get_id(), | |
'seller_id' => (int) self::get_vendor_id_from_order( $order ), | |
'order_type' => (int) ( ( new OrderType() )->get_type( $order ) ), | |
// Seller Data | |
'seller_earning' => $vendor_earning, | |
'seller_gateway_fee' => $gateway_fee_provider === 'seller' ? (float) $gateway_fee : 0.0, | |
'seller_shipping_fee' => $shipping_fee_recipient === 'seller' ? (float) $shipping_fee : 0.0, | |
'seller_discount' => (float) $order->get_meta( '_seller_discount' ), | |
// Admin Data | |
'admin_commission' => $admin_earning, | |
'admin_gateway_fee' => $gateway_fee_provider !== 'seller' ? (float) $gateway_fee : 0.0, | |
'admin_shipping_fee' => $shipping_fee_recipient !== 'seller' ? (float) $shipping_fee : 0.0, | |
'admin_discount' => (float) $order->get_meta( '_admin_discount' ), | |
'admin_subsidy' => (float) $order->get_meta( '_admin_subsidy' ), | |
), |
Scenario:
N.B. won't fix issue No. 2 as per decision Steps to reproduce:
|
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
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:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores