diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 6da10a300f..218c20ee67 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -2261,20 +2261,22 @@ def strip_if_string(value): def get_user_by_username_or_email(username_or_email): """ - Return a User object, looking up by email if username_or_email contains a - '@', otherwise by username. + Return a User object by looking up a user against username_or_email. Raises: User.DoesNotExist if no user object can be found, the user was retired, or the user is in the process of being retired. + + MultipleObjectsReturned if one user has same email as username of + second user + + MultipleObjectsReturned if more than one user has same email or + username """ username_or_email = strip_if_string(username_or_email) - user = None - if '@' in username_or_email: - user = User.objects.filter(email=username_or_email).first() - - if user is None: - user = User.objects.get(username=username_or_email) + # there should be one user with either username or email equal to username_or_email + user = User.objects.get(Q(email=username_or_email) | Q(username=username_or_email)) + if user is not None and user.username == username_or_email: UserRetirementRequest = apps.get_model('user_api', 'UserRetirementRequest') if UserRetirementRequest.has_user_requested_retirement(user): raise User.DoesNotExist diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 22236e3266..237769009c 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -9,6 +9,7 @@ import unittest import mock import six from django.contrib.auth.models import User +from django.core.exceptions import MultipleObjectsReturned from django.test import TestCase from django.test.utils import override_settings from pytz import UTC @@ -365,24 +366,38 @@ class TestStudentFromIdentifier(TestCase): """ Test get_student_from_identifier() """ - def setUp(self): - """ - Fixtures - """ - super(TestStudentFromIdentifier, self).setUp() - self.students = [ - UserFactory.create(username='foo@touchstone'), # a student with character `@` in user name - UserFactory.create() - ] + @classmethod + def setUpClass(cls): + super(TestStudentFromIdentifier, cls).setUpClass() + cls.valid_student = UserFactory.create(username='baz@touchstone') + cls.duplicate_email = UserFactory.create(email='foo@touchstone.com') + cls.duplicate_user_name = UserFactory.create(username='foo@touchstone.com') def test_valid_student_id(self): - for student in self.students: - assert student == tools.get_student_from_identifier(student.username) + """Test with valid username""" + assert self.valid_student == tools.get_student_from_identifier(self.valid_student.username) def test_valid_student_email(self): - for student in self.students: - assert student == tools.get_student_from_identifier(student.email) + """Test with valid email""" + assert self.valid_student == tools.get_student_from_identifier(self.valid_student.email) + + def test_student_username_has_conflict_with_others_email(self): + """ + An edge case where there is a user A with username example: foo@touchstone.com and + there is user B with email example: foo@touchstone.com + """ + with self.assertRaises(MultipleObjectsReturned): + tools.get_student_from_identifier(self.duplicate_user_name.username) + + def test_student_email_has_conflict_with_others_username(self): + """ + An edge case where there is a user A with email example: foo@touchstone.com and + there is user B with username example: foo@touchstone.com + """ + with self.assertRaises(MultipleObjectsReturned): + tools.get_student_from_identifier(self.duplicate_email.email) def test_invalid_student_id(self): + """Test with invalid identifier""" with self.assertRaises(User.DoesNotExist): assert tools.get_student_from_identifier("invalid") diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 19849087ba..00886585f3 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -18,7 +18,12 @@ import time import unicodecsv from django.conf import settings from django.contrib.auth.models import User -from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError +from django.core.exceptions import ( + MultipleObjectsReturned, + ObjectDoesNotExist, + PermissionDenied, + ValidationError +) from django.core.mail.message import EmailMessage from django.core.urlresolvers import reverse from django.core.validators import validate_email @@ -153,6 +158,8 @@ def common_exceptions_400(func): return func(request, *args, **kwargs) except User.DoesNotExist: message = _('User does not exist.') + except MultipleObjectsReturned: + message = _('Found a conflict with given identifier. Please try an alternative identifier') except (AlreadyRunningError, QueueConnectionError) as err: message = unicode(err)