feat(api): Introduce new error #3215
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
julienbourdeau
force-pushed
the
feat/payment-provider-error
branch
from
1af5bd2 to
41c6850
Compare
There was a problem hiding this comment.
Nice to put this topic on the table. I would just suggest to build a dedicated way to handle errors for all providers rather than adding a new logic just for payment providers.
My dream is to have a single way to handle failures for integrations:
- Webhooks (with have 10 differents error webhooks, with different signatures...)
- Error storing for display purpose in the front-end or the API, like it has been done for tax providers
- API responses for sync actions
- ...
Happy to discuss about it :)
| @@ -93,6 +93,20 @@ def initialize(result, message:) | |||
| end | |||
| end | |||
|
|
|||
| class PaymentProviderFailure < FailedResult | |||
There was a problem hiding this comment.
Since we are relying on many integration, I would suggest to rename the ThirdPartyFailure into IntegrationFailure and allow it to expose the initial error from the provider.
In many places in the project we have different ways of dealing with provider error, I'm not sure that adding a new one will simplify it 😄
| @@ -203,6 +203,21 @@ def deliver_error_webhook(stripe_error) | |||
| ) | |||
| end | |||
|
|
|||
| def fail!(stripe_error) | |||
| result.fail_with_error!(PaymentProviderFailure.new( | |||
There was a problem hiding this comment.
To keep the "easy" way, you could rely on result.provider_error!(...). That way, no need to build yourself the instance of the error at this stage
| @@ -84,7 +84,7 @@ def generate_checkout_url(send_webhook: true) | |||
| result | |||
| rescue ::Stripe::InvalidRequestError, ::Stripe::PermissionError => e | |||
| deliver_error_webhook(e) | |||
| result | |||
| fail! e | |||
There was a problem hiding this comment.
Oups... Indeed, this one in this scope should not be swallowed... 👍
There was a problem hiding this comment.
I'll fix this separately so we can get it fixed today
Context
As I was working on adding crypto, I realized if the checkout URL failed, we just return an 200 response where the checkout url is incorrect.
Description
I sometimes find that the API swallow to many errors. You receive a webhook with all the details but the API will simply return a cryptic error code.
Here I'm thinking about adding a new kind of failure for Payment Provider.
A contributor recently introduced
thirdparty_errorsbecause there was no easy way to return error from another service. I don't like the name too much, we usually use "integrations" or "provider" rather than third parties but more importantly I think it's too limited to return only a message.Please share anything you have in mind. I'm not too sure we should merge the PR in this state but I'd like the API to return more errors from payment provider.
Error before
Error after