-
Notifications
You must be signed in to change notification settings - Fork 69
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
Allow the forceNetworkSavedCards
payment method setting to be overridden
#9460
Allow the forceNetworkSavedCards
payment method setting to be overridden
#9460
Conversation
…idden Allow the network saved card setting to be overriden by the `wcpay_force_network_saved_cards` filter
forceNetworkSavedCards
payment method setting to be overridden by wcpay_force_network_saved_cards
filterforceNetworkSavedCards
payment method setting to be overridden
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
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.
@romarioraffington The code change looks good, thanks! I wonder if testing steps for this PR should contain sanity checks within WooPayments plugin only, while the rest is done in https://href.li/?https://code.a8c.com/D161783. I also asked in https://a8c.slack.com/archives/C011RA2S7BJ/p1726733430075369
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.
Looked at this code once again and I have some thoughts. I tried to review the original change for this area, and it seems that forceNetworkSavedCards
is applicable only for cards. My question is whether we're OK with adding WC_Payments::is_network_saved_cards_enabled()
to payment methods other than card, which your change seems to do, especially in terms of potential side effects.
When using should_use_stripe_platform_on_checkout_page
before, it checks the payment method type to be card, while is_network_saved_cards_enabled
could potentially return true
for non-card methods as well, if network-saved cards are enabled. Given the complexity of this legacy feature (I also asked if anyone from WooPayments group possesses the knowledge for this feature these days: p1726739953413419-slack-CGGCLBN58), we might consider adding the check against the card payment method type either as part of $settings[ $payment_method_id ]['forceNetworkSavedCards']
assignment or as part of is_network_saved_cards_enabled
, right before applying the filter. Another option would be to make JS take forceNetworkSavedCards
from paymentMethodsConfig
for non-cards only, but take it from here in case of card payment method.
I agree! Sanity checks are always good. Do you have a link to another PR or docs with sanity checks to include? I'm not as familiar with WooPayments dev setup / testing.
You're right. It's best to error on the side of caution here and only allow the filter to override settings for cards. I'll make the improvements as suggested. Thanks! |
…network_saved_cards-filter
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.
Thanks for quick responses and for all the work so far @romarioraffington! I've left two related comments which should impact the functionality, I'm looking forward to hearing from you whether this makes sense to you.
In the meantime, I've tested WooPayments with the changes proposed in my comments (moving the payment method check to class-wc-payments-checkout
) and below are the results:
- Used
add_filter( 'wcpay_force_network_saved_cards', '__return_true' );
to returntrue
andfalse
consequently and check whether the correct result of theis_network_saved_cards_enabled()
method is taken into account for card payment method only ✅ - Performed a payment with credit card ✅
- Performed a payment and saved the credit card ✅
- Paid with a saved credit card ✅
- Paid with non-card payment methods - Bancontact and SEPA ✅
I also tested in sandbox per testing instructions and it works but there are two things to keep in mind:
- I tested your current version of the code from D161783-code which means that
is_network_saved_cards_enabled
returns the filter value for every payment method, not only cards - I assume D161783-code will be tested additionally by someone from your team as well, as you have more context and might catch up things that I could potentially miss
includes/class-wc-payments.php
Outdated
@@ -1544,7 +1544,7 @@ public static function remove_woo_admin_notes() { | |||
* @return bool Normal WCPay behavior (false, default) or TRUE if the site should only use network-wide saved payment methods. | |||
*/ | |||
public static function is_network_saved_cards_enabled() { | |||
return apply_filters( 'wcpay_force_network_saved_cards', false ); | |||
return Payment_Method::CARD === self::$card_gateway->get_selected_stripe_payment_type_id() && apply_filters( 'wcpay_force_network_saved_cards', false ); |
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 agree this makes sense, but unfortunately, get_selected_stripe_payment_type_id()
from the $card_gateway
will always return card
. I agree that semantically this method seems to be able to return the currently selected payment method, but it doesn't.
My proposal would be to keep is_network_saved_cards_enabled()
as it is, without changes, and move the check against card payment method to the place where we assign value to $settings[ $payment_method_id ]['forceNetworkSavedCards']
in class-wc-payments-checkout.php
@@ -332,7 +332,7 @@ public function get_enabled_payment_method_config() { | |||
'number' => '<button type="button" class="js-woopayments-copy-test-number" aria-label="' . esc_attr( __( 'Click to copy the test number to clipboard', 'woocommerce-payments' ) ) . '" title="' . esc_attr( __( 'Copy to clipboard', 'woocommerce-payments' ) ) . '"><i></i><span>', | |||
] | |||
); | |||
$settings[ $payment_method_id ]['forceNetworkSavedCards'] = $gateway_for_payment_method->should_use_stripe_platform_on_checkout_page(); | |||
$settings[ $payment_method_id ]['forceNetworkSavedCards'] = WC_Payments::is_network_saved_cards_enabled() || $gateway_for_payment_method->should_use_stripe_platform_on_checkout_page(); |
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.
As per another comment, I think it would be more safe to move the check here as shown below (or a similar syntax which achieves the same result):
$settings[ $payment_method_id ]['forceNetworkSavedCards'] = WC_Payments::is_network_saved_cards_enabled() || $gateway_for_payment_method->should_use_stripe_platform_on_checkout_page(); | |
$settings[ $payment_method_id ]['forceNetworkSavedCards'] = | |
'card' === $payment_method_id && | |
(WC_Payments::is_network_saved_cards_enabled() || | |
$gateway_for_payment_method->should_use_stripe_platform_on_checkout_page()); |
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.
Makes sense to me. Addressed in 2c840ae
For sure! |
…network_saved_cards-filter
Hey @romarioraffington and @timur27! I've tested this again from the WooPayments side.
I'd like to note that when
I think this is expected due to the nature of the change, with the payment method not being available on the platform account, the inverse of what prompted this issue. If it's OK, I'd like to confirm that with @timur27 before approving. |
Thanks for taking a look @mdmoore. Based on the error message above, my understanding is that the payment method is available on the platform account but the request was sent to the connected account. For TumblrPay, we expect the platform payment method to be passed to the backend and attached to the platform customer, but I suspect this is custom TumlbrPay logic. |
…network_saved_cards-filter
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.
The change looks good @romarioraffington, thanks for all your efforts! 🚀
Regarding
add_filter( 'wcpay_force_network_saved_cards', '__return_true' );
causing the error, which was rightly pointed out by @mdmoore - I also think this is expected because we're trying to use a payment method that belongs to the platform account (due to wcpay_force_network_saved_cards
returning true
) with a connected account's API request. My understanding is that this filter returns true
for Tumblrpay only, based on this original PR. I started the discussion about this flag in p1726739953413419-slack-CGGCLBN58 because it seems that this feature is not well known.
Fixes https://github.com/Automattic/payments-florin/issues/605
Relates to D161783-code and D161783-code
These changes were introduced in #7869
Changes proposed in this Pull Request
Allow the network saved card payment method setting to be overridden by the
wcpay_force_network_saved_cards
filter. See https://github.com/Automattic/payments-florin/issues/605 for additional context.Testing instructions
Testing instructions can be found in D161783-code
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge