Fixup! refactor email_exists, and handle many more cases

lms/djangoapps/instructor/enrollment.py

@@ -27,7 +27,12 @@ from lms.djangoapps.grades.signals.signals import PROBLEM_RAW_SCORE_CHANGED
 from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.user_api.models import UserPreference
-from student.models import CourseEnrollment, CourseEnrollmentAllowed, anonymous_id_for_user
+from student.models import (
+    CourseEnrollment,
+    CourseEnrollmentAllowed,
+    anonymous_id_for_user,
+    is_email_retired,
+)
 from submissions import api as sub_api  # installed from the edx-submissions repository
 from submissions.models import score_set
 from track.event_transaction_utils import (
@@ -44,6 +49,9 @@ log = logging.getLogger(__name__)
 class EmailEnrollmentState(object):
     """ Store the complete enrollment state of an email in a class """
     def __init__(self, course_id, email):
+        # N.B. retired users are not a concern here because they should be
+        # handled at a higher level (i.e. in enroll_email).  Besides, this
+        # class creates readonly objects.
         exists_user = User.objects.filter(email=email).exists()
         if exists_user:
             user = User.objects.get(email=email)
@@ -142,7 +150,7 @@ def enroll_email(course_id, student_email, auto_enroll=False, email_students=Fal
             email_params['email_address'] = student_email
             email_params['full_name'] = previous_state.full_name
             send_mail_to_student(student_email, email_params, language=language)
-    else:
+    elif not is_email_retired(student_email):
         cea, _ = CourseEnrollmentAllowed.objects.get_or_create(course_id=course_id, email=student_email)
         cea.auto_enroll = auto_enroll
         cea.save()

lms/djangoapps/instructor/tests/test_api.py

@@ -845,15 +845,23 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas
         self.assertTrue(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED)
 
     def test_user_with_retired_email_in_csv(self):
+        """
+        If the CSV contains email addresses which correspond with users which
+        have already been retired, confirm that the attempt returns invalid
+        email errors.
+        """
+
+        # This email address is re-used to create a retired account and another account.
+        conflicting_email = 'test_student@example.com'
 
         # prep a retired user
-        user = UserFactory.create(username='old_test_student', email='test_student@example.com')
+        user = UserFactory.create(username='old_test_student', email=conflicting_email)
         user.email = get_retired_email_by_email(user.email)
         user.username = get_retired_username_by_username(user.username)
         user.is_active = False
         user.save()
 
-        csv_content = "test_student@example.com,test_student,tester,USA" \
+        csv_content = "{email},{username},tester,USA".format(email=conflicting_email, username='new_test_student')
 
         uploaded_file = SimpleUploadedFile("temp.csv", csv_content)
         response = self.client.post(self.url, {'students_list': uploaded_file})
@@ -862,9 +870,9 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas
         self.assertNotEquals(len(data['row_errors']), 0)
         self.assertEquals(
             data['row_errors'][0]['response'],
-            'Invalid email {email}.'.format(email='test_student@example.com')
+            'Invalid email {email}.'.format(email=conflicting_email)
         )
-        self.assertFalse(User.objects.filter(email='test_student@example.com').exists())
+        self.assertFalse(User.objects.filter(email=conflicting_email).exists())
 
     def test_user_with_already_existing_username_in_csv(self):
         """

lms/djangoapps/instructor/views/tools.py

@@ -9,8 +9,9 @@ from django.http import HttpResponseBadRequest
 from pytz import UTC
 from django.utils.translation import ugettext as _
 from opaque_keys.edx.keys import UsageKey
-from six import text_type
+from six import text_type, string_types
 
+from student.models import get_user_by_username_or_email
 from courseware.field_overrides import disable_overrides
 from courseware.models import StudentFieldOverride
 from courseware.student_field_overrides import clear_override_for_user, get_override_for_user, override_field_for_user
@@ -50,7 +51,7 @@ def handle_dashboard_error(view):
 
 
 def strip_if_string(value):
-    if isinstance(value, basestring):
+    if isinstance(value, string_types):
         return value.strip()
     return value
 
@@ -61,14 +62,12 @@ def get_student_from_identifier(unique_student_identifier):
 
     Returns the student object associated with `unique_student_identifier`
 
-    Raises User.DoesNotExist if no user object can be found.
+    Raises User.DoesNotExist if no user object can be found, the user was
+    retired, or the user is in the process of being retired.
+
+    DEPRECATED: use student.models.get_user_by_username_or_email instead.
     """
-    unique_student_identifier = strip_if_string(unique_student_identifier)
-    if "@" in unique_student_identifier:
-        student = User.objects.get(email=unique_student_identifier)
-    else:
-        student = User.objects.get(username=unique_student_identifier)
-    return student
+    return get_user_by_username_or_email(unique_student_identifier)
 
 
 def require_student_from_identifier(unique_student_identifier):