Merge pull request #12099 from edx/christina/fix-default-cohort

Allow the "Default Group" to be renamed (automatic cohorting)

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_course_cohorts, CourseCohort, is_course_cohorted, get_random_cohort, is_default_cohort
 )
 
 import logging
@@ -34,26 +34,39 @@ def move_to_verified_cohort(sender, instance, **kwargs):  # pylint: disable=unus
 
     if verified_cohort_enabled and (instance.mode != instance._old_mode):  # pylint: disable=protected-access
         if not is_course_cohorted(course_key):
-            log.error("Automatic verified cohorting enabled for course '%s', but course is not cohorted", course_key)
+            log.error("Automatic verified cohorting enabled for course '%s', but course is not cohorted.", course_key)
         else:
-            existing_cohorts = get_course_cohorts(get_course_by_id(course_key), CourseCohort.MANUAL)
-            if any(cohort.name == verified_cohort_name for cohort in existing_cohorts):
-                args = {
-                    'course_id': unicode(course_key),
-                    'user_id': instance.user.id,
-                    'verified_cohort_name': verified_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)
+            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.
+                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)
             else:
                 log.error(
-                    "Automatic verified cohorting enabled for course '%s', but cohort named '%s' does not exist.",
+                    "Automatic verified cohorting enabled for course '%s', "
+                    "but verified cohort named '%s' does not exist.",
                     course_key,
                     verified_cohort_name,
                 )

lms/djangoapps/verified_track_content/tasks.py

@@ -9,14 +9,14 @@ from celery.utils.log import get_task_logger
 from opaque_keys.edx.keys import CourseKey
 from student.models import CourseEnrollment, CourseMode
 from openedx.core.djangoapps.course_groups.cohorts import (
-    get_cohort_by_name, get_cohort, add_user_to_cohort, DEFAULT_COHORT_NAME
+    get_cohort_by_name, get_cohort, add_user_to_cohort
 )
 
 LOGGER = get_task_logger(__name__)
 
 
 @task()
-def sync_cohort_with_mode(course_id, user_id, verified_cohort_name):
+def sync_cohort_with_mode(course_id, user_id, verified_cohort_name, default_cohort_name):
     """
     If the learner's mode does not match their assigned cohort, move the learner into the correct cohort.
     It is assumed that this task is only initiated for courses that are using the
@@ -34,17 +34,20 @@ def sync_cohort_with_mode(course_id, user_id, verified_cohort_name):
 
     if enrollment.mode == CourseMode.VERIFIED and (current_cohort.id != verified_cohort.id):
         LOGGER.info(
-            "MOVING_TO_VERIFIED: Moving user '%s' to the verified cohort for course '%s'", user.username, course_id
+            "MOVING_TO_VERIFIED: Moving user '%s' to the verified cohort '%s' for course '%s'",
+            user.username, verified_cohort.name, course_id
         )
         add_user_to_cohort(verified_cohort, user.username)
     elif enrollment.mode != CourseMode.VERIFIED and current_cohort.id == verified_cohort.id:
+        default_cohort = get_cohort_by_name(course_key, default_cohort_name)
         LOGGER.info(
-            "MOVING_TO_DEFAULT: Moving user '%s' to the default cohort for course '%s'", user.username, course_id
+            "MOVING_TO_DEFAULT: Moving user '%s' to the default cohort '%s' for course '%s'",
+            user.username, default_cohort.name, course_id
         )
-        default_cohort = get_cohort_by_name(course_key, DEFAULT_COHORT_NAME)
         add_user_to_cohort(default_cohort, user.username)
     else:
         LOGGER.info(
-            "NO_ACTION_NECESSARY: No action necessary for user '%s' in course '%s' and enrollment mode '%s'",
-            user.username, course_id, enrollment.mode
+            "NO_ACTION_NECESSARY: No action necessary for user '%s' in course '%s' and enrollment mode '%s'. "
+            "The user is already in cohort '%s'.",
+            user.username, course_id, enrollment.mode, current_cohort.name
         )

lms/djangoapps/verified_track_content/tests/test_models.py

@@ -86,6 +86,9 @@ class TestMoveToVerified(SharedModuleStoreTestCase):
     def _create_verified_cohort(self, name=DEFAULT_VERIFIED_COHORT_NAME):
         add_cohort(self.course.id, name, CourseCohort.MANUAL)
 
+    def _create_named_random_cohort(self, name):
+        return add_cohort(self.course.id, name, CourseCohort.RANDOM)
+
     def _enable_verified_track_cohorting(self, cohort_name=None):
         """ Enable verified track cohorting for the default course. """
         if cohort_name:
@@ -162,6 +165,27 @@ 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
@@ -205,10 +229,43 @@ class TestMoveToVerified(SharedModuleStoreTestCase):
         self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
 
     def test_custom_verified_cohort_name(self):
-        CUSTOM_COHORT_NAME = 'special verified cohort'
+        """
+        Test that enrolling in verified works correctly when the "verified cohort" has a custom name.
+        """
+        custom_cohort_name = 'special verified cohort'
         self._enable_cohorting()
-        self._create_verified_cohort(name=CUSTOM_COHORT_NAME)
-        self._enable_verified_track_cohorting(cohort_name=CUSTOM_COHORT_NAME)
+        self._create_verified_cohort(name=custom_cohort_name)
+        self._enable_verified_track_cohorting(cohort_name=custom_cohort_name)
         self._enroll_in_course()
         self._upgrade_to_verified()
-        self.assertEqual(CUSTOM_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
+        self.assertEqual(custom_cohort_name, get_cohort(self.user, self.course.id, assign=False).name)
+
+    def test_custom_default_cohort_name(self):
+        """
+        Test that enrolling and un-enrolling works correctly when the single cohort
+        of type random has a different name from "Default Group".
+        """
+        random_cohort_name = "custom random cohort"
+        self._enable_cohorting()
+        self._create_verified_cohort()
+        default_cohort = self._create_named_random_cohort(random_cohort_name)
+        self._enable_verified_track_cohorting()
+        self._enroll_in_course()
+        self.assertEqual(random_cohort_name, get_cohort(self.user, self.course.id, assign=False).name)
+        self._upgrade_to_verified()
+        self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
+
+        # Un-enroll from the course. The learner stays in the verified cohort, but is no longer active.
+        self._unenroll()
+        self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)
+
+        # Change the name of the "default" cohort.
+        modified_cohort_name = "renamed random cohort"
+        default_cohort.name = modified_cohort_name
+        default_cohort.save()
+
+        # Re-enroll in the course, which will downgrade the learner to audit.
+        self._reenroll()
+        self.assertEqual(modified_cohort_name, get_cohort(self.user, self.course.id, assign=False).name)
+        self._upgrade_to_verified()
+        self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name)

openedx/core/djangoapps/course_groups/cohorts.py

@@ -82,11 +82,11 @@ def _cohort_membership_changed(sender, **kwargs):
 
 
 # A 'default cohort' is an auto-cohort that is automatically created for a course if no cohort with automatic
-# assignment have been specified. It is intended to be used in a cohorted-course for users who have yet to be assigned
-# to a cohort.
-# Translation Note: We are NOT translating this string since it is the constant identifier for the "default group"
-#                   and needed across product boundaries.
-DEFAULT_COHORT_NAME = "Default Group"
+# assignment have been specified. It is intended to be used in a cohorted course for users who have yet to be assigned
+# to a cohort, if the course staff have not explicitly created a cohort of type "RANDOM".
+# Note that course staff have the ability to change the name of this cohort after creation via the cohort
+# management UI in the instructor dashboard.
+DEFAULT_COHORT_NAME = _("Default Group")
 
 
 # tl;dr: global state is bad.  capa reseeds random every time a problem is loaded.  Even
@@ -196,14 +196,17 @@ def get_cohort(user, course_key, assign=True, use_cached=False):
     # Otherwise assign the user a cohort.
     membership = CohortMembership.objects.create(
         user=user,
-        course_user_group=_get_default_cohort(course_key)
+        course_user_group=get_random_cohort(course_key)
     )
     return request_cache.data.setdefault(cache_key, membership.course_user_group)
 
 
-def _get_default_cohort(course_key):
+def get_random_cohort(course_key):
     """
-    Helper method to get a default cohort for assignment in get_cohort
+    Helper method to get a cohort for random assignment.
+
+    If there are multiple cohorts of type RANDOM in the course, one of them will be randomly selected.
+    If there are no existing cohorts of type RANDOM in the course, one will be created.
     """
     course = courses.get_course(course_key)
     cohorts = get_course_cohorts(course, assignment_type=CourseCohort.RANDOM)