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

feat: Add auto generated username #34349

Merged

Conversation

attiyaIshaque
Copy link
Contributor

@attiyaIshaque attiyaIshaque commented Mar 11, 2024

Description

This PR enhances the user registration process by implementing auto-generated usernames when no username is provided and the ENABLE_AUTO_GENERATED_USERNAME feature flag is enabled.

The generated username follows a specific pattern: it consists of name initials, the current year and month, and a random configurable string. Here's how it's structured:
<name initials>_<current year><current month>_<random configurable string>

Example: AI_2403_AK8L

Supporting information

VAN-1871

Testing instructions

Tested locally
Unit tests added.

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch 4 times, most recently from ffbdf19 to 7df11ab Compare March 13, 2024 09:10
openedx/core/djangoapps/user_authn/views/utils.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/user_authn/views/utils.py Outdated Show resolved Hide resolved
return username


def generate_username_from_data(data):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def generate_username_from_data(data):
def generate_username_from_request_payload(data):

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch from 7df11ab to 86361e7 Compare March 13, 2024 11:48
Copy link
Contributor

@zainab-amir zainab-amir left a comment

Choose a reason for hiding this comment

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

This use case came to my mind that we might need to address:

Our code for auto-generating usernames currently assumes the presence of a learner's full name (first and last name combined or a single name field). However, are there scenarios where a learner or an external workflow could submit a registration form or call the registration API without providing a name?

If such cases exist, our current error handling might not be helpful. The way the code is currently written, it will return username error message (because the generate_username_from_request_payload function returns None). However, if the username field is hidden from the registration form, it would be beneficial to inform users directly that their name cannot be empty. This would guide them towards completing the registration process properly and then we can generate their username.

Comment on lines 837 to 838
### Random configurable username string length
RANDOM_USERNAME_STRING_LENGTH = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Can you move this to the section where other registration settings are present?

    ######################## Registration ########################
    # Social-core setting that allows inactive users to be able to
    # log in. The only case it's used is when user registering a new account through the LMS.
    INACTIVE_USER_LOGIN = True
    # Redirect URL for inactive user. If not set, user will be redirected to /login after the login itself (loop)
    INACTIVE_USER_URL = f'http://{CMS_BASE}'

  2. Update setting name

Suggested change
### Random configurable username string length
RANDOM_USERNAME_STRING_LENGTH = 4
### String length for the configurable part of the auto-generated username
AUTO_GENERATED_USERNAME_RANDOM_STRING_LENGTH = 4

@@ -4462,6 +4462,9 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
FINANCIAL_ASSISTANCE_MIN_LENGTH = 1250
FINANCIAL_ASSISTANCE_MAX_LENGTH = 2500

### Random configurable username string length
RANDOM_USERNAME_STRING_LENGTH = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Comment on lines 578 to 579
if is_auto_generated_username_enabled() and 'username' not in data:
data['username'] = generate_username_from_request_payload(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line before and after

return username


def generate_username_from_request_payload(data):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def generate_username_from_request_payload(data):
def get_auto_generated_username(data):


def generate_username(username_initials):
"""
Generate a username based on initials and current date.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Generate a username based on initials and current date.
Generate username based on learner's name initials, current date and configurable random string.
This function creates a username in the format <name_initials>_<YYMM>_<configurable_random_string>
The length of random string is determined by AUTO_GENERATED_USERNAME_RANDOM_STRING_LENGTH setting.

Generate a username based on the provided data.

Args:
- data (dict): Dictionary containing user data, including 'first_name', 'last_name', or 'name'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- data (dict): Dictionary containing user data, including 'first_name', 'last_name', or 'name'.
- data (dict): Registration payload.

Returns:
- str: Generated username.
"""
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we can improve the flow of code.

  1. Get username_prefix i.e name initials (this should maybe be a separate util function)
  2. Add the logic of actually generating username in this function and get rid of generate_username


username_initials = ''.join(initials)
return generate_username(username_initials)
except KeyError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid this by using data.get('first_name', '')

openedx/core/djangoapps/user_authn/views/utils.py Outdated Show resolved Hide resolved
first_name = data['first_name']
last_name = data['last_name']

username_initials = f"{first_name[0]}{last_name[0]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will raise IndexError if first and last name is empty

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch 3 times, most recently from ff99fae to 86d6017 Compare March 15, 2024 08:15
Copy link
Contributor

@zainab-amir zainab-amir left a comment

Choose a reason for hiding this comment

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

Can you add tests for the registration flow too? A few that come to my mind are:

  • Test registration is successful when the auto-generated username flag is true
  • Test some unsuccessful flows like missing name or invalid name

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch 3 times, most recently from 4448f3c to 58dc1db Compare March 19, 2024 10:01
@attiyaIshaque attiyaIshaque marked this pull request as ready for review March 19, 2024 11:07
@attiyaIshaque attiyaIshaque requested a review from a team as a code owner March 19, 2024 11:07
Copy link
Contributor

@zainab-amir zainab-amir left a comment

Choose a reason for hiding this comment

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

Is there a way to test the failure case too where username returned by get_auto_generated_username is already in use? Maybe by mocking the get_auto_generated_username to return a username that is already taken.

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch 6 times, most recently from 2f57847 to 55ead87 Compare March 20, 2024 07:06
Comment on lines 160 to 161
# We generate the username regardless of whether the name is empty or invalid. We do this
# because the name validations occur later, ensuring that users cannot create an account without a valid name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this either in the docstring or above the return statement at the end?

@attiyaIshaque attiyaIshaque force-pushed the attiya/VAN-1871-auto-generated-username branch from 55ead87 to 8c43530 Compare March 20, 2024 09:26
@attiyaIshaque attiyaIshaque merged commit a521fb2 into simplify-registration Mar 20, 2024
64 checks passed
@attiyaIshaque attiyaIshaque deleted the attiya/VAN-1871-auto-generated-username branch March 20, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants