-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass previous Exception with Exception #8824
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: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
Hi @hsingyuc Thanks for working on this. So, in order to work around this issue, this PR depends on a core change in WooCommerce that retrieves the error class and sends it through the What are your thoughts of conditionally throwing the exception in Untested code:
|
If I understood you correctly, that's the reason for this PR to log the Exception class so it will be easier for us to find where the error comes from.
I agree with you that it would take some time. I think retrying orders and auto refunding gives us time to solve the other issues and If we had merged the previous PRs, we would have the information to find the root causes of this issue by now. I believe we can and should both add additional data and look for another faster solution this time.
Wouldn't this error message be shown to shoppers? We can try to check if it's WooPay and then use |
@hsingyuc No, it won't be shown to the shopper. It will just be logged on WooPay logs. WooPay shows a custom error to the shopper instead of showing the error from the merchant site as-is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @hsingyuc, I've tested this with the WC-core PR and was able to get the class-name on to WooPay error logs, which will help narrow down merchant-store errors WooPay shoppers encounter.
If we had merged the #7873 PRs, we would have the information to find the root causes of this issue by now.
I disagree. The PR you linked sends intent_meta_order_id
and order_id
via additional_data
, same information is currently available to us via the changes in #7951.
I do agree that, had we gone ahead with your core changes earlier, it would help us with the additional logs we are adding now :)
@malithsen, thank you for the review!
Sorry, I meant to link to the WC PR that allows us to pass additional data to exceptions.. This would mean that a number of WooCommerce stores already allow additional data in the Store API responses so we could try and diagnose some of these errors. |
Changes proposed in this Pull Request
While investigating successfully captured and failed orders root causes, I found we could use more information on error 'We're not able to process this request. Please refresh the page and try again.'
Changes made in this PR:
Exception
.Process_Payment_Exception
inside theprocess_payment
to more unique Exceptions likeFraud_Prevention_Enabled_Exception
,Invalid_Phone_Number_Exception
, andRate_Limiter_Enabled_Exception
so when WC catches the Exception that throws fromprocess_payment
, we can pass the Exception class as additional data with the Exception to help WooPay better understanding where the errors come from.Testing instructions
true
in check_nonceprocess_payment
:( I tested them by commenting out the condition check )
{YOUR_DOMAIN}/wp-json/wc/store/v1/cart/add-item
npm run changelog
to add a changelog file, choosepatch
to 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.Post merge