Skip to content

LG-8788: USPS controller, refactor error handling #7812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
allthesignals merged 1 commit into from
Feb 13, 2023
Merged

LG-8788: USPS controller, refactor error handling #7812

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 34 additions & 31 deletions app/controllers/idv/in_person/usps_locations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,42 +12,27 @@ class UspsLocationsController < ApplicationController

before_action :confirm_authenticated_for_api, only: [:update]

rescue_from Faraday::TimeoutError,
Faraday::BadRequestError,
Faraday::ForbiddenError,
StandardError,
with: :handle_error

# retrieve the list of nearby IPP Post Office locations with a POST request
def index
response = []
begin
if IdentityConfig.store.arcgis_search_enabled
candidate = UspsInPersonProofing::Applicant.new(
address: search_params['street_address'],
city: search_params['city'], state: search_params['state'],
zip_code: search_params['zip_code']
)
response = proofer.request_facilities(candidate)
else
response = proofer.request_pilot_facilities
end
render json: response.to_json
rescue Faraday::TimeoutError, Faraday::BadRequestError, Faraday::ForbiddenError => err
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,
)
render json: {}, status: :unprocessable_entity
rescue => err
analytics.idv_in_person_locations_request_failure(
api_status_code: 500,
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,
if IdentityConfig.store.arcgis_search_enabled
candidate = UspsInPersonProofing::Applicant.new(
address: search_params['street_address'],
city: search_params['city'], state: search_params['state'],
zip_code: search_params['zip_code']
)
render json: {}, status: :internal_server_error
response = proofer.request_facilities(candidate)
else
response = proofer.request_pilot_facilities
Copy link
Copy Markdown
Contributor

@eileen-nava eileen-nava Feb 13, 2023

Choose a reason for hiding this comment

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

(I know this PR was already merged - I was looking it over while reviewing the refactor of the address search controller and had a question while doing that.)

Why are we returning the pilot facilities here? What's the use case where a user would see this response? Since we've now rolled out nationwide, I'm unclear on when a user would hit this scenario.

Copy link
Copy Markdown
Contributor Author

@allthesignals allthesignals Feb 13, 2023

Choose a reason for hiding this comment

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

Yeah, that's a good question, I don't know haha. I want to remove it all but wasn't sure if product wanted it for some reason.

Copy link
Copy Markdown
Contributor

@eileen-nava eileen-nava Feb 13, 2023

Choose a reason for hiding this comment

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

Cool! We can check in with product about this. I'll ask in slack tomorrow.

end

render json: response.to_json
end

def proofer
Expand All @@ -66,6 +51,24 @@ def update

protected

def handle_error(err)
remapped_error = {
Faraday::TimeoutError => :unprocessable_entity,
Faraday::BadRequestError => :unprocessable_entity,
Faraday::ForbiddenError => :unprocessable_entity,
Comment on lines +56 to +58
Copy link
Copy Markdown
Contributor Author

@allthesignals allthesignals Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wish there were some dictionary of canonical HTTP errors that had things like status code and Rails' symbol. Rack::Utils has this but it's just a simple map. I was hoping Faraday error subclasses would have this info so I could just remap to the correct one!

But, I can't be sure that when Faraday::TimeoutError is instantiated, it's actually being instantiated with the correct response_body. If something times out, is it because the client gave up on waiting, or the server explicitly returned a timeout? 🤷 That's where we'd just lean on the resonse_body (and hash that!) but it seems like that's not always available...

}[err.class] || :internal_server_error

analytics.idv_in_person_locations_request_failure(
api_status_code: Rack::Utils.status_code(remapped_error),
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,
)
render json: {}, status: remapped_error
end

def confirm_authenticated_for_api
render json: { success: false }, status: :unauthorized if !effective_user
end
Expand Down