Skip to content
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.

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

Switch Geocoder to use Mapbox API instead of Google Maps API #37009

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

daynew
Copy link
Member

@daynew daynew commented Oct 1, 2020

Soon we will no longer be able to use the Google Maps API. This change switches our Geocoder.search method calls to use the Mapbox API instead.

Links

Testing story

  • Manually tested in the dashboard console.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@daynew daynew force-pushed the plc-1300-replace-geocoder-gmaps-with-mapbox branch 2 times, most recently from 07e640a to 05a2b6e Compare October 6, 2020 16:59
Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -31,7 +31,7 @@ def self.find_potential_street_address(text)
results = Geocoder.search(first_number_to_end)
return nil if results.empty?

if results.first.types.include?('street_address')
if results.first.address
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting a comment here but I was trying to run this and it was crashing in summarize as a number of those attributes , such as street_address, don't exist for Mapbox. Can you take a look?

@bethanyaconnor
Copy link
Contributor

I'm a bit more worried about this change than I was yesterday. How much have you tested? I'm mostly concerned by the fact that the responses from the two APIs seem to be quite different and a lot of places in our code base use this class

@daynew daynew force-pushed the plc-1300-replace-geocoder-gmaps-with-mapbox branch from 05a2b6e to b66dc04 Compare October 10, 2020 19:05
@daynew daynew force-pushed the plc-1300-replace-geocoder-gmaps-with-mapbox branch from b66dc04 to 388f732 Compare October 11, 2020 22:26
@daynew
Copy link
Member Author

daynew commented Oct 12, 2020

Added tests and will manually test in staging. I will get a proper review during the week.

@daynew daynew merged commit 911a4eb into staging Oct 12, 2020
@daynew daynew deleted the plc-1300-replace-geocoder-gmaps-with-mapbox branch October 12, 2020 02:15
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.

None yet

2 participants