Skip to content

fix: analyze and fix users bugs/issues #258

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.

Sign up for GitHub

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

Closed
AliK070 wants to merge 1 commit into from
Closed

fix: analyze and fix users bugs/issues #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion app/controllers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,6 @@ def destroy
def has_required_roles? = current_user.has_roles?(:admin)

def user_params
params.expect(user: [:username, :password, :password_confirmation, { roles: [] }])
params.expect(user: [:username, :name, :password, :password_confirmation, { roles: [] }])
end
end
5 changes: 5 additions & 0 deletions db/migrate/20260323123000_add_name_to_users.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddNameToUsers < ActiveRecord::Migration[8.0]
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This new migration is defined as ActiveRecord::Migration[8.0], but the app is on Rails/ActiveRecord 8.1 (Gemfile + schema.rb). New migrations should generally target the current version ([8.1]) to ensure the migration DSL/defaults match the runtime framework behavior.

Suggested change
class AddNameToUsers < ActiveRecord::Migration[8.0]
class AddNameToUsers < ActiveRecord::Migration[8.1]

Copilot uses AI. Check for mistakes.
def change
add_column :users, :name, :string
end
Comment on lines +2 to +4
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

name is marked required: true in the Users form, but this migration adds a nullable column with no default/backfill. Consider enforcing the requirement at the data layer (e.g., add a User model presence validation and/or a null: false constraint with a backfill for existing rows) so non-browser clients/console cannot persist users with a blank name.

Suggested change
def change
add_column :users, :name, :string
end
def up
add_column :users, :name, :string, null: false, default: ""
change_column_default :users, :name, from: "", to: nil
end
def down
remove_column :users, :name
end

Copilot uses AI. Check for mistakes.
end
3 changes: 2 additions & 1 deletion db/schema.rb
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion test/controllers/users_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
test "#create successfully creates a user with valid parameters" do
user_params = {
username: "newuser",
name: "New User",
password: "password",
password_confirmation: "password",
roles: %w[admin reporter]
Expand All @@ -80,6 +81,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest

created_user = User.find_by(username: "newuser")
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This test will raise a NoMethodError on authenticate if the user wasn't created (making failures harder to diagnose). Add an explicit assert_not_nil created_user (or assert User.exists?(username: ...)) before calling methods on it so the failure message points at the real problem.

Suggested change
created_user = User.find_by(username: "newuser")
created_user = User.find_by(username: "newuser")
assert_not_nil created_user

Copilot uses AI. Check for mistakes.
assert created_user.authenticate("password")
assert_equal "New User", created_user.name
assert_equal %w[admin reporter], created_user.user_roles.pluck(:role)
end

Expand Down Expand Up @@ -107,11 +109,13 @@ class UsersControllerTest < ActionDispatch::IntegrationTest

test "#update successfully updates a user's username" do
updated_username = "Updated user"
updated_name = "Updated Name"

patch user_url(@user), params: { user: { username: updated_username } }
patch user_url(@user), params: { user: { username: updated_username, name: updated_name } }

assert_redirected_to users_path
assert_equal updated_username, @user.reload.username
assert_equal updated_name, @user.reload.name
Comment on lines 117 to +118
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Minor: @user.reload is called twice in a row here, which causes two DB queries. Reload once into a local variable (or call @user.reload once and assert on both attributes) to keep the test a bit leaner.

Suggested change
assert_equal updated_username, @user.reload.username
assert_equal updated_name, @user.reload.name
reloaded_user = @user.reload
assert_equal updated_username, reloaded_user.username
assert_equal updated_name, reloaded_user.name

Copilot uses AI. Check for mistakes.
end

test "#update does not update a user with invalid params" do
Expand Down
Loading