-
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
Changes from 9 commits
91d638b
35837db
9cfccaf
68bee81
c622d76
2e20c28
b733cd3
243d766
341f885
20ade01
6c9eb2f
0b12464
b6e1e68
aaa7ba7
188098a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -30,6 +30,11 @@ class WC_Stripe_Customer { | |||||
WC_Stripe_UPE_Payment_Method_Becs_Debit::STRIPE_ID, | ||||||
]; | ||||||
|
||||||
/** | ||||||
* The maximum value for the `limit` argument in the Stripe payment_methods API. | ||||||
*/ | ||||||
protected const PAYMENT_METHODS_API_LIMIT = 100; | ||||||
|
||||||
/** | ||||||
* Stripe customer ID | ||||||
* | ||||||
|
@@ -712,7 +717,7 @@ public function get_payment_methods( $payment_method_type ) { | |||||
[ | ||||||
'customer' => $this->get_id(), | ||||||
'type' => $payment_method_type, | ||||||
'limit' => 100, // Maximum allowed value. | ||||||
'limit' => self::PAYMENT_METHODS_API_LIMIT, | ||||||
], | ||||||
'payment_methods' . $params, | ||||||
'GET' | ||||||
|
@@ -741,6 +746,94 @@ public function get_payment_methods( $payment_method_type ) { | |||||
return empty( $payment_methods ) ? [] : $payment_methods; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Get all payment methods for a customer. | ||||||
* | ||||||
* @param string[] $payment_method_types The payment method types to look for using Stripe method IDs. If the array is empty, it implies all payment method types. | ||||||
* @param int $limit The maximum number of payment methods to return. If the value is -1, no limit is applied. | ||||||
* @return array | ||||||
*/ | ||||||
public function get_all_payment_methods( array $payment_method_types = [], int $limit = -1 ) { | ||||||
if ( ! $this->get_id() ) { | ||||||
return []; | ||||||
} | ||||||
|
||||||
$cache_key = self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The cache key uses a hardcoded '_all' separator which could conflict with existing keys or be unclear. Consider using a more descriptive separator or adding it as a class constant for consistency.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think our long-term plan here should just be to use one cache, as most users don't have a large number of saved payment methods, and I think most of our use cases revolve around all saved payment methods rather than saved payment methods for only one particular type of payment. |
||||||
$all_payment_methods = get_transient( $cache_key ); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Expired transients are not automatically removed from the DB; that requires custom code (calling I can create a quick PR that adds an additional options entry for the expiration timestamp in the private 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 commentThe 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? |
||||||
|
||||||
if ( false === $all_payment_methods || ! is_array( $all_payment_methods ) ) { | ||||||
$all_payment_methods = []; | ||||||
$last_payment_method_id = null; | ||||||
|
||||||
do { | ||||||
$request_params = [ | ||||||
'customer' => $this->get_id(), | ||||||
'limit' => self::PAYMENT_METHODS_API_LIMIT, | ||||||
]; | ||||||
|
||||||
if ( $last_payment_method_id ) { | ||||||
$request_params['starting_after'] = $last_payment_method_id; | ||||||
} | ||||||
|
||||||
$response = WC_Stripe_API::request( $request_params, 'payment_methods?expand[]=data.sepa_debit.generated_from.charge&expand[]=data.sepa_debit.generated_from.setup_attempt', 'GET' ); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hardcoded expand parameters in the API request URL make the code inflexible. Consider extracting these parameters into a constant or making them configurable to improve maintainability.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would love to, but that should be tackled in a follow-up PR.
daledupreez marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
if ( ! empty( $response->error ) ) { | ||||||
if ( | ||||||
isset( $response->error->code ) | ||||||
&& isset( $response->error->param ) | ||||||
&& 'customer' === $response->error->param | ||||||
wjrosa marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
&& 'resource_missing' === $response->error->code | ||||||
) { | ||||||
// If the customer doesn't exist, cache an empty array. | ||||||
set_transient( $cache_key, [], DAY_IN_SECONDS ); | ||||||
wjrosa marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
return []; | ||||||
} | ||||||
|
||||||
if ( ! is_array( $response->data ) || [] === $response->data ) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
break; | ||||||
} | ||||||
|
||||||
$all_payment_methods = array_merge( $all_payment_methods, $response->data ); | ||||||
|
||||||
// Reset the last payment method ID so we can paginate correctly. | ||||||
$last_payment_method_id = null; | ||||||
if ( isset( $response->has_more ) && $response->has_more ) { | ||||||
$last_payment_method = end( $response->data ); | ||||||
daledupreez marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
if ( $last_payment_method && ! empty( $last_payment_method->id ) ) { | ||||||
wjrosa marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
$last_payment_method_id = $last_payment_method->id; | ||||||
} | ||||||
} | ||||||
} while ( null !== $last_payment_method_id ); | ||||||
|
||||||
// Always cache the result without any filters applied. | ||||||
set_transient( $cache_key, $all_payment_methods, DAY_IN_SECONDS ); | ||||||
} | ||||||
|
||||||
// If there are no payment methods, no need to apply any filters below. | ||||||
if ( [] === $all_payment_methods ) { | ||||||
return $all_payment_methods; | ||||||
} | ||||||
|
||||||
// Only apply the limit and type filters after fetching and caching all payment methods. | ||||||
$filtered_payment_methods = $all_payment_methods; | ||||||
if ( [] !== $payment_method_types ) { | ||||||
$filtered_payment_methods = array_filter( | ||||||
$filtered_payment_methods, | ||||||
function ( $payment_method ) use ( $payment_method_types ) { | ||||||
return in_array( $payment_method->type, $payment_method_types, true ); | ||||||
} | ||||||
); | ||||||
} | ||||||
|
||||||
if ( $limit > 0 ) { | ||||||
return array_slice( $filtered_payment_methods, 0, $limit ); | ||||||
} | ||||||
|
||||||
return $filtered_payment_methods; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Delete a source from stripe. | ||||||
* | ||||||
|
@@ -846,6 +939,7 @@ public function clear_cache( $payment_method_id = null ) { | |||||
foreach ( self::STRIPE_PAYMENT_METHODS as $payment_method_type ) { | ||||||
delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . $payment_method_type . $this->get_id() ); | ||||||
} | ||||||
delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id() ); | ||||||
// Clear cache for the specific payment method if provided. | ||||||
if ( $payment_method_id ) { | ||||||
WC_Stripe_Database_Cache::delete( 'payment_method_for_source_' . $payment_method_id ); | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.