Adds auto-population of the user_geos table.#9108
Conversation
9b5a03e to
ee66600
Compare
ee66600 to
0e46d4b
Compare
|
This looks great! |
| return unless current_user.current_sign_in_ip | ||
| return if UserGeo.find_by_user_id(current_user.id) | ||
|
|
||
| UserGeo.new.populate(current_user.id, current_user.current_sign_in_ip) |
There was a problem hiding this comment.
I think this might break the response returned by the SessionsController#create action unless you pass the overridden logic as a block to super (see related SO answer), like this:
# POST /login
def create
super do |user|
# custom code here
return unless user.current_sign_in_ip # ...etc
end
end
|
High-level, one concern is that this duplicates the geolocation information we are already storing in Solr. If we have a need for maintaining each geolocation setup independently that's fine, I'd just like to make sure we're not creating "multiple ways to do the same thing"-type technical debt without good reason, so more context on this would be helpful. Lower-level, the implementation looks solid, though the migration + updated schema for the added table seems to be missing (or was it added in a previous PR?). Rails associations could be a convenient addition but could be easily added later if needed. |
|
High Level: There seems to be a need for this information in the DB, given that we are wanting to join geolocation information with Code Studio information (e.g., is the student a CSF, CSD, or CSP student) that is not available in SOLR. My hope is that having the geolocation information in the DB, combined with piping that data to Tableau via Redshift, will allow us to slowly remove our dependency on SOLR. At least, that is my plan. Low Level: The migration was done in another PR (#9052) so that, had that migration or an earlier migration failed and the DTP continued, the code would not have been pushed with a missing DB dependency. |
|
PTAL. |
|
A longer-term plan to move towards RDS/Redshift/Tableau and away from Solr with our geolocation information sounds good to me, especially if it could eventually mean one less server to maintain ourselves for this type of analysis. LGTM! (though looks like some |
|
Done (removed swap files). |
Revert "Merge pull request #9108 from code-dot-org/autoPopulateUserGeos"
…teUserGeos"" This reverts commit 68efe7e.
Of course, after this PR is merged, I'll create and run a script to backfill rows in the user_geos table for existing users.
DOC: https://docs.google.com/document/d/1s5nYo8uSHWjCLWTtYCjLwTX5_E_ZwBVBRlOewN8tiw4