From 4896b2075dc11bb415709d37570333015dabac35 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 13 Mar 2017 20:42:14 -0400 Subject: [PATCH 1/5] Introduce EnrollmentTrackUserPartition. TNL-6674 --- .../contentstore/tests/test_utils.py | 8 +- cms/djangoapps/contentstore/tests/utils.py | 10 +- .../views/tests/test_group_configurations.py | 46 +++--- .../contentstore/views/tests/test_item.py | 95 +++++++---- cms/envs/common.py | 3 + common/djangoapps/course_modes/admin.py | 42 ++++- .../migrations/0008_enrollmentmode.py | 46 ++++++ common/djangoapps/course_modes/models.py | 19 +++ .../course_modes/tests/test_models.py | 21 ++- common/lib/xmodule/xmodule/course_module.py | 50 ++++++ .../xmodule/modulestore/inheritance.py | 15 +- .../xmodule/xmodule/partitions/partitions.py | 45 ++++-- .../lib/xmodule/xmodule/split_test_module.py | 2 +- .../xmodule/tests/test_course_module.py | 60 +++++++ .../tests/test_field_override_performance.py | 54 +++---- .../course_api/blocks/tests/test_api.py | 4 +- .../courseware/tests/test_access.py | 6 +- lms/djangoapps/courseware/tests/test_views.py | 8 +- lms/djangoapps/courseware/testutils.py | 9 ++ lms/djangoapps/grades/tests/test_tasks.py | 20 +-- .../instructor/views/instructor_dashboard.py | 2 +- .../tests/test_tasks_helper.py | 2 +- .../lti_provider/tests/test_views.py | 18 +++ lms/envs/common.py | 4 +- lms/urls.py | 2 +- .../credit/tests/test_verification_access.py | 49 +++--- .../verified_track_content/__init__.py | 0 .../verified_track_content/admin.py | 4 +- .../verified_track_content/forms.py | 2 +- .../migrations/0001_initial.py | 0 ...rackcohortedcourse_verified_cohort_name.py | 0 .../migrations/__init__.py | 0 .../verified_track_content/models.py | 4 +- .../partition_scheme.py | 147 +++++++++++++++++ .../verified_track_content/tasks.py | 0 .../verified_track_content/tests/__init__.py | 0 .../tests/test_forms.py | 2 +- .../tests/test_models.py | 28 +++- .../tests/test_partition_scheme.py | 152 ++++++++++++++++++ .../tests/test_views.py | 10 +- .../verified_track_content/views.py | 4 +- setup.py | 1 + 42 files changed, 811 insertions(+), 183 deletions(-) create mode 100644 common/djangoapps/course_modes/migrations/0008_enrollmentmode.py rename {lms => openedx/core}/djangoapps/verified_track_content/__init__.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/admin.py (61%) rename {lms => openedx/core}/djangoapps/verified_track_content/forms.py (94%) rename {lms => openedx/core}/djangoapps/verified_track_content/migrations/0001_initial.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/migrations/0002_verifiedtrackcohortedcourse_verified_cohort_name.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/migrations/__init__.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/models.py (97%) create mode 100644 openedx/core/djangoapps/verified_track_content/partition_scheme.py rename {lms => openedx/core}/djangoapps/verified_track_content/tasks.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/tests/__init__.py (100%) rename {lms => openedx/core}/djangoapps/verified_track_content/tests/test_forms.py (94%) rename {lms => openedx/core}/djangoapps/verified_track_content/tests/test_models.py (92%) create mode 100644 openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py rename {lms => openedx/core}/djangoapps/verified_track_content/tests/test_views.py (89%) rename {lms => openedx/core}/djangoapps/verified_track_content/views.py (88%) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index c5e679ccb339..a70193474ff1 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -493,12 +493,12 @@ def test_retrieves_partition_info_with_selected_groups(self): ] } ] - self.assertEqual(self._get_partition_info(), expected) + self.assertEqual(self._get_partition_info(schemes=["cohort", "random"]), expected) # Update group access and expect that now one group is marked as selected. self._set_group_access({0: [1]}) expected[0]["groups"][1]["selected"] = True - self.assertEqual(self._get_partition_info(), expected) + self.assertEqual(self._get_partition_info(schemes=["cohort", "random"]), expected) def test_deleted_groups(self): # Select a group that is not defined in the partition @@ -546,7 +546,7 @@ def test_exclude_inactive_partitions(self): ]) # Expect that the inactive scheme is excluded from the results - partitions = self._get_partition_info() + partitions = self._get_partition_info(schemes=["cohort", "verification"]) self.assertEqual(len(partitions), 1) self.assertEqual(partitions[0]["scheme"], "cohort") @@ -572,7 +572,7 @@ def test_exclude_partitions_with_no_groups(self): ]) # Expect that the partition with no groups is excluded from the results - partitions = self._get_partition_info() + partitions = self._get_partition_info(schemes=["cohort", "verification"]) self.assertEqual(len(partitions), 1) self.assertEqual(partitions[0]["scheme"], "verification") diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index af5e7b8e526b..735610714e01 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -299,8 +299,14 @@ def assertCoursesEqual(self, course1_id, course2_id): store = self.store._get_modulestore_for_courselike(course2_id) # pylint: disable=protected-access new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_loc.run course2_item_loc = course2_item_loc.replace(name=new_name) - course2_item = self.store.get_item(course2_item_loc) - + course2_item = self.store.get_item(course2_item_loc) + # The EnrollmentTrackuserPartition stores the course_id, which will vary split vs. old mongo. + # Also at least one test calls this comparison method with a purposefully different course. + # Clear stored course_id so that own_metadata call below does not fail. + course1_item.user_partitions[0].parameters['course_id'] = None + course2_item.user_partitions[0].parameters['course_id'] = None + else: + course2_item = self.store.get_item(course2_item_loc) # compare published state self.assertEqual( self.store.has_published_version(course1_item), diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 5e04336d606d..f1ea51b2fa78 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -132,6 +132,10 @@ def _add_user_partitions(self, count=1, scheme_id="random"): self.course.user_partitions = partitions self.save_course() + def _get_non_enrollment_track_partitions(self): + """ Return the user_partitions that are not of type "enrollment_track". """ + return [p for p in self.course.user_partitions if p.scheme.name != "enrollment_track"] + # pylint: disable=no-member class GroupConfigurationsBaseTestCase(object): @@ -282,23 +286,23 @@ def test_can_create_group_configuration(self): self.assertEqual(len(group_ids), 2) self.reload_course() # Verify that user_partitions in the course contains the new group configuration. - user_partititons = self.course.user_partitions - self.assertEqual(len(user_partititons), 1) - self.assertEqual(user_partititons[0].name, u'Test name') - self.assertEqual(len(user_partititons[0].groups), 2) - self.assertEqual(user_partititons[0].groups[0].name, u'Group A') - self.assertEqual(user_partititons[0].groups[1].name, u'Group B') - self.assertEqual(user_partititons[0].parameters, {}) + user_partitions = self._get_non_enrollment_track_partitions() + self.assertEqual(len(user_partitions), 1) + self.assertEqual(user_partitions[0].name, u'Test name') + self.assertEqual(len(user_partitions[0].groups), 2) + self.assertEqual(user_partitions[0].groups[0].name, u'Group A') + self.assertEqual(user_partitions[0].groups[1].name, u'Group B') + self.assertEqual(user_partitions[0].parameters, {}) def test_lazily_creates_cohort_configuration(self): """ Test that a cohort schemed user partition is NOT created by default for the user. """ - self.assertEqual(len(self.course.user_partitions), 0) + self.assertEqual(len(self._get_non_enrollment_track_partitions()), 0) self.client.get(self._url()) self.reload_course() - self.assertEqual(len(self.course.user_partitions), 0) + self.assertEqual(len(self._get_non_enrollment_track_partitions()), 0) class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): @@ -348,7 +352,7 @@ def test_can_create_new_content_group_if_it_does_not_exist(self): self.assertEqual(content, expected) self.reload_course() # Verify that user_partitions in the course contains the new group configuration. - user_partitions = self.course.user_partitions + user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, u'Test name') self.assertEqual(len(user_partitions[0].groups), 2) @@ -389,7 +393,7 @@ def test_can_edit_content_group(self): self.reload_course() # Verify that user_partitions is properly updated in the course. - user_partititons = self.course.user_partitions + user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, u'New Test name') @@ -415,11 +419,11 @@ def test_can_delete_content_group(self): self.assertEqual(response.status_code, 204) self.reload_course() # Verify that group and partition is properly updated in the course. - user_partititons = self.course.user_partitions - self.assertEqual(len(user_partititons), 1) - self.assertEqual(user_partititons[0].name, 'Name 0') - self.assertEqual(len(user_partititons[0].groups), 2) - self.assertEqual(user_partititons[0].groups[1].name, 'Group C') + user_partitions = self._get_non_enrollment_track_partitions() + self.assertEqual(len(user_partitions), 1) + self.assertEqual(user_partitions[0].name, 'Name 0') + self.assertEqual(len(user_partitions[0].groups), 2) + self.assertEqual(user_partitions[0].groups[1].name, 'Group C') def test_cannot_delete_used_content_group(self): """ @@ -440,7 +444,7 @@ def test_cannot_delete_used_content_group(self): self.assertTrue(content['error']) self.reload_course() # Verify that user_partitions and groups are still the same. - user_partititons = self.course.user_partitions + user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 1) self.assertEqual(len(user_partititons[0].groups), 3) self.assertEqual(user_partititons[0].groups[1].name, 'Group B') @@ -493,7 +497,7 @@ def test_can_create_new_group_configuration_if_it_does_not_exist(self): self.assertEqual(content, expected) self.reload_course() # Verify that user_partitions in the course contains the new group configuration. - user_partitions = self.course.user_partitions + user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, u'Test name') self.assertEqual(len(user_partitions[0].groups), 2) @@ -535,7 +539,7 @@ def test_can_edit_group_configuration(self): self.reload_course() # Verify that user_partitions is properly updated in the course. - user_partititons = self.course.user_partitions + user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, u'New Test name') @@ -560,7 +564,7 @@ def test_can_delete_group_configuration(self): self.assertEqual(response.status_code, 204) self.reload_course() # Verify that user_partitions is properly updated in the course. - user_partititons = self.course.user_partitions + user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, 'Name 1') @@ -582,7 +586,7 @@ def test_cannot_delete_used_group_configuration(self): self.assertTrue(content['error']) self.reload_course() # Verify that user_partitions is still the same. - user_partititons = self.course.user_partitions + user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 2) self.assertEqual(user_partititons[0].name, 'Name 0') diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 3e28f0e20595..d114f483cd8c 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -343,13 +343,13 @@ def test_invalid_paging(self, page_number, page_size): def test_get_user_partitions_and_groups(self): self.course.user_partitions = [ UserPartition( - id=0, + id=100, name="Verification user partition", scheme=UserPartition.get_scheme("verification"), description="Verification user partition", groups=[ - Group(id=0, name="Group A"), - Group(id=1, name="Group B"), + Group(id=101, name="Group A"), + Group(id=102, name="Group B"), ], ), ] @@ -366,17 +366,30 @@ def test_get_user_partitions_and_groups(self): self.assertEqual(result["user_partitions"], [ { "id": 0, + "name": "Enrollment Track Partition", + "scheme": "enrollment_track", + "groups": [ + { + "id": 1, + "name": "Audit", + "selected": False, + "deleted": False, + } + ] + }, + { + "id": 100, "name": "Verification user partition", "scheme": "verification", "groups": [ { - "id": 0, + "id": 101, "name": "Group A", "selected": False, "deleted": False, }, { - "id": 1, + "id": 102, "name": "Group B", "selected": False, "deleted": False, @@ -1704,15 +1717,30 @@ class TestEditSplitModule(ItemTest): def setUp(self): super(TestEditSplitModule, self).setUp() self.user = UserFactory() + + self.first_user_partition_group_1 = Group("101", 'alpha') + self.first_user_partition_group_2 = Group("102", 'beta') + self.first_user_partition = UserPartition( + 100, 'first_partition', 'First Partition', + [self.first_user_partition_group_1, self.first_user_partition_group_2] + ) + + # There is a test point below (test_create_groups) that purposefully wants the group IDs + # of the 2 partitions to overlap (which is not something that normally happens). + self.second_user_partition_group_1 = Group("101", 'Group 1') + self.second_user_partition_group_2 = Group("102", 'Group 2') + self.second_user_partition_group_3 = Group("103", 'Group 3') + self.second_user_partition = UserPartition( + 101, 'second_partition', 'Second Partition', + [ + self.second_user_partition_group_1, + self.second_user_partition_group_2, + self.second_user_partition_group_3 + ] + ) self.course.user_partitions = [ - UserPartition( - 0, 'first_partition', 'First Partition', - [Group("0", 'alpha'), Group("1", 'beta')] - ), - UserPartition( - 1, 'second_partition', 'Second Partition', - [Group("0", 'Group 0'), Group("1", 'Group 1'), Group("2", 'Group 2')] - ) + self.first_user_partition, + self.second_user_partition ] self.store.update_item(self.course, self.user.id) root_usage_key = self._create_vertical() @@ -1760,8 +1788,8 @@ def test_create_groups(self): self.assertEqual(-1, split_test.user_partition_id) self.assertEqual(0, len(split_test.children)) - # Set the user_partition_id to 0. - split_test = self._update_partition_id(0) + # Set the user_partition_id to match the first user_partition. + split_test = self._update_partition_id(self.first_user_partition.id) # Verify that child verticals have been set to match the groups self.assertEqual(2, len(split_test.children)) @@ -1769,13 +1797,13 @@ def test_create_groups(self): vertical_1 = self.get_item_from_modulestore(split_test.children[1], verify_is_draft=True) self.assertEqual("vertical", vertical_0.category) self.assertEqual("vertical", vertical_1.category) - self.assertEqual("Group ID 0", vertical_0.display_name) - self.assertEqual("Group ID 1", vertical_1.display_name) + self.assertEqual("Group ID 101", vertical_0.display_name) + self.assertEqual("Group ID 102", vertical_1.display_name) # Verify that the group_id_to_child mapping is correct. self.assertEqual(2, len(split_test.group_id_to_child)) - self.assertEqual(vertical_0.location, split_test.group_id_to_child['0']) - self.assertEqual(vertical_1.location, split_test.group_id_to_child['1']) + self.assertEqual(vertical_0.location, split_test.group_id_to_child[str(self.first_user_partition_group_1.id)]) + self.assertEqual(vertical_1.location, split_test.group_id_to_child[str(self.first_user_partition_group_2.id)]) def test_split_xblock_info_group_name(self): """ @@ -1785,8 +1813,8 @@ def test_split_xblock_info_group_name(self): # Initially, no user_partition_id is set, and the split_test has no children. self.assertEqual(split_test.user_partition_id, -1) self.assertEqual(len(split_test.children), 0) - # Set the user_partition_id to 0. - split_test = self._update_partition_id(0) + # Set the user_partition_id to match the first user_partition. + split_test = self._update_partition_id(self.first_user_partition.id) # Verify that child verticals have been set to match the groups self.assertEqual(len(split_test.children), 2) @@ -1808,13 +1836,13 @@ def test_change_user_partition_id(self): group configuration. """ # Set to first group configuration. - split_test = self._update_partition_id(0) + split_test = self._update_partition_id(self.first_user_partition.id) self.assertEqual(2, len(split_test.children)) initial_vertical_0_location = split_test.children[0] initial_vertical_1_location = split_test.children[1] # Set to second group configuration - split_test = self._update_partition_id(1) + split_test = self._update_partition_id(self.second_user_partition.id) # We don't remove existing children. self.assertEqual(5, len(split_test.children)) self.assertEqual(initial_vertical_0_location, split_test.children[0]) @@ -1825,9 +1853,9 @@ def test_change_user_partition_id(self): # Verify that the group_id_to child mapping is correct. self.assertEqual(3, len(split_test.group_id_to_child)) - self.assertEqual(vertical_0.location, split_test.group_id_to_child['0']) - self.assertEqual(vertical_1.location, split_test.group_id_to_child['1']) - self.assertEqual(vertical_2.location, split_test.group_id_to_child['2']) + self.assertEqual(vertical_0.location, split_test.group_id_to_child[str(self.second_user_partition_group_1.id)]) + self.assertEqual(vertical_1.location, split_test.group_id_to_child[str(self.second_user_partition_group_2.id)]) + self.assertEqual(vertical_2.location, split_test.group_id_to_child[str(self.second_user_partition_group_3.id)]) self.assertNotEqual(initial_vertical_0_location, vertical_0.location) self.assertNotEqual(initial_vertical_1_location, vertical_1.location) @@ -1836,12 +1864,12 @@ def test_change_same_user_partition_id(self): Test that nothing happens when the user_partition_id is set to the same value twice. """ # Set to first group configuration. - split_test = self._update_partition_id(0) + split_test = self._update_partition_id(self.first_user_partition.id) self.assertEqual(2, len(split_test.children)) initial_group_id_to_child = split_test.group_id_to_child # Set again to first group configuration. - split_test = self._update_partition_id(0) + split_test = self._update_partition_id(self.first_user_partition.id) self.assertEqual(2, len(split_test.children)) self.assertEqual(initial_group_id_to_child, split_test.group_id_to_child) @@ -1852,7 +1880,7 @@ def test_change_non_existent_user_partition_id(self): The user_partition_id will be updated, but children and group_id_to_child map will not change. """ # Set to first group configuration. - split_test = self._update_partition_id(0) + split_test = self._update_partition_id(self.first_user_partition.id) self.assertEqual(2, len(split_test.children)) initial_group_id_to_child = split_test.group_id_to_child @@ -1869,13 +1897,14 @@ def test_add_groups(self): TODO: move tests that can go over to common after the mixed modulestore work is done. # pylint: disable=fixme """ # Set to first group configuration. - split_test = self._update_partition_id(0) + split_test = self._update_partition_id(self.first_user_partition.id) # Add a group to the first group configuration. + new_group_id = "1002" split_test.user_partitions = [ UserPartition( - 0, 'first_partition', 'First Partition', - [Group("0", 'alpha'), Group("1", 'beta'), Group("2", 'pie')] + self.first_user_partition.id, 'first_partition', 'First Partition', + [self.first_user_partition_group_1, self.first_user_partition_group_2, Group(new_group_id, 'pie')] ) ] self.store.update_item(split_test, self.user.id) @@ -1896,7 +1925,7 @@ def test_add_groups(self): split_test = self._assert_children(3) self.assertNotEqual(group_id_to_child, split_test.group_id_to_child) group_id_to_child = split_test.group_id_to_child - self.assertEqual(split_test.children[2], group_id_to_child["2"]) + self.assertEqual(split_test.children[2], group_id_to_child[new_group_id]) # Call add_missing_groups again -- it should be a no-op. split_test.add_missing_groups(self.request) diff --git a/cms/envs/common.py b/cms/envs/common.py index 40a46e6ab787..8de1a5127e60 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -882,6 +882,9 @@ # for managing course modes 'course_modes', + # Verified Track Content Cohorting (Beta feature that will hopefully be removed) + 'openedx.core.djangoapps.verified_track_content', + # Dark-launching languages 'openedx.core.djangoapps.dark_lang', diff --git a/common/djangoapps/course_modes/admin.py b/common/djangoapps/course_modes/admin.py index a500d5cdea17..46f3d816aaad 100644 --- a/common/djangoapps/course_modes/admin.py +++ b/common/djangoapps/course_modes/admin.py @@ -14,7 +14,7 @@ from util.date_utils import get_time_display from xmodule.modulestore.django import modulestore -from course_modes.models import CourseMode, CourseModeExpirationConfig +from course_modes.models import CourseMode, CourseModeExpirationConfig, EnrollmentMode # Technically, we shouldn't be doing this, since verify_student is defined # in LMS, and course_modes is defined in common. @@ -29,12 +29,15 @@ from lms.djangoapps.verify_student import models as verification_models -class CourseModeForm(forms.ModelForm): - - class Meta(object): - model = CourseMode - fields = '__all__' +COURSE_MODE_SLUG_CHOICES = [] +try: + COURSE_MODE_SLUG_CHOICES = [(mode.mode_slug, mode.mode_slug) for mode in EnrollmentMode.objects.all()] +except: # pylint: disable=bare-except + pass +if not COURSE_MODE_SLUG_CHOICES: + # The entries in this table are populated in a migration file, and migration files do not + # run as part of unit tests. Therefore, for tests hard-code the choices. COURSE_MODE_SLUG_CHOICES = ( [(CourseMode.DEFAULT_MODE_SLUG, CourseMode.DEFAULT_MODE_SLUG)] + [(mode_slug, mode_slug) for mode_slug in CourseMode.VERIFIED_MODES] + @@ -44,6 +47,14 @@ class Meta(object): [(CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG, CourseMode.DEFAULT_SHOPPINGCART_MODE_SLUG)] ) + +class CourseModeForm(forms.ModelForm): + """ CourseModeForm """ + + class Meta(object): + model = CourseMode + fields = '__all__' + mode_slug = forms.ChoiceField(choices=COURSE_MODE_SLUG_CHOICES, label=_("Mode")) # The verification deadline is stored outside the course mode in the verify_student app. @@ -216,5 +227,24 @@ class CourseModeExpirationConfigAdmin(admin.ModelAdmin): class Meta(object): model = CourseModeExpirationConfig + +class EnrollmentModeAdmin(admin.ModelAdmin): + """Read-only interface for the enrollment mode configuration. """ + + readonly_fields = ('mode_slug',) + + class Meta(object): + model = EnrollmentMode + + def has_add_permission(self, request): + """ Don't allow adding via the admin table at this time-- this should be done in a migration file. """ + return False + + def has_delete_permission(self, request, obj=None): + """ Don't allow deleting via the admin table, as the mode slugs must match expected values in code. """ + return False + + admin.site.register(CourseMode, CourseModeAdmin) admin.site.register(CourseModeExpirationConfig, CourseModeExpirationConfigAdmin) +admin.site.register(EnrollmentMode, EnrollmentModeAdmin) diff --git a/common/djangoapps/course_modes/migrations/0008_enrollmentmode.py b/common/djangoapps/course_modes/migrations/0008_enrollmentmode.py new file mode 100644 index 000000000000..49d0387d73dd --- /dev/null +++ b/common/djangoapps/course_modes/migrations/0008_enrollmentmode.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +def create_modes(apps, schema_editor): + """ + Populate the enrollment modes table with our current modes. + """ + + enrollment_mode_model = apps.get_model("course_modes", "EnrollmentMode") + + objects = enrollment_mode_model.objects + if not objects.exists(): + for mode in ['audit', 'verified', 'professional', 'no-id-professional', 'credit', 'honor']: + objects.create(mode_slug=mode) + + +def delete_modes(apps, schema_editor): + """ + Delete the enrollment modes table data. + """ + enrollment_mode_model = apps.get_model("course_modes", "EnrollmentMode") + enrollment_mode_model.objects.all().delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_modes', '0007_coursemode_bulk_sku'), + ] + + operations = [ + migrations.CreateModel( + name='EnrollmentMode', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('mode_slug', models.CharField(unique=True, max_length=100)), + ], + ), + migrations.RunPython( + code=create_modes, + reverse_code=delete_modes + ) + ] diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 51999ff3bf31..107fbcfbfea0 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -26,6 +26,25 @@ ]) +class EnrollmentMode(models.Model): + """ + A model for storing attributes of an enrollment mode. Currently used mostly for + the EnrollmentTrackUserPartition, but this could be extended to add properties of enrollment + modes (for example, that "verified" and "professional" issue certificates, but "audit" does not. + """ + class Meta(object): + app_label = "course_modes" + + # TODO: should we try to tie "mode_slug" in CourseMode and this together? + mode_slug = models.CharField(max_length=100, unique=True) + + def __unicode__(self): + """ Return a string representation """ + return u"EnrollmentMode {} (id={})".format( + self.mode_slug, self.id + ) + + class CourseMode(models.Model): """ We would like to offer a course in a variety of modes. diff --git a/common/djangoapps/course_modes/tests/test_models.py b/common/djangoapps/course_modes/tests/test_models.py index f992e54ebca8..c86a974e72fb 100644 --- a/common/djangoapps/course_modes/tests/test_models.py +++ b/common/djangoapps/course_modes/tests/test_models.py @@ -16,7 +16,7 @@ import pytz from course_modes.helpers import enrollment_mode_display -from course_modes.models import CourseMode, Mode +from course_modes.models import CourseMode, Mode, EnrollmentMode from course_modes.tests.factories import CourseModeFactory @@ -471,3 +471,22 @@ def test_verified_min_price(self, mode_slug, is_error_expected): self.assertTrue(is_error_expected, "Did not expect a ValidationError to be thrown.") else: self.assertFalse(is_error_expected, "Expected a ValidationError to be thrown.") + + +class EnrollmentModeTest(TestCase): + """ + Tests for EnrollmentMode Model. + """ + def tearDown(self): + super(EnrollmentModeTest, self).tearDown() + EnrollmentMode.objects.all().delete() + + def test_default_mode_ids(self): + """ + Test that the generated mode IDs are small integers (we never expect this table to grow large). + """ + EnrollmentMode.objects.get_or_create(mode_slug="audit") + EnrollmentMode.objects.get_or_create(mode_slug="verified") + modes = EnrollmentMode.objects.all() + for mode in modes: + self.assertLess(mode.id, 100, "Mode IDs must be less than 100") diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 9c39c7b54951..a0b8fe9692fa 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -17,6 +17,7 @@ from xmodule.course_metadata_utils import DEFAULT_START_DATE from xmodule.graders import grader_from_conf from xmodule.mixin import LicenseMixin +from xmodule.partitions.partitions import UserPartition, UserPartitionError, UserPartitionList from xmodule.seq_module import SequenceDescriptor, SequenceModule from xmodule.tabs import CourseTabList, InvalidTabsException from .fields import Date @@ -824,6 +825,13 @@ class CourseFields(object): scope=Scope.settings ) + user_partitions = UserPartitionList( + display_name=_("Group Configurations"), + help=_("Enter the configurations that govern how students are grouped together."), + default=[], + scope=Scope.settings + ) + """ instructor_info dict structure: { @@ -909,6 +917,48 @@ def __init__(self, *args, **kwargs): if self.discussion_topics == {}: self.discussion_topics = {_('General'): {'id': self.location.html_id()}} + try: + enrollment_track_scheme = UserPartition.get_scheme("enrollment_track") + enrollment_track_partitions = self.get_user_partitions_for_scheme(enrollment_track_scheme) + # Note that when a course_module is initially created, the ID is the draft version. + # We just want to store the course_id without published or draft. + course_id = unicode(self.id.for_branch(None)) + create_partition = False + new_id = 0 + + if len(enrollment_track_partitions) == 0: + used_ids = set(p.id for p in self.user_partitions) + while new_id in used_ids: + new_id += 1 + create_partition = True + elif enrollment_track_partitions[0].parameters["course_id"] != course_id: + # With older courses, the first time through this code we may end up storing + # the "old mongo" version of the course key. If so, update it to the new version. + self.user_partitions.remove(enrollment_track_partitions[0]) + new_id = enrollment_track_partitions[0].id + create_partition = True + + if create_partition: + partition = enrollment_track_scheme.create_user_partition( + id=new_id, + name=_(u"Enrollment Track Partition"), + description=_(u"Partition for segmenting users by enrollment track"), + parameters={"course_id": course_id} + ) + self.user_partitions.append(partition) + + if len(enrollment_track_partitions) > 1: + log.warning( + "Multiple EnrollmentTrackUserPartitions found for course {course_id}".format( + course_id=unicode(self.id) + ) + ) + + except UserPartitionError: + log.warning( + "No 'enrollment_track' scheme registered, EnrollmentTrackUserPartition will not be created." + ) + try: if not getattr(self, "tabs", []): CourseTabList.initialize_default(self) diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 2eb0e54f48a7..6bacf11bee0b 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -5,8 +5,8 @@ from django.conf import settings -from xmodule.partitions.partitions import UserPartition -from xblock.fields import Scope, Boolean, String, Float, XBlockMixin, Dict, Integer, List +from xmodule.partitions.partitions import UserPartitionList +from xblock.fields import Scope, Boolean, String, Float, XBlockMixin, Dict, Integer from xblock.runtime import KeyValueStore, KvsFieldData from xmodule.fields import Date, Timedelta from ..course_metadata_utils import DEFAULT_START_DATE @@ -17,16 +17,6 @@ _ = lambda text: text -class UserPartitionList(List): - """Special List class for listing UserPartitions""" - def from_json(self, values): - return [UserPartition.from_json(v) for v in values] - - def to_json(self, values): - return [user_partition.to_json() - for user_partition in values] - - class InheritanceMixin(XBlockMixin): """Field definitions for inheritable fields.""" @@ -147,6 +137,7 @@ class InheritanceMixin(XBlockMixin): ) # This is should be scoped to content, but since it's defined in the policy # file, it is currently scoped to settings. + # user_partitions should be removed from this file-- See TNL-6738. user_partitions = UserPartitionList( display_name=_("Group Configurations"), help=_("Enter the configurations that govern how students are grouped together."), diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index 559dcf81def5..81f3cb9e387a 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -2,12 +2,23 @@ from collections import namedtuple from stevedore.extension import ExtensionManager +from xblock.fields import List # We use ``id`` in this file as the IDs of our Groups and UserPartitions, # which Pylint disapproves of. # pylint: disable=redefined-builtin +class UserPartitionList(List): + """Special List class for listing UserPartitions""" + def from_json(self, values): + return [UserPartition.from_json(v) for v in values] + + def to_json(self, values): + return [user_partition.to_json() + for user_partition in values] + + class UserPartitionError(Exception): """ Base Exception for when an error was found regarding user partitions. @@ -127,7 +138,7 @@ def get_scheme(name): try: scheme = UserPartition.scheme_extensions[name].plugin except KeyError: - raise UserPartitionError("Unrecognized scheme {0}".format(name)) + raise UserPartitionError("Unrecognized scheme '{0}'".format(name)) scheme.name = name return scheme @@ -188,15 +199,25 @@ def from_json(value): if not scheme: raise TypeError("UserPartition dict {0} has unrecognized scheme {1}".format(value, scheme_id)) - return UserPartition( - value["id"], - value["name"], - value["description"], - groups, - scheme, - parameters, - active, - ) + if hasattr(scheme, "create_user_partition"): + return scheme.create_user_partition( + value["id"], + value["name"], + value["description"], + groups, + parameters, + active, + ) + else: + return UserPartition( + value["id"], + value["name"], + value["description"], + groups, + scheme, + parameters, + active, + ) def get_group(self, group_id): """ @@ -214,5 +235,7 @@ def get_group(self, group_id): return group raise NoSuchUserPartitionGroupError( - "could not find a Group with ID [{}] in UserPartition [{}]".format(group_id, self.id) + "could not find a Group with ID [{group_id}] in UserPartition [{partition_id}]".format( + group_id=group_id, partition_id=self.id + ) ) diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index 144460590e56..ebd9c7897174 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -13,7 +13,7 @@ from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor from xmodule.x_module import XModule, module_attr, STUDENT_VIEW from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.modulestore.inheritance import UserPartitionList +from xmodule.partitions.partitions import UserPartitionList from lxml import etree diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 35c93220170c..9482468eb834 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -7,10 +7,14 @@ from fs.memoryfs import MemoryFS from mock import Mock, patch from pytz import utc +from stevedore.extension import Extension, ExtensionManager from xblock.runtime import KvsFieldData, DictKeyValueStore import xmodule.course_module from xmodule.modulestore.xml import ImportSystem, XMLModuleStore +from xmodule.partitions.partitions import ( + UserPartition, USER_PARTITION_SCHEME_NAMESPACE +) from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -253,6 +257,62 @@ def test_default_discussion_topics(self): self.assertEqual({'General': {'id': 'i4x-test_org-test_course-course-test'}}, d.discussion_topics) +class MockEnrollmentTrackPartitionScheme(object): + """ + Mock user partition scheme + """ + name = "enrollment_track" + + def get_group_for_user(self, course_id, user, user_partition, assign=True, track_function=None): # pylint: disable=unused-argument + """ + Returns None for all users. + """ + return None + + @classmethod + def create_user_partition(cls, id, name, description, groups=None, parameters=None, active=True): # pylint: disable=redefined-builtin, invalid-name, unused-argument + """ + Create a UserPartition, as the real EnrollmentTrackPartitionScheme supports this method + to create a custom UserPartition. + """ + return UserPartition(id, name, description, [], cls, parameters, active) + + +class EnrollmentTrackUserPartitionTestCase(unittest.TestCase): + """ + Tests that the "enrollment_track" UserPartition is created on new courses. + """ + + def setUp(self): + super(EnrollmentTrackUserPartitionTestCase, self).setUp() + # Set on a mock "enrollment track" partition scheme. + enrollment_track_scheme = MockEnrollmentTrackPartitionScheme + + extensions = [ + Extension( + enrollment_track_scheme.name, USER_PARTITION_SCHEME_NAMESPACE, enrollment_track_scheme, None + ) + ] + UserPartition.scheme_extensions = ExtensionManager.make_test_instance( + extensions, namespace=USER_PARTITION_SCHEME_NAMESPACE + ) + + # Be sure to clean up the global scheme_extensions after the test. + self.addCleanup(self.cleanup_scheme_extensions) + + def cleanup_scheme_extensions(self): + """ + Unset the UserPartition.scheme_extensions cache. + """ + UserPartition.scheme_extensions = None + + def test_default_user_partitions(self): + course = get_dummy_course('2012-12-02T12:00') + self.assertEqual(len(course.user_partitions), 1) + user_partition = course.user_partitions[0] + self.assertEqual(user_partition.scheme.name, "enrollment_track") + + class TeamsConfigurationTestCase(unittest.TestCase): """ Tests for the configuration of teams and the helper methods for accessing them. diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index aedcab4313ce..b3ed594c6302 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -231,18 +231,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (21, 6), - ('no_overrides', 2, True, False): (21, 6), - ('no_overrides', 3, True, False): (21, 6), - ('ccx', 1, True, False): (21, 6), - ('ccx', 2, True, False): (21, 6), - ('ccx', 3, True, False): (21, 6), - ('no_overrides', 1, False, False): (21, 6), - ('no_overrides', 2, False, False): (21, 6), - ('no_overrides', 3, False, False): (21, 6), - ('ccx', 1, False, False): (21, 6), - ('ccx', 2, False, False): (21, 6), - ('ccx', 3, False, False): (21, 6), + ('no_overrides', 1, True, False): (23, 6), + ('no_overrides', 2, True, False): (23, 6), + ('no_overrides', 3, True, False): (23, 6), + ('ccx', 1, True, False): (23, 6), + ('ccx', 2, True, False): (23, 6), + ('ccx', 3, True, False): (23, 6), + ('no_overrides', 1, False, False): (23, 6), + ('no_overrides', 2, False, False): (23, 6), + ('no_overrides', 3, False, False): (23, 6), + ('ccx', 1, False, False): (23, 6), + ('ccx', 2, False, False): (23, 6), + ('ccx', 3, False, False): (23, 6), } @@ -254,19 +254,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (21, 3), - ('no_overrides', 2, True, False): (21, 3), - ('no_overrides', 3, True, False): (21, 3), - ('ccx', 1, True, False): (21, 3), - ('ccx', 2, True, False): (21, 3), - ('ccx', 3, True, False): (21, 3), - ('ccx', 1, True, True): (22, 3), - ('ccx', 2, True, True): (22, 3), - ('ccx', 3, True, True): (22, 3), - ('no_overrides', 1, False, False): (21, 3), - ('no_overrides', 2, False, False): (21, 3), - ('no_overrides', 3, False, False): (21, 3), - ('ccx', 1, False, False): (21, 3), - ('ccx', 2, False, False): (21, 3), - ('ccx', 3, False, False): (21, 3), + ('no_overrides', 1, True, False): (23, 3), + ('no_overrides', 2, True, False): (23, 3), + ('no_overrides', 3, True, False): (23, 3), + ('ccx', 1, True, False): (23, 3), + ('ccx', 2, True, False): (23, 3), + ('ccx', 3, True, False): (23, 3), + ('ccx', 1, True, True): (24, 3), + ('ccx', 2, True, True): (24, 3), + ('ccx', 3, True, True): (24, 3), + ('no_overrides', 1, False, False): (23, 3), + ('no_overrides', 2, False, False): (23, 3), + ('no_overrides', 3, False, False): (23, 3), + ('ccx', 1, False, False): (23, 3), + ('ccx', 2, False, False): (23, 3), + ('ccx', 3, False, False): (23, 3), } diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index b73250c7cb14..aef500a5786f 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -147,7 +147,7 @@ def test_query_counts_cached(self, store_type, with_storage_backing): self._get_blocks( course, expected_mongo_queries=0, - expected_sql_queries=4 if with_storage_backing else 3, + expected_sql_queries=6 if with_storage_backing else 5, ) @ddt.data( @@ -165,5 +165,5 @@ def test_query_counts_uncached(self, store_type_tuple, with_storage_backing): self._get_blocks( course, expected_mongo_queries, - expected_sql_queries=11 if with_storage_backing else 3, + expected_sql_queries=13 if with_storage_backing else 5, ) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index c0e9b33b4134..af80bfc95f3f 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -301,9 +301,9 @@ def test_has_access_in_preview_mode_with_group(self): """ Test that a user masquerading as a member of a group sees appropriate content in preview mode. """ - partition_id = 0 - group_0_id = 0 - group_1_id = 1 + partition_id = 100 + group_0_id = 101 + group_1_id = 102 user_partition = UserPartition( partition_id, 'Test User Partition', '', [Group(group_0_id, 'Group 1'), Group(group_1_id, 'Group 2')], diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 5bfd709857fd..b78b2a964e62 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -206,7 +206,7 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 9), + (ModuleStoreEnum.Type.mongo, 10), (ModuleStoreEnum.Type.split, 4), ) @ddt.unpack @@ -1419,17 +1419,17 @@ def test_progress_queries_paced_courses(self, self_paced, self_paced_enabled): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(38), check_mongo_calls(4): + with self.assertNumQueries(40), check_mongo_calls(4): self._get_progress_page() def test_progress_queries(self): self.setup_course() - with self.assertNumQueries(38), check_mongo_calls(4): + with self.assertNumQueries(40), check_mongo_calls(4): self._get_progress_page() # subsequent accesses to the progress page require fewer queries. for _ in range(2): - with self.assertNumQueries(24), check_mongo_calls(4): + with self.assertNumQueries(26), check_mongo_calls(4): self._get_progress_page() @patch( diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 1ef13ad45e8c..c09da8543acc 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -178,6 +178,8 @@ def test_courseware_html(self, block_name, default_store, mongo_calls): @ddt.unpack def test_success_enrolled_staff(self, default_store, mongo_calls): with self.store.default_store(default_store): + if default_store is ModuleStoreEnum.Type.mongo: + mongo_calls = self.get_success_enrolled_staff_mongo_count() self.setup_course(default_store) self.setup_user(admin=True, enroll=True, login=True) @@ -197,6 +199,13 @@ def test_success_enrolled_staff(self, default_store, mongo_calls): with check_mongo_calls(mongo_calls): self.verify_response() + def get_success_enrolled_staff_mongo_count(self): + """ + Helper method used by test_success_enrolled_staff because one test + class using this mixin has an increased number of mongo (only) queries. + """ + return 5 + def test_success_unenrolled_staff(self): self.setup_course() self.setup_user(admin=True, enroll=False, login=True) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 33fdd277080d..d7ee2bb9f50b 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -144,10 +144,10 @@ def test_block_structure_created_only_once(self): self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 24, True), - (ModuleStoreEnum.Type.mongo, 1, 21, False), - (ModuleStoreEnum.Type.split, 3, 23, True), - (ModuleStoreEnum.Type.split, 3, 20, False), + (ModuleStoreEnum.Type.mongo, 1, 26, True), + (ModuleStoreEnum.Type.mongo, 1, 23, False), + (ModuleStoreEnum.Type.split, 3, 25, True), + (ModuleStoreEnum.Type.split, 3, 22, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -159,8 +159,8 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 24), - (ModuleStoreEnum.Type.split, 3, 23), + (ModuleStoreEnum.Type.mongo, 1, 26), + (ModuleStoreEnum.Type.split, 3, 25), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -205,8 +205,8 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal): ) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 9), - (ModuleStoreEnum.Type.split, 3, 8), + (ModuleStoreEnum.Type.mongo, 1, 11), + (ModuleStoreEnum.Type.split, 3, 10), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -220,8 +220,8 @@ def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_ self.assertEqual(len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)), 0) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 22), - (ModuleStoreEnum.Type.split, 3, 21), + (ModuleStoreEnum.Type.mongo, 1, 24), + (ModuleStoreEnum.Type.split, 3, 23), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index fdb241f5c1ee..1428c22a9d9e 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -53,6 +53,7 @@ from .tools import get_units_with_due_date, title_or_url from opaque_keys.edx.locations import SlashSeparatedCourseKey from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse from openedx.core.djangolib.markup import HTML, Text @@ -635,7 +636,6 @@ def _section_send_email(course, access): if is_course_cohorted(course_key): cohorts = get_course_cohorts(course) course_modes = [] - from verified_track_content.models import VerifiedTrackCohortedCourse if not VerifiedTrackCohortedCourse.is_verified_track_cohort_enabled(course_key): course_modes = CourseMode.modes_for_course(course_key, include_expired=True, only_selectable=False) email_editor = fragment.content diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 24d517d7e60b..999edb84762c 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1775,7 +1775,7 @@ def test_certificate_generation_for_students(self): 'failed': 3, 'skipped': 2 } - with self.assertNumQueries(168): + with self.assertNumQueries(184): self.assertCertificatesGenerated(task_input, expected_results) expected_results = { diff --git a/lms/djangoapps/lti_provider/tests/test_views.py b/lms/djangoapps/lti_provider/tests/test_views.py index ed7f539585b9..669e5cb801f8 100644 --- a/lms/djangoapps/lti_provider/tests/test_views.py +++ b/lms/djangoapps/lti_provider/tests/test_views.py @@ -212,3 +212,21 @@ def test_unauthenticated(self): self.setup_course() self.setup_user(admin=False, enroll=True, login=False) self.verify_response() + + def get_success_enrolled_staff_mongo_count(self): + """ + Override because mongo queries are higher for this + particular test. This has not been investigated exhaustively + as mongo is no longer used much, and removing user_partitions + from inheritance fixes the problem (see below). + + # The 9 mongoDB calls include calls for + # Old Mongo: + # (1) fill_in_run + # (2) get_course in get_course_with_access + # (3) get_item for HTML block in get_module_by_usage_id + # (4) get_parent when loading HTML block + # (5)-(8) calls related to the inherited user_partitions field. To be removed by TNL-6738. + # (9) edx_notes descriptor call to get_course + """ + return 9 diff --git a/lms/envs/common.py b/lms/envs/common.py index 6712bef1f8af..3280d780a369 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2146,8 +2146,8 @@ # API access administration 'openedx.core.djangoapps.api_admin', - # Verified Track Content Cohorting - 'verified_track_content', + # Verified Track Content Cohorting (Beta feature that will hopefully be removed) + 'openedx.core.djangoapps.verified_track_content', # Learner's dashboard 'learner_dashboard', diff --git a/lms/urls.py b/lms/urls.py index e5b6736bdd58..ec68eb3a5fe4 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -553,7 +553,7 @@ r'^courses/{}/verified_track_content/settings'.format( settings.COURSE_KEY_PATTERN, ), - 'verified_track_content.views.cohorting_settings', + 'openedx.core.djangoapps.verified_track_content.views.cohorting_settings', name='verified_track_cohorting', ), url( diff --git a/openedx/core/djangoapps/credit/tests/test_verification_access.py b/openedx/core/djangoapps/credit/tests/test_verification_access.py index 763d2fed2d8a..ff7fc30b5c64 100644 --- a/openedx/core/djangoapps/credit/tests/test_verification_access.py +++ b/openedx/core/djangoapps/credit/tests/test_verification_access.py @@ -80,8 +80,9 @@ def test_creates_user_partitions(self): self._update_partitions() # Check that a new user partition was created for the ICRV block - self.assertEqual(len(self.course.user_partitions), 1) - partition = self.course.user_partitions[0] + partitions = _get_non_enrollment_track_partitions(self.course) + self.assertEqual(len(partitions), 1) + partition = partitions[0] self.assertEqual(partition.scheme.name, "verification") self.assertEqual(partition.parameters["location"], unicode(self.icrv.location)) @@ -108,8 +109,9 @@ def test_removes_deleted_user_partitions(self): self._update_partitions() # Check that the user partition was marked as inactive - self.assertEqual(len(self.course.user_partitions), 1) - partition = self.course.user_partitions[0] + partitions = _get_non_enrollment_track_partitions(self.course) + self.assertEqual(len(partitions), 1) + partition = partitions[0] self.assertFalse(partition.active) self.assertEqual(partition.scheme.name, "verification") @@ -126,23 +128,23 @@ def test_preserves_existing_user_partitions(self): # Add other, non-verified partition to the course self.course.user_partitions = [ UserPartition( - id=0, + id=10, name='Cohort user partition', scheme=UserPartition.get_scheme('cohort'), description='Cohorted user partition', groups=[ - Group(id=0, name="Group A"), - Group(id=1, name="Group B"), + Group(id=100, name="Group A"), + Group(id=101, name="Group B"), ], ), UserPartition( - id=1, + id=11, name='Random user partition', scheme=UserPartition.get_scheme('random'), description='Random user partition', groups=[ - Group(id=0, name="Group A"), - Group(id=1, name="Group B"), + Group(id=102, name="Group A"), + Group(id=103, name="Group B"), ], ), ] @@ -151,10 +153,10 @@ def test_preserves_existing_user_partitions(self): # Update the verification partitions. # The existing partitions should still be available self._update_partitions() - partition_ids = [p.id for p in self.course.user_partitions] + partition_ids = [p.id for p in _get_non_enrollment_track_partitions(self.course)] self.assertEqual(len(partition_ids), 3) - self.assertIn(0, partition_ids) - self.assertIn(1, partition_ids) + self.assertIn(10, partition_ids) + self.assertIn(11, partition_ids) def test_multiple_reverification_blocks(self): # Add an additional ICRV block in another section @@ -162,8 +164,9 @@ def test_multiple_reverification_blocks(self): self._update_partitions() # Expect that both ICRV blocks have corresponding partitions - self.assertEqual(len(self.course.user_partitions), 2) - partition_locations = [p.parameters.get("location") for p in self.course.user_partitions] + partitions = _get_non_enrollment_track_partitions(self.course) + self.assertEqual(len(partitions), 2) + partition_locations = [p.parameters.get("location") for p in partitions] self.assertIn(unicode(self.icrv.location), partition_locations) self.assertIn(unicode(other_icrv.location), partition_locations) @@ -177,10 +180,11 @@ def test_multiple_reverification_blocks(self): self._update_partitions() # Expect that the correct partition is marked as inactive - self.assertEqual(len(self.course.user_partitions), 2) + partitions = _get_non_enrollment_track_partitions(self.course) + self.assertEqual(len(partitions), 2) partitions_by_loc = { p.parameters["location"]: p - for p in self.course.user_partitions + for p in partitions } self.assertFalse(partitions_by_loc[unicode(icrv_location)].active) self.assertTrue(partitions_by_loc[unicode(other_icrv.location)].active) @@ -195,7 +199,7 @@ def test_query_counts_with_no_reverification_blocks(self): # 2 calls: get the course (definitions + structures) # 2 calls: look up ICRV blocks in the course (definitions + structures) - with check_mongo_calls_range(max_finds=4, max_sends=2): + with check_mongo_calls_range(max_finds=4, max_sends=3): self._update_partitions(reload_items=False) def test_query_counts_with_one_reverification_block(self): @@ -259,7 +263,7 @@ def setUp(self): @patch.dict(settings.FEATURES, {"ENABLE_COURSEWARE_INDEX": False}) def test_can_write_on_publish_signal(self): # Sanity check -- initially user partitions should be empty - self.assertEqual(self.course.user_partitions, []) + self.assertEqual(_get_non_enrollment_track_partitions(self.course), []) # Make and publish a change to a block, which should trigger the publish signal with self.store.bulk_operations(self.course.id): # pylint: disable=no-member @@ -272,4 +276,9 @@ def test_can_write_on_publish_signal(self): # should have been created. # We need to verify that these changes were actually persisted to the modulestore. retrieved_course = self.store.get_course(self.course.id) # pylint: disable=no-member - self.assertEqual(len(retrieved_course.user_partitions), 1) + self.assertEqual(len(_get_non_enrollment_track_partitions(retrieved_course)), 1) + + +def _get_non_enrollment_track_partitions(course): + """ Return the user_partitions that are not of type "enrollment_track". """ + return [p for p in course.user_partitions if p.scheme.name != "enrollment_track"] diff --git a/lms/djangoapps/verified_track_content/__init__.py b/openedx/core/djangoapps/verified_track_content/__init__.py similarity index 100% rename from lms/djangoapps/verified_track_content/__init__.py rename to openedx/core/djangoapps/verified_track_content/__init__.py diff --git a/lms/djangoapps/verified_track_content/admin.py b/openedx/core/djangoapps/verified_track_content/admin.py similarity index 61% rename from lms/djangoapps/verified_track_content/admin.py rename to openedx/core/djangoapps/verified_track_content/admin.py index 71ae02219e05..b536c596fece 100644 --- a/lms/djangoapps/verified_track_content/admin.py +++ b/openedx/core/djangoapps/verified_track_content/admin.py @@ -4,8 +4,8 @@ from django.contrib import admin -from verified_track_content.forms import VerifiedTrackCourseForm -from verified_track_content.models import VerifiedTrackCohortedCourse +from openedx.core.djangoapps.verified_track_content.forms import VerifiedTrackCourseForm +from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse @admin.register(VerifiedTrackCohortedCourse) diff --git a/lms/djangoapps/verified_track_content/forms.py b/openedx/core/djangoapps/verified_track_content/forms.py similarity index 94% rename from lms/djangoapps/verified_track_content/forms.py rename to openedx/core/djangoapps/verified_track_content/forms.py index 2de81ee8c69b..53eddf9719c8 100644 --- a/lms/djangoapps/verified_track_content/forms.py +++ b/openedx/core/djangoapps/verified_track_content/forms.py @@ -9,7 +9,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from verified_track_content.models import VerifiedTrackCohortedCourse +from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse class VerifiedTrackCourseForm(forms.ModelForm): diff --git a/lms/djangoapps/verified_track_content/migrations/0001_initial.py b/openedx/core/djangoapps/verified_track_content/migrations/0001_initial.py similarity index 100% rename from lms/djangoapps/verified_track_content/migrations/0001_initial.py rename to openedx/core/djangoapps/verified_track_content/migrations/0001_initial.py diff --git a/lms/djangoapps/verified_track_content/migrations/0002_verifiedtrackcohortedcourse_verified_cohort_name.py b/openedx/core/djangoapps/verified_track_content/migrations/0002_verifiedtrackcohortedcourse_verified_cohort_name.py similarity index 100% rename from lms/djangoapps/verified_track_content/migrations/0002_verifiedtrackcohortedcourse_verified_cohort_name.py rename to openedx/core/djangoapps/verified_track_content/migrations/0002_verifiedtrackcohortedcourse_verified_cohort_name.py diff --git a/lms/djangoapps/verified_track_content/migrations/__init__.py b/openedx/core/djangoapps/verified_track_content/migrations/__init__.py similarity index 100% rename from lms/djangoapps/verified_track_content/migrations/__init__.py rename to openedx/core/djangoapps/verified_track_content/migrations/__init__.py diff --git a/lms/djangoapps/verified_track_content/models.py b/openedx/core/djangoapps/verified_track_content/models.py similarity index 97% rename from lms/djangoapps/verified_track_content/models.py rename to openedx/core/djangoapps/verified_track_content/models.py index fcf7f7113160..213c53bf5b6f 100644 --- a/lms/djangoapps/verified_track_content/models.py +++ b/openedx/core/djangoapps/verified_track_content/models.py @@ -8,9 +8,9 @@ from openedx.core.djangoapps.xmodule_django.models import CourseKeyField from student.models import CourseEnrollment -from courseware.courses import get_course_by_id +from lms.djangoapps.courseware.courses import get_course_by_id -from verified_track_content.tasks import sync_cohort_with_mode +from openedx.core.djangoapps.verified_track_content.tasks import sync_cohort_with_mode from openedx.core.djangoapps.course_groups.cohorts import ( get_course_cohorts, CourseCohort, is_course_cohorted, get_random_cohort ) diff --git a/openedx/core/djangoapps/verified_track_content/partition_scheme.py b/openedx/core/djangoapps/verified_track_content/partition_scheme.py new file mode 100644 index 000000000000..592e7c6b8418 --- /dev/null +++ b/openedx/core/djangoapps/verified_track_content/partition_scheme.py @@ -0,0 +1,147 @@ +""" +UserPartitionScheme for enrollment tracks. +""" +from courseware.masquerade import ( # pylint: disable=import-error + get_course_masquerade, + get_masquerading_group_info, + is_masquerading_as_specific_student, +) +from course_modes.models import CourseMode, EnrollmentMode +from student.models import CourseEnrollment +from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse +from xmodule.partitions.partitions import NoSuchUserPartitionGroupError, Group, UserPartition + + +# EnrollmentMode IDs will be low integers (starting at 0), which is important so that they +# do not overlap with Groups in the CohortUserPartition or the RandomUserPartitionScheme +# (CMS' course_group_config uses a minimum value of 100 for all generated IDs). +ENROLLMENT_GROUP_IDS = {} +try: + for enrollment_mode in EnrollmentMode.objects.all(): + ENROLLMENT_GROUP_IDS[enrollment_mode.mode_slug] = enrollment_mode.id +except: # pylint: disable=bare-except + pass + +if not ENROLLMENT_GROUP_IDS: + # The entries in this table are populated in a migration file, and migration files do not + # run as part of unit tests. Therefore, for tests hard-code the modes. + ENROLLMENT_GROUP_IDS = { + CourseMode.AUDIT: 1, + CourseMode.VERIFIED: 2, + CourseMode.PROFESSIONAL: 3, + CourseMode.NO_ID_PROFESSIONAL_MODE: 4, + CourseMode.CREDIT_MODE: 5, + CourseMode.HONOR: 6 + } + + +class EnrollmentTrackUserPartition(UserPartition): + """ + Extends UserPartition to support dynamic groups pulled from the current course Enrollment tracks. + """ + + @property + def groups(self): + """ + Return the groups (based on CourseModes) for the course associated with this + EnrollmentTrackUserPartition instance. + """ + course_key = CourseKey.from_string(self.parameters["course_id"]) + + if is_course_using_cohort_instead(course_key): + return [] + + all_groups = [] + for mode in CourseMode.modes_for_course(course_key, include_expired=True, only_selectable=False): + group = Group(ENROLLMENT_GROUP_IDS[mode.slug], unicode(mode.name)) + all_groups.append(group) + + return all_groups + + def to_json(self): + """ + 'Serialize' to a json-serializable representation. + + Returns: + a dictionary with keys for the properties of the partition, with an empty array for + `groups` because it is dynamic based on the current course modes. + """ + return { + "id": self.id, + "name": self.name, + "scheme": self.scheme.name, + "description": self.description, + "parameters": self.parameters, + "groups": [], # Do not persist groups because they are dynamic. + "active": bool(self.active), + "version": UserPartition.VERSION + } + + +class EnrollmentTrackPartitionScheme(object): + """ + This scheme uses learner enrollment tracks to map learners into partition groups. + """ + + @classmethod + def get_group_for_user(cls, course_key, user, user_partition): + """ + Returns the Group from the specified user partition to which the user + is assigned, via enrollment mode. + """ + # NOTE: masquerade code was copied from CohortPartitionScheme, and it may need + # some changes (or if not, code should be refactored out and shared). + # This work will be done in a future story TNL-6739. + + # First, check if we have to deal with masquerading. + # If the current user is masquerading as a specific student, use the + # same logic as normal to return that student's group. If the current + # user is masquerading as a generic student in a specific group, then + # return that group. + if get_course_masquerade(user, course_key) and not is_masquerading_as_specific_student(user, course_key): + group_id, user_partition_id = get_masquerading_group_info(user, course_key) + if user_partition_id == user_partition.id and group_id is not None: + try: + return user_partition.get_group(group_id) + except NoSuchUserPartitionGroupError: + return None + # The user is masquerading as a generic student. We can't show any particular group. + return None + + if is_course_using_cohort_instead(course_key): + return None + mode_slug, is_active = CourseEnrollment.enrollment_mode_for_user(user, course_key) + if mode_slug and is_active: + course_mode = CourseMode.mode_for_course(course_key, mode_slug) + if not course_mode: + course_mode = CourseMode.DEFAULT_MODE + return Group(ENROLLMENT_GROUP_IDS[course_mode.slug], unicode(course_mode.name)) + else: + return None + + @classmethod + def create_user_partition(cls, id, name, description, groups=None, parameters=None, active=True): # pylint: disable=redefined-builtin, invalid-name, unused-argument + """ + Create a custom UserPartition to support dynamic groups. + + A Partition has an id, name, scheme, description, parameters, and a list + of groups. The id is intended to be unique within the context where these + are used. (e.g., for partitions of users within a course, the ids should + be unique per-course). The scheme is used to assign users into groups. + The parameters field is used to save extra parameters e.g., location of + the course ID for this partition scheme. + + Partitions can be marked as inactive by setting the "active" flag to False. + Any group access rule referencing inactive partitions will be ignored + when performing access checks. + """ + return EnrollmentTrackUserPartition(id, name, description, [], cls, parameters, active) + + +def is_course_using_cohort_instead(course_key): + """ + Returns whether the given course_context is using verified-track cohorts + and therefore shouldn't use a track-based partition. + """ + return VerifiedTrackCohortedCourse.is_verified_track_cohort_enabled(course_key) diff --git a/lms/djangoapps/verified_track_content/tasks.py b/openedx/core/djangoapps/verified_track_content/tasks.py similarity index 100% rename from lms/djangoapps/verified_track_content/tasks.py rename to openedx/core/djangoapps/verified_track_content/tasks.py diff --git a/lms/djangoapps/verified_track_content/tests/__init__.py b/openedx/core/djangoapps/verified_track_content/tests/__init__.py similarity index 100% rename from lms/djangoapps/verified_track_content/tests/__init__.py rename to openedx/core/djangoapps/verified_track_content/tests/__init__.py diff --git a/lms/djangoapps/verified_track_content/tests/test_forms.py b/openedx/core/djangoapps/verified_track_content/tests/test_forms.py similarity index 94% rename from lms/djangoapps/verified_track_content/tests/test_forms.py rename to openedx/core/djangoapps/verified_track_content/tests/test_forms.py index b400c292c0af..c80c7db68758 100644 --- a/lms/djangoapps/verified_track_content/tests/test_forms.py +++ b/openedx/core/djangoapps/verified_track_content/tests/test_forms.py @@ -4,7 +4,7 @@ from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from verified_track_content.forms import VerifiedTrackCourseForm +from openedx.core.djangoapps.verified_track_content.forms import VerifiedTrackCourseForm class TestVerifiedTrackCourseForm(SharedModuleStoreTestCase): diff --git a/lms/djangoapps/verified_track_content/tests/test_models.py b/openedx/core/djangoapps/verified_track_content/tests/test_models.py similarity index 92% rename from lms/djangoapps/verified_track_content/tests/test_models.py rename to openedx/core/djangoapps/verified_track_content/tests/test_models.py index 71d2458d7416..0c3a4c8d10ce 100644 --- a/lms/djangoapps/verified_track_content/tests/test_models.py +++ b/openedx/core/djangoapps/verified_track_content/tests/test_models.py @@ -1,6 +1,9 @@ """ Tests for Verified Track Cohorting models """ +# pylint: disable=attribute-defined-outside-init +# pylint: disable=no-member + from django.test import TestCase import mock from mock import patch @@ -12,11 +15,12 @@ from student.tests.factories import UserFactory, CourseEnrollmentFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from verified_track_content.models import VerifiedTrackCohortedCourse, DEFAULT_VERIFIED_COHORT_NAME -from verified_track_content.tasks import sync_cohort_with_mode +from ..models import VerifiedTrackCohortedCourse, DEFAULT_VERIFIED_COHORT_NAME +from ..tasks import sync_cohort_with_mode from openedx.core.djangoapps.course_groups.cohorts import ( set_course_cohort_settings, add_cohort, CourseCohort, DEFAULT_COHORT_NAME ) +from openedx.core.djangolib.testing.utils import skip_unless_lms class TestVerifiedTrackCohortedCourse(TestCase): @@ -48,13 +52,13 @@ def test_unicode(self): self.assertEqual(unicode(config), "Course: {}, enabled: True".format(self.SAMPLE_COURSE)) def test_verified_cohort_name(self): - COHORT_NAME = 'verified cohort' + cohort_name = 'verified cohort' course_key = CourseKey.from_string(self.SAMPLE_COURSE) config = VerifiedTrackCohortedCourse.objects.create( - course_key=course_key, enabled=True, verified_cohort_name=COHORT_NAME + course_key=course_key, enabled=True, verified_cohort_name=cohort_name ) config.save() - self.assertEqual(VerifiedTrackCohortedCourse.verified_cohort_name_for_course(course_key), COHORT_NAME) + self.assertEqual(VerifiedTrackCohortedCourse.verified_cohort_name_for_course(course_key), cohort_name) def test_unset_verified_cohort_name(self): fake_course_id = 'fake/course/key' @@ -62,6 +66,7 @@ def test_unset_verified_cohort_name(self): self.assertEqual(VerifiedTrackCohortedCourse.verified_cohort_name_for_course(course_key), None) +@skip_unless_lms class TestMoveToVerified(SharedModuleStoreTestCase): """ Tests for the post-save listener. """ @@ -82,12 +87,15 @@ def setUp(self): self.addCleanup(celery_task_patcher.stop) def _enable_cohorting(self): + """ Turn on cohorting in the course. """ set_course_cohort_settings(self.course.id, is_cohorted=True) def _create_verified_cohort(self, name=DEFAULT_VERIFIED_COHORT_NAME): + """ Create a verified cohort. """ add_cohort(self.course.id, name, CourseCohort.MANUAL) def _create_named_random_cohort(self, name): + """ Create a random cohort with the supplied name. """ return add_cohort(self.course.id, name, CourseCohort.RANDOM) def _enable_verified_track_cohorting(self, cohort_name=None): @@ -101,6 +109,7 @@ def _enable_verified_track_cohorting(self, cohort_name=None): config.save() def _enroll_in_course(self): + """ Enroll self.user in self.course. """ self.enrollment = CourseEnrollmentFactory(course_id=self.course.id, user=self.user) def _upgrade_to_verified(self): @@ -108,6 +117,7 @@ def _upgrade_to_verified(self): self.enrollment.update_enrollment(mode=CourseMode.VERIFIED) def _verify_no_automatic_cohorting(self): + """ Check that upgrading self.user to verified does not move them into a cohort. """ self._enroll_in_course() self.assertIsNone(get_cohort(self.user, self.course.id, assign=False)) self._upgrade_to_verified() @@ -115,13 +125,15 @@ def _verify_no_automatic_cohorting(self): self.assertEqual(0, self.mocked_celery_task.call_count) def _unenroll(self): + """ Unenroll self.user from self.course. """ self.enrollment.unenroll(self.user, self.course.id) def _reenroll(self): + """ Re-enroll the learner into mode AUDIT. """ self.enrollment.activate() self.enrollment.change_mode(CourseMode.AUDIT) - @mock.patch('verified_track_content.models.log.error') + @mock.patch('openedx.core.djangoapps.verified_track_content.models.log.error') def test_automatic_cohorting_disabled(self, error_logger): """ If the VerifiedTrackCohortedCourse feature is disabled for a course, enrollment mode changes do not move @@ -136,7 +148,7 @@ def test_automatic_cohorting_disabled(self, error_logger): # No logging occurs if feature is disabled for course. self.assertFalse(error_logger.called) - @mock.patch('verified_track_content.models.log.error') + @mock.patch('openedx.core.djangoapps.verified_track_content.models.log.error') def test_cohorting_enabled_course_not_cohorted(self, error_logger): """ If the VerifiedTrackCohortedCourse feature is enabled for a course, but the course is not cohorted, @@ -149,7 +161,7 @@ def test_cohorting_enabled_course_not_cohorted(self, error_logger): self.assertTrue(error_logger.called) self.assertIn("course is not cohorted", error_logger.call_args[0][0]) - @mock.patch('verified_track_content.models.log.error') + @mock.patch('openedx.core.djangoapps.verified_track_content.models.log.error') def test_cohorting_enabled_missing_verified_cohort(self, error_logger): """ If the VerifiedTrackCohortedCourse feature is enabled for a course and the course is cohorted, diff --git a/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py new file mode 100644 index 000000000000..cd92f6b07935 --- /dev/null +++ b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py @@ -0,0 +1,152 @@ +""" +Tests for verified_track_content/partition_scheme.py. +""" +from datetime import datetime, timedelta +import pytz + +from ..partition_scheme import EnrollmentTrackPartitionScheme, EnrollmentTrackUserPartition +from ..models import VerifiedTrackCohortedCourse +from course_modes.models import CourseMode + +from student.models import CourseEnrollment +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.partitions.partitions import UserPartition + + +class EnrollmentTrackUserPartitionTest(SharedModuleStoreTestCase): + """ + Tests for the custom EnrollmentTrackUserPartition (dynamic groups). + """ + + @classmethod + def setUpClass(cls): + super(EnrollmentTrackUserPartitionTest, cls).setUpClass() + cls.course = CourseFactory.create() + + def test_only_default_mode(self): + self.assertEqual(len(self.course.user_partitions), 1) + groups = self.course.user_partitions[0].groups + self.assertEqual(1, len(groups)) + self.assertEqual("Audit", groups[0].name) + + def test_using_verified_track_cohort(self): + VerifiedTrackCohortedCourse.objects.create(course_key=self.course.id, enabled=True).save() + self.assertEqual(len(self.course.user_partitions), 1) + groups = self.course.user_partitions[0].groups + self.assertEqual(0, len(groups)) + + def test_multiple_groups(self): + create_mode(self.course, CourseMode.AUDIT, "Audit Enrollment Track", min_price=0) + # Note that the verified mode is expired-- this is intentional. + create_mode( + self.course, CourseMode.VERIFIED, "Verified Enrollment Track", min_price=1, + expiration_datetime=datetime.now(pytz.UTC) + timedelta(days=-1) + ) + # Note that the credit mode is not selectable-- this is intentional. + create_mode(self.course, CourseMode.CREDIT_MODE, "Credit Mode", min_price=2) + + self.assertEqual(len(self.course.user_partitions), 1) + groups = self.course.user_partitions[0].groups + self.assertEqual(3, len(groups)) + self.assertIsNotNone(self.get_group_by_name("Audit Enrollment Track")) + self.assertIsNotNone(self.get_group_by_name("Verified Enrollment Track")) + self.assertIsNotNone(self.get_group_by_name("Credit Mode")) + + def test_to_json(self): + create_mode(self.course, CourseMode.VERIFIED, "Verified Enrollment Track", min_price=1) + user_partition = self.course.user_partitions[0] + self.assertEqual(1, len(user_partition.groups)) + self.assertIsNotNone(self.get_group_by_name("Verified Enrollment Track")) + + json = user_partition.to_json() + self.assertEqual(json['groups'], []) + recreated_user_partition = EnrollmentTrackUserPartition.from_json(json) + self.assertEqual(user_partition, recreated_user_partition) + + groups = recreated_user_partition.groups + self.assertEqual(1, len(groups)) + self.assertEqual("Verified Enrollment Track", groups[0].name) + + def get_group_by_name(self, name): + """ + Return the group in the EnrollmentTrackUserPartition with the given name. + If no such group exists, returns `None`. + """ + for group in self.course.user_partitions[0].groups: + if group.name == name: + return group + return None + + +class EnrollmentTrackPartitionSchemeTest(SharedModuleStoreTestCase): + """ + Tests for EnrollmentTrackPartitionScheme. + """ + + @classmethod + def setUpClass(cls): + super(EnrollmentTrackPartitionSchemeTest, cls).setUpClass() + cls.course = CourseFactory.create() + cls.student = UserFactory() + + def test_get_scheme(self): + """ + Ensure that the scheme extension is correctly plugged in (via entry point in setup.py) + """ + self.assertEquals(UserPartition.get_scheme('enrollment_track'), EnrollmentTrackPartitionScheme) + + def test_create_user_partition(self): + user_partition = UserPartition.get_scheme('enrollment_track').create_user_partition( + 301, "partition", "test partition", parameters={"course_id": unicode(self.course.id)} + ) + self.assertEqual(type(user_partition), EnrollmentTrackUserPartition) + self.assertEqual(user_partition.name, "partition") + + groups = user_partition.groups + self.assertEqual(1, len(groups)) + self.assertEqual("Audit", groups[0].name) + + def test_not_enrolled(self): + self.assertIsNone(self._get_user_group()) + + def test_default_enrollment(self): + CourseEnrollment.enroll(self.student, self.course.id) + self.assertEqual("Audit", self._get_user_group().name) + + def test_enrolled_in_nonexistent_mode(self): + CourseEnrollment.enroll(self.student, self.course.id, mode=CourseMode.VERIFIED) + self.assertEqual("Audit", self._get_user_group().name) + + def test_enrolled_in_verified(self): + create_mode(self.course, CourseMode.VERIFIED, "Verified Enrollment Track", min_price=1) + CourseEnrollment.enroll(self.student, self.course.id, mode=CourseMode.VERIFIED) + self.assertEqual("Verified Enrollment Track", self._get_user_group().name) + + def test_using_verified_track_cohort(self): + VerifiedTrackCohortedCourse.objects.create(course_key=self.course.id, enabled=True).save() + CourseEnrollment.enroll(self.student, self.course.id) + self.assertIsNone(self._get_user_group()) + + def _get_user_group(self): + """ + Gets the group the user is assigned to. + """ + user_partition = self.course.user_partitions[0] + return user_partition.scheme.get_group_for_user(self.course.id, self.student, user_partition) + + +def create_mode(course, mode_slug, mode_name, min_price=0, expiration_datetime=None): + """ + Create a new course mode + """ + return CourseMode.objects.get_or_create( + course_id=course.id, + mode_display_name=mode_name, + mode_slug=mode_slug, + min_price=min_price, + suggested_prices='', + _expiration_datetime=expiration_datetime, + currency='usd' + ) diff --git a/lms/djangoapps/verified_track_content/tests/test_views.py b/openedx/core/djangoapps/verified_track_content/tests/test_views.py similarity index 89% rename from lms/djangoapps/verified_track_content/tests/test_views.py rename to openedx/core/djangoapps/verified_track_content/tests/test_views.py index 6e3017d817ba..dfc80affbb7d 100644 --- a/lms/djangoapps/verified_track_content/tests/test_views.py +++ b/openedx/core/djangoapps/verified_track_content/tests/test_views.py @@ -5,22 +5,21 @@ import json from nose.plugins.attrib import attr -from unittest import skipUnless from django.http import Http404 from django.test.client import RequestFactory -from django.conf import settings +from openedx.core.djangolib.testing.utils import skip_unless_lms from student.tests.factories import UserFactory, AdminFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from verified_track_content.models import VerifiedTrackCohortedCourse -from verified_track_content.views import cohorting_settings +from ..models import VerifiedTrackCohortedCourse +from ..views import cohorting_settings @attr(shard=2) -@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +@skip_unless_lms class CohortingSettingsTestCase(SharedModuleStoreTestCase): """ Tests the `cohort_discussion_topics` view. @@ -65,6 +64,7 @@ def test_cohorting_settings_disabled(self): self._verify_cohort_settings_response(expected_response) def _verify_cohort_settings_response(self, expected_response): + """ Verify that the response was successful and matches the expected JSON payload. """ request = RequestFactory().get("dummy_url") request.user = AdminFactory() response = cohorting_settings(request, unicode(self.course.id)) diff --git a/lms/djangoapps/verified_track_content/views.py b/openedx/core/djangoapps/verified_track_content/views.py similarity index 88% rename from lms/djangoapps/verified_track_content/views.py rename to openedx/core/djangoapps/verified_track_content/views.py index e5d776323b2c..4067a87a34b9 100644 --- a/lms/djangoapps/verified_track_content/views.py +++ b/openedx/core/djangoapps/verified_track_content/views.py @@ -6,9 +6,9 @@ from django.contrib.auth.decorators import login_required from opaque_keys.edx.keys import CourseKey -from courseware.courses import get_course_with_access +from lms.djangoapps.courseware.courses import get_course_with_access -from verified_track_content.models import VerifiedTrackCohortedCourse +from openedx.core.djangoapps.verified_track_content.models import VerifiedTrackCohortedCourse @expect_json diff --git a/setup.py b/setup.py index 08f2637360b4..3d6713cef1cd 100644 --- a/setup.py +++ b/setup.py @@ -42,6 +42,7 @@ "random = openedx.core.djangoapps.user_api.partition_schemes:RandomUserPartitionScheme", "cohort = openedx.core.djangoapps.course_groups.partition_scheme:CohortPartitionScheme", "verification = openedx.core.djangoapps.credit.partition_schemes:VerificationPartitionScheme", + "enrollment_track = openedx.core.djangoapps.verified_track_content.partition_scheme:EnrollmentTrackPartitionScheme", ], "openedx.block_structure_transformer": [ "library_content = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryTransformer", From 9ff4560d04744fd40907359e1aca79d249e74083 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 13 Mar 2017 13:29:55 -0400 Subject: [PATCH 2/5] REMOVE: Hacky UX code to enable togging enrollment groups. --- cms/djangoapps/contentstore/utils.py | 34 ++++++-------- cms/lib/xblock/test/test_authoring_mixin.py | 14 ++++-- cms/static/js/xblock/authoring.js | 26 ++++++++++- cms/static/sass/elements/_modal-window.scss | 2 +- cms/templates/visibility_editor.html | 51 +++++++++++++++++---- 5 files changed, 90 insertions(+), 37 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index f1a5ed149471..da6c2237e5bb 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -83,7 +83,6 @@ def delete_course_and_groups(course_key, user_id): def get_lms_link_for_item(location, preview=False): """ Returns an LMS link to the course with a jump_to to the provided location. - :param location: the location to jump to :param preview: True if the preview version of LMS should be returned. Default value is false. """ @@ -303,25 +302,18 @@ def get_group_display_name(user_partitions, xblock_display_name): def get_user_partition_info(xblock, schemes=None, course=None): """ Retrieve user partition information for an XBlock for display in editors. - * If a partition has been disabled, it will be excluded from the results. - * If a group within a partition is referenced by the XBlock, but the group has been deleted, the group will be marked as deleted in the results. - Arguments: xblock (XBlock): The courseware component being edited. - Keyword Arguments: schemes (iterable of str): If provided, filter partitions to include only schemes with the provided names. - course (XBlock): The course descriptor. If provided, uses this to look up the user partitions instead of loading the course. This is useful if we're calling this function multiple times for the same course want to minimize queries to the modulestore. - Returns: list - Example Usage: >>> get_user_partition_info(block, schemes=["cohort", "verification"]) [ @@ -358,7 +350,6 @@ def get_user_partition_info(xblock, schemes=None, course=None): ] } ] - """ course = course or modulestore().get_course(xblock.location.course_key) @@ -419,38 +410,39 @@ def get_user_partition_info(xblock, schemes=None, course=None): def get_visibility_partition_info(xblock): """ Retrieve user partition information for the component visibility editor. - This pre-processes partition information to simplify the template. - Arguments: xblock (XBlock): The component being edited. - Returns: dict - """ - user_partitions = get_user_partition_info(xblock, schemes=["verification", "cohort"]) + user_partitions = get_user_partition_info(xblock, schemes=["verification", "enrollment_track", "cohort"]) cohort_partitions = [] + enrollment_mode_partitions = [] verification_partitions = [] - has_selected_groups = False + has_selected_content_groups = False + has_selected_enrollment_modes = False selected_verified_partition_id = None # Pre-process the partitions to make it easier to display the UI for p in user_partitions: - has_selected = any(g["selected"] for g in p["groups"]) - has_selected_groups = has_selected_groups or has_selected - if p["scheme"] == "cohort": cohort_partitions.append(p) + has_selected_content_groups = any(g["selected"] for g in p["groups"]) elif p["scheme"] == "verification": verification_partitions.append(p) - if has_selected: - selected_verified_partition_id = p["id"] + # if has_selected: + # selected_verified_partition_id = p["id"] + elif p["scheme"] == "enrollment_track": + enrollment_mode_partitions.append(p) + has_selected_enrollment_modes = any(g["selected"] for g in p["groups"]) return { "user_partitions": user_partitions, "cohort_partitions": cohort_partitions, + "enrollment_mode_partitions": enrollment_mode_partitions, "verification_partitions": verification_partitions, - "has_selected_groups": has_selected_groups, + "has_selected_content_groups": has_selected_content_groups, + "has_selected_enrollment_modes": has_selected_enrollment_modes, "selected_verified_partition_id": selected_verified_partition_id, } diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index f9eb62ff5cff..2b17ce11bfe5 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -101,12 +101,14 @@ def verify_visibility_view_contains(self, item_location, substrings): for string in substrings: self.assertIn(string, html) - def test_html_no_partition(self): - self.verify_visibility_view_contains(self.video_location, 'No content groups exist') + # def test_html_no_partition(self): + # TODO: update with UX changes + # self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_empty_partition(self): self.create_content_groups([]) - self.verify_visibility_view_contains(self.video_location, 'No content groups exist') + # TODO: update with UX changes + # self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_populated_partition(self): self.create_content_groups(self.pet_groups) @@ -114,12 +116,14 @@ def test_html_populated_partition(self): def test_html_no_partition_staff_locked(self): self.set_staff_only(self.vertical_location) - self.verify_visibility_view_contains(self.video_location, ['No content groups exist']) + # TODO: update with UX changes + # self.verify_visibility_view_contains(self.video_location, ['No content groups exist']) def test_html_empty_partition_staff_locked(self): self.create_content_groups([]) self.set_staff_only(self.vertical_location) - self.verify_visibility_view_contains(self.video_location, 'No content groups exist') + # TODO: update with UX changes + # self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_populated_partition_staff_locked(self): self.create_content_groups(self.pet_groups) diff --git a/cms/static/js/xblock/authoring.js b/cms/static/js/xblock/authoring.js index ffeed552b9ef..bd6a6d8ffbc6 100644 --- a/cms/static/js/xblock/authoring.js +++ b/cms/static/js/xblock/authoring.js @@ -20,7 +20,9 @@ } // Cohort partitions (user is allowed to select more than one) - element.find('.field-visibility-content-group input:checked').each(function(index, input) { + element.find( + '.field-visibility-content-group input:checked, .field-visibility-enrollment-mode input:checked' + ).each(function(index, input) { checkboxValues = $(input).val().split('-'); partitionId = parseInt(checkboxValues[0], 10); groupId = parseInt(checkboxValues[1], 10); @@ -44,7 +46,11 @@ // When selecting "all students and staff", uncheck the specific groups element.find('.field-visibility-level input').change(function(event) { if ($(event.target).hasClass('visibility-level-all')) { - element.find('.field-visibility-content-group input, .field-visibility-verification input') + element.find( + '.field-visibility-content-group input, ' + + '.field-visibility-enrollment-mode input, ' + + '.field-visibility-verification input' + ) .prop('checked', false); } }); @@ -54,7 +60,23 @@ element.find('.field-visibility-content-group input, .field-visibility-verification input') .change(function() { element.find('.visibility-level-all').prop('checked', false); + element.find('.visibility-level-enrollment-mode').prop('checked', false); element.find('.visibility-level-specific').prop('checked', true); + + element.find('.field-visibility-enrollment-mode input') + .prop('checked', false); + }); + + // When selecting a specific group, deselect "all students and staff" and + // select "specific content groups" instead.` + element.find('.field-visibility-enrollment-mode input') + .change(function() { + element.find('.visibility-level-all').prop('checked', false); + element.find('.visibility-level-specific').prop('checked', false); + element.find('.visibility-level-enrollment-mode').prop('checked', true); + + element.find('.field-visibility-content-group input') + .prop('checked', false); }); } diff --git a/cms/static/sass/elements/_modal-window.scss b/cms/static/sass/elements/_modal-window.scss index 472c869a270e..1fc177cf6d33 100644 --- a/cms/static/sass/elements/_modal-window.scss +++ b/cms/static/sass/elements/_modal-window.scss @@ -485,7 +485,7 @@ .visibility-controls-secondary { max-height: 100%; overflow-y: auto; - @include margin(($baseline*0.75), 0, 0, $baseline); + @include margin(0, 0, ($baseline*0.75), $baseline); } .visibility-controls-group { diff --git a/cms/templates/visibility_editor.html b/cms/templates/visibility_editor.html index 1344901a2cf9..4faaffab6cca 100644 --- a/cms/templates/visibility_editor.html +++ b/cms/templates/visibility_editor.html @@ -6,8 +6,10 @@ partition_info = get_visibility_partition_info(xblock) user_partitions = partition_info["user_partitions"] cohort_partitions = partition_info["cohort_partitions"] +enrollment_mode_partitions = partition_info["enrollment_mode_partitions"] verification_partitions = partition_info["verification_partitions"] -has_selected_groups = partition_info["has_selected_groups"] +has_selected_content_groups = partition_info["has_selected_content_groups"] +has_selected_enrollment_modes = partition_info["has_selected_enrollment_modes"] selected_verified_partition_id = partition_info["selected_verified_partition_id"] is_staff_locked = ancestor_has_staff_lock(xblock) @@ -54,18 +56,46 @@

${_('Make visible to:')}

- +
- + + +
+ +
+
+
+

${_('Content Groups')}

+
+ % for partition in enrollment_mode_partitions: + % for group in partition["groups"]: +
+ + +
+ % endfor + % endfor + +
+
+
+
+ +
+
-
-
-
+

${_('Content Groups')}

@@ -129,10 +159,15 @@

${_('Content Gro

% endif
-
+ + + + + + -% endif +% endif \ No newline at end of file From 04bf534dedaa260725a3e407bedff7f8066b9373 Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 16 Mar 2017 12:21:45 -0400 Subject: [PATCH 3/5] Add temporary hack until TNL-6731 is addressed. --- cms/djangoapps/contentstore/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index da6c2237e5bb..ddd3f7f220b7 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -295,7 +295,8 @@ def get_group_display_name(user_partitions, xblock_display_name): """ for user_partition in user_partitions: for group in user_partition['groups']: - if str(group['id']) in xblock_display_name: + # Temporary hack until TNL-6731 is addressed. + if group['id'] > 99 and str(group['id']) in xblock_display_name: return group['name'] From c1e8d483469eb04d5a9f47d97b7e2b3139375498 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 21 Mar 2017 10:15:02 -0400 Subject: [PATCH 4/5] Change group to a method. Will need different change on review branch. --- cms/djangoapps/contentstore/utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index ddd3f7f220b7..27b11ef28c7b 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -369,11 +369,11 @@ def get_user_partition_info(xblock, schemes=None, course=None): # Exclude disabled partitions, partitions with no groups defined # Also filter by scheme name if there's a filter defined. - if p.active and p.groups and (schemes is None or p.scheme.name in schemes): + if p.active and p.groups() and (schemes is None or p.scheme.name in schemes): # First, add groups defined by the partition groups = [] - for g in p.groups: + for g in p.groups(): # Falsey group access for a partition mean that all groups # are selected. In the UI, though, we don't show the particular @@ -387,7 +387,7 @@ def get_user_partition_info(xblock, schemes=None, course=None): }) # Next, add any groups set on the XBlock that have been deleted - all_groups = set(g.id for g in p.groups) + all_groups = set(g.id for g in p.groups()) missing_group_ids = selected_groups - all_groups for gid in missing_group_ids: groups.append({ From 68d6efc3acc4f5655da165ec8523722512af6cf3 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 21 Mar 2017 10:49:40 -0400 Subject: [PATCH 5/5] Change group to a method. Change group to a method. --- .../contentstore/course_group_config.py | 2 +- .../contentstore/courseware_index.py | 2 +- cms/djangoapps/contentstore/views/course.py | 6 +-- .../views/tests/test_group_configurations.py | 42 +++++++++---------- .../contentstore/views/tests/test_item.py | 2 +- cms/templates/visibility_editor.html | 4 +- .../xmodule/xmodule/partitions/partitions.py | 21 ++++++++-- .../partitions/tests/test_partitions.py | 14 +++---- .../lib/xmodule/xmodule/split_test_module.py | 10 ++--- .../xmodule/tests/test_split_test_module.py | 4 +- .../course_blocks/transformers/split_test.py | 2 +- .../courseware/tests/test_group_access.py | 4 +- .../discussion_api/tests/test_api.py | 8 ++-- .../tests/test_tasks_helper.py | 8 ++-- lms/lib/xblock/test/test_mixin.py | 4 +- .../tests/test_partition_scheme.py | 6 +-- .../credit/tests/test_verification_access.py | 4 +- .../djangoapps/credit/verification_access.py | 2 +- .../djangoapps/user_api/partition_schemes.py | 4 +- openedx/core/djangoapps/util/testing.py | 8 ++-- .../partition_scheme.py | 3 +- .../tests/test_partition_scheme.py | 14 +++---- 22 files changed, 94 insertions(+), 80 deletions(-) diff --git a/cms/djangoapps/contentstore/course_group_config.py b/cms/djangoapps/contentstore/course_group_config.py index dcbf19b30ee2..791cfb39718e 100644 --- a/cms/djangoapps/contentstore/course_group_config.py +++ b/cms/djangoapps/contentstore/course_group_config.py @@ -84,7 +84,7 @@ def assign_group_ids(self): """ Assign ids for the group_configuration's groups. """ - used_ids = [g.id for p in self.course.user_partitions for g in p.groups] + used_ids = [g.id for p in self.course.user_partitions for g in p.groups()] # Assign ids to every group in configuration. for group in self.configuration.get('groups', []): if group.get('id') is None: diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index 99e0052e2ed4..18062310d9d1 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -190,7 +190,7 @@ def prepare_item_index(item, skip_index=False, groups_usage_info=None): split_partition = item.get_selected_partition() for split_test_child in item.get_children(): if split_partition: - for group in split_partition.groups: + for group in split_partition.groups(): group_id = unicode(group.id) child_location = item.group_id_to_child.get(group_id, None) if child_location == split_test_child.location: diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4b883b0176e7..c2bbe910e597 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1482,10 +1482,10 @@ def remove_content_or_experiment_group(request, store, course, configuration, gr status=400 ) - matching_groups = [group for group in configuration.groups if group.id == group_id] + matching_groups = [group for group in configuration.groups() if group.id == group_id] if matching_groups: - group_index = configuration.groups.index(matching_groups[0]) - configuration.groups.pop(group_index) + group_index = configuration.groups().index(matching_groups[0]) + configuration.groups().pop(group_index) else: return JsonResponse(status=404) diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index f1ea51b2fa78..b75f8dba5e2b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -150,7 +150,7 @@ def _remove_ids(self, content): Returns a tuple that contains removed group configuration ID and group IDs. """ configuration_id = content.pop("id") - group_ids = [group.pop("id") for group in content["groups"]] + group_ids = [group.pop("id") for group in content['groups']] return (configuration_id, group_ids) @@ -289,9 +289,9 @@ def test_can_create_group_configuration(self): user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, u'Test name') - self.assertEqual(len(user_partitions[0].groups), 2) - self.assertEqual(user_partitions[0].groups[0].name, u'Group A') - self.assertEqual(user_partitions[0].groups[1].name, u'Group B') + self.assertEqual(len(user_partitions[0].groups()), 2) + self.assertEqual(user_partitions[0].groups()[0].name, u'Group A') + self.assertEqual(user_partitions[0].groups()[1].name, u'Group B') self.assertEqual(user_partitions[0].parameters, {}) def test_lazily_creates_cohort_configuration(self): @@ -355,9 +355,9 @@ def test_can_create_new_content_group_if_it_does_not_exist(self): user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, u'Test name') - self.assertEqual(len(user_partitions[0].groups), 2) - self.assertEqual(user_partitions[0].groups[0].name, u'Group A') - self.assertEqual(user_partitions[0].groups[1].name, u'Group B') + self.assertEqual(len(user_partitions[0].groups()), 2) + self.assertEqual(user_partitions[0].groups()[0].name, u'Group A') + self.assertEqual(user_partitions[0].groups()[1].name, u'Group B') self.assertEqual(user_partitions[0].parameters, {}) def test_can_edit_content_group(self): @@ -397,9 +397,9 @@ def test_can_edit_content_group(self): self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, u'New Test name') - self.assertEqual(len(user_partititons[0].groups), 2) - self.assertEqual(user_partititons[0].groups[0].name, u'New Group Name') - self.assertEqual(user_partititons[0].groups[1].name, u'Group C') + self.assertEqual(len(user_partititons[0].groups()), 2) + self.assertEqual(user_partititons[0].groups()[0].name, u'New Group Name') + self.assertEqual(user_partititons[0].groups()[1].name, u'Group C') self.assertEqual(user_partititons[0].parameters, {}) def test_can_delete_content_group(self): @@ -422,8 +422,8 @@ def test_can_delete_content_group(self): user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, 'Name 0') - self.assertEqual(len(user_partitions[0].groups), 2) - self.assertEqual(user_partitions[0].groups[1].name, 'Group C') + self.assertEqual(len(user_partitions[0].groups()), 2) + self.assertEqual(user_partitions[0].groups()[1].name, 'Group C') def test_cannot_delete_used_content_group(self): """ @@ -446,8 +446,8 @@ def test_cannot_delete_used_content_group(self): # Verify that user_partitions and groups are still the same. user_partititons = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partititons), 1) - self.assertEqual(len(user_partititons[0].groups), 3) - self.assertEqual(user_partititons[0].groups[1].name, 'Group B') + self.assertEqual(len(user_partititons[0].groups()), 3) + self.assertEqual(user_partititons[0].groups()[1].name, 'Group B') def test_cannot_delete_non_existent_content_group(self): """ @@ -465,7 +465,7 @@ def test_cannot_delete_non_existent_content_group(self): # Verify that user_partitions is still the same. user_partititons = self.course.user_partitions self.assertEqual(len(user_partititons), 1) - self.assertEqual(len(user_partititons[0].groups), 3) + self.assertEqual(len(user_partititons[0].groups()), 3) def test_can_create_new_group_configuration_if_it_does_not_exist(self): """ @@ -500,9 +500,9 @@ def test_can_create_new_group_configuration_if_it_does_not_exist(self): user_partitions = self._get_non_enrollment_track_partitions() self.assertEqual(len(user_partitions), 1) self.assertEqual(user_partitions[0].name, u'Test name') - self.assertEqual(len(user_partitions[0].groups), 2) - self.assertEqual(user_partitions[0].groups[0].name, u'Group A') - self.assertEqual(user_partitions[0].groups[1].name, u'Group B') + self.assertEqual(len(user_partitions[0].groups()), 2) + self.assertEqual(user_partitions[0].groups()[0].name, u'Group A') + self.assertEqual(user_partitions[0].groups()[1].name, u'Group B') self.assertEqual(user_partitions[0].parameters, {}) def test_can_edit_group_configuration(self): @@ -543,9 +543,9 @@ def test_can_edit_group_configuration(self): self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, u'New Test name') - self.assertEqual(len(user_partititons[0].groups), 2) - self.assertEqual(user_partititons[0].groups[0].name, u'New Group Name') - self.assertEqual(user_partititons[0].groups[1].name, u'Group C') + self.assertEqual(len(user_partititons[0].groups()), 2) + self.assertEqual(user_partititons[0].groups()[0].name, u'New Group Name') + self.assertEqual(user_partititons[0].groups()[1].name, u'Group C') self.assertEqual(user_partititons[0].parameters, {}) def test_can_delete_group_configuration(self): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index d114f483cd8c..4b382d4531f9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -827,7 +827,7 @@ def setup_and_verify_content_experiment(self, partition_id): ) split_test = self.get_item_from_modulestore(self.split_test_usage_key, verify_is_draft=True) self.assertEqual(split_test.user_partition_id, partition_id) - self.assertEqual(len(split_test.children), len(self.course.user_partitions[partition_id].groups)) + self.assertEqual(len(split_test.children), len(self.course.user_partitions[partition_id].groups())) return split_test def _move_component(self, source_usage_key, target_usage_key, target_index=None): diff --git a/cms/templates/visibility_editor.html b/cms/templates/visibility_editor.html index 4faaffab6cca..1c6d8f7a927b 100644 --- a/cms/templates/visibility_editor.html +++ b/cms/templates/visibility_editor.html @@ -71,7 +71,7 @@

${_('Make visible to:')}

${_('Content Groups')}

% for partition in enrollment_mode_partitions: - % for group in partition["groups"]: + % for group in partition.groups:
${_('Content Gro

${_('Content Groups')}

% for partition in cohort_partitions: - % for group in partition["groups"]: + % for group in partition.groups: