From bd73cc16eee223f1cc9c3f3f5c14f7bcd59c3d01 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 18 Feb 2013 18:49:45 -0500 Subject: [PATCH 1/7] compute (and cache) a generation of metadata inheritance tree for the course. We can then add that metadata to a newly instantiated xmodule --- common/lib/xmodule/xmodule/mako_module.py | 3 +- .../lib/xmodule/xmodule/modulestore/mongo.py | 95 +++++++++++++++++-- common/lib/xmodule/xmodule/x_module.py | 33 ++++--- 3 files changed, 105 insertions(+), 26 deletions(-) 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..83ce95ba0f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -1,15 +1,17 @@ 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 -from xmodule.x_module import XModuleDescriptor +from xmodule.x_module import XModuleDescriptor, inheritable_metadata from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor @@ -29,7 +31,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): from, with a backup of calling to the underlying modulestore for more data """ 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 +56,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) @@ -65,8 +68,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # always load an entire course. We're punting on this until after launch, and then # will build a proper course policy framework. try: - return XModuleDescriptor.load_from_json(json_data, self, self.default_class) + module = XModuleDescriptor.load_from_json(json_data, self, self.default_class) + metadata_to_inherit = self.metadata_inheritance_tree.get('parent_metadata', {}).get(location.url(),{}) + module.inherit_metadata(metadata_to_inherit) + return module except: + raise return ErrorDescriptor.from_json( json_data, self, @@ -142,6 +149,76 @@ 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): + + # 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 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): + 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): """ @@ -203,6 +280,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 +339,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 +363,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 +392,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 +406,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 07b9d3653e..0dd5488495 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -411,6 +411,19 @@ class ResourceTemplates(object): return templates +# A list of metadata that this module can inherit from its parent module +inheritable_metadata = ( + 'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize', + # TODO (ichuang): used for Fall 2012 xqa server access + 'xqa_key', + # TODO: This is used by the XMLModuleStore to provide for locations for + # static files, and will need to be removed when that code is removed + 'data_dir', + # How many days early to show a course element to beta testers (float) + # intended to be set per-course, but can be overridden in for specific + # elements. Can be a float. + 'days_early_for_beta' +) class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): """ @@ -433,20 +446,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): # It should respond to max_score() and grade(). It can be graded or ungraded # (like a practice problem). - # A list of metadata that this module can inherit from its parent module - inheritable_metadata = ( - 'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize', - # TODO (ichuang): used for Fall 2012 xqa server access - 'xqa_key', - # TODO: This is used by the XMLModuleStore to provide for locations for - # static files, and will need to be removed when that code is removed - 'data_dir', - # How many days early to show a course element to beta testers (float) - # intended to be set per-course, but can be overridden in for specific - # elements. Can be a float. - 'days_early_for_beta' - ) - # cdodge: this is a list of metadata names which are 'system' metadata # and should not be edited by an end-user system_metadata_fields = ['data_dir', 'published_date', 'published_by', 'is_draft'] @@ -585,12 +584,12 @@ 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 - for attr in self.inheritable_metadata: + # in inheritable_metadata and aren't already set in metadata + for attr in inheritable_metadata: if attr not in self.metadata and attr in metadata: self._inherited_metadata.add(attr) self.metadata[attr] = metadata[attr] From 1a2e7941260afde12129c8e8fdce7e1bb8994c0d Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 19 Feb 2013 09:51:07 -0500 Subject: [PATCH 2/7] add test of inheritance on xmodule insert --- .../contentstore/tests/test_contentstore.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 7622ee7661..f2a3443648 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,35 @@ 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]) + + 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']) + + + class TemplateTestCase(ModuleStoreTestCase): def test_template_cleanup(self): From 94613c3ed3df123ac1913cf88023b184a0b33e0d Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 19 Feb 2013 10:33:38 -0500 Subject: [PATCH 3/7] add a null guard --- common/lib/xmodule/xmodule/modulestore/mongo.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 83ce95ba0f..d73ea641c7 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -69,8 +69,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # will build a proper course policy framework. try: module = XModuleDescriptor.load_from_json(json_data, self, self.default_class) - metadata_to_inherit = self.metadata_inheritance_tree.get('parent_metadata', {}).get(location.url(),{}) - module.inherit_metadata(metadata_to_inherit) + 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: raise From 756959812fec50e23a906ec2a0ee606070355e10 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 20 Feb 2013 14:11:57 -0500 Subject: [PATCH 4/7] add some comments regarding which code can be removed when the 'split mongo module store' work is completed --- common/lib/xmodule/xmodule/modulestore/mongo.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index d73ea641c7..06cdf75dc6 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -29,6 +29,8 @@ 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, metadata_inheritance_tree = None): @@ -153,6 +155,9 @@ class MongoModuleStore(ModuleStoreBase): 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, @@ -207,6 +212,9 @@ class MongoModuleStore(ModuleStoreBase): 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)}) @@ -274,6 +282,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, From da223d9e1166fe54b9e9fc2a30f276e4362a0d04 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 20 Feb 2013 14:13:19 -0500 Subject: [PATCH 5/7] remove raise statement which was just needed for debugging --- common/lib/xmodule/xmodule/modulestore/mongo.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 06cdf75dc6..3a889be201 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -76,7 +76,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module.inherit_metadata(metadata_to_inherit) return module except: - raise return ErrorDescriptor.from_json( json_data, self, From bd188f92ef59b23235a396bbc139aa6362bd99e0 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 20 Feb 2013 16:20:39 -0500 Subject: [PATCH 6/7] put back the inheritable_metadata list into the XModuleDescriptor class. Also extend the metadata_inheritance test case --- .../contentstore/tests/test_contentstore.py | 17 +++++++++++ .../lib/xmodule/xmodule/modulestore/mongo.py | 4 +-- common/lib/xmodule/xmodule/x_module.py | 30 +++++++++---------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index f2a3443648..45a57f6156 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -431,6 +431,11 @@ class ContentStoreTest(ModuleStoreTestCase): 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') @@ -450,6 +455,18 @@ class ContentStoreTest(ModuleStoreTestCase): 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): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 3a889be201..86540571d1 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -11,7 +11,7 @@ from datetime import datetime, timedelta from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str -from xmodule.x_module import XModuleDescriptor, inheritable_metadata +from xmodule.x_module import XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor @@ -185,7 +185,7 @@ class MongoModuleStore(ModuleStoreBase): def _compute_inherited_metadata(url): my_metadata = results_by_url[url]['metadata'] for key in my_metadata.keys(): - if key not in inheritable_metadata: + if key not in XModuleDescriptor.inheritable_metadata: del my_metadata[key] results_by_url[url]['metadata'] = my_metadata diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 0dd5488495..42a94d2d44 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -411,20 +411,6 @@ class ResourceTemplates(object): return templates -# A list of metadata that this module can inherit from its parent module -inheritable_metadata = ( - 'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize', - # TODO (ichuang): used for Fall 2012 xqa server access - 'xqa_key', - # TODO: This is used by the XMLModuleStore to provide for locations for - # static files, and will need to be removed when that code is removed - 'data_dir', - # How many days early to show a course element to beta testers (float) - # intended to be set per-course, but can be overridden in for specific - # elements. Can be a float. - 'days_early_for_beta' -) - class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): """ An XModuleDescriptor is a specification for an element of a course. This @@ -446,6 +432,20 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): # It should respond to max_score() and grade(). It can be graded or ungraded # (like a practice problem). + # A list of metadata that this module can inherit from its parent module + inheritable_metadata = ( + 'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize', + # TODO (ichuang): used for Fall 2012 xqa server access + 'xqa_key', + # TODO: This is used by the XMLModuleStore to provide for locations for + # static files, and will need to be removed when that code is removed + 'data_dir', + # How many days early to show a course element to beta testers (float) + # intended to be set per-course, but can be overridden in for specific + # elements. Can be a float. + 'days_early_for_beta' + ) + # cdodge: this is a list of metadata names which are 'system' metadata # and should not be edited by an end-user system_metadata_fields = ['data_dir', 'published_date', 'published_by', 'is_draft'] @@ -589,7 +589,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): """ # Set all inheritable metadata from kwargs that are # in inheritable_metadata and aren't already set in metadata - for attr in inheritable_metadata: + for attr in self.inheritable_metadata: if attr not in self.metadata and attr in metadata: self._inherited_metadata.add(attr) self.metadata[attr] = metadata[attr] From 67f97c2c566f99ba733f6757d1366f7fa46d9eec Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 20 Feb 2013 16:26:09 -0500 Subject: [PATCH 7/7] update Victor's metadata inheritance comment --- common/lib/xmodule/xmodule/modulestore/mongo.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 86540571d1..012efb0c27 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -66,9 +66,7 @@ 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: module = XModuleDescriptor.load_from_json(json_data, self, self.default_class) if self.metadata_inheritance_tree is not None: