LG-8582: Implementation of temp 500 error on PO search

[eileen-nava]

🎫 Ticket

LG-8582: Implementation of temp 500 error on PO search

🛠 Summary of changes

• Display 500 error message when a request to the USPS API times out. Don't display any locations.
• Display 500 error when a request to the USPS API receives a non-timeout error. Don't display any locations.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Check out the code locally and add raise Faraday::TimeoutError to line 25 of app/controllers/idv/in_person/usps_locations_controller.rb, after response = proofer.request_facilities(candidate)
• OR block network requests to the url localhost:3000/verify/in_person/usps_locations (I included a screen recording of both approaches)
• Search for an address, like 1600 Pennsylvania Ave NW
• Observe the internal server error displayed above the heading

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Testing with a Faraday::TimeoutError in the controller:

InternalServerError.mov

Testing with blocked network requests to `localhost:3000/verify/in_person/usps_locations` :

InternalServerErrorBlockedURL_NoAudio.mov

[allthesignals]

I just have some questions. I'm not too sure what's relevant for review in the JS test spec for the IPP component. Biggest concern probably is important SWRConfig from 'swr/_internal'

[allthesignals]

In order to use SWR's error feature, we need to have the promise reject.

[eileen-nava]

Thanks for the feedback! 👍🏻 I refactored in response.

[allthesignals]

Your test "displays a 500 error if the request to the USPS API throws an error" seems to include some unnecessary code that might misdirect other readers... I would remove that and just see if you can get away with that test file's DEFAULT_PROPS at the top.

Also: this is awesome!!!

[aduth]

It'd be good to have spec coverage for the new behaviors here:

• Non-200 status code responses throw
• What happens if the response isn't JSON? I might wonder if it'd be better to avoid using JSON as the error text. Also worth noting that if uncaught, these would be logged to NewRelic, and depending on what's returned from the endpoint JSON, it could include sensitive details we wouldn't want logged (not a problem in this case)

[allthesignals]

@aduth thanks! I'll open a ticket for added coverage.

What happens if the response isn't JSON? I might wonder if it'd be better to avoid using JSON as the error text.

What's the standard approach to this? Is it better to assume text because other errors return as text? Some of it is API design — should our API return JSON error bodies? Or text? Etc.

Also, re: NewRelic, do you mean... the .json() parse attempt would fail, uncaught, and log potentially sensitive erro text ("Could not parse token someone's special info\ ")?

[aduth]

What's the standard approach to this? Is it better to assume text because other errors return as text? Some of it is API design — should our API return JSON error bodies? Or text? Etc.

I guess my first question is what details do we want from what's thrown? It doesn't look like we really care what the detail is at the moment, since we're not using it. I think we could even go as far as removing the parameter altogether. But otherwise I think it could be fine to use a general text that's descriptive enough of what happened (e.g. something like 'Request response status was unsuccessful' 🤷 )

Also, re: NewRelic, do you mean... the .json() parse attempt would fail, uncaught, and log potentially sensitive erro text ("Could not parse token someone's special info\ ")?

I mean if the API responds with anything other than 200 status code, but sent a JSON payload like...

{ ssn: '666-12-1234' }

...we'd risk logging that in NewRelic.

I think it can also be helpful to assign a generic name for the error so that we can aggregate it more easily (i.e. otherwise how would we know how often this line of code is throwing?).