Add support for specifying filters when searching orders. #43356
Conversation
github-actions
bot
added
the
plugin: woocommerce
vedanshujain
changed the title
Test Results SummaryCommit SHA: 55bf3c8
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. |
vedanshujain
requested review from
a team and
lsinger
and removed request for
a team
|
Hi @lsinger, 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: |
1 similar comment
|
Hi @lsinger, 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: |
There was a problem hiding this comment.
This tests well and almost as described. I believe that the differences between the testing instructions and my testing are to be expected, however?
Similarly, repeat the same for terms "Product", "Customer" and "Common" with "Product" selected in dropdown. No orders will show for term "Customer", whereas, order created in step 2 should show up for term "Product" and "Common".
When searching for "Common" or "Product", both orders show up, as they both contain the "Product Common" product. I assume this is the correct behavior.
Repeat the same for terms "Product", "Customer" and "Common" with "All" selected in dropdown. The behavior here should be exactly the same as the trunk, i.e. order in step 2 will show up for Product, order in step 3 will show for Customer, and both the orders will show up for "Common".
Same issue: when searching for "Common" or "Product", both orders show up, as they both contain the "Product Common" product. Again I assume this is the correct behavior.
When searching for a valid STREET CITY no results are returned, whereas searching for STREET or CITY does return the correct results. In the future we could maybe OR the terms from the search box and then also support adding quotes to ensure only full strings are matched. (I'd consider this definitely out of scope for this PR, FWIW.) I was reminded of this when searching for FIRSTNAME LASTNAME, which does work.
I left a few nitpicks that I'd all consider optional. If the differences between the testing instructions and my testing mentioned above look correct to you as well then I believe this is good to go.
| } | ||
|
|
||
| /** | ||
| * Generate JOIN clause for different search filter. |
There was a problem hiding this comment.
| * Generate JOIN clause for different search filter. | |
| * Generate JOIN clause for a given search filter. |
|
|
||
| /** | ||
| * Generate JOIN clause for different search filter. | ||
| * Right now we only have the products filter that actually does any join, but in future we may add more, for example, in order search, or payment tokens etc. This function is to make it easier to add more filters in the future. |
There was a problem hiding this comment.
| * Right now we only have the products filter that actually does any join, but in future we may add more, for example, in order search, or payment tokens etc. This function is to make it easier to add more filters in the future. | |
| * Right now we only have the products filter that actually does a JOIN, but in the future we may add more -- for example, in order search, payment tokens, and so on. This function makes it easier to add more filters in the future. |
| * Generate JOIN clause for different search filter. | ||
| * Right now we only have the products filter that actually does any join, but in future we may add more, for example, in order search, or payment tokens etc. This function is to make it easier to add more filters in the future. | ||
| * | ||
| * If a search filter needs a join, it will also need a where clause. |
There was a problem hiding this comment.
| * If a search filter needs a join, it will also need a where clause. | |
| * If a search filter needs a JOIN, it will also need a WHERE clause. |
| 'search_query_items.order_item_name LIKE %s', | ||
| '%' . $wpdb->esc_like( $this->search_term ) . '%' | ||
| ) . " OR `$order_table`.id IN ( $meta_sub_query ) "; | ||
| $where = implode( ' OR ', $where ); |
There was a problem hiding this comment.
I don't really like this pattern where we assign a variable to itself and in doing so change its type (from array to string in this case). We could either introduce a different name for either the array or the string, or pull the implode into the return. What do you think? (Optional nitpick.)
|
|
||
| return " ( $where ) "; | ||
| } | ||
|
|
||
| /** | ||
| * Generates where clause for different search filter. Right now we only have the products and customers filter that actually use any `where`, but in future we may add more, for example, in order search, or payment tokens etc. This function is to make it easier to add more filters in the future. |
There was a problem hiding this comment.
| * Generates where clause for different search filter. Right now we only have the products and customers filter that actually use any `where`, but in future we may add more, for example, in order search, or payment tokens etc. This function is to make it easier to add more filters in the future. | |
| * Generates WHERE clause for a given search filter. Right now we only have the products and customers filters that actually use WHERE, but in the future we may add more -- for example, in order search, payment tokens, and so on. This function makes it easier to add more filters in the future. |
| return '-1'; | ||
| } | ||
|
|
||
| // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- $meta_fields is already escaped before imploding, $meta_table is hardcoded. |
There was a problem hiding this comment.
Thank you for explaining the exception, that's a great practice.
| @@ -446,4 +446,95 @@ public function test_query_s_argument() { | |||
| $this->assertCount( 0, $query->orders ); | |||
| } | |||
|
|
|||
| /** | |||
| * Setup some dummy orders, to help testing the search filter. | |||
There was a problem hiding this comment.
| * Setup some dummy orders, to help testing the search filter. | |
| * Set up some dummy orders to help test the search filter. |
(Yes, this is super nitpicky. 😅)
(Not suggesting to change the setup_ method name also because set_ as a prefix implies a setter to me.)
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with 'all' param is passed. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with 'all' param is passed. | |
| * @testDox The 'search_filter' argument works with an 'all' param passed in. |
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with 'product' param is passed. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with 'product' param is passed. | |
| * @testDox The 'search_filter' argument works with a 'product' param passed in. |
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with 'customer' param is passed. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with 'customer' param is passed. | |
| * @testDox The 'search_filter' argument works with a 'customer' param passed in. |
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with a 'customer' param is passed in. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with a 'customer' param is passed in. | |
| * @testDox The 'search_filter' argument works with a 'customer' param passed in. |
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with a 'product' param is passed in. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with a 'product' param is passed in. | |
| * @testDox The 'search_filter' argument works with a 'product' param passed in. |
| } | ||
|
|
||
| /** | ||
| * @testDox The 'search_filter' argument works with an 'all' param is passed in. |
There was a problem hiding this comment.
| * @testDox The 'search_filter' argument works with an 'all' param is passed in. | |
| * @testDox The 'search_filter' argument works with an 'all' param passed in. |
| } | ||
|
|
||
| /** | ||
| * Generates WHERE clause for a given search filter. Right now we only have the products and customers filters that actually use WHERE, but the in future we may add more -- for example, order custom fields, payment tokens etc. This function makes it easier to add more filters in the future. |
There was a problem hiding this comment.
| * Generates WHERE clause for a given search filter. Right now we only have the products and customers filters that actually use WHERE, but the in future we may add more -- for example, order custom fields, payment tokens etc. This function makes it easier to add more filters in the future. | |
| * Generates WHERE clause for a given search filter. Right now we only have the products and customers filters that actually use WHERE, but in the future we may add more -- for example, custom order fields, payment tokens, and so on. This function makes it easier to add more filters in the future. |
|
|
||
| /** | ||
| * Generate JOIN clause for a given search filter. | ||
| * Right now we only have the products filter that actually does a JOIN, but in the future we may add more -- for example, custom order fields, payment tokens and so on. This function makes it easier to add more filters in the future. |
There was a problem hiding this comment.
| * Right now we only have the products filter that actually does a JOIN, but in the future we may add more -- for example, custom order fields, payment tokens and so on. This function makes it easier to add more filters in the future. | |
| * Right now we only have the products filter that actually does a JOIN, but in the future we may add more -- for example, custom order fields, payment tokens, and so on. This function makes it easier to add more filters in the future. |
github-actions
bot
added
the
needs: analysis
|
Note that all comments from last review were addressed in 55bf3c8 |
nigeljamesstevenson
added
needs: external testing
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR adds a select box to the left of the search button to the admin HPOS screen. This allows merchants to select whether they want to search in products or in customer data for the term that they are searching. The primary reason here is performance, in testing on a store with 2.5 million orders, it would take around 1.3s ~ 1.5s to search in the product, about 2.38 ~ 3s to search in customers, but about 5 ~ 6s to search in both.
This PR enables merchants to decide where they want to search, and that way they can enjoy about 2x faster searching when they want to search for products or customers.
Additionally, this also refactors the OrderTableSearchQuery class to support more filters down the line.
Partially addresses #41454
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
/wp-admin/admin.php?page=wc-ordersand see the search form in top right corner:Changelog entry
Significance
Type
Message
Allow selecting search criteria - Products/Customers/All when searching orders.
Comment