Fix/revenue reports #2423
Fix/revenue reports #2423
Conversation
…sible for analytics revenue page.
… into fix/revenue-reports
WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
shashwatahalder01
added
Needs: Testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
includes/Analytics/Reports/Orders/Stats/QueryFilter.php (3)
66-66: Remove unnecessary commented-out code
The commented-out line assigning $types is no longer needed and can be safely removed to keep the codebase clean and maintainable.
Apply this diff to remove the redundant code:
- // $types = $this->get_order_types_for_sql_excluding_refunds();73-79: Clean up outdated comments to enhance readability
The comment block from lines 73 to 79 references restoring a file and includes an old code link. Since the code has evolved, this comment may no longer be relevant. Consider updating or removing it to reduce confusion.
Apply this diff to remove the outdated comment:
/**
- * Override WC column.
- *
- * We can apply the common where clause after Dokan Coupon Distribution.
- * File to restore: @see https://github.com/getdokan/dokan/blob/2cffa360a94b32033e7591fece5950068ab758f5/includes/Analytics/Reports/Orders/Stats/QueryFilter.php#L4
- */
+ * Override WC columns to include Dokan-specific calculations.
*/138-140: Evaluate the necessity of overriding add_where_subquery_for_vendor_filter
The method add_where_subquery_for_vendor_filter currently only calls the parent method without adding any new functionality. If no additional processing is required, consider removing this override to simplify the code. This can prevent confusion for future maintainers who might expect custom behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- assets/src/js/dokan-admin-analytics.js (0 hunks)
- includes/Analytics/Reports/Orders/Stats/QueryFilter.php (4 hunks)
- includes/Assets.php (0 hunks)
- webpack.config.js (0 hunks)
💤 Files with no reviewable changes (3)
- assets/src/js/dokan-admin-analytics.js
- includes/Assets.php
- webpack.config.js
🔇 Additional comments (2)
includes/Analytics/Reports/Orders/Stats/QueryFilter.php (2)
6-6: Import statement approved
The addition of the OrderType import is appropriate and necessary for the updated functionality.
33-37: Confirm the replacement of filter callbacks
You've commented out the previous filters using add_where_subquery and replaced them with add_where_subquery_for_vendor_filter. Ensure that the new method fully replicates or enhances the functionality of the old one to avoid any regression.
To verify that the new filters produce the desired effect, consider testing the analytics reports to confirm that vendor filtering works as intended.
| public function modify_admin_report_columns( array $column, string $context, string $table_name ): array { | ||
| if ( $context !== $this->context ) { | ||
| return $column; | ||
| } |
There was a problem hiding this comment.
Update the docblock parameter name to match the method signature
The parameter in the method docblock is still named $wc_table_name, but it has been renamed to $table_name in the method signature. Update the docblock to reflect this change for consistency and clarity.
Apply this diff to update the docblock:
/**
* Modifies the admin report columns to include Dokan-specific data.
*
* @param array $column The existing report columns.
* @param string $context The context of the report.
- * @param string $wc_table_name The WooCommerce table name being queried.
+ * @param string $table_name The WooCommerce table name being queried.
*
* @return array Modified report columns.
*/📝 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 modify_admin_report_columns( array $column, string $context, string $table_name ): array { | |
| if ( $context !== $this->context ) { | |
| return $column; | |
| } | |
| /** | |
| * Modifies the admin report columns to include Dokan-specific data. | |
| * | |
| * @param array $column The existing report columns. | |
| * @param string $context The context of the report. | |
| * @param string $table_name The WooCommerce table name being queried. | |
| * | |
| * @return array Modified report columns. | |
| */ | |
| public function modify_admin_report_columns( array $column, string $context, string $table_name ): array { | |
| if ( $context !== $this->context ) { | |
| return $column; | |
| } |
| $dokan_table_name = $this->get_dokan_table(); | ||
| $order_types = $this->get_order_types_for_sql_excluding_refunds(); | ||
| $types = implode( ',', ( new OrderType() )->get_vendor_order_types() ); | ||
| // $types = $this->get_order_types_for_sql_excluding_refunds(); | ||
|
|
||
| $parent_order_types_str = implode( ',', ( new OrderType() )->get_admin_order_types_excluding_refunds() ); | ||
| $refund_order_types_str = implode( ',', ( new OrderType() )->get_vendor_refund_types() ); | ||
|
|
||
| $order_count = "SUM( CASE WHEN {$dokan_table_name}.order_type IN($order_types) THEN 1 ELSE 0 END )"; |
There was a problem hiding this comment.
Ensure consistent usage of $types and $order_types variables
There is potential confusion between the variables $order_types and $types. In line 71, you use $order_types within the SQL string, but elsewhere $types is used for similar purposes. For consistency and to avoid errors, consider standardizing the variable names or verifying that each variable is correctly used.
Option 1: Update the SQL expression to use $types if that is the intended variable.
- $order_count = "SUM( CASE WHEN {$dokan_table_name}.order_type IN($order_types) THEN 1 ELSE 0 END )";
+ $order_count = "SUM( CASE WHEN {$dokan_table_name}.order_type IN($types) THEN 1 ELSE 0 END )";Option 2: If $order_types is intended, ensure its value matches the expected format and usage throughout the code.
Committable suggestion was skipped due to low confidence.
|
Scenario: Steps to reproduce:
Screenshot: |
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
Chores
dokan-admin-analyticsscript from registration and enqueueing processes.dokan-admin-analytics.