diff --git a/cms/djangoapps/contentstore/course_group_config.py b/cms/djangoapps/contentstore/course_group_config.py index 69b2051a72..2056ce91cf 100644 --- a/cms/djangoapps/contentstore/course_group_config.py +++ b/cms/djangoapps/contentstore/course_group_config.py @@ -199,6 +199,8 @@ class GroupConfiguration(object): """ Returns all units names and their urls. + This will return only groups for the cohort user partition. + Returns: {'group_id': [ @@ -214,25 +216,22 @@ class GroupConfiguration(object): } """ usage_info = {} - for item in items: - if hasattr(item, 'group_access') and item.group_access: - (__, group_ids), = item.group_access.items() - for group_id in group_ids: - if group_id not in usage_info: - usage_info[group_id] = [] + for item, group_id in GroupConfiguration._iterate_items_and_content_group_ids(course, items): + if group_id not in usage_info: + usage_info[group_id] = [] - unit = item.get_parent() - if not unit: - log.warning("Unable to find parent for component %s", item.location) - continue + 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( - course, - unit=unit, - item=item, - usage_info=usage_info, - group_id=group_id - ) + usage_info = GroupConfiguration._get_usage_info( + course, + unit=unit, + item=item, + usage_info=usage_info, + group_id=group_id + ) return usage_info @@ -250,6 +249,8 @@ class GroupConfiguration(object): """ Returns all items names and their urls. + This will return only groups for the cohort user partition. + Returns: {'group_id': [ @@ -265,23 +266,38 @@ class GroupConfiguration(object): } """ usage_info = {} - for item in items: - if hasattr(item, 'group_access') and item.group_access: - (__, group_ids), = item.group_access.items() - for group_id in group_ids: - if group_id not in usage_info: - usage_info[group_id] = [] + for item, group_id in GroupConfiguration._iterate_items_and_content_group_ids(course, items): + if group_id not in usage_info: + usage_info[group_id] = [] - usage_info = GroupConfiguration._get_usage_info( - course, - unit=item, - item=item, - usage_info=usage_info, - group_id=group_id - ) + usage_info = GroupConfiguration._get_usage_info( + course, + unit=item, + item=item, + usage_info=usage_info, + group_id=group_id + ) return usage_info + @staticmethod + def _iterate_items_and_content_group_ids(course, items): + """ + Iterate through items and content group IDs in a course. + + This will yield group IDs *only* for cohort user partitions. + + Yields: tuple of (item, group_id) + """ + content_group_configuration = get_cohorted_user_partition(course) + if content_group_configuration is not None: + for item in items: + if hasattr(item, 'group_access') and item.group_access: + group_ids = item.group_access.get(content_group_configuration.id, []) + + for group_id in group_ids: + yield item, group_id + @staticmethod def update_usage_info(store, course, configuration): """ @@ -329,7 +345,7 @@ class GroupConfiguration(object): the client explicitly creates a group within the partition and POSTs back. """ - content_group_configuration = get_cohorted_user_partition(course.id) + content_group_configuration = get_cohorted_user_partition(course) if content_group_configuration is None: content_group_configuration = UserPartition( id=generate_int_id(MINIMUM_GROUP_ID, MYSQL_MAX_INT, GroupConfiguration.get_used_ids(course)), diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 69b8fc6a69..2ed22b6a3c 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -12,6 +12,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.django import modulestore +from xmodule.partitions.partitions import UserPartition, Group from contentstore import utils from contentstore.tests.utils import CourseTestCase @@ -413,6 +414,43 @@ class GroupVisibilityTest(CourseTestCase): self.html = self.store.get_item(html.location) self.problem = self.store.get_item(problem.location) + # Add partitions to the course + self.course.user_partitions = [ + UserPartition( + id=0, + name="Partition 0", + description="Partition 0", + scheme=UserPartition.get_scheme("random"), + groups=[ + Group(id=0, name="Group A"), + Group(id=1, name="Group B"), + ], + ), + UserPartition( + id=1, + name="Partition 1", + description="Partition 1", + scheme=UserPartition.get_scheme("random"), + groups=[ + Group(id=0, name="Group C"), + Group(id=1, name="Group D"), + ], + ), + UserPartition( + id=2, + name="Partition 2", + description="Partition 2", + scheme=UserPartition.get_scheme("random"), + groups=[ + Group(id=0, name="Group E"), + Group(id=1, name="Group F"), + Group(id=2, name="Group G"), + Group(id=3, name="Group H"), + ], + ), + ] + self.course = self.store.update_item(self.course, ModuleStoreEnum.UserID.test) + def set_group_access(self, xblock, value): """ Sets group_access to specified value and calls update_item to persist the change. """ xblock.group_access = value @@ -452,3 +490,173 @@ class GroupVisibilityTest(CourseTestCase): self.assertFalse(utils.is_visible_to_specific_content_groups(self.vertical)) self.assertFalse(utils.is_visible_to_specific_content_groups(self.html)) self.assertTrue(utils.is_visible_to_specific_content_groups(self.problem)) + + +class GetUserPartitionInfoTest(ModuleStoreTestCase): + """ + Tests for utility function that retrieves user partition info + and formats it for consumption by the editing UI. + """ + + def setUp(self): + """Create a dummy course. """ + super(GetUserPartitionInfoTest, self).setUp() + self.course = CourseFactory() + self.block = ItemFactory.create(category="problem", parent_location=self.course.location) # pylint: disable=no-member + + # Set up some default partitions + self._set_partitions([ + UserPartition( + id=0, + 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"), + ], + ), + UserPartition( + id=1, + name="Random user partition", + scheme=UserPartition.get_scheme("random"), + description="Random user partition", + groups=[ + Group(id=0, name="Group C"), + ], + ), + ]) + + def test_retrieves_partition_info_with_selected_groups(self): + # Initially, no group access is set on the block, so no groups should + # be marked as selected. + expected = [ + { + "id": 0, + "name": "Cohort user partition", + "scheme": "cohort", + "groups": [ + { + "id": 0, + "name": "Group A", + "selected": False, + "deleted": False, + }, + { + "id": 1, + "name": "Group B", + "selected": False, + "deleted": False, + }, + ] + }, + { + "id": 1, + "name": "Random user partition", + "scheme": "random", + "groups": [ + { + "id": 0, + "name": "Group C", + "selected": False, + "deleted": False, + }, + ] + } + ] + self.assertEqual(self._get_partition_info(), 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) + + def test_deleted_groups(self): + # Select a group that is not defined in the partition + self._set_group_access({0: [3]}) + + # Expect that the group appears as selected but is marked as deleted + partitions = self._get_partition_info() + groups = partitions[0]["groups"] + self.assertEqual(len(groups), 3) + self.assertEqual(groups[2], { + "id": 3, + "name": "Deleted group", + "selected": True, + "deleted": True + }) + + def test_filter_by_partition_scheme(self): + partitions = self._get_partition_info(schemes=["random"]) + self.assertEqual(len(partitions), 1) + self.assertEqual(partitions[0]["scheme"], "random") + + def test_exclude_inactive_partitions(self): + # Include an inactive verification scheme + self._set_partitions([ + UserPartition( + id=0, + 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"), + ], + ), + UserPartition( + id=1, + name="Verification user partition", + scheme=UserPartition.get_scheme("verification"), + description="Verification user partition", + groups=[ + Group(id=0, name="Group C"), + ], + active=False, + ), + ]) + + # Expect that the inactive scheme is excluded from the results + partitions = self._get_partition_info() + self.assertEqual(len(partitions), 1) + self.assertEqual(partitions[0]["scheme"], "cohort") + + def test_exclude_partitions_with_no_groups(self): + # The cohort partition has no groups defined + self._set_partitions([ + UserPartition( + id=0, + name="Cohort user partition", + scheme=UserPartition.get_scheme("cohort"), + description="Cohorted user partition", + groups=[], + ), + UserPartition( + id=1, + name="Verification user partition", + scheme=UserPartition.get_scheme("verification"), + description="Verification user partition", + groups=[ + Group(id=0, name="Group C"), + ], + ), + ]) + + # Expect that the partition with no groups is excluded from the results + partitions = self._get_partition_info() + self.assertEqual(len(partitions), 1) + self.assertEqual(partitions[0]["scheme"], "verification") + + def _set_partitions(self, partitions): + """Set the user partitions of the course descriptor. """ + self.course.user_partitions = partitions + self.course = self.store.update_item(self.course, ModuleStoreEnum.UserID.test) + + def _set_group_access(self, group_access): + """Set group access of the block. """ + self.block.group_access = group_access + self.block = self.store.update_item(self.block, ModuleStoreEnum.UserID.test) + + def _get_partition_info(self, schemes=None): + """Retrieve partition info and selected groups. """ + return utils.get_user_partition_info(self.block, schemes=schemes) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 9534f34d28..eba7ecc678 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -10,6 +10,7 @@ from pytz import UTC from django.conf import settings from django.core.urlresolvers import reverse +from django.utils.translation import ugettext as _ from django_comment_common.models import assign_default_role from django_comment_common.utils import seed_permissions_roles @@ -213,12 +214,11 @@ def is_visible_to_specific_content_groups(xblock): """ if not xblock.group_access: return False - for __, value in xblock.group_access.iteritems(): - # value should be a list of group IDs. If it is an empty list or None, the xblock is visible - # to all groups in that particular partition. So if value is a truthy value, the xblock is - # restricted in some way. - if value: + + for partition in get_user_partition_info(xblock): + if any(g["selected"] for g in partition["groups"]): return True + return False @@ -331,3 +331,158 @@ def has_active_web_certificate(course): cert_config = True break return cert_config + + +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"]) + [ + { + "id": 12345, + "name": "Cohorts" + "scheme": "cohort", + "groups": [ + { + "id": 7890, + "name": "Foo", + "selected": True, + "deleted": False, + } + ] + }, + { + "id": 7292, + "name": "Midterm A", + "scheme": "verification", + "groups": [ + { + "id": 1, + "name": "Completed verification at Midterm A", + "selected": False, + "deleted": False + }, + { + "id": 0, + "name": "Did not complete verification at Midterm A", + "selected": False, + "deleted": False, + } + ] + } + ] + + """ + course = course or modulestore().get_course(xblock.location.course_key) + + if course is None: + log.warning( + "Could not find course %s to retrieve user partition information", + xblock.location.course_key + ) + return [] + + if schemes is not None: + schemes = set(schemes) + + partitions = [] + for p in sorted(course.user_partitions, key=lambda p: p.name): + + # 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): + + # First, add groups defined by the partition + 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 + # groups selected, since there's a separate option for "all users". + selected_groups = set(xblock.group_access.get(p.id, []) or []) + groups.append({ + "id": g.id, + "name": g.name, + "selected": g.id in selected_groups, + "deleted": False, + }) + + # Next, add any groups set on the XBlock that have been deleted + 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({ + "id": gid, + "name": _("Deleted group"), + "selected": True, + "deleted": True, + }) + + # Put together the entire partition dictionary + partitions.append({ + "id": p.id, + "name": p.name, + "scheme": p.scheme.name, + "groups": groups, + }) + + return partitions + + +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"]) + cohort_partitions = [] + verification_partitions = [] + has_selected_groups = 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) + elif p["scheme"] == "verification": + verification_partitions.append(p) + if has_selected: + selected_verified_partition_id = p["id"] + + return { + "user_partitions": user_partitions, + "cohort_partitions": cohort_partitions, + "verification_partitions": verification_partitions, + "has_selected_groups": has_selected_groups, + "selected_verified_partition_id": selected_verified_partition_id, + } diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 04bf559e65..debc327ab5 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -40,8 +40,11 @@ from util.date_utils import get_default_time_display from util.json_request import expect_json, JsonResponse from student.auth import has_studio_write_access, has_studio_read_access -from contentstore.utils import find_release_date_source, find_staff_lock_source, is_currently_visible_to_students, \ - ancestor_has_staff_lock, has_children_visible_to_specific_content_groups +from contentstore.utils import ( + find_release_date_source, find_staff_lock_source, is_currently_visible_to_students, + ancestor_has_staff_lock, has_children_visible_to_specific_content_groups, + get_user_partition_info, +) from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \ xblock_type_display_name, get_parent_xblock, create_xblock, usage_key_with_run from contentstore.views.preview import get_preview_fragment @@ -756,7 +759,7 @@ def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=Fa def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=False, include_child_info=False, course_outline=False, include_children_predicate=NEVER, parent_xblock=None, graders=None, - user=None): + user=None, course=None): """ Creates the information needed for client-side XBlockInfo. @@ -788,6 +791,11 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F # Filter the graders data as needed graders = _filter_entrance_exam_grader(graders) + # We need to load the course in order to retrieve user partition information. + # For this reason, we load the course once and re-use it when recursively loading children. + if course is None: + course = modulestore().get_course(xblock.location.course_key) + # Compute the child info first so it can be included in aggregate information for the parent should_visit_children = include_child_info and (course_outline and not is_xblock_unit or not course_outline) if should_visit_children and xblock.has_children: @@ -796,7 +804,8 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F course_outline, graders, include_children_predicate=include_children_predicate, - user=user + user=user, + course=course ) else: child_info = None @@ -850,6 +859,8 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F "has_changes": has_changes, "actions": xblock_actions, "explanatory_message": explanatory_message, + "group_access": xblock.group_access, + "user_partitions": get_user_partition_info(xblock, course=course), } # update xblock_info with proctored_exam information if the feature flag is enabled @@ -1023,7 +1034,7 @@ def _create_xblock_ancestor_info(xblock, course_outline): } -def _create_xblock_child_info(xblock, course_outline, graders, include_children_predicate=NEVER, user=None): +def _create_xblock_child_info(xblock, course_outline, graders, include_children_predicate=NEVER, user=None, course=None): # pylint: disable=line-too-long """ Returns information about the children of an xblock, as well as about the primary category of xblock expected as children. @@ -1042,7 +1053,8 @@ def _create_xblock_child_info(xblock, course_outline, graders, include_children_ include_children_predicate=include_children_predicate, parent_xblock=xblock, graders=graders, - user=user + user=user, + course=course, ) for child in xblock.get_children() ] return child_info diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index f6ea5f21a9..a498ac86f8 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -262,6 +262,8 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations {u'name': u'Group A', u'version': 1}, {u'name': u'Group B', u'version': 1}, ], + u'parameters': {}, + u'active': True } response = self.client.ajax_post( self._url(), @@ -283,6 +285,7 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations 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, {}) def test_lazily_creates_cohort_configuration(self): """ @@ -327,6 +330,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio {u'id': 0, u'name': u'Group A', u'version': 1, u'usage': []}, {u'id': 1, u'name': u'Group B', u'version': 1, u'usage': []}, ], + u'parameters': {}, + u'active': True, } response = self.client.put( self._url(cid=666), @@ -346,6 +351,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio 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): """ @@ -364,6 +370,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio {u'id': 0, u'name': u'New Group Name', u'version': 1, u'usage': []}, {u'id': 2, u'name': u'Group C', u'version': 1, u'usage': []}, ], + u'parameters': {}, + u'active': True, } response = self.client.put( @@ -385,6 +393,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio 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): """ @@ -466,6 +475,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio {u'id': 1, u'name': u'Group B', u'version': 1}, ], u'usage': [], + u'parameters': {}, + u'active': True, } response = self.client.put( @@ -485,6 +496,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio 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): """ @@ -504,6 +516,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio {u'id': 2, u'name': u'Group C', u'version': 1}, ], u'usage': [], + u'parameters': {}, + u'active': True, } response = self.client.put( @@ -525,6 +539,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio 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): """ @@ -609,6 +624,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): {'id': 1, 'name': 'Group B', 'version': 1, 'usage': usage_for_group}, {'id': 2, 'name': 'Group C', 'version': 1, 'usage': []}, ], + u'parameters': {}, + u'active': True, } def test_content_group_not_used(self): @@ -701,6 +718,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): {'id': 2, 'name': 'Group C', 'version': 1}, ], 'usage': [], + 'parameters': {}, + 'active': True, }] self.assertEqual(actual, expected) @@ -730,6 +749,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): 'label': 'Test Unit 0 / Test Content Experiment 0', 'validation': None, }], + 'parameters': {}, + 'active': True, }, { 'id': 1, 'name': 'Name 1', @@ -742,6 +763,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): {'id': 2, 'name': 'Group C', 'version': 1}, ], 'usage': [], + 'parameters': {}, + 'active': True, }] self.assertEqual(actual, expected) @@ -772,6 +795,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): 'label': u"Test Unit 0 / Test Content Experiment 0JOSÉ ANDRÉS", 'validation': None, }], + 'parameters': {}, + 'active': True, }] self.assertEqual(actual, expected) @@ -807,6 +832,8 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): 'label': 'Test Unit 1 / Test Content Experiment 1', 'validation': None, }], + 'parameters': {}, + 'active': True, }] self.assertEqual(actual, expected) @@ -829,6 +856,48 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): actual = GroupConfiguration.get_content_experiment_usage_info(self.store, self.course) self.assertEqual(actual, {0: []}) + def test_can_handle_multiple_partitions(self): + # Create the user partitions + self.course.user_partitions = [ + UserPartition( + id=0, + 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"), + ], + ), + UserPartition( + id=1, + 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"), + ], + ), + ] + self.store.update_item(self.course, ModuleStoreEnum.UserID.test) + + # Assign group access rules for multiple partitions, one of which is a cohorted partition + __, problem = self._create_problem_with_content_group(0, 1) + problem.group_access = { + 0: [0], + 1: [1], + } + self.store.update_item(problem, ModuleStoreEnum.UserID.test) + + # This used to cause an exception since the code assumed that + # only one partition would be available. + actual = GroupConfiguration.get_content_groups_usage_info(self.store, self.course) + self.assertEqual(actual.keys(), [0]) + + actual = GroupConfiguration.get_content_groups_items_usage_info(self.store, self.course) + self.assertEqual(actual.keys(), [0]) + class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): """ diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index fec27e64d6..f633e0b170 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -119,9 +119,9 @@ class GetItemTest(ItemTest): return resp @ddt.data( - (1, 16, 14, 15, 11), - (2, 16, 14, 15, 11), - (3, 16, 14, 15, 11), + (1, 17, 15, 16, 12), + (2, 17, 15, 16, 12), + (3, 17, 15, 16, 12), ) @ddt.unpack def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries): @@ -137,9 +137,9 @@ class GetItemTest(ItemTest): self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1])) @ddt.data( - (1, 26), - (2, 28), - (3, 30), + (1, 30), + (2, 32), + (3, 34), ) @ddt.unpack def test_container_get_query_count(self, branching_factor, unit_queries,): @@ -310,6 +310,52 @@ class GetItemTest(ItemTest): content_contains="Couldn't parse paging parameters" ) + def test_get_user_partitions_and_groups(self): + self.course.user_partitions = [ + UserPartition( + id=0, + 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"), + ], + ), + ] + self.store.update_item(self.course, self.user.id) + + # Create an item and retrieve it + resp = self.create_xblock(category='vertical') + usage_key = self.response_usage_key(resp) + resp = self.client.get(reverse_usage_url('xblock_handler', usage_key)) + self.assertEqual(resp.status_code, 200) + + # Check that the partition and group information was returned + result = json.loads(resp.content) + self.assertEqual(result["user_partitions"], [ + { + "id": 0, + "name": "Verification user partition", + "scheme": "verification", + "groups": [ + { + "id": 0, + "name": "Group A", + "selected": False, + "deleted": False, + }, + { + "id": 1, + "name": "Group B", + "selected": False, + "deleted": False, + }, + ] + } + ]) + self.assertEqual(result["group_access"], {}) + @ddt.ddt class DeleteItem(ItemTest): @@ -1414,7 +1460,7 @@ class TestXBlockInfo(ItemTest): @ddt.data( (ModuleStoreEnum.Type.split, 5, 5), - (ModuleStoreEnum.Type.mongo, 4, 6), + (ModuleStoreEnum.Type.mongo, 5, 7), ) @ddt.unpack def test_xblock_outline_handler_mongo_calls(self, store_type, chapter_queries, chapter_queries_1): diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index 26c41dc480..f9eb62ff5c 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -56,6 +56,26 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): self.course.user_partitions = [self.content_partition] self.store.update_item(self.course, self.user.id) + def create_verification_user_partitions(self, checkpoint_names): + """ + Create user partitions for verification checkpoints. + """ + scheme = UserPartition.get_scheme("verification") + self.course.user_partitions = [ + UserPartition( + id=0, + name=checkpoint_name, + description="Verification checkpoint", + scheme=scheme, + groups=[ + Group(scheme.ALLOW, "Completed verification at {}".format(checkpoint_name)), + Group(scheme.DENY, "Did not complete verification at {}".format(checkpoint_name)), + ], + ) + for checkpoint_name in checkpoint_names + ] + self.store.update_item(self.course, self.user.id) + def set_staff_only(self, item_location): """Make an item visible to staff only.""" item = self.store.get_item(item_location) @@ -129,3 +149,14 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): 'Content group no longer exists.' ] ) + + def test_html_verification_checkpoints(self): + self.create_verification_user_partitions(["Midterm A", "Midterm B"]) + self.verify_visibility_view_contains( + self.video_location, + [ + "Verification Checkpoint", + "Midterm A", + "Midterm B", + ] + ) diff --git a/cms/static/js/models/xblock_info.js b/cms/static/js/models/xblock_info.js index 639ae1e2ed..7c7ef81fba 100644 --- a/cms/static/js/models/xblock_info.js +++ b/cms/static/js/models/xblock_info.js @@ -144,8 +144,18 @@ function(Backbone, _, str, ModuleUtils) { /** * Optional explanatory message about the xblock. */ - 'explanatory_message': null - + 'explanatory_message': null, + /** + * The XBlock's group access rules. This is a dictionary keyed to user partition IDs, + * where the values are lists of group IDs. + */ + 'group_access': null, + /** + * User partition dictionary. This is pre-processed by Studio, so it contains + * some additional fields that are not stored in the course descriptor + * (for example, which groups are selected for this particular XBlock). + */ + 'user_partitions': null, }, initialize: function () { @@ -238,7 +248,20 @@ function(Backbone, _, str, ModuleUtils) { */ isEditableOnCourseOutline: function() { return this.isSequential() || this.isChapter() || this.isVertical(); - } + }, + + /* + * Check whether any verification checkpoints are defined in the course. + * Verification checkpoints are defined if there exists a user partition + * that uses the verification partition scheme. + */ + hasVerifiedCheckpoints: function() { + var partitions = this.get("user_partitions") || []; + + return Boolean(_.find(partitions, function(p) { + return p.scheme === "verification"; + })); + } }); return XBlockInfo; }); diff --git a/cms/static/js/views/modals/course_outline_modals.js b/cms/static/js/views/modals/course_outline_modals.js index 2fa152dd7a..5b00400447 100644 --- a/cms/static/js/views/modals/course_outline_modals.js +++ b/cms/static/js/views/modals/course_outline_modals.js @@ -13,7 +13,8 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', ) { 'use strict'; var CourseOutlineXBlockModal, SettingsXBlockModal, PublishXBlockModal, AbstractEditor, BaseDateEditor, - ReleaseDateEditor, DueDateEditor, GradingEditor, PublishEditor, StaffLockEditor, TimedExaminationPreferenceEditor; + ReleaseDateEditor, DueDateEditor, GradingEditor, PublishEditor, StaffLockEditor, + VerificationAccessEditor, TimedExaminationPreferenceEditor; CourseOutlineXBlockModal = BaseModal.extend({ events : { @@ -427,7 +428,7 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', }, hasChanges: function() { - return this.isModelLocked() != this.isLocked(); + return this.isModelLocked() !== this.isLocked(); }, getRequestData: function() { @@ -443,7 +444,110 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', return { hasExplicitStaffLock: this.isModelLocked(), ancestorLocked: this.isAncestorLocked() + }; + } + }); + + VerificationAccessEditor = AbstractEditor.extend({ + templateName: 'verification-access-editor', + className: 'edit-verification-access', + + // This constant MUST match the group ID + // defined by VerificationPartitionScheme on the backend! + ALLOW_GROUP_ID: 1, + + getSelectedPartition: function() { + var hasRestrictions = $("#verification-access-checkbox").is(":checked"), + selectedPartitionID = null; + + if (hasRestrictions) { + selectedPartitionID = $("#verification-partition-select").val(); } + + return parseInt(selectedPartitionID, 10); + }, + + getGroupAccess: function() { + var groupAccess = _.clone(this.model.get('group_access')) || [], + userPartitions = this.model.get('user_partitions') || [], + selectedPartition = this.getSelectedPartition(), + that = this; + + // We display a simplified UI to course authors. + // On the backend, each verification checkpoint is associated + // with a user partition that has two groups. For example, + // if two checkpoints were defined, they might look like: + // + // Midterm A: |-- ALLOW --|-- DENY --| + // Midterm B: |-- ALLOW --|-- DENY --| + // + // To make life easier for course authors, we display + // *one* option for each checkpoint: + // + // [X] Must complete verification checkpoint + // Dropdown: + // * Midterm A + // * Midterm B + // + // This is where we map the simplified UI to + // the underlying user partition. If the user checked + // the box, that means there *is* a restriction, + // so only the "ALLOW" group for the selected partition has access. + // Otherwise, all groups in the partition have access. + // + _.each(userPartitions, function(partition) { + if (partition.scheme === "verification") { + if (selectedPartition === partition.id) { + groupAccess[partition.id] = [that.ALLOW_GROUP_ID]; + } else { + delete groupAccess[partition.id]; + } + } + }); + + return groupAccess; + }, + + getRequestData: function() { + var groupAccess = this.getGroupAccess(), + hasChanges = !_.isEqual(groupAccess, this.model.get('group_access')); + + return hasChanges ? { + publish: 'republish', + metadata: { + group_access: groupAccess, + } + } : {}; + }, + + getContext: function() { + var partitions = this.model.get("user_partitions"), + hasRestrictions = false, + verificationPartitions = [], + isSelected = false; + + // Display a simplified version of verified partition schemes. + // Although there are two groups defined (ALLOW and DENY), + // we show only the ALLOW group. + // To avoid searching all the groups, we're assuming that the editor + // either sets the ALLOW group or doesn't set any groups (implicitly allow all). + _.each(partitions, function(item) { + if (item.scheme === "verification") { + isSelected = _.any(_.pluck(item.groups, "selected")); + hasRestrictions = hasRestrictions || isSelected; + + verificationPartitions.push({ + "id": item.id, + "name": item.name, + "selected": isSelected, + }); + } + }); + + return { + "hasVerificationRestrictions": hasRestrictions, + "verificationPartitions": verificationPartitions, + }; } }); @@ -472,8 +576,13 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', } editors.push(StaffLockEditor); + } else if (xblockInfo.isVertical()) { editors = [StaffLockEditor]; + + if (xblockInfo.hasVerifiedCheckpoints()) { + editors.push(VerificationAccessEditor); + } } return new SettingsXBlockModal($.extend({ editors: editors, diff --git a/cms/static/js/xblock/authoring.js b/cms/static/js/xblock/authoring.js index dfc8e7cc0d..d9abe7d6a6 100644 --- a/cms/static/js/xblock/authoring.js +++ b/cms/static/js/xblock/authoring.js @@ -6,29 +6,56 @@ function VisibilityEditorView(runtime, element) { this.getGroupAccess = function() { - var groupAccess, userPartitionId, selectedGroupIds; + var groupAccess = {}, + checkboxValues, + partitionId, + groupId, + + // This constant MUST match the group ID + // defined by VerificationPartitionScheme on the backend! + ALLOW_GROUP_ID = 1; + if (element.find('.visibility-level-all').prop('checked')) { return {}; } - userPartitionId = element.find('.wrapper-visibility-specific').data('user-partition-id').toString(); - selectedGroupIds = []; + + // Cohort partitions (user is allowed to select more than one) element.find('.field-visibility-content-group input:checked').each(function(index, input) { - selectedGroupIds.push(parseInt($(input).val())); + checkboxValues = $(input).val().split("-"); + partitionId = parseInt(checkboxValues[0], 10); + groupId = parseInt(checkboxValues[1], 10); + + if (groupAccess.hasOwnProperty(partitionId)) { + groupAccess[partitionId].push(groupId); + } else { + groupAccess[partitionId] = [groupId]; + } }); - groupAccess = {}; - groupAccess[userPartitionId] = selectedGroupIds; + + // Verification partitions (user can select exactly one) + if (element.find('#verification-access-checkbox').prop('checked')) { + partitionId = parseInt($('#verification-access-dropdown').val(), 10); + groupAccess[partitionId] = [ALLOW_GROUP_ID]; + } + return groupAccess; }; + // 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').prop('checked', false); + element.find('.field-visibility-content-group input, .field-visibility-verification input') + .prop('checked', false); } }); - element.find('.field-visibility-content-group input').change(function(event) { - element.find('.visibility-level-all').prop('checked', false); - element.find('.visibility-level-specific').prop('checked', true); - }); + + // When selecting a specific group, deselect "all students and staff" and + // select "specific content groups" instead.` + 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-specific').prop('checked', true); + }); } VisibilityEditorView.prototype.collectFieldData = function collectFieldData() { diff --git a/cms/static/sass/elements/_modal-window.scss b/cms/static/sass/elements/_modal-window.scss index e7d06cf9c0..53f6835a20 100644 --- a/cms/static/sass/elements/_modal-window.scss +++ b/cms/static/sass/elements/_modal-window.scss @@ -465,6 +465,15 @@ } } + .field-visibility-verification { + .note { + @extend %t-copy-sub2; + @extend %t-regular; + margin: 14px 0 0 24px; + display: block; + } + } + // CASE: content group has been removed .field-visibility-content-group.was-removed { @@ -629,8 +638,12 @@ } } + .edit-staff-lock { + margin-bottom: $baseline; + } + // UI: staff lock section - .edit-staff-lock, .edit-settings-timed-examination { + .edit-staff-lock, .edit-settings-timed-examination, .edit-verification-access { .checkbox-cosmetic .input-checkbox { @extend %cont-text-sr; @@ -653,6 +666,20 @@ .checkbox-cosmetic .label { margin-bottom: 0; } + + .note { + @extend %t-copy-sub2; + @extend %t-regular; + margin: 14px 0 0 21px; + display: block; + } + } + + .verification-access { + .checkbox-cosmetic .label { + @include float(left); + margin: 2px 6px 0 0; + } } // UI: timed and proctored exam section diff --git a/cms/templates/course_outline.html b/cms/templates/course_outline.html index b8896b0d5e..839689d6ef 100644 --- a/cms/templates/course_outline.html +++ b/cms/templates/course_outline.html @@ -21,7 +21,7 @@ from microsite_configuration import microsite <%block name="header_extras"> -% for template_name in ['course-outline', 'xblock-string-field-editor', 'basic-modal', 'modal-button', 'course-outline-modal', 'due-date-editor', 'release-date-editor', 'grading-editor', 'publish-editor', 'staff-lock-editor', 'timed-examination-preference-editor']: +% for template_name in ['course-outline', 'xblock-string-field-editor', 'basic-modal', 'modal-button', 'course-outline-modal', 'due-date-editor', 'release-date-editor', 'grading-editor', 'publish-editor', 'staff-lock-editor', 'verification-access-editor', 'timed-examination-preference-editor']: diff --git a/cms/templates/js/verification-access-editor.underscore b/cms/templates/js/verification-access-editor.underscore new file mode 100644 index 0000000000..d5dd25468f --- /dev/null +++ b/cms/templates/js/verification-access-editor.underscore @@ -0,0 +1,44 @@ +
diff --git a/cms/templates/visibility_editor.html b/cms/templates/visibility_editor.html index b5b9d9bd22..3dfaa54173 100644 --- a/cms/templates/visibility_editor.html +++ b/cms/templates/visibility_editor.html @@ -1,19 +1,20 @@ <% from django.utils.translation import ugettext as _ +from openedx.core.djangoapps.credit.partition_schemes import VerificationPartitionScheme +from contentstore.utils import ancestor_has_staff_lock, get_visibility_partition_info -from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition -from contentstore.utils import ancestor_has_staff_lock +partition_info = get_visibility_partition_info(xblock) +user_partitions = partition_info["user_partitions"] +cohort_partitions = partition_info["cohort_partitions"] +verification_partitions = partition_info["verification_partitions"] +has_selected_groups = partition_info["has_selected_groups"] +selected_verified_partition_id = partition_info["selected_verified_partition_id"] -cohorted_user_partition = get_cohorted_user_partition(xblock.location.course_key) -unsorted_groups = cohorted_user_partition.groups if cohorted_user_partition else [] -groups = sorted(unsorted_groups, key=lambda group: group.name) -selected_group_ids = xblock.group_access.get(cohorted_user_partition.id, []) if cohorted_user_partition else [] -has_selected_groups = len(selected_group_ids) > 0 is_staff_locked = ancestor_has_staff_lock(xblock) %>