Skip to content

Conversation

@daledupreez
Copy link
Contributor

Changes proposed in this Pull Request:

While reviewing another change, I noticed that the logic in WC_Stripe_Payment_Tokens->woocommerce_payment_token_deleted() effectively replicates the logic from the central WC_Stripe_API::should_detach_payment_method_from_customer() method. Rather than replicating that logic, and having the implementation drift apart, I have updated WC_Stripe_Payment_Tokens->woocommerce_payment_token_deleted() to call the existing helper method.

Testing instructions

I think inspection should be sufficient for this.


  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Changelog entry

  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Comment

Comment

Post merge

Copy link
Contributor

Copilot AI left a 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 refactors the payment token deletion logic to use a centralized helper method (WC_Stripe_API::should_detach_payment_method_from_customer()) instead of duplicated inline checks. This consolidates the logic for determining when payment methods should be detached from Stripe customers during token deletion.

  • Replaces inline detachment checks with call to existing helper method
  • Updates changelog to document the fix

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
includes/payment-tokens/class-wc-stripe-payment-tokens.php Replaced inline payment method detachment checks with centralized helper method call
changelog.txt Added changelog entry documenting the refactoring
readme.txt Added changelog entry documenting the refactoring

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +483 to +484
// Check if we should detach the payment method from the customer.
if ( ! WC_Stripe_API::should_detach_payment_method_from_customer() ) {
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper method should_detach_payment_method_from_customer() includes an additional check for WooCommerce Subscriptions staging mode (is_woocommerce_subscriptions_staging_mode()) that was not present in the original inline logic. This means the behavior will change: payment methods will now also NOT be detached when WooCommerce Subscriptions staging mode is enabled, even if the site is identified as production by wp_get_environment_type(). While this may be intentional for better safety, it represents a behavioral change that should be acknowledged and tested.

Suggested change
// Check if we should detach the payment method from the customer.
if ( ! WC_Stripe_API::should_detach_payment_method_from_customer() ) {
// Only detach payment methods in production environment.
if ( 'production' !== wp_get_environment_type() ) {

Copilot uses AI. Check for mistakes.
@daledupreez daledupreez requested review from a team, Mayisha and wjrosa and removed request for a team November 18, 2025 20:21
@daledupreez daledupreez marked this pull request as ready for review November 18, 2025 20:21
Copy link
Contributor

@wjrosa wjrosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing those issues! Looks good to me :shipit:

Copy link
Contributor

@Mayisha Mayisha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@daledupreez daledupreez merged commit 3f4a907 into develop Nov 20, 2025
40 checks passed
@daledupreez daledupreez deleted the refactor/use-common-method-for-payment-method-detachment branch November 20, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants