Skip to content

LG-8776: Change 500 response code to 422 for expected USPS failures#7726

Merged
sheldon-b merged 3 commits intomainfrom
sbachstein/lg-8776-change-500-errors-to-4xx-errors-to-prevent-pages
Jan 30, 2023
Merged

LG-8776: Change 500 response code to 422 for expected USPS failures#7726
sheldon-b merged 3 commits intomainfrom
sbachstein/lg-8776-change-500-errors-to-4xx-errors-to-prevent-pages

Conversation

@sheldon-b
Copy link
Contributor

@sheldon-b sheldon-b commented Jan 30, 2023

🎫 Ticket

LG-8776

🛠 Summary of changes

Update the POST /verify/in_person/usps_locations endpoint so that common USPS failures result in a 422 response code to the front end instead of 500. This is to prevent appdev on-call from being paged. There are so few requests to the endpoint that a very small number of common failure types cause the on-call engineer to be paged.

We attempt to rescue common USPS errors and return a 422 for them which won't cause the on-call engineer to be paged. Other types of errors will continue to raise a 500 and cause a page. I found these types of failures when looking in the prod analytics events logs over the past 6 days:

count exception status code reason
16 bad request 400 access denied/Failed to establish a backside connection
4 timeout
3 forbidden 403 access denied/authentication rejected

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Make sure that your config values are set correctly:
    • arcgis_search_enabled: true
  • Update the app/controllers/idv/in_person/usps_locations_controller.rb controller, line 24, to raise the kind of error you want to test
    • Screen Shot 2023-01-30 at 3 34 58 PM
  • Navigate to the /verify/doc_auth/document_capture#location page and submit any address to simulate a failure

👀 Screenshots

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

User experience of timeout error (unchanged)

localhost_3000_verify_doc_auth_document_capture (2)

Sheldon Bachstein added 3 commits January 30, 2023 13:21
Still needs some work on the error responses when there are unexpected
USPS failures
changelog: Internal, In-person proofing, stop trigger on-call paging for
expected usps facilities failures
@sheldon-b sheldon-b marked this pull request as ready for review January 30, 2023 20:42
@sheldon-b sheldon-b requested review from a team, eileen-nava and jess-fortier January 30, 2023 20:42
Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

Good work. Approved!

Comment on lines +30 to +37
analytics.idv_in_person_locations_request_failure(
api_status_code: 422,
exception_class: err.class,
exception_message: err.message,
response_body_present: err.respond_to?(:response_body) && err.response_body.present?,
response_body: err.respond_to?(:response_body) && err.response_body,
response_status_code: err.respond_to?(:response_status) && err.response_status,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was an urgent change but I wonder if we can leverage something inside of Analytics class that pulls out relevant info from what looks like a pretty standard interface for this err variable.

I'm hoping for some "decorator" style patterns (or something) to help prevent bloat in these controller methods. Most of the code here is error handling :)

Happy to open a ticket/refactor later if that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allthesignals I like that idea -- will you create a ticket for it? We do some similar things in the proofing results job and I wonder if there's a better/more general way to do it, but a way that prevents PII from being logged

Copy link
Contributor

Choose a reason for hiding this comment

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

rescue_from looks interesting.

@allthesignals I like that idea -- will you create a ticket for it? We do some similar things in the proofing results job and I wonder if there's a better/more general way to do it, but a way that prevents PII from being logged

Sure thing!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that's a great idea. Thanks @allthesignals! 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

@sheldon-b sheldon-b merged commit 34dea94 into main Jan 30, 2023
@sheldon-b sheldon-b deleted the sbachstein/lg-8776-change-500-errors-to-4xx-errors-to-prevent-pages branch January 30, 2023 21:06
mitchellhenke pushed a commit that referenced this pull request Jan 30, 2023
…7726)

* Begin returning 422 for expected usps failures

Still needs some work on the error responses when there are unexpected
USPS failures

* Annotate which requests are expected

* Remove todo comment

changelog: Internal, In-person proofing, stop trigger on-call paging for
expected usps facilities failures
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.

4 participants