Skip to content
Open
30 changes: 14 additions & 16 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -548,25 +548,23 @@ def self.fetch_orphan_account

def update_pseud_name
return unless saved_change_to_login? && login_before_last_save.present?

old_pseud = pseuds.where(name: login_before_last_save).first
if login.downcase == login_before_last_save.downcase
old_pseud.name = login
old_pseud.save!
else

pseud_to_update = pseuds.where(name: login_before_last_save).first
# If the new login is (case insensitive) different from the old login
if login.downcase != login_before_last_save.downcase
new_pseud = pseuds.where(name: login).first
# do nothing if they already have the matching pseud
if new_pseud.blank?
if old_pseud.present?
# change the old pseud to match
old_pseud.name = login
old_pseud.save!(validate: false)
else
# shouldn't be able to get here, but just in case
Pseud.create!(name: login, user_id: id)
end
# If the user does have an existing pseud for the new login
if new_pseud.present?
pseud_to_update = new_pseud
# If the pseud for the old login doesn't exist
elsif pseud_to_update.blank?
# shouldn't be able to get here, but just in case
Pseud.create!(name: login, user_id: id)
return
end
end
pseud_to_update.name = login
pseud_to_update.save!(validate: justification_enabled?)
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see the Justifiable module already disables its validation is justification is disabled, so we shouldn't need this special case here

Copy link
Author

Choose a reason for hiding this comment

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

I think that is disabled here: https://github.com/otwcode/otwarchive/blob/master/app/models/user.rb#L538-L540
Without justification_enabled the test cases where an Admin updates the user name fails for

     Failure/Error: pseud_to_update.save!

     ActiveRecord::RecordInvalid:
       Validation failed: Ticket ID can't be blank, Ticket ID may begin with an # and otherwise contain only numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which test case fails?

Copy link
Author

Choose a reason for hiding this comment

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

These 3

rspec ./spec/models/user_spec.rb:226 # User#update when logged in as an admin when username is changed only sets admin_renamed_at
rspec ./spec/models/user_spec.rb:233 # User#update when logged in as an admin when username is changed creates an admin log item
rspec ./spec/models/user_spec.rb:249 # User#update when logged in as an admin username was recently changed does not prevent changing the username

All for this reason

  3) User#update when logged in as an admin username was recently changed does not prevent changing the username
     Failure/Error: pseud_to_update.save!

     ActiveRecord::RecordInvalid:
       Validation failed: Ticket ID can't be blank, Ticket ID may begin with an # and otherwise contain only numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! The test setup of these is a bit strange but ultimately you are correct that this validation fails for renames if enabled. To confirm that, I modified the feature test in admin_abuser_users to have an existing pseud for what it renames to and that test also fails. Poking at the code while it's in action, the justification_enabled? here is always false because login_changed? is always false.

So the straightforward change here would be to just go back to setting false directly. I'm a bit hesitant about this because we also disable validations like pseud name length here. However, the pseud validations on names are more permissive than the username ones and we're already manually making sure it's unique, so there's not really any harm disabling them. And it's what the old code did. So please just change this to pseud_to_update.save!(validate: false)

(We could theoretically set the ticket number on the pseud to force the validations to pass but I don't think there's much of a point. Hopefully.)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for looking into this! I will update it as soon as I can.

end

def reindex_user_creations_after_rename
Expand Down
52 changes: 52 additions & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,58 @@
expect(log_item.note).to eq("Change made by #{admin.login}")
end
end

context "username was changed to match an existing pseud's name except downcased" do
let(:new_pseud) { build(:pseud, name: "New_Usernamé") }

before do
existing_user.pseuds << new_pseud
existing_user.update!(login: "new_username")
Comment on lines +398 to +403
Copy link
Contributor

Choose a reason for hiding this comment

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

They have different diacritics, are they meant to?

Copy link
Author

@Dobe-Time Dobe-Time Feb 4, 2026

Choose a reason for hiding this comment

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

Yes, I can make the message more descriptive, but we want it to match regardless of diacritics so I had this test as a exiting pseud with capitals and diacritics with the most basic version as the new login.

Copy link
Contributor

@Bilka2 Bilka2 Feb 5, 2026

Choose a reason for hiding this comment

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

I suppose the better question is what the difference between this and the next test is meant to be. And then put that answer into the description

existing_user.reload
end

it "pseud's capitalization and diacritics were changed to match the new username's" do
expect(existing_user.pseuds.size).to eq(2)
expect(existing_user.pseuds.second.name).to eq(existing_user.login)
expect(existing_user.pseuds.second.name).to eq("new_username")
expect(existing_user.login).to eq("new_username")
end
end

context "username was changed to match an existing pseud's name with mixed capitalization and diacritics" do
let(:new_pseud) { build(:pseud, name: "New_Usernamé") }

before do
existing_user.pseuds << new_pseud
existing_user.update!(login: "new_UsernaMe")
existing_user.reload
end

it "pseud's capitalization and diacritics were changed to match the new username's" do
expect(existing_user.pseuds.size).to eq(2)
expect(existing_user.pseuds.second.name).to eq(existing_user.login)
expect(existing_user.pseuds.second.name).to eq("new_UsernaMe")
expect(existing_user.login).to eq("new_UsernaMe")
end
end

context "username was changed to not match an existing pseud's name, and the users has no psueds" do
let(:new_pseud) { build(:pseud, name: "New_Usernamé") }

before do
existing_user.pseuds = []

existing_user.update!(login: "new_UsernaMe")
existing_user.reload
end

it "creates a new pseud for the users new login" do
expect(existing_user.pseuds.size).to eq(1)
expect(existing_user.pseuds.first.name).to eq(existing_user.login)
expect(existing_user.pseuds.first.name).to eq("new_UsernaMe")
expect(existing_user.login).to eq("new_UsernaMe")
end
end
end

describe ".search_multiple_by_email" do
Expand Down
Loading