fix: Fix edX username validation to avoid username collision #1389
Conversation
users/serializers.py
Outdated
| try: | ||
| openedx_username_taken = check_username_exists_in_edx(trimmed_value) | ||
| openedx_validation_msg = validate_username_with_edx(trimmed_value) | ||
| openedx_validation_msg = OPENEDX_USERNAME_VALIDATION_MSGS_MAP.get( |
There was a problem hiding this comment.
So I was thinking. If edX returns some error message for the username while validating account registration and the message is not related to the existing username that we had in OPENEDX_USERNAME_VALIDATION_MSGS_MAP we will get None for openedx_validation_msg even though there can be some other error for username?
In the above case, I think the user creation would still succeed, whereas I think if we get any error for the username field we should block the registration process, and in this case, we can send a generic message to the frontend for the user that the registration can't proceed.
There was a problem hiding this comment.
I think it would be good to take @rhysyngsun's opinion on this too.
There was a problem hiding this comment.
I have used the .get() method of the dictionary and provided the msg returned by edX as default. So, in case the msg does not exist in our OPENEDX_USERNAME_VALIDATION_MSGS_MAP then it will return the msg returned by edX as I am doing OPENEDX_USERNAME_VALIDATION_MSGS_MAP.get(openedx_validation_msg, openedx_validation_msg)
There was a problem hiding this comment.
The approach here is fine to pass through the openedx error message.
There was a problem hiding this comment.
In addition to the changes I'm suggesting here, we'd talked about making the username validation required (meaning if openedx fails, we don't allow account creation to proceed).
To accomplish this I think we should actually move the username validation out of validate_username and into the general purpose validate(). Then in that method we can update this so that if the API call to openedx fails, return an object-level error (make sure the form displays this) that we're unable to create an account at this time and to please reach out to support.
This is a bit more invasive than your original changes but I think we need to stop soft-failing when there's an openedx outage because then we're right back to having accounts get created that need username repairs, but if we block on openedx being available, if there's an outage there's no further effort required on our part once that system is available again.
openedx/exceptions.py
Outdated
| @@ -133,7 +133,7 @@ def __init__(self, username, http_error, msg=None): | |||
|
|
|||
| Args: | |||
| username (str): The username being compared for uniqueness | |||
| http_error (requests.exceptions.HTTPError): The exception from the API call which contains | |||
| http_error (requests.Response): The exception from the API call which contains | |||
There was a problem hiding this comment.
Since you're fixing the type, can you fix the argument and property name too since it's not an error?
asadali145
force-pushed
the
asad/fix-username-collision
branch
from
2d9caa1 to
87f379b
Compare
users/serializers.py
Outdated
| RequestsConnectionError, | ||
| EdxApiRegistrationValidationException, | ||
| ): | ||
| raise serializers.ValidationError( |
There was a problem hiding this comment.
Could you log this exception so we'll know registrations are failing?
|
@asadali145 if your PR is ready and @arslanashraf7 is able to functionally test it, don't let me own functional testing hold up merging this. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #1389 +/- ##
==========================================
- Coverage 83.28% 83.25% -0.03%
==========================================
Files 309 309
Lines 13105 13116 +11
Branches 930 931 +1
==========================================
+ Hits 10914 10920 +6
- Misses 1925 1928 +3
- Partials 266 268 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Pre-Flight checklist
What are the relevant tickets?
Somewhat #259 and this slack discussion.
Fixes #1393
What's this PR do?
Fixes the username validation with edX.
How should this be manually tested?
FEATURES["ENABLE_UNICODE_USERNAME"] = TrueA user already exists with this username. Please try a different one.Any background context you want to provide?
Most of the details are available in the slack thread and #259
Here are the alternative approaches that I considered: