Ensure shopper saved card is used as default payment method (default was being overwritten in some circumstances) #3131
Conversation
haszari
requested review from
senadir and
Aljullu
and removed request for
a team
|
Size Change: +16 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for investigating this issue and creating a fix. If I understand what's going on is that in SavedPaymentMethodOptions we are setting the saved payment method as the active payment option, but that was overridden in the PaymentMethodDataProvider useEffect. With this PR, we bail out soon if there are saved customer payment methods so the issue is fixed, is that correct?
Below I suggested an alternative approach that I think would work as well and is more generic because it doesn't handle the customer payment methods case specially. But I might have missed some special cases, so feel free to ignore if you don't think it will work.
Please also test a variety of other payment method scenarios and ordering - with / without saved cards (are there any other gateways that support saved payment methods?), with more/less gateways available, in different orders, and with dynamically-available gateways (COD can depend on shipping option).
I did a variety of tests and everything seemed to work alright. Seeing there are so many different scenarios is a good proof that it would be nice adding e2e tests at some point! 🙂
| if ( customerPaymentMethods ) { | ||
| return; | ||
| } | ||
|
|
||
| // If there's no active payment method, or the active payment method has | ||
| // been removed (e.g. COD vs shipping methods), set one as active. | ||
| if ( |
There was a problem hiding this comment.
There is an alternative approach that doesn't require the customerPaymentMethods dependency. It would involve using useState functional updates. Something like:
setActive( ( currentActivePaymentMethod ) => {
// If there's no active payment method, or the active payment method has
// been removed (e.g. COD vs shipping methods), set one as active.
if (
! currentActivePaymentMethod ||
! paymentMethodKeys.includes( currentActivePaymentMethod )
) {
return Object.keys( paymentData.paymentMethods )[ 0 ];
}
return currentActivePaymentMethod;
} );This would ensure currentActivePaymentMethod is evaluated right at the moment when the state is updated, so the payment method activated in SavedPaymentMethodOptions would already be active.
I didn't do a lot of testing of this approach, so feel free to ignore, but wanted to propose it in case you think it might be a good alternative.
There was a problem hiding this comment.
Smart! I like this, and of course since it's more general it avoids the issue with my truthy test below. Thanks for picking up on this, much better fix 🚀
The crux of the issue is that we were previously using a stale copy of currentActivePaymentMethod; so it makes sense that we'd fix this by using the functional form of setState. The principle to keep in mind here (just reminding myself!) - if a state update depends on the previous value, use functional form.
I've made the change and I'll do some more testing of a few scenarios. Note the other change from the sketch above is the reset of checkout status to PRISTINE when applying the default.
There was a problem hiding this comment.
Actually I think I found an issue. If I open the Checkout page in a private window (incognito), the first tab is not selected. That happens because customerPaymentMethods is an (empty) object and evaluates to true so the effect returns too early.
Can you reproduce that too?
- use functional form of setState; previous state (current payment method) is not potentially stale value - so the default is only set if there really is no active payment method
haszari
changed the title
|
I've tested the new approach (functional @Aljullu ready for another review! |
There was a problem hiding this comment.
Thanks for applying the feedback. I did another round of testing and everything looked good. I just added a small comment about an effect dependency that I guess can be removed, but pre-approving. 👍
Travis tests are failing, but I don't think it's related to this PR. I will spend some time today investigating what's going on with them.
| paymentMethodsInitialized, | ||
| paymentData.paymentMethods, | ||
| setActivePaymentMethod, | ||
| setActive, | ||
| customerPaymentMethods, |
There was a problem hiding this comment.
Is this dependency needed?
There was a problem hiding this comment.
No, and it looks like it hasn't been needed for a while! Removed it and retested (just in case) 👍
|
Merging this (with tests failing) as I believe the test failure is unrelated - see #3139 |
Fixes #3120
The issue here is that when a user has saved payment method(s) - e.g. a Stripe saved credit card (token), the logic that sets an initial default payment method can overwrite the saved payment method.
This PR fixes this by adding a guard to the default / init effect. If there's a saved payment method, it now exits early; previously was overwriting the saved payment method, causing #3120.Update: this PR now uses the functional form of
setStateto apply the default payment method. This ensures that the default is only set if there is no payment method active (selected); previously this decision was made using a stale copy of state. Hat tip @Aljullu for the suggestion 🎉How to test the changes in this Pull Request:
WooCommerce > Settings > Payments).Save payment information to my account for future purchases.Should always complete the purchase with the correct payment method - the one visibly selected when user clicks submit, i.e. the saved card.
Please also test a variety of other payment method scenarios and ordering - with / without saved cards (are there any other gateways that support saved payment methods?), with more/less gateways available, in different orders, and with dynamically-available gateways (COD can depend on shipping option). In all cases confirm the following:
Changelog