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 #221

Closed
wants to merge 3 commits into from

Conversation

kwhinnery
Copy link
Contributor

This PR addresses #206 and revises the way a request is saved.

  • Do a geocode API request when a new Request is saved, document configuration of Google Maps API key
  • Don't retry saving the record infinitely
  • Replace previous sequential number with a hash value that will be unique, removing need for retries

Let me know what you think...

@@ -28,6 +28,9 @@ exports.systemEmail = 'jrandom@example.com';
// of the navigation for the admin section.
exports.signupEnabled = false;

// Configure Geocoding API API key
exports.googleMapsApiKey = 'AIzaSyB9r-02V9oy8iluVuLweHx6IFi_VuUMXVg';
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @kwhinnery, I see that you removed this in 3dd6ca0. Are you okay with having this in the history or does this mean you need to change your key?

@cecilia-donnelly
Copy link
Contributor

@kwhinnery, thank you! This looks great. I don't know if you talked to @OhMcGoo about the sequential public ids vs. hashed public ids, but I think we want to preserve the sequential public ids since they're seen by humans. I'm definitely glad you've fixed the infinite loop problem (#159)! We might add a counter or something to avoid infinity while preserving the sequential ids.

@frankduncan
Copy link

Ping @cecilia-donnelly

When reviewing this, I'm not sure how the information is getting to allReady. Once I understand that link, I can rebase this off master and take another look.

It also looks like we're still using hashed ids (?), or if that commit was replaced by corrected code.

@kfogel
Copy link
Member

kfogel commented Oct 9, 2018

@frankduncan I think the question of how the information gets to AllReady is separate from this change. This change is about ensuring that the Smoke Alarm Portal has the lat/long. Later we can take a look at the API by which we communicate with allReady (or other systems) and see what it takes to get the information across.

@kfogel
Copy link
Member

kfogel commented Oct 9, 2018

@frankduncan Re the hashed IDs, I agree with @cecilia-donnelly that we need human-friendly sequential public IDs.

@frankduncan
Copy link

@kfogel Do we have an google maps API key to use for OTS?

@kfogel
Copy link
Member

kfogel commented Oct 10, 2018

@frankduncan We have a Google Maps API key for DCSOps, but I think we should get a separate one for Smoke Alarm. I'll call you to work out details.

@kfogel
Copy link
Member

kfogel commented Oct 24, 2018

Okay, the new Google Maps API Key has been communicated via private channel to @frankduncan.

@frankduncan
Copy link

This PR has been replaced by #271, which is a branch local to the repository that I can manipulate.

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