Pagination Controls for Frontend Subscriptions View

[barryhughes]

Subscribers can visit My Account ‣ Subscriptions ‣ View Subscription and, amongst other things, they will be presented with a list of related orders. However, there is currently no limit on how many orders are listed. Imagine a subscription that renews daily: if this runs for a few years, we'll have hundreds (and eventually thousands) of related orders.

• This presents a usability issue:
  • A massive table of hundreds (/thousands) of rows is not easy to consume.
  • It pushes other information out of sight (by default, billing and shipping addresses).
• It presents a performance issue:
  • Our default caching strategy (see WCS_Related_Order_Store_Cached_CPT) seems to scale really quite well, by storing several lists of related order IDs for each subscription, and I have not touched this. If we do feel a need to adapt this, or supplement the current caching strategy with something else, it should probably be a project of its own.
  • The cost of fetching the raw IDs, then, isn't part of what we address here. Hydration of the order objects is.

With that context in mind, in this PR we introduce a new method to the WC_Subscription class, making it possible to fetch individual pages of related orders, and we add pagination controls to the frontend related orders list.

Fixes WOOSUBS-162 | https://github.com/woocommerce/woocommerce-subscriptions/issues/4751

Further notes

From a local test using a subscription with 339 related orders, I observed the following results (measured using Query Monitor):

Branch	Object cache	Peak memory (MB)	Total DB queries
Trunk	off	198	5381
Trunk	on (+primed)	147	3241
PR	off	131	3124
PR	on (+primed)	85	1159

💡 Be aware that if you test with enough orders, and with Query Monitor activated, you may run out of memory. So be ready to adjust that php.ini file and bump memory_limit accordingly (or else reduce the size of the test data set).

How can this code break?

Before this change, we passed an array of all related order IDs into the related-orders.php template whereas, with this change, a maximum of 10 IDs will be passed in. It's possible that, in some cases, our users will have implemented their own pagination (or have some other custom feature that depends on having access to the full range of IDs for some other reason).

I suspect this would be the exception rather than the rule, and if it surfaces we could initially guide them toward restoring the previous behaviour: for example, they could unhook WC_Subscriptions_Order::get_related_orders_template() from the woocommerce_subscription_details_after_subscription_table action, and replace it with a custom callback that follows the earlier strategy.

Additionally, it was previously the case that the woocommerce_subscription_related_orders filter hook would be invoked one or more times (once for each related order type, ie 'renewal', 'switch', etc) whereas with this change, that will no longer happen. Once again, this could cause issues in some cases, and the mitigation would be similar to what I described earlier.

How to test this PR

If you want to look at performance characteristics, I'd recommend using Query Monitor. However, if, like me, you test using a 'real' customer account instead of the main admin account, Query Monitor output won't be available. You can fix this with a small snippet like this:

/**
 * Make Query Monitor available to all users.
 * 
 * ⚠️ Remove after testing!
 */
add_action( 'init', function() {
	$user = wp_get_current_user();

	if ( is_object( $user ) ) {
		$user->add_cap( 'view_query_monitor' );
	}
} );

To functionally test pagination, you may also wish to generate a large-ish number of renewal orders. You can do this manually, via the subscription editor, by repeatedly running the create pending renewal order action. That could get a little tiresome, though, depending on how many orders you want to generate and so, though not perfect, you could also consider leveraging a bit of code like this:

/**
 * This script can be used to quickly add a stack of renewals
 * to an existing subscription. Can be useful when testing
 * scalability concerns for WooCommerce Subscriptions.
 */
function generate_renewal_order_for_subscription( $subscription_id ) {
	$subscription = new WC_Subscription( $subscription_id );

	// We can't successfully trigger creation of a renewal unless the subscription
	// is active, which may not initially be the case.
	if ( 'active' !== $subscription->get_status() ) {
		$subscription->update_status( 'active' );
	}

	// Try triggering a renewal. This also often sets the subscription to on-hold,
	// so we again change the status.
	WC_Subscriptions_Manager::prepare_renewal( $subscription_id );
	$subscription->update_status( 'active' );
}

// Generate 100 renewals for the subscription (adjust the sub ID as needed).
for ( $i = 1; $i <= 100; $i++ ) {
	generate_renewal_order_for_subscription( 12345 ); // ⚠️ Supply actual subscription ID.
	print '.' . ( ( $i % 10 ) ? '' : PHP_EOL );       // Optional CLI progress dots :-)
}

For the testing itself, navigate to a suitable subscription on the frontend: My Account Subscriptions ‣ View Subscription:

1. A maximum of 10 related orders will be rendered.
2. "First", "Previous", "Next" and "Last" pagination links will appear below the table.
3. If they are not useful (for instance, "First" or "Previous" are not useful if we are looking at page 1 of the results) then they still appear, and are still functional, but are styled a little differently by applying the disabled class.
4. No ajax magic here. However, the links take advantage of an anchor/ID to keep the customer in broadly the same part of the screen whenever they use the pagination controls.

Product impact

• Added changelog entry (or does not apply)
• Will this PR affect WooCommerce Subscriptions? Yes → WOOSUBS-162
• Will this PR affect WooCommerce Payments? No
• Added deprecated functions, hooks or classes to the spreadsheet *None

[barryhughes]

✍🏼 I referenced 7.4.0 a few times (for @since tags, etc). I am happy to update those as needed, as the timing of this PR means it may not land in that release.

[barryhughes]

Lint fails flagged in CI aren't actually related to lines touched in this PR. Happy to address them if preferred (my usual default is not to do that, but, I'm flexible!).

FILE: /home/runner/work/woocommerce-subscriptions-core/woocommerce-subscriptions-core/includes/class-wc-subscriptions-order.php
+------+--------------------------------------------------+
| Line | Description                                      |
+------+--------------------------------------------------+
| 2432 | Processing form data without nonce verification. |
| 2432 | Processing form data without nonce verification. |
+------+--------------------------------------------------+

[barryhughes]

✍🏼 One other note: the pagination controls seem (to my easily pleased eye) pretty good when using the usual Twenty Twenty-something themes. However, they don't look quite as pleasing when using Storefront ... but since Storefront is also less of an active concern these days, I'm not sure if we ought to add special CSS rules for it or not (I assume not, but happy to revise as needed).

[barryhughes]

I left this in place ($limit = ...), but happy to remove it. It provides some flexibility for themers/developers, and the attendant risk of a user supplying -1 probably isn't that bad when you consider that is effectively the same as the extant behaviour in our stable release.

An alternative could be introducing a filter hook but, rather than inflate the public API surface pre-emptively, it may be better to wait and see if there is demand for such a thing.

[vbelolapotkov]

I've tested the code and everything works as expected, great enhancement @barryhughes! 🙌

I'm surprised the number of DB queries is still high, but I suppose it's because we still need to fetch meta keys for order to select only related ones.

Anyway, DB query time is reduced ~30% on my local site, so I think it's a good enhancement.

I've made some minor comments, but those are not critical IMO, so approving.

Finally, you've raised some good points in description, but I don't have enough knowledge at the moment to comment on them, so worth asking for additional opinion.

[vbelolapotkov]

I think there is an opportunity to extract helper method for composing query params

[barryhughes]

WordPress provides just such a helper, in the form of add_query_arg(), though I'm not sure it would make the code any more concise in this case. That said, I agree what we currently have doesn't feel great (though, it's simple).

Are you thinking along these sorts of lines?

$first = paginated_related_orders_url( 1 );
$prev  = paginated_related_orders_url( $prev_page );
$next  = paginated_related_orders_url( $next_page );
/* etc */

If so, though a little unconventional, what do you think of defining this as an anonymous function right here, within the template (ie $paginated_url = function () {})? This would avoid adding such a specific helper function to the public API surface.

Further alternative: we could move this entire chunk of pagination code into a child template.

[vbelolapotkov]

I like anonymous function option, but do not insist on it :)

[barryhughes]

Added in 91959bd.

[vbelolapotkov]

Lint fails flagged in CI aren't actually related to lines touched in this PR. Happy to address them if preferred (my usual default is not to do that, but, I'm flexible!).

I agree here.

[barryhughes]

Interesting observation/thing that tripped me up: if WooCommerce Subscriptions Gifting is also active, then it overrides the myaccount/related-orders.php template with its own (so, pagination will not be present).

[barryhughes]

@vbelolapotkov could I ask you to take one further look, particularly at these new commits?

91959bd
2c4d394

[vbelolapotkov]

@barryhughes I've tested everything again, and it works as expected in case of many orders.

When tested with little number of related orders I noticed that pagination buttons are still rendered and they are clickable, which is not ideal IMO. The most annoying behavior is that on the first click the page is reloaded.

What do you think about hiding them completely (that would be my preference) when there is only one page or at least make them non-clickable?

Interesting observation/thing that tripped me up: if WooCommerce Subscriptions Gifting is also active, then it overrides the myaccount/related-orders.php template with its own (so, pagination will not be present).

Tested this as well, and you're right. I guess to fix this, we'll have to ship a template update for subs gifting. Do you plan doing this? Also are you aware of other our extensions which customize this template? I suppose we'd need to communicate this change with devs during or before release.

Side note, subs gifting list looks cleaner IMO because of the links instead of buttons. Maybe we should do the same? Let's ask designers, cc @poligilad-auto on this or any other design recommendations here.

[vbelolapotkov]

I like this implementation a lot, well done! I've made another round of testing, and noticed that layout looks terrible on mobile devices:

Let's update markup and styling for that:

• Replace text on buttons with "<<" for first and "<" for pervious.
• Fix buttons alignment on the right edge.

[barryhughes]

🎅🏼 I come bearing updates.

---

Replace text on buttons with "<<" for first and "<" for previous.

This has been implemented for small viewports.

Fix buttons alignment on the right edge.

Also addressed, and a small tweak made to improve things when using Storefront.

What do you think about hiding them completely (that would be my preference) when there is only one page or at least make them non-clickable?

Done, and I also hide them if they are not needed (ie, the next and last buttons are not needed if we are on the final page of the result set).

I guess to fix this, we'll have to ship a template update for subs gifting. Do you plan doing this?

I will create an issue so we don't forget (or perhaps will go ahead and create a PR since it's a small change). Inspired by this problem, though, I moved the pagination controls into their own template (and I integrate them into the related-orders template using an existing hook).

For Gifting, we'll still need to make an update, though, so that it triggers the same hook and passes along the same set of parameters.

[vbelolapotkov]

Great improvements @barryhughes, thank you! I've tested it and everything works well. I have some minor non-blocking comments, but don't want to hold this PR anymore, so approving. Feel free to merge once ready.

Styling and layout look much better now. The small thing would be nice to fix before shipping is the space between buttons and customer billing info. Right now there is very little space, especially in Storefront.

What do you think about hiding them completely (that would be my preference) when there is only one page or at least make them non-clickable?
Done, and I also hide them if they are not needed (ie, the next and last buttons are not needed if we are on the final page of the result set).

I played with it, and found UX a bit odd, especially when you have 11 orders. Those buttons just jump on the left/right. I leave the final decision up to you, but I'd show/hide all of them together.

Inspired by this problem, though, I moved the pagination controls into their own template (and I integrate them into the related-orders template using an existing hook).

I love that, thanks!

[vbelolapotkov]

@barryhughes I'd like to understand where the value comes from. The rest of the codebase uses px values (different and weird at times), so I'm wondering what's your source for this choice?

[barryhughes]

It looked like both units of measure are in use in this file (for instance, look at the two blocks which precede the pagination rules—both use em) though you're right that px is more common. It was really just personal choice: I feel em makes more sense when we're shaping how text is laid out. I can certainly switch to px. As to how I got to this specific value, just through experimentation with different themes since there isn't a single breakpoint that works perfectly for all.

[vbelolapotkov]

Thanks for clarifying. I'd say go with whatever makes the most sense for you at this point, but I'd want to review our design guidelines later, and hopefully come to some more consistent approach.

[vbelolapotkov]

FYI, there seem to be some formatting discrepancy. Looks like spaces applied instead of tabs.

[barryhughes]

Apologies ... not sure how that slipped through. Should be addressed in d7d92e9.

[vbelolapotkov]

No worries, it's hard/impossible to spot. I noticed in GitHub UI, but it looked fine in IDE until I highlighted everything :D

[vbelolapotkov]

Double-checking my understanding here. I see that $subscription and $subscription_orders are not used in pagination template. Is it here for other extensions in case they want to have more advanced pagination? If that's the case, sure let's keep it, otherwise, we can limit pagination template to only two params used page and max_num_pages.

[barryhughes]

The template doc block was incorrect, and implied that $subscription and $subscription_orders were in scope. That was wrong, so I've updated it:

266e9b3

Does that clarify things?

[barryhughes]

I played with it, and found UX a bit odd, especially when you have 11 orders. Those buttons just jump on the left/right. I leave the final decision up to you, but I'd show/hide all of them together.

Yep, I agree. Updated so that they remain on the page, but they are more obviously disabled (ie, clicking on them doesn't do anything): 1ed30d2.

[barryhughes]

The small thing would be nice to fix before shipping is the space between buttons and customer billing info. Right now there is very little space, especially in Storefront.

In Storefront specifically, a rule from the theme was taking precedence so our margins weren't being respected. I've made our own rule a little more specific (see ea8e383). This now looks fairly balanced to me, on Storefront as well as Twenty Twenty-Two/Three/Four, but let me know if you want them to be tweaked a little more.

[barryhughes]

I have some minor non-blocking comments, but don't want to hold this PR anymore, so approving. Feel free to merge once ready.

Thanks, though definitely feel free to take another look if you have time and bandwidth.

At this point, I don't want to merge immediately (there are some structural changes unrelated to this issue that it probably makes sense to complete first), so we have time and space to keep finessing, bandwidth-permitting. Thank you, too, for all the feedback—it should result in a much more polished change 😎

[poligilad-auto]

Hey @vbelolapotkov thanks for the ping!

Side note, subs gifting list looks cleaner IMO because of the links instead of buttons. Maybe we should do the same? Let's ask designers, cc @poligilad-auto on this or any other design recommendations here.

Regarding the buttons, I think it makes sense to use buttons as "View" order / subscription are the main actions users want to take on the page. Also, it's best to keep the account area unified and that is what we are using in the "Orders" page.

I took a look and I see we are using the pagination style that is also used in the Orders page.

Ideally we'd used the pagination component and style that's coming from the site 👇

I'll check how we can raise this topic holistically for the account area.

cc @barryhughes

[barryhughes]

I took a look and I see we are using the pagination style that is also used in the Orders page.

Thanks for taking a peek, @poligilad-auto! Yes, that was the basis for this.

Ideally we'd used the pagination component and style that's coming from the site 👇

OK. I'm not sure what's involved there or what sort of variation we might see in pagination implementations across themes, but can certainly take a look.

Would this be worth holding up the change for? I'm happy with that, to be clear, but just trying to gauge if it's something where we can ship now (there are performance and usability benefits to this work, for stores with a large number of renewal or switch orders) and iterate later on the pagination aesthetics?

[poligilad-auto]

Would this be worth holding up the change for? I'm happy with that, to be clear, but just trying to gauge if it's something where we can ship now (there are performance and usability benefits to this work, for stores with a large number of renewal or switch orders) and iterate later on the pagination aesthetics?

@barryhughes I think it’s definitely good to ship for now! We can explore that as a separate step. I’ll post this in a quick P2, I’m also not sure how it was originally implemented.

[barryhughes]

Subscriptions Core has now been archived. For more information, please refer to this announcement.