From 288620592aa1c2d5c07a34d3a9bbd272f3295ee5 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Sun, 7 Apr 2013 19:33:04 -0400 Subject: [PATCH 1/4] get metadata inheritance to work with draft stores. Fix bug on .publish() method whereby inherited metadata got written to the DB as 'own-metadata'. Change filter in mako_module.py to return list of own-metadata and inherited-metadata as 'editable-metadata'. Also, add unit test for draft metadata-inheritance and publish --- .../contentstore/tests/test_contentstore.py | 63 +++++++++++++++++++ common/lib/xmodule/xmodule/mako_module.py | 15 +++-- .../lib/xmodule/xmodule/modulestore/draft.py | 3 +- .../lib/xmodule/xmodule/modulestore/mongo.py | 21 +++++-- common/test/data/simple/course.xml | 7 +++ 5 files changed, 98 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 49a609a879..1791d845cd 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -92,6 +92,69 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): return cnt + def test_draft_metadata(self): + ''' + This verifies a bug we had where inherited metadata was getting written to the + module as 'own-metadata' when publishing. Also verifies the metadata inheritance is + properly computed + ''' + store = modulestore() + draft_store = modulestore('draft') + import_from_xml(store, 'common/test/data/', ['simple']) + + course = draft_store.get_item(Location(['i4x', 'edX', 'simple', + 'course', '2012_Fall', None]), depth=None) + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertNotIn('graceperiod', own_metadata(html_module)) + + draft_store.clone_item(html_module.location, html_module.location) + + # refetch to check metadata + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertNotIn('graceperiod', own_metadata(html_module)) + + # publish module + draft_store.publish(html_module.location, 0) + + # refetch to check metadata + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertNotIn('graceperiod', own_metadata(html_module)) + + # put back in draft and change metadata and see if it's now marked as 'own_metadata' + draft_store.clone_item(html_module.location, html_module.location) + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + new_graceperiod = timedelta(**{'hours': 1}) + + self.assertNotIn('graceperiod', own_metadata(html_module)) + html_module.lms.graceperiod = new_graceperiod + self.assertIn('graceperiod', own_metadata(html_module)) + self.assertEqual(html_module.lms.graceperiod, new_graceperiod) + + draft_store.update_metadata(html_module.location, own_metadata(html_module)) + + # read back to make sure it reads as 'own-metadata' + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + self.assertIn('graceperiod', own_metadata(html_module)) + self.assertEqual(html_module.lms.graceperiod, new_graceperiod) + + # republish + draft_store.publish(html_module.location, 0) + + # and re-read and verify 'own-metadata' + draft_store.clone_item(html_module.location, html_module.location) + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + self.assertIn('graceperiod', own_metadata(html_module)) + self.assertEqual(html_module.lms.graceperiod, new_graceperiod) + def test_get_depth_with_drafts(self): import_from_xml(modulestore(), 'common/test/data/', ['simple']) diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index 84db6ad779..0ce0b3c01b 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -1,5 +1,6 @@ from .x_module import XModuleDescriptor, DescriptorSystem -from .modulestore.inheritance import own_metadata +from .modulestore.inheritance import own_metadata, INHERITABLE_METADATA +from xblock.core import Scope class MakoDescriptorSystem(DescriptorSystem): @@ -45,9 +46,15 @@ class MakoModuleDescriptor(XModuleDescriptor): @property def editable_metadata_fields(self): fields = {} - for field, value in own_metadata(self).items(): - if field in self.system_metadata_fields: + for field in self.fields + self.lms.fields: + if field.scope != Scope.settings: continue - fields[field] = value + if field.name in self.system_metadata_fields: + continue + + if field.name in self._model_data: + fields[field.name] = self._model_data[field.name] + elif field.name in self._inherited_metadata: + fields[field.name] = self._inherited_metadata[field.name] return fields diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index cfce5eb7db..3682dea55a 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -2,6 +2,7 @@ from datetime import datetime from . import ModuleStoreBase, Location, namedtuple_to_son from .exceptions import ItemNotFoundError +from .inheritance import own_metadata import logging DRAFT = 'draft' @@ -181,7 +182,7 @@ class DraftModuleStore(ModuleStoreBase): draft.cms.published_by = published_by_id super(DraftModuleStore, self).update_item(location, draft._model_data._kvs._data) super(DraftModuleStore, self).update_children(location, draft._model_data._kvs._children) - super(DraftModuleStore, self).update_metadata(location, draft._model_data._kvs._metadata) + super(DraftModuleStore, self).update_metadata(location, own_metadata(draft)) self.delete_item(location) def unpublish(self, location): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index f1e09b024a..dbc8b4506c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -171,7 +171,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem): model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location)) module = class_(self, location, model_data) if self.cached_metadata is not None: - metadata_to_inherit = self.cached_metadata.get(location.url(), {}) + # parent container pointers don't differentiate between draft and non-draft + # so when we do the lookup, we should do so with a non-draft location + non_draft_loc = location._replace(revision=None) + metadata_to_inherit = self.cached_metadata.get(non_draft_loc.url(), {}) inherit_metadata(module, metadata_to_inherit) return module except: @@ -277,6 +280,17 @@ class MongoModuleStore(ModuleStoreBase): # now go through the results and order them by the location url for result in resultset: location = Location(result['_id']) + # We need to collate between draft and non-draft + # i.e. draft verticals can have children which are not in non-draft versions + location = location._replace(revision=None) + location_url = location.url() + if location_url in results_by_url: + existing_children = results_by_url[location_url].get('definition', {}).get('children', []) + additional_children = result.get('definition', {}).get('children', []) + total_children = existing_children + additional_children + if 'definition' not in results_by_url[location_url]: + results_by_url[location_url]['definition'] = {} + results_by_url[location_url]['definition']['children'] = total_children results_by_url[location.url()] = result if location.category == 'course': root = location.url() @@ -288,17 +302,12 @@ class MongoModuleStore(ModuleStoreBase): """ Helper method for computing inherited metadata for a specific location url """ - my_metadata = {} # check for presence of metadata key. Note that a given module may not yet be fully formed. # example: update_item -> update_children -> update_metadata sequence on new item create # if we get called here without update_metadata called first then 'metadata' hasn't been set # as we're not fully transactional at the DB layer. Same comment applies to below key name # check my_metadata = results_by_url[url].get('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 diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index 660411384f..532a55792c 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -19,6 +19,13 @@