Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,18 @@
# remember_created_at :datetime
# admin :boolean default(FALSE)
# dagschotel_id :integer
# avatar_file_name :string
# avatar_content_type :string
# avatar_file_size :integer
# avatar_updated_at :datetime
# orders_count :integer default(0)
# koelkast :boolean default(FALSE)
# name :string
# private :boolean default(FALSE)
# frecency :integer default(0), not null
# quickpay_hidden :boolean default(FALSE)
# userkey :string
# zauth_id :integer
#

class User < ApplicationRecord
include FriendlyId
include Avatarable
include Statistics

friendly_id :name, use: :finders
Expand All @@ -40,12 +36,28 @@ class User < ApplicationRecord
scope :publik, -> { where private: false }

def self.from_omniauth(auth)
db_user = find_or_create_by!(name: auth.uid) do |user|
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"]
Copy link
Member

Choose a reason for hiding this comment

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

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.

unless zauth_id.is_a? Integer
Copy link
Member

Choose a reason for hiding this comment

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

Can this happen?

raise(
"zauth id is not valid, this is not good, what is happening? what did you do? i am confused and will give up"
)

end

db_user = find_by(zauth_id: zauth_id)

if db_user.nil?
db_user = find_or_create_by!(name: auth.uid) do |user|
user.generate_key!
user.private = true
end

db_user.zauth_id = zauth_id
end

# overwrite name (for if name changed)
db_user.name = auth.uid
Copy link
Member

Choose a reason for hiding this comment

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

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.


# get roles info
roles = auth.dig(:extra, :raw_info, :roles) || []

Expand Down Expand Up @@ -107,12 +119,14 @@ def balance
bal - total_pending
end

def avatar(*)
Copy link
Member

Choose a reason for hiding this comment

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

Is the (*) needed here?

"https://zpi.zeus.gent/image/#{zauth_id || 0}"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a config option? @TomNaessens where in the config should you put functions?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, somewhere in the application.rb config at the end, there's some examples on production.rb already (e.g. Tab url)

end

# Static Users

def self.guest
@guest || find_or_create_by(name: "Guest") do |user|
user.avatar = File.new(File.join("app", "assets", "images", "guest.png"))
end
@guest || find_or_create_by(name: "Guest")
end

def guest?
Expand All @@ -121,7 +135,6 @@ def guest?

def self.koelkast
@koelkast || find_or_create_by(name: "Koelkast") do |user|
user.avatar = File.new(File.join("app", "assets", "images", "logo.png"))
user.koelkast = true
end
end
Expand Down
8 changes: 8 additions & 0 deletions db/migrate/20251223210834_remove_avatar_from_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class RemoveAvatarFromUsers < ActiveRecord::Migration[8.1]
def change
remove_column :users, :avatar_file_name, :string
remove_column :users, :avatar_content_type, :string
remove_column :users, :avatar_file_size, :integer
remove_column :users, :avatar_updated_at, :datetime
end
end
5 changes: 5 additions & 0 deletions db/migrate/20251223211742_add_zauth_id_to_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddZauthIdToUsers < ActiveRecord::Migration[8.1]
def change
add_column :users, :zauth_id, :integer
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Should this get a unique constraint? Having two users with the same id would be strange and should throw some error.

end
end
7 changes: 2 additions & 5 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[8.1].define(version: 2025_12_16_022119) do
ActiveRecord::Schema[8.1].define(version: 2025_12_23_211742) do
create_table "barcodes", force: :cascade do |t|
t.string "code", default: "", null: false
t.datetime "created_at", precision: nil
Expand Down Expand Up @@ -68,10 +68,6 @@

create_table "users", force: :cascade do |t|
t.boolean "admin", default: false
t.string "avatar_content_type"
t.string "avatar_file_name"
t.bigint "avatar_file_size"
t.datetime "avatar_updated_at", precision: nil
t.datetime "created_at", precision: nil
t.integer "dagschotel_id"
t.integer "frecency", default: 0, null: false
Expand All @@ -83,6 +79,7 @@
t.datetime "remember_created_at", precision: nil
t.datetime "updated_at", precision: nil
t.string "userkey"
t.integer "zauth_id"
t.index ["koelkast"], name: "index_users_on_koelkast"
t.index ["orders_count"], name: "index_users_on_orders_count"
end
Expand Down
1 change: 0 additions & 1 deletion db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
User.create!(
name: name,
private: false,
avatar: Paperclip.io_adapters.for(Identicon.data_url_for(name))
)
end

Expand Down
20 changes: 2 additions & 18 deletions spec/controllers/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,13 @@
# remember_created_at :datetime
# admin :boolean default(FALSE)
# dagschotel_id :integer
# avatar_file_name :string
# avatar_content_type :string
# avatar_file_size :integer
# avatar_updated_at :datetime
# orders_count :integer default(0)
# koelkast :boolean default(FALSE)
# name :string
# private :boolean default(FALSE)
# frecency :integer default(0), not null
# quickpay_hidden :boolean
# zauth_id :integer
#

# quickpay_user GET /users/:id/quickpay(.:format) users#quickpay
Expand Down Expand Up @@ -65,7 +62,7 @@

describe "PUT update" do
it "loads the correct user" do
put :update, params: { id: user, user: attributes_for(:user).except(:avatar) }
put :update, params: { id: user, user: attributes_for(:user) }
expect(assigns(:user)).to eq(user)
end

Expand All @@ -82,12 +79,6 @@
put :update, params: { id: user, user: { dagschotel_id: product.id } }
expect(user.reload.dagschotel).to eq(product)
end

it "accepts real images" do
file = fixture_file_upload("real-image.png", "image/png")
put :update, params: { id: user, user: { avatar: file } }
expect(flash[:success]).to be_present
end
end

context "danger zone" do
Expand All @@ -96,13 +87,6 @@
put :update, params: { id: user, user: {} }
end.to raise_error(ActionController::ParameterMissing)
end

it "does not accept unreal images" do
file = fixture_file_upload("unreal-image.svg", "image/svg+xml")
expect do
put :update, params: { id: user, user: { avatar: file } }
end.to raise_error(ActiveRecord::RecordInvalid)
end
end
end

Expand Down
6 changes: 1 addition & 5 deletions spec/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,14 @@
# remember_created_at :datetime
# admin :boolean default(FALSE)
# dagschotel_id :integer
# avatar_file_name :string
# avatar_content_type :string
# avatar_file_size :integer
# avatar_updated_at :datetime
# orders_count :integer default(0)
# koelkast :boolean default(FALSE)
# name :string
# private :boolean default(FALSE)
# frecency :integer default(0), not null
# quickpay_hidden :boolean default(FALSE)
# userkey :string
# zauth_id :integer
#

require "faker"
Expand All @@ -29,7 +26,6 @@
FactoryBot.define do
factory :user do
name { Faker::Internet.user_name }
avatar { File.new(Rails.root.join("spec/fixtures/files/real-image.png")) }
private { false }

factory :admin do
Expand Down
93 changes: 77 additions & 16 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,14 @@
# remember_created_at :datetime
# admin :boolean default(FALSE)
# dagschotel_id :integer
# avatar_file_name :string
# avatar_content_type :string
# avatar_file_size :integer
# avatar_updated_at :datetime
# orders_count :integer default(0)
# koelkast :boolean default(FALSE)
# name :string
# private :boolean default(FALSE)
# frecency :integer default(0), not null
# quickpay_hidden :boolean default(FALSE)
# userkey :string
# zauth_id :integer
#

require "webmock/rspec"
Expand All @@ -37,13 +34,6 @@
############

describe "fields" do
describe "avatar" do
it "is present" do
user.avatar = nil
expect(user).not_to be_valid
end
end

describe "orders_count" do
it "automaticallies cache the number of orders" do
balance = 5
Expand Down Expand Up @@ -79,7 +69,7 @@
{
uid: "yet-another-test-user",
extra: {
raw_info: { roles: [] }
raw_info: { roles: [], id: 7 }
}
}
)
Expand All @@ -88,6 +78,7 @@
it "creates a new user with the correct name" do
user = described_class.from_omniauth(auth_hash)
expect(user.name).to eq("yet-another-test-user")
expect(user.zauth_id).to eq(7)
expect(user.admin).to be(false)
end
end
Expand All @@ -99,7 +90,30 @@
{
uid: existing_user.name,
extra: {
raw_info: { roles: [] }
raw_info: { roles: [], id: 7 }
}
}
)
end

it "finds the existing user" do
existing_user.zauth_id = 7
existing_user.save

user = described_class.from_omniauth(auth_hash)
expect(user).to eq(existing_user)
expect(user.admin).to be(false)
end
end

describe "when the user already exists but without zauth id" do
let!(:existing_user) { create(:user) }
let(:auth_hash) do
OmniAuth::AuthHash.new(
{
uid: existing_user.name,
extra: {
raw_info: { roles: [], id: 7 }
}
}
)
Expand All @@ -112,14 +126,37 @@
end
end

describe "when the user exists but has changed their name" do
let!(:existing_user) { create(:user) }
let(:auth_hash) do
OmniAuth::AuthHash.new(
{
uid: "new-username-from-zauth",
extra: {
raw_info: { roles: [], id: existing_user.zauth_id }
}
}
)
end

it "updated the username" do
existing_user.zauth_id = 7
existing_user.save

user = described_class.from_omniauth(auth_hash)
expect(user.id).to eq(existing_user.id)
expect(user.name).to eq("new-username-from-zauth")
end
end

describe "when the user already exists and now has bestuur role" do
let!(:existing_user) { create(:user) }
let(:auth_hash) do
OmniAuth::AuthHash.new(
{
uid: existing_user.name,
extra: {
raw_info: { roles: ["bestuur"] }
raw_info: { roles: ["bestuur"], id: 7 }
}
}
)
Expand All @@ -130,6 +167,9 @@
end

it "gets admin permissions" do
existing_user.zauth_id = 7
existing_user.save

user = described_class.from_omniauth(auth_hash)
expect(user).to eq(existing_user)
expect(user.admin).to be(true)
Expand All @@ -142,7 +182,7 @@
{
uid: "a-test-admin-user-bestuur",
extra: {
raw_info: { roles: ["bestuur"] }
raw_info: { roles: ["bestuur"], id: 7 }
}
}
)
Expand All @@ -161,7 +201,7 @@
{
uid: "a-test-admin-user-tap_admin",
extra: {
raw_info: { roles: ["tap_admin"] }
raw_info: { roles: ["tap_admin"], id: 7 }
}
}
)
Expand All @@ -173,6 +213,27 @@
expect(user.admin).to be(true)
end
end

describe "when the user has an invalid zauth id" do
let(:auth_hash) do
OmniAuth::AuthHash.new(
{
uid: "a-test-user",
extra: {
raw_info: { roles: ["tap_admin"], id: "7" }
}
}
)
end

it "fails" do
expect do
described_class.from_omniauth(auth_hash)
end.to raise_error(
"zauth id is not valid, this is not good, what is happening? what did you do? i am confused and will give up"
)
end
end
end

describe "static users" do
Expand Down