Show only the current user's payment methods when editing subscriptions #11143
Conversation
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. |
|
Size Change: +1.22 kB (0%) Total Size: 878 kB
ℹ️ View Unchanged
|
…yment-methods-instead-of
…yment-methods-instead-of
…yment-methods-instead-of
frosso
left a comment
frosso
left a comment
There was a problem hiding this comment.
The pub/sub pattern on the token cache is a really cool concept; however I don't think it's necessary.
There is a lot of "manual" rendering happening, despite the implementation of PaymentMethodSelect as a ReactJS component.
The current approach is a bit "mixed", and seems to defeat the purpose of using ReactJS. React should handle re-rendering automatically through state changes, instead of manual function calls.
For example, I see that the onChange prop is drilled down to the select, but at every change it manually re-renders the whole root. It's kinda like ReactJS works, but re-invented :)
Here's a possible alternative.
Make the cache a global, it can just be a key-value-pair of userId and their tokens:
const cache: Record< number, Token[] > = {};
On the setupPaymentSelector, you can substitute most of the rendering/listening to just creating the root and rendering the PaymentMethodManager - which is a helper component around the PaymentMethodSelect:
const setupPaymentSelector = ( element: HTMLSpanElement ): void => {
const data = JSON.parse(
element.getAttribute( 'data-wcpay-pm-selector' ) || '{}'
) as WCPayPMSelectorData;
const initialUserId = data.userId ?? 0;
const initialValue = data.value ?? 0;
// Initial population of cache with pre-loaded tokens
if ( initialUserId && data.tokens ) {
cache[ initialUserId ] = data.tokens;
}
// In older Subscriptions versions, there was just a simple input.
const input = element.querySelector( 'select,input' ) as
| HTMLSelectElement
| HTMLInputElement
| null;
if ( ! input ) {
return;
}
const root = createRoot( element );
root.render(
<PaymentMethodManager
inputName={ input.name }
initialUserId={ initialUserId }
initialValue={ initialValue }
config={ data }
/>
);
};
Then, the PaymentMethodManager can take care of listening to DOM changes on the customer_user input and fetching data, providing the data to the select:
const PaymentMethodManager = ( {
inputName,
initialUserId,
initialValue,
config,
}: {
inputName: string;
initialUserId: number;
initialValue: number;
config: WCPayPMSelectorData;
} ) => {
const [ userId, setUserId ] = useState< number >( initialUserId );
const [ selectedToken, setSelectedToken ] = useState< number >(
initialValue
);
const [ isLoading, setIsLoading ] = useState< boolean >( false );
const [ loadingError, setLoadingError ] = useState< string >( '' );
// Handle customer select changes
useEffect( () => {
const updateSelectedToken = ( tokens: Token[] ) => {
const selectedTokenId = tokens.find(
( token ) => token.tokenId === selectedToken
)?.tokenId;
setSelectedToken( selectedTokenId || 0 );
};
return addCustomerSelectListener( async ( newUserId ) => {
setUserId( newUserId );
// Check if already in cache
const tokens = cache[ newUserId ];
// If already loaded, no need to fetch again.
if ( tokens ) {
updateSelectedToken( tokens );
return;
}
setIsLoading( true );
try {
const response = await fetchUserTokens(
newUserId,
config.ajaxUrl,
config.nonce
);
if ( undefined === response ) {
throw new Error(
__(
'Failed to fetch user tokens. Please reload the page and try again.',
'woocommerce-payments'
)
);
}
cache[ newUserId ] = response.tokens;
updateSelectedToken( response.tokens );
setLoadingError( '' );
} catch ( error ) {
setLoadingError(
error instanceof Error
? error.message
: __( 'Unknown error', 'woocommerce-payments' )
);
}
setIsLoading( false );
} );
}, [ config.ajaxUrl, config.nonce, selectedToken ] );
if ( loadingError ) {
return <strong>{ loadingError }</strong>;
}
if ( isLoading || ! cache[ userId ] ) {
return <>{ __( 'Loading…', 'woocommerce-payments' ) }</>;
}
return (
<select name={ inputName } defaultValue={ initialValue } key={ userId }>
<option value={ 0 } key="select" disabled>
{ __(
'Please select a payment method',
'woocommerce-payments'
) }
</option>
{ cache[ userId ].map( ( token ) => (
<option value={ token.tokenId } key={ token.tokenId }>
{ token.displayName }
</option>
) ) }
</select>
);
};
What do you think?
| // TypeScript declaration for jQuery | ||
| declare const jQuery: ( | ||
| selector: any | ||
| ) => { | ||
| on: ( event: string, handler: () => void ) => void; | ||
| off: ( event: string, handler: () => void ) => void; | ||
| }; |
There was a problem hiding this comment.
Is this needed? I don't see TS blowing up if it gets removed 🤷
There was a problem hiding this comment.
Removed in 4043b49. I thought it would be good to get some types there, but this is more than redundant.
There was a problem hiding this comment.
It looks like it came back :D
There was a problem hiding this comment.
Claude seems to be stubborn, and this one went under my radar... Removed again in 59b6b22, everything seems to still work well.
There was a problem hiding this comment.
And it's back again! 🙈
The front-end works fine, but tests are always failing without the declaration. I can't figure out a way around it.
…efinition, don't force element types.
- Convert UserTokenCache class tests to pure function tests (startLoading,
tokensLoaded, loadingFailed, hasEntry, getUserEntry, userHasToken,
getDefaultTokenId)
- Update PaymentMethodSelect tests to use new props (initialValue,
initialUserId, initialCache, nonce, ajaxUrl)
- Add immutability tests for cache functions
- Add jQuery type declaration to index.tsx for TypeScript compilation
- Adjust tests for new placeholder behavior (only shown when value === 0)
…yment-methods-instead-of
…instead-of' of github.com:Automattic/woocommerce-payments into woopmnt-5508-shows-all-customers-saved-payment-methods-instead-of
|
@frosso I did go a bit overboard with the cache as a separate class. I went back and combined some of your suggestions with a bit of cleaner logic. Previously, I had things in a state that was a tad too messy, and it was difficult for me to break things down into testable and graspable methods. The
Actually, that was due to me not knowing about the |
…yment-methods-instead-of
frosso
left a comment
frosso
left a comment
There was a problem hiding this comment.
It doesn't look like it's functional anymore - I think because of the condition on the select?
| // TypeScript declaration for jQuery | ||
| declare const jQuery: ( | ||
| selector: any | ||
| ) => { | ||
| on: ( event: string, handler: () => void ) => void; | ||
| off: ( event: string, handler: () => void ) => void; | ||
| }; |
There was a problem hiding this comment.
It looks like it came back :D
| // If there is no value but a user, try to fall back to the default token. | ||
| useEffect( () => { | ||
| if ( 0 !== userId && value === 0 ) { | ||
| const defaultTokenId = getDefaultTokenId( cache, userId ); | ||
| if ( value !== defaultTokenId ) { | ||
| setValue( value ); | ||
| } | ||
| } | ||
| }, [ userId, value, cache ] ); |
There was a problem hiding this comment.
| // If there is no value but a user, try to fall back to the default token. | |
| useEffect( () => { | |
| if ( 0 !== userId && value === 0 ) { | |
| const defaultTokenId = getDefaultTokenId( cache, userId ); | |
| if ( value !== defaultTokenId ) { | |
| setValue( value ); | |
| } | |
| } | |
| }, [ userId, value, cache ] ); | |
| // If there is no value but a user, try to fall back to the default token. | |
| useEffect( () => { | |
| if ( 0 !== userId && value === 0 ) { | |
| const defaultTokenId = getDefaultTokenId( cache, userId ); | |
| if ( value !== defaultTokenId ) { | |
| setValue( defaultTokenId ); | |
| } | |
| } | |
| }, [ userId, value, cache ] ); |
Given the description above the useEffect, should this have been the intent?
Also, is cache necessary as part of the dependencies?
There was a problem hiding this comment.
Given the description above the
useEffect, should this have been the intent?
Absolutely. I'm not sure how I let this through. Maybe there was some TS error in the test file, and I was actually looking at an outdated front-end, as this was one of the later changes. 🤔
I updated it to use defaultTokenId.
Also, is cache necessary as part of the dependencies?
Once tokens finish loading for a new user, we need to use the cache update to trigger the effect. userId and value are not directly changed when tokens get loaded.
| cachedData: CachedUserData, | ||
| userId: number | ||
| ): CachedUserDataItem | undefined => { | ||
| return cachedData.find( ( userData ) => userData.userId === userId ); |
There was a problem hiding this comment.
Have you considered implementing the cache as a key-value-pair (where the key is the user id) for faster O(1) access? It seems that we're always looking for an entry by its user id.
There was a problem hiding this comment.
Yes, that's a good idea. Done in 59b6b22.
| const value = data.value ?? 0; | ||
|
|
||
| // Initial population. | ||
| const cache: CachedUserData = []; |
There was a problem hiding this comment.
It looks like each instance of a .wcpay-subscription-payment-method is getting its own cache. And this array is also re-created as a useState down in the PaymentMethodSelect.
What do you think about having the cache as a global key-value-pair? Or it can also be a global variable in the client/subscription-edit-page/user-token-cache.ts file, so you don't need to pass it as an argument each time you want to call one of the utility functions. And you don't need to prop-drill it.
There was a problem hiding this comment.
At this stage, I find it arbitrary. I decided to go away from the global cache (it was never used in more than one instance anyway), and managing the cache in state made it relatively easier to break things down.
If I move to a global cache as an object to avoid having to pass it as an argument, I'll still need to contain loading and failure state management within the select. I feel like that brings me back to a bigger and unwieldier method, and I don't see a significant upside anyway.
I'll try to see how it works, and how much/what will need to still be passed around, but I'm not promising anything yet.
Nevermind, done. It's so much cleaner. Thank you for being persisent here!
Fixes WOOPMNT-5508
Changes proposed in this Pull Request
When editing subscriptions in WooCommerce admin, the payment method dropdown was showing all customers' saved payment methods instead of only the selected customer's methods for new subscriptions. For existing subscriptions, payment methods were not reloaded when the customer was changed. This PR fixes the issue by dynamically loading payment methods when the customer is changed.
Key changes:
UserTokenCacheclass to cache fetched tokens and manage loading/error stateswcpay_get_user_payment_tokens) to fetch payment tokens for a specific userHow can this code break?
Testing instructions
Note: You will see failing workflows, but they should not be related to the changes from this PR.
npm run changelogto add a changelog file, choosepatchto 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.