Merge pull request #12155 from edx/christina/allow-multiple-random-cohorts

Allow multiple random cohorts.

lms/djangoapps/verified_track_content/models.py

@@ -12,7 +12,7 @@ from courseware.courses import get_course_by_id
 
 from verified_track_content.tasks import sync_cohort_with_mode
 from openedx.core.djangoapps.course_groups.cohorts import (
-    get_course_cohorts, CourseCohort, is_course_cohorted, get_random_cohort, is_default_cohort
+    get_course_cohorts, CourseCohort, is_course_cohorted, get_random_cohort
 )
 
 import logging
@@ -39,30 +39,24 @@ def move_to_verified_cohort(sender, instance, **kwargs):  # pylint: disable=unus
             course = get_course_by_id(course_key)
             existing_manual_cohorts = get_course_cohorts(course, CourseCohort.MANUAL)
             if any(cohort.name == verified_cohort_name for cohort in existing_manual_cohorts):
-                # Verify that a single random cohort exists in the course. Note that calling this method will create
-                # a "Default Group" random cohort if no random cohorts exist yet.
+                # Get a random cohort to use as the default cohort (for audit learners).
+                # Note that calling this method will create a "Default Group" random cohort if no random
+                # cohort yet exist.
                 random_cohort = get_random_cohort(course_key)
-                if not is_default_cohort(random_cohort):
-                    log.error(
-                        "Automatic verified cohorting enabled for course '%s', "
-                        "but course does not have exactly one default cohort for audit learners.",
-                        course_key
-                    )
-                else:
-                    args = {
-                        'course_id': unicode(course_key),
-                        'user_id': instance.user.id,
-                        'verified_cohort_name': verified_cohort_name,
-                        'default_cohort_name': random_cohort.name
-                    }
-                    # Do the update with a 3-second delay in hopes that the CourseEnrollment transaction has been
-                    # completed before the celery task runs. We want a reasonably short delay in case the learner
-                    # immediately goes to the courseware.
-                    sync_cohort_with_mode.apply_async(kwargs=args, countdown=3)
-
-                    # In case the transaction actually was not committed before the celery task runs,
-                    # run it again after 5 minutes. If the first completed successfully, this task will be a no-op.
-                    sync_cohort_with_mode.apply_async(kwargs=args, countdown=300)
+                args = {
+                    'course_id': unicode(course_key),
+                    'user_id': instance.user.id,
+                    'verified_cohort_name': verified_cohort_name,
+                    'default_cohort_name': random_cohort.name
+                }
+                # Do the update with a 3-second delay in hopes that the CourseEnrollment transaction has been
+                # completed before the celery task runs. We want a reasonably short delay in case the learner
+                # immediately goes to the courseware.
+                sync_cohort_with_mode.apply_async(kwargs=args, countdown=3)
+
+                # In case the transaction actually was not committed before the celery task runs,
+                # run it again after 5 minutes. If the first completed successfully, this task will be a no-op.
+                sync_cohort_with_mode.apply_async(kwargs=args, countdown=300)
             else:
                 log.error(
                     "Automatic verified cohorting enabled for course '%s', "

lms/djangoapps/verified_track_content/tests/test_forms.py

 """
 Test for forms helpers.
 """
-from opaque_keys.edx.keys import CourseKey
-
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 

lms/djangoapps/verified_track_content/tests/test_models.py

@@ -165,27 +165,6 @@ class TestMoveToVerified(SharedModuleStoreTestCase):
         error_message = "cohort named '%s' does not exist"
         self.assertIn(error_message, error_logger.call_args[0][0])
 
-    @mock.patch('verified_track_content.models.log.error')
-    def test_cohorting_enabled_too_many_random_cohorts(self, error_logger):
-        """
-        If the VerifiedTrackCohortedCourse feature is enabled for a course and the course is cohorted,
-        but the course has > 1 random cohorts, an error is logged and enrollment mode changes do not
-        move learners into a cohort.
-        """
-        # Enable cohorting, and create the verified cohort.
-        self._enable_cohorting()
-        self._create_verified_cohort()
-        # Create two random cohorts.
-        self._create_named_random_cohort("Random 1")
-        self._create_named_random_cohort("Random 2")
-        # Enable verified track cohorting feature
-        self._enable_verified_track_cohorting()
-        self.assertTrue(VerifiedTrackCohortedCourse.is_verified_track_cohort_enabled(self.course.id))
-        self._verify_no_automatic_cohorting()
-        self.assertTrue(error_logger.called)
-        error_message = "course does not have exactly one default cohort"
-        self.assertIn(error_message, error_logger.call_args[0][0])
-
     def test_automatic_cohorting_enabled(self):
         """
         If the VerifiedTrackCohortedCourse feature is enabled for a course (with course cohorting enabled
@@ -207,6 +186,30 @@ class TestMoveToVerified(SharedModuleStoreTestCase):
         self.assertEqual(4, self.mocked_celery_task.call_count)
         self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
 
+    def test_cohorting_enabled_multiple_random_cohorts(self):
+        """
+        If the VerifiedTrackCohortedCourse feature is enabled for a course, and the course is cohorted
+        with > 1 random cohorts, the learner is randomly assigned to one of the random
+        cohorts when in the audit track.
+        """
+        # Enable cohorting, and create the verified cohort.
+        self._enable_cohorting()
+        self._create_verified_cohort()
+        # Create two random cohorts.
+        self._create_named_random_cohort("Random 1")
+        self._create_named_random_cohort("Random 2")
+        # Enable verified track cohorting feature
+        self._enable_verified_track_cohorting()
+
+        self._enroll_in_course()
+        self.assertIn(get_cohort(self.user, self.course.id, assign=False).name, ["Random 1", "Random 2"])
+        self._upgrade_to_verified()
+        self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
+
+        self._unenroll()
+        self._reenroll()
+        self.assertIn(get_cohort(self.user, self.course.id, assign=False).name, ["Random 1", "Random 2"])
+
     def test_unenrolled(self):
         """
         Test that un-enrolling and re-enrolling works correctly. This is important because usually

openedx/core/djangoapps/course_groups/cohorts.py

@@ -430,7 +430,7 @@ def set_assignment_type(user_group, assignment_type):
     """
     course_cohort = user_group.cohort
 
-    if is_default_cohort(user_group) and course_cohort.assignment_type != assignment_type:
+    if is_last_random_cohort(user_group) and course_cohort.assignment_type != assignment_type:
         raise ValueError(_("There must be one cohort to which students can automatically be assigned."))
 
     course_cohort.assignment_type = assignment_type
@@ -445,9 +445,9 @@ def get_assignment_type(user_group):
     return course_cohort.assignment_type
 
 
-def is_default_cohort(user_group):
+def is_last_random_cohort(user_group):
     """
-    Check if a cohort is default.
+    Check if this cohort is the only random cohort in the course.
     """
     random_cohorts = CourseUserGroup.objects.filter(
         course_id=user_group.course_id,