TNL-160 Default cohorts, automatically created.

common/djangoapps/course_groups/cohorts.py

@@ -15,11 +15,21 @@ from .models import CourseUserGroup
 log = logging.getLogger(__name__)
 
 
+# A 'default cohort' is an auto-cohort that is automatically created for a course if no auto-cohorts 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 an administrator chooses to configure a cohort with the same name, the said cohort will also be used as
+# the "default 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 Cohort Group"
+
+
 # tl;dr: global state is bad.  capa reseeds random every time a problem is loaded.  Even
 # if and when that's fixed, it's a good idea to have a local generator to avoid any other
 # code that messes with the global random module.
 _local_random = None
 
+
 def local_random():
     """
     Get the local random number generator.  In a function so that we don't run
@@ -135,27 +145,19 @@ def get_cohort(user, course_key):
         # Didn't find the group.  We'll go on to create one if needed.
         pass
 
-    if not course.auto_cohort:
-        return None
-
     choices = course.auto_cohort_groups
-    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",
-                    course_key)
-        return None
-
-    # Put user in a random group, creating it if needed
+    if len(choices) > 0:
+        # Randomly choose one of the auto_cohort_groups, creating it if needed.
         group_name = local_random().choice(choices)
+    else:
+        # Use the "default cohort".
+        group_name = DEFAULT_COHORT_NAME
 
-    group, created = CourseUserGroup.objects.get_or_create(
+    group, _ = CourseUserGroup.objects.get_or_create(
         course_id=course_key,
         group_type=CourseUserGroup.COHORT,
         name=group_name
     )
-
     user.course_groups.add(group)
     return group
 
@@ -172,8 +174,6 @@ def get_course_cohorts(course):
         A list of CourseUserGroup objects.  Empty if there are no cohorts. Does
         not check whether the course is cohorted.
     """
-    # TODO: remove auto_cohort check with TNL-160
-    if course.auto_cohort:
     # Ensure all auto cohorts are created.
     for group_name in course.auto_cohort_groups:
         CourseUserGroup.objects.get_or_create(

common/djangoapps/course_groups/tests/helpers.py

@@ -3,6 +3,8 @@ Helper methods for testing cohorts.
 """
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore import ModuleStoreEnum
+from course_groups.models import CourseUserGroup
+from course_groups.cohorts import DEFAULT_COHORT_NAME
 
 
 def topic_name_to_id(course, name):
@@ -17,11 +19,36 @@ def topic_name_to_id(course, name):
     )
 
 
-def config_course_cohorts(course, discussions,
+def get_default_cohort(course):
+    """
+    Returns the default cohort for a course.
+    Returns None if the default cohort hasn't yet been created.
+    """
+    return get_cohort_in_course(DEFAULT_COHORT_NAME, course)
+
+
+def get_cohort_in_course(cohort_name, course):
+    """
+    Returns the cohort with the name `cohort_name` in the given `course`.
+    Returns None if it doesn't exist.
+    """
+    try:
+        return CourseUserGroup.objects.get(
+            course_id=course.id,
+            group_type=CourseUserGroup.COHORT,
+            name=cohort_name
+        )
+    except CourseUserGroup.DoesNotExist:
+        return None
+
+
+def config_course_cohorts(
+        course,
+        discussions,
         cohorted,
         cohorted_discussions=None,
-                          auto_cohort=None,
-                          auto_cohort_groups=None):
+        auto_cohort_groups=None
+):
     """
     Given a course with no discussion set up, add the discussions and set
     the cohort config appropriately.
@@ -33,7 +60,6 @@ def config_course_cohorts(course, discussions,
         cohorted: bool.
         cohorted_discussions: optional list of topic names.  If specified,
             converts them to use the same ids as topic names.
-        auto_cohort: optional bool.
         auto_cohort_groups: optional list of strings
                   (names of groups to put students into).
 
@@ -54,8 +80,6 @@ def config_course_cohorts(course, discussions,
         d["cohorted_discussions"] = [to_id(name)
                                      for name in cohorted_discussions]
 
-    if auto_cohort is not None:
-        d["auto_cohort"] = auto_cohort
     if auto_cohort_groups is not None:
         d["auto_cohort_groups"] = auto_cohort_groups
 

common/djangoapps/course_groups/tests/test_cohorts.py

@@ -8,7 +8,7 @@ from django.test.utils import override_settings
 from student.models import CourseEnrollment
 from course_groups.models import CourseUserGroup
 from course_groups import cohorts
-from course_groups.tests.helpers import topic_name_to_id, config_course_cohorts
+from course_groups.tests.helpers import topic_name_to_id, config_course_cohorts, get_default_cohort
 
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -62,10 +62,12 @@ class TestCohorts(django.test.TestCase):
         user = User.objects.create(username="test", email="a@b.com")
         self.assertIsNone(cohorts.get_cohort_id(user, course.id))
 
-        config_course_cohorts(course, [], cohorted=True)
-        cohort = CourseUserGroup.objects.create(name="TestCohort",
+        config_course_cohorts(course, discussions=[], cohorted=True)
+        cohort = CourseUserGroup.objects.create(
+            name="TestCohort",
             course_id=course.id,
-                                                group_type=CourseUserGroup.COHORT)
+            group_type=CourseUserGroup.COHORT
+        )
         cohort.users.add(user)
         self.assertEqual(cohorts.get_cohort_id(user, course.id), cohort.id)
 
@@ -87,27 +89,36 @@ class TestCohorts(django.test.TestCase):
 
         self.assertIsNone(cohorts.get_cohort(user, course.id), "No cohort created yet")
 
-        cohort = CourseUserGroup.objects.create(name="TestCohort",
+        cohort = CourseUserGroup.objects.create(
+            name="TestCohort",
             course_id=course.id,
-                                                group_type=CourseUserGroup.COHORT)
+            group_type=CourseUserGroup.COHORT
+        )
 
         cohort.users.add(user)
 
-        self.assertIsNone(cohorts.get_cohort(user, course.id),
-                          "Course isn't cohorted, so shouldn't have a cohort")
+        self.assertIsNone(
+            cohorts.get_cohort(user, course.id),
+            "Course isn't cohorted, so shouldn't have a cohort"
+        )
 
         # Make the course cohorted...
-        config_course_cohorts(course, [], cohorted=True)
+        config_course_cohorts(course, discussions=[], cohorted=True)
 
-        self.assertEquals(cohorts.get_cohort(user, course.id).id, cohort.id,
-                          "Should find the right cohort")
-
-        self.assertEquals(cohorts.get_cohort(other_user, course.id), None,
-                          "other_user shouldn't have a cohort")
+        self.assertEquals(
+            cohorts.get_cohort(user, course.id).id,
+            cohort.id,
+            "user should be assigned to the correct cohort"
+        )
+        self.assertEquals(
+            cohorts.get_cohort(other_user, course.id).id,
+            get_default_cohort(course).id,
+            "other_user should be assigned to the default cohort"
+        )
 
     def test_auto_cohorting(self):
         """
-        Make sure cohorts.get_cohort() does the right thing when the course is auto_cohorted
+        Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups
         """
         course = modulestore().get_course(self.toy_course_key)
         self.assertFalse(course.is_cohorted)
@@ -115,48 +126,65 @@ class TestCohorts(django.test.TestCase):
         user1 = User.objects.create(username="test", email="a@b.com")
         user2 = User.objects.create(username="test2", email="a2@b.com")
         user3 = User.objects.create(username="test3", email="a3@b.com")
+        user4 = User.objects.create(username="test4", email="a4@b.com")
 
-        cohort = CourseUserGroup.objects.create(name="TestCohort",
+        cohort = CourseUserGroup.objects.create(
+            name="TestCohort",
             course_id=course.id,
-                                                group_type=CourseUserGroup.COHORT)
+            group_type=CourseUserGroup.COHORT
+        )
 
         # user1 manually added to a cohort
         cohort.users.add(user1)
 
-        # Make the course auto cohorted...
+        # Add an auto_cohort_group to the course...
         config_course_cohorts(
-            course, [], cohorted=True,
-            auto_cohort=True,
+            course,
+            discussions=[],
+            cohorted=True,
             auto_cohort_groups=["AutoGroup"]
         )
 
-        self.assertEquals(cohorts.get_cohort(user1, course.id).id, cohort.id,
-                          "user1 should stay put")
+        self.assertEquals(cohorts.get_cohort(user1, course.id).id, cohort.id, "user1 should stay put")
 
-        self.assertEquals(cohorts.get_cohort(user2, course.id).name, "AutoGroup",
-                          "user2 should be auto-cohorted")
+        self.assertEquals(cohorts.get_cohort(user2, course.id).name, "AutoGroup", "user2 should be auto-cohorted")
 
-        # Now make the group list empty
+        # Now make the auto_cohort_group list empty
         config_course_cohorts(
-            course, [], cohorted=True,
-            auto_cohort=True,
+            course,
+            discussions=[],
+            cohorted=True,
             auto_cohort_groups=[]
         )
 
-        self.assertEquals(cohorts.get_cohort(user3, course.id), None,
-                          "No groups->no auto-cohorting")
+        self.assertEquals(
+            cohorts.get_cohort(user3, course.id).id,
+            get_default_cohort(course).id,
+            "No groups->default cohort"
+        )
 
-        # Now make it different
+        # Now set the auto_cohort_group to something different
         config_course_cohorts(
-            course, [], cohorted=True,
-            auto_cohort=True,
+            course,
+            discussions=[],
+            cohorted=True,
             auto_cohort_groups=["OtherGroup"]
         )
 
-        self.assertEquals(cohorts.get_cohort(user3, course.id).name, "OtherGroup",
-                          "New list->new group")
-        self.assertEquals(cohorts.get_cohort(user2, course.id).name, "AutoGroup",
-                          "user2 should still be in originally placed cohort")
+        self.assertEquals(
+            cohorts.get_cohort(user4, course.id).name, "OtherGroup", "New list->new group"
+        )
+        self.assertEquals(
+            cohorts.get_cohort(user1, course.id).name, "TestCohort", "user1 should still be in originally placed cohort"
+        )
+        self.assertEquals(
+            cohorts.get_cohort(user2, course.id).name, "AutoGroup", "user2 should still be in originally placed cohort"
+        )
+        self.assertEquals(
+            cohorts.get_cohort(user3, course.id).name,
+            get_default_cohort(course).name,
+            "user3 should still be in the default cohort"
+        )
 
     def test_auto_cohorting_randomization(self):
         """
@@ -167,15 +195,15 @@ class TestCohorts(django.test.TestCase):
 
         groups = ["group_{0}".format(n) for n in range(5)]
         config_course_cohorts(
-            course, [], cohorted=True,
-            auto_cohort=True,
-            auto_cohort_groups=groups
+            course, discussions=[], cohorted=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))
+            user = User.objects.create(
+                username="test_{0}".format(i),
+                email="a@b{0}.com".format(i)
+            )
             cohorts.get_cohort(user, course.id)
 
         # Now make sure that the assignment was at least vaguely random:
@@ -196,13 +224,13 @@ class TestCohorts(django.test.TestCase):
         config_course_cohorts(course, [], cohorted=True)
         self.assertEqual([], cohorts.get_course_cohorts(course))
 
-    def _verify_course_cohorts(self, auto_cohort, expected_cohort_set):
+    def test_get_course_cohorts(self):
         """
-        Helper method for testing get_course_cohorts with both manual and auto cohorts.
+        Tests that get_course_cohorts returns all cohorts, including auto cohorts.
         """
         course = modulestore().get_course(self.toy_course_key)
         config_course_cohorts(
-            course, [], cohorted=True, auto_cohort=auto_cohort,
+            course, [], cohorted=True,
             auto_cohort_groups=["AutoGroup1", "AutoGroup2"]
         )
 
@@ -220,21 +248,7 @@ class TestCohorts(django.test.TestCase):
         )
 
         cohort_set = {c.name for c in cohorts.get_course_cohorts(course)}
-        self.assertEqual(cohort_set, expected_cohort_set)
-
-    def test_get_course_cohorts_auto_cohort_enabled(self):
-        """
-        Tests that get_course_cohorts returns all cohorts, including auto cohorts,
-        when auto_cohort is True.
-        """
-        self._verify_course_cohorts(True, {"AutoGroup1", "AutoGroup2", "ManualCohort", "ManualCohort2"})
-
-    # TODO: Update test case with TNL-160 (auto cohorts WILL be returned).
-    def test_get_course_cohorts_auto_cohort_disabled(self):
-        """
-        Tests that get_course_cohorts does not return auto cohorts if auto_cohort is False.
-        """
-        self._verify_course_cohorts(False, {"ManualCohort", "ManualCohort2"})
+        self.assertEqual(cohort_set, {"AutoGroup1", "AutoGroup2", "ManualCohort", "ManualCohort2"})
 
     def test_is_commentable_cohorted(self):
         course = modulestore().get_course(self.toy_course_key)
@@ -244,25 +258,31 @@ class TestCohorts(django.test.TestCase):
             return topic_name_to_id(course, name)
 
         # no topics
-        self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
-                         "Course doesn't even have a 'General' topic")
+        self.assertFalse(
+            cohorts.is_commentable_cohorted(course.id, to_id("General")),
+            "Course doesn't even have a 'General' topic"
+        )
 
         # not cohorted
         config_course_cohorts(course, ["General", "Feedback"], cohorted=False)
 
-        self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
-                         "Course isn't cohorted")
+        self.assertFalse(
+            cohorts.is_commentable_cohorted(course.id, to_id("General")),
+            "Course isn't cohorted"
+        )
 
         # cohorted, but top level topics aren't
         config_course_cohorts(course, ["General", "Feedback"], cohorted=True)
 
         self.assertTrue(course.is_cohorted)
-        self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
-                         "Course is cohorted, but 'General' isn't.")
-
+        self.assertFalse(
+            cohorts.is_commentable_cohorted(course.id, to_id("General")),
+            "Course is cohorted, but 'General' isn't."
+        )
         self.assertTrue(
             cohorts.is_commentable_cohorted(course.id, to_id("random")),
-            "Non-top-level discussion is always cohorted in cohorted courses.")
+            "Non-top-level discussion is always cohorted in cohorted courses."
+        )
 
         # cohorted, including "Feedback" top-level topics aren't
         config_course_cohorts(
@@ -272,12 +292,14 @@ class TestCohorts(django.test.TestCase):
         )
 
         self.assertTrue(course.is_cohorted)
-        self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
-                         "Course is cohorted, but 'General' isn't.")
-
+        self.assertFalse(
+            cohorts.is_commentable_cohorted(course.id, to_id("General")),
+            "Course is cohorted, but 'General' isn't."
+        )
         self.assertTrue(
             cohorts.is_commentable_cohorted(course.id, to_id("Feedback")),
-            "Feedback was listed as cohorted.  Should be.")
+            "Feedback was listed as cohorted.  Should be."
+        )
 
     def test_get_cohorted_commentables(self):
         """

common/djangoapps/course_groups/tests/test_views.py

@@ -4,7 +4,7 @@ from django.test.client import RequestFactory
 from django.test.utils import override_settings
 from factory import post_generation, Sequence
 from factory.django import DjangoModelFactory
-from course_groups.tests.helpers import config_course_cohorts
+from course_groups.tests.helpers import config_course_cohorts, get_cohort_in_course, get_default_cohort
 from collections import namedtuple
 
 from django.http import Http404
@@ -18,6 +18,7 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from course_groups.views import list_cohorts, add_cohort, users_in_cohort, add_users_to_cohort, remove_user_from_cohort
+from course_groups.cohorts import get_cohort
 
 class CohortFactory(DjangoModelFactory):
     FACTORY_FOR = CourseUserGroup
@@ -61,22 +62,6 @@ class CohortViewsTestCase(ModuleStoreTestCase):
         self.cohort2 = CohortFactory.create(course_id=self.course.id, users=self.cohort2_users)
         self.cohort3 = CohortFactory.create(course_id=self.course.id, users=self.cohort3_users)
 
-    def _cohort_in_course(self, cohort_name, course):
-        """
-        Returns true iff `course` contains a cohort with the name
-        `cohort_name`.
-        """
-        try:
-            CourseUserGroup.objects.get(
-                course_id=course.id,
-                group_type=CourseUserGroup.COHORT,
-                name=cohort_name
-            )
-        except CourseUserGroup.DoesNotExist:
-            return False
-        else:
-            return True
-
     def _user_in_cohort(self, username, cohort):
         """
         Return true iff a user with `username` exists in `cohort`.
@@ -117,10 +102,14 @@ class ListCohortsTestCase(CohortViewsTestCase):
         self.assertEqual(response.status_code, 200)
         return json.loads(response.content)
 
-    def verify_lists_expected_cohorts(self, response_dict, expected_cohorts):
+    def verify_lists_expected_cohorts(self, expected_cohorts, response_dict=None):
         """
         Verify that the server response contains the expected_cohorts.
+        If response_dict is None, the list of cohorts is requested from the server.
         """
+        if response_dict is None:
+            response_dict = self.request_list_cohorts(self.course)
+
         self.assertTrue(response_dict.get("success"))
         self.assertItemsEqual(
             response_dict.get("cohorts"),
@@ -148,7 +137,7 @@ class ListCohortsTestCase(CohortViewsTestCase):
         """
         Verify that no cohorts are in response for a course with no cohorts.
         """
-        self.verify_lists_expected_cohorts(self.request_list_cohorts(self.course), [])
+        self.verify_lists_expected_cohorts([])
 
     def test_some_cohorts(self):
         """
@@ -160,13 +149,13 @@ class ListCohortsTestCase(CohortViewsTestCase):
             ListCohortsTestCase.create_expected_cohort(self.cohort2, 2),
             ListCohortsTestCase.create_expected_cohort(self.cohort3, 2),
         ]
-        self.verify_lists_expected_cohorts(self.request_list_cohorts(self.course), expected_cohorts)
+        self.verify_lists_expected_cohorts(expected_cohorts)
 
     def test_auto_cohorts(self):
         """
         Verify that auto cohorts are included in the response.
         """
-        config_course_cohorts(self.course, [], cohorted=True, auto_cohort=True,
+        config_course_cohorts(self.course, [], cohorted=True,
                               auto_cohort_groups=["AutoGroup1", "AutoGroup2"])
 
         # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated.
@@ -174,16 +163,8 @@ class ListCohortsTestCase(CohortViewsTestCase):
         # Get the cohorts from the course, which will cause auto cohorts to be created.
         actual_cohorts = self.request_list_cohorts(self.course)
         # Get references to the created auto cohorts.
-        auto_cohort_1 = CourseUserGroup.objects.get(
-            course_id=self.course.location.course_key,
-            group_type=CourseUserGroup.COHORT,
-            name="AutoGroup1"
-        )
-        auto_cohort_2 = CourseUserGroup.objects.get(
-            course_id=self.course.location.course_key,
-            group_type=CourseUserGroup.COHORT,
-            name="AutoGroup2"
-        )
+        auto_cohort_1 = get_cohort_in_course("AutoGroup1", self.course)
+        auto_cohort_2 = get_cohort_in_course("AutoGroup2", self.course)
         expected_cohorts = [
             ListCohortsTestCase.create_expected_cohort(self.cohort1, 3),
             ListCohortsTestCase.create_expected_cohort(self.cohort2, 2),
@@ -191,7 +172,36 @@ class ListCohortsTestCase(CohortViewsTestCase):
             ListCohortsTestCase.create_expected_cohort(auto_cohort_1, 0),
             ListCohortsTestCase.create_expected_cohort(auto_cohort_2, 0),
         ]
-        self.verify_lists_expected_cohorts(actual_cohorts, expected_cohorts)
+        self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts)
+
+    def test_default_cohort(self):
+        """
+        Verify that the default cohort is not created and included in the response until students are assigned to it.
+        """
+        # verify the default cohort is not created when the course is not cohorted
+        self.verify_lists_expected_cohorts([])
+
+        # create a cohorted course without any auto_cohort_groups
+        config_course_cohorts(self.course, [], cohorted=True)
+
+        # verify the default cohort is not yet created until a user is assigned
+        self.verify_lists_expected_cohorts([])
+
+        # create enrolled users
+        unassigned_users = [UserFactory.create() for _ in range(3)]
+        self._enroll_users(unassigned_users, self.course.id)
+
+        # mimic users accessing the discussion forum
+        for user in unassigned_users:
+            get_cohort(user, self.course.id)
+
+        # verify the default cohort is automatically created
+        actual_cohorts = self.request_list_cohorts(self.course)
+        default_cohort = get_default_cohort(self.course)
+        self.verify_lists_expected_cohorts(
+            [ListCohortsTestCase.create_expected_cohort(default_cohort, len(unassigned_users))],
+            actual_cohorts,
+        )
 
 
 class AddCohortTestCase(CohortViewsTestCase):
@@ -208,7 +218,7 @@ class AddCohortTestCase(CohortViewsTestCase):
         self.assertEqual(response.status_code, 200)
         return json.loads(response.content)
 
-    def verify_contains_added_cohort(self, response_dict, cohort_name, course, expected_error_msg=None):
+    def verify_contains_added_cohort(self, response_dict, cohort_name, expected_error_msg=None):
         """
         Check that `add_cohort`'s response correctly returns the newly added
         cohort (or error) in the response.  Also verify that the cohort was
@@ -226,7 +236,7 @@ class AddCohortTestCase(CohortViewsTestCase):
                 response_dict.get("cohort").get("name"),
                 cohort_name
             )
-        self.assertTrue(self._cohort_in_course(cohort_name, course))
+        self.assertIsNotNone(get_cohort_in_course(cohort_name, self.course))
 
     def test_non_staff(self):
         """
@@ -242,7 +252,6 @@ class AddCohortTestCase(CohortViewsTestCase):
         self.verify_contains_added_cohort(
             self.request_add_cohort(cohort_name, self.course),
             cohort_name,
-            self.course
         )
 
     def test_no_cohort(self):
@@ -263,7 +272,6 @@ class AddCohortTestCase(CohortViewsTestCase):
         self.verify_contains_added_cohort(
             self.request_add_cohort(cohort_name, self.course),
             cohort_name,
-            self.course,
             expected_error_msg="Can't create two cohorts with the same name"
         )