Add caching for Stripe payment method configuration #4241
Conversation
…ove reset_primary_configuration in favor of clear_primary_configuration_cache
diegocurbelo
requested review from
a team and
Mayisha
and removed request for
a team
There was a problem hiding this comment.
Overall, things are looking OK from inspection, but I have two concerns (with one being less important):
- We are using the same cache for test and live mode, and I am concerned that we could end up with stale data and possibly weird behaviour if users switch between live and test mode. We should either have separate caches for each mode, or we should clear the cache when live/test mode is toggled.
- While I agree with making the naming more consistent, I would personally lean towards doing that across two PRs, as we're introducing a lot more testing and possible side effects in this PR, where we're adding caching and making some naming changes. I am still not too familiar with the code, though, so I may be overstating the testing impact.
I will start manual testing shortly.
| WC_Stripe_API::get_instance()->update_payment_method_configuration( $pmc->id, $updated_payment_methods ); | ||
| self::clear_pmc_cache(); |
There was a problem hiding this comment.
It feels risky to ignore the return value from update_payment_method_configuration(). We're clearing the cache despite us not confirming whether the operation succeeded.
This is a pre-existing feature of the code, but I think clearing the cache won't be majorly problematic, though it feels like it could be overly aggressive, and we may have downstream issues if we've cleared the cache, but can't communicate with the Stripe API for some reason.
There was a problem hiding this comment.
I added a log entry in case of request failure, so we have the full context in the logs.
The proper solution would be to bubble-up the error message (and description) and show it to the merchant in the settings with a failure notice. But I think we should change that in another PR and keep this one focused on the cache. I think that until we refactor the save settings error notice it makes sense to clear the cache on failure, even in the case we can't communicate with the Stripe API, keeping the cache content won't help, as Intent creation will also fail, for example.
There was a problem hiding this comment.
I've now tested, but had some minor difficulties getting exactly the same details right.
- Go to the Payment Methods tab
- Check that only one call to the Configurations API was made
- Reload the page, and check no new Configurations API call was made
My understanding is that this case should always trigger a new API call (which is what I am seeing).
- Check the transient with this WP-CLI
wp transient list --search=wcstripe_payment_method_configuration_cache --unserialize --human-readable
The site I am testing with is using an object cache, so the wp transient list command shows the following warning, and doesn't show the transient we want:
Warning: Transients are stored in an external object cache, and this command only shows those stored in the database.
I can get the transient contents, but not the expiration time. (In this case, I am fairly confident the 10 minute timeout is being obeyed.)
- Delete the transient:
wp transient delete wcstripe_payment_method_configuration_cache- Refresh the checkout page. and check taht only one call was made to the Configurations API
I am seeing another call to fetch the configuration before I reload the checkout page. But I can confirm we're only making one API call. I am not 100% sure where/how the call is being triggered, but I think that's less relevant as long as it is not forcing a refresh from Stripe and bypassing the cache.
- Check the transient again and verify the expiration:
wp transient list --search=wcstripe_payment_method_configuration_cache --unserialize --human-readable
I wasn't able to check this aspect. I was using wp transient get wcstripe_payment_method_configuration_cache almost immediately, but I suspect something in the code loading was fetching the configuration from Stripe, as running the delete and then get commands from the CLI was giving me data. (I suspected the delete might not be working, but I was seeing a new fetch command in the log each time.)
Given the behaviour of transients when we have an object cache in place, it may be worth storing a timestamp in the cached data, and using a cache format that includes some wrapper information around what we're actually caching. But that may be overkill for this first iteration.
|
Don't forget to change the base branch to |
Fixed in 76f3398
Reverted those changes, and will create a new PR for them. |
Correct, I updated the testing instructions... and removed the transient wp-cli commands, as those would be testing how the transients work; as you said, we are fairly confident the 10-minute timeout is being obeyed... are we OK with 10 minutes? Or should we be more aggressive and use 1 hour, for example? |
There was a problem hiding this comment.
Changes look good to me. The main focus of the PR; the caching mechanism is working well.
✅ On the settings page, payment method configuration is always retrieved from Stripe.
✅ On checkout and other pages, cached data is used.
✅ Cache is cleared after 10 minutes and payment method configuration is always retrieved from Stripe again.
In develop branch, just loading any page would make a API request to /payment_method_configurations.
In this branch, only one API request is sent. Extra requests are sent when the settings page is loaded or if the cache is empty.
- On the plugin settings page, we are still showing the available payment methods based on the capabilities object and the
is_available_for_account_countryfunction, instead of the data received from the payment method configuration API.- I have opened a draft PR to implement this Show payment methods available in the PMC on settings page #4252
- When the store currency is USD,
update_payment_method_configurationfails with the error -
'amazon_pay' is not available to this merchant- I confirmed it does not happen in the draft branch.
- Another solution would be to check Amazon Pay here similar to other feature flagged methods.
|
@diegocurbelo, I went ahead and merged this as I was seeing too much noise in my logs from the API calls when testing a different issue. |
📈 PHP Unit Code Coverage Report
|
Fixes #4182
Fixes STRIPE-384
Changes proposed in this Pull Request:
It adds a cache layer (using a transient) when fetching Payment Method Configurations (PMCs) from the Stripe Configurations API.
Testing instructions
Payment MethodstabChangelog entry
Changelog Entry Comment
Comment
Post merge