Skip to content
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

New error handling method #42

Merged
merged 31 commits into from Sep 18, 2022
Merged

New error handling method #42

merged 31 commits into from Sep 18, 2022

Conversation

Keivan-sf
Copy link
Collaborator

@Keivan-sf Keivan-sf commented Sep 17, 2022

resolves #23

I've separated errors into 3 different exception objects such as discussed in #23 :

  • BadConfigError: Denotes an error caused by wrong types or configurations provided by the developer. It could be an error from IPG (e.g. wrong API key) or it could be caught in the application itself (e.g. amount must be a number)
  • UserError: Denotes an error caused by the end user due to a behavior. (e.g. cancelling the transaction)
  • GatewayFailureError: Denotes an error either caused by a failure from gateway or an unrecognizable reason. (e.g. internal gateway error)

All errors extend an abstract class called MonopayError which offers some properties:

  • message ?: It's a string|undefined property which contains the error message if there is any
  • isIPGError: It's a boolean property which shows whether the error was thrown from the IPG or the application itself
  • isSafeToDisplay: it's a boolean property showing whether the error message is ok to be shown to the end user or not

For the drivers that lack error codes, exceptions that are mostly thrown are GatewayFailureError's. Due to them being unrecognizable.

TODO

  • In IPG's documentations, some errors are just called Payment failure. I'm having trouble categorizing them and I think I've been inconsistent about them in code as well. We need to make a decision about them. My initial idea is we count them as GatewayFailure's. Because we probably can't know if it was the IPG that caused the error or the user. It just seems unrecognizable
  • Documentations need to be updated
  • We might want to mention IPG error codes in the message. So the developers could easily follow along with the error in the official IPG documentation

Copy link
Owner

@alitnk alitnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job so far. Some new thoughts I had:

  • When the error is thrown because of We could add a underlying IPG code like error.underlyingErrorCode that has the error code from IPG. So in addition of isIPGError being true, we add a underlyingErrorCode. Could call it underlyingErrorCode, ipgErrorCode or just errorCode... yours to decide - we can change the name before the release anyways.
    Update: just noticed you mentioned this in the PR description - my bad.
  • What do you think about making the error names more explicit?
    • BadConfigError => BadGatewayConfigError or BadIPGConfigError
    • UserError => UserPaymentError
    • Not sure about this one: PaymentFailureError could be PaymentFaultError as well but let's see what you think
    • OR we could just categorize them by fault: ConfigFaultError, UserFaultError and GatewayFaultError - again, not really sure about this one either but let's talk about it

@Keivan-sf
Copy link
Collaborator Author

Keivan-sf commented Sep 17, 2022

I've had the same thoughts about error codes being in a separate field as well. However it might lead to some confusion. For example if a developer sees both isIPGError and IPGErrorCode , they would assume the IPGErrorCode will be always available when isIPGError is true. Which is clearly not the correct approach in so many cases (e.g. internal errors , lack of error codes etc.)
But on the other hand developers might want to make decisions based on them so it's really beneficial if we could somehow provide them. I thought maybe we could have an IPGResponse but we might get an empty response in some cases as well. Also it's an extra interface to deal with

I'm kinda unsure what we should do but I think providing a string|undefined field like errorCode is a good choice.
If you're having the same thoughts I shall get on it

@alitnk
Copy link
Owner

alitnk commented Sep 17, 2022

That sounds good - Maybe we can add a jsdoc note and make these concerns clear

All error constructors now use an object instead of multiple parameters. This has been done in order to simplify the constructor calls
@alitnk
Copy link
Owner

alitnk commented Sep 18, 2022

lgtm!

@alitnk alitnk merged commit 4931574 into v2 Sep 18, 2022
@Keivan-sf Keivan-sf deleted the error-handling branch September 18, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants