diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index df8b22ef26..f84e18b214 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/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,17 +91,12 @@ 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 + # TODO: add auto-cohorting logic here once we know what that will be. + return None def get_course_cohorts(course_id): @@ -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)) diff --git a/common/djangoapps/course_groups/tests/tests.py b/common/djangoapps/course_groups/tests/tests.py index 0303eaaa55..21fad8bbeb 100644 --- a/common/djangoapps/course_groups/tests/tests.py +++ b/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,7 +76,8 @@ 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,8 +93,8 @@ 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.") + + +