LG-8410 po search logging

[svalexander]

🎫 Ticket

Lg-8410

🛠 Summary of changes

Add analytics event for po search.

Addition in address_search_controller calls analytics event for arcgis api when no addresses are found and when errors other than TimeoutError and ConnectionFailed.

Addition in usps_locations_controller calls analytics event for usps api when response successfully returns addresses and when response has no locations.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• To test a success: Enter a working address, ex 10421 Motor City Dr, Bethesda, MD 20817
• To test a usps no addresses failure: Enter an address that fails for usps, ex Mile 20 I-83 North, Glen Rock, PA
• To test when arcgis has a successful response with an error code: Enter an address that have 200+ characters, ex 1000 mainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmainmain boulevard, staten island, massachussetts 10001-12341
• To test a timeout error, edit ArcgisApi::Geocoder.handle_api_errors to raise Faraday::TimeoutError
• To test a failed connection error, edit ArcgisApi::Geocoder.handle_api_errors to raise Faraday::ConnectionFailed
• To test an arcgis no candidates failure: after api call reassign addresses to []
• To test an arcgis standarderror: after api call reassign addresses to an int

[JackRyan1989]

This looks awesome! I just have a few comments but am happy to approve after that.

[JackRyan1989]

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?

[svalexander]

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

[JackRyan1989]

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

[JackRyan1989]

Tests pass in dev. Looks good to me!

[tomas-nava]

I think we're missing AC#4 (first bullet) from LG-8410:

ArcGIS' service returns 200 OK HTTP responses but indicates that there were errors when the response bodies include an "error" key.

I think the right place to capture these is in the find_address_candidates method of ArcgisApi::Geocoder; something like

response = faraday.get(ADDRESS_CANDIDATES_ENDPOINT, params, dynamic_headers) do |req|
  req.options.context = { service_name: 'arcgis_geocoder_find_address_candidates' }
end

if response['error_code'].present?
  # log an error analytics event
 return []
end

return parse_address_candidates(response.body)

This is very pseudocodey, because I'm not sure what an error response will look like; I couldn't find good examples in the documentation @allthesignals linked to in the ticket. Maybe you two could pair on this part?

[tomas-nava]

approve!

I suggested some additional small changes that should be made before merging

[tomas-nava]

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.

[tomas-nava]

Suggested change

	status: response_body.dig('error', 'code'),
	status: error_code,

...and we can use the variable defined in my last suggestion here

[tomas-nava]

Suggested change

	exception_message: err.message,
	exception_message: exception_message,

[tomas-nava]

Suggested change

	else
	response = proofer.request_pilot_facilities
	end
	else
	response = proofer.request_pilot_facilities
	end