-
Notifications
You must be signed in to change notification settings - Fork 81
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
Fix #946: Make usernames case insensitive #1276
Conversation
@valaparthvi, these are two different issues.. your PR fixes #1105 whereas this PR is a fix for #946 |
hey @bjohare , yes I observed that. I just got too excited and commented. sorry for that. |
@valaparthvi, no problem :-) |
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.
Just an initial comment. This PR makes new usernames case insensitively unique. Do we need to make existing usernames case insensitively unique as well? (Are there any that are not CI unique right now?)
@amplifi thoughts on how to handle the existing usernames that aren't case sensitive unique? I think we should probably do this manually and contact people. What are you thoughts? |
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.
Works as expected. Would there be a way to DRY this out a little, there's a lot of repetition in there. Maybe move getting the user names and raising the eception into a function which is called from all different places?
@wonderchook As of just now, all usernames in staging, demo, and prod are case-insensitive unique. There was one test account in staging, plus the production account (taken care of back in Nov by contacting the user) that prompted the original issue to require CI-uniqueness. I'll add a note to double-check and confirm that still holds before this PR goes live. |
@oliverroick fair point. I'll take a look. |
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.
Looks good!
Proposed changes in this pull request
Fixes #946: Make usernames case insensitive
Currently, usernames are case sensitive. This PR makes username comparisons for account registration and profile updates case insensitive for both default and api views. New usernames and updates to existing usernames are checked for uniqueness against the full set of usernames using the
casefold
algorithm. Existing mixed-case usernames in the database remain untouched.Add username comparisons using
casefold
toRegisterForm
andProfileForm
.Add username comparisons using
casefold
toRegistrationSerializer
andUserSerializer
.Added tests for the above changes.
When should this PR be merged
Anytime.
Risks
Low.
Follow up actions
None required.
Checklist (for reviewing)
General
migration
label if a new migration is added.Functionality
Code
Tests
Documentation