Disable WooPay for suspended and rejected accounts #8857
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
asumaran
force-pushed
the
as-disable-woopay-rejected-suspended-accounts
branch
4 times, most recently
from
10c720c to
5c1fa2c
Compare
There was a problem hiding this comment.
Code LGTM and all tests have passed.
- ✅ Disable WooPay for rejected accounts.
- ✅ Disable WooPay for suspended accounts.
- ✅ Remove active WooPay webhooks for rejected accounts.
- ✅ Remove active WooPay webhooks for suspended accounts.
@asumaran Please check my comment below before merging.
| $is_account_rejected = WC_Payments::get_account_service()->is_account_rejected(); | ||
|
|
||
| $is_account_under_review = WC_Payments::get_account_service()->is_account_under_review(); |
There was a problem hiding this comment.
I created this issue only taking into account "rejected*" and "under_review" statuses, but it looks like there could be more statuses and I'm not sure if/how these would be set in a real scenario.
@asumaran should we consider those other statuses as well? Perhaps untimately it would be easier to just check if the account is enabled/complete/not currently restricted?
There was a problem hiding this comment.
should we consider those other statuses as well?
Good question. We'd need to confirm what statuses actually block the account. I'll double check tomorrow. In the meantime I'll ask internally about that.
There was a problem hiding this comment.
I noticed the account status is set to restricted in two cases:
- When payments are paused on Stripe.
- When payouts are paused on Stripe.
When the merchant's account is restricted WooPayments is not disabled on the site neither the WooPayments account.
Does it make sense to turn off the merchant's WooPay eligibility if their Stripe account is restricted? cc @pierorocca @bborman22
There was a problem hiding this comment.
I've tested with a JN site + my sandbox and can confirm WooPay doesn't work if the merchant has payments disabled on Stripe. No iframe is triggered after entering a valid WooPay email. The account status is set to restricted.
The server will set the site as not eligible for WooPay. So, nothing to add there as it's already being handled correctly.
Perhaps untimately it would be easier to just check if the account is enabled/complete/not currently restricted?
I think disabling WooPay for "rejected", "under review" and "restricted" accounts it's OK from WooPayments. If there's more status we need to handle it would be better to do it server side.
Also removes active webhooks if the account is suspended or rejected
account is restriced
asumaran
force-pushed
the
as-disable-woopay-rejected-suspended-accounts
branch
3 times, most recently
from
69035bb to
4c7036a
Compare
asumaran
force-pushed
the
as-disable-woopay-rejected-suspended-accounts
branch
from
4c7036a to
5334184
Compare
Fixes a fatal error when clearing the account data cache. In this PR we call `WC_Payments::get_account_service()->is_account_rejected()` to know if the account is rejected. This method will get the data from Stripe and the `WC_Payments_Account->get_cached_account_data()` will be called. Then the action `woocommerce_payments_account_refreshed` will be fired. The `Compatibility_Service` class is registering a hook for that action. https://github.com/Automattic/woocommerce-payments/blob/trunk/includes/class-compatibility-service.php#L41 Then `Compatibility_Service->get_compatibility_data()` is called. Which calls the `get_permalink()` function. This function internally calls to `_get_page_link` which uses `$wp_rewrite` global but at the time it’s not defined resulting in a PHP fatal error. This commit fixes it. Note that the request to the server is not done on every page refresh, only after clearing the account cache contents.
…er_review` We are already checking if the account is rejected or under review in the `WC_Payments_Features::is_woopay_eligible()` method.
asumaran
force-pushed
the
as-disable-woopay-rejected-suspended-accounts
branch
from
5334184 to
e95c392
Compare
| self::maybe_register_woopay_hooks(); | ||
| // Defer registering the WooPay hooks. Later on, $wp_rewrite is used and causes a fatal error on the WooPayment Dev Tools, | ||
| // given that $wp_rewrite is defined right after the `plugins_loaded` action is fired. See #8857. | ||
| add_action( 'setup_theme', [ __CLASS__, 'maybe_register_woopay_hooks' ] ); |
There was a problem hiding this comment.
It would be good to have more eyes on this. The reasoning is explained in the commit message 69fa9ef
I've tested the full WooPay flow locally and did not experience any errors with this change.
Pinging @waclawjacek since he introduce this line initially and @malithsen since worked on something very similar.
There was a problem hiding this comment.
Hi! 👋 Could you please provide some more details on how I could help here? I'm not sure what the code I introduced is, and I've been detached from this codebase for quite some time so some extra context would be much appreciated.
There was a problem hiding this comment.
Hi @waclawjacek. I believe the original line was added in this PR of yours. I just wanted to get more eyes on this from people familiar with it. The change looks a bit sensitive since I'm delaying some hooks registrations for the reasons mentioned in 4e1b201. I've tested it with WooPay and double-checked that it's working. If you don't see anything to comment that's fine. I reached out to you as an extra precaution ;-)
There was a problem hiding this comment.
I see! Thanks for the explanation and the ping - I appreciate the cautiousness.
I haven't tested the code or done a super deep dive into the WP codebase but the reasoning sounds good to me. It looks like setup_theme is the next hook that fires in wp-settings.php after plugins_loaded (which is when WC_Payments::init() fires right now), and it looks like a good candidate to hook into, despite the hook's name being somewhat irrelevant to the logic here.
There was a problem hiding this comment.
Apologies for the delay in getting to this.
Change looks good to me. I tested WooPay locally with a WCPay account having status=complete and did not notice any regressions.
Closes https://github.com/Automattic/woopay/issues/2641
Changes proposed in this Pull Request
Add checks to
WC_Payments_Features::is_woopay_eligible()to returnfalsein case the merchant account is suspended or rejected.Testing instructions
Warning
Rejecting a merchant account is irreversible. It will inform Stripe about the rejection and it can not be undone.
Since I didn't find a way to reverse a rejection I'm sharing another way to simulate a rejected account.
a) Reject the account using the MC tool.
b) Hardcode the account status
server/wp-content/rest-api-plugins/endpoints/wcpay/class-accounts-controller.phpline885'status' => 'rejected.something',I'd suggest using option "b" if you don't want to go through the hassle of creating a new WCPay account every time you want to test a rejected account.
Disable WooPay for rejected accounts.
Disable WooPay for suspended accounts.
Remove active WooPay webhooks for rejected accounts.
Remove active WooPay webhooks for suspended accounts.
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