use zauth id's so usernames can be updated (and use zpi for profile images!) #220
Conversation
hannes-dev
force-pushed
the
zpi_images
branch
from
39ffd3a to
4e1a77c
Compare
hannes-dev
changed the title
| end | ||
|
|
||
| def avatar(*) | ||
| "https://zpi.zeus.gent/image/#{zauth_id || 0}" |
There was a problem hiding this comment.
Should this be a config option? @TomNaessens where in the config should you put functions?
There was a problem hiding this comment.
Yup, somewhere in the application.rb config at the end, there's some examples on production.rb already (e.g. Tab url)
| user.avatar = Paperclip.io_adapters.for(Identicon.data_url_for(auth.uid)) | ||
| user.generate_key! | ||
| user.private = true | ||
| zauth_id = auth.extra.raw_info["id"] |
There was a problem hiding this comment.
Wouldn't it be cleaner to let the uid in the omniauth block return this id, release a new major version of the zeuswpi omniauth policy, and then use the extra.raw_info["name"] down below instead of uid?
It's a bit more work but if we have a unique id in Zauth to use as identification, we should probably migrate everything to it in the long run.
| user.generate_key! | ||
| user.private = true | ||
| zauth_id = auth.extra.raw_info["id"] | ||
| unless zauth_id.is_a? Integer |
| end | ||
|
|
||
| # overwrite name (for if name changed) | ||
| db_user.name = auth.uid |
There was a problem hiding this comment.
An alternative here would be to just take some downtime and do a migration where we prepare a mapping of usernames to ZAuth id's, and fill them in the database. That way, the whole table will be filled, we can make the zauth_id a non-nullable column, and there's no need for changes like these in code.
| @@ -0,0 +1,5 @@ | |||
| class AddZauthIdToUsers < ActiveRecord::Migration[6.1] | |||
| def change | |||
| add_column :users, :zauth_id, :integer | |||
There was a problem hiding this comment.
Since we're finding by zauth_id, having an index here would be nice. Together with https://github.com/ZeusWPI/Tap/pull/220/changes#r2622094045, we could even make this non nullable after doing the migration.
| bal - total_pending | ||
| end | ||
|
|
||
| def avatar(*) |
| @@ -0,0 +1,5 @@ | |||
| class AddZauthIdToUsers < ActiveRecord::Migration[6.1] | |||
| def change | |||
| add_column :users, :zauth_id, :integer | |||
There was a problem hiding this comment.
Should this get a unique constraint? Having two users with the same id would be strange and should throw some error.
|
↑ Rebased on master, re-creating the migrations. |
closes #216
this is the first step in using zauth id's for syncing transactions as well