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
Add simple products to the Analytics orders query for the attributes filter #44901
Conversation
Test Results SummaryCommit SHA: 6147225
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
3756836
to
ec04e6e
Compare
$attr_term = get_term_by( 'slug', $meta_value, $meta_key ); | ||
if ( false !== $attr_term ) { | ||
$term_id = $attr_term->term_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.
$term_id
wasn't previously being used, but now it is, this makes sure it is set in both instances.
$sql_clauses['where'][] = $wpdb->prepare( | ||
" | ||
( ( {$join_alias}.meta_key = %s AND {$join_alias}.meta_value {$comparator} %s ) or ( | ||
{$wpdb->prefix}wc_order_product_lookup.variation_id = 0 and {$wpdb->prefix}wc_order_product_lookup.product_id {$in_comparator} ( select product_id from {$wpdb->prefix}wc_product_attributes_lookup where is_variation_attribute=0 and term_id = %s ) |
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.
Added an order sub query, to see if a simple product has a matching attribute id.
Note: this query works only for simple products with global attributes, given the attributes filter only shows global attributes to filter by.
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.
I'm not a fan of this line. Maybe extracting the subquery to a new variable would improve its legibility.
Maybe something like this would work:
$prepared_term_id = intval( $term_id );
$subquery = "SELECT product_id FROM {$wpdb->prefix}wc_product_attributes_lookup WHERE is_variation_attribute = 0 AND term_id = {$prepared_term_id}";
$where_condition = "
( ( {$join_alias}.meta_key = %s AND {$join_alias}.meta_value {$comparator} %s ) OR
({$wpdb->prefix}wc_order_product_lookup.variation_id = 0 AND
{$wpdb->prefix}wc_order_product_lookup.product_id {$in_comparator} ({$subquery}) ) )";
$sql_clauses['where'][] = $wpdb->prepare( $where_condition, $meta_key, $meta_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.
Addressed as part of: 96f2b45
@@ -207,8 +229,8 @@ public function test_product_attributes_filter() { | |||
$response_orders = $response->get_data(); | |||
|
|||
$this->assertEquals( 200, $response->get_status() ); | |||
$this->assertEquals( 1, $response_orders['totals']['orders_count'] ); | |||
$this->assertEquals( 11, $response_orders['totals']['orders_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.
This is 11
now, because previously even when using is_not
it would filter out any orders with simple products. Now the query actually works correctly for is_not
.
Hi @octaedro, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
ec04e6e
to
287f438
Compare
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.
Good job @louwie17! The code looks good and is testing well on my end. I left a small suggestion, although the PR is ok as it is now, so I'll approve it.
287f438
to
96f2b45
Compare
@octaedro could you have another look and also test it once more ( I don't have time ) and if good merge it for me? |
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.
LGTM! 🚀
…filter (#44901) * Add subquery for simple products * Update query to make use of product lookup instead * Add changelog * Revert change * Update PHP tests and fix lint errors * Move subquery out to its own variable to make it more readable
Submission Review Guidelines:
Changes proposed in this Pull Request:
Modifies the attributes filter to include simple products that use global attributes.
Closes #32237
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Changelog entry
Significance
Type
Message
Comment