From bc32819ce0e049e1348184d43772880ee0734f3d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 10 Jan 2019 13:48:30 -0500 Subject: [PATCH] Fix Studio Group Configurations for courses with FBE Enabled This prevents the Feature-Based Enrollments groups from showing in Group Configurations unless the studio_override_enabled flag is enabled. It also fixes a bug where FBE group configurations were displaying usage as though they were Track-based group configurations. REVMI-78 --- .../contentstore/course_group_config.py | 105 ++++++++---------- .../contentstore/courseware_index.py | 29 +++-- cms/djangoapps/contentstore/utils.py | 5 +- cms/djangoapps/contentstore/views/course.py | 15 ++- .../views/tests/test_group_configurations.py | 51 ++++++++- .../swap_from_auto_track_cohort_pilot.py | 2 +- .../content_type_gating/partitions.py | 19 ++-- 7 files changed, 138 insertions(+), 88 deletions(-) diff --git a/cms/djangoapps/contentstore/course_group_config.py b/cms/djangoapps/contentstore/course_group_config.py index e06a23be1b..455b18f34a 100644 --- a/cms/djangoapps/contentstore/course_group_config.py +++ b/cms/djangoapps/contentstore/course_group_config.py @@ -1,6 +1,7 @@ """ Class for manipulating groups configuration on a course object. """ +from collections import defaultdict import json import logging @@ -111,7 +112,7 @@ class GroupConfiguration(object): raise GroupConfigurationsValidationError(_("unable to load this type of group configuration")) @staticmethod - def _get_usage_info(course, unit, item, usage_info, group_id, scheme_name=None): + def _get_usage_dict(course, unit, item, scheme_name=None): """ Get usage info for unit/module. """ @@ -145,10 +146,7 @@ class GroupConfiguration(object): if scheme_name == RANDOM_SCHEME: validation_summary = item.general_validation_message() usage_dict.update({'validation': validation_summary.to_json() if validation_summary else None}) - - usage_info[group_id].append(usage_dict) - - return usage_info + return usage_dict @staticmethod def get_content_experiment_usage_info(store, course): @@ -192,24 +190,21 @@ class GroupConfiguration(object): ], } """ - usage_info = {} + usage_info = defaultdict(list) for split_test in split_tests: - if split_test.user_partition_id not in usage_info: - usage_info[split_test.user_partition_id] = [] - unit = split_test.get_parent() if not unit: log.warning("Unable to find parent for split_test %s", split_test.location) + # Make sure that this user_partition appears in the output even though it has no content + usage_info[split_test.user_partition_id] = [] continue - usage_info = GroupConfiguration._get_usage_info( + usage_info[split_test.user_partition_id].append(GroupConfiguration._get_usage_dict( course=course, unit=unit, item=split_test, - usage_info=usage_info, - group_id=split_test.user_partition_id, - scheme_name=RANDOM_SCHEME - ) + scheme_name=RANDOM_SCHEME, + )) return usage_info @staticmethod @@ -218,38 +213,35 @@ class GroupConfiguration(object): Returns all units names and their urls. Returns: - {'group_id': - [ - { - 'label': 'Unit 1 / Problem 1', - 'url': 'url_to_unit_1' - }, - { - 'label': 'Unit 2 / Problem 2', - 'url': 'url_to_unit_2' - } - ], + {'partition_id': + {'group_id': + [ + { + 'label': 'Unit 1 / Problem 1', + 'url': 'url_to_unit_1' + }, + { + 'label': 'Unit 2 / Problem 2', + 'url': 'url_to_unit_2' + } + ], + } } """ items = store.get_items(course.id, settings={'group_access': {'$exists': True}}, include_orphans=False) - usage_info = {} - for item, group_id in GroupConfiguration._iterate_items_and_group_ids(course, items): - if group_id not in usage_info: - usage_info[group_id] = [] - + usage_info = defaultdict(lambda: defaultdict(list)) + for item, partition_id, group_id in GroupConfiguration._iterate_items_and_group_ids(course, items): unit = item.get_parent() if not unit: log.warning("Unable to find parent for component %s", item.location) continue - usage_info = GroupConfiguration._get_usage_info( + usage_info[partition_id][group_id].append(GroupConfiguration._get_usage_dict( course, unit=unit, item=item, - usage_info=usage_info, - group_id=group_id - ) + )) return usage_info @@ -267,34 +259,31 @@ class GroupConfiguration(object): """ Returns all items names and their urls. - This will return only groups for the cohort user partition. + This will return only groups for all non-random partitions. Returns: - {'group_id': - [ - { - 'label': 'Problem 1 / Problem 1', - 'url': 'url_to_item_1' - }, - { - 'label': 'Problem 2 / Problem 2', - 'url': 'url_to_item_2' - } - ], + {'partition_id': + {'group_id': + [ + { + 'label': 'Problem 1 / Problem 1', + 'url': 'url_to_item_1' + }, + { + 'label': 'Problem 2 / Problem 2', + 'url': 'url_to_item_2' + } + ], + } } """ - usage_info = {} - for item, group_id in GroupConfiguration._iterate_items_and_group_ids(course, items): - if group_id not in usage_info: - usage_info[group_id] = [] - - usage_info = GroupConfiguration._get_usage_info( + usage_info = defaultdict(lambda: defaultdict(list)) + for item, partition_id, group_id in GroupConfiguration._iterate_items_and_group_ids(course, items): + usage_info[partition_id][group_id].append(GroupConfiguration._get_usage_dict( course, unit=item, item=item, - usage_info=usage_info, - group_id=group_id - ) + )) return usage_info @@ -305,7 +294,7 @@ class GroupConfiguration(object): This will yield group IDs for all user partitions except those with a scheme of random. - Yields: tuple of (item, group_id) + Yields: tuple of (item, partition_id, group_id) """ all_partitions = get_all_partitions_for_course(course) for config in all_partitions: @@ -315,7 +304,7 @@ class GroupConfiguration(object): group_ids = item.group_access.get(config.id, []) for group_id in group_ids: - yield item, group_id + yield item, config.id, group_id @staticmethod def update_usage_info(store, course, configuration): @@ -351,7 +340,7 @@ class GroupConfiguration(object): partition_configuration = configuration.to_json() for group in partition_configuration['groups']: - group['usage'] = usage_info.get(group['id'], []) + group['usage'] = usage_info[configuration.id].get(group['id'], []) return partition_configuration diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index d08eabeda3..d6fab2c9a6 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -379,22 +379,21 @@ class CoursewareSearchIndexer(SearchIndexerBase): @classmethod def fetch_group_usage(cls, modulestore, structure): groups_usage_dict = {} - groups_usage_info = GroupConfiguration.get_partitions_usage_info(modulestore, structure).items() - groups_usage_info.extend( - GroupConfiguration.get_content_groups_items_usage_info( - modulestore, - structure - ).items() + partitions_info = GroupConfiguration.get_partitions_usage_info(modulestore, structure) + content_group_info = GroupConfiguration.get_content_groups_items_usage_info( + modulestore, + structure ) - if groups_usage_info: - for name, group in groups_usage_info: - for module in group: - view, args, kwargs = resolve(module['url']) # pylint: disable=unused-variable - usage_key_string = unicode(kwargs['usage_key_string']) - if groups_usage_dict.get(usage_key_string, None): - groups_usage_dict[usage_key_string].append(name) - else: - groups_usage_dict[usage_key_string] = [name] + for group_info in (partitions_info, content_group_info): + for groups in group_info.values(): + for name, group in groups.items(): + for module in group: + view, args, kwargs = resolve(module['url']) # pylint: disable=unused-variable + usage_key_string = unicode(kwargs['usage_key_string']) + if groups_usage_dict.get(usage_key_string, None): + groups_usage_dict[usage_key_string].append(name) + else: + groups_usage_dict[usage_key_string] = [name] return groups_usage_dict @classmethod diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index ac45a98629..20f4f149d9 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -16,11 +16,12 @@ from six import text_type from django_comment_common.models import assign_default_role from django_comment_common.utils import seed_permissions_roles from openedx.core.djangoapps.site_configuration.models import SiteConfiguration -from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.config import ( CONTENT_TYPE_GATING_FLAG, FEATURE_BASED_ENROLLMENT_GLOBAL_KILL_FLAG ) +from openedx.features.content_type_gating.models import ContentTypeGatingConfig +from openedx.features.content_type_gating.partitions import CONTENT_TYPE_GATING_SCHEME from student import auth from student.models import CourseEnrollment from student.roles import CourseInstructorRole, CourseStaffRole @@ -469,7 +470,7 @@ def get_visibility_partition_info(xblock, course=None): if not is_library and ( flag_enabled or ContentTypeGatingConfig.current(course_key=course_key).studio_override_enabled ): - selectable_partitions += get_user_partition_info(xblock, schemes=["content_type_gate"], course=course) + selectable_partitions += get_user_partition_info(xblock, schemes=[CONTENT_TYPE_GATING_SCHEME], course=course) # Now add the cohort user partitions. selectable_partitions = selectable_partitions + get_user_partition_info(xblock, schemes=["cohort"], course=course) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 6e4c8771da..b469e57a86 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1,6 +1,7 @@ """ Views related to operations on course objects """ +from collections import defaultdict import copy import json import logging @@ -66,6 +67,8 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_ from openedx.core.djangolib.js_utils import dump_js_escaped_json from openedx.core.lib.course_tabs import CourseTabPluginManager from openedx.core.lib.courses import course_image_url +from openedx.features.content_type_gating.partitions import CONTENT_TYPE_GATING_SCHEME +from openedx.features.content_type_gating.models import ContentTypeGatingConfig from student import auth from student.auth import has_course_author_access, has_studio_read_access, has_studio_write_access from student.roles import CourseCreatorRole, CourseInstructorRole, CourseStaffRole, GlobalStaff, UserBasedRole @@ -1551,7 +1554,7 @@ def remove_content_or_experiment_group(request, store, course, configuration, gr group_id = int(group_id) usages = GroupConfiguration.get_partitions_usage_info(store, course) - used = group_id in usages + used = group_id in usages[configuration.id] if used: return JsonResponse( @@ -1608,6 +1611,10 @@ def group_configurations_list_handler(request, course_key_string): if partition['scheme'] == COHORT_SCHEME: has_content_groups = True displayable_partitions.append(partition) + elif partition['scheme'] == CONTENT_TYPE_GATING_SCHEME: + # Add it to the front of the list if it should be shown. + if ContentTypeGatingConfig.current(course_key=course_key).studio_override_enabled: + displayable_partitions.append(partition) elif partition['scheme'] == ENROLLMENT_SCHEME: should_show_enrollment_track = len(partition['groups']) > 1 @@ -1619,6 +1626,12 @@ def group_configurations_list_handler(request, course_key_string): # want to display their groups twice. displayable_partitions.append(partition) + # Set the sort-order. Higher numbers sort earlier + scheme_priority = defaultdict(lambda: -1, { + ENROLLMENT_SCHEME: 1, + CONTENT_TYPE_GATING_SCHEME: 0 + }) + displayable_partitions.sort(key=lambda p: scheme_priority[p['scheme']], reverse=True) # Add empty content group if there is no COHORT User Partition in the list. # This will add ability to add new groups in the view. if not has_content_groups: diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index bebface6e3..b202f0fa3b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -9,9 +9,10 @@ from mock import patch from operator import itemgetter from contentstore.utils import reverse_course_url, reverse_usage_url -from contentstore.course_group_config import GroupConfiguration, CONTENT_GROUP_CONFIGURATION_NAME +from contentstore.course_group_config import GroupConfiguration, CONTENT_GROUP_CONFIGURATION_NAME, ENROLLMENT_SCHEME from contentstore.tests.utils import CourseTestCase from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITION_ID +from openedx.features.content_type_gating.partitions import CONTENT_TYPE_GATING_SCHEME from xmodule.partitions.partitions import Group, UserPartition, ENROLLMENT_TRACK_PARTITION_ID from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.validation import StudioValidation, StudioValidationMessage @@ -650,7 +651,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio self.assertEqual(len(user_partititons), 2) self.assertEqual(user_partititons[0].name, 'Name 0') - @ddt.data('content_type_gate', 'enrollment_track') + @ddt.data(CONTENT_TYPE_GATING_SCHEME, ENROLLMENT_SCHEME) def test_cannot_create_restricted_group_configuration(self, scheme_id): """ Test that you cannot create a restricted group configuration. @@ -665,8 +666,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio self.assertEqual(response.status_code, 400) @ddt.data( - ('content_type_gate', CONTENT_GATING_PARTITION_ID), - ('enrollment_track', ENROLLMENT_TRACK_PARTITION_ID), + (CONTENT_TYPE_GATING_SCHEME, CONTENT_GATING_PARTITION_ID), + (ENROLLMENT_SCHEME, ENROLLMENT_TRACK_PARTITION_ID), ) @ddt.unpack def test_cannot_edit_restricted_group_configuration(self, scheme_id, partition_id): @@ -1120,6 +1121,48 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): actual = GroupConfiguration.get_content_groups_items_usage_info(self.store, self.course) self.assertEqual(actual.keys(), [0]) + def test_can_handle_duplicate_group_ids(self): + # Create the user partitions + self.course.user_partitions = [ + UserPartition( + id=0, + name='Cohort user partition 1', + scheme=UserPartition.get_scheme('cohort'), + description='Cohorted user partition', + groups=[ + Group(id=2, name="Group 1A"), + Group(id=3, name="Group 1B"), + ], + ), + UserPartition( + id=1, + name='Cohort user partition 2', + scheme=UserPartition.get_scheme('cohort'), + description='Random user partition', + groups=[ + Group(id=2, name="Group 2A"), + Group(id=3, name="Group 2B"), + ], + ), + ] + self.store.update_item(self.course, ModuleStoreEnum.UserID.test) + + # Assign group access rules for multiple partitions, one of which is a cohorted partition + self._create_problem_with_content_group(0, 2, name_suffix='0') + self._create_problem_with_content_group(1, 3, name_suffix='1') + + # This used to cause an exception since the code assumed that + # only one partition would be available. + actual = GroupConfiguration.get_partitions_usage_info(self.store, self.course) + self.assertEqual(actual.keys(), [0, 1]) + self.assertEqual(actual[0].keys(), [2]) + self.assertEqual(actual[1].keys(), [3]) + + actual = GroupConfiguration.get_content_groups_items_usage_info(self.store, self.course) + self.assertEqual(actual.keys(), [0, 1]) + self.assertEqual(actual[0].keys(), [2]) + self.assertEqual(actual[1].keys(), [3]) + class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): """ diff --git a/openedx/core/djangoapps/verified_track_content/management/commands/swap_from_auto_track_cohort_pilot.py b/openedx/core/djangoapps/verified_track_content/management/commands/swap_from_auto_track_cohort_pilot.py index 63a09eed69..711efbb3c5 100644 --- a/openedx/core/djangoapps/verified_track_content/management/commands/swap_from_auto_track_cohort_pilot.py +++ b/openedx/core/djangoapps/verified_track_content/management/commands/swap_from_auto_track_cohort_pilot.py @@ -242,7 +242,7 @@ class Command(BaseCommand): # since they should have been converted to use enrollment tracks instead. # Taken from contentstore/views/course.py.remove_content_or_experiment_group usages = GroupConfiguration.get_partitions_usage_info(module_store, course) - used = group_id in usages + used = group_id in usages[partition.id] if used: errors.append("Content group '%s' is in use and cannot be deleted." % partition_to_delete.group_id) diff --git a/openedx/features/content_type_gating/partitions.py b/openedx/features/content_type_gating/partitions.py index 41eeeeef1c..4fea2b1631 100644 --- a/openedx/features/content_type_gating/partitions.py +++ b/openedx/features/content_type_gating/partitions.py @@ -22,6 +22,8 @@ from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITIO LOG = logging.getLogger(__name__) +CONTENT_TYPE_GATING_SCHEME = "content_type_gate" + def create_content_gating_partition(course): """ @@ -34,9 +36,12 @@ def create_content_gating_partition(course): return None try: - content_gate_scheme = UserPartition.get_scheme("content_type_gate") + content_gate_scheme = UserPartition.get_scheme(CONTENT_TYPE_GATING_SCHEME) except UserPartitionError: - LOG.warning("No 'content_type_gate' scheme registered, ContentTypeGatingPartitionScheme will not be created.") + LOG.warning( + "No %r scheme registered, ContentTypeGatingPartitionScheme will not be created.", + CONTENT_TYPE_GATING_SCHEME + ) return None used_ids = set(p.id for p in course.user_partitions) @@ -45,11 +50,11 @@ def create_content_gating_partition(course): # partition with id 51, it will collide with the Content Gating Partition. We'll catch that here, and # then fix the course content as needed (or get the course team to). LOG.warning( - "Can't add 'content_type_gate' partition, as ID {id} is assigned to {partition} in course {course}.".format( - id=CONTENT_GATING_PARTITION_ID, - partition=_get_partition_from_id(course.user_partitions, CONTENT_GATING_PARTITION_ID).name, - course=unicode(course.id) - ) + "Can't add %r partition, as ID %r is assigned to %r in course %s.", + CONTENT_TYPE_GATING_SCHEME, + CONTENT_GATING_PARTITION_ID, + _get_partition_from_id(course.user_partitions, CONTENT_GATING_PARTITION_ID).name, + unicode(course.id), ) return None