diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index 782ce16b47..4f2fbf5d31 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/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 + 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 - # Put user in a random group, creating it if needed - group_name = local_random().choice(choices) - - 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,15 +174,13 @@ 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( - course_id=course.location.course_key, - group_type=CourseUserGroup.COHORT, - name=group_name - ) + # 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.location.course_key, diff --git a/common/djangoapps/course_groups/tests/helpers.py b/common/djangoapps/course_groups/tests/helpers.py index 321f05a341..8afa541f11 100644 --- a/common/djangoapps/course_groups/tests/helpers.py +++ b/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, - cohorted, - cohorted_discussions=None, - auto_cohort=None, - auto_cohort_groups=None): +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_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 diff --git a/common/djangoapps/course_groups/tests/test_cohorts.py b/common/djangoapps/course_groups/tests/test_cohorts.py index 032b10bbf2..fd1a5b0dcd 100644 --- a/common/djangoapps/course_groups/tests/test_cohorts.py +++ b/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", - course_id=course.id, - group_type=CourseUserGroup.COHORT) + config_course_cohorts(course, discussions=[], cohorted=True) + cohort = CourseUserGroup.objects.create( + name="TestCohort", + course_id=course.id, + 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", - course_id=course.id, - group_type=CourseUserGroup.COHORT) + cohort = CourseUserGroup.objects.create( + name="TestCohort", + course_id=course.id, + 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", - course_id=course.id, - group_type=CourseUserGroup.COHORT) + cohort = CourseUserGroup.objects.create( + name="TestCohort", + course_id=course.id, + 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): """ diff --git a/common/djangoapps/course_groups/tests/test_views.py b/common/djangoapps/course_groups/tests/test_views.py index 9be53603c3..38f724d1f4 100644 --- a/common/djangoapps/course_groups/tests/test_views.py +++ b/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" )