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/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 @@