Don't throw an error when registering a payment method fails #3134
Conversation
Aljullu
added
status: needs review
type: bug
|
Size Change: -301 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Switching to console.error fixes the issue, but I'm thinking we should revisit our intentions here, and either make this a real feature, or remove this special support for admins.
There are two options as I see it:
- Silently hide the misconfigured payment method – i.e. same behaviour for admin and guest/customer. There are other ways for the merchant to figure out why their payment method is not working - this isn't a core responsibility of the block front-end code.
- Show a notice (to admins) informing them that there's a problem with one of their payment methods.
Option 2 seems preferable, based on the comments in the code, seems like it's what we intended/designed.
Option 1 seems like a fine fallback if we hit issues/complexity with other options. While we could help some merchants figure out what's wrong, it doesn't feel like an essential feature to me. Having said that, I'm not clear on our approach/policy for showing "editor"/merchant notices or guidance in the front end.
Note – if for some reason it's best to go with shipping this as a console.error, I don't see any issue with that (i.e. all the above is non blocking). But in that case please tweak the comments to make the intention clear 🔮
| throw new Error( | ||
| // This is only shown to admin users, so it's ok to print an error to the browser console. | ||
| // eslint-disable-next-line no-console | ||
| console.error( |
There was a problem hiding this comment.
Seems like we have a few lines of code just to throw up a console.error – is it worth it?
Later in the file, there's a comment about this info being presented as a notice. That makes sense to me – if we're going to add explicit support for admins of misconfigured sites, we might as well use notice to show in the UI.
|
Thanks for the review @haszari!
I thought it was the expected behavior to show a console error instead of a frontend notice when a payment method fails to load in the frontend, but tagging @nerrad for confirmation. In 8985a56 I updated this PR to use notices instead of a console error and I also updated the PR testing steps. We can easily revert these changes depending on the final decision taken. |
|
The intention with the thrown error was that when admins are logged in, the error boundary would catch the error. At one point I think it did, but somewhere along the way it stopped. So ideally I'd like to some investigation around why the error boundary isn't working to catch this error (we might have to implement the If not present (or removed somewhere along the way), we should ideally have an error boundary around the payment method section of the ui. |
|
Ah yes, the error boundary is used in the editor to display the error. I will move this PR back to in progress so I can investigate it further. |
Aljullu
force-pushed
the
fix/3125-dont-throw-on-payment-registration-error
branch
2 times, most recently
from
d2515c5
to
4faf8f5
Compare
|
Hmmm, I have been investigating this and I'm not sure if it ever worked as described. If it did, I couldn't find when/how. The issue seems to be that we are throwing inside a What they do is to enable all payment methods for admin users, so if one is failing, the error will be caught by the error boundary and shown as a notice. That works mostly fine, but I needed to add a filter to hide saved payment options for methods that had thrown an error, because by default they are unfiltered. I'm not a big fan of this approach since it requires some special logic for a specific use case (admin user), but I can't think of any better approach (ideas welcome). I split the commits in self-contained changes, so it might be easier to review commit-by-commit than all at once. I also updated the testing instructions in the PR description. |
Valiant effort @Aljullu !! 🙌😅 If this feature hasn't ever worked as intended, maybe it's better to keep this simple, and remove the feature (i.e. admins get the exact same behaviour as other users). This is not an introduced issue, and the impact is small – this is a small convenience to give admins a clue in the front end why Stripe is not visible (or another misconfigured payment gateway). We can always implement it later if there's a clear need for it. I haven't reviewed the recent code changes – I think we should clarify that we actually want/need this feature before investing any further time. @nerrad do you have thoughts on this – do we need to display an admin-only notice if a payment method fails init or is misconfigured?
My naive assumption when I heard about this feature was that there'd be a try/catch within the loop body, to protect against rogue payment methods. Something like this: // pseudocode!!!
// initialising payment methods and deciding whether they should be shown as tabs
foreach ( current in all_payment_methods ) {
try {
current.init();
if ( current.canMakePayment() ) {
addAvailablePaymentMethod( current );
}
}
catch ( error ) {
if ( userIsAdmin ) {
addNotice( { message: `Payment method ${ current.name } failed initialisation with error ${ error }` } );
}
}
}However, personally I don't think this is worth following up now, and there might be extra complexity getting the error handling right across components/hooks in a react context. |
|
Thanks for the feedback @haszari! Yeah, let's wait for @nerrad's thoughts in order to decide how to proceed.
There isn't but in some way React error boundaries work similarly (but in a React-ish way). So if an error happens inside a component, the error boundary catches it and handles it as required; in this case, showing a notice. Here, the error is thrown in the payment method components and caught by |
nerrad
force-pushed
the
fix/3125-dont-throw-on-payment-registration-error
branch
from
4faf8f5
to
73a19d1
Compare
Short answer yes. Long answer: Anything we can do to make it easier for merchant's to more easily see what's wrong (especially when there are potential configuration issues) is of benefit. As for why admin only, I don't think we want to expose faulty payment method options to shoppers or potentially expose sensitive info to shoppers given we have no control over the contents of error notices. So restricting display of these notices to only Admin's limits the potential problematic surface area. As for this feature never working, that may be the case, but the intention was still always to display useful error messages to admins of a store for troubleshooting configuration issues. @Aljullu, assuming this tests well (I haven't tested) I'm fine with rolling with this iteration for now. However, I've pulled from the 3.4.0 milestone to give it a bit more time on the main branch for our development cycle (which hopefully will surface any potential issues between releases). |
assets/js/base/components/payment-methods/payment-method-error-boundary.js
Show resolved
Hide resolved
There was a problem hiding this comment.
I found a bug with this - haven't yet done a full test/review yet sorry. Lots of code impact here so will need to review carefully!
The issue I found is that if COD is dependent on shipping method, it's always shown. It's supposed to hide (dynamically) if a non-COD-compatible shipping method is selected. I switched branch to main to confirm the correct behaviour – so confirming that the issue is introduced with these changes. It's a delicate balance getting these payment tabs working well.
Here's the relevant option in COD config:
Aaah, good catch! That was a consequence of loading all payment methods for admins, without checking what was returned by |
There was a problem hiding this comment.
Epic work on this @Aljullu - this is a really tricky issue and you're really getting to the heart of it. 🎖️ 🦖
I found another issue (sorry). And also note I've paused reviewing/testing because of the issue I hit - so this is not a full review.
Here's how to reproduce the issue:
- Enable COD and/or Cheque payment method.
- Add a
throwstatement incanMakePaymentfor COD or Cheque payment method. - View checkout block in front end.
Expected: see all valid payment methods as tabs, any saved cards as radio buttons, and a tab for the broken payment method (COD or cheque) with a notice.
Actual: no payment methods! Note this applies to admin and anon shopper.
Now .. why was I testing this? The most important part of this issue is that we want to ensure payment which throw errors are not able to break the checkout (i.e. other payment options should be unaffected).
I also saw another specific issue with Stripe (kind of a separate issue but related). If there's no key, it returns an api object with an error property, but our check for successful init is if the api object is truthy. We probably need to check for stripe.error and if so, throw the error for the notice.
return canPayStripePromise.then( ( stripe ) => {
// Need to check `stripe.error` here and if so throw the error for the notice.
if ( stripe === null ) {
isStripeInitialized = true;
return canPay;
}
// Do a test payment to confirm if payment method is available.
// This line is an error if API keys are missing, because stripe.paymentRequest is not a function.
const paymentRequest = stripe.paymentRequest( {
total: {
label: 'Test total',
amount: 1000,
},
country: getSetting( 'baseLocation', {} )?.country,
currency: currencyCode,
} );
return paymentRequest.canMakePayment().then( ( result ) => {
canPay = !! result;
isStripeInitialized = true;
return canPay;
} );
} );| const updateOptions = useCallback( async () => { | ||
| // Admin users have all payment methods enabled, so we need to filter | ||
| // the ones that do not accept payments. | ||
| const getFilteredPaymentMethods = async ( paymentMethods ) => { |
There was a problem hiding this comment.
This is a pretty big, complex change - I'm having trouble reviewing it to be honest!
It looks like this component now needs to manually replicate a lot of the core logic for calling canMakePayment, specifically for the admin use case. I think this is so the payment method tab is rendered (showing the error) but any saved cards for that payment method are not rendered. (Let me know if I've got that wrong!)
So to put it another way, we're mixing concerns/use cases here and that's led to this complexity. The two use cases are:
- Admin using the front end of the shop to buy stuff, as a shopper. If Stripe settings are broken, their (stripe) saved card should not be shown, because it wouldn't work.
- Admin using the front end to confirm that payment methods are set up correctly. If Stripe settings are broken, we should display a notice so admin can fix it.
First I think we need to clarify some of this in the PR description - it's taken me a while to get my head around it and it's an important aspect of the fix.
Second, I'm hoping that we can avoid having to "reimplement" the canMakePayment call in both places. This feels to me like it should be managed by the payment context (i.e. store/reducer state). I suspect we should be able to represent the state of the payment methods sufficiently in the store, so the component doesn't need to manually filter.
Right now the payment methods are either present in the store (as customerPaymentMethods) or not, if not available (e.g. due to canMakePayment() returning false. I think we need to represent the fact that admins see more payment methods in the store/context. So for example, store all payment methods in context, and have a property for whether they are functioning/available, and another property for if they have an error. Sorry I don't have a more specific suggestion here, I think this would take a bit of work but might be a more appropriate fix which is easier to maintain.
The key for me is that payment method has an error is real state that should be in the store, not computed inside component (UI). But I could be confused :)
There was a problem hiding this comment.
All good points, I agree calling canMakePayment in two different places was not ideal. In fact that was what was making me not to be really comfortable with that approach either.
I took another take at this issue. Instead of filtering saved payment methods with canMakePayment, now I filter them with the supports.savePaymentInfo property, which we now set to false if the payment failed to register.
I also considered adding another attribute like error or hasError to 'mark' the payment methods that failed to register but are still added because the user is an admin. But I feel setting savePaymentInfo to false is a more natural approach.
In addition to that, I needed to replace the content so the error is thrown in the component (and then caught by the error boundary). Let me know if this new approach looks good to you.
| @@ -167,7 +208,7 @@ const SavedPaymentMethodOptions = ( { onSelect } ) => { | |||
|
|
|||
| // In the editor, show `Use a new payment method` option as selected. | |||
| const selectedOption = isEditor ? '0' : selectedToken + ''; | |||
| return currentOptions.current.length > 0 ? ( | |||
| return currentOptions.current.length > 1 ? ( | |||
There was a problem hiding this comment.
I'm curious why this change - is it needed, does it make a difference, or is it just "more correct"?
I'm assuming that it works either way because we always have the Use a new method radio button, so it's 0 or 2+.
There was a problem hiding this comment.
That was part of the refactor in 73a19d1 (see related lines of code) in order to make it easier to understand. Thinking about it twice, I should have broken it in two different PRs in order to make it easier to review. Let me know if you want me to split it.
The idea behind the refactor was to remove the if clause from the effect and the push to the array stored in the ref. I was having a hard time understanding how they worked and felt I was simplifying it. But I might have achieved the opposite. 😅
I did another try to simplify this (e3ca65d), basically instead of appending the default option inside the effect, we append it in the render.
| return typeMethods.map( ( paymentMethod ) => { | ||
| const method = | ||
| standardMethods[ paymentMethod.method.gateway ]; | ||
| if ( ! method?.supports?.savePaymentInfo ) { |
There was a problem hiding this comment.
This block of code might be easier to review with Hide whitespace changes. The only change inside this useEffect which is not a refactor is this new check: if savePaymentInfo is not truthy, the saved payment methods are not appended (ie: if Stripe fails to load, saved credit cards will not be appended).
Before, that would be achieved because the payment method would not have been registered at all. But with the changes in this PR, all payment methods are registered for admins, so we need to add this extra check.
There was a problem hiding this comment.
Gotcha, thanks for the tip about Hide whitespace changes - much clearer now :)
|
While working on this PR I noticed an issue in |
There was a problem hiding this comment.
Excellent job @Aljullu !! 🙌
The new approach seems really robust to me – should work well as more payment methods add support for blocks. I'm curious too whether the new error-catching support in this PR will help us fix various edge-cases for payment errors (e.g. #2743).
I tested a variety of scenarios with admin, editor, and shopper roles, saved cards, various payment methods broken in different ways, and saved card option enabled/disabled, and didn't see any issues.
Ship it!!! 🎉 🚢 
| @@ -32,6 +32,9 @@ function paymentRequestAvailable( currencyCode ) { | |||
| isStripeInitialized = true; | |||
| return canPay; | |||
| } | |||
| if ( stripe.error && stripe.error instanceof Error ) { | |||
There was a problem hiding this comment.
This check can doesn't need to be too specific, e.g. could just check stripe.error (type doesn't matter). Or check the API we need is available, e.g. stripe.error || ! stripe.paymentRequest instanceof Function )
There was a problem hiding this comment.
(Not a blocker, just could be more robust, assume less about error behaviour.)
There was a problem hiding this comment.
I'm not sure about this one... Considering in the next line we call throw stripe.error, we need to ensure it's an instance of Error.
We could do something like:
if ( stripe.error ) {
if ( stripe.error instanceof Error ) {
throw stripe.error;
} else {
throw new Error( stripe.error );
}
}But I would like to ensure there is a case where that's needed before adding the extra logic.
| @@ -57,7 +42,8 @@ const handleRegistrationError = ( error ) => { | |||
| const usePaymentMethodRegistration = ( | |||
| dispatcher, | |||
| registeredPaymentMethods, | |||
| paymentMethodsSortOrder | |||
| paymentMethodsSortOrder, | |||
| noticeContext | |||
There was a problem hiding this comment.
Don't forget to document the new param in JSdoc!
| @@ -118,10 +99,25 @@ const usePaymentMethodRegistration = ( | |||
| throw new Error( canPay.error.message ); | |||
| } | |||
| addAvailablePaymentMethod( paymentMethod ); | |||
| removeNotice( | |||
There was a problem hiding this comment.
Does this happen – i.e. we have a payment method fail, show a notice, then on a later render succeed? Just curious about when this removeNotice is needed.
There was a problem hiding this comment.
Good point... The notice is only added on registration, so probably no need for the removeNotice. Removed this line in 93d37bc.
| // when a payment method fails to register. | ||
| if ( isEditor || CURRENT_USER_IS_ADMIN ) { | ||
| return ( | ||
| <StoreNoticesProvider context="wc/express-payment-area"></StoreNoticesProvider> |
There was a problem hiding this comment.
I see we have constants for these notice context ids now (useEmitResponse().noticeContexts) - would be good to consistently use the constant rather than hard-code. (Not a blocker)
There was a problem hiding this comment.
Absolutely! Changed in 3d5309e.
|
Thanks for all these reviews and helping me shape this up, @haszari. Really appreciated!
I didn't take a deep look at that issue. I suppose if the error is in |
…at throw an error
Aljullu
force-pushed
the
fix/3125-dont-throw-on-payment-registration-error
branch
from
93d37bc
to
7516608
Compare
Aljullu
deleted the
fix/3125-dont-throw-on-payment-registration-error
branch
Fixes #3125.
Fixes #3157.
This PR has some changes in how failing payment methods are handled: they will silently fail for shoppers, but a notice will be shown for admins.
How to test the changes in this Pull Request:
Excuses in advance for the long testing steps, this PR touches some logic that affects several critical flows so I tried to list the steps as detailed as possible.
Test #3125.
Preparation
Steps from #3125:
Tests
* In order to save a payment method with a user. Enable the WooCommerce Stripe plugin, set the keys and make a purchase with a user selecting the
Save payment information to my account for future purchases.option. Next time you visit the Checkout with that user, the saved payment method will show up.Test #3157
Assuming you already have a user with saved credit cards in Stripe from the steps above.
Enable Payment via Saved Cards. Make sure you have added back the API keys that you might have removed in the steps above.Regression testing
It would also be great to do a purchase flow with some of the scenarios described above to ensure the payment flow still works properly.
Changelog