Merge pull request #1820 from MITx/fix/cdodge/draft-metadata-inheritance

Fix/cdodge/draft metadata inheritance

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'])
 

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):

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

common/test/data/simple/course.xml

@@ -19,6 +19,13 @@
       </sequential>
     </section>
     <video name="Lost Video" youtube="1.0:TBvX7HzxexQ"/>
+    <sequential format="Lecture Sequence">
+        <vertical url_name='test_verical'>
+            <html url_name='test_html'>
+                Foobar
+            </html>
+        </vertical>
+    </sequential>
   </chapter>
 
 </course>