Warn merchant when deactivating WCPay plugin that WCPay Subscriptions (Stripe Billing )will continue renewing #5169
Conversation
- WCPay-specific warning message coming soon to this file - Also added comments to clarify
|
Size Change: +365 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
- hard coded true for now
- convert result array to bool - only query for ids to optimise
| 'fields' => 'ids', | ||
| 'subscription_status' => 'active', | ||
| // Ignoring phpcs warning, we need to search meta. | ||
| 'meta_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query |
There was a problem hiding this comment.
Is there a way to avoid the meta_query here?
There was a problem hiding this comment.
I don't think there's a way to avoid using meta_query here.
That said, this isn't the correct way to use the 'meta_query' param.
It needs to be a nested inside an array for it to work.
For example:
$results = wcs_get_subscriptions(
[
'subscriptions_per_page' => 1,
'subscription_status' => 'active',
'meta_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query
[
'key' => '_wcpay_subscription_id',
'compare' => 'EXISTS',
]
],
]
);
There was a problem hiding this comment.
Thanks for explaining, I had no idea how this was supposed to work (I did read the docs for wcs_get_subscriptions!).
Once I've fixed up the PR I'll add something to the docblock (meta_query is not mentioned).
There was a problem hiding this comment.
Just retested with this logic in place. I see now the flaw in my previous testing – I needed to test with WCS / tokenised subscriptions active. Since the meta_query wasn't doing anything I was getting false positives – warning shown when merchant only has active tokenised subs. Good catch 🏀
There was a problem hiding this comment.
I added a follow up issue to document meta_query:
| </div> | ||
| </div> | ||
| <div class="wc-backbone-modal-backdrop modal-close"></div> | ||
| </script> |
There was a problem hiding this comment.
This template is a copy of the other warning modal. I thought that was simpler / more robust than parameterising so we have a single modal template.
| self::SUBSCRIPTION_ID_META_KEY => 'EXISTS', | ||
| ], | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Generally keen for feedback on this query and edge cases it might need to detect. For the modal it doesn't need to be 100% perfect; false positives are ok. But if this method gets used elsewhere later it's probably a good idea to be super clear about what this searches for, and any potential edge cases.
There was a problem hiding this comment.
I don't have any feedback on this. I think this approach will catch 99.99999% which I'm fine with :D There will always be edge cases but the nice thing about having this code in a separate function is if we do discover edge cases we can update this query or add more queries
| $( this ).WCBackboneModal( { | ||
| template: 'wcpay-subscriptions-plugin-warning', | ||
| } ); | ||
|
|
||
| return false; | ||
| }, | ||
| deactivate_subscriptions: function ( event ) { | ||
| // Trigger deactivate flow for WC Subscriptions. | ||
| redirect_deactivate_wc_subscriptions: function ( event ) { |
There was a problem hiding this comment.
Renaming these as I've taken a "duplicate the existing code" approach, rather than parameterise these functions (which IMO would be more complicated without much payoff - these functions are all one-liners).
|
Requesting a review from @Automattic/helix FYI @mattallan Questions for reviewer:
Also FYI:
|
haszari
added
the
component: wcpay subscriptions
|
@elizaan36 - FYI adding a new modal when disabling WCPay, to warn merchant that WCPay (built in) subscriptions will continue renewing. Let me know if you have any feedback on the modal or how this fits into the overall UI. Thanks! |
|
@haszari Can we adjust the first two paragraphs of the modal to be the following? |
|
Thanks for the ping, @haszari ! Just a quick note on the modal component styles and CTA.
Last thing is the modal title ("Are you sure?") looks like the right size but Bold weight. This is the Here's a quick sketch to illustrate the notes above. |
There was a problem hiding this comment.
I've done some testing on this PR, mainly focusing on the new store_has_active_wcpay_subscriptions() function.
From my testing, this function was always returning true even when I didn't have a single subscription with _wcpay_subscription_id meta.
I've provided some feedback on how to fix this as part of my review (slight issue with how the meta query param was used)
With my suggested changes, I've tested this function will work in HPOS and CPT environments and also with older versions of WC Subscriptions and the latest version of subscriptions bundled in WC Payments.
| 'fields' => 'ids', | ||
| 'subscription_status' => 'active', | ||
| // Ignoring phpcs warning, we need to search meta. | ||
| 'meta_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query |
There was a problem hiding this comment.
I don't think there's a way to avoid using meta_query here.
That said, this isn't the correct way to use the 'meta_query' param.
It needs to be a nested inside an array for it to work.
For example:
$results = wcs_get_subscriptions(
[
'subscriptions_per_page' => 1,
'subscription_status' => 'active',
'meta_query' => [ // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query
[
'key' => '_wcpay_subscription_id',
'compare' => 'EXISTS',
]
],
]
);
| $results = wcs_get_subscriptions( | ||
| [ | ||
| 'subscriptions_per_page' => 1, | ||
| 'fields' => 'ids', |
There was a problem hiding this comment.
'fields' isn't a valid param for wcs_get_subscriptions() so this can be removed. This function always returns the object.
There was a problem hiding this comment.
I saw that eventually wc_get_orders was called, so assumed the args would carry through. Fixed in 14ef0a4
| self::SUBSCRIPTION_ID_META_KEY => 'EXISTS', | ||
| ], | ||
| ] | ||
| ); |
There was a problem hiding this comment.
I don't have any feedback on this. I think this approach will catch 99.99999% which I'm fine with :D There will always be edge cases but the nice thing about having this code in a separate function is if we do discover edge cases we can update this query or add more queries
|
Addressed reviewer feedback. @mattallan thanks for the tip about @mattallan I did some more testing with WCS active and inactive, and a couple of subscriptions (one tokenised, one stripe billing). If you could retest with a couple of interesting scenarios I'd appreciate it :) Here's the current state of the modal, incorporating new wording (@aheckler) and revised default button position (on right) (@elizaan36).
Note - I haven't fixed the other styling issues picked up by @elizaan36 - title weight, button layout on mobile. These styles come from WooCommerce core, and as far as I can tell, our markup is correct. So if we want to improve/fix the title or button layout issues, I think that should be logged as a WC core issue. I checked a core modal and see that it also has a bold title.
|
|
@haszari Two minor changes:
Thanks! |
…ore . full stops
…ve-stripe-subscriptions' into fix/5090-warn-deactivate-with-live-stripe-subscriptions
|
Thanks @aheckler - good spotting! I've fixed that up now:
|
|
Ready for a final review @mattallan / @brucealdridge 🙇 |
There was a problem hiding this comment.
Really nice additions coming in this PR @haszari !!
I have tested the following cases against your latest changes:
- Store with one or many active WC Pay Subscriptions and no other payment methods. Correctly shows the message on deactivation ✅
- Store with one or many canceled WC Pay Subscriptions. No notice is shown. ✅
- Store with only active WC Payments (tokenized) and no stripe billing subscriptions. No notice is shown. ✅
- Store with some active WC Payments (tokenized) and some active stripe billing subscriptions. Notice is correctly shown. ✅
- Store with some active WC Payments (tokenized) and some cancelled stripe billing subscriptions. No notice is shown. ✅
- Tested clicking "Yes, Deactivate WooCommerce Payments" actually does deactivate the plugin ✅
The notice/warning also looks good to me (I double checked the links as well):
If there are no more design changes/feedback this is good to merge :) Marking as approved.
|
Thanks for the thorough testing @mattallan 👌 |
haszari
deleted the
fix/5090-warn-deactivate-with-live-stripe-subscriptions
branch
… (Stripe Billing )will continue renewing (#5169) * minor refactor to rename various functions to scope to WCS: - WCPay-specific warning message coming soon to this file - Also added comments to clarify * stub modal to warn when disabling WCPay plugin * add script data to control wcpay subs plugin deactivate warning - hard coded true for now * function for determining if store has active subscriptions (first cut) * fix has_active_wcpay subs: - convert result array to bool - only query for ids to optimise * use has_wcpay_subs to only show warning modal when appropriate * polish wording & docs links in active wcpay subs warning modal * add changelog * clarify changelog * fix syntax of meta_query - was not working, causing false positives for tokenised WCS subs * Update modal wording & links as advised by @aheckler * reorder buttons in subscriptions warning modals – primary button should be rightmost * remove fields arg - not supported by wcs_get_subscriptions * modal tidies: move `learn more` to first para, links should close before . full stops Co-authored-by: Rua Haszard <[email protected]>
Fixes #5090
Changes proposed in this Pull Request
This PR adds a new modal if merchant clicks deactivate link for WCPay on plugins screen. If the store has active WCPay subscriptions, the modal will be shown.
The purpose of the modal is to warn merchants that Stripe billing (off site) subscriptions will continue renewing automatically.
Screenshots
This modal is displayed if merchant clicks
Deactivatefor WooCommerce Payments plugin, with active WCPay built-in subscriptions.Full wording for easy review:
Testing instructions
Basic setup
You should now have an active WCPay Subscription.
Deactivate WCPay Plugin to see the warning modal & test deactivate/cancel flow
Dashboard > Plugins.WooCommerce Payments.Deactivate.CancelDeactivate.Yes, deactivate WooCommerce Payments- you accept that subscriptions will continue renewing, you have a plan for how to handle that (e.g. reactivate plugin, this is temporary).Repeat test with no WCPay Subscriptions active
Reactivate WCPay & ensure you have an active subscription (
Basic setupabove).Dashboard > WooCommerce > Subscriptions.Activestatus.Suspendlink.Deactivate WCPay Plugintest steps. The modal should not be displayed.I followed the test steps above. I encourage reviewers to test other flow combinations, for example:
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge
What's New in 5.2draft on WCPay P2.