LG 7912: Create controller for fetching addresses from ArcgisApi #7282
LG 7912: Create controller for fetching addresses from ArcgisApi #7282allthesignals merged 11 commits intomainfrom
Conversation
| suggestion = suggest(permitted_params[:address]).first | ||
| arcgis_api_response = [find_address_candidates(suggestion.magic_key).first] |
There was a problem hiding this comment.
We probably want to rate limit these external calls in some form, but I'm not sure on the best strategy there
There was a problem hiding this comment.
@mitchellhenke any prior art elsewhere in the code that does this?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yep, I think that would be the path to take. It should probably be implemented within the service rather than the controller though.
There was a problem hiding this comment.
Cool! Makes sense. Thank you.
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Are we planning to implement rate-limiting in this pull request, or separately?
allthesignals
force-pushed
the
wmg/7912-arcgis-controller
branch
from
3901b0f to
aa6707a
Compare
I am trying to add it in now and if for some reason turns into a rabbit hole will try to do separately. |
config/routes.rb
Outdated
| as: :in_person_ready_to_verify | ||
| get '/in_person/usps_locations' => 'in_person/usps_locations#index' | ||
| put '/in_person/usps_locations' => 'in_person/usps_locations#update' | ||
| get '/in_person/addresses' => 'in_person/address_search#index' |
There was a problem hiding this comment.
a light convention (not 100% followed) we have is that we put our API endpoints under the /api namespace. Since this controller returns JSON and isn't expected to return HTML, I'd consider moving this there. But that could also happen in a separate PR when we actually have UI that makes s request here
There was a problem hiding this comment.
also, because this is PII, going to change to a post.
There was a problem hiding this comment.
also, because this is PII, going to change to a post.
Good catch!
It also has me wondering if it'd make sense to change the specs to use Mx McFakerson's address. While it wouldn't have helped for HTTP logging issues (something to look in to!), we do have some guardrails to catching details from this PII constant in logging and I could imagine us doing something similar for HTTP request query parameters.
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
allthesignals
force-pushed
the
wmg/7912-arcgis-controller
branch
from
ba65ea7 to
f7634b2
Compare
allthesignals
force-pushed
the
wmg/7912-arcgis-controller
branch
from
f7634b2 to
a5b3385
Compare
|
I am going to add rate-limiting in a follow-up PR. |
4 participants
🎫 Ticket
Link to the relevant ticket.
🛠 Summary of changes
Adds a controller, exposing an endpoint
/verify/in_person/addresses, which returns ArcgisApi::Geocoder::AddressCandidate structs.📜 Testing Plan
Provide a checklist of steps to confirm the changes.
🚀 Notes for Deployment
Need feedback on hiding this behind flag and authentication issues...