-
Notifications
You must be signed in to change notification settings - Fork 217
Add method to fetch and cache all saved payment methods for a customer #4567
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
Add method to fetch and cache all saved payment methods for a customer #4567
Conversation
…ment_method() to use get_all_payment_methods()
…tion_payment_method() to use get_all_payment_methods()" This reverts commit c622d76.
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 introduces a new method to efficiently fetch and cache all saved payment methods for a Stripe customer, reducing the number of API calls made to Stripe's payment_methods endpoint.
- Adds a new
get_all_payment_methods()
method to theWC_Stripe_Customer
class that fetches all payment methods in paginated requests and caches the complete result - Updates the payment token retrieval logic to use the new method instead of making separate API calls per payment method type
- Implements proper pagination handling for customers with more than 100 saved payment methods
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
includes/class-wc-stripe-customer.php | Adds the new get_all_payment_methods() method with pagination and caching logic |
includes/payment-tokens/class-wc-stripe-payment-tokens.php | Updates token retrieval to use the new unified method instead of multiple API calls |
readme.txt | Documents the API call reduction improvement |
changelog.txt | Adds changelog entry for the fix |
} | ||
|
||
$cache_key = self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id(); | ||
$all_payment_methods = get_transient( $cache_key ); |
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.
Shouldn't we use the WC_Stripe_Database_Cache
here?
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.
We could, but I was wary of adding new code and adding new data to the database cache before we have a cleanup mechanism (which I created an issue for earlier today in #4569).
For large stores with regular customers, using the database cache would result in a big jump in the options size, which could easily drive other performance issues on sites.
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 also want to pull @diegocurbelo into this thread as he flagged the persistent cache usage as well in his review.
I've implemented calling WC_Stripe_Database_Cache
in #4570, mostly so we can merge that change into this branch if we decide to go in that direction.
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.
For large stores with regular customers, using the database cache would result in a big jump in the options size, which could easily drive other performance issues on sites.
Expired transients are not automatically removed from the DB; that requires custom code (calling delete_expired_transients ()
) or using a plugin... and each transient adds two items to the options table (one with the data, and one with the expiration), the DB cache uses one so it would be half the amount of entries than using transients.
I can create a quick PR that adds an additional options entry for the expiration timestamp in the private write_to_cache()
method, and then add a new public method delete_expired()
to WC_Stripe_Database_Cache
similar to this one. And schedule it to run every 24 hours (filterable).
This would generate the same number of options entries as using transients, and provide a way for custom code to remove expired cache items, what do you think @daledupreez?
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.
For now, I am concerned about timelines and stability, and what changes should (and should not) be included in 9.8.0. Maybe it makes sense to have a quick sync with the team to work things out?
return []; | ||
} | ||
|
||
if ( ! is_array( $response->data ) || [] === $response->data ) { |
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.
Since this is getting lengthy, I would consider creating new private methods to handle different parts of the logic.
This looks good @daledupreez! My only concern is the use of transients; given the problems we had with the PMC cache, maybe we should use the DB cache instead. And as you mentioned in Slack, we should also implement a scheduled task to remove expired items from the cache, since we use the We need to improve the database cache to fully support this use case... currently it stores the |
@diegocurbelo, I agree that this could be problematic, but we're still reducing the number of API calls considerably with this implementation, even if transients are not working. I am also concerned about causing the options data size to balloon, even if we're explicitly flagging these items with So I went with the more conservative approach of continuing to use a transient and only changing the API we call. |
Sounds good, we can migrate it to use the DB cache in the next release, after we implement a cache cleanup solution. |
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.
#4567) * Basic get_all_payment_methods() implementation * Ensure we clear the new _all payment methods cache * Always send expand flags for SEPA * Refactor logic to get customer UPE tokens * Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() to use get_all_payment_methods() * Revert "Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() to use get_all_payment_methods()" This reverts commit c622d76. * Reinstate SEPA logic * Remove unnecessary array_merge() call * Changelog * Use Payment Method Configuration to identify active payment methods * Add link to Stripe docs for limit argument * Use one-line isset() for missing customer checks
Related to #4560
Changes proposed in this Pull Request:
This change introduces a new
get_all_payment_methods()
method to theWC_Stripe_Customer
class as a way to perform fewer overall API calls when trying to load all saved payment methods for a customer. The basic approach mimics the implementation in the existingget_payment_methods()
method, but removes the query parameter to limit the accepted payment method types. Due to Stripe's API limiting the result page size to 100 rows, the new logic will fetch in a loop. The new code also fetches all the data and caches it before applying any filtering. That way the cached data can be re-used as needed, even if filters are re-applied. It's worth noting that we're using the same underlying mechanism for the cache as for specific payment methods, including the method responsible for clearing the customer cache.The PR also updates the logic in
WC_Stripe_Payment_Tokens->woocommerce_get_customer_upe_payment_tokens()
to use the new method to fetch all saved tokens for a user across the set of currently enabled payment methods. The key improvement here is that we will now make at most 1 request per customer (or 2, if they have >100 saved payment methods!), and then cache the result. Previously, we would make an API call per enabled payment method, and we'd make more API calls when Optimized Checkout was enabled, as we weren't filtering on active payment methods. The code also makes an important code change to use a more recently introduced function to identify which payment methods are currently active, which also simplifies the code for identifying the active payment methods.From an implementation perspective, it's worth noting that the new code always includes
expand[]=data.sepa_debit.generated_from.charge&expand[]=data.sepa_debit.generated_from.setup_attempt
as URL parameters, just in case we have any SEPA debits created from other payment methods like Bancontact or iDEAL.Testing instructions
Basic setup
Enable SEPA Direct Debit tokens for other methods
setting for the Stripe gateway is enabledSteps
Refresh the page, and verify that only one API call was made to
https://api.stripe.com/v1/payment_methods
(though it will have additional URL parameters).Refresh the page again, and verify that we don't make another outbound HTTP call
develop
, clear transients and repeat the steps to see how many API requests we make 😬Now add a simple product to your cart and navigate to checkout
Use a card to pay for the product, and save your card details
Navigate to My Account -> Payment Methods
Verify that your saved card is listed
Clear transients and refresh the page
Verify there is only one
payment_methods
API callRefresh the page, and verify that we don't trigger another API call
Now add a product to your cart and navigate to checkout.
Clear transients again, and reload checkout.
Verify that you only see one API call to the Stripe
payment_methods
APIReload checkout, and verify that you don't see any calls to the
payment_methods
APIChangelog entry
Changelog Entry Comment
Comment
Post merge