Merge pull request #1650 from MITx/victor/fix-auto-cohorting

Add logging to debug strange cohort behavior on prod.

common/djangoapps/course_groups/cohorts.py

@@ -65,23 +65,23 @@ def is_commentable_cohorted(course_id, commentable_id):
                                                                ans))
     return ans
 
-    
+
 def get_cohorted_commentables(course_id):
     """
     Given a course_id return a list of strings representing cohorted commentables
     """
 
     course = courses.get_course_by_id(course_id)
-    
+
     if not course.is_cohorted:
         # this is the easy case :)
         ans = []
-    else: 
+    else:
         ans = course.cohorted_discussions
 
     return ans
-    
-    
+
+
 def get_cohort(user, course_id):
     """
     Given a django User and a course_id, return the user's cohort in that
@@ -120,7 +120,8 @@ def get_cohort(user, course_id):
         return None
 
     choices = course.auto_cohort_groups
-    if len(choices) == 0:
+    n = len(choices)
+    if n == 0:
         # Nowhere to put user
         log.warning("Course %s is auto-cohorted, but there are no"
                     " auto_cohort_groups specified",
@@ -128,12 +129,19 @@ def get_cohort(user, course_id):
         return None
 
     # Put user in a random group, creating it if needed
-    group_name = random.choice(choices)
+    choice = random.randrange(0, n)
+    group_name = choices[choice]
+
+    # Victor: we are seeing very strange behavior on prod, where almost all users
+    # end up in the same group.  Log at INFO to try to figure out what's going on.
+    log.info("DEBUG: adding user {0} to cohort {1}.  choice={2}".format(
+        user, group_name,choice))
+
     group, created = CourseUserGroup.objects.get_or_create(
         course_id=course_id,
         group_type=CourseUserGroup.COHORT,
         name=group_name)
-    
+
     user.course_groups.add(group)
     return group
 

common/djangoapps/course_groups/tests/tests.py

@@ -6,7 +6,7 @@ from django.test.utils import override_settings
 
 from course_groups.models import CourseUserGroup
 from course_groups.cohorts import (get_cohort, get_course_cohorts,
-                                   is_commentable_cohorted)
+                                   is_commentable_cohorted, get_cohort_by_name)
 
 from xmodule.modulestore.django import modulestore, _MODULESTORES
 
@@ -168,7 +168,7 @@ class TestCohorts(django.test.TestCase):
 
         self.assertEquals(get_cohort(user3, course.id), None,
                           "No groups->no auto-cohorting")
-        
+
         # Now make it different
         self.config_course_cohorts(course, [], cohorted=True,
                                    auto_cohort=True,
@@ -180,6 +180,37 @@ class TestCohorts(django.test.TestCase):
                           "user2 should still be in originally placed cohort")
 
 
+    def test_auto_cohorting_randomization(self):
+        """
+        Make sure get_cohort() randomizes properly.
+        """
+        course = modulestore().get_course("edX/toy/2012_Fall")
+        self.assertEqual(course.id, "edX/toy/2012_Fall")
+        self.assertFalse(course.is_cohorted)
+
+        groups = ["group_{0}".format(n) for n in range(5)]
+        self.config_course_cohorts(course, [], cohorted=True,
+                                   auto_cohort=True,
+                                   auto_cohort_groups=groups)
+
+        # Assign 100 users to cohorts
+        for i in range(100):
+            user = User.objects.create(username="test_{0}".format(i),
+                                       email="a@b{0}.com".format(i))
+            get_cohort(user, course.id)
+
+        # Now make sure that the assignment was at least vaguely random:
+        # each cohort should have at least 1, and fewer than 50 students.
+        # (with 5 groups, probability of 0 users in any group is about
+        # .8**100= 2.0e-10)
+        for cohort_name in groups:
+            cohort = get_cohort_by_name(course.id, cohort_name)
+            num_users = cohort.users.count()
+            self.assertGreater(num_users, 1)
+            self.assertLess(num_users, 50)
+
+
+
     def test_get_course_cohorts(self):
         course1_id = 'a/b/c'
         course2_id = 'e/f/g'