Skip to content
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

Adds username normalization #627

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jkachel
Copy link
Contributor

@jkachel jkachel commented Jun 15, 2022

Pre-Flight checklist

  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#259

What's this PR do?

Fixes #259.

This adds a normalized_username field to the users model and an API function to perform the normalization. It also updates the openedx create_edx_user call to pass the normalized_username field instead of the regular username field.

How should this be manually tested?

The migrations to add the normalized_username field will automatically normalize the usernames. Because of that, it's pretty important that your user database does not contain accounts that will conflict. (Per #259, the normalized_username field is uniqued.) There is a management command to check for and fix usernames that would conflict that's in PR #629.

After performing the migrations, create a user with a username using accented characters (áüô, etc.). The username should be created on the Open edX side with those characters converted to non-accented characters (auo, etc.).

Some characters will not be normalized and will instead be dropped. This includes ø so testing with that character will result in a normalized username without that character (for example, Søren will normalize to Sren).

@odlbot odlbot temporarily deployed to mitxonline-ci-pr-627 June 15, 2022 20:52 Inactive
@jkachel jkachel marked this pull request as ready for review June 17, 2022 20:49
@annagav annagav self-requested a review June 21, 2022 13:02
@annagav annagav self-assigned this Jun 21, 2022
Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

Just a minor comment



@pytest.mark.django_db
def test_username_normalization():
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a duplicate of the test in the api.py? Is this test incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in ea43e81 - this was old and got left by mistake

@jkachel jkachel force-pushed the jkachel/259-accented-characters-fix branch from d29cc60 to ea43e81 Compare June 22, 2022 17:13
@jkachel jkachel requested a review from annagav June 22, 2022 17:28
Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

👍

@pdpinch
Copy link
Member

pdpinch commented Nov 2, 2022

@jkachel
Copy link
Contributor Author

jkachel commented Nov 2, 2022

@jkachel can you remind me why you put this on hold?

Would this have avoided the situation in https://odl.zendesk.com/agent/tickets/139242 and https://sentry.io/organizations/mit-office-of-digital-learning/issues/2616447470/events/9ab540c0d80f4983933e71bb7b9cd728/?project=5864687

This was put on hold because it needed some coordination to do - there was a part to this that was normalizing the existing usernames in the database. (See #259 (comment)) This was done about the time Brian left, though; I was going to get him to help with that. Otherwise, the code worked and it would have caught this, I think. It'd need some revising to work in the current version of the app before we could deploy it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix user creation for users with accented characters in their usernames
4 participants