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

Add lat/long to Request model, rework ID implementation #271

Merged
merged 1 commit into from
Nov 5, 2018

Conversation

frankduncan
Copy link

@frankduncan frankduncan commented Oct 25, 2018

This is a working PR that imports the changes from #221.

Adds latitude/longitude to DB and does a geocode lookup from google maps api to get the location and put in database.

Of note is that the code to loop over serial's was removed to prevent an infinite loop (#159), but doesn't seem to be replaced with anything preventing duplicate serials as using shortid's was found not to be optimal.

Not sure what the solution here is, since this will come up as a race condition.

@kfogel
Copy link
Member

kfogel commented Oct 28, 2018

Hey, @frankduncan. I responded to your questions re Google Maps API key in Zulip. See the thread here. I think I might have fixed the thing that was causing a problem -- I thought I had enabled geocoding and geolocation before, but it looks like I hadn't done it in the right way, and now I (hope I) have.

Added a lookup on google maps API for requests coming in, which requires
an API key to fulfill.

Also fixes an infinite loop and changes the id creation mechanism.
@frankduncan frankduncan changed the title WIP - Add lat/long to Request model, rework ID implementation Add lat/long to Request model, rework ID implementation Oct 28, 2018
@frankduncan
Copy link
Author

@kfogel I've updated the description, and things are now working. Take a look when you have a chance :)

Copy link
Contributor

@cecilia-donnelly cecilia-donnelly left a comment

Choose a reason for hiding this comment

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

This looks great! I like that it doesn't fail everything when run without a Google Maps API key. I was not able to test with the key yet, but presuming that works I'm +1 to merge this.

Copy link
Contributor

@cecilia-donnelly cecilia-donnelly left a comment

Choose a reason for hiding this comment

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

Works great, including finding the lat/long! Thanks @frankduncan.

@frankduncan frankduncan merged commit 3bfa1c3 into master Nov 5, 2018
@frankduncan frankduncan deleted the issue-206 branch November 5, 2018 21:22
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.

4 participants