Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Ensure shopper saved card is used as default payment method (default was being overwritten in some circumstances) #3131

Merged
merged 3 commits into from
Sep 10, 2020
Merged

Ensure shopper saved card is used as default payment method (default was being overwritten in some circumstances) #3131

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4a87bd
bail out of setting a default payment method if shopper has saved card
haszari Sep 9, 2020
08642f8
ensure default payment method is only set as active if necessary:
haszari Sep 9, 2020
8f9a396
remove unnecessary remnant hook dependency 🧟‍♀️
haszari Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions assets/js/base/context/cart-checkout/payment-methods/payment-method-data-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,11 @@ export const PaymentMethodDataProvider = ( { children } ) => {
return;
}

// Customer has saved card/payment methods - no need to set a default.
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 (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -356,6 +361,7 @@ export const PaymentMethodDataProvider = ( { children } ) => {
paymentMethodsInitialized,
paymentData.paymentMethods,
setActivePaymentMethod,
customerPaymentMethods,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dependency needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and it looks like it hasn't been needed for a while! Removed it and retested (just in case) 👍

] );

// emit events.
Expand Down