search

alphagov / notifications-net-client

.NET client for the GOV.UK Notify API
https://www.nuget.org/packages/GovukNotify/
MIT License
25 stars 20 forks source link

Convert http errors to json #167

Open IamAlecYoung opened 1 year ago

IamAlecYoung commented 1 year ago

What problem does the pull request solve?

When the Notify service throws a 'NotifyClientException' Exception, the returned string is unparsable due to inconsistent json formatting.

Examples below:

{"Status code 400. Error: {\"errors\":[{\"error\":\"BadRequestError\",\"message\":\"Can\u2019t send to this recipient using a team-only API key\"}],\"status_code\":400}\n, Exception: Status code 400. The following errors occured [\r\n {\r\n \"error\": \"BadRequestError\",\r\n \"message\": \"Can’t send to this recipient using a team-only API key\"\r\n }\r\n]"}

{"Status code 400. Error: {\"errors\":[{\"error\":\"ValidationError\",\"message\":\"phone_number Not enough digits\"}],\"status_code\":400}\n, Exception: Status code 400. The following errors occured [\r\n {\r\n \"error\": \"ValidationError\",\r\n \"message\": \"phone_number Not enough digits\"\r\n }\r\n]"}

Change converts output from BaseClient.HandleHTTPErrors to return a standard JSON object of type 'NotifyHTTPErrorResponse' to allow Json Deserialisation.

{"status_code":"400", "errors": [{\"error\":\"ValidationError\",\"message\":\"phone_number Too many digits\"}],"exception":null}

{"status_code":"400", "errors":[{\"error\":\"ValidationError\",\"message\":\"phone_number Not enough digits\"}],"exception":null}

Related issue thread: https://github.com/alphagov/notifications-net-client/issues/161

Checklist

symd-uk commented 1 year ago

Any way we can get this merged to master?

IamAlecYoung commented 1 year ago

Any way we can get this merged to master?

I'm not 100% sure if I missed a step in requesting the merge, @samuelhwilliams can you advise (Apologies if wrong person, I've seen you are quite active on this repo)

symd-uk commented 1 year ago

Any way we can get this merged to master?

I'm not 100% sure if I missed a step in requesting the merge, @samuelhwilliams can you advise (Apologies if wrong person, I've seen you are quite active on this repo)

Thank you for responding. Just realised, does this change mean the documentation will have to change to? https://docs.notifications.service.gov.uk/net.html