-
Notifications
You must be signed in to change notification settings - Fork 216
Don't allow payment methods to be detached from WP-Cron jobs on staging sites #4799
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
Don't allow payment methods to be detached from WP-Cron jobs on staging sites #4799
Conversation
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.
Pull Request Overview
This PR fixes an issue where WP-Cron jobs performing bulk user operations on staging sites were incorrectly treated as user-initiated requests, causing payment methods to be detached from production Stripe accounts. The fix treats WP-Cron requests as admin requests, preventing payment method detachment on staging environments.
- Updated
should_detach_payment_method_from_customer()to includewp_doing_cron()in the admin request check - Added comprehensive test coverage for WP-Cron scenarios in both test and live modes
- Updated changelog to document the fix
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/class-wc-stripe-api.php | Added wp_doing_cron() check to treat WP-Cron requests as admin requests |
| tests/phpunit/WC_Stripe_API_Test.php | Added test cases and is_cron_request parameter to verify WP-Cron behavior |
| changelog.txt | Added changelog entry for the fix |
| readme.txt | Added changelog entry for the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
The code changes look good, and it tests well. I also confirmed in the Stripe dashboard that the payment method was not detached.
Just one minor detail with the instructions:
Now check out this branch and repeat the steps above.
Confirm that after the deletion occurs, the log file includes true for the Cron request
The TEMP DEBUG section needs to be updated to the same condition used later in the function:
$is_admin_request = is_admin() ||
( defined( 'WP_CLI' ) && WP_CLI ) ||
wp_doing_cron();
🤦 You're right! Done. |
…ayment-methods-can-be-detached
malithsen
left a comment
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.
Looks good, and it works as described.
…ayment-methods-can-be-detached
Fixes STRIPE-814
Changes proposed in this Pull Request:
This PR updates our logic for payment method detachment to treat WP-Cron requests as admin requests rather than user-initiated requests. The result of that is that when we get user modification requests via WP-Cron on staging sites, we won't detach the payment method.
The driver for this change is that there are some plugins that perform bulk actions via WP-Cron code, and our current implementation treats those requests as user-initiated requests, which then means we don't check the current environment type.
Testing instructions
We pretty much want to follow the original report in STRIPE-814:
developfor your siteshould_detach_payment_method_from_customer()so that it starts with the following:TARGET_EMAILto your email from above and activate it:falsefor the request, which indicates that we will returntruefor the function we checked.|| wp_doing_cron()to the$debug_is_admin_requestassignmenttruefor the Cron requestChangelog entry
Changelog Entry Comment
Comment
Post merge