(HPOS/COT) Orders Admin List > Prevent user from paging out-of-bounds

[barryhughes]

With the 'legacy' admin list table for orders, or in fact any of the default CPT admin list tables, users are prevented from paging beyond the range of available pages. For example, suppose:

• 10 pages of results are available.
• The user tries to access page 20 (or follows an old link).
• They will be redirected back to page 10.

Due to an oversight, that same behavior does not currently happen with respect to the COT/HPOS admin list table. This change addresses that.

How to test the changes in this Pull Request:

Enable COT/HPOS.

1. Visit the admin list table for orders.
2. Note the maximum page number.
3. Try to access a page beyond that.
   • With this change, you should be redirected to whatever the highest available page number is.
   • Without this change, you will see a helper screen explaining that no orders exist just yet.

FOR PR REVIEWER ONLY:

• I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

[barryhughes]

This isn't directly related to the problem at hand, but I thought it was a sensible safeguard (since custom code hooked in to the above filter could remove it, but we depend on it being set to true).

[github-actions]

Test Results Summary

Commit SHA: 2e43f52

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	118	0	0	2	0	120	0m 41s
E2E Tests	185	0	0	2	0	187	17m 33s

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.

[jorgeatorres]

Hey @barryhughes!

This works perfectly. Thanks! 🥇

Not really a blocker, but I had a question about a particular line of code (marked in the review). Let me know what you think!

[jorgeatorres]

Can't we just use $order_count->max_num_pages instead of calculating $max_num_pages here? For that to work, we would have to not override the limit arg, though, so that the one in $order_query_args is used instead.

I might be missing something, though, so please let me know.

[barryhughes]

Hi @jorgeatorres,

Yes—that would definitely make the code a lot cleaner/shorter (and it's what I tried first) ... but it doesn't work quite as one might expect.

Suppose we have 100 orders and we are fetching pages of 20 items (so, 5 pages in total). Regardless of whether the data-store is COT or CPT, once we are 'out of bounds' (in the following example I am trying to access page 20, which is not available) we aren't going to get back the information we need:

wc_get_orders( [
	'limit'    => 20,
	'page'     => 20,
	'paginate' => true,
] )->max_num_pages;

# => (int) 0

In fact, the same thing can be observed if we use a WP_Query (and in the case of the CPT data-store that is presumably the origin of this behavior):

( new WP_Query( [
	'post_type'      => 'shop_order',
	'paged'          => 20,
	'posts_per_page' => 20,
	'post_status'    => 'any',
] ) )->max_num_pages;

# => (int) 0

So, in summary, we will get back useful pagination data so long as we are within the bounds of the available data. Outside of that, it zeroes out and we either need to workaround it as I did here or use direct queries (I'm trying to be data-store agnostic in my approach, though).

Please do let me know if you can see another path (or if I'm misunderstanding), though.

[jorgeatorres]

Yes, I observed that same behavior (which can be argued is confusing), but in any case, I believe $order_count should be within bounds because we're setting page to 1 in the args for that specific query. So if we don't set limit = 1 we should be basically running the same query as the main query but on page 1, which would give us correct information... or am I missing something @barryhughes?

[barryhughes]

Sorry, @jorgeatorres—I misunderstood your question 😓

Yes, I believe you are correct. It's more a question of trade-offs. If we retain the same limit (let's say it is 20) then we are also going to fetch upto that number of orders and load them into memory, simply to discard them and perform a redirect (via the call to set_pagination_args()).

With this strategy, we fetch only 1 entity and the arithmetic to calculate $max_num_pages is surely very fast. So, while we could shave off a couple of lines of code by keeping limit (and in some cases, this may be a lot higher than 20—such as if the merchant configures the number of items per page to 100), it comes with a higher cost.

On the flip-side, your suggestion probably makes the code a bit more readable. What do you think?

[jorgeatorres]

With this strategy, we fetch only 1 entity and the arithmetic to calculate $max_num_pages is surely very fast.

I wasn't considering performance when I posted my comment, but that makes sense. I still think it's not ideal that we have to do this, and possibly something to add/change in OrdersTableQuery, but right now is probably not the best moment to do that 😅.

[barryhughes]

I still think it's not ideal that we have to do this, and possibly something to add/change in OrdersTableQuery, but right now is probably not the best moment to do that 😅.

Yep, I agree with both parts; though changing OrdersTableQuery might mean we also need a corresponding change in WC_Order_Data_Store_CPT. I think it would be a reasonable change to introduce, and could definitely simplify things.

[jorgeatorres]

LGTM 🚢.

[github-actions]

Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

• Add the release: add testing instructions label