From 17d892c521c74bb71c29ef9ad11d1a9903803b59 Mon Sep 17 00:00:00 2001 From: jsa Date: Mon, 8 Dec 2014 10:11:39 -0500 Subject: [PATCH 01/44] make block.get_parent() work. Co-Authored-By: Christina Roberts Co-Authored-By: Daniel Friedman Co-Authored-By: Don Mitchell --- cms/djangoapps/contentstore/views/item.py | 3 +- .../contentstore/views/tests/test_item.py | 52 ++++++-- .../modulestore/modulestore_settings.py | 4 + .../xmodule/xmodule/modulestore/mongo/base.py | 123 +++++++++++++++--- .../xmodule/modulestore/mongo/draft.py | 3 + .../xmodule/modulestore/tests/factories.py | 31 ++--- .../tests/test_mixed_modulestore.py | 36 ++--- .../xmodule/modulestore/tests/test_mongo.py | 10 +- .../xmodule/modulestore/tests/test_publish.py | 28 ++-- .../xmodule/modulestore/tests/test_xml.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 51 +------- .../xmodule/modulestore/xml_importer.py | 13 +- .../xmodule/xmodule/tests/test_conditional.py | 1 - .../xmodule/tests/test_course_module.py | 2 - .../lib/xmodule/xmodule/tests/test_import.py | 2 - common/lib/xmodule/xmodule/x_module.py | 3 +- .../courseware/tests/test_lti_integration.py | 4 +- .../tests/test_submitting_problems.py | 16 +-- lms/djangoapps/instructor/tests/test_tools.py | 3 +- lms/lib/xblock/test/test_mixin.py | 107 ++++++++++++++- 20 files changed, 333 insertions(+), 161 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index e390811096..bdb5a8d248 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -143,7 +143,8 @@ def xblock_handler(request, usage_key_string): # right now can't combine output of this w/ output of _get_module_info, but worthy goal return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key)) # TODO: pass fields to _get_module_info and only return those - rsp = _get_module_info(_get_xblock(usage_key, request.user)) + with modulestore().bulk_operations(usage_key.course_key): + rsp = _get_module_info(_get_xblock(usage_key, request.user)) return JsonResponse(rsp) else: return HttpResponse(status=406) diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index c322233319..d7f8d165ec 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -116,9 +116,20 @@ class GetItemTest(ItemTest): return resp @ddt.data( - (1, 21, 23, 35, 37), - (2, 22, 24, 38, 39), - (3, 23, 25, 41, 41), + # chapter explanation: + # 1-3. get course, chapter, chapter's children, + # 4-7. chapter's published grandchildren, chapter's draft grandchildren, published & then draft greatgrand + # 8 compute chapter's parent + # 9 get chapter's parent + # 10-16. run queries 2-8 again + # 17-19. compute seq, vert, and problem's parents (odd since it's going down; so, it knows) + # 20-22. get course 3 times + # 23. get chapter + # 24. compute chapter's parent (course) + # 25. compute course's parent (None) + (1, 20, 20, 26, 26), + (2, 21, 21, 29, 28), + (3, 22, 22, 32, 30), ) @ddt.unpack def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries): @@ -411,21 +422,46 @@ class TestDuplicateItem(ItemTest): except for location and display name. """ def duplicate_and_verify(source_usage_key, parent_usage_key): + """ Duplicates the source, parenting to supplied parent. Then does equality check. """ usage_key = self._duplicate_item(parent_usage_key, source_usage_key) - self.assertTrue(check_equality(source_usage_key, usage_key), "Duplicated item differs from original") + self.assertTrue( + check_equality(source_usage_key, usage_key, parent_usage_key), + "Duplicated item differs from original" + ) - def check_equality(source_usage_key, duplicate_usage_key): + def check_equality(source_usage_key, duplicate_usage_key, parent_usage_key=None): + """ + Gets source and duplicated items from the modulestore using supplied usage keys. + Then verifies that they represent equivalent items (modulo parents and other + known things that may differ). + """ original_item = self.get_item_from_modulestore(source_usage_key) duplicated_item = self.get_item_from_modulestore(duplicate_usage_key) self.assertNotEqual( - original_item.location, - duplicated_item.location, + unicode(original_item.location), + unicode(duplicated_item.location), "Location of duplicate should be different from original" ) - # Set the location and display name to be the same so we can make sure the rest of the duplicate is equal. + + # Parent will only be equal for root of duplicated structure, in the case + # where an item is duplicated in-place. + if parent_usage_key and unicode(original_item.parent) == unicode(parent_usage_key): + self.assertEqual( + unicode(parent_usage_key), unicode(duplicated_item.parent), + "Parent of duplicate should equal parent of source for root xblock when duplicated in-place" + ) + else: + self.assertNotEqual( + unicode(original_item.parent), unicode(duplicated_item.parent), + "Parent duplicate should be different from source" + ) + + # Set the location, display name, and parent to be the same so we can make sure the rest of the + # duplicate is equal. duplicated_item.location = original_item.location duplicated_item.display_name = original_item.display_name + duplicated_item.parent = original_item.parent # Children will also be duplicated, so for the purposes of testing equality, we will set # the children to the original after recursively checking the children. diff --git a/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py b/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py index ee1a7fc106..0b29f3ead7 100644 --- a/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py +++ b/common/lib/xmodule/xmodule/modulestore/modulestore_settings.py @@ -98,6 +98,7 @@ def update_module_store_settings( module_store_options=None, xml_store_options=None, default_store=None, + mappings=None, ): """ Updates the settings for each store defined in the given module_store_setting settings @@ -123,6 +124,9 @@ def update_module_store_settings( return raise Exception("Could not find setting for requested default store: {}".format(default_store)) + if mappings and 'mappings' in module_store_setting['default']['OPTIONS']: + module_store_setting['default']['OPTIONS']['mappings'] = mappings + def get_mixed_stores(mixed_setting): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 51227e4b2d..a858fb8d21 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -29,7 +29,7 @@ from contracts import contract, new_contract from importlib import import_module from opaque_keys.edx.keys import UsageKey, CourseKey, AssetKey -from opaque_keys.edx.locations import Location +from opaque_keys.edx.locations import Location, BlockUsageLocator from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.locator import CourseLocator, LibraryLocator @@ -56,6 +56,7 @@ new_contract('CourseKey', CourseKey) new_contract('AssetKey', AssetKey) new_contract('AssetMetadata', AssetMetadata) new_contract('long', long) +new_contract('BlockUsageLocator', BlockUsageLocator) # sort order that returns DRAFT items first SORT_REVISION_FAVOR_DRAFT = ('_id.revision', pymongo.DESCENDING) @@ -93,12 +94,13 @@ class MongoKeyValueStore(InheritanceKeyValueStore): A KeyValueStore that maps keyed data access to one of the 3 data areas known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, data, children, metadata): + def __init__(self, data, parent, children, metadata): super(MongoKeyValueStore, self).__init__() if not isinstance(data, dict): self._data = {'data': data} else: self._data = data + self._parent = parent self._children = children self._metadata = metadata @@ -106,7 +108,7 @@ class MongoKeyValueStore(InheritanceKeyValueStore): if key.scope == Scope.children: return self._children elif key.scope == Scope.parent: - return None + return self._parent elif key.scope == Scope.settings: return self._metadata[key.field_name] elif key.scope == Scope.content: @@ -219,15 +221,35 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): self._convert_reference_to_key(childloc) for childloc in definition.get('children', []) ] + + parent = None + if self.cached_metadata is not None: + # fish the parent out of here if it's available + parent_url = self.cached_metadata.get(unicode(location), {}).get('parent', {}).get( + ModuleStoreEnum.Branch.published_only if location.revision is None + else ModuleStoreEnum.Branch.draft_preferred + ) + if parent_url: + parent = BlockUsageLocator.from_string(parent_url) + if not parent and category != 'course': + # try looking it up just-in-time (but not if we're working with a root node (course). + parent = self.modulestore.get_parent_location( + as_published(location), + ModuleStoreEnum.RevisionOption.published_only if location.revision is None + else ModuleStoreEnum.RevisionOption.draft_preferred + ) + data = definition.get('data', {}) if isinstance(data, basestring): data = {'data': data} + mixed_class = self.mixologist.mix(class_) if data: # empty or None means no work data = self._convert_reference_fields_to_keys(mixed_class, location.course_key, data) metadata = self._convert_reference_fields_to_keys(mixed_class, location.course_key, metadata) kvs = MongoKeyValueStore( data, + parent, children, metadata, ) @@ -439,6 +461,32 @@ class MongoBulkOpsMixin(BulkOperationsMixin): ) +class ParentLocationCache(dict): + """ + Dict-based object augmented with a more cache-like interface, for internal use. + """ + # pylint: disable=missing-docstring + + @contract(key=unicode) + def has(self, key): + return key in self + + @contract(key=unicode, value="BlockUsageLocator | None") + def set(self, key, value): + self[key] = value + + @contract(key=unicode) + def delete(self, key): + if key in self: + del self[key] + + @contract(value="BlockUsageLocator") + def delete_by_value(self, value): + keys_to_delete = [k for k, v in self.iteritems() if v == value] + for key in keys_to_delete: + del self[key] + + class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, MongoBulkOpsMixin): """ A Mongodb backed ModuleStore @@ -572,6 +620,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo return location.replace(revision=MongoRevisionKey.draft) return location.replace(revision=MongoRevisionKey.published) + def _get_parent_cache(self, branch): + """ + Provides a reference to one of the two branch-specific + ParentLocationCaches associated with the current request (if any). + """ + if self.request_cache is not None: + return self.request_cache.data.setdefault('parent-location-{}'.format(branch), ParentLocationCache()) + else: + return ParentLocationCache() + def _compute_metadata_inheritance_tree(self, course_id): ''' TODO (cdodge) This method can be deleted when the 'split module store' work has been completed @@ -640,7 +698,13 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo _compute_inherited_metadata(child) else: # this is likely a leaf node, so let's record what metadata we need to inherit - metadata_to_inherit[child] = my_metadata + metadata_to_inherit[child] = my_metadata.copy() + # WARNING: 'parent' is not part of inherited metadata, but + # we're piggybacking on this recursive traversal to grab + # and cache the child's parent, as a performance optimization. + # The 'parent' key will be popped out of the dictionary during + # CachingDescriptorSystem.load_item + metadata_to_inherit[child].setdefault('parent', {})[self.get_branch_setting()] = url if root is not None: _compute_inherited_metadata(root) @@ -735,12 +799,18 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo data = {} to_process = list(items) course_key = self.fill_in_run(course_key) + parent_cache = self._get_parent_cache(self.get_branch_setting()) + while to_process and depth is None or depth >= 0: children = [] for item in to_process: self._clean_item_data(item) - children.extend(item.get('definition', {}).get('children', [])) - data[Location._from_deprecated_son(item['location'], course_key.run)] = item + item_location = Location._from_deprecated_son(item['location'], course_key.run) + item_children = item.get('definition', {}).get('children', []) + children.extend(item_children) + for item_child in item_children: + parent_cache.set(item_child, item_location) + data[item_location] = item if depth == 0: break @@ -1245,6 +1315,13 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo if xblock.has_children: children = self._serialize_scope(xblock, Scope.children) payload.update({'definition.children': children['children']}) + + # Remove all old pointers to me, then add my current children back + parent_cache = self._get_parent_cache(self.get_branch_setting()) + parent_cache.delete_by_value(xblock.location) + for child in xblock.children: + parent_cache.set(unicode(child), xblock.location) + self._update_single_item(xblock.scope_ids.usage_id, payload, allow_not_found=allow_not_found) # update subtree edited info for ancestors @@ -1339,6 +1416,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo assert revision == ModuleStoreEnum.RevisionOption.published_only \ or revision == ModuleStoreEnum.RevisionOption.draft_preferred + parent_cache = self._get_parent_cache(self.get_branch_setting()) + if parent_cache.has(unicode(location)): + return parent_cache.get(unicode(location)) + # create a query with tag, org, course, and the children field set to the given location query = self._course_key_to_son(location.course_key) query['definition.children'] = unicode(location) @@ -1347,30 +1428,35 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo if revision == ModuleStoreEnum.RevisionOption.published_only: query['_id.revision'] = MongoRevisionKey.published - # query the collection, sorting by DRAFT first - parents = self.collection.find(query, {'_id': True}, sort=[SORT_REVISION_FAVOR_DRAFT]) + def cache_and_return(parent_loc): # pylint:disable=missing-docstring + parent_cache.set(unicode(location), parent_loc) + return parent_loc - if parents.count() == 0: + # query the collection, sorting by DRAFT first + parents = list( + self.collection.find(query, {'_id': True}, sort=[SORT_REVISION_FAVOR_DRAFT]) + ) + if len(parents) == 0: # no parents were found - return None + return cache_and_return(None) if revision == ModuleStoreEnum.RevisionOption.published_only: - if parents.count() > 1: + if len(parents) > 1: non_orphan_parents = self._get_non_orphan_parents(location, parents, revision) if len(non_orphan_parents) == 0: # no actual parent found - return None + return cache_and_return(None) if len(non_orphan_parents) > 1: # should never have multiple PUBLISHED parents raise ReferentialIntegrityError( - u"{} parents claim {}".format(parents.count(), location) + u"{} parents claim {}".format(len(parents), location) ) else: - return non_orphan_parents[0] + return cache_and_return(non_orphan_parents[0].replace(run=location.course_key.run)) else: # return the single PUBLISHED parent - return Location._from_deprecated_son(parents[0]['_id'], location.course_key.run) + return cache_and_return(Location._from_deprecated_son(parents[0]['_id'], location.course_key.run)) else: # there could be 2 different parents if # (1) the draft item was moved or @@ -1386,11 +1472,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo # since we sorted by SORT_REVISION_FAVOR_DRAFT, the 0'th parent is the one we want if published_parents > 1: non_orphan_parents = self._get_non_orphan_parents(location, all_parents, revision) - return non_orphan_parents[0] + return cache_and_return(non_orphan_parents[0].replace(run=location.course_key.run)) found_id = all_parents[0]['_id'] # don't disclose revision outside modulestore - return Location._from_deprecated_son(found_id, location.course_key.run) + return cache_and_return(Location._from_deprecated_son(found_id, location.course_key.run)) def get_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs): ''' @@ -1409,7 +1495,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo ''' parent = self._get_raw_parent_location(location, revision) if parent: - return as_published(parent) + return parent return None def get_modulestore_type(self, course_key=None): @@ -1463,6 +1549,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo """ kvs = MongoKeyValueStore( definition_data, + None, [], metadata, ) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 93a5eac922..d603287b08 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -643,6 +643,9 @@ class DraftModuleStore(MongoModuleStore): Raises: ItemNotFoundError: if any of the draft subtree nodes aren't found + + Returns: + The newly published xblock """ # NOTE: cannot easily use self._breadth_first b/c need to get pub'd and draft as pairs # (could do it by having 2 breadth first scans, the first to just get all published children diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index d0229cc200..fb704299c9 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -210,26 +210,18 @@ class ItemFactory(XModuleFactory): # replace the display name with an optional parameter passed in from the caller if display_name is not None: metadata['display_name'] = display_name - runtime = parent.runtime if parent else None - store.create_item( + + module = store.create_child( user_id, - location.course_key, + parent.location, location.block_type, block_id=location.block_id, metadata=metadata, definition_data=data, - runtime=runtime + runtime=parent.runtime, + fields=kwargs, ) - module = store.get_item(location) - - for attr, val in kwargs.items(): - setattr(module, attr, val) - # Save the attributes we just set - module.save() - - store.update_item(module, user_id) - # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so # if we add one then we need to also add it to the policy information (i.e. metadata) # we should remove this once we can break this reference from the course to static tabs @@ -248,12 +240,15 @@ class ItemFactory(XModuleFactory): parent.children.append(location) store.update_item(parent, user_id) if publish_item: - store.publish(parent.location, user_id) + published_parent = store.publish(parent.location, user_id) + # module is last child of parent + return published_parent.get_children()[-1] + else: + return store.get_item(location) elif publish_item: - store.publish(location, user_id) - - # return the published item - return store.get_item(location) + return store.publish(location, user_id) + else: + return module @contextmanager diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index a8712f4d66..5c0e23ea46 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -358,12 +358,12 @@ class TestMixedModuleStore(CourseComparisonTest): self.store.has_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred) # draft queries: - # problem: find draft item, find all items pertinent to inheritance computation + # problem: find draft item, find all items pertinent to inheritance computation, find parent # non-existent problem: find draft, find published # split: # problem: active_versions, structure # non-existent problem: ditto - @ddt.data(('draft', [2, 2], 0), ('split', [2, 2], 0)) + @ddt.data(('draft', [3, 2], 0), ('split', [2, 2], 0)) @ddt.unpack def test_get_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -388,10 +388,10 @@ class TestMixedModuleStore(CourseComparisonTest): self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred) # Draft: - # wildcard query, 6! load pertinent items for inheritance calls, course root fetch (why) + # wildcard query, 6! load pertinent items for inheritance calls, load parents, course root fetch (why) # Split: # active_versions (with regex), structure, and spurious active_versions refetch - @ddt.data(('draft', 8, 0), ('split', 3, 0)) + @ddt.data(('draft', 14, 0), ('split', 3, 0)) @ddt.unpack def test_get_items(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -405,7 +405,6 @@ class TestMixedModuleStore(CourseComparisonTest): course_locn = self.course_locations[self.MONGO_COURSEID] with check_mongo_calls(max_find, max_send): - # NOTE: use get_course if you just want the course. get_items is expensive modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'}) self.assertEqual(len(modules), 6) @@ -416,12 +415,11 @@ class TestMixedModuleStore(CourseComparisonTest): revision=ModuleStoreEnum.RevisionOption.draft_preferred ) - # draft: get draft, count parents, get parents, count & get grandparents, count & get greatgrand, - # count & get next ancestor (chapter's parent), count non-existent next ancestor, get inheritance + # draft: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: active_versions, definitions (calculator field), structures # 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data(('draft', 11, 5), ('split', 3, 2)) + @ddt.data(('draft', 7, 5), ('split', 3, 2)) @ddt.unpack def test_update_item(self, default_ms, max_find, max_send): """ @@ -886,9 +884,9 @@ class TestMixedModuleStore(CourseComparisonTest): # notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split # still only 2) - # Draft: count via definition.children query, then fetch via that query + # Draft: get_parent # Split: active_versions, structure - @ddt.data(('draft', 2, 0), ('split', 2, 0)) + @ddt.data(('draft', 1, 0), ('split', 2, 0)) @ddt.unpack def test_get_parent_locations(self, default_ms, max_find, max_send): """ @@ -1022,20 +1020,12 @@ class TestMixedModuleStore(CourseComparisonTest): # Draft: # Problem path: # 1. Get problem - # 2-3. count matches definition.children called 2x? - # 4. get parent via definition.children query - # 5-7. 2 counts and 1 get grandparent via definition.children - # 8-10. ditto for great-grandparent - # 11-13. ditto for next ancestor - # 14. fail count query looking for parent of course (unnecessary) - # 15. get course record direct query (not via definition.children) (already fetched in 13) - # 16. get items for inheritance computation - # 17. get vertical (parent of problem) - # 18. get items for inheritance computation (why? caching should handle) - # 19-20. get vertical_x1b (? why? this is the only ref in trace) & items for inheritance computation - # Chapter path: get chapter, count parents 2x, get parents, count non-existent grandparents + # 2-6. get parent and rest of ancestors up to course + # 7-8. get sequential, compute inheritance + # 8-9. get vertical, compute inheritance + # 10-11. get other vertical_x1b (why?) and compute inheritance # Split: active_versions & structure - @ddt.data(('draft', [20, 5], 0), ('split', [2, 2], 0)) + @ddt.data(('draft', [12, 3], 0), ('split', [2, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index a70fb7d9c2..737e0c8b7a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -717,15 +717,16 @@ class TestMongoKeyValueStore(object): def setUp(self): self.data = {'foo': 'foo_value'} self.course_id = SlashSeparatedCourseKey('org', 'course', 'run') + self.parent = self.course_id.make_usage_key('parent', 'p') self.children = [self.course_id.make_usage_key('child', 'a'), self.course_id.make_usage_key('child', 'b')] self.metadata = {'meta': 'meta_val'} - self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata) + self.kvs = MongoKeyValueStore(self.data, self.parent, self.children, self.metadata) def test_read(self): assert_equals(self.data['foo'], self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'foo'))) + assert_equals(self.parent, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent'))) assert_equals(self.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children'))) assert_equals(self.metadata['meta'], self.kvs.get(KeyValueStore.Key(Scope.settings, None, None, 'meta'))) - assert_equals(None, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent'))) def test_read_invalid_scope(self): for scope in (Scope.preferences, Scope.user_info, Scope.user_state): @@ -735,7 +736,7 @@ class TestMongoKeyValueStore(object): assert_false(self.kvs.has(key)) def test_read_non_dict_data(self): - self.kvs = MongoKeyValueStore('xml_data', self.children, self.metadata) + self.kvs = MongoKeyValueStore('xml_data', self.parent, self.children, self.metadata) assert_equals('xml_data', self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'data'))) def _check_write(self, key, value): @@ -746,9 +747,10 @@ class TestMongoKeyValueStore(object): yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'foo'), 'new_data') yield (self._check_write, KeyValueStore.Key(Scope.children, None, None, 'children'), []) yield (self._check_write, KeyValueStore.Key(Scope.settings, None, None, 'meta'), 'new_settings') + # write Scope.parent raises InvalidScope, which is covered in test_write_invalid_scope def test_write_non_dict_data(self): - self.kvs = MongoKeyValueStore('xml_data', self.children, self.metadata) + self.kvs = MongoKeyValueStore('xml_data', self.parent, self.children, self.metadata) self._check_write(KeyValueStore.Key(Scope.content, None, None, 'data'), 'new_data') def test_write_invalid_scope(self): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index b067a8bc5c..0484eb81da 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -47,14 +47,10 @@ class TestPublish(SplitWMongoCourseBoostrapper): # For each (4) item created # - try to find draft # - try to find non-draft - # - retrieve draft of new parent - # - get last error - # - load parent - # - load inheritable data - # - load parent - # - load ancestors + # - compute what is parent + # - load draft parent again & compute its parent chain up to course # count for updates increased to 16 b/c of edit_info updating - with check_mongo_calls(40, 16): + with check_mongo_calls(36, 16): self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False) self._create_item( 'discussion', 'Discussion1', @@ -96,22 +92,22 @@ class TestPublish(SplitWMongoCourseBoostrapper): item = self.draft_mongo.get_item(vert_location, 2) # Finds: # 1 get draft vert, - # 2-10 for each child: (3 children x 3 queries each) - # get draft and then published child + # 2 compute parent + # 3-14 for each child: (3 children x 4 queries each) + # get draft, compute parent, and then published child # compute inheritance - # 11 get published vert - # 12-15 get each ancestor (count then get): (2 x 2), - # 16 then fail count of course parent (1) - # 17 compute inheritance - # 18-19 get draft and published vert + # 15 get published vert + # 16-18 get ancestor chain + # 19 compute inheritance + # 20-22 get draft and published vert, compute parent # Sends: # delete the subtree of drafts (1 call), # update the published version of each node in subtree (4 calls), # update the ancestors up to course (2 calls) if mongo_uses_error_check(self.draft_mongo): - max_find = 20 + max_find = 23 else: - max_find = 19 + max_find = 22 with check_mongo_calls(max_find, 7): self.draft_mongo.publish(item.location, self.user_id) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index 0c518a14d8..c72e91a1e0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -31,7 +31,7 @@ class TestXMLModuleStore(unittest.TestCase): Test around the XML modulestore """ def test_xml_modulestore_type(self): - store = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'simple']) + store = XMLModuleStore(DATA_DIR, course_dirs=[]) self.assertEqual(store.get_modulestore_type(), ModuleStoreEnum.Type.xml) def test_unicode_chars_in_xml_content(self): diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index a836597c00..18c8ea4160 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -53,7 +53,7 @@ def clean_out_mako_templating(xml_string): class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): def __init__(self, xmlstore, course_id, course_dir, - error_tracker, parent_tracker, + error_tracker, load_error_modules=True, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that @@ -209,7 +209,8 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): if descriptor.has_children: for child in descriptor.get_children(): - parent_tracker.add_parent(child.scope_ids.usage_id, descriptor.scope_ids.usage_id) + child.parent = descriptor.location + child.save() # After setting up the descriptor, save any changes that we have # made to attributes on the descriptor to the underlying KeyValueStore. @@ -278,41 +279,6 @@ class CourseLocationManager(OpaqueKeyReader, AsideKeyGenerator): return usage_id -class ParentTracker(object): - """A simple class to factor out the logic for tracking location parent pointers.""" - def __init__(self): - """ - Init - """ - # location -> parent. Not using defaultdict because we care about the empty case. - self._parents = dict() - - def add_parent(self, child, parent): - """ - Add a parent of child location to the set of parents. Duplicate calls have no effect. - - child and parent must be :class:`.Location` instances. - """ - self._parents[child] = parent - - def is_known(self, child): - """ - returns True iff child has some parents. - """ - return child in self._parents - - def make_known(self, location): - """Tell the parent tracker about an object, without registering any - parents for it. Used for the top level course descriptor locations.""" - self._parents.setdefault(location, None) - - def parent(self, child): - """ - Return the parent of this child. If not is_known(child), will throw a KeyError - """ - return self._parents[child] - - class XMLModuleStore(ModuleStoreReadBase): """ An XML backed ModuleStore @@ -352,8 +318,6 @@ class XMLModuleStore(ModuleStoreReadBase): class_ = getattr(import_module(module_path), class_name) self.default_class = class_ - self.parent_trackers = defaultdict(ParentTracker) - # All field data will be stored in an inheriting field data. self.field_data = inheriting_field_data(kvs=DictKeyValueStore()) @@ -400,7 +364,7 @@ class XMLModuleStore(ModuleStoreReadBase): else: self.courses[course_dir] = course_descriptor self._course_errors[course_descriptor.id] = errorlog - self.parent_trackers[course_descriptor.id].make_known(course_descriptor.scope_ids.usage_id) + course_descriptor.parent = None def __unicode__(self): ''' @@ -512,7 +476,6 @@ class XMLModuleStore(ModuleStoreReadBase): course_id=course_id, course_dir=course_dir, error_tracker=tracker, - parent_tracker=self.parent_trackers[course_id], load_error_modules=self.load_error_modules, get_policy=get_policy, mixins=self.xblock_mixins, @@ -756,10 +719,8 @@ class XMLModuleStore(ModuleStoreReadBase): '''Find the location that is the parent of this location in this course. Needed for path_to_location(). ''' - if not self.parent_trackers[location.course_key].is_known(location): - raise ItemNotFoundError("{0} not in {1}".format(location, location.course_key)) - - return self.parent_trackers[location.course_key].parent(location) + block = self.get_item(location, 0) + return block.parent def get_modulestore_type(self, course_key=None): """ diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 4b946e4bb5..e697c61011 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -28,7 +28,7 @@ import json import re from lxml import etree -from .xml import XMLModuleStore, ImportSystem, ParentTracker +from .xml import XMLModuleStore, ImportSystem from xblock.runtime import KvsFieldData, DictKeyValueStore from xmodule.x_module import XModuleDescriptor from opaque_keys.edx.keys import UsageKey @@ -479,11 +479,13 @@ def _import_module_and_update_references( fields = {} for field_name, field in module.fields.iteritems(): - if field.is_set_on(module): - if field.scope == Scope.parent: - continue + if field.scope != Scope.parent and field.is_set_on(module): if isinstance(field, Reference): - fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module)) + value = field.read_from(module) + if value is None: + fields[field_name] = None + else: + fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module)) elif isinstance(field, ReferenceList): references = field.read_from(module) fields[field_name] = [_convert_reference_fields_to_new_namespace(reference) for reference in references] @@ -548,7 +550,6 @@ def _import_course_draft( course_id=source_course_id, course_dir=draft_course_dir, error_tracker=errorlog.tracker, - parent_tracker=ParentTracker(), load_error_modules=False, mixins=xml_module_store.xblock_mixins, field_data=KvsFieldData(kvs=DictKeyValueStore()), diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index effdf2784a..00f8600162 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -30,7 +30,6 @@ class DummySystem(ImportSystem): course_id=SlashSeparatedCourseKey(ORG, COURSE, 'test_run'), course_dir='test_dir', error_tracker=Mock(), - parent_tracker=Mock(), load_error_modules=load_error_modules, ) diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 5083c79845..30313c641b 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -36,14 +36,12 @@ class DummySystem(ImportSystem): course_id = SlashSeparatedCourseKey(ORG, COURSE, 'test_run') course_dir = "test_dir" error_tracker = Mock() - parent_tracker = Mock() super(DummySystem, self).__init__( xmlstore=xmlstore, course_id=course_id, course_dir=course_dir, error_tracker=error_tracker, - parent_tracker=parent_tracker, load_error_modules=load_error_modules, field_data=KvsFieldData(DictKeyValueStore()), ) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 079b73f81c..3a8af1257c 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -39,14 +39,12 @@ class DummySystem(ImportSystem): course_id = SlashSeparatedCourseKey(ORG, COURSE, 'test_run') course_dir = "test_dir" error_tracker = Mock() - parent_tracker = Mock() super(DummySystem, self).__init__( xmlstore=xmlstore, course_id=course_id, course_dir=course_dir, error_tracker=error_tracker, - parent_tracker=parent_tracker, load_error_modules=load_error_modules, mixins=(InheritanceMixin, XModuleMixin), field_data=KvsFieldData(DictKeyValueStore()), diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 770d6429ad..892e0a99ae 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -918,7 +918,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): # =============================== BUILTIN METHODS ========================== def __eq__(self, other): - return (self.scope_ids == other.scope_ids and + return (hasattr(other, 'scope_ids') and + self.scope_ids == other.scope_ids and self.fields.keys() == other.fields.keys() and all(getattr(self, field.name) == getattr(other, field.name) for field in self.fields.values())) diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index 2c7da15823..8c72a183ce 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -163,7 +163,7 @@ class TestLTIModuleListing(ModuleStoreTestCase): parent_location=self.section2.location, display_name="lti draft", category="lti", - location=self.course.id.make_usage_key('lti', 'lti_published'), + location=self.course.id.make_usage_key('lti', 'lti_draft'), publish_item=False, ) @@ -199,7 +199,7 @@ class TestLTIModuleListing(ModuleStoreTestCase): "lti_1_1_result_service_xml_endpoint": self.expected_handler_url('grade_handler'), "lti_2_0_result_service_json_endpoint": self.expected_handler_url('lti_2_0_result_rest_handler') + "/user/{anon_user_id}", - "display_name": self.lti_draft.display_name + "display_name": self.lti_published.display_name, } self.assertEqual([expected], json.loads(response.content)) diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 3a7ddd8939..0953391eb6 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -1159,11 +1159,11 @@ class TestConditionalContent(TestSubmittingProblems): vertical_0, vertical_1 = self.split_setup(user_partition_group) # Group 0 will have 2 problems in the section, worth a total of 4 points. - self.add_dropdown_to_section(vertical_0.location, 'H2P1', 1).location.html_id() - self.add_dropdown_to_section(vertical_0.location, 'H2P2', 3).location.html_id() + self.add_dropdown_to_section(vertical_0.location, 'H2P1_GROUP0', 1).location.html_id() + self.add_dropdown_to_section(vertical_0.location, 'H2P2_GROUP0', 3).location.html_id() # Group 1 will have 1 problem in the section, worth a total of 1 point. - self.add_dropdown_to_section(vertical_1.location, 'H2P1', 1).location.html_id() + self.add_dropdown_to_section(vertical_1.location, 'H2P1_GROUP1', 1).location.html_id() # Submit answers for problem in Section 1, which is visible to all students. self.submit_question_answer('H1P1', {'2_1': 'Correct', '2_2': 'Incorrect'}) @@ -1175,8 +1175,8 @@ class TestConditionalContent(TestSubmittingProblems): """ self.split_different_problems_setup(self.user_partition_group_0) - self.submit_question_answer('H2P1', {'2_1': 'Correct'}) - self.submit_question_answer('H2P2', {'2_1': 'Correct', '2_2': 'Incorrect', '2_3': 'Correct'}) + self.submit_question_answer('H2P1_GROUP0', {'2_1': 'Correct'}) + self.submit_question_answer('H2P2_GROUP0', {'2_1': 'Correct', '2_2': 'Incorrect', '2_3': 'Correct'}) self.assertEqual(self.score_for_hw('homework1'), [1.0]) self.assertEqual(self.score_for_hw('homework2'), [1.0, 2.0]) @@ -1194,7 +1194,7 @@ class TestConditionalContent(TestSubmittingProblems): """ self.split_different_problems_setup(self.user_partition_group_1) - self.submit_question_answer('H2P1', {'2_1': 'Correct'}) + self.submit_question_answer('H2P1_GROUP1', {'2_1': 'Correct'}) self.assertEqual(self.score_for_hw('homework1'), [1.0]) self.assertEqual(self.score_for_hw('homework2'), [1.0]) @@ -1219,7 +1219,7 @@ class TestConditionalContent(TestSubmittingProblems): [_, vertical_1] = self.split_setup(user_partition_group) # Group 1 will have 1 problem in the section, worth a total of 1 point. - self.add_dropdown_to_section(vertical_1.location, 'H2P1', 1).location.html_id() + self.add_dropdown_to_section(vertical_1.location, 'H2P1_GROUP1', 1).location.html_id() self.submit_question_answer('H1P1', {'2_1': 'Correct'}) @@ -1244,7 +1244,7 @@ class TestConditionalContent(TestSubmittingProblems): """ self.split_one_group_no_problems_setup(self.user_partition_group_1) - self.submit_question_answer('H2P1', {'2_1': 'Correct'}) + self.submit_question_answer('H2P1_GROUP1', {'2_1': 'Correct'}) self.assertEqual(self.score_for_hw('homework1'), [1.0]) self.assertEqual(self.score_for_hw('homework2'), [1.0]) diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 6c6c96e88b..a879592374 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -119,7 +119,8 @@ class TestFindUnit(ModuleStoreTestCase): Test finding a nested unit. """ url = self.homework.location.to_deprecated_string() - self.assertEqual(tools.find_unit(self.course, url), self.homework) + found_unit = tools.find_unit(self.course, url) + self.assertEqual(found_unit.location, self.homework.location) def test_find_unit_notfound(self): """ diff --git a/lms/lib/xblock/test/test_mixin.py b/lms/lib/xblock/test/test_mixin.py index d97666baeb..329404c4aa 100644 --- a/lms/lib/xblock/test/test_mixin.py +++ b/lms/lib/xblock/test/test_mixin.py @@ -1,8 +1,12 @@ """ Tests of the LMS XBlock Mixin """ +import ddt +from django.conf import settings from xblock.validation import ValidationMessage +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.modulestore_settings import update_module_store_settings from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.partitions.partitions import Group, UserPartition @@ -13,9 +17,11 @@ class LmsXBlockMixinTestCase(ModuleStoreTestCase): Base class for XBlock mixin tests cases. A simple course with a single user partition is created in setUp for all subclasses to use. """ - - def setUp(self): - super(LmsXBlockMixinTestCase, self).setUp() + def build_course(self): + """ + Build up a course tree with a UserPartition. + """ + # pylint: disable=attribute-defined-outside-init self.user_partition = UserPartition( 0, 'first_partition', @@ -31,13 +37,16 @@ class LmsXBlockMixinTestCase(ModuleStoreTestCase): self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section') self.subsection = ItemFactory.create(parent=self.section, category='sequential', display_name='Test Subsection') self.vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='Test Unit') - self.video = ItemFactory.create(parent=self.subsection, category='video', display_name='Test Video') + self.video = ItemFactory.create(parent=self.vertical, category='video', display_name='Test Video 1') class XBlockValidationTest(LmsXBlockMixinTestCase): """ Unit tests for XBlock validation """ + def setUp(self): + super(XBlockValidationTest, self).setUp() + self.build_course() def verify_validation_message(self, message, expected_message, expected_message_type): """ @@ -92,6 +101,9 @@ class XBlockGroupAccessTest(LmsXBlockMixinTestCase): """ Unit tests for XBlock group access. """ + def setUp(self): + super(XBlockGroupAccessTest, self).setUp() + self.build_course() def test_is_visible_to_group(self): """ @@ -143,3 +155,90 @@ class OpenAssessmentBlockMixinTestCase(ModuleStoreTestCase): Test has_score is true for ora2 problems. """ self.assertTrue(self.open_assessment.has_score) + + +@ddt.ddt +class XBlockGetParentTest(LmsXBlockMixinTestCase): + """ + Test that XBlock.get_parent returns correct results with each modulestore + backend. + """ + def _pre_setup(self): + # load the one xml course into the xml store + update_module_store_settings( + settings.MODULESTORE, + mappings={'edX/toy/2012_Fall': ModuleStoreEnum.Type.xml}, + xml_store_options={ + 'data_dir': settings.COMMON_TEST_DATA_ROOT # where toy course lives + }, + ) + super(XBlockGetParentTest, self)._pre_setup() + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.xml) + def test_parents(self, modulestore_type): + with self.store.default_store(modulestore_type): + + # setting up our own local course tree here, since it needs to be + # created with the correct modulestore type. + + if modulestore_type == 'xml': + course_key = self.store.make_course_key('edX', 'toy', '2012_Fall') + else: + course_key = self.create_toy_course('edX', 'toy', '2012_Fall_copy') + course = self.store.get_course(course_key) + + self.assertIsNone(course.get_parent()) + + def recurse(parent): + """ + Descend the course tree and ensure the result of get_parent() + is the expected one. + """ + visited = [] + for child in parent.get_children(): + self.assertEqual(parent.location, child.get_parent().location) + visited.append(child) + visited += recurse(child) + return visited + + visited = recurse(course) + self.assertEqual(len(visited), 28) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_parents_draft_content(self, modulestore_type): + # move the video to the new vertical + with self.store.default_store(modulestore_type): + self.build_course() + new_vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='New Test Unit') + child_to_move_location = self.video.location.for_branch(None) + new_parent_location = new_vertical.location.for_branch(None) + old_parent_location = self.vertical.location.for_branch(None) + + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + self.assertIsNone(self.course.get_parent()) + + with self.store.bulk_operations(self.course.id): + user_id = ModuleStoreEnum.UserID.test + + old_parent = self.store.get_item(old_parent_location) + old_parent.children.remove(child_to_move_location) + self.store.update_item(old_parent, user_id) + + new_parent = self.store.get_item(new_parent_location) + new_parent.children.append(child_to_move_location) + self.store.update_item(new_parent, user_id) + + # re-fetch video from draft store + video = self.store.get_item(child_to_move_location) + + self.assertEqual( + new_parent_location, + video.get_parent().location + ) + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only): + # re-fetch video from published store + video = self.store.get_item(child_to_move_location) + self.assertEqual( + old_parent_location, + video.get_parent().location.for_branch(None) + ) From 6fb3daa772433565fe7011077de3c7274e98b77c Mon Sep 17 00:00:00 2001 From: jsa Date: Mon, 15 Dec 2014 08:36:05 -0500 Subject: [PATCH 02/44] support group_access. JIRA: TNL-649 --- .../xmodule/modulestore/inheritance.py | 9 - .../xmodule/xmodule/modulestore/mongo/base.py | 5 - .../xmodule/xmodule/partitions/partitions.py | 27 +- .../partitions/tests/test_partitions.py | 21 +- lms/djangoapps/courseware/access.py | 72 ++++ .../courseware/tests/test_access.py | 4 +- .../courseware/tests/test_group_access.py | 384 ++++++++++++++++++ lms/djangoapps/lms_xblock/mixin.py | 97 +++-- lms/lib/xblock/test/test_mixin.py | 139 +++++++ .../course_groups/partition_scheme.py | 12 +- .../djangoapps/user_api/partition_schemes.py | 23 +- 11 files changed, 736 insertions(+), 57 deletions(-) create mode 100644 lms/djangoapps/courseware/tests/test_group_access.py diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index ad3c5eb2c0..b8cd344510 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -57,15 +57,6 @@ class InheritanceMixin(XBlockMixin): default=False, scope=Scope.settings, ) - group_access = Dict( - help="A dictionary that maps which groups can be shown this block. The keys " - "are group configuration ids and the values are a list of group IDs. " - "If there is no key for a group configuration or if the list of group IDs " - "is empty then the block is considered visible to all. Note that this " - "field is ignored if the block is visible_to_staff_only.", - default={}, - scope=Scope.settings, - ) course_edit_method = String( display_name=_("Course Editor"), help=_("Enter the method by which this course is edited (\"XML\" or \"Studio\")."), diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index a858fb8d21..15022f6bec 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -475,11 +475,6 @@ class ParentLocationCache(dict): def set(self, key, value): self[key] = value - @contract(key=unicode) - def delete(self, key): - if key in self: - del self[key] - @contract(value="BlockUsageLocator") def delete_by_value(self, value): keys_to_delete = [k for k, v in self.iteritems() if v == value] diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index 715bbb0338..79425b9505 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -10,7 +10,21 @@ from stevedore.extension import ExtensionManager class UserPartitionError(Exception): """ - An error was found regarding user partitions. + Base Exception for when an error was found regarding user partitions. + """ + pass + + +class NoSuchUserPartitionError(UserPartitionError): + """ + Exception to be raised when looking up a UserPartition by its ID fails. + """ + pass + + +class NoSuchUserPartitionGroupError(UserPartitionError): + """ + Exception to be raised when looking up a UserPartition Group by its ID fails. """ pass @@ -171,9 +185,14 @@ class UserPartition(namedtuple("UserPartition", "id name description groups sche def get_group(self, group_id): """ - Returns the group with the specified id. + Returns the group with the specified id. Raises NoSuchUserPartitionGroupError if not found. """ - for group in self.groups: # pylint: disable=no-member + # pylint: disable=no-member + + for group in self.groups: if group.id == group_id: return group - return None + + raise NoSuchUserPartitionGroupError( + "could not find a Group with ID [{}] in UserPartition [{}]".format(group_id, self.id) + ) diff --git a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py index f52f968ca9..7bf79caa80 100644 --- a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py +++ b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py @@ -8,7 +8,9 @@ from mock import Mock from opaque_keys.edx.locations import SlashSeparatedCourseKey from stevedore.extension import Extension, ExtensionManager -from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError, USER_PARTITION_SCHEME_NAMESPACE +from xmodule.partitions.partitions import ( + Group, UserPartition, UserPartitionError, NoSuchUserPartitionGroupError, USER_PARTITION_SCHEME_NAMESPACE +) from xmodule.partitions.partitions_service import PartitionService @@ -259,6 +261,23 @@ class TestUserPartition(PartitionTestCase): user_partition = UserPartition.from_json(jsonified) self.assertNotIn("programmer", user_partition.to_json()) + def test_get_group(self): + """ + UserPartition.get_group correctly returns the group referenced by the + `group_id` parameter, or raises NoSuchUserPartitionGroupError when + the lookup fails. + """ + self.assertEqual( + self.user_partition.get_group(self.TEST_GROUPS[0].id), # pylint: disable=no-member + self.TEST_GROUPS[0] + ) + self.assertEqual( + self.user_partition.get_group(self.TEST_GROUPS[1].id), # pylint: disable=no-member + self.TEST_GROUPS[1] + ) + with self.assertRaises(NoSuchUserPartitionGroupError): + self.user_partition.get_group(3) + class StaticPartitionService(PartitionService): """ diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index fe4159fefe..8ed87b2ce0 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -15,6 +15,7 @@ from xmodule.error_module import ErrorDescriptor from xmodule.x_module import XModule from xblock.core import XBlock +from xmodule.partitions.partitions import NoSuchUserPartitionError, NoSuchUserPartitionGroupError from external_auth.models import ExternalAuthMap from courseware.masquerade import is_masquerading_as_student @@ -301,6 +302,71 @@ def _has_access_error_desc(user, action, descriptor, course_key): return _dispatch(checkers, action, user, descriptor) +def _has_group_access(descriptor, user, course_key): + """ + This function returns a boolean indicating whether or not `user` has + sufficient group memberships to "load" a block (the `descriptor`) + """ + if len(descriptor.user_partitions) == 0: + # short circuit the process, since there are no defined user partitions + return True + + # use merged_group_access which takes group access on the block's + # parents / ancestors into account + merged_access = descriptor.merged_group_access + # check for False in merged_access, which indicates that at least one + # partition's group list excludes all students. + if False in merged_access.values(): + log.warning("Group access check excludes all students, access will be denied.", exc_info=True) + return False + + # resolve the partition IDs in group_access to actual + # partition objects, skipping those which contain empty group directives. + # if a referenced partition could not be found, access will be denied. + try: + partitions = [ + descriptor._get_user_partition(partition_id) # pylint:disable=protected-access + for partition_id, group_ids in merged_access.items() + if group_ids is not None + ] + except NoSuchUserPartitionError: + log.warning("Error looking up user partition, access will be denied.", exc_info=True) + return False + + # next resolve the group IDs specified within each partition + partition_groups = [] + try: + for partition in partitions: + groups = [ + partition.get_group(group_id) + for group_id in merged_access[partition.id] + ] + if groups: + partition_groups.append((partition, groups)) + except NoSuchUserPartitionGroupError: + log.warning("Error looking up referenced user partition group, access will be denied.", exc_info=True) + return False + + # look up the user's group for each partition + user_groups = {} + for partition, groups in partition_groups: + user_groups[partition.id] = partition.scheme.get_group_for_user( + course_key, + user, + partition, + ) + + # finally: check that the user has a satisfactory group assignment + # for each partition. + if not all( + user_groups.get(partition.id) in groups for partition, groups in partition_groups + ): + return False + + # all checks passed. + return True + + def _has_access_descriptor(user, action, descriptor, course_key=None): """ Check if user has access to this descriptor. @@ -323,6 +389,12 @@ def _has_access_descriptor(user, action, descriptor, course_key=None): if descriptor.visible_to_staff_only and not _has_staff_access_to_descriptor(user, descriptor, course_key): return False + # enforce group access + if not _has_group_access(descriptor, user, course_key): + # if group_access check failed, deny access unless the requestor is staff, + # in which case immediately grant access. + return _has_staff_access_to_descriptor(user, descriptor, course_key) + # If start dates are off, can always load if settings.FEATURES['DISABLE_START_DATES'] and not is_masquerading_as_student(user): debug("Allow: DISABLE_START_DATES") diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 59a9a5b86b..105171c71b 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -105,7 +105,7 @@ class AccessTestCase(TestCase): def test__has_access_descriptor(self): # TODO: override DISABLE_START_DATES and test the start date branch of the method user = Mock() - descriptor = Mock() + descriptor = Mock(user_partitions=[]) # Always returns true because DISABLE_START_DATES is set in test.py self.assertTrue(access._has_access_descriptor(user, 'load', descriptor)) @@ -118,7 +118,7 @@ class AccessTestCase(TestCase): """ Tests that "visible_to_staff_only" overrides start date. """ - mock_unit = Mock() + mock_unit = Mock(user_partitions=[]) mock_unit._class_tags = {} # Needed for detached check in _has_access_descriptor def verify_access(student_should_have_access): diff --git a/lms/djangoapps/courseware/tests/test_group_access.py b/lms/djangoapps/courseware/tests/test_group_access.py new file mode 100644 index 0000000000..4962a3137a --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_group_access.py @@ -0,0 +1,384 @@ +""" +This module defines tests for courseware.access that are specific to group +access control rules. +""" + +import ddt +from stevedore.extension import Extension, ExtensionManager + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.partitions.partitions import Group, UserPartition, USER_PARTITION_SCHEME_NAMESPACE +from xmodule.modulestore.django import modulestore + +import courseware.access as access +from courseware.tests.factories import StaffFactory, UserFactory + + +class MemoryUserPartitionScheme(object): + """ + In-memory partition scheme for testing. + """ + name = "memory" + + def __init__(self): + self.current_group = {} + + def set_group_for_user(self, user, user_partition, group): + """ + Link this user to this group in this partition, in memory. + """ + self.current_group.setdefault(user.id, {})[user_partition.id] = group + + def get_group_for_user(self, course_id, user, user_partition, track_function=None): # pylint: disable=unused-argument + """ + Fetch the group to which this user is linked in this partition, or None. + """ + return self.current_group.get(user.id, {}).get(user_partition.id) + + +def resolve_attrs(test_method): + """ + Helper function used with ddt. It allows passing strings to test methods + via @ddt.data, which are the names of instance attributes on `self`, and + replaces them with the resolved values of those attributes in the method + call. + """ + def _wrapper(self, *args): # pylint: disable=missing-docstring + new_args = [getattr(self, arg) for arg in args] + return test_method(self, *new_args) + return _wrapper + + +@ddt.ddt +class GroupAccessTestCase(ModuleStoreTestCase): + """ + Tests to ensure that has_access() correctly enforces the visibility + restrictions specified in the `group_access` field of XBlocks. + """ + # pylint: disable=no-member + + def set_user_group(self, user, partition, group): + """ + Internal DRY / shorthand. + """ + partition.scheme.set_group_for_user(user, partition, group) + + def set_group_access(self, block, access_dict): + """ + DRY helper. + """ + block.group_access = access_dict + modulestore().update_item(block, 1) + + def setUp(self): + + UserPartition.scheme_extensions = ExtensionManager.make_test_instance( + [ + Extension( + "memory", + USER_PARTITION_SCHEME_NAMESPACE, + MemoryUserPartitionScheme(), + None + ), + ], + namespace=USER_PARTITION_SCHEME_NAMESPACE + ) + + self.cat_group = Group(10, 'cats') + self.dog_group = Group(20, 'dogs') + self.worm_group = Group(30, 'worms') + self.animal_partition = UserPartition( + 0, + 'Pet Partition', + 'which animal are you?', + [self.cat_group, self.dog_group, self.worm_group], + scheme=UserPartition.get_scheme("memory"), + ) + + self.red_group = Group(1000, 'red') + self.blue_group = Group(2000, 'blue') + self.gray_group = Group(3000, 'gray') + self.color_partition = UserPartition( + 100, + 'Color Partition', + 'what color are you?', + [self.red_group, self.blue_group, self.gray_group], + scheme=UserPartition.get_scheme("memory"), + ) + + self.course = CourseFactory.create( + user_partitions=[self.animal_partition, self.color_partition], + ) + self.chapter = ItemFactory.create(category='chapter', parent=self.course) + self.section = ItemFactory.create(category='sequential', parent=self.chapter) + self.vertical = ItemFactory.create(category='vertical', parent=self.section) + self.component = ItemFactory.create(category='problem', parent=self.vertical) + + self.red_cat = UserFactory() # student in red and cat groups + self.set_user_group(self.red_cat, self.animal_partition, self.cat_group) + self.set_user_group(self.red_cat, self.color_partition, self.red_group) + + self.blue_dog = UserFactory() # student in blue and dog groups + self.set_user_group(self.blue_dog, self.animal_partition, self.dog_group) + self.set_user_group(self.blue_dog, self.color_partition, self.blue_group) + + self.white_mouse = UserFactory() # student in no group + + self.gray_worm = UserFactory() # student in deleted group + self.set_user_group(self.gray_worm, self.animal_partition, self.worm_group) + self.set_user_group(self.gray_worm, self.color_partition, self.gray_group) + # delete the gray/worm groups from the partitions now so we can test scenarios + # for user whose group is missing. + self.animal_partition.groups.pop() + self.color_partition.groups.pop() + + # add a staff user, whose access will be unconditional in spite of group access. + self.staff = StaffFactory.create(course_key=self.course.id) + + # avoid repeatedly declaring the same sequence for ddt in all the test cases. + PARENT_CHILD_PAIRS = ( + ('chapter', 'chapter'), + ('chapter', 'section'), + ('chapter', 'vertical'), + ('chapter', 'component'), + ('section', 'section'), + ('section', 'vertical'), + ('section', 'component'), + ('vertical', 'vertical'), + ('vertical', 'component'), + ) + + def tearDown(self): + """ + Clear out the stevedore extension points on UserPartition to avoid + side-effects in other tests. + """ + UserPartition.scheme_extensions = None + + def check_access(self, user, block, is_accessible): + """ + DRY helper. + """ + self.assertIs( + access.has_access(user, 'load', block, self.course.id), + is_accessible + ) + + def ensure_staff_access(self, block): + """ + Another DRY helper. + """ + self.assertTrue(access.has_access(self.staff, 'load', block, self.course.id)) + + # NOTE: in all the tests that follow, `block_specified` and + # `block_accessed` designate the place where group_access rules are + # specified, and where access is being checked in the test, respectively. + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_partition_single_group(self, block_specified, block_accessed): + """ + Access checks are correctly enforced on the block when a single group + is specified for a single partition. + """ + self.set_group_access( + block_specified, + {self.animal_partition.id: [self.cat_group.id]}, + ) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_partition_two_groups(self, block_specified, block_accessed): + """ + Access checks are correctly enforced on the block when multiple groups + are specified for a single partition. + """ + self.set_group_access( + block_specified, + {self.animal_partition.id: [self.cat_group.id, self.dog_group.id]}, + ) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_partition_disjoint_groups(self, block_specified, block_accessed): + """ + When the parent's and child's group specifications do not intersect, + access is denied to the child regardless of the user's groups. + """ + if block_specified == block_accessed: + # this test isn't valid unless block_accessed is a descendant of + # block_specified. + return + + self.set_group_access( + block_specified, + {self.animal_partition.id: [self.dog_group.id]}, + ) + self.set_group_access( + block_accessed, + {self.animal_partition.id: [self.cat_group.id]}, + ) + self.check_access(self.red_cat, block_accessed, False) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_empty_partition(self, block_specified, block_accessed): + """ + No group access checks are enforced on the block when group_access + declares a partition but does not specify any groups. + """ + self.set_group_access(block_specified, {self.animal_partition.id: []}) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, True) + self.check_access(self.gray_worm, block_accessed, True) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_empty_dict(self, block_specified, block_accessed): + """ + No group access checks are enforced on the block when group_access is an + empty dictionary. + """ + self.set_group_access(block_specified, {}) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, True) + self.check_access(self.gray_worm, block_accessed, True) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_none(self, block_specified, block_accessed): + """ + No group access checks are enforced on the block when group_access is None. + """ + self.set_group_access(block_specified, None) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, True) + self.check_access(self.gray_worm, block_accessed, True) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_partition_group_none(self, block_specified, block_accessed): + """ + No group access checks are enforced on the block when group_access + specifies a partition but its value is None. + """ + self.set_group_access(block_specified, {self.animal_partition.id: None}) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, True) + self.check_access(self.gray_worm, block_accessed, True) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_single_partition_group_empty_list(self, block_specified, block_accessed): + """ + No group access checks are enforced on the block when group_access + specifies a partition but its value is an empty list. + """ + self.set_group_access(block_specified, {self.animal_partition.id: []}) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, True) + self.check_access(self.white_mouse, block_accessed, True) + self.check_access(self.gray_worm, block_accessed, True) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_nonexistent_nonempty_partition(self, block_specified, block_accessed): + """ + Access will be denied to the block when group_access specifies a + nonempty partition that does not exist in course.user_partitions. + """ + self.set_group_access(block_specified, {9: [99]}) + self.check_access(self.red_cat, block_accessed, False) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_has_access_nonexistent_group(self, block_specified, block_accessed): + """ + Access will be denied to the block when group_access contains a group + id that does not exist in its referenced partition. + """ + self.set_group_access(block_specified, {self.animal_partition.id: [99]}) + self.check_access(self.red_cat, block_accessed, False) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_multiple_partitions(self, block_specified, block_accessed): + """ + Group access restrictions are correctly enforced when multiple partition + / group rules are defined. + """ + self.set_group_access( + block_specified, + { + self.animal_partition.id: [self.cat_group.id], + self.color_partition.id: [self.red_group.id], + }, + ) + self.check_access(self.red_cat, block_accessed, True) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.white_mouse, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + @resolve_attrs + def test_multiple_partitions_deny_access(self, block_specified, block_accessed): + """ + Group access restrictions correctly deny access even when some (but not + all) group_access rules are satisfied. + """ + self.set_group_access( + block_specified, + { + self.animal_partition.id: [self.cat_group.id], + self.color_partition.id: [self.blue_group.id], + }, + ) + self.check_access(self.red_cat, block_accessed, False) + self.check_access(self.blue_dog, block_accessed, False) + self.check_access(self.gray_worm, block_accessed, False) + self.ensure_staff_access(block_accessed) diff --git a/lms/djangoapps/lms_xblock/mixin.py b/lms/djangoapps/lms_xblock/mixin.py index a3055d0437..d7a6433986 100644 --- a/lms/djangoapps/lms_xblock/mixin.py +++ b/lms/djangoapps/lms_xblock/mixin.py @@ -1,14 +1,28 @@ """ Namespace that defines fields common to all blocks used in the LMS """ +from lazy import lazy + from xblock.fields import Boolean, Scope, String, XBlockMixin, Dict from xblock.validation import ValidationMessage from xmodule.modulestore.inheritance import UserPartitionList +from xmodule.partitions.partitions import NoSuchUserPartitionError, NoSuchUserPartitionGroupError # Make '_' a no-op so we can scrape strings _ = lambda text: text +class GroupAccessDict(Dict): + """Special Dict class for serializing the group_access field""" + def from_json(self, access_dict): + if access_dict is not None: + return {int(k): access_dict[k] for k in access_dict} + + def to_json(self, access_dict): + if access_dict is not None: + return {unicode(k): access_dict[k] for k in access_dict} + + class LmsBlockMixin(XBlockMixin): """ Mixin that defines fields common to all blocks used in the LMS @@ -55,16 +69,56 @@ class LmsBlockMixin(XBlockMixin): default=False, scope=Scope.settings, ) - group_access = Dict( - help="A dictionary that maps which groups can be shown this block. The keys " - "are group configuration ids and the values are a list of group IDs. " - "If there is no key for a group configuration or if the list of group IDs " - "is empty then the block is considered visible to all. Note that this " - "field is ignored if the block is visible_to_staff_only.", + + group_access = GroupAccessDict( + help=_( + "A dictionary that maps which groups can be shown this block. The keys " + "are group configuration ids and the values are a list of group IDs. " + "If there is no key for a group configuration or if the set of group IDs " + "is empty then the block is considered visible to all. Note that this " + "field is ignored if the block is visible_to_staff_only." + ), default={}, scope=Scope.settings, ) + @lazy + def merged_group_access(self): + """ + This computes access to a block's group_access rules in the context of its position + within the courseware structure, in the form of a lazily-computed attribute. + Each block's group_access rule is merged recursively with its parent's, guaranteeing + that any rule in a parent block will be enforced on descendants, even if a descendant + also defined its own access rules. The return value is always a dict, with the same + structure as that of the group_access field. + + When merging access rules results in a case where all groups are denied access in a + user partition (which effectively denies access to that block for all students), + the special value False will be returned for that user partition key. + """ + parent = self.get_parent() + if not parent: + return self.group_access or {} + + merged_access = parent.merged_group_access.copy() + if self.group_access is not None: + for partition_id, group_ids in self.group_access.items(): + if group_ids: # skip if the "local" group_access for this partition is None or empty. + if partition_id in merged_access: + if merged_access[partition_id] is False: + # special case - means somewhere up the hierarchy, merged access rules have eliminated + # all group_ids from this partition, so there's no possible intersection. + continue + # otherwise, if the parent defines group access rules for this partition, + # intersect with the local ones. + merged_access[partition_id] = list( + set(merged_access[partition_id]).intersection(group_ids) + ) or False + else: + # add the group access rules for this partition to the merged set of rules. + merged_access[partition_id] = group_ids + return merged_access + # Specified here so we can see what the value set at the course-level is. user_partitions = UserPartitionList( help=_("The list of group configurations for partitioning students in content experiments."), @@ -74,29 +128,14 @@ class LmsBlockMixin(XBlockMixin): def _get_user_partition(self, user_partition_id): """ - Returns the user partition with the specified id, or None if there is no such partition. + Returns the user partition with the specified id. Raises + `NoSuchUserPartitionError` if the lookup fails. """ for user_partition in self.user_partitions: if user_partition.id == user_partition_id: return user_partition - return None - - def is_visible_to_group(self, user_partition, group): - """ - Returns true if this xblock should be shown to a user in the specified user partition group. - This method returns true if one of the following is true: - - the xblock has no group_access dictionary specified - - if the dictionary has no key for the user partition's id - - if the value for the user partition's id is an empty list - - if the value for the user partition's id contains the specified group's id - """ - if not self.group_access: - return True - group_ids = self.group_access.get(user_partition.id, []) - if len(group_ids) == 0: - return True - return group.id in group_ids + raise NoSuchUserPartitionError("could not find a UserPartition with ID [{}]".format(user_partition_id)) def validate(self): """ @@ -105,8 +144,9 @@ class LmsBlockMixin(XBlockMixin): _ = self.runtime.service(self, "i18n").ugettext # pylint: disable=redefined-outer-name validation = super(LmsBlockMixin, self).validate() for user_partition_id, group_ids in self.group_access.iteritems(): - user_partition = self._get_user_partition(user_partition_id) - if not user_partition: + try: + user_partition = self._get_user_partition(user_partition_id) + except NoSuchUserPartitionError: validation.add( ValidationMessage( ValidationMessage.ERROR, @@ -115,8 +155,9 @@ class LmsBlockMixin(XBlockMixin): ) else: for group_id in group_ids: - group = user_partition.get_group(group_id) - if not group: + try: + user_partition.get_group(group_id) + except NoSuchUserPartitionGroupError: validation.add( ValidationMessage( ValidationMessage.ERROR, diff --git a/lms/lib/xblock/test/test_mixin.py b/lms/lib/xblock/test/test_mixin.py index 329404c4aa..642aa3d3c8 100644 --- a/lms/lib/xblock/test/test_mixin.py +++ b/lms/lib/xblock/test/test_mixin.py @@ -242,3 +242,142 @@ class XBlockGetParentTest(LmsXBlockMixinTestCase): old_parent_location, video.get_parent().location.for_branch(None) ) + + +class RenamedTuple(tuple): # pylint: disable=incomplete-protocol + """ + This class is only used to allow overriding __name__ on the tuples passed + through ddt, in order to have the generated test names make sense. + """ + pass + + +def ddt_named(parent, child): + """ + Helper to get more readable dynamically-generated test names from ddt. + """ + args = RenamedTuple([parent, child]) + setattr(args, '__name__', 'parent_{}_child_{}'.format(parent, child)) + return args + + +@ddt.ddt +class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): + """ + Test that XBlock.merged_group_access is computed correctly according to + our access control rules. + """ + + PARTITION_1 = 1 + PARTITION_1_GROUP_1 = 11 + PARTITION_1_GROUP_2 = 12 + + PARTITION_2 = 2 + PARTITION_2_GROUP_1 = 21 + PARTITION_2_GROUP_2 = 22 + + PARENT_CHILD_PAIRS = ( + ddt_named('section', 'subsection'), + ddt_named('section', 'vertical'), + ddt_named('section', 'video'), + ddt_named('subsection', 'vertical'), + ddt_named('subsection', 'video'), + ) + + def setUp(self): + super(XBlockMergedGroupAccessTest, self).setUp() + self.build_course() + + def set_group_access(self, block, access_dict): + """ + DRY helper. + """ + block.group_access = access_dict + block.runtime.modulestore.update_item(block, 1) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + def test_intersecting_groups(self, parent, child): + """ + When merging group_access on a block, the resulting group IDs for each + partition is the intersection of the group IDs defined for that + partition across all ancestor blocks (including this one). + """ + parent_block = getattr(self, parent) + child_block = getattr(self, child) + + self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}) + self.set_group_access(child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) + + self.assertEqual( + parent_block.merged_group_access, + {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}, + ) + self.assertEqual( + child_block.merged_group_access, + {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}, + ) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + def test_disjoint_groups(self, parent, child): + """ + When merging group_access on a block, if the intersection of group IDs + for a partition is empty, the merged value for that partition is False. + """ + parent_block = getattr(self, parent) + child_block = getattr(self, child) + + self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.set_group_access(child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) + + self.assertEqual( + parent_block.merged_group_access, + {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}, + ) + self.assertEqual( + child_block.merged_group_access, + {self.PARTITION_1: False}, + ) + + def test_disjoint_groups_no_override(self): + """ + Special case of the above test - ensures that `False` propagates down + to the block being queried even if blocks further down in the hierarchy + try to override it. + """ + self.set_group_access(self.section, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.set_group_access(self.subsection, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) + self.set_group_access(self.vertical, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}) + + self.assertEqual( + self.vertical.merged_group_access, + {self.PARTITION_1: False}, + ) + self.assertEqual( + self.video.merged_group_access, + {self.PARTITION_1: False}, + ) + + @ddt.data(*PARENT_CHILD_PAIRS) + @ddt.unpack + def test_union_partitions(self, parent, child): + """ + When merging group_access on a block, the result's keys (partitions) + are the union of all partitions specified across all ancestor blocks + (including this one). + """ + parent_block = getattr(self, parent) + child_block = getattr(self, child) + + self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.set_group_access(child_block, {self.PARTITION_2: [self.PARTITION_1_GROUP_2]}) + + self.assertEqual( + parent_block.merged_group_access, + {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}, + ) + self.assertEqual( + child_block.merged_group_access, + {self.PARTITION_1: [self.PARTITION_1_GROUP_1], self.PARTITION_2: [self.PARTITION_1_GROUP_2]}, + ) diff --git a/openedx/core/djangoapps/course_groups/partition_scheme.py b/openedx/core/djangoapps/course_groups/partition_scheme.py index cc36c14373..5ab30fc5d1 100644 --- a/openedx/core/djangoapps/course_groups/partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/partition_scheme.py @@ -3,6 +3,8 @@ Provides a UserPartition driver for cohorts. """ import logging +from xmodule.partitions.partitions import NoSuchUserPartitionGroupError + from .cohorts import get_cohort, get_partition_group_id_for_cohort log = logging.getLogger(__name__) @@ -56,8 +58,9 @@ class CohortPartitionScheme(object): # fail silently return None - group = user_partition.get_group(group_id) - if group is None: + try: + return user_partition.get_group(group_id) + except NoSuchUserPartitionGroupError: # if we have a match but the group doesn't exist in the partition, # it means the mapping is invalid. the previous state of the # partition configuration may have been modified. @@ -67,9 +70,8 @@ class CohortPartitionScheme(object): "requested_partition_id": user_partition.id, "requested_group_id": group_id, "cohort_id": cohort.id, - } + }, + exc_info=True ) # fail silently return None - - return group diff --git a/openedx/core/djangoapps/user_api/partition_schemes.py b/openedx/core/djangoapps/user_api/partition_schemes.py index a664500b98..0e30f195bd 100644 --- a/openedx/core/djangoapps/user_api/partition_schemes.py +++ b/openedx/core/djangoapps/user_api/partition_schemes.py @@ -1,11 +1,13 @@ """ Provides partition support to the user service. """ - +import logging import random import api.course_tag as course_tag_api -from xmodule.partitions.partitions import UserPartitionError +from xmodule.partitions.partitions import UserPartitionError, NoSuchUserPartitionGroupError + +log = logging.getLogger(__name__) class RandomUserPartitionScheme(object): @@ -22,7 +24,22 @@ class RandomUserPartitionScheme(object): """ partition_key = cls._key_for_partition(user_partition) group_id = course_tag_api.get_course_tag(user, course_id, partition_key) - group = user_partition.get_group(int(group_id)) if not group_id is None else None + + group = None + if group_id is not None: + # attempt to look up the presently assigned group + try: + group = user_partition.get_group(int(group_id)) + except NoSuchUserPartitionGroupError: + # jsa: we can turn off warnings here if this is an expected case. + log.warn( + "group not found in RandomUserPartitionScheme: %r", + { + "requested_partition_id": user_partition.id, + "requested_group_id": group_id, + }, + exc_info=True + ) if group is None and assign: if not user_partition.groups: From 55d2f1d547bcb5fa7b56cffa16dd0a13ad43517c Mon Sep 17 00:00:00 2001 From: jsa Date: Tue, 16 Dec 2014 12:17:22 -0500 Subject: [PATCH 03/44] test that students are auto-assigned to cohorts in partition scheme --- .../tests/test_partition_scheme.py | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py index 6e0f2e1a4b..43817268d7 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py @@ -16,13 +16,13 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from ..partition_scheme import CohortPartitionScheme from ..models import CourseUserGroupPartitionGroup -from ..cohorts import add_user_to_cohort +from ..cohorts import add_user_to_cohort, get_course_cohorts from .helpers import CohortFactory, config_course_cohorts TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT TEST_MAPPING = {'edX/toy/2012_Fall': 'xml'} -TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING) +TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING, include_xml=True) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -37,7 +37,8 @@ class TestCohortPartitionScheme(django.test.TestCase): and a student for each test. """ self.course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") - config_course_cohorts(modulestore().get_course(self.course_key), [], cohorted=True) + self.course = modulestore().get_course(self.course_key) + config_course_cohorts(self.course, [], cohorted=True) self.groups = [Group(10, 'Group 10'), Group(20, 'Group 20')] self.user_partition = UserPartition( @@ -152,6 +153,30 @@ class TestCohortPartitionScheme(django.test.TestCase): # scheme should now return nothing self.assert_student_in_group(None) + def test_student_lazily_assigned(self): + """ + Test that the lazy assignment of students to cohorts works + properly when accessed via the CohortPartitionScheme. + """ + # don't assign the student to any cohort initially + self.assert_student_in_group(None) + + # get the default cohort, which is automatically created + # during the `get_course_cohorts` API call if it doesn't yet exist + cohort = get_course_cohorts(self.course)[0] + + # map that cohort to a group in our partition + self.link_cohort_partition_group( + cohort, + self.user_partition, + self.groups[0], + ) + + # The student will be lazily assigned to the default cohort + # when CohortPartitionScheme.get_group_for_user makes its internal + # call to cohorts.get_cohort. + self.assert_student_in_group(self.groups[0]) + def setup_student_in_group_0(self): """ Utility to set up a cohort, add our student to the cohort, and link From c4e8673db007e7327938c7956a300e87c2bbbefb Mon Sep 17 00:00:00 2001 From: Marco Morales Date: Wed, 12 Nov 2014 11:06:27 -0500 Subject: [PATCH 04/44] creation of a courseware preview menu to contain staff view toggle. --- lms/static/sass/base/_base.scss | 3 ++ lms/static/sass/base/_variables.scss | 6 ++- lms/static/sass/course-rtl.scss.mako | 1 + lms/static/sass/course.scss.mako | 1 + lms/static/sass/course/base/_base.scss | 10 +++- lms/static/sass/course/base/_extends.scss | 11 ++++ .../course/layout/_courseware_header.scss | 22 ++++---- .../course/layout/_courseware_preview.scss | 27 ++++++++++ .../course/layout/_courseware_subnav.scss | 52 ------------------- lms/static/sass/ie.scss | 2 +- .../courseware/course_navigation.html | 30 ++++++++++- 11 files changed, 94 insertions(+), 71 deletions(-) create mode 100644 lms/static/sass/course/layout/_courseware_preview.scss delete mode 100644 lms/static/sass/course/layout/_courseware_subnav.scss diff --git a/lms/static/sass/base/_base.scss b/lms/static/sass/base/_base.scss index 6d279a8d47..d9ca9b74db 100644 --- a/lms/static/sass/base/_base.scss +++ b/lms/static/sass/base/_base.scss @@ -1,3 +1,6 @@ +// lms - base +// ==================== + // html { // overflow-y: scroll; // } diff --git a/lms/static/sass/base/_variables.scss b/lms/static/sass/base/_variables.scss index 2d73b00afc..2e2f407a8b 100644 --- a/lms/static/sass/base/_variables.scss +++ b/lms/static/sass/base/_variables.scss @@ -360,12 +360,14 @@ $dashboard-course-cover-border: $light-gray; // MISC: course assets $content-wrapper-bg: $white; -$course-bg-color: #d6d6d6; -$course-bg-image: url(../images/bg-texture.png); +$course-bg-color: #f2f2f2; $account-content-wrapper-bg: shade($body-bg, 2%); $course-profile-bg: rgb(245,245,245); $course-header-bg: rgba(255,255,255, 0.93); +// MISC: course background texture +//$course-bg-image: url(../images/bg-texture.png); + // MISC: borders $border-color-1: rgb(190,190,190); $border-color-2: rgb(200,200,200); diff --git a/lms/static/sass/course-rtl.scss.mako b/lms/static/sass/course-rtl.scss.mako index e9d4d2930a..fdef27cbe2 100644 --- a/lms/static/sass/course-rtl.scss.mako +++ b/lms/static/sass/course-rtl.scss.mako @@ -32,6 +32,7 @@ // course - base @import 'course/layout/courseware_header'; +@import 'course/layout/courseware_preview'; @import 'course/layout/footer'; @import 'course/base/mixins'; @import 'course/base/base'; diff --git a/lms/static/sass/course.scss.mako b/lms/static/sass/course.scss.mako index 0fc53f0adf..7fb09e2677 100644 --- a/lms/static/sass/course.scss.mako +++ b/lms/static/sass/course.scss.mako @@ -31,6 +31,7 @@ // course - base @import 'course/layout/courseware_header'; +@import 'course/layout/courseware_preview'; @import 'course/layout/footer'; @import 'course/base/mixins'; @import 'course/base/base'; diff --git a/lms/static/sass/course/base/_base.scss b/lms/static/sass/course/base/_base.scss index cf8e9c4793..7b02e4f2a7 100644 --- a/lms/static/sass/course/base/_base.scss +++ b/lms/static/sass/course/base/_base.scss @@ -1,8 +1,13 @@ +// lms - course - base +// ==================== + body { min-width: 980px; min-height: 100%; - background-image: $course-bg-image; background-color: $course-bg-color; + + //for background texture: + //background-image: $course-bg-image; } body, h1, h2, h3, h4, h5, h6, p, p a:link, p a:visited, a, label { @@ -27,7 +32,7 @@ a { } .container { - padding: 20px 0 0 0; + padding: 0; > div { display: table; @@ -53,6 +58,7 @@ form.choicegroup { } } + textarea, input[type="text"], input[type="email"], diff --git a/lms/static/sass/course/base/_extends.scss b/lms/static/sass/course/base/_extends.scss index c6652f94d6..cac78034be 100644 --- a/lms/static/sass/course/base/_extends.scss +++ b/lms/static/sass/course/base/_extends.scss @@ -1,3 +1,14 @@ +// lms - course - extends +// ==================== + +// lms inner wrapper +%inner-wrapper { + margin: 0 auto; + max-width: 1180px; + width: flex-grid(12); +} + +// Older Course Extends - to review/remove with LMS cleanup h1.top-header { border-bottom: 1px solid $border-color-2; @include text-align(left); diff --git a/lms/static/sass/course/layout/_courseware_header.scss b/lms/static/sass/course/layout/_courseware_header.scss index 4b5fcc3e67..3e1e686802 100644 --- a/lms/static/sass/course/layout/_courseware_header.scss +++ b/lms/static/sass/course/layout/_courseware_header.scss @@ -1,22 +1,20 @@ -nav.course-material { +.wrapper-course-material { @include clearfix(); @include box-sizing(border-box); border-bottom: none; - margin: 0px auto 0px; - padding: 0px; + margin: 0 auto 0; + padding: 0; width: 100%; - .inner-wrapper { - margin: 0 auto; - max-width: 1200px; - width: flex-grid(12); + .course-material{ + @extend %inner-wrapper; } ol.course-tabs { @include border-top-radius(4px); @include clearfix(); - padding: 28px 0 10px 0; @include margin-left(10px); + padding: ($baseline*0.75) 0 ($baseline*0.75) 0; li { @include float(left); @@ -30,7 +28,7 @@ nav.course-material { } &.prominent + li { - padding-left: 15px; + padding-left: ($baseline*0.75); border-left: 1px solid #333; } @@ -39,7 +37,7 @@ nav.course-material { color: #555; display: block; text-align: center; - padding: 10px 13px 12px; + padding: ($baseline/2) 13px 12px; font-size: 14px; font-weight: bold; text-decoration: none; @@ -74,7 +72,7 @@ nav.course-material { header.global.slim { box-shadow: 0 1px 2px $shadow-l1; height: auto; - padding: 5px 0 10px 0; + padding: ($baseline/4) 0 ($baseline/2) 0; border-bottom: 1px solid $outer-border-color; background: $header-bg; @@ -119,7 +117,7 @@ header.global.slim { } h1.logo { - margin: 0 10px 0 13px; + margin: 0 ($baseline/2) 0 0; @include padding-right(20px); &:before { diff --git a/lms/static/sass/course/layout/_courseware_preview.scss b/lms/static/sass/course/layout/_courseware_preview.scss new file mode 100644 index 0000000000..1b7cc6c531 --- /dev/null +++ b/lms/static/sass/course/layout/_courseware_preview.scss @@ -0,0 +1,27 @@ +.wrapper-preview-menu { + @include clearfix(); + @include box-sizing(border-box); + margin: 0 auto 0; + padding: ($baseline*0.75); + width: 100%; + background-color: $gray-l3; + + .preview-menu { + @extend %inner-wrapper; + } + + .preview-actions { + display: inline-block; + + .action-preview { + display: inline-block; + + .action-preview-label { + display: inline-block; + margin-right: ($baseline/2); + margin-bottom: 0; + vertical-align: middle; + } + } + } +} diff --git a/lms/static/sass/course/layout/_courseware_subnav.scss b/lms/static/sass/course/layout/_courseware_subnav.scss deleted file mode 100644 index 38e088d66a..0000000000 --- a/lms/static/sass/course/layout/_courseware_subnav.scss +++ /dev/null @@ -1,52 +0,0 @@ -nav.course-material { - @include clearfix(); - @include box-sizing(border-box); - background: none; - margin: 0px auto 0px; - padding: 0px; - width: 100%; - - .inner-wrapper { - margin: 0 auto; - max-width: 1200px; - width: flex-grid(12); - } - - ol.course-tabs { - @include border-top-radius(4px); - @include clearfix(); - padding: 10px 0 0 0; - - li { - @include float(left); - list-style: none; - - a { - color: darken($lighter-base-font-color, 20%); - display: block; - text-align: center; - padding: 8px 13px 12px; - font-size: 14px; - font-weight: 400; - text-decoration: none; - text-shadow: 0 1px rgb(255,255,255); - - &:hover, &:focus { - color: $base-font-color; - } - - &.active { - color: $blue; - } - } - } - } -} - -.course-content { - margin-top: ($baseline*1.5); - - .courseware { - min-height: 300px; - } -} diff --git a/lms/static/sass/ie.scss b/lms/static/sass/ie.scss index 3654d3957d..b4acb8dfa1 100644 --- a/lms/static/sass/ie.scss +++ b/lms/static/sass/ie.scss @@ -139,7 +139,7 @@ } // active navigation - nav.course-material ol.course-tabs li a.active, nav.course-material .xmodule_SequenceModule nav.sequence-nav ol.course-tabs li a.seq_video.active, .xmodule_SequenceModule nav.sequence-nav nav.course-material ol.course-tabs li a.seq_video.active { + nav.wrapper-course-material ol.course-tabs li a.active, nav.wrapper-course-material .xmodule_SequenceModule nav.sequence-nav ol.course-tabs li a.seq_video.active, .xmodule_SequenceModule nav.sequence-nav nav.wrapper-course-material ol.course-tabs li a.seq_video.active { background-color: #333; background-color: rgba(0, 0, 0, .4); } diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index 248ef0f45d..c60e6673b5 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -23,9 +23,35 @@ def url_class(is_active): user_is_enrolled = user.is_authenticated() and CourseEnrollment.is_enrolled(user, course.id) %> +% if show_preview_menu: + +% endif + % if disable_tabs is UNDEFINED or not disable_tabs: - From f58e96a2fad171ffc2f87b9e0c06baa8c49f8632 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Fri, 9 Jan 2015 15:27:17 -0500 Subject: [PATCH 22/44] Fix quality failures --- .../xmodule/modulestore/tests/test_mixed_modulestore.py | 7 ++++--- lms/djangoapps/courseware/access.py | 4 +--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index b0ec1dc447..4c4a4696df 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -946,19 +946,20 @@ class TestMixedModuleStore(CourseComparisonTest): self.assertEqual(new_parent_location, self.store.get_item(child_to_move_location).get_parent().location) with self.store.branch_setting(ModuleStoreEnum.Branch.published_only): self.assertEqual(old_parent_location, self.store.get_item(child_to_move_location).get_parent().location) - + old_parent_published_location = old_parent_location.for_branch(ModuleStoreEnum.BranchName.published) self.verify_get_parent_locations_results([ (child_to_move_location, new_parent_location, None), (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_move_location, old_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only), + (child_to_move_location, old_parent_published_location, ModuleStoreEnum.RevisionOption.published_only), ]) # publish the course again self.store.publish(self.course.location, self.user_id) + new_parent_published_location = new_parent_location.for_branch(ModuleStoreEnum.BranchName.published) self.verify_get_parent_locations_results([ (child_to_move_location, new_parent_location, None), (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_move_location, new_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only), + (child_to_move_location, new_parent_published_location, ModuleStoreEnum.RevisionOption.published_only), ]) @ddt.data('draft') diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index e0fac92929..ca204b3868 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -358,9 +358,7 @@ def _has_group_access(descriptor, user, course_key): # finally: check that the user has a satisfactory group assignment # for each partition. - if not all( - user_groups.get(partition.id) in groups for partition, groups in partition_groups - ): + if not all(user_groups.get(partition.id) in groups for partition, groups in partition_groups): return False # all checks passed. From d556cdd586f829aad9e67050200217125951bbf2 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Fri, 9 Jan 2015 11:41:16 -0500 Subject: [PATCH 23/44] syncing icon class syntax + semantically hiding changed icons from assistive tech TNL-1137 --- cms/templates/js/publish-xblock.underscore | 6 ++-- cms/templates/studio_xblock_wrapper.html | 2 +- .../ux/reference/modal_access-component.html | 4 +-- cms/templates/visibility_editor.html | 13 ++++++-- .../discussion/test_cohort_management.py | 5 +-- .../js/spec/groups/views/cohorts_spec.js | 11 ++++--- .../instructor_dashboard/ecommerce_spec.js | 8 ++--- .../cohort-editor.underscore | 2 +- .../cohort-form.underscore | 33 +++++++++++++++++-- 9 files changed, 61 insertions(+), 23 deletions(-) diff --git a/cms/templates/js/publish-xblock.underscore b/cms/templates/js/publish-xblock.underscore index 3af6f4c258..521cfc31a2 100644 --- a/cms/templates/js/publish-xblock.underscore +++ b/cms/templates/js/publish-xblock.underscore @@ -79,7 +79,7 @@ var visibleToStaffOnly = visibilityState === 'staff_only'; <% } %> <% if (hasContentGroupComponents) { %>

- + <%= gettext("Some content in this unit is only visible to particular groups") %>

<% } %> @@ -87,9 +87,9 @@ var visibleToStaffOnly = visibilityState === 'staff_only';
  • <% if (hasExplicitStaffLock) { %> - + <% } else { %> - + <% } %> <%= gettext('Hide from students') %> diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 3c0b34288e..11beba2c9a 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -83,7 +83,7 @@ messages = json.dumps(xblock.validate().to_json())
  • - + ${_("Visibility")}
    • diff --git a/cms/templates/ux/reference/modal_access-component.html b/cms/templates/ux/reference/modal_access-component.html index 321cc4d83a..4d5ea570c6 100644 --- a/cms/templates/ux/reference/modal_access-component.html +++ b/cms/templates/ux/reference/modal_access-component.html @@ -13,8 +13,8 @@
    - -

    This component is contained in a unit that is hidden from students. Component visibility settings are overridden by the unit visibility settings.

    + +

    Warning: This component is contained in a unit that is hidden from students. Component visibility settings are overridden by the unit visibility settings.

    diff --git a/cms/templates/visibility_editor.html b/cms/templates/visibility_editor.html index 0db7f74b31..1eebb5b37b 100644 --- a/cms/templates/visibility_editor.html +++ b/cms/templates/visibility_editor.html @@ -27,8 +27,17 @@ is_staff_locked = ancestor_has_staff_lock(xblock)
    % elif is_staff_locked:
    - -

    ${_('The Unit this component is contained in is hidden from students. Visibility settings here will be trumped by this.')}

    + +

    + ## Translators: Any text between {screen_reader_start} and {screen_reader_end} is only read by screen readers and never shown in the browser. + ${_( + "{screen_reader_start}Warning:{screen_reader_end} The Unit this component is contained in is hidden from students. Visibility settings here will be trumped by this." + ).format( + screen_reader_start='', + screen_reader_end='', + ) + } +

    % endif diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 7c12492e94..5390bac698 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -112,7 +112,8 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.cohort_management_page.select_cohort(self.manual_cohort_name) self.assertIsNone(self.cohort_management_page.get_cohort_associated_content_group()) self.assertEqual( - "No content groups exist. Create a content group to associate with cohort groups. Create a content group", + "Warning:\nNo content groups exist. " + "Create a content group to associate with cohort groups. Create a content group", self.cohort_management_page.get_cohort_related_content_group_message() ) self.assertFalse(self.cohort_management_page.select_content_group_radio_button()) @@ -616,7 +617,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): self.cohort_management_page.get_all_content_groups() ) self.assertEqual( - "The previously selected content group was deleted. Select another content group.", + "Warning:\nThe previously selected content group was deleted. Select another content group.", self.cohort_management_page.get_cohort_related_content_group_message() ) self.cohort_management_page.set_cohort_associated_content_group("Pears") diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index b9987099a2..652c42316e 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -646,7 +646,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.tab-settings a').click(); expect(cohortsView.$('option.option-unavailable').text().trim()).toBe('Deleted Content Group'); expect(cohortsView.$('.copy-error').text().trim()).toBe( - 'The previously selected content group was deleted. Select another content group.' + 'Warning: The previously selected content group was deleted. Select another content group.' ); }); @@ -687,13 +687,14 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe }); it("shows an error message when no content groups are specified", function () { + var message; createCohortsView(this, {selectCohort: 1, contentGroups: []}); cohortsView.$('.tab-settings a').click(); expect(cohortsView.$('.radio-yes').prop('disabled')).toBeTruthy(); - expect(cohortsView.$('.msg-inline').text().trim()).toBe( - 'No content groups exist. Create a content group to associate with cohort groups. ' + - 'Create a content group' - ); + message = cohortsView.$('.msg-inline').text().trim(); + expect(message).toContain('Warning: No content groups exist.'); + expect(message).toContain('Create a content group to associate with cohort groups.'); + expect(message).toContain('Create a content group'); expect( cohortsView.$('.msg-inline a').attr('href'), MOCK_STUDIO_GROUP_CONFIGURATIONS_URL diff --git a/lms/static/js/spec/instructor_dashboard/ecommerce_spec.js b/lms/static/js/spec/instructor_dashboard/ecommerce_spec.js index 38c1163b6f..273fbd6f53 100644 --- a/lms/static/js/spec/instructor_dashboard/ecommerce_spec.js +++ b/lms/static/js/spec/instructor_dashboard/ecommerce_spec.js @@ -1,10 +1,10 @@ -define(['backbone', 'jquery', 'js/instructor_dashboard/ecommerce', 'js/common_helpers/template_helpers'], - function (Backbone, $, ExpiryCouponView, TemplateHelpers) { +define(['backbone', 'jquery', 'js/instructor_dashboard/ecommerce'], + function (Backbone, $, ExpiryCouponView) { 'use strict'; - var expiryCouponView, createExpiryCoupon; + var expiryCouponView; describe("edx.instructor_dashboard.ecommerce.ExpiryCouponView", function() { beforeEach(function() { - setFixtures('
    • ') + setFixtures('
    • '); expiryCouponView = new ExpiryCouponView(); }); diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore index 65f2a68a66..0090a87f79 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore @@ -61,7 +61,7 @@
    diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore index edfe6622c1..960def3ac2 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -71,14 +71,41 @@ <% if (hasSelectedContentGroup && !foundSelected) { %>
    -

    <%- gettext("The previously selected content group was deleted. Select another content group.") %>

    +

    + + <%= + interpolate( + // Translators: Any text between %(screen_reader_start)s and %(screen_reader_end)s is only read by screen readers and never shown in the browser. + '%(screen_reader_start)sWarning:%(screen_reader_end)s The previously selected content group was deleted. Select another content group.', + { + screen_reader_start: '', + screen_reader_end: '' + }, + true + ) + %> +

    <% } %> <% } else { // no content groups available %>
    -

    <%- gettext("No content groups exist. Create a content group to associate with cohort groups.") %> <%- gettext("Create a content group") %>

    +

    + + <%= + interpolate( + // Translators: Any text between %(screen_reader_start)s and %(screen_reader_end)s is only read by screen readers and never shown in the browser. + '%(screen_reader_start)sWarning:%(screen_reader_end)s No content groups exist. Create a content group to associate with cohort groups.', + { + screen_reader_start: '', + screen_reader_end: '' + }, + true + ) + %> + <%- gettext("Create a content group") %> +

    <% } %> @@ -89,7 +116,7 @@
    <% if (isNewCohort) { %> From 8928f85fbf05824a9996b6ae0c0e4a0cf8c9e656 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Sun, 11 Jan 2015 12:32:16 -0500 Subject: [PATCH 24/44] Cohorted Courseware: resolving missing icon from add group button in 'nohorts' scenario --- .../instructor/instructor_dashboard_2/notification.underscore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/instructor/instructor_dashboard_2/notification.underscore b/lms/templates/instructor/instructor_dashboard_2/notification.underscore index 48b9b59be4..1d1db6ac2e 100644 --- a/lms/templates/instructor/instructor_dashboard_2/notification.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/notification.underscore @@ -22,7 +22,7 @@
    From e2f7f343bd10b2294a6a472c6ba650f82f267d51 Mon Sep 17 00:00:00 2001 From: Chris Rodriguez Date: Mon, 12 Jan 2015 09:23:53 -0500 Subject: [PATCH 25/44] Fixing padding issues --- .../sass/course/instructor/_instructor_2.scss | 14 +++++++++++++- .../instructor_dashboard_2/cohort-form.underscore | 7 +++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index 2407dac461..2fa3016600 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -559,7 +559,11 @@ } .form-actions { - @include padding($baseline 0 $baseline 0); + padding: $baseline 0; + + &.new-cohort-form { + padding: $baseline; + } } } @@ -1097,6 +1101,14 @@ .tab-content { @include padding($baseline $baseline 0 $baseline); border: 1px solid $gray-l5; + + &.new-cohort-form { + padding: 0 $baseline; + + .form-label { + margin-top: $baseline; + } + } } } diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore index 960def3ac2..0fbb8d251f 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -3,8 +3,8 @@
    <% if (isNewCohort) { %>

    <%- gettext('Add a New Cohort Group') %>

    +
    <% } %> -
    <% // Don't allow renaming of existing cohorts yet as it doesn't interact well with @@ -113,8 +113,11 @@
    + <% if (isNewCohort) { %> +
    + <% } %> -
    +
    <% } %> diff --git a/common/static/js/src/string_utils.js b/common/static/js/src/string_utils.js index 232b36f02e..dc48dcb01d 100644 --- a/common/static/js/src/string_utils.js +++ b/common/static/js/src/string_utils.js @@ -32,8 +32,8 @@ * Example usages: * interpolate_text('{title} ({count})', {title: expectedTitle, count: expectedCount} * interpolate_text( - * ngettext("{numUsersAdded} student has been added to this cohort group", - * "{numUsersAdded} students have been added to this cohort group", numUsersAdded), + * ngettext("{numUsersAdded} student has been added to this cohort", + * "{numUsersAdded} students have been added to this cohort", numUsersAdded), * {numUsersAdded: numUsersAdded} * ); * diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index e709118720..d12e6a6328 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -104,7 +104,7 @@ class MembershipPageCohortManagementSection(PageObject): def _get_cohort_options(self): """ - Returns the available options in the cohort dropdown, including the initial "Select a cohort group". + Returns the available options in the cohort dropdown, including the initial "Select a cohort". """ return self.q(css=self._bounded_selector("#cohort-select option")) @@ -122,7 +122,7 @@ class MembershipPageCohortManagementSection(PageObject): def get_cohorts(self): """ - Returns, as a list, the names of the available cohorts in the drop-down, filtering out "Select a cohort group". + Returns, as a list, the names of the available cohorts in the drop-down, filtering out "Select a cohort". """ return [ self._cohort_name(opt.text) @@ -227,7 +227,7 @@ class MembershipPageCohortManagementSection(PageObject): def set_cohort_associated_content_group(self, content_group=None, select_settings=True): """ Sets the content group associated with the cohort currently being edited. - If content_group is None, un-links the cohort group from any content group. + If content_group is None, un-links the cohort from any content group. Presses Save to update the cohort's settings. """ if select_settings: diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index 41f83624d9..46cdba1155 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/common/test/acceptance/tests/discussion/helpers.py @@ -41,7 +41,7 @@ class CohortTestMixin(object): def setup_cohort_config(self, course_fixture, auto_cohort_groups=None): """ Sets up the course to use cohorting with the given list of auto_cohort_groups. - If auto_cohort_groups is None, no auto cohort groups are set. + If auto_cohort_groups is None, no auto cohorts are set. """ course_fixture._update_xblock(course_fixture._course_location, { "metadata": { @@ -67,7 +67,7 @@ class CohortTestMixin(object): def add_manual_cohort(self, course_fixture, cohort_name): """ - Adds a cohort group by name, returning the ID for the group. + Adds a cohort by name, returning its ID. """ url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/' data = json.dumps({"name": cohort_name}) @@ -77,7 +77,7 @@ class CohortTestMixin(object): def add_user_to_cohort(self, course_fixture, username, cohort_id): """ - Adds a user to the specified cohort group. + Adds a user to the specified cohort. """ url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + "/cohorts/{}/add".format(cohort_id) data = {"users": username} diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 5390bac698..76cd69c62d 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -89,12 +89,12 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): """ self.verify_cohort_description( self.manual_cohort_name, - 'Students are added to this cohort group only when you provide ' + 'Students are added to this cohort only when you provide ' 'their email addresses or usernames on this page', ) self.verify_cohort_description( self.auto_cohort_name, - 'Students are added to this cohort group automatically', + 'Students are added to this cohort automatically', ) def test_no_content_groups(self): @@ -104,7 +104,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): Given I have a course with a cohort defined but no content groups When I view the cohort in the instructor dashboard and select settings - Then the cohort group is not linked to a content group + Then the cohort is not linked to a content group And there is text stating that no content groups are defined And I cannot select the radio button to enable content group association And there is a link I can select to open Group settings in Studio @@ -113,7 +113,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.assertIsNone(self.cohort_management_page.get_cohort_associated_content_group()) self.assertEqual( "Warning:\nNo content groups exist. " - "Create a content group to associate with cohort groups. Create a content group", + "Create a content group to associate with cohorts. Create a content group", self.cohort_management_page.get_cohort_related_content_group_message() ) self.assertFalse(self.cohort_management_page.select_content_group_radio_button()) @@ -167,7 +167,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): confirmation_messages = self.cohort_management_page.get_cohort_confirmation_messages() self.assertEqual( [ - "2 students have been added to this cohort group", + "2 students have been added to this cohort", "1 student was removed from " + self.manual_cohort_name ], confirmation_messages @@ -235,8 +235,8 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.assertEqual( [ - "0 students have been added to this cohort group", - "1 student was already in the cohort group" + "0 students have been added to this cohort", + "1 student was already in the cohort" ], self.cohort_management_page.get_cohort_confirmation_messages() ) @@ -359,7 +359,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): "Your file '{}' has been uploaded. Allow a few minutes for processing.".format(filename) ) - # student_user is moved from manual cohort group to auto cohort group + # student_user is moved from manual cohort to auto cohort self.assertEqual( self.event_collection.find({ "name": "edx.cohort.user_added", @@ -378,7 +378,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): }).count(), 1 ) - # instructor_user (previously unassigned) is added to manual cohort group + # instructor_user (previously unassigned) is added to manual cohort self.assertEqual( self.event_collection.find({ "name": "edx.cohort.user_added", @@ -388,7 +388,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): }).count(), 1 ) - # unicode_student_user (previously unassigned) is added to manual cohort group + # unicode_student_user (previously unassigned) is added to manual cohort self.assertEqual( self.event_collection.find({ "name": "edx.cohort.user_added", @@ -467,7 +467,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): """ - Tests for linking between content groups and cohort groups in the instructor dashboard. + Tests for linking between content groups and cohort in the instructor dashboard. """ def setUp(self): @@ -515,7 +515,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): Given I have a course with a cohort defined and content groups defined When I view the cohort in the instructor dashboard and select settings - Then the cohort group is not linked to a content group + Then the cohort is not linked to a content group And there is no text stating that content groups are undefined And the content groups are listed in the selector """ @@ -622,7 +622,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): ) self.cohort_management_page.set_cohort_associated_content_group("Pears") confirmation_messages = self.cohort_management_page.get_cohort_settings_messages() - self.assertEqual(["Saved cohort group."], confirmation_messages) + self.assertEqual(["Saved cohort"], confirmation_messages) self.assertIsNone(self.cohort_management_page.get_cohort_related_content_group_message()) self.assertEquals(["Bananas", "Pears"], self.cohort_management_page.get_all_content_groups()) @@ -652,7 +652,7 @@ class CohortContentGroupAssociationTest(UniqueCourseTest, CohortTestMixin): Then refreshes the page and selects the cohort. """ confirmation_messages = self.cohort_management_page.get_cohort_settings_messages() - self.assertEqual(["Saved cohort group."], confirmation_messages) + self.assertEqual(["Saved cohort"], confirmation_messages) self.browser.refresh() self.cohort_management_page.wait_for_page() self.cohort_management_page.select_cohort(cohort_name) diff --git a/common/test/acceptance/tests/discussion/test_cohorts.py b/common/test/acceptance/tests/discussion/test_cohorts.py index 0fed9573a6..3c567a15b9 100644 --- a/common/test/acceptance/tests/discussion/test_cohorts.py +++ b/common/test/acceptance/tests/discussion/test_cohorts.py @@ -36,10 +36,10 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin, CohortTestMixin): """ def setup_cohorts(self): """ - Sets up the course to use cohorting with a single defined cohort group. + Sets up the course to use cohorting with a single defined cohort. """ self.setup_cohort_config(self.course_fixture) - self.cohort_1_name = "Cohort Group 1" + self.cohort_1_name = "Cohort 1" self.cohort_1_id = self.add_manual_cohort(self.course_fixture, self.cohort_1_name) def test_cohort_visibility_label(self): diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index 4ed4e9000f..698a51a836 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -551,7 +551,7 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, task_progress = TaskProgress(action_name, enrolled_students.count(), start_time) course = get_course_by_id(course_id) - cohorts_header = ['Cohort Group Name'] if course.is_cohorted else [] + cohorts_header = ['Cohort Name'] if course.is_cohorted else [] partition_service = LmsPartitionService(user=None, course_id=course_id) partitions = partition_service.course_partitions diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 7002a0a69e..84b3d53855 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -75,7 +75,7 @@ class TestInstructorGradeReport(TestReportMixin, InstructorTaskCourseTestCase): report_csv_filename = report_store.links_for(course_id)[0][0] with open(report_store.path_to(course_id, report_csv_filename)) as csv_file: for row in unicodecsv.DictReader(csv_file): - cohort_groups_in_csv.append(row['Cohort Group Name']) + cohort_groups_in_csv.append(row['Cohort Name']) self.assertEqual(cohort_groups_in_csv, expected_cohort_groups) @@ -97,12 +97,12 @@ class TestInstructorGradeReport(TestReportMixin, InstructorTaskCourseTestCase): def test_unicode_cohort_data_in_grading(self): """ - Test that cohort groups can contain unicode characters. + Test that cohorts can contain unicode characters. """ cohort_groups = [u'ÞrÖfessÖr X', u'MàgnëtÖ'] course = CourseFactory.create(cohort_config={'cohorted': True}) - # Create users and manually assign cohort groups + # Create users and manually assign cohorts user1 = UserFactory.create(username='user1') user2 = UserFactory.create(username='user2') CourseEnrollment.enroll(user1, course.id) diff --git a/lms/static/js/groups/views/cohort_editor.js b/lms/static/js/groups/views/cohort_editor.js index da243bcc8d..9d3485e185 100644 --- a/lms/static/js/groups/views/cohort_editor.js +++ b/lms/static/js/groups/views/cohort_editor.js @@ -54,7 +54,7 @@ var edx = edx || {}; event.preventDefault(); cohortFormView.saveForm() .done(function() { - cohortFormView.showMessage(gettext('Saved cohort group.')); + cohortFormView.showMessage(gettext('Saved cohort')); }); }, @@ -145,8 +145,8 @@ var edx = edx || {}; numPresent = modifiedUsers.present.length; if (numUsersAdded > 0 || numPresent > 0) { title = interpolate_text( - ngettext("{numUsersAdded} student has been added to this cohort group", - "{numUsersAdded} students have been added to this cohort group", numUsersAdded), + ngettext("{numUsersAdded} student has been added to this cohort", + "{numUsersAdded} students have been added to this cohort", numUsersAdded), {numUsersAdded: numUsersAdded} ); @@ -174,8 +174,8 @@ var edx = edx || {}; if (numPresent > 0) { details.push( interpolate_text( - ngettext("{numPresent} student was already in the cohort group", - "{numPresent} students were already in the cohort group", numPresent), + ngettext("{numPresent} student was already in the cohort", + "{numPresent} students were already in the cohort", numPresent), {numPresent: numPresent} ) ); diff --git a/lms/static/js/groups/views/cohort_form.js b/lms/static/js/groups/views/cohort_form.js index 6152cedcad..ed9d23decd 100644 --- a/lms/static/js/groups/views/cohort_form.js +++ b/lms/static/js/groups/views/cohort_form.js @@ -93,7 +93,7 @@ var edx = edx || {}; var errorMessages; errorMessages = []; if (!fieldData.name) { - errorMessages.push(gettext('You must specify a name for the cohort group')); + errorMessages.push(gettext('You must specify a name for the cohort')); } if (this.hasAssociatedContentGroup() && fieldData.group_id === null) { if (this.$('.input-cohort-group-association').val() === 'None') { @@ -125,8 +125,7 @@ var edx = edx || {}; errorMessages = this.validate(fieldData); if (errorMessages.length > 0) { showErrorMessage( - isUpdate ? gettext("The cohort group cannot be saved") - : gettext("The cohort group cannot be added"), + isUpdate ? gettext("The cohort cannot be saved") : gettext("The cohort cannot be added"), errorMessages ); saveOperation.reject(); diff --git a/lms/static/js/groups/views/cohorts.js b/lms/static/js/groups/views/cohorts.js index 516d592492..4fb202dd28 100644 --- a/lms/static/js/groups/views/cohorts.js +++ b/lms/static/js/groups/views/cohorts.js @@ -78,8 +78,8 @@ var edx = edx || {}; additionalCohortControlElement.addClass(hiddenClass); this.showNotification({ type: 'warning', - title: gettext('You currently have no cohort groups configured'), - actionText: gettext('Add Cohort Group'), + title: gettext('You currently have no cohorts configured'), + actionText: gettext('Add Cohort'), actionClass: 'action-create', actionIconClass: 'fa-plus' }); @@ -182,7 +182,7 @@ var edx = edx || {}; self.showNotification({ type: 'confirmation', title: interpolate_text( - gettext('The {cohortGroupName} cohort group has been created. You can manually add students to this group below.'), + gettext('The {cohortGroupName} cohort has been created. You can manually add students to this cohort below.'), {cohortGroupName: newCohort.get('name')} ) }); @@ -212,7 +212,7 @@ var edx = edx || {}; if (!this.fileUploaderView) { this.fileUploaderView = new FileUploaderView({ el: uploadElement, - title: gettext("Assign students to cohort groups by uploading a CSV file."), + title: gettext("Assign students to cohorts by uploading a CSV file."), inputLabel: gettext("Choose a .csv file"), inputTip: gettext("Only properly formatted .csv files will be accepted."), submitButtonText: gettext("Upload File and Assign Students"), diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index 652c42316e..a4859aab45 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -153,10 +153,10 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe var requestCount = requests.length, form, expectedTitle; if (action === 'add') { - expectedTitle = 'The cohort group cannot be added'; + expectedTitle = 'The cohort cannot be added'; form = getAddModal(); } else { - expectedTitle = 'The cohort group cannot be saved'; + expectedTitle = 'The cohort cannot be saved'; form = cohortsView.$('.cohort-management-settings-form'); } form.find('.action-save').click(); @@ -181,9 +181,9 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it("shows an error if no cohorts are defined", function() { createCohortsView(this, {cohorts: []}); verifyMessage( - 'You currently have no cohort groups configured', + 'You currently have no cohorts configured', 'warning', - 'Add Cohort Group' + 'Add Cohort' ); // If no cohorts have been created, can't upload a CSV file. @@ -210,7 +210,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$(fileUploadFormCss).length).toBe(0); uploadCsvToggle = cohortsView.$('.toggle-cohort-management-secondary'); expect(uploadCsvToggle.text()). - toContain('Assign students to cohort groups by uploading a CSV file'); + toContain('Assign students to cohorts by uploading a CSV file'); uploadCsvToggle.click(); // After toggle is clicked, it should be hidden. expect(uploadCsvToggle).toHaveClass('is-hidden'); @@ -294,8 +294,8 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe { cohorts: createMockCohort(defaultCohortName) } ); verifyMessage( - 'The ' + defaultCohortName + ' cohort group has been created.' + - ' You can manually add students to this group below.', + 'The ' + defaultCohortName + ' cohort has been created.' + + ' You can manually add students to this cohort below.', 'confirmation' ); verifyHeader(1, defaultCohortName, 0); @@ -316,7 +316,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe createCohortsView(this, {selectCohort: 1}); cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(' '); - saveFormAndExpectErrors('add', ['You must specify a name for the cohort group']); + saveFormAndExpectErrors('add', ['You must specify a name for the cohort']); }); it("shows a message saving when choosing to have content groups but not selecting one", function() { @@ -333,7 +333,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.cohort-name').val(''); cohortsView.$('.radio-yes').prop('checked', true).change(); saveFormAndExpectErrors('add', [ - 'You must specify a name for the cohort group', + 'You must specify a name for the cohort', 'You did not select a cohorted content group' ]); }); @@ -373,9 +373,9 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.cohort-management-nav')).toHaveClass('is-disabled'); cohortsView.$('.action-cancel').click(); verifyMessage( - 'You currently have no cohort groups configured', + 'You currently have no cohorts configured', 'warning', - 'Add Cohort Group' + 'Add Cohort' ); }); @@ -385,7 +385,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe // First try to save a blank name to create a message cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(''); - saveFormAndExpectErrors('add', ['You must specify a name for the cohort group']); + saveFormAndExpectErrors('add', ['You must specify a name for the cohort']); // Now switch to a different cohort cohortsView.$('.cohort-select').val('2').change(); @@ -399,7 +399,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe // First try to save a blank name to create a message cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val(''); - saveFormAndExpectErrors('add', ['You must specify a name for the cohort group']); + saveFormAndExpectErrors('add', ['You must specify a name for the cohort']); // Now cancel the form cohortsView.$('.action-cancel').click(); @@ -441,7 +441,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe respondToAdd({ added: ['student@sample.com'] }); respondToRefresh(catLoversUpdatedCount, dogLoversInitialCount); verifyHeader(1, 'Cat Lovers', catLoversUpdatedCount); - verifyMessage('1 student has been added to this cohort group', 'confirmation'); + verifyMessage('1 student has been added to this cohort', 'confirmation'); expect(getStudentInput().val()).toBe(''); }); @@ -508,19 +508,19 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe ); respondToAdd({ changed: [ - {email: 'moved1@sample.com', name: 'moved1', previous_cohort: 'group 2', username: 'moved1'}, - {email: 'moved2@sample.com', name: 'moved2', previous_cohort: 'group 2', username: 'moved2'}, - {email: 'moved3@sample.com', name: 'moved3', previous_cohort: 'group 3', username: 'moved3'} + {email: 'moved1@sample.com', name: 'moved1', previous_cohort: 'cohort 2', username: 'moved1'}, + {email: 'moved2@sample.com', name: 'moved2', previous_cohort: 'cohort 2', username: 'moved2'}, + {email: 'moved3@sample.com', name: 'moved3', previous_cohort: 'cohort 3', username: 'moved3'} ], present: ['alreadypresent@sample.com'] }); respondToRefresh(); - verifyDetailedMessage('3 students have been added to this cohort group', 'confirmation', + verifyDetailedMessage('3 students have been added to this cohort', 'confirmation', [ - "2 students were removed from group 2", - "1 student was removed from group 3", - "1 student was already in the cohort group" + "2 students were removed from cohort 2", + "1 student was removed from cohort 3", + "1 student was already in the cohort" ] ); expect(getStudentInput().val()).toBe(''); @@ -546,7 +546,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe addStudents('student@sample.com'); respondToAdd({ added: ['student@sample.com'] }); respondToRefresh(catLoversInitialCount + 1, dogLoversInitialCount); - verifyMessage('1 student has been added to this cohort group', 'confirmation'); + verifyMessage('1 student has been added to this cohort', 'confirmation'); }); }); @@ -602,7 +602,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe requests, createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0) ); - verifyMessage('Saved cohort group.', 'confirmation'); + verifyMessage('Saved cohort', 'confirmation'); }); it("can clear selected content group", function () { @@ -630,7 +630,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe requests, createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0) ); - verifyMessage('Saved cohort group.', 'confirmation'); + verifyMessage('Saved cohort', 'confirmation'); }); it("shows a message saving when choosing to have content groups but not selecting one", function() { @@ -662,7 +662,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe requests, createMockCohort('Cat Lovers', 1, catLoversInitialCount, 0, 0) ); - verifyMessage('Saved cohort group.', 'confirmation'); + verifyMessage('Saved cohort', 'confirmation'); // Verify that the deleted content group and associated message have been removed expect(cohortsView.$('option.option-unavailable').text().trim()).toBe(''); @@ -693,7 +693,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.radio-yes').prop('disabled')).toBeTruthy(); message = cohortsView.$('.msg-inline').text().trim(); expect(message).toContain('Warning: No content groups exist.'); - expect(message).toContain('Create a content group to associate with cohort groups.'); + expect(message).toContain('Create a content group to associate with cohorts.'); expect(message).toContain('Create a content group'); expect( cohortsView.$('.msg-inline a').attr('href'), diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index 2fa3016600..a70993de8f 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -540,7 +540,7 @@ } } - // create or edit cohort group + // create or edit cohort .cohort-management-settings, .cohort-management-edit { @extend %cohort-management-form; @@ -567,7 +567,7 @@ } } - // cohort group + // cohort .cohort-management-group-header { padding: $baseline; border-bottom: ($baseline/10) solid $gray-l4; diff --git a/lms/templates/courseware/legacy_instructor_dashboard.html b/lms/templates/courseware/legacy_instructor_dashboard.html index 87a3d716b7..9fbe0b3fff 100644 --- a/lms/templates/courseware/legacy_instructor_dashboard.html +++ b/lms/templates/courseware/legacy_instructor_dashboard.html @@ -373,7 +373,7 @@ function goto( mode) %if modeflag.get('Manage Groups'): %if instructor_access: %if course.is_cohorted: -

    ${_("To manage beta tester roles and cohort groups, visit the Membership section of the Instructor Dashboard.")}

    +

    ${_("To manage beta tester roles and cohorts, visit the Membership section of the Instructor Dashboard.")}

    %else:

    ${_("To manage beta tester roles, visit the Membership section of the Instructor Dashboard.")}

    %endif diff --git a/lms/templates/discussion/_underscore_templates.html b/lms/templates/discussion/_underscore_templates.html index a2ce4c9b91..2334d8df5a 100644 --- a/lms/templates/discussion/_underscore_templates.html +++ b/lms/templates/discussion/_underscore_templates.html @@ -399,7 +399,7 @@ ${'<% }); %>'}
    - ${_("Discussion admins, moderators, and TAs can make their posts visible to all students or specify a single cohort group.")} + ${_("Discussion admins, moderators, and TAs can make their posts visible to all students or specify a single cohort.")}
    ${'<% } %>'} diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore index 0090a87f79..a441514845 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-editor.underscore @@ -13,10 +13,10 @@
    <% if (cohort.get('assignment_type') == "none") { %> - <%- gettext("Students are added to this cohort group only when you provide their email addresses or usernames on this page.") %> + <%- gettext("Students are added to this cohort only when you provide their email addresses or usernames on this page.") %> <%= gettext("What does this mean?") %> <% } else { %> - <%- gettext("Students are added to this cohort group automatically.") %> + <%- gettext("Students are added to this cohort automatically.") %> <%- gettext("What does this mean?") %> <% } %>
    @@ -36,10 +36,10 @@
    -

    <%- gettext('Add students to this cohort group') %>

    +

    <%- gettext('Add students to this cohort') %>

    -

    <%- gettext('Note: Students can be in only one cohort group. Adding students to this group overrides any previous group assignment.') %>

    +

    <%- gettext('Note: Students can be in only one cohort. Adding students to this group overrides any previous group assignment.') %>

    diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore index 0fbb8d251f..b72501be90 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -2,7 +2,7 @@
    <% if (isNewCohort) { %> -

    <%- gettext('Add a New Cohort Group') %>

    +

    <%- gettext('Add a New Cohort') %>

    <% } %>
    @@ -14,12 +14,12 @@
    " required="required" /> + placeholder="<%- gettext("Enter the name of the cohort") %>" required="required" />
    <% } %> @@ -96,7 +96,7 @@ <%= interpolate( // Translators: Any text between %(screen_reader_start)s and %(screen_reader_end)s is only read by screen readers and never shown in the browser. - '%(screen_reader_start)sWarning:%(screen_reader_end)s No content groups exist. Create a content group to associate with cohort groups.', + '%(screen_reader_start)sWarning:%(screen_reader_end)s No content groups exist. Create a content group to associate with cohorts.', { screen_reader_start: '', screen_reader_end: '' diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore index 36ee294685..e91c2d2c33 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore @@ -1,5 +1,5 @@ <% if (!selectedCohort) { %> - + <% } %> <% _.each(cohorts, function(cohort) { %> <% diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore index 4ea2087566..e68104b150 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore @@ -4,23 +4,23 @@
    -

    <%- gettext('Assign students to cohort groups manually') %>

    +

    <%- gettext('Assign students to cohorts manually') %>

    - +
    - +
    - <%- gettext('Add Cohort Group') %> + <%- gettext('Add Cohort') %>
    @@ -35,14 +35,14 @@
    - <%- gettext('Assign students to cohort groups by uploading a CSV file') %> + <%- gettext('Assign students to cohorts by uploading a CSV file') %>

    <%= interpolate( - gettext('To review student cohort group assignments or see the results of uploading a CSV file, download course profile information or cohort results on %(link_start)s the Data Download page. %(link_end)s'), + gettext('To review student cohort assignments or see the results of uploading a CSV file, download course profile information or cohort results on %(link_start)s the Data Download page. %(link_end)s'), {link_start: '', link_end: ''}, true ) %> diff --git a/lms/templates/ux/reference/instructor_dashboard/membership.html b/lms/templates/ux/reference/instructor_dashboard/membership.html deleted file mode 100644 index 439c6e911c..0000000000 --- a/lms/templates/ux/reference/instructor_dashboard/membership.html +++ /dev/null @@ -1,464 +0,0 @@ -<%! from django.utils.translation import ugettext as _ %> -<%! from django.core.urlresolvers import reverse %> - -<%inherit file="/main.html" /> -<%namespace name='static' file='/static_content.html'/> - -<%block name="pagetitle">Instructor Dashboard -<%block name="nav_skip">#instructor-dashboard-content - -<%block name="headextra"> -<%static:css group='style-course-vendor'/> -<%static:css group='style-vendor-tinymce-content'/> -<%static:css group='style-vendor-tinymce-skin'/> -<%static:css group='style-course'/> - - - - - - - - - - - - - - - - -<%static:js group='module-descriptor-js'/> -<%static:js group='instructor_dash'/> - - -

    -
    -
    -

    Instructor Dashboard

    - -
    - -
    -

    Batch Enrollment

    -

    - - -

    - -
    - - -
    - -

    - If this option is checked, users who have not yet registered for {platform_name} will be automatically enrolled.").format(platform_name=settings.PLATFORM_NAME)} - If this option is left unchecked, users who have not yet registered for {platform_name} will not be enrolled, but will be allowed to enroll once they make an account.").format(platform_name=settings.PLATFORM_NAME)} -

    - Checking this box has no effect if 'Unenroll' is selected. -

    -
    -
    - -
    - - - -
    - -
    - - -
    -
    -
    -
    - -
    - -
    -

    Batch Beta Tester Addition

    -

    - - - -

    - -
    - - -
    - -

    - If this option is checked, users who have not enrolled in your course will be automatically enrolled. -

    - Checking this box has no effect if 'Remove beta testers' is selected. -

    -
    -
    - -
    - - - -
    - -
    - - -
    - -
    -
    -
    - -
    - -
    - ## Translators: an "Administration List" is a list, such as Course Staff, that users can be added to. -

    Administration List Management

    - -
    - -
    - ## Translators: an "Administrator Group" is a group, such as Course Staff, that users can be added to. - - - -
    - - -

    - Staff cannot modify staff or beta tester lists. To modify these lists, " - "contact your instructor and ask them to add you as an instructor for staff " - "and beta lists, or a discussion admin for discussion management. -

    - -
    - -
    - -
    - -
    - -
    - -
    -
    - -
    - -
    -

    - Cohort Management - Cohorts are such and such and are used for this and that to do all the things -

    - - -
    -
    - -
    -
    - - - -
    -
    - -
    - -
    -
    - - - - Add Cohort Group - -
    - - -
    -

    You currently have no cohort groups configured

    - -
    -

    Please complete your cohort group configuration by creating groups within Studio

    -
    - -
    - -
    -
    - - -
    -

    There's currently an error with your cohorts configuration within this course.

    - -
    -

    Error output (if any and near-human legible/comprehendable can be displayed here)

    -
    - -
    - Review Configuration in Studio -
    -
    - - -
    -
    - -

    Add a New Cohort Group

    - -
    -
    - - -
    -
    - -
    - - Cancel -
    -
    -
    - - -
    -
    - -

    Editing "Cohort Group's Name"

    - -
    -
    - - -
    -
    - -
    - - Cancel -
    -
    -
    - - -
    -

    New Cohort Name has been created. You can manually add students to this group below.

    -
    - - -
    -
    -

    - Cohort Name Can be Placed Here and Should Accommodate Almost Everything - (contains 12,546 Students) - - Edit -

    - -
    -

    This cohort group's current management set up:

    -
    - Students are added to this group automatically. - What does this mean? -
    - -
    - Edit settings in Studio -
    -
    - -
    -

    This cohort group's current management set up:

    -
    - Students are added to this group only when you provide their email addresses or usernames on this page. - What does this mean? -
    - -
    - Edit settings in Studio -
    -
    -
    - - -
    -
    - -

    Add students to this cohort group

    - -
    -

    Note: Students can only be in one cohort group. Adding students to this group overrides any previous group assignment.

    -
    - - -
    -

    2,546 students have been added to this cohort group

    - -
    -
      -
    • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
      • -
    • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
      • -
    • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
      • -
    -
    -
    - - -
    -

    There were 25 errors when trying to add students:

    - -
    -
      -
    • Unknown user: ahgaeubgoq
      • -
    • Unknown user: hagaihga
      • -
    • Unknown user: ahgaeubgoq
      • -
    • Unknown user: ahgaeubgoq
      • -
    • Unknown user: hagaihga
      • -
    -
    - -
    - View all errors -
    -
    - -
    -
    - - - - - You will not get notification for emails that bounce, so please double-check spelling. -
    -
    - -
    - -
    -
    -
    -
    - - -
    -

    You may view individual student information for each cohort via your entire course profile data download on the data download view

    -
    -
    -
    -
    -
    -
    diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 8d0b096271..ed98ebf80e 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -1,5 +1,5 @@ """ -This file contains the logic for cohort groups, as exposed internally to the +This file contains the logic for cohorts, as exposed internally to the forums, and to the cohort admin views. """ @@ -88,8 +88,8 @@ class CohortAssignmentType(object): # No automatic rules are applied to this cohort; users must be manually added. NONE = "none" - # One of (possibly) multiple cohort groups to which users are randomly assigned. - # Note: The 'default cohort' group is included in this category iff it exists and + # One of (possibly) multiple cohorts to which users are randomly assigned. + # Note: The 'default' cohort is included in this category iff it exists and # there are no other random groups. (Also see Note 2 above.) RANDOM = "random" From 798342e1772ff5b0cbc729bd582cd72cf3b384b8 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 8 Jan 2015 15:05:13 -0500 Subject: [PATCH 31/44] Rename 'cohorted content group' to 'content group' --- cms/lib/xblock/test/test_authoring_mixin.py | 18 +++++++++--------- .../tests/lms/test_lms_user_preview.py | 2 +- lms/static/js/groups/views/cohort_form.js | 4 ++-- .../js/spec/groups/views/cohorts_spec.js | 8 ++++---- .../cohort-form.underscore | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index 574c5c761f..5cf93fa469 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -36,15 +36,15 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): ) self.pet_groups = [Group(1, 'Cat Lovers'), Group(2, 'Dog Lovers')] - def create_cohorted_content_groups(self, groups): + def create_content_groups(self, content_groups): """ - Create a cohorted content partition with specified groups. + Create a cohorted user partition with the specified content groups. """ self.content_partition = UserPartition( 1, 'Content Groups', 'Contains Groups for Cohorted Courseware', - groups, + content_groups, scheme_id='cohort' ) self.course.user_partitions = [self.content_partition] @@ -76,11 +76,11 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') def test_html_empty_partition(self): - self.create_cohorted_content_groups([]) + self.create_content_groups([]) self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') def test_html_populated_partition(self): - self.create_cohorted_content_groups(self.pet_groups) + self.create_content_groups(self.pet_groups) self.verify_visibility_view_contains(self.video, ['Cat Lovers', 'Dog Lovers']) def test_html_no_partition_staff_locked(self): @@ -88,26 +88,26 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): self.verify_visibility_view_contains(self.video, ['You have not set up any groups to manage visibility with.']) def test_html_empty_partition_staff_locked(self): - self.create_cohorted_content_groups([]) + self.create_content_groups([]) self.set_staff_only(self.vertical) self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') def test_html_populated_partition_staff_locked(self): - self.create_cohorted_content_groups(self.pet_groups) + self.create_content_groups(self.pet_groups) self.set_staff_only(self.vertical) self.verify_visibility_view_contains( self.video, ['The Unit this component is contained in is hidden from students.', 'Cat Lovers', 'Dog Lovers'] ) def test_html_false_content_group(self): - self.create_cohorted_content_groups(self.pet_groups) + self.create_content_groups(self.pet_groups) self.set_group_access(self.video, ['false_group_id']) self.verify_visibility_view_contains( self.video, ['Cat Lovers', 'Dog Lovers', 'Content group no longer exists.'] ) def test_html_false_content_group_staff_locked(self): - self.create_cohorted_content_groups(self.pet_groups) + self.create_content_groups(self.pet_groups) self.set_staff_only(self.vertical) self.set_group_access(self.video, ['false_group_id']) self.verify_visibility_view_contains( diff --git a/common/test/acceptance/tests/lms/test_lms_user_preview.py b/common/test/acceptance/tests/lms/test_lms_user_preview.py index 9eb6666773..d1cb626f52 100644 --- a/common/test/acceptance/tests/lms/test_lms_user_preview.py +++ b/common/test/acceptance/tests/lms/test_lms_user_preview.py @@ -229,7 +229,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): class CourseWithContentGroupsTest(StaffViewTest): """ - Verifies that changing the "previewing as" selector works properly for cohorted content. + Verifies that changing the "previewing as" selector works properly for content groups. """ def setUp(self): diff --git a/lms/static/js/groups/views/cohort_form.js b/lms/static/js/groups/views/cohort_form.js index ed9d23decd..98f3815ae6 100644 --- a/lms/static/js/groups/views/cohort_form.js +++ b/lms/static/js/groups/views/cohort_form.js @@ -97,10 +97,10 @@ var edx = edx || {}; } if (this.hasAssociatedContentGroup() && fieldData.group_id === null) { if (this.$('.input-cohort-group-association').val() === 'None') { - errorMessages.push(gettext('You did not select a cohorted content group')); + errorMessages.push(gettext('You did not select a content group')); } else { // If a value was selected, then it must be for a non-existent/deleted content group - errorMessages.push(gettext('The selected cohorted content group does not exist')); + errorMessages.push(gettext('The selected content group does not exist')); } } return errorMessages; diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index a4859aab45..f12c7326c5 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -324,7 +324,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.action-create').click(); cohortsView.$('.cohort-name').val('New Cohort'); cohortsView.$('.radio-yes').prop('checked', true).change(); - saveFormAndExpectErrors('add', ['You did not select a cohorted content group']); + saveFormAndExpectErrors('add', ['You did not select a content group']); }); it("shows two message when both fields have problems", function() { @@ -334,7 +334,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.radio-yes').prop('checked', true).change(); saveFormAndExpectErrors('add', [ 'You must specify a name for the cohort', - 'You did not select a cohorted content group' + 'You did not select a content group' ]); }); @@ -638,7 +638,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe cohortsView.$('.tab-settings a').click(); cohortsView.$('.cohort-name').val('New Cohort'); cohortsView.$('.radio-yes').prop('checked', true).change(); - saveFormAndExpectErrors('update', ['You did not select a cohorted content group']); + saveFormAndExpectErrors('update', ['You did not select a content group']); }); it("shows a message when the selected content group does not exist", function () { @@ -672,7 +672,7 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe it("shows an error when saving with a deleted content group", function () { createCohortsViewWithDeletedContentGroup(this); cohortsView.$('.tab-settings a').click(); - saveFormAndExpectErrors('save', ['The selected cohorted content group does not exist']); + saveFormAndExpectErrors('save', ['The selected content group does not exist']); }); it("shows an error when the save fails", function () { diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore index b72501be90..6ebcef8c9e 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -34,7 +34,7 @@

    - <%- gettext('Associated Cohorted Content Group') %> + <%- gettext('Associated Content Group') %>

    From bca6274eb5ab6c3f7c7023b56eb0a98e6ccfc576 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 8 Jan 2015 15:26:10 -0500 Subject: [PATCH 32/44] Miscellaneous text updates --- cms/lib/xblock/test/test_authoring_mixin.py | 8 +++---- cms/templates/js/publish-xblock.underscore | 2 +- cms/templates/visibility_editor.html | 8 +++---- .../discussion/test_cohort_management.py | 3 +-- .../tests/lms/test_lms_user_preview.py | 2 +- .../tests/studio/test_studio_container.py | 22 +++++++++---------- .../js/spec/groups/views/cohorts_spec.js | 1 - .../courseware/course_navigation.html | 2 +- .../cohort-form.underscore | 2 +- 9 files changed, 24 insertions(+), 26 deletions(-) diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index 5cf93fa469..563f605a66 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -73,11 +73,11 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): self.assertIn(string, html) def test_html_no_partition(self): - self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') + self.verify_visibility_view_contains(self.video, 'No content groups exist') def test_html_empty_partition(self): self.create_content_groups([]) - self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') + self.verify_visibility_view_contains(self.video, 'No content groups exist') def test_html_populated_partition(self): self.create_content_groups(self.pet_groups) @@ -85,12 +85,12 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): def test_html_no_partition_staff_locked(self): self.set_staff_only(self.vertical) - self.verify_visibility_view_contains(self.video, ['You have not set up any groups to manage visibility with.']) + self.verify_visibility_view_contains(self.video, ['No content groups exist']) def test_html_empty_partition_staff_locked(self): self.create_content_groups([]) self.set_staff_only(self.vertical) - self.verify_visibility_view_contains(self.video, 'You have not set up any groups to manage visibility with.') + self.verify_visibility_view_contains(self.video, 'No content groups exist') def test_html_populated_partition_staff_locked(self): self.create_content_groups(self.pet_groups) diff --git a/cms/templates/js/publish-xblock.underscore b/cms/templates/js/publish-xblock.underscore index 521cfc31a2..8d40947443 100644 --- a/cms/templates/js/publish-xblock.underscore +++ b/cms/templates/js/publish-xblock.underscore @@ -80,7 +80,7 @@ var visibleToStaffOnly = visibilityState === 'staff_only'; <% if (hasContentGroupComponents) { %>

    - <%= gettext("Some content in this unit is only visible to particular groups") %> + <%= gettext("Some content in this unit is visible only to particular content groups") %>

    <% } %>
      diff --git a/cms/templates/visibility_editor.html b/cms/templates/visibility_editor.html index 1eebb5b37b..b5b9d9bd22 100644 --- a/cms/templates/visibility_editor.html +++ b/cms/templates/visibility_editor.html @@ -15,14 +15,14 @@ is_staff_locked = ancestor_has_staff_lock(xblock)
      % if len(groups) == 0:
      -

      ${_('You have not set up any groups to manage visibility with.')}

      +

      ${_('No content groups exist')}

      -

      ${_('Groups are a way for you to organize content in your course with a particular student experience in mind. They are commonly used to facilitate content and pedagogical experiments as well as provide different tracks of content.')}

      +

      ${_('Use content groups to give groups of students access to a specific set of course content. Create one or more content groups, and make specific components visible to them.')}

      - ${_('Manage groups in this course')} + ${_('Manage content groups')}
      % elif is_staff_locked: @@ -46,7 +46,7 @@ is_staff_locked = ancestor_has_staff_lock(xblock)
      -

      ${_('Set visibility to:')}

      +

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

      diff --git a/common/test/acceptance/tests/discussion/test_cohort_management.py b/common/test/acceptance/tests/discussion/test_cohort_management.py index 76cd69c62d..124927b35c 100644 --- a/common/test/acceptance/tests/discussion/test_cohort_management.py +++ b/common/test/acceptance/tests/discussion/test_cohort_management.py @@ -112,8 +112,7 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin): self.cohort_management_page.select_cohort(self.manual_cohort_name) self.assertIsNone(self.cohort_management_page.get_cohort_associated_content_group()) self.assertEqual( - "Warning:\nNo content groups exist. " - "Create a content group to associate with cohorts. Create a content group", + "Warning:\nNo content groups exist. Create a content group", self.cohort_management_page.get_cohort_related_content_group_message() ) self.assertFalse(self.cohort_management_page.select_content_group_radio_button()) diff --git a/common/test/acceptance/tests/lms/test_lms_user_preview.py b/common/test/acceptance/tests/lms/test_lms_user_preview.py index d1cb626f52..b17e74250c 100644 --- a/common/test/acceptance/tests/lms/test_lms_user_preview.py +++ b/common/test/acceptance/tests/lms/test_lms_user_preview.py @@ -229,7 +229,7 @@ class StaffDebugTest(CourseWithoutContentGroupsTest): class CourseWithContentGroupsTest(StaffViewTest): """ - Verifies that changing the "previewing as" selector works properly for content groups. + Verifies that changing the "View this course as" selector works properly for content groups. """ def setUp(self): diff --git a/common/test/acceptance/tests/studio/test_studio_container.py b/common/test/acceptance/tests/studio/test_studio_container.py index a6432fc967..a1338e2131 100644 --- a/common/test/acceptance/tests/studio/test_studio_container.py +++ b/common/test/acceptance/tests/studio/test_studio_container.py @@ -301,7 +301,7 @@ class EditVisibilityModalTest(ContainerBase): MISSING_GROUP_LABEL = 'Deleted Content Group\nContent group no longer exists. Please choose another or allow access to All Students and staff' VALIDATION_ERROR_LABEL = 'This component has validation issues.' VALIDATION_ERROR_MESSAGE = 'Error:\nThis component refers to deleted or invalid content groups.' - GROUP_VISIBILITY_MESSAGE = 'Some content in this unit is only visible to particular groups' + GROUP_VISIBILITY_MESSAGE = 'Some content in this unit is visible only to particular content groups' def setUp(self): super(EditVisibilityModalTest, self).setUp() @@ -429,7 +429,7 @@ class EditVisibilityModalTest(ContainerBase): When I go to the container page for that unit And I open the visibility editor modal for that unit's component Then the default visibility selection should be 'All Students and Staff' - And the container page should not display 'Some content in this unit is only visible to particular groups' + And the container page should not display the content visibility warning """ self.verify_selected_labels(self.edit_component_visibility(self.html_component), [self.VISIBILITY_LABEL_ALL]) self.verify_visibility_set(self.html_component, False) @@ -442,12 +442,12 @@ class EditVisibilityModalTest(ContainerBase): And I open the visibility editor modal for that unit's component And I select 'Dogs' And I save the modal - Then the container page should display 'Some content in this unit is only visible to particular groups' + Then the container page should display the content visibility warning And I re-open the visibility editor modal for that unit's component And I select 'All Students and Staff' And I save the modal Then the visibility selection should be 'All Students and Staff' - And the container page should not display 'Some content in this unit is only visible to particular groups' + And the container page should not display the content visibility warning """ self.select_and_verify_saved(self.html_component, ['Dogs']) self.verify_visibility_set(self.html_component, True) @@ -463,7 +463,7 @@ class EditVisibilityModalTest(ContainerBase): And I select 'Dogs' And I save the modal Then the visibility selection should be 'Dogs' and 'Specific Content Groups' - And the container page should display 'Some content in this unit is only visible to particular groups' + And the container page should display the content visibility warning """ self.select_and_verify_saved(self.html_component, ['Dogs']) self.verify_visibility_set(self.html_component, True) @@ -477,7 +477,7 @@ class EditVisibilityModalTest(ContainerBase): And I select 'Dogs' and 'Cats' And I save the modal Then the visibility selection should be 'Dogs', 'Cats', and 'Specific Content Groups' - And the container page should display 'Some content in this unit is only visible to particular groups' + And the container page should display the content visibility warning """ self.select_and_verify_saved(self.html_component, ['Dogs', 'Cats']) self.verify_visibility_set(self.html_component, True) @@ -492,7 +492,7 @@ class EditVisibilityModalTest(ContainerBase): And I select 'Specific Content Groups' And I save the modal Then the visibility selection should be 'All Students and Staff' - And the container page should not display 'Some content in this unit is only visible to particular groups' + And the container page should not display the content visibility warning """ self.select_and_verify_saved( self.html_component, [self.VISIBILITY_LABEL_SPECIFIC], expected_labels=[self.VISIBILITY_LABEL_ALL] @@ -509,12 +509,12 @@ class EditVisibilityModalTest(ContainerBase): Then I should see a validation error message on that unit's component And I open the visibility editor modal for that unit's component Then I should see that I have selected multiple deleted groups - And the container page should display 'Some content in this unit is only visible to particular groups' + And the container page should display the content visibility warning And I de-select the missing groups And I save the modal Then the visibility selection should be 'All Students and Staff' And I should not see any validation errors on the component - And the container page should not display 'Some content in this unit is only visible to particular groups' + And the container page should not display the content visibility warning """ self.update_component(self.html_component, {'group_access': {0: [2, 3]}}) self.verify_component_validation_error(self.html_component) @@ -533,12 +533,12 @@ class EditVisibilityModalTest(ContainerBase): Then I should see a validation error message on that unit's component And I open the visibility editor modal for that unit's component Then I should see that I have selected multiple deleted groups - And the container page should display 'Some content in this unit is only visible to particular groups' + And the container page should display the content visibility warning And I de-select the missing groups And I save the modal Then the visibility selection should be the names of the valid groups. And I should not see any validation errors on the component - And the container page should display 'Some content in this unit is only visible to particular groups' + And the container page should display the content visibility warning """ self.update_component(self.html_component, {'group_access': {0: [0, 1, 2, 3]}}) self.verify_component_validation_error(self.html_component) diff --git a/lms/static/js/spec/groups/views/cohorts_spec.js b/lms/static/js/spec/groups/views/cohorts_spec.js index f12c7326c5..a2356b5f20 100644 --- a/lms/static/js/spec/groups/views/cohorts_spec.js +++ b/lms/static/js/spec/groups/views/cohorts_spec.js @@ -693,7 +693,6 @@ define(['backbone', 'jquery', 'js/common_helpers/ajax_helpers', 'js/common_helpe expect(cohortsView.$('.radio-yes').prop('disabled')).toBeTruthy(); message = cohortsView.$('.msg-inline').text().trim(); expect(message).toContain('Warning: No content groups exist.'); - expect(message).toContain('Create a content group to associate with cohorts.'); expect(message).toContain('Create a content group'); expect( cohortsView.$('.msg-inline a').attr('href'), diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index a2982b2e71..635be77502 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -35,7 +35,7 @@ def url_class(is_active):
      1. - + + placeholder="<%- gettext('e.g. johndoe@example.com, JaneDoe, joeydoe@example.com') %>" + aria-describedby="email-notification-tip"> - <%- gettext('You will not receive notification for emails that bounce, so double-check your spelling.') %> + <%- gettext('You will not receive notification for emails that bounce, so double-check your spelling.') %>
      @@ -67,6 +68,6 @@
      -