Hook up payment methods configuration #4091
Conversation
hsingyuc
requested review from
a team and
malithsen
and removed request for
a team
| $payment_method_configurations = $this->stripe_api->get_payment_method_configurations() && property_exists( $this->stripe_api->get_payment_method_configurations(), 'data' ) | ||
| ? $this->stripe_api->get_payment_method_configurations()->data | ||
| : null; |
There was a problem hiding this comment.
What do we think about storing $this->stripe_api->get_payment_method_configurations() in an intermediate variable and doing the checks on that, to avoid multiple hits on the endpoint?
Something like
| $payment_method_configurations = $this->stripe_api->get_payment_method_configurations() && property_exists( $this->stripe_api->get_payment_method_configurations(), 'data' ) | |
| ? $this->stripe_api->get_payment_method_configurations()->data | |
| : null; | |
| $result = $this->stripe_api->get_payment_method_configurations(); | |
| $payment_method_configurations = $result->data ?? null; |
There was a problem hiding this comment.
Thank you @annemirasol! Updated ebb6e90
There was a problem hiding this comment.
Nice work so far here 👍
Everything tests and works as expected.
✅ Updates to the payment methods on Stripe dashboard are reflected on the settings page.
✅ Updates to the settings page are propagated to Stripe's configuration.
✅ Tested with USD/EUR.
One question: On my Stripe dashboard, there are two configurations.
- "Your configuration"
- "WooCommerce Inc. Configuration"
2nd config is what this PR uses. Is that the correct behavior?
Left some comments about page load performance which we might want to address before merge.
| * @return array|null | ||
| */ | ||
| private function get_merchant_payment_method_configuration() { | ||
| $payment_method_configurations = $this->stripe_api->get_payment_method_configurations() && property_exists( $this->stripe_api->get_payment_method_configurations(), 'data' ) |
There was a problem hiding this comment.
In addition to what Anne mentioned below, calling the Stripe API every time the settings page is loaded will impact page load performance.
Is there a way to avoid that?
One approach is to tie it up with the existing "Refresh account details" button and avoid automatic fetches.
If configuration updates result in a webhook event, another approach might be to listen to those for updates.
There was a problem hiding this comment.
On a separate note, we'd want to add a log if the call to Stripe API fails for some reason.
if ( is_wp_error( $response ) ) {
WC_Stripe_Logger::log( 'Failed to fetch payment method configurations from Stripe API.' );
return null;
}
There was a problem hiding this comment.
One approach is to tie it up with the existing "Refresh account details" button and avoid automatic fetches.
If configuration updates result in a webhook event, another approach might be to listen to those for updates.
This could work, but this would change how the feature works and prevent users who update settings from the Stripe dashboard from seeing them immediately in admin settings. @diegocurbelo, would this work for us?
There was a problem hiding this comment.
On a separate note, we'd want to add a log if the call to Stripe API fails for some reason.
Yes, and we already have a log here
There was a problem hiding this comment.
One approach is to tie it up with the existing "Refresh account details" button and avoid automatic fetches.
If configuration updates result in a webhook event, another approach might be to listen to those for updates.This could work, but this would change how the feature works and prevent users who update settings from the Stripe dashboard from seeing them immediately in admin settings. @diegocurbelo, would this work for us?
Using the "Refresh payment methods" in the options menu was discussed in the Figma 0fiyiUe18lGU6kGyQNmXxE-fi-165_4304 (or maybe in P2 🤷🏼), but as the settings page is not frequently visited and most of the time (if not always) the merchant will want up-to-date information it makes sense to fetch the enabled PMs every time. Using webhooks would be a valid alternative, but it adds another point of failure.
| /** | ||
| * Get the merchant payment method configuration in Stripe. | ||
| * | ||
| * @return array|null |
There was a problem hiding this comment.
Looks like this function returns an object instead of an array.
| * @return array|null | |
| * @return object|null |
There was a problem hiding this comment.
Thank you @malithsen ! Updated ebb6e90
@diegocurbelo, WooCommerce Inc. configuration is the merchant copy that is the one we should update, correct? |
|
Is connecting the settings to the store front out of scope for this PR? For example, if I enable or disable Klarna in the settings page, the checkout page does not reflect the changes. |
Correct, that's the merchant-editable copy of the platform PMC. |
hsingyuc
force-pushed
the
add/settings-synchronization
branch
from
97b5270 to
581556a
Compare
Fixes #4088
Changes proposed in this Pull Request:
This PR hooks up payment method configuration settings to the admin Stripe enabled payment methods.
Out of scope:
Screen.Recording.2025-03-13.at.3.00.23.PM.mov
Testing instructions
Test USD payment methods -
Test EURO payment methods - Cards,
Amazon Pay as an example. Please also test other payment gateways and remember to switch to EURO
_wcstripe_feature_amazon_payoption to yesChangelog entry
Changelog Entry Comment
Post merge