Fix Analytics report test cases #2436
Conversation
WalkthroughThe changes involve significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![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 (5)
tests/php/src/Analytics/Reports/ReportTestCase.php (1)
115-118: Consider improving test isolationWhile the tear_down method correctly sets up the Commission instance and calls the parent method, consider the following improvements:
- Reset any Commission-specific state before setting the new instance
- Document why this setup is needed in tear_down rather than setUp
Consider this enhanced implementation:
public function tear_down() { + // Reset Commission state after each test + dokan()->get_container()->remove('commission'); dokan()->get_container()->extend( 'commission' )->setConcrete( new Commission() ); parent::tear_down(); }tests/php/src/Analytics/Reports/OrderStatsQueryFilterTest.php (2)
47-48: Document the reason for commented assertionsThese assertions are testing important hook registrations. If they're temporarily disabled due to the coupon amount distribution issue, please add a TODO comment explaining:
- Why these assertions are commented out
- The ticket/issue number for tracking
- When they will be re-enabled
- // self::assertNotFalse( has_filter( 'woocommerce_analytics_clauses_where_orders_stats_total', [ $order_stats_query_filter, 'add_where_subquery' ] ) ); - // self::assertNotFalse( has_filter( 'woocommerce_analytics_clauses_where_orders_stats_interval', [ $order_stats_query_filter, 'add_where_subquery' ] ) ); + // TODO: Temporarily disabled due to coupon amount distribution issue (#ISSUE-NUMBER) + // Re-enable once the where clause filtering is updated + // self::assertNotFalse( has_filter( 'woocommerce_analytics_clauses_where_orders_stats_total', [ $order_stats_query_filter, 'add_where_subquery' ] ) ); + // self::assertNotFalse( has_filter( 'woocommerce_analytics_clauses_where_orders_stats_interval', [ $order_stats_query_filter, 'add_where_subquery' ] ) );
128-135: Consider enhancing assertion messagesWhile the error messages are improved, they could be even more helpful during failures.
- $this->assertEquals( $expected, $report_data->totals->{"total_$key"}, $key . ' Mismatch: Expected: ' . $expected . ' Got: ' . $val ); + $this->assertEquals( + $expected, + $report_data->totals->{"total_$key"}, + sprintf( + '%s Mismatch: Expected %.2f (%.2f × %d sub-orders), Got %.2f', + $key, + $expected, + $val, + $sub_ord_count, + $report_data->totals->{"total_$key"} + ) + );tests/php/src/Analytics/Reports/OrderQueryFilterTest.php (2)
96-110: Consider extracting the mock earnings logic to a helper methodThe Commission mock setup is well-structured with proper conditional logic for different scenarios. However, the callback logic in
get_earning_by_ordercould be moved to a separate helper method to improve readability.Consider refactoring like this:
$mock_commission = Mockery::mock( Commission::class ); dokan()->get_container()->extend( 'commission' )->setConcrete( $mock_commission ); -$mock_commission->shouldReceive( 'get_earning_by_order' )->andReturnUsing( - function ( $order, $context = 'seller' ) use ( $expected_data ) { - if ( $order->get_meta( 'has_sub_order' ) ) { - return 0; - } - if ( $context === 'admin' ) { - return $expected_data['admin_commission']; - } - return $expected_data['vendor_earning']; - } -); +$mock_commission->shouldReceive( 'get_earning_by_order' )->andReturnUsing( + [ $this, 'get_mock_earning_by_order' ] +); + +private function get_mock_earning_by_order( $order, $context = 'seller' ) { + if ( $order->get_meta( 'has_sub_order' ) ) { + return 0; + } + if ( $context === 'admin' ) { + return $this->expected_data['admin_commission']; + } + return $this->expected_data['vendor_earning']; +}
139-147: Consider optimizing the array_reduce callbackThe array_reduce implementation is clean but could be optimized with an early return to avoid unnecessary iterations once the matching order is found.
Consider this optimization:
$order_data = array_reduce( $report_data, function ( $carry, $item ) use ( $s_id ) { - if ( $item['order_id'] === $s_id ) { - $carry = $item; - } - - return $carry; + return $item['order_id'] === $s_id ? $item : $carry; }, null );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
includes/Analytics/Reports/Orders/Stats/QueryFilter.php(1 hunks)tests/php/src/Analytics/Reports/OrderQueryFilterTest.php(3 hunks)tests/php/src/Analytics/Reports/OrderStatsQueryFilterTest.php(4 hunks)tests/php/src/Analytics/Reports/ReportTestCase.php(2 hunks)
🔇 Additional comments (5)
tests/php/src/Analytics/Reports/ReportTestCase.php (1)
5-5: LGTM: Commission class import added correctly
The import statement is properly placed and necessary for the Commission class usage in the tear_down method.
tests/php/src/Analytics/Reports/OrderStatsQueryFilterTest.php (3)
7-7: LGTM!
The Commission class import is correctly added and aligns with the test modifications.
101-115: LGTM! Well-structured Commission mock setup
The Commission mock is well implemented with:
- Clear context handling (seller/admin)
- Proper sub-order logic
- Explicit return values
69-69: Verify test coverage for where clause filtering
The where clause filtering is commented out, which could leave a gap in test coverage.
Let's check the test coverage for the where clause filtering:
✅ Verification successful
Where clause filtering is adequately covered in other test files
The commented out add_where_subquery in OrderStatsQueryFilterTest.php is specifically related to coupon amount distribution, while the where clause filtering functionality is still being tested in:
ProductStatsQueryFilterTest.phpProductQueryFilterTest.phpOrderQueryFilterTest.php
These files contain active assertions and mocking for the where clause filtering, ensuring the functionality remains tested across different contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests that might be covering the where clause filtering
rg -l "add_where_subquery" "tests/"
# Check if there are any existing tests for coupon amount distribution
rg -l "coupon.*amount.*distribution" "tests/"
Length of output: 321
Script:
#!/bin/bash
# Check the context of add_where_subquery usage in the test files
rg "add_where_subquery" tests/php/src/Analytics/Reports/ -B 2 -A 2
# Check if there are any active tests for where clause filtering
ast-grep --pattern 'function test_$_($$$) {
$$$
add_where_subquery
$$$
}'
Length of output: 10488
tests/php/src/Analytics/Reports/OrderQueryFilterTest.php (1)
6-6: LGTM: Clean import addition
The Commission class import is properly placed and necessary for the new commission-related test functionality.
| // 'vendor_gateway_fee' => random_int( 5, 10 ), | ||
| // 'vendor_discount' => random_int( 5, 10 ), | ||
| 'admin_commission' => random_int( 5, 10 ), | ||
| 'admin_gateway_fee' => random_int( 5, 10 ), | ||
| 'admin_discount' => random_int( 5, 10 ), | ||
| 'admin_subsidy' => random_int( 5, 10 ), | ||
| // 'admin_gateway_fee' => random_int( 5, 10 ), | ||
| // 'admin_discount' => random_int( 5, 10 ), | ||
| // 'admin_subsidy' => random_int( 5, 10 ), |
There was a problem hiding this comment.
💡 Codebase verification
Keep the commented metrics in test data
The commented-out metrics (vendor_gateway_fee, vendor_discount, admin_gateway_fee, admin_discount, admin_subsidy) are actively used throughout the codebase:
- They are part of the database schema in
includes/Install/Installer.php - Used in order analytics and reporting in
includes/Analytics/Reports/Orders/Stats/DataStore.php - Included in query filters for order statistics
- Stored as order meta data
These metrics should remain in the test data to ensure proper test coverage of the analytics and reporting features.
🔗 Analysis chain
Verify the intentional removal of financial metrics
Several financial metrics have been commented out without replacement:
- vendor_gateway_fee
- vendor_discount
- admin_gateway_fee
- admin_discount
- admin_subsidy
Please confirm if this is intentional and whether these metrics are no longer needed in the test data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these metrics are still used in other test files or main code
rg -l "(vendor|admin)_(gateway_fee|discount|subsidy)" --type php
Length of output: 335
Script:
#!/bin/bash
# Check the actual usage of these metrics in the related files
rg "(vendor|admin)_(gateway_fee|discount|subsidy)" --type php -C 2
Length of output: 6386
| $commission = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_commission ELSE 0 END)"; | ||
| $vendor_earning = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_earning ELSE 0 END)"; | ||
|
|
||
| $clauses[] = "$vendor_earning as total_vendor_earning"; | ||
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_gateway_fee ELSE 0 END) as total_vendor_gateway_fee"; | ||
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_discount ELSE 0 END) as total_vendor_discount"; | ||
| $clauses[] = "$commission as total_admin_commission"; | ||
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_gateway_fee ELSE 0 END) as total_admin_gateway_fee"; | ||
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_discount ELSE 0 END) as total_admin_discount"; | ||
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_subsidy ELSE 0 END) as total_admin_subsidy"; | ||
| $clauses[] = " $commission / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_admin_commission"; | ||
| $clauses[] = "$vendor_earning / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_vendor_earning"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix leading commas to prevent SQL syntax errors
The variables $commission and $vendor_earning start with leading commas. When these are concatenated into the $clauses[] array, it results in extra commas in the SQL query, which can cause syntax errors.
Apply the following diff to remove the leading commas from the variable definitions:
- $commission = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_commission ELSE 0 END)";
- $vendor_earning = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_earning ELSE 0 END)";
+ $commission = "SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_commission ELSE 0 END)";
+ $vendor_earning = "SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_earning ELSE 0 END)";Additionally, ensure that commas are correctly placed when adding these variables to the clauses:
- $clauses[] = "$vendor_earning as total_vendor_earning";
- $clauses[] = "$commission as total_admin_commission";
+ $clauses[] = ", $vendor_earning as total_vendor_earning";
+ $clauses[] = ", $commission as total_admin_commission";This adjustment will prevent extra commas in the SQL query and ensure proper syntax.
📝 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.
| $commission = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_commission ELSE 0 END)"; | |
| $vendor_earning = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_earning ELSE 0 END)"; | |
| $clauses[] = "$vendor_earning as total_vendor_earning"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_gateway_fee ELSE 0 END) as total_vendor_gateway_fee"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_discount ELSE 0 END) as total_vendor_discount"; | |
| $clauses[] = "$commission as total_admin_commission"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_gateway_fee ELSE 0 END) as total_admin_gateway_fee"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_discount ELSE 0 END) as total_admin_discount"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_subsidy ELSE 0 END) as total_admin_subsidy"; | |
| $clauses[] = " $commission / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_admin_commission"; | |
| $clauses[] = "$vendor_earning / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_vendor_earning"; | |
| $commission = "SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_commission ELSE 0 END)"; | |
| $vendor_earning = "SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_earning ELSE 0 END)"; | |
| $clauses[] = ", $vendor_earning as total_vendor_earning"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_gateway_fee ELSE 0 END) as total_vendor_gateway_fee"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN vendor_discount ELSE 0 END) as total_vendor_discount"; | |
| $clauses[] = ", $commission as total_admin_commission"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_gateway_fee ELSE 0 END) as total_admin_gateway_fee"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_discount ELSE 0 END) as total_admin_discount"; | |
| $clauses[] = ", SUM( CASE WHEN {$table_name}.order_type IN($types) THEN admin_subsidy ELSE 0 END) as total_admin_subsidy"; | |
| $clauses[] = " $commission / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_admin_commission"; | |
| $clauses[] = "$vendor_earning / SUM( CASE WHEN {$table_name}.order_type IN($types) THEN 1 ELSE 0 END ) AS avg_vendor_earning"; |
All Submissions:
Changes proposed in this Pull Request:
Update the
SQLcondition to sum of the Dokan Total e.i.total_admin_commission,total_vendor_earning, and etc.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
Commissionclass, improving validation of order statistics.