Merge pull request #1357 from MITx/feature/victor/cohorts

refactor tests, add test for is_commentable_cohorted, fix a bug

common/djangoapps/course_groups/cohorts.py

@@ -35,8 +35,12 @@ def get_cohort_id(user, course_id):
 
 def is_commentable_cohorted(course_id, commentable_id):
     """
-    Given a course and a commentable id, return whether or not this commentable
-    is cohorted.
+    Args:
+        course_id: string
+        commentable_id: string
+
+    Returns:
+        Bool: is this commentable cohorted?
 
     Raises:
         Http404 if the course doesn't exist.
@@ -49,7 +53,7 @@ def is_commentable_cohorted(course_id, commentable_id):
     elif commentable_id in course.top_level_discussion_topic_ids:
         # top level discussions have to be manually configured as cohorted
         # (default is not)
-        ans = commentable_id in course.cohorted_discussions()
+        ans = commentable_id in course.cohorted_discussions
     else:
         # inline discussions are cohorted by default
         ans = True
@@ -61,21 +65,10 @@ def is_commentable_cohorted(course_id, commentable_id):
 
 
 def get_cohort(user, course_id):
-    c = _get_cohort(user, course_id)
-    log.debug("get_cohort({0}, {1}) = {2}".format(
-        user, course_id,
-        c.id if c is not None else None))
-    return c
-
-
-def _get_cohort(user, course_id):
     """
     Given a django User and a course_id, return the user's cohort in that
     cohort.
 
-    TODO: In classes with auto-cohorting, put the user in a cohort if they
-    aren't in one already.
-
     Arguments:
         user: a Django User object.
         course_id: string in the format 'org/course/run'
@@ -88,7 +81,7 @@ def _get_cohort(user, course_id):
        ValueError if the course_id doesn't exist.
     """
     # First check whether the course is cohorted (users shouldn't be in a cohort
-    # in non-cohorted courses, but settings can change after )
+    # in non-cohorted courses, but settings can change after course starts)
     try:
         course = courses.get_course_by_id(course_id)
     except Http404:
@@ -98,15 +91,10 @@ def _get_cohort(user, course_id):
         return None
 
     try:
-        group = CourseUserGroup.objects.get(course_id=course_id,
+        return CourseUserGroup.objects.get(course_id=course_id,
                                             group_type=CourseUserGroup.COHORT,
                                             users__id=user.id)
     except CourseUserGroup.DoesNotExist:
-        group = None
-
-    if group:
-        return group
-
         # TODO: add auto-cohorting logic here once we know what that will be.
         return None
 
@@ -119,7 +107,8 @@ def get_course_cohorts(course_id):
         course_id: string in the format 'org/course/run'
 
     Returns:
-        A list of CourseUserGroup objects.  Empty if there are no cohorts.
+        A list of CourseUserGroup objects.  Empty if there are no cohorts. Does
+        not check whether the course is cohorted.
     """
     return list(CourseUserGroup.objects.filter(course_id=course_id,
                                                group_type=CourseUserGroup.COHORT))

common/djangoapps/course_groups/tests/tests.py

@@ -5,12 +5,68 @@ from django.conf import settings
 from override_settings import override_settings
 
 from course_groups.models import CourseUserGroup
-from course_groups.cohorts import get_cohort, get_course_cohorts
+from course_groups.cohorts import (get_cohort, get_course_cohorts,
+                                   is_commentable_cohorted)
 
-from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.django import modulestore, _MODULESTORES
 
 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):
+        """
+        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.
+
+        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.metadata["discussion_topics"] = topics
+
+        d = {"cohorted": cohorted}
+        if cohorted_discussions is not None:
+            d["cohorted_discussions"] = [to_id(name)
+                                         for name in cohorted_discussions]
+        course.metadata["cohort_config"] = d
+
+
+    def setUp(self):
+        """
+        Make sure that course is reloaded every time--clear out the modulestore.
+        """
+        # don't like this, but don't know a better way to undo all changes made
+        # to course.  We don't have a course.clone() method.
+        _MODULESTORES.clear()
+
+
     def test_get_cohort(self):
         # Need to fix this, but after we're testing on staging.  (Looks like
         # problem is that when get_cohort internally tries to look up the
@@ -20,6 +76,7 @@ class TestCohorts(django.test.TestCase):
         # dir.
         course = modulestore().get_course("edX/toy/2012_Fall")
         self.assertEqual(course.id, "edX/toy/2012_Fall")
+        self.assertFalse(course.is_cohorted)
         
         user = User.objects.create(username="test", email="a@b.com")
         other_user = User.objects.create(username="test2", email="a2@b.com")
@@ -36,7 +93,7 @@ class TestCohorts(django.test.TestCase):
                           "Course isn't cohorted, so shouldn't have a cohort")
 
         # Make the course cohorted...
-        course.metadata["cohort_config"] = {"cohorted": True}
+        self.config_course_cohorts(course, [], cohorted=True)
 
         self.assertEquals(get_cohort(user, course.id).id, cohort.id,
                           "Should find the right cohort")
@@ -65,3 +122,49 @@ class TestCohorts(django.test.TestCase):
         cohorts = sorted([c.name for c in get_course_cohorts(course1_id)])
         self.assertEqual(cohorts, ['TestCohort', 'TestCohort2'])
 
+
+    def test_is_commentable_cohorted(self):
+        course = modulestore().get_course("edX/toy/2012_Fall")
+        self.assertFalse(course.is_cohorted)
+
+        def to_id(name):
+            return self.topic_name_to_id(course, name)
+
+        # no topics
+        self.assertFalse(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)
+
+        self.assertFalse(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)
+
+        self.assertTrue(course.is_cohorted)
+        self.assertFalse(is_commentable_cohorted(course.id, to_id("General")),
+                         "Course is cohorted, but 'General' isn't.")
+
+        self.assertTrue(
+            is_commentable_cohorted(course.id, to_id("random")),
+            "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"])
+
+        self.assertTrue(course.is_cohorted)
+        self.assertFalse(is_commentable_cohorted(course.id, to_id("General")),
+                         "Course is cohorted, but 'General' isn't.")
+
+        self.assertTrue(
+            is_commentable_cohorted(course.id, to_id("Feedback")),
+            "Feedback was listed as cohorted.  Should be.")
+
+
+