Merge pull request #5133 from edx/christina/cohorts-api

Create auto cohorts when list_cohorts is called.

common/djangoapps/course_groups/cohorts.py

@@ -160,19 +160,30 @@ def get_cohort(user, course_key):
     return group
 
 
-def get_course_cohorts(course_key):
+def get_course_cohorts(course):
     """
-    Get a list of all the cohorts in the given course.
+    Get a list of all the cohorts in the given course. This will include auto cohorts,
+    regardless of whether or not the auto cohorts include any users.
 
     Arguments:
-        course_key: CourseKey
+        course: the course for which cohorts should be returned
 
     Returns:
         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(
+                course_id=course.location.course_key,
+                group_type=CourseUserGroup.COHORT,
+                name=group_name
+            )
+
     return list(CourseUserGroup.objects.filter(
-        course_id=course_key,
+        course_id=course.location.course_key,
         group_type=CourseUserGroup.COHORT
     ))
 
@@ -269,13 +280,6 @@ def add_user_to_cohort(cohort, username_or_email):
     return (user, previous_cohort)
 
 
-def get_course_cohort_names(course_key):
-    """
-    Return a list of the cohort names in a course.
-    """
-    return [c.name for c in get_course_cohorts(course_key)]
-
-
 def delete_empty_cohort(course_key, name):
     """
     Remove an empty cohort.  Raise ValueError if cohort is not empty.

common/djangoapps/course_groups/tests/helpers.py

+"""
+Helper methods for testing cohorts.
+"""
+from xmodule.modulestore.django import modulestore
+from xmodule.modulestore import ModuleStoreEnum
+
+
+def topic_name_to_id(course, name):
+    """
+    Given a discussion topic name, return an id for that name (includes
+    course and url_name).
+    """
+    return "{course}_{run}_{name}".format(
+        course=course.location.course,
+        run=course.url_name,
+        name=name
+    )
+
+
+def config_course_cohorts(course, discussions,
+                          cohorted,
+                          cohorted_discussions=None,
+                          auto_cohort=None,
+                          auto_cohort_groups=None):
+    """
+    Given a course with no discussion set up, add the discussions and set
+    the cohort config appropriately.
+
+    Arguments:
+        course: CourseDescriptor
+        discussions: list of topic names strings.  Picks ids and sort_keys
+            automatically.
+        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).
+
+    Returns:
+        Nothing -- modifies course in place.
+    """
+    def to_id(name):
+        return topic_name_to_id(course, name)
+
+    topics = dict((name, {"sort_key": "A",
+                          "id": to_id(name)})
+                  for name in discussions)
+
+    course.discussion_topics = topics
+
+    d = {"cohorted": cohorted}
+    if cohorted_discussions is not None:
+        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
+
+    course.cohort_config = d
+
+    try:
+        # Not implemented for XMLModulestore, which is used by test_cohorts.
+        modulestore().update_item(course, ModuleStoreEnum.UserID.test)
+    except NotImplementedError:
+        pass

common/djangoapps/course_groups/tests/test_cohorts.py

@@ -8,6 +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 xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -26,61 +27,6 @@ TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING)
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class TestCohorts(django.test.TestCase):
 
-    @staticmethod
-    def topic_name_to_id(course, name):
-        """
-        Given a discussion topic name, return an id for that name (includes
-        course and url_name).
-        """
-        return "{course}_{run}_{name}".format(course=course.location.course,
-                                              run=course.url_name,
-                                              name=name)
-
-    @staticmethod
-    def config_course_cohorts(course, discussions,
-                              cohorted,
-                              cohorted_discussions=None,
-                              auto_cohort=None,
-                              auto_cohort_groups=None):
-        """
-        Given a course with no discussion set up, add the discussions and set
-        the cohort config appropriately.
-
-        Arguments:
-            course: CourseDescriptor
-            discussions: list of topic names strings.  Picks ids and sort_keys
-                automatically.
-            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).
-
-        Returns:
-            Nothing -- modifies course in place.
-        """
-        def to_id(name):
-            return TestCohorts.topic_name_to_id(course, name)
-
-        topics = dict((name, {"sort_key": "A",
-                              "id": to_id(name)})
-                      for name in discussions)
-
-        course.discussion_topics = topics
-
-        d = {"cohorted": cohorted}
-        if cohorted_discussions is not None:
-            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
-
-        course.cohort_config = d
-
     def setUp(self):
         """
         Make sure that course is reloaded every time--clear out the modulestore.
@@ -96,7 +42,7 @@ class TestCohorts(django.test.TestCase):
         self.assertFalse(course.is_cohorted)
         self.assertFalse(cohorts.is_course_cohorted(course.id))
 
-        self.config_course_cohorts(course, [], cohorted=True)
+        config_course_cohorts(course, [], cohorted=True)
 
         self.assertTrue(course.is_cohorted)
         self.assertTrue(cohorts.is_course_cohorted(course.id))
@@ -116,7 +62,7 @@ class TestCohorts(django.test.TestCase):
         user = User.objects.create(username="test", email="a@b.com")
         self.assertIsNone(cohorts.get_cohort_id(user, course.id))
 
-        self.config_course_cohorts(course, [], cohorted=True)
+        config_course_cohorts(course, [], cohorted=True)
         cohort = CourseUserGroup.objects.create(name="TestCohort",
                                                 course_id=course.id,
                                                 group_type=CourseUserGroup.COHORT)
@@ -151,7 +97,7 @@ class TestCohorts(django.test.TestCase):
                           "Course isn't cohorted, so shouldn't have a cohort")
 
         # Make the course cohorted...
-        self.config_course_cohorts(course, [], cohorted=True)
+        config_course_cohorts(course, [], cohorted=True)
 
         self.assertEquals(cohorts.get_cohort(user, course.id).id, cohort.id,
                           "Should find the right cohort")
@@ -178,9 +124,11 @@ class TestCohorts(django.test.TestCase):
         cohort.users.add(user1)
 
         # Make the course auto cohorted...
-        self.config_course_cohorts(course, [], cohorted=True,
-                                   auto_cohort=True,
-                                   auto_cohort_groups=["AutoGroup"])
+        config_course_cohorts(
+            course, [], cohorted=True,
+            auto_cohort=True,
+            auto_cohort_groups=["AutoGroup"]
+        )
 
         self.assertEquals(cohorts.get_cohort(user1, course.id).id, cohort.id,
                           "user1 should stay put")
@@ -189,17 +137,21 @@ class TestCohorts(django.test.TestCase):
                           "user2 should be auto-cohorted")
 
         # Now make the group list empty
-        self.config_course_cohorts(course, [], cohorted=True,
-                                   auto_cohort=True,
-                                   auto_cohort_groups=[])
+        config_course_cohorts(
+            course, [], cohorted=True,
+            auto_cohort=True,
+            auto_cohort_groups=[]
+        )
 
         self.assertEquals(cohorts.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,
-                                   auto_cohort_groups=["OtherGroup"])
+        config_course_cohorts(
+            course, [], cohorted=True,
+            auto_cohort=True,
+            auto_cohort_groups=["OtherGroup"]
+        )
 
         self.assertEquals(cohorts.get_cohort(user3, course.id).name, "OtherGroup",
                           "New list->new group")
@@ -214,9 +166,11 @@ class TestCohorts(django.test.TestCase):
         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)
+        config_course_cohorts(
+            course, [], cohorted=True,
+            auto_cohort=True,
+            auto_cohort_groups=groups
+        )
 
         # Assign 100 users to cohorts
         for i in range(100):
@@ -234,46 +188,73 @@ class TestCohorts(django.test.TestCase):
             self.assertGreater(num_users, 1)
             self.assertLess(num_users, 50)
 
-    def test_get_course_cohorts(self):
-        course1_key = SlashSeparatedCourseKey('a', 'b', 'c')
-        course2_key = SlashSeparatedCourseKey('e', 'f', 'g')
+    def test_get_course_cohorts_noop(self):
+        """
+        Tests get_course_cohorts returns an empty list when no cohorts exist.
+        """
+        course = modulestore().get_course(self.toy_course_key)
+        config_course_cohorts(course, [], cohorted=True)
+        self.assertEqual([], cohorts.get_course_cohorts(course))
 
-        # add some cohorts to course 1
-        cohort = CourseUserGroup.objects.create(name="TestCohort",
-                                                course_id=course1_key,
-                                                group_type=CourseUserGroup.COHORT)
+    def _verify_course_cohorts(self, auto_cohort, expected_cohort_set):
+        """
+        Helper method for testing get_course_cohorts with both manual and auto cohorts.
+        """
+        course = modulestore().get_course(self.toy_course_key)
+        config_course_cohorts(
+            course, [], cohorted=True, auto_cohort=auto_cohort,
+            auto_cohort_groups=["AutoGroup1", "AutoGroup2"]
+        )
 
-        cohort = CourseUserGroup.objects.create(name="TestCohort2",
-                                                course_id=course1_key,
-                                                group_type=CourseUserGroup.COHORT)
+        # add manual cohorts to course 1
+        CourseUserGroup.objects.create(
+            name="ManualCohort",
+            course_id=course.location.course_key,
+            group_type=CourseUserGroup.COHORT
+        )
 
-        # second course should have no cohorts
-        self.assertEqual(cohorts.get_course_cohorts(course2_key), [])
+        CourseUserGroup.objects.create(
+            name="ManualCohort2",
+            course_id=course.location.course_key,
+            group_type=CourseUserGroup.COHORT
+        )
 
-        course1_cohorts = sorted([c.name for c in cohorts.get_course_cohorts(course1_key)])
-        self.assertEqual(course1_cohorts, ['TestCohort', 'TestCohort2'])
+        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"})
 
     def test_is_commentable_cohorted(self):
         course = modulestore().get_course(self.toy_course_key)
         self.assertFalse(course.is_cohorted)
 
         def to_id(name):
-            return self.topic_name_to_id(course, name)
+            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")
 
         # not cohorted
-        self.config_course_cohorts(course, ["General", "Feedback"],
-                                   cohorted=False)
+        config_course_cohorts(course, ["General", "Feedback"], cohorted=False)
 
         self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
                          "Course isn't cohorted")
 
         # cohorted, but top level topics aren't
-        self.config_course_cohorts(course, ["General", "Feedback"],
-                                   cohorted=True)
+        config_course_cohorts(course, ["General", "Feedback"], cohorted=True)
 
         self.assertTrue(course.is_cohorted)
         self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
@@ -284,9 +265,11 @@ class TestCohorts(django.test.TestCase):
             "Non-top-level discussion is always cohorted in cohorted courses.")
 
         # cohorted, including "Feedback" top-level topics aren't
-        self.config_course_cohorts(course, ["General", "Feedback"],
-                                   cohorted=True,
-                                   cohorted_discussions=["Feedback"])
+        config_course_cohorts(
+            course, ["General", "Feedback"],
+            cohorted=True,
+            cohorted_discussions=["Feedback"]
+        )
 
         self.assertTrue(course.is_cohorted)
         self.assertFalse(cohorts.is_commentable_cohorted(course.id, to_id("General")),
@@ -305,28 +288,27 @@ class TestCohorts(django.test.TestCase):
 
         self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
 
-        self.config_course_cohorts(course, [], cohorted=True)
+        config_course_cohorts(course, [], cohorted=True)
         self.assertEqual(cohorts.get_cohorted_commentables(course.id), set())
 
-        self.config_course_cohorts(
+        config_course_cohorts(
             course, ["General", "Feedback"],
             cohorted=True,
             cohorted_discussions=["Feedback"]
         )
         self.assertItemsEqual(
             cohorts.get_cohorted_commentables(course.id),
-            set([self.topic_name_to_id(course, "Feedback")])
+            set([topic_name_to_id(course, "Feedback")])
         )
 
-        self.config_course_cohorts(
+        config_course_cohorts(
             course, ["General", "Feedback"],
             cohorted=True,
             cohorted_discussions=["General", "Feedback"]
         )
         self.assertItemsEqual(
             cohorts.get_cohorted_commentables(course.id),
-            set([self.topic_name_to_id(course, "General"),
-                 self.topic_name_to_id(course, "Feedback")])
+            set([topic_name_to_id(course, "General"), topic_name_to_id(course, "Feedback")])
         )
         self.assertRaises(
             Http404,
@@ -442,44 +424,6 @@ class TestCohorts(django.test.TestCase):
             lambda: cohorts.add_user_to_cohort(first_cohort, "non_existent_username")
         )
 
-    def test_get_course_cohort_names(self):
-        """
-        Make sure cohorts.get_course_cohort_names() properly returns a list of cohort
-        names for a given course.
-        """
-        course = modulestore().get_course(self.toy_course_key)
-
-        self.assertItemsEqual(
-            cohorts.get_course_cohort_names(course.id),
-            []
-        )
-        self.assertItemsEqual(
-            cohorts.get_course_cohort_names(SlashSeparatedCourseKey('course', 'does_not', 'exist')),
-            []
-        )
-
-        CourseUserGroup.objects.create(
-            name="FirstCohort",
-            course_id=course.id,
-            group_type=CourseUserGroup.COHORT
-        )
-
-        self.assertItemsEqual(
-            cohorts.get_course_cohort_names(course.id),
-            ["FirstCohort"]
-        )
-
-        CourseUserGroup.objects.create(
-            name="SecondCohort",
-            course_id=course.id,
-            group_type=CourseUserGroup.COHORT
-        )
-
-        self.assertItemsEqual(
-            cohorts.get_course_cohort_names(course.id),
-            ["FirstCohort", "SecondCohort"]
-        )
-
     def test_delete_empty_cohort(self):
         """
         Make sure that cohorts.delete_empty_cohort() properly removes an empty cohort

common/djangoapps/course_groups/tests/test_views.py

@@ -4,6 +4,8 @@ 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 collections import namedtuple
 
 from django.http import Http404
 from django.contrib.auth.models import User
@@ -123,11 +125,19 @@ class ListCohortsTestCase(CohortViewsTestCase):
         self.assertItemsEqual(
             response_dict.get("cohorts"),
             [
-                {"name": cohort.name, "id": cohort.id}
+                {"name": cohort.name, "id": cohort.id, "user_count": cohort.user_count}
                 for cohort in expected_cohorts
             ]
         )
 
+    @staticmethod
+    def create_expected_cohort(cohort, user_count):
+        """
+        Create a tuple storing the expected cohort information.
+        """
+        cohort_tuple = namedtuple("Cohort", "name id user_count")
+        return cohort_tuple(name=cohort.name, id=cohort.id, user_count=user_count)
+
     def test_non_staff(self):
         """
         Verify that we cannot access list_cohorts if we're a non-staff user.
@@ -145,12 +155,44 @@ class ListCohortsTestCase(CohortViewsTestCase):
         Verify that cohorts are in response for a course with some cohorts.
         """
         self._create_cohorts()
-        expected_cohorts = CourseUserGroup.objects.filter(
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
-        )
+        expected_cohorts = [
+            ListCohortsTestCase.create_expected_cohort(self.cohort1, 3),
+            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)
 
+    def test_auto_cohorts(self):
+        """
+        Verify that auto cohorts are included in the response.
+        """
+        config_course_cohorts(self.course, [], cohorted=True, auto_cohort=True,
+                              auto_cohort_groups=["AutoGroup1", "AutoGroup2"])
+
+        # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated.
+        self._create_cohorts()
+        # 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"
+        )
+        expected_cohorts = [
+            ListCohortsTestCase.create_expected_cohort(self.cohort1, 3),
+            ListCohortsTestCase.create_expected_cohort(self.cohort2, 2),
+            ListCohortsTestCase.create_expected_cohort(self.cohort3, 2),
+            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)
+
 
 class AddCohortTestCase(CohortViewsTestCase):
     """

common/djangoapps/course_groups/views.py

@@ -46,10 +46,10 @@ def list_cohorts(request, course_key_string):
     # this is a string when we get it here
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string)
 
-    get_course_with_access(request.user, 'staff', course_key)
+    course = get_course_with_access(request.user, 'staff', course_key)
 
-    all_cohorts = [{'name': c.name, 'id': c.id}
-                   for c in cohorts.get_course_cohorts(course_key)]
+    all_cohorts = [{'name': c.name, 'id': c.id, 'user_count': c.users.count()}
+                   for c in cohorts.get_course_cohorts(course)]
 
     return json_http_response({'success': True,
                                'cohorts': all_cohorts})

lms/djangoapps/django_comment_client/forum/views.py

@@ -11,8 +11,7 @@ import newrelic.agent
 
 from edxmako.shortcuts import render_to_response
 from courseware.courses import get_course_with_access
-from course_groups.cohorts import (is_course_cohorted, get_cohort_id, is_commentable_cohorted,
-                                   get_cohorted_commentables, get_course_cohorts, get_cohort_by_id)
+from course_groups.cohorts import is_course_cohorted, get_cohort_id, get_course_cohorts, is_commentable_cohorted
 from courseware.access import has_access
 
 from django_comment_client.permissions import cached_has_permission
@@ -52,7 +51,7 @@ def make_course_settings(course, include_category_map=False):
         'is_cohorted': is_course_cohorted(course.id),
         'allow_anonymous': course.allow_anonymous,
         'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
-        'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course.id)],
+        'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
     }
 
     if include_category_map: