Fixing 500 when user is enrolled in a course for the second time

lms/djangoapps/api_manager/users/tests.py

@@ -374,15 +374,34 @@ class UsersApiTests(ModuleStoreTestCase):
     def test_user_list_post_duplicate(self):
         test_uri = self.users_base_uri
         local_username = self.test_username + str(randint(11, 99))
+
+        def post_duplicate_and_assert_409(email, username):
+            """
+            Posts user data with and asserts that return status code was 409 CONFLICT
+            """
+            data = {'email': email, 'username': username, 'password': self.test_password}
+            expected_message = "Username '{username}' or email '{email}' already exists".format(
+                username=username, email=email
+            )
+            response = self.do_post(test_uri, data)
+            self.assertEqual(response.status_code, 409)
+            self.assertEqual(response.data['message'], expected_message)
+            self.assertEqual(response.data['field_conflict'], 'username or email')
+
         data = {'email': self.test_email, 'username': local_username, 'password':
                 self.test_password, 'first_name': self.test_first_name, 'last_name': self.test_last_name}
         response = self.do_post(test_uri, data)
-        expected_message = "Username '{username}' or email '{email}' already exists".format(
-            username=local_username, email=self.test_email
-        )
-        self.assertEqual(response.status_code, 409)
-        self.assertEqual(response.data['message'], expected_message)
-        self.assertEqual(response.data['field_conflict'], 'username or email')
+        self.assertEqual(response.status_code, 201)
+
+        # try creating a user with same email and username
+        post_duplicate_and_assert_409(self.test_email, local_username)
+
+        # try creating a user with same username
+        post_duplicate_and_assert_409(str(uuid.uuid4()) + '@test.org', local_username)
+
+        # creating a user with same email - does not work, since auth_user table in test database does not have unique
+        # constraint on email, added by a migration in common/djangoapps/student/migrations/0004_add_email_index.py
+        # Test engine uses in-memory sqlite DB, so migrations are not applied to it
 
     @mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_EMAIL_DIGEST": True})
     def test_user_list_post_discussion_digest_email(self):
@@ -840,6 +859,26 @@ class UsersApiTests(ModuleStoreTestCase):
         self.assertEqual(response.data['id'], unicode(self.course.id))
         self.assertTrue(response.data['is_active'])
 
+    def test_user_courses_list_post_duplicate(self):
+        # creating user
+        test_uri = self.users_base_uri
+        local_username = self.test_username + str(randint(11, 99))
+        data = {'email': self.test_email, 'username': local_username, 'password':
+                self.test_password, 'first_name': self.test_first_name, 'last_name': self.test_last_name}
+        response = self.do_post(test_uri, data)
+        user_id = response.data['id']
+
+        # adding it to a cohort
+        test_uri = '{}/{}/courses'.format(test_uri, str(user_id))
+        data = {'course_id': unicode(self.course.id)}
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 201)
+
+        # and trying to add it second time
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 409)
+        self.assertIn("already added to cohort", response.data['message'])
+
     def test_user_courses_list_post_undefined_user(self):
         course = CourseFactory.create(org='TUCLPUU', run='TUCLPUU1')
         test_uri = self.users_base_uri

lms/djangoapps/api_manager/users/views.py

@@ -34,7 +34,8 @@ from openedx.core.djangoapps.course_groups.cohorts import (
     get_cohort_by_name,
     add_cohort,
     add_user_to_cohort,
-    remove_user_from_cohort
+    remove_user_from_cohort,
+    AlreadyAddedToCohortException
 )
 from openedx.core.djangoapps.user_api.models import UserPreference
 from openedx.core.djangoapps.user_api.preferences.api import set_user_preference
@@ -769,7 +770,18 @@ class UsersCoursesList(SecureAPIView):
             default_cohort = get_cohort_by_name(course_key, CourseUserGroup.default_cohort_name)
         except CourseUserGroup.DoesNotExist:
             default_cohort = add_cohort(course_key, CourseUserGroup.default_cohort_name, CourseCohort.RANDOM)
-        add_user_to_cohort(default_cohort, user.username)
+        try:
+            add_user_to_cohort(default_cohort, user.username)
+        except AlreadyAddedToCohortException:
+            msg_tpl = _('Student {student} already added to cohort {cohort_name} for course {course}')
+            # pylint reports msg_tpl to not have `format` member, which is obviously a type resolution issue
+            # related - http://stackoverflow.com/questions/10025710/pylint-reports-as-not-callable
+            # pylint: disable=no-member
+            response_data = {
+                'message': msg_tpl.format(student=user.username, cohort_name=default_cohort.name, course=course_key)
+            }
+            return Response(response_data, status=status.HTTP_409_CONFLICT)
+
         log.debug('User "{}" has been automatically added in cohort "{}" for course "{}"'.format(
             user.username, default_cohort.name, course_descriptor.display_name)
         )

openedx/core/djangoapps/course_groups/cohorts.py

@@ -20,6 +20,13 @@ from student.models import get_user_by_username_or_email
 from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup
 
 
+class AlreadyAddedToCohortException(ValueError):
+    """
+    Raised when an attempt is made to add user to a cohort he's already added to
+    """
+    pass
+
+
 log = logging.getLogger(__name__)
 
 
@@ -386,7 +393,7 @@ def add_user_to_cohort(cohort, username_or_email):
     )
     if course_cohorts.exists():
         if course_cohorts[0] == cohort:
-            raise ValueError("User {user_name} already present in cohort {cohort_name}".format(
+            raise AlreadyAddedToCohortException("User {user_name} already present in cohort {cohort_name}".format(
                 user_name=user.username,
                 cohort_name=cohort.name
             ))

openedx/core/djangoapps/course_groups/tests/test_cohorts.py

@@ -706,9 +706,9 @@ class TestCohorts(ModuleStoreTestCase):
             }
         )
         # Error cases
-        # Should get ValueError if user already in cohort
+        # Should get AlreadyAddedToCohortException if user already in cohort
         self.assertRaises(
-            ValueError,
+            cohorts.AlreadyAddedToCohortException,
             lambda: cohorts.add_user_to_cohort(second_cohort, "Username")
         )
         # UserDoesNotExist if user truly does not exist