Show only the current user's payment methods when editing subscriptions #11143
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
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. |
Contributor
|
Size Change: +1.27 kB (0%) Total Size: 877 kB
ℹ️ View Unchanged
|
…yment-methods-instead-of
…yment-methods-instead-of
…yment-methods-instead-of
frosso
reviewed
Nov 26, 2025
Contributor
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?
| userId = newUserId; | ||
| render(); | ||
|
|
||
| // Looaded, loading, or errored out, we do not need to load anything. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Looaded, loading, or errored out, we do not need to load anything. | |
| // Loaded, loading, or errored out, we do not need to load anything. |
| const internalCallback = () => | ||
| callback( parseInt( customerUserSelect.value, 10 ) || 0 ); | ||
|
|
||
| // Add the listner with the right technique, as select2 does not emit <select> events. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Add the listner with the right technique, as select2 does not emit <select> events. | |
| // Add the listener with the right technique, as select2 does not emit <select> events. |
Comment on lines
+16
to
+19
| export interface Token { | ||
| tokenId: number; | ||
| displayName: string; | ||
| } |
Contributor
There was a problem hiding this comment.
(minor) the Token type should also have a isDefault boolean, since I see it in render_custom_payment_meta_input. Although it doesn't seem to be used by the JS.
Comment on lines
+6
to
+11
| /** | ||
| * Props for the WCPayPaymentMethodElement component | ||
| */ | ||
| export interface PaymentMethodElementProps { | ||
| element: HTMLSpanElement; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| /** | |
| * Props for the WCPayPaymentMethodElement component | |
| */ | |
| export interface PaymentMethodElementProps { | |
| element: HTMLSpanElement; | |
| } |
This doesn't seem to be used anywhere
Comment on lines
+41
to
+43
| const customerUserSelect = document.getElementById( | ||
| 'customer_user' | ||
| ) as HTMLSelectElement | null; |
Contributor
There was a problem hiding this comment.
(minor) rather than using as, we can check the type returned by the getElementById. This way we don't need to force the casting of the type.
Suggested change
| const customerUserSelect = document.getElementById( | |
| 'customer_user' | |
| ) as HTMLSelectElement | null; | |
| const element = document.getElementById( 'customer_user' ); | |
| const customerUserSelect = | |
| element instanceof HTMLSelectElement ? element : null; |
Comment on lines
+195
to
+198
| const input = element.querySelector( 'select,input' ) as | ||
| | HTMLSelectElement | ||
| | HTMLInputElement | ||
| | null; |
Contributor
There was a problem hiding this comment.
(minor) similar to above - we can check the returned type, instead of forcing it.
Suggested change
| const input = element.querySelector( 'select,input' ) as | |
| | HTMLSelectElement | |
| | HTMLInputElement | |
| | null; | |
| const input = element.querySelector( 'select,input' ); | |
| if ( ! input ) { | |
| return; | |
| } | |
| if ( | |
| ! ( | |
| input instanceof HTMLSelectElement || | |
| input instanceof HTMLInputElement | |
| ) | |
| ) { | |
| return; | |
| } |
Comment on lines
+273
to
+277
| document | ||
| .querySelectorAll( '.wcpay-subscription-payment-method' ) | ||
| .forEach( ( element ) => { | ||
| setupPaymentSelector( element as HTMLSpanElement, cache ); | ||
| } ); |
Contributor
There was a problem hiding this comment.
(minor) to remove the forced casting
Suggested change
| document | |
| .querySelectorAll( '.wcpay-subscription-payment-method' ) | |
| .forEach( ( element ) => { | |
| setupPaymentSelector( element as HTMLSpanElement, cache ); | |
| } ); | |
| document | |
| .querySelectorAll< HTMLSpanElement >( | |
| '.wcpay-subscription-payment-method' | |
| ) | |
| .forEach( ( element ) => { | |
| setupPaymentSelector( element, cache ); | |
| } ); |
|
|
||
| if ( ! customerUserSelect ) { | ||
| return (): void => { | ||
| // No-op cleanup function when element is not found |
Contributor
There was a problem hiding this comment.
( ⛏️ )
Suggested change
| // No-op cleanup function when element is not found | |
| // No-op cleanup function when an element is not found |
Comment on lines
+11
to
+17
| // TypeScript declaration for jQuery | ||
| declare const jQuery: ( | ||
| selector: any | ||
| ) => { | ||
| on: ( event: string, handler: () => void ) => void; | ||
| off: ( event: string, handler: () => void ) => void; | ||
| }; |
Contributor
There was a problem hiding this comment.
Is this needed? I don't see TS blowing up if it gets removed 🤷
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.