Classify sms errors #5012
Classify sms errors #5012
Conversation
| // This is not something we understand, check the status code to determine if it is a success | ||
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We should check if the error is produced by the json package. Please add tests to ensure this does not break legacy gateways.
There was a problem hiding this comment.
Originally we don't even try to read the response and only check the statusCode. If we want to ensure it behave the same as before we should still treat it as success even it is not a json parsing error.
|
I've addressed the comments above, but e2e tests are failing so I'm checking. |
|
This is ready for review, thanks! |
|
TODO:
|
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| responseBody, err := ParseResponseBody(jsonText) |
There was a problem hiding this comment.
If the existing SMS deno hook return {"code": 1}, the json parsing will fail, but we should still treat this as success. Please add a test case to show this.
There was a problem hiding this comment.
Currently, sms deno hook is not allowed to return anything. So existing deno hooks should return null.
There was a problem hiding this comment.
What do you mean by not allowed? I think the hook can return anything they want, given that they write enough type annotation to bypass type checking.
There was a problem hiding this comment.
You are right that the hook can return anything if the function return type is set to any. However I think existing hooks are configured by us so there should be no hook written using incorrect return type? In fact I am not sure if there is anyone using the deno sms hook after we have our sms gateway deployed?
| WebHook: webhook, | ||
| Client: &mockWebHookClient{ | ||
| ResponseStatusCode: 200, | ||
| ResponseBody: io.NopCloser(strings.NewReader("")), |
There was a problem hiding this comment.
Add a test case in which the response body is {"code": 1}.
- It should log the dispatch error instead of the send error, which already logged in other places.
- Only log the error if it is async. Sync errors are returned to the auth ui and logged there. - Async routines should always use his own transaction
|
Rebased and added b0e11ab for deno hook backward compatibility. |
ref DEV-2454
Test with authgear/authgear-sms-gateway#23