fix: support numeric usernames on get_user method #372
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @andrey-canon! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
|
||
| if username: | ||
| try: | ||
| user = User.objects.get(id=int(username_or_id)) |
There was a problem hiding this comment.
What if the username is equal to a user ID in the database?
There was a problem hiding this comment.
the first thing, that current implementation makes, is to try to set the user_id here and then look for the user with that id , so if there is a user with username=5 and another user with id=5, this will return the user with id=5, and this basically keeps that logic. The modification implemented here considers the case when there is no a user with id=5 instead of returning None this will return the user with username =5.
Finally my comment is that there is not an easy way to identify the right case, the method uses username_or_id parameter but that is too ambiguous and the method can not identify that by itself that needs some criteria like first id and then username or limit the method scope to username or id, not both, but that requires to standard the events to use just one of them.
There was a problem hiding this comment.
What if we verify if the user_id is already an integer (all events send this param as int) then we query by ID, in another case query by username?
There was a problem hiding this comment.
it's the same with an extra step
|
|
||
| @data("edx", "10228945687") | ||
| def test_get_user_by_username(self, username): | ||
| """Test that the method get_user returns the right user based on given username parameter. | ||
|
|
||
| Expected behavior: | ||
| - Returned user corresponds to the username. | ||
| """ | ||
| user = get_user(username) | ||
|
|
||
| self.assertEqual(username, user.username) | ||
|
|
||
| def test_get_user_by_id(self): | ||
| """ Test that the method get_user returns the right user based on the user id. | ||
|
|
||
| Expected behavior: | ||
| - Returned user is the edx_user | ||
| """ | ||
| user = get_user(self.edx_user.id) | ||
|
|
||
| self.assertEqual(self.edx_user, user) |
There was a problem hiding this comment.
Add a new test with two users, the first user with a hardcoded ID and the second one with the same ID as the username, verify that getting the user by the user_id as str returns the second user
|
@andrey-canon Can you add a changelog entry and bump version? |
andrey-canon
force-pushed
the
and/support_numeric_usernames
branch
from
fd688f9 to
2e7756c
Compare
|
@andrey-canon 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
When a user sets their username as a numeric value, for example the local id (1022547854), the current code handles this as the user id therefore the right user is not found and that returns None, This changes add support for that specific case and includes some basic unit tests which validates the basic behavior
Testing instructions:
User with username "1100865375" does not exist.shouldn't appearor directly call the method in a shell
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.