Add non-ASCII character check for username suggestions #36056

CodeWithEmad · 2024-12-22T11:52:37Z

Added validation to prevent generating username suggestions for names containing non-ASCII characters. This fixes an issue where the system would generate usernames containing non-ASCII characters that would later be rejected by username validation.

Changes include:

Added ASCII character validation in generate_username_suggestions
Updated function's type hints and docstring
Added comprehensive test cases covering ASCII, non-ASCII (including various scripts like Persian, Japanese, Greek, and Cyrillic), and edge cases

Testing

Added new test class TestUsernameGeneration with test cases for:
- Special character removal
- ASCII/non-ASCII handling
- Uniqueness of suggestions
- Maximum length compliance
- Various edge cases

Close #35984

openedx-webhooks · 2024-12-22T11:52:42Z

Thanks for the pull request, @CodeWithEmad!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

CodeWithEmad · 2024-12-22T12:48:54Z

Sorry to ping you here @kdmccormick but I noticed that it takes about 20-25 minutes to run all the lint/test jobs and lining is a big chunk of it (about 10-12 minutes!). Pylint is so slow and as the project grows, it takes more time just to check the linting and code style! Have we ever considered using ruff?

kdmccormick · 2024-12-23T23:22:04Z

#32174

mphilbrick211 · 2025-03-03T17:35:56Z

Hi @CodeWithEmad! Just checking to see if this is still in progress?

CodeWithEmad · 2025-03-03T18:01:25Z

Hey @mphilbrick211
I believe @kdmccormick reviewed the task, but the task is stuck in the decision-making level where what exactly we should do for usernames with non-ascii characters. I mentioned the issue in the slack channels before to ask others opinions, but no luck there either.

kdmccormick

Because this is a bugfix, I think it does not need product review. Please rebase and confirm that the feature works as expected on the PR sandbox (which should spin up within an hour). Please also change your commit message to fix:, since we are fixing an active bug rather than adding a new feature.

Thank you for finding and resolving this!

kdmccormick · 2025-03-05T16:47:16Z

openedx/core/djangoapps/user_authn/utils.py

+def generate_username_suggestions(name: str) -> list[str] | list:
+    """
+    Generate 3 available username suggestions based on the provided name.
+
+    Args:
+        name (str): The full name to generate username suggestions from.
+            Must contain only ASCII characters.
+
+    Returns:
+        list[str] | list: A list of up to 3 available username suggestions,


An empty list is a valid list[str] (because it vacuously it satisfies the predicate "every item in the list is a string"), so there is no need to specify | list. In fact, it is preferable to omit | list, because list[str] | list would allow for lists of integers, booleans, or any other type.

Suggested change

def generate_username_suggestions(name: str) -> list[str] | list:

"""

Generate 3 available username suggestions based on the provided name.

Args:

name (str): The full name to generate username suggestions from.

Must contain only ASCII characters.

Returns:

list[str] | list: A list of up to 3 available username suggestions,

def generate_username_suggestions(name: str) -> list[str]:

"""

Generate 3 available username suggestions based on the provided name.

Args:

name (str): The full name to generate username suggestions from.

Must contain only ASCII characters.

Returns:

list[str]: A list of up to 3 available username suggestions,

kdmccormick · 2025-03-05T16:47:33Z

openedx/core/djangoapps/user_authn/tests/test_utils.py

+        # Test normal ASCII name
+        ('John Doe', True),  # Should return suggestions
+        ('Jane Smith', True),  # Should return suggestions
+        # Test non-ASCII names
+        ('José García', False),  # Contains non-ASCII characters
+        ('مریم میرزاخانی', False),  # Persian name
+        ('明美 田中', False),  # Japanese name
+        ('Σωκράτης', False),  # Greek name
+        ('Владимир', False),  # Cyrillic characters
+        # Test edge cases
+        ('A B', True),  # Minimal valid name
+        ('', True),  # Empty string
+        ('   ', True),  # Just spaces


open-craft-grove · 2025-03-05T18:01:29Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

kdmccormick · 2025-03-05T18:02:29Z

@CodeWithEmad ^ username/password for the sandbox is openedx/openedx

… detailed docstring - Updated `generate_username_suggestions` function to include validation for non-ASCII characters. - Improved function documentation to clarify arguments, return types, and username generation logic. - Added type hints for better code clarity and maintainability.

- Introduced `remove_special_characters_from_name` and `generate_username_suggestions` functions to enhance username handling. - Added comprehensive test cases for username generation, including ASCII validation and uniqueness checks. - Implemented tests for special character removal and suggestion generation based on various input scenarios, including edge cases.

- Removed unnecessary blank lines in the add-remove-label-on-comment.yml and check_python_dependencies.yml files to improve readability and maintainability.

CodeWithEmad · 2025-03-05T21:05:09Z

Thank you, dear @kdmccormick.
I can confirm that the registration page doesn't suggest a username for non-ascii languages (Persian for the image)

and the original suggestion for ASCII names works as expected:

kdmccormick

Excellent! I'll merge tomorrow.

open-craft-grove · 2025-03-05T21:50:57Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

edx-pipeline-bot · 2025-03-06T17:17:29Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2025-03-06T17:47:04Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2025-03-06T18:01:13Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2025-03-06T19:08:02Z

2U Release Notice: This PR has been deployed to the edX production environment.

CodeWithEmad requested a review from a team as a code owner December 22, 2024 11:52

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 22, 2024

CodeWithEmad force-pushed the fix/username-suggestions branch 3 times, most recently from 59b463c to 111c5d3 Compare December 22, 2024 12:20

CodeWithEmad force-pushed the fix/username-suggestions branch from 111c5d3 to 1ceffb5 Compare January 8, 2025 09:14

kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 5, 2025

kdmccormick approved these changes Mar 5, 2025

View reviewed changes

CodeWithEmad added 4 commits March 6, 2025 00:26

chore: fixed typo

7fbf8e4

chore: Clean up workflow YAML files

54ef475

- Removed unnecessary blank lines in the add-remove-label-on-comment.yml and check_python_dependencies.yml files to improve readability and maintainability.

CodeWithEmad force-pushed the fix/username-suggestions branch from 1ceffb5 to 54ef475 Compare March 5, 2025 20:57

kdmccormick approved these changes Mar 5, 2025

View reviewed changes

AhtishamShahid approved these changes Mar 6, 2025

View reviewed changes

kdmccormick merged commit 7c8781e into openedx:master Mar 6, 2025
49 checks passed

github-project-automation bot moved this from Ready for Review to Done in Contributions Mar 6, 2025

CodeWithEmad deleted the fix/username-suggestions branch March 6, 2025 21:10

Add non-ASCII character check for username suggestions #36056

Add non-ASCII character check for username suggestions #36056

Conversation

CodeWithEmad commented Dec 22, 2024

Testing

Uh oh!

openedx-webhooks commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeWithEmad commented Dec 22, 2024

Uh oh!

kdmccormick commented Dec 23, 2024

Uh oh!

mphilbrick211 commented Mar 3, 2025

Uh oh!

CodeWithEmad commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

kdmccormick Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

kdmccormick Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

open-craft-grove commented Mar 5, 2025

Uh oh!

kdmccormick commented Mar 5, 2025

Uh oh!

CodeWithEmad commented Mar 5, 2025

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

open-craft-grove commented Mar 5, 2025

Uh oh!

Uh oh!

edx-pipeline-bot commented Mar 6, 2025

Uh oh!

edx-pipeline-bot commented Mar 6, 2025

Uh oh!

edx-pipeline-bot commented Mar 6, 2025

Uh oh!

edx-pipeline-bot commented Mar 6, 2025

Uh oh!

Uh oh!

openedx-webhooks commented Dec 22, 2024 •

edited

Loading

CodeWithEmad commented Mar 3, 2025 •

edited

Loading