Properly handle errors for SimpleFIN. #375
Conversation
psybers
changed the title
psybers
changed the title
|
Related actualbudget/actual#2891 |
| if (accessKey == null || accessKey === 'Forbidden') { | ||
| const token = secretsService.get(SecretName.simplefin_token); | ||
| if (token == null || token === 'Forbidden') { | ||
| throw new Error('No token'); |
There was a problem hiding this comment.
💬 suggestion: (comment applicable to all instances of thow within this file)
Would you mind either adding a prefix to the message OR creating a new Error class? If these errors happen - they will be printed in the error log. At which point it's easier to follow them if a bit more context is added.
| throw new Error('No token'); | |
| throw new Error('SimpleFIN: no token provided'); |
There was a problem hiding this comment.
Even if they are caught? LIne 44 below catches these errors and actually re-throws a different error (with a better log message).
There was a problem hiding this comment.
(Or more accurately, it catches these and then returns an error response to the client and does not throw anything.)
There was a problem hiding this comment.
Good comment. But this raises another point - could we have different error messages for each of the two different errors? The more granularity we have in error handling - the less support tickets we will get with confused users.
There was a problem hiding this comment.
In general, that is a good point. In this particular case though, if there is a problem with their access key or their setup token the solution is still the same: get a new setup token. So here I think a single error message makes most sense.
| const account = results.accounts.find((a) => a.id === accountId); | ||
|
|
||
| const needsAttention = results.errors.find( | ||
| (e) => e === `Connection to ${account.org.name} may need attention`, |
There was a problem hiding this comment.
Someone pointed me to this PR that now surfaces errors to users. This is great! Seeing this string matching, though, makes me think that SimpleFIN ought to return published error codes in addition to error messages. I'll try to make that happen soon.
There was a problem hiding this comment.
Great! Could you update us when/if you implement this in Simplefin? <3
There was a problem hiding this comment.
Someone pointed me to this PR that now surfaces errors to users. This is great! Seeing this string matching, though, makes me think that SimpleFIN ought to return published error codes in addition to error messages. I'll try to make that happen soon.
@iffy Thanks! Honestly what would be the most helpful is if the errors were contextualized to the account data itself. So then you would know which error matches which specific account(s).
There was a problem hiding this comment.
That's a good idea. I'll ping you when it happens.
This PR tries to provide better error handling for SimpleFIN. First, sometimes the calls were never returning a response (they just
returnfrom the method) so this fixes that issues, which notifies the client side that something happened.Second, it tries to provide better error messages on the client side.