get metadata inheritance to work with draft stores. Fix bug on .publish() method…

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

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/mako_module.py

 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

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>