diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index 446274543c..782ce16b47 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/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. diff --git a/common/djangoapps/course_groups/tests/helpers.py b/common/djangoapps/course_groups/tests/helpers.py new file mode 100644 index 0000000000..321f05a341 --- /dev/null +++ b/common/djangoapps/course_groups/tests/helpers.py @@ -0,0 +1,68 @@ +""" +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 diff --git a/common/djangoapps/course_groups/tests/test_cohorts.py b/common/djangoapps/course_groups/tests/test_cohorts.py index 42fe2dff28..032b10bbf2 100644 --- a/common/djangoapps/course_groups/tests/test_cohorts.py +++ b/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 diff --git a/common/djangoapps/course_groups/tests/test_views.py b/common/djangoapps/course_groups/tests/test_views.py index 6472b38bc1..9be53603c3 100644 --- a/common/djangoapps/course_groups/tests/test_views.py +++ b/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): """ diff --git a/common/djangoapps/course_groups/views.py b/common/djangoapps/course_groups/views.py index c1ec01d66a..5f670f1f83 100644 --- a/common/djangoapps/course_groups/views.py +++ b/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}) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 615654f0e2..ec8e090643 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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: