diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index b79d86b52f..ee22b0e9af 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -264,6 +264,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertContains(resp, '/c4x/edX/full/asset/handouts_schematic_tutorial.pdf') + class ContentStoreTest(ModuleStoreTestCase): """ Tests for the CMS ContentStore application. @@ -422,6 +423,52 @@ class ContentStoreTest(ModuleStoreTestCase): self.assertNotIn('markdown', problem.editable_metadata_fields, "Markdown slipped into the editable metadata fields") + def test_metadata_inheritance(self): + import_from_xml(modulestore(), 'common/test/data/', ['full']) + + ms = modulestore('direct') + course = ms.get_item(Location(['i4x', 'edX', 'full', 'course', '6.002_Spring_2012', None])) + + verticals = ms.get_items(['i4x', 'edX', 'full', 'vertical', None, None]) + + # let's assert on the metadata_inheritance on an existing vertical + for vertical in verticals: + self.assertIn('xqa_key', vertical.metadata) + self.assertEqual(course.metadata['xqa_key'], vertical.metadata['xqa_key']) + + self.assertGreater(len(verticals), 0) + + new_component_location = Location('i4x', 'edX', 'full', 'html', 'new_component') + source_template_location = Location('i4x', 'edx', 'templates', 'html', 'Empty') + + # crate a new module and add it as a child to a vertical + ms.clone_item(source_template_location, new_component_location) + parent = verticals[0] + ms.update_children(parent.location, parent.definition.get('children', []) + [new_component_location.url()]) + + # flush the cache + ms.get_cached_metadata_inheritance_tree(new_component_location, -1) + new_module = ms.get_item(new_component_location) + + # check for grace period definition which should be defined at the course level + self.assertIn('graceperiod', new_module.metadata) + + self.assertEqual(course.metadata['graceperiod'], new_module.metadata['graceperiod']) + + # + # now let's define an override at the leaf node level + # + new_module.metadata['graceperiod'] = '1 day' + ms.update_metadata(new_module.location, new_module.metadata) + + # flush the cache and refetch + ms.get_cached_metadata_inheritance_tree(new_component_location, -1) + new_module = ms.get_item(new_component_location) + + self.assertIn('graceperiod', new_module.metadata) + self.assertEqual('1 day', new_module.metadata['graceperiod']) + + class TemplateTestCase(ModuleStoreTestCase): def test_template_cleanup(self): diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index dab5d5e85b..da96bfa212 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -44,5 +44,6 @@ class MakoModuleDescriptor(XModuleDescriptor): # cdodge: encapsulate a means to expose "editable" metadata fields (i.e. not internal system metadata) @property def editable_metadata_fields(self): - subset = [name for name in self.metadata.keys() if name not in self.system_metadata_fields] + subset = [name for name in self.metadata.keys() if name not in self.system_metadata_fields and + name not in self._inherited_metadata] return subset diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index f4db62ac31..012efb0c27 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -1,11 +1,13 @@ import pymongo import sys import logging +import copy from bson.son import SON from fs.osfs import OSFS from itertools import repeat from path import path +from datetime import datetime, timedelta from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str @@ -27,9 +29,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem): """ A system that has a cache of module json that it will use to load modules from, with a backup of calling to the underlying modulestore for more data + TODO (cdodge) when the 'split module store' work has been completed we can remove all + references to metadata_inheritance_tree """ def __init__(self, modulestore, module_data, default_class, resources_fs, - error_tracker, render_template): + error_tracker, render_template, metadata_inheritance_tree = None): """ modulestore: the module store that can be used to retrieve additional modules @@ -54,6 +58,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # cdodge: other Systems have a course_id attribute defined. To keep things consistent, let's # define an attribute here as well, even though it's None self.course_id = None + self.metadata_inheritance_tree = metadata_inheritance_tree def load_item(self, location): location = Location(location) @@ -61,11 +66,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem): if json_data is None: return self.modulestore.get_item(location) else: - # TODO (vshnayder): metadata inheritance is somewhat broken because mongo, doesn't - # always load an entire course. We're punting on this until after launch, and then - # will build a proper course policy framework. + # load the module and apply the inherited metadata try: - return XModuleDescriptor.load_from_json(json_data, self, self.default_class) + module = XModuleDescriptor.load_from_json(json_data, self, self.default_class) + if self.metadata_inheritance_tree is not None: + metadata_to_inherit = self.metadata_inheritance_tree.get('parent_metadata', {}).get(location.url(),{}) + module.inherit_metadata(metadata_to_inherit) + return module except: return ErrorDescriptor.from_json( json_data, @@ -142,6 +149,82 @@ class MongoModuleStore(ModuleStoreBase): self.fs_root = path(fs_root) self.error_tracker = error_tracker self.render_template = render_template + self.metadata_inheritance_cache = {} + + def get_metadata_inheritance_tree(self, location): + ''' + TODO (cdodge) This method can be deleted when the 'split module store' work has been completed + ''' + + # get all collections in the course, this query should not return any leaf nodes + query = { '_id.org' : location.org, + '_id.course' : location.course, + '_id.revision' : None, + 'definition.children':{'$ne': []} + } + # we just want the Location, children, and metadata + record_filter = {'_id':1,'definition.children':1,'metadata':1} + + # call out to the DB + resultset = self.collection.find(query, record_filter) + + results_by_url = {} + root = None + + # now go through the results and order them by the location url + for result in resultset: + location = Location(result['_id']) + results_by_url[location.url()] = result + if location.category == 'course': + root = location.url() + + # now traverse the tree and compute down the inherited metadata + metadata_to_inherit = {} + def _compute_inherited_metadata(url): + my_metadata = results_by_url[url]['metadata'] + for key in my_metadata.keys(): + if key not in XModuleDescriptor.inheritable_metadata: + del my_metadata[key] + results_by_url[url]['metadata'] = my_metadata + + # go through all the children and recurse, but only if we have + # in the result set. Remember results will not contain leaf nodes + for child in results_by_url[url].get('definition',{}).get('children',[]): + if child in results_by_url: + new_child_metadata = copy.deepcopy(my_metadata) + new_child_metadata.update(results_by_url[child]['metadata']) + results_by_url[child]['metadata'] = new_child_metadata + metadata_to_inherit[child] = new_child_metadata + _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 + + if root is not None: + _compute_inherited_metadata(root) + + cache = {'parent_metadata': metadata_to_inherit, + 'timestamp' : datetime.now()} + + return cache + + def get_cached_metadata_inheritance_tree(self, location, max_age_allowed): + ''' + TODO (cdodge) This method can be deleted when the 'split module store' work has been completed + ''' + cache_name = '{0}/{1}'.format(location.org, location.course) + cache = self.metadata_inheritance_cache.get(cache_name,{'parent_metadata': {}, + 'timestamp': datetime.now() - timedelta(hours=1)}) + age = (datetime.now() - cache['timestamp']) + + if age.seconds >= max_age_allowed: + logging.debug('loading entire inheritance tree for {0}'.format(cache_name)) + cache = self.get_metadata_inheritance_tree(location) + self.metadata_inheritance_cache[cache_name] = cache + + return cache + + def _clean_item_data(self, item): """ @@ -196,6 +279,8 @@ class MongoModuleStore(ModuleStoreBase): resource_fs = OSFS(root) + # TODO (cdodge): When the 'split module store' work has been completed, we should remove + # the 'metadata_inheritance_tree' parameter system = CachingDescriptorSystem( self, data_cache, @@ -203,6 +288,7 @@ class MongoModuleStore(ModuleStoreBase): resource_fs, self.error_tracker, self.render_template, + metadata_inheritance_tree = self.get_cached_metadata_inheritance_tree(Location(item['location']), 60) ) return system.load_item(item['location']) @@ -261,11 +347,11 @@ class MongoModuleStore(ModuleStoreBase): descendents of the queried modules for more efficient results later in the request. The depth is counted in the number of calls to get_children() to cache. None indicates to cache all descendents. - """ location = Location.ensure_fully_specified(location) item = self._find_one(location) - return self._load_items([item], depth)[0] + module = self._load_items([item], depth)[0] + return module def get_instance(self, course_id, location, depth=0): """ @@ -285,7 +371,8 @@ class MongoModuleStore(ModuleStoreBase): sort=[('revision', pymongo.ASCENDING)], ) - return self._load_items(list(items), depth) + modules = self._load_items(list(items), depth) + return modules def clone_item(self, source, location): """ @@ -313,7 +400,7 @@ class MongoModuleStore(ModuleStoreBase): raise DuplicateItemError(location) - def get_course_for_item(self, location): + def get_course_for_item(self, location, depth=0): ''' VS[compat] cdodge: for a given Xmodule, return the course that it belongs to @@ -327,7 +414,7 @@ class MongoModuleStore(ModuleStoreBase): # know the 'name' parameter in this context, so we have # to assume there's only one item in this query even though we are not specifying a name course_search_location = ['i4x', location.org, location.course, 'course', None] - courses = self.get_items(course_search_location) + courses = self.get_items(course_search_location, depth=depth) # make sure we found exactly one match on this above course search found_cnt = len(courses) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index b5e9b10ea6..dccc96a7ca 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -411,7 +411,6 @@ class ResourceTemplates(object): return templates - class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): """ An XModuleDescriptor is a specification for an element of a course. This @@ -585,11 +584,11 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): def inherit_metadata(self, metadata): """ Updates this module with metadata inherited from a containing module. - Only metadata specified in self.inheritable_metadata will + Only metadata specified in inheritable_metadata will be inherited """ # Set all inheritable metadata from kwargs that are - # in self.inheritable_metadata and aren't already set in metadata + # in inheritable_metadata and aren't already set in metadata for attr in self.inheritable_metadata: if attr not in self.metadata and attr in metadata: self._inherited_metadata.add(attr)