Skip to content

Create the user_geos table for storing geo information derived from IP address.#9052

Merged
ashercodeorg merged 2 commits into
staging-nextfrom
createUserGeos
Jun 20, 2016
Merged

Create the user_geos table for storing geo information derived from IP address.#9052
ashercodeorg merged 2 commits into
staging-nextfrom
createUserGeos

Conversation

@ashercodeorg

Copy link
Copy Markdown
Contributor

t.string :state
t.string :country
t.string :postal_code
t.string :latitude_longitude

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not one each for lat and lng?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For consistency with existing code?
E.g., https://github.com/code-dot-org/code-dot-org/blob/staging/bin/cron/process_forms#L93, https://github.com/code-dot-org/code-dot-org/blob/staging/bin/cron/index_users_in_solr#L63, etc.

That said, I'm very open to splitting them apart, I'm just following existing usage. In terms of DB usage, if they are ever going to be used, we would definitely want them stored as decimals for distance computations (which will already be slow enough already).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much as I like consistency, I can't see this not complicating future implementations

@Hamms

Hamms commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

A couple tiny questions, but generally LGTM!

t.integer :user_id
t.timestamps null: false
t.datetime :indexed_at, null: false
t.string :ip_address

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we want a ip_address_at timestamp, indicating the timestamp at which we obtained the ip_address from the user?

t.string :country
t.string :postal_code
t.decimal :latitude, precision: 8, scale: 6
t.decimal :longitude, precision: 9, scale: 6

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that we are running version 5.6.19 of MySQL, so are unable to take advantage of the Point object introduced in 5.7.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are these not using the same precision?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both give six digits after the decimal point, since latitude is between -90 and 90 and longitude is between -180 and 180.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

@ashercodeorg

Copy link
Copy Markdown
Contributor Author

PTAL.

@Hamms

Hamms commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

Looks great!

@ashercodeorg ashercodeorg merged commit 6da512b into staging-next Jun 20, 2016
@ashercodeorg ashercodeorg deleted the createUserGeos branch June 20, 2016 17:51
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.

2 participants