-
Notifications
You must be signed in to change notification settings - Fork 671
AO3-6359 Fix Possible to delete the pseud that corresponds with your username if the capitalization or diacritics differ #5534
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
34456be
b8db8aa
1bfcfbe
1a7c62d
1d044cc
6688ebc
b6ec0e4
074d320
cad132d
1d6da05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They have different diacritics, are they meant to?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
There was a problem hiding this comment.
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
Justifiablemodule already disables its validation is justification is disabled, so we shouldn't need this special case hereThere was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which test case fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3
All for this reason
There was a problem hiding this comment.
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_usersto have an existing pseud for what it renames to and that test also fails. Poking at the code while it's in action, thejustification_enabled?here is always false becauselogin_changed?is always false.So the straightforward change here would be to just go back to setting
falsedirectly. 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 topseud_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.)
There was a problem hiding this comment.
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.