-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: support numeric usernames on get_user method #372
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,4 @@ | |
Various backends for receiving edX LMS events.. | ||
""" | ||
|
||
__version__ = '7.1.0' | ||
__version__ = '7.2.0' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,25 +3,30 @@ | |
""" | ||
from unittest.mock import patch | ||
|
||
from ddt import data, ddt | ||
from django.test import TestCase | ||
|
||
from event_routing_backends.helpers import ( | ||
get_anonymous_user_id, | ||
get_block_id_from_event_referrer, | ||
get_course_from_id, | ||
get_user, | ||
get_user_email, | ||
get_uuid5, | ||
) | ||
from event_routing_backends.tests.factories import UserFactory | ||
|
||
|
||
@ddt | ||
class TestHelpers(TestCase): | ||
""" | ||
Test the helper methods. | ||
""" | ||
|
||
def setUp(self): | ||
super().setUp() | ||
UserFactory.create(username='edx', email='[email protected]') | ||
self.edx_user = UserFactory.create(username='edx', email='[email protected]') | ||
UserFactory.create(username='10228945687', email='[email protected]') | ||
|
||
def test_get_block_id_from_event_referrer_with_error(self): | ||
sample_event = { | ||
|
@@ -83,3 +88,40 @@ def test_get_course_from_id_unknown_course(self, mock_get_course_overviews): | |
mock_get_course_overviews.return_value = [] | ||
with self.assertRaises(ValueError): | ||
get_course_from_id("foo") | ||
|
||
@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) | ||
Comment on lines
+91
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
def test_get_user_priority(self): | ||
"""Since the method can find users by id or username this checks that the user | ||
found by id will be returned instead of the user found by username when a user's | ||
username is the same as the id of another user. | ||
|
||
Expected behavior: | ||
- Returned user corresponds to the id. | ||
""" | ||
right_user = UserFactory.create(username='testing', email='[email protected]') | ||
# Create user with the previous user id as username. | ||
UserFactory.create(username=right_user.id, email='[email protected]') | ||
|
||
user = get_user(str(right_user.id)) | ||
|
||
self.assertEqual(right_user, user) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
[bumpversion] | ||
current_version = 7.1.0 | ||
current_version = 7.2.0 | ||
commit = False | ||
tag = False | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the username is equal to a user ID in the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withusername=5
and another user withid=5
, this will return the user withid=5
, and this basically keeps that logic. The modification implemented here considers the case when there is no a user withid=5
instead of returning None this will return the user withusername =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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the same with an extra step