Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 42 additions & 13 deletions app/controllers/idv/in_person/address_search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,58 @@ def index

protected

def api_status_from_error(err)
if err.instance_of?(Faraday::ClientError)
:bad_request
elsif err.instance_of?(Faraday::ConnectionFailed)
:bad_gateway
elsif err.instance_of?(Faraday::TimeoutError)
:gateway_timeout
else
:internal_server_error
end
end

def addresses(search_term)
addresses = geocoder.find_address_candidates(SingleLine: search_term).slice(0, 1)
if addresses.empty?
analytics.idv_in_person_locations_searched(
success: false, errors: 'No address candidates found by ArcGIS',
)
end

{ json: addresses, status: :ok }
rescue Faraday::ConnectionFailed => err
analytics.idv_arcgis_request_failure(
api_status_code: 422,
rescue StandardError => err
api_status = api_status_from_error(err)
api_status_code = Rack::Utils::SYMBOL_TO_STATUS_CODE[api_status]
exception_message = err.message
response_status_code = err.respond_to?(:response_status) && err.response_status
errors = if err.instance_of?(Faraday::ClientError)
err.response_body && err.response_body[:details]
end
errors ||= 'ArcGIS error performing operation'

# log search event
analytics.idv_in_person_locations_searched(
success: false,
errors: errors,
api_status_code: api_status_code,
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,
exception_message: exception_message,
response_status_code: response_status_code,
)
{ json: [], status: :unprocessable_entity }
rescue Faraday::TimeoutError => err

# log the request failure
analytics.idv_arcgis_request_failure(
api_status_code: 422,
exception_class: err.class,
exception_message: err.message,
exception_message: exception_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,
response_status_code: response_status_code,
api_status_code: api_status_code,
)
{ json: [], status: :unprocessable_entity }

{ json: [], status: api_status }
end

def geocoder
Expand Down
11 changes: 10 additions & 1 deletion app/controllers/idv/in_person/usps_locations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,19 @@ def index
zip_code: search_params['zip_code']
)
response = proofer.request_facilities(candidate)
if response.length > 0
analytics.idv_in_person_locations_searched(
success: true,
result_total: response.length,
)
else
analytics.idv_in_person_locations_searched(
success: false, errors: 'No USPS locations found',
)
end
else
response = proofer.request_pilot_facilities
end

render json: response.to_json
end

Expand Down
28 changes: 28 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,34 @@ def idv_in_person_location_visited(flow_path:, **extra)
track_event('IdV: in person proofing location visited', flow_path: flow_path, **extra)
end

# @param [Boolean] success
# @param [Integer] result_total
# @param [String] errors
# @param [String] exception_class
# @param [String] exception_message
# @param [Integer] response_status_code
# User submitted a search on the location search page and response received
def idv_in_person_locations_searched(
success:,
result_total: 0,
errors: nil,
exception_class: nil,
exception_message: nil,
response_status_code: nil,
**extra
)
track_event(
'IdV: in person proofing location search submitted',
success: success,
result_total: result_total,
errors: errors,
exception_class: exception_class,
exception_message: exception_message,
response_status_code: response_status_code,
**extra,
)
end

# @param [String] selected_location Selected in-person location
# @param [String] flow_path Document capture path ("hybrid" or "standard")
# The user submitted the in person proofing location step
Expand Down
10 changes: 8 additions & 2 deletions app/services/arcgis_api/geocoder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,17 @@ def parse_address_candidates(response_body)
# @param response_body [Hash]
def handle_api_errors(response_body)
if response_body['error']
# response_body is in this format:
# {"error"=>{"code"=>400, "message"=>"", "details"=>[""]}}
error_code = response_body.dig('error', 'code')
error_message = response_body.dig('error', 'message') || "Received error code #{error_code}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error_message = response_body.dig('error', 'message') || "Received error code #{error_code}"
error_code = response_body.dig('error', 'code')
error_message = response_body.dig('error', 'message') || "Received error code #{error_code}"

This is a mistake I made when I suggested this change... we need to define error_code here.


raise Faraday::ClientError.new(
RuntimeError.new("received error code #{error_code}"),
response_body,
RuntimeError.new(error_message),
{
status: error_code,
body: { details: response_body.dig('error', 'details')&.join(', ') },
},
)
end
end
Expand Down
108 changes: 105 additions & 3 deletions spec/controllers/idv/in_person/address_search_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,22 +51,82 @@
expect(response.status).to eq(200)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
success: false,
errors: 'No address candidates found by ArcGIS',
result_total: 0,
exception_class: nil,
exception_message: nil,
response_status_code: nil,
)
end
end

context 'with error code' do
let(:response_body) do
{ 'error' => {
'code' => 400,
'details' => ['request is too many characters'],
'message' => 'Unable to complete operation.',
} }
end

before do
exception = Faraday::ClientError.new(
RuntimeError.new(response_body['error']['message']),
{
status: response_body['error']['code'],
body: { details: response_body['error']['details'].join(', ') },
},
)
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
end

it 'logs analytics event' do
response = get :index
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 400,
success: false,
errors: 'request is too many characters',
result_total: 0,
exception_class: Faraday::ClientError,
exception_message: 'Unable to complete operation.',
response_status_code: 400,
)
end
end
end

context 'with unsuccessful fetch' do
before do
exception = Faraday::ConnectionFailed.new('error')
exception = Faraday::ConnectionFailed.new('connection failed')
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
end

it 'gets an empty pilot response' do
response = get :index
expect(response.status).to eq(422)
expect(response.status).to eq(502)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
end

it 'logs search analytics' do
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 502,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
exception_class: Faraday::ConnectionFailed,
exception_message: 'connection failed',
response_status_code: nil,
)
end
end

context 'with a timeout error' do
Expand All @@ -77,10 +137,52 @@

it 'returns an error code' do
response = get :index
expect(response.status).to eq(422)
expect(response.status).to eq(504)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
end

it 'logs search analytics' do
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 504,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
exception_class: Faraday::TimeoutError,
exception_message: 'timeout',
response_status_code: nil,
)
end
end

context 'with an error' do
before do
exception = StandardError.new('error')
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
end

it 'returns a 500 error code' do
response = get :index
expect(response.status).to eq(500)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
end

it 'logs search analytics' do
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 500,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
exception_class: StandardError,
exception_message: 'error',
response_status_code: false,
)
end
end

context 'with feature disabled' do
Expand Down
29 changes: 29 additions & 0 deletions spec/controllers/idv/in_person/usps_locations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
let(:sp) { nil }
let(:in_person_proofing_enabled) { true }
let(:arcgis_search_enabled) { true }
let(:empty_locations) { [] }
let(:address) do
UspsInPersonProofing::Applicant.new(
address: '1600 Pennsylvania Ave',
Expand Down Expand Up @@ -121,6 +122,25 @@
end
end

context 'no addresses found by usps' do
before do
allow(proofer).to receive(:request_facilities).with(address).and_return(empty_locations)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nit picky, but it seems like the pattern between this test and the address_controller_spec.rb are different, but essentially doing the same thing. Is there a reason why we're not doing something simple likelet(:addresses) do [] end in address_search_controller.spec.rb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought it might be a bit easier to read the test like this

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally agree. I was more just wondering if there was any value in remaining consistent?

end

it 'logs analytics with error when successful response is empty' do
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
success: false,
errors: 'No USPS locations found',
result_total: 0,
exception_class: nil,
exception_message: nil,
response_status_code: nil,
)
end
end

context 'with successful fetch' do
before do
allow(proofer).to receive(:request_facilities).with(address).and_return(locations)
Expand All @@ -130,6 +150,15 @@
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 3
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
success: true,
errors: nil,
result_total: 3,
exception_class: nil,
exception_message: nil,
response_status_code: nil,
)
end
end

Expand Down
4 changes: 2 additions & 2 deletions spec/services/arcgis_api/geocoder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

expect { subject.suggest('100 Main') }.to raise_error do |error|
expect(error).to be_instance_of(Faraday::ClientError)
expect(error.message).to eq('received error code 400')
expect(error.message).to eq('Unable to complete operation.')
expect(error.response).to be_kind_of(Hash)
end
end
Expand Down Expand Up @@ -72,7 +72,7 @@

expect { subject.find_address_candidates(magicKey: 'abc123') }.to raise_error do |error|
expect(error).to be_instance_of(Faraday::ClientError)
expect(error.message).to eq('received error code 400')
expect(error.message).to eq('Unable to complete operation.')
expect(error.response).to be_kind_of(Hash)
end
end
Expand Down