-
Notifications
You must be signed in to change notification settings - Fork 211
Adding the subscriptions detached notice as a WC debug tool #4391
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
base: develop
Are you sure you want to change the base?
Conversation
// Check if we have a cached result. | ||
$cached_subscriptions = get_transient( self::DETACHED_SUBSCRIPTIONS_TRANSIENT_KEY ); | ||
if ( ! empty( $cached_subscriptions ) ) { | ||
if ( is_array( $cached_subscriptions ) ) { |
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.
As @annemirasol pointed out, this should fix the lack of caching for an empty list.
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.
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 haven't retested, but I have some high-level concerns that I want to flag before testing.
The admin/merchant experience
I think we should assume people will run this tool on hundreds of subscriptions. That may or may not be intentional, but the user experience will be bad if we're making hundreds of API requests to Stripe, and the HTTP request either reaches a server response timeout or looks like it stalled.
I think we should make this work by default, even when run on larger sites. So I think we should do something like the following:
- Work out how long these API requests to Stripe end up taking, on average
- Use that average to set a default maximum number of subscriptions to check so we get a response in less than 60 seconds -- we should be conservative here, as some sites will have additional latency when making requests to Stripe's APIs
- Add a filter that uses that number as the default maximum number of subscriptions to check, e.g.
wc_stripe_detached_subscriptions_maximum_count
- Add a filter that also applies a maximum timeout of 55 seconds, and make sure we respond within that timeframe when processing detached subscriptions, e.g.
wc_stripe_detached_subscriptions_maximum_time
- If we can detect the server response timeout, we could use 5 seconds less than that value.
- We should specify these limits and the related filters in the upfront message, so users' expectations are set correctly
- If we exceed the maximum number or maximum time, we can include messaging to state that we did so
The cumulative effect of all these changes should be as follows:
- If people run the tool on a site that has too many subscriptions or too much latency, we trip over the respective limit, and give them feedback about the limit we hit, instead of failing and giving them nothing but frustration at waiting for something that times out.
- If more experienced merchants or developers want to increase either limit, they can do so via the filters, but they are then making explicit changes that we can warn them against.
- When we implement logic to allow bulk subscription checks via the subscriptions UI, we can re-use the filters above to continue keeping the experience usable rather than complete.
Fixes
keyword in description
I just checked, and STRIPE-483 includes multiple sub-items -- we shouldn't use Fixes
for any one of the related PRs. It may be better to create sub-issues for each piece of the work, and then mark each of those as complete via the respective PRs.
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
📈 PHP Unit Code Coverage Report
|
I have created a new issue to handle the filters you suggested: STRIPE-502. I have included it under the main issue STRIPE-483, and added sub-issues for each part of the change 👍 |
Fixes STRIPE-499
Changes proposed in this Pull Request:
As part of STRIPE-483, this PR introduces a new WooCommerce Debug Tool to list subscriptions with detached payment methods. This feature was previously used to render an admin notice on every page load and removed due to performance issues.
Tool display in the WooCommerce -> Status -> Tools tab:

Display of detached subscriptions:

Testing instructions
add/subscriptions-detached-tool
)includes/compat/class-wc-stripe-subscriptions-helper.php:53
andincludes/compat/class-wc-stripe-subscriptions-helper.php:88
)includes/compat/class-wc-stripe-subscriptions-helper.php:74
andincludes/compat/class-wc-stripe-subscriptions-helper.php:77
, also comment out the closing brackets to avoid breaking the block) ** You can also purchase subscriptions as a shopper and detach their payment method using the Stripe dashboard instead.
Changelog entry
Changelog Entry Comment
Comment
Post merge