generalize draft import/export TNL-574

add test for videomodule

Import drafts handles modules w/o xml_attributes
and ones using the old parent_sequential_url pointer

Handle drafts for modules which don't define nor extract xml_attributes

Support  and  in get_items qualifiers

Unit test for   value query

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -250,6 +250,14 @@ class ImportRequiredTestCases(ContentStoreTestCase):
         # check for about content
         self.verify_content_existence(self.store, root_dir, course_id, 'about', 'about', '.html')
 
+        # assert that there is an html and video directory in drafts:
+        draft_dir = OSFS(root_dir / 'test_export/drafts')
+        self.assertTrue(draft_dir.exists('html'))
+        self.assertTrue(draft_dir.exists('video'))
+        # and assert that they contain the created modules
+        self.assertIn(self.DRAFT_HTML + ".xml", draft_dir.listdir('html'))
+        self.assertIn(self.DRAFT_VIDEO + ".xml", draft_dir.listdir('video'))
+
         # check for grading_policy.json
         filesystem = OSFS(root_dir / 'test_export/policies/2012_Fall')
         self.assertTrue(filesystem.exists('grading_policy.json'))
@@ -302,6 +310,7 @@ class ImportRequiredTestCases(ContentStoreTestCase):
             """Verifies all temporary attributes added during export are removed"""
             self.assertNotIn('index_in_children_list', attributes)
             self.assertNotIn('parent_sequential_url', attributes)
+            self.assertNotIn('parent_url', attributes)
 
         vertical = self.store.get_item(course_id.make_usage_key('vertical', self.TEST_VERTICAL))
         verify_export_attrs_removed(vertical.xml_attributes)

cms/djangoapps/contentstore/tests/test_import_pure_xblock.py

@@ -7,7 +7,6 @@ from xblock.fields import String
 
 from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.mongo.draft import as_draft
 
 

cms/djangoapps/contentstore/tests/utils.py

@@ -128,6 +128,8 @@ class CourseTestCase(ModuleStoreTestCase):
     PRIVATE_VERTICAL = 'a_private_vertical'
     PUBLISHED_VERTICAL = 'a_published_vertical'
     SEQUENTIAL = 'vertical_sequential'
+    DRAFT_HTML = 'draft_html'
+    DRAFT_VIDEO = 'draft_video'
     LOCKED_ASSET_KEY = AssetLocation.from_deprecated_string('/c4x/edX/toy/asset/sample_static.txt')
 
     def import_and_populate_course(self):
@@ -167,6 +169,20 @@ class CourseTestCase(ModuleStoreTestCase):
         sequential.children.append(public_vertical.location)
         self.store.update_item(sequential, self.user.id)
 
+        # create an html and video component to make drafts:
+        draft_html = self.store.create_item(self.user.id, course_id, 'html', self.DRAFT_HTML)
+        draft_video = self.store.create_item(self.user.id, course_id, 'video', self.DRAFT_VIDEO)
+
+        # add them as children to the public_vertical
+        public_vertical.children.append(draft_html.location)
+        public_vertical.children.append(draft_video.location)
+        self.store.update_item(public_vertical, self.user.id)
+        # publish changes to vertical
+        self.store.publish(public_vertical.location, self.user.id)
+        # convert html/video to draft
+        self.store.convert_to_draft(draft_html.location, self.user.id)
+        self.store.convert_to_draft(draft_video.location, self.user.id)
+
         # lock an asset
         content_store.set_attr(self.LOCKED_ASSET_KEY, 'locked', True)
 
@@ -199,18 +215,25 @@ class CourseTestCase(ModuleStoreTestCase):
             self.assertEqual(self.store.has_published_version(item), publish_state)
 
         def get_and_verify_publish_state(item_type, item_name, publish_state):
-            """Gets the given item from the store and verifies the publish state of the item is as expected."""
+            """
+            Gets the given item from the store and verifies the publish state
+            of the item is as expected.
+            """
             item = self.store.get_item(course_id.make_usage_key(item_type, item_name))
             verify_item_publish_state(item, publish_state)
             return item
 
-        # verify that the draft vertical is draft
+        # verify draft vertical has a published version with published children
         vertical = get_and_verify_publish_state('vertical', self.TEST_VERTICAL, True)
         for child in vertical.get_children():
             verify_item_publish_state(child, True)
 
-        # make sure that we don't have a sequential that is not in draft mode
+        # verify that it has a draft too
+        self.assertTrue(getattr(vertical, "is_draft", False))
+
+        # make sure that we don't have a sequential that is in draft mode
         sequential = get_and_verify_publish_state('sequential', self.SEQUENTIAL, True)
+        self.assertFalse(getattr(sequential, "is_draft", False))
 
         # verify that we have the private vertical
         private_vertical = get_and_verify_publish_state('vertical', self.PRIVATE_VERTICAL, False)
@@ -218,10 +241,24 @@ class CourseTestCase(ModuleStoreTestCase):
         # verify that we have the public vertical
         public_vertical = get_and_verify_publish_state('vertical', self.PUBLISHED_VERTICAL, True)
 
+        # verify that we have the draft html
+        draft_html = self.store.get_item(course_id.make_usage_key('html', self.DRAFT_HTML))
+        self.assertTrue(getattr(draft_html, 'is_draft', False))
+
+        # verify that we have the draft video
+        draft_video = self.store.get_item(course_id.make_usage_key('video', self.DRAFT_VIDEO))
+        self.assertTrue(getattr(draft_video, 'is_draft', False))
+
         # verify verticals are children of sequential
         for vert in [vertical, private_vertical, public_vertical]:
             self.assertIn(vert.location, sequential.children)
 
+        # verify draft html is the child of the public vertical
+        self.assertIn(draft_html.location, public_vertical.children)
+
+        # verify draft video is the child of the public vertical
+        self.assertIn(draft_video.location, public_vertical.children)
+
         # verify textbook exists
         course = self.store.get_course(course_id)
         self.assertGreater(len(course.textbooks), 0)

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -382,6 +382,7 @@ class ModuleStoreRead(object):
         If target is a list, do any of the list elements meet the criteria
         If the criteria is a regex, does the target match it?
         If the criteria is a function, does invoking it on the target yield something truthy?
+        If criteria is a dict {($nin|$in): []}, then do (none|any) of the list elements meet the criteria
         Otherwise, is the target == criteria
         '''
         if isinstance(target, list):
@@ -390,6 +391,12 @@ class ModuleStoreRead(object):
             return criteria.search(target) is not None
         elif callable(criteria):
             return criteria(target)
+        elif isinstance(criteria, dict) and '$in' in criteria:
+            # note isn't handling any other things in the dict other than in
+            return any(self._value_matches(target, test_val) for test_val in criteria['$in'])
+        elif isinstance(criteria, dict) and '$nin' in criteria:
+            # note isn't handling any other things in the dict other than nin
+            return not any(self._value_matches(target, test_val) for test_val in criteria['$nin'])
         else:
             return criteria == target
 

common/lib/xmodule/xmodule/modulestore/store_utilities.py

 import re
 import logging
+from collections import namedtuple
 
 import uuid
 
@@ -71,3 +72,30 @@ def rewrite_nonportable_content_links(source_course_id, dest_course_id, text):
             logging.warning("Error producing regex substitution %r for text = %r.\n\nError msg = %s", source_course_id, text, str(exc))
 
     return text
+
+
+def draft_node_constructor(module, url, parent_url, location=None, parent_location=None, index=None):
+    """
+    Contructs a draft_node namedtuple with defaults.
+    """
+    draft_node = namedtuple('draft_node', ['module', 'location', 'url', 'parent_location', 'parent_url', 'index'])
+    return draft_node(module, location, url, parent_location, parent_url, index)
+
+
+def get_draft_subtree_roots(draft_nodes):
+    """
+    Takes a list of draft_nodes, which are namedtuples, each of which identify
+    itself and its parent.
+
+    If a draft_node is in `draft_nodes`, then we expect for all its children
+    should be in `draft_nodes` as well. Since `_import_draft` is recursive,
+    we only want to import the roots of any draft subtrees contained in
+    `draft_nodes`.
+
+    This generator yields those roots.
+    """
+    urls = [draft_node.url for draft_node in draft_nodes]
+
+    for draft_node in draft_nodes:
+        if draft_node.parent_url not in urls:
+            yield draft_node

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py

@@ -884,6 +884,11 @@ class SplitModuleItemTests(SplitModuleTest):
         self.assertFalse(modulestore()._value_matches('I need some help', re.compile(r'Help')))
         self.assertTrue(modulestore()._value_matches(['I need some help', 'today'], re.compile(r'Help', re.IGNORECASE)))
 
+        self.assertTrue(modulestore()._value_matches('gotcha', {'$in': ['a', 'bunch', 'of', 'gotcha']}))
+        self.assertFalse(modulestore()._value_matches('gotcha', {'$in': ['a', 'bunch', 'of', 'gotchas']}))
+        self.assertFalse(modulestore()._value_matches('gotcha', {'$nin': ['a', 'bunch', 'of', 'gotcha']}))
+        self.assertTrue(modulestore()._value_matches('gotcha', {'$nin': ['a', 'bunch', 'of', 'gotchas']}))
+
         self.assertTrue(modulestore()._block_matches({'a': 1, 'b': 2}, {'a': 1}))
         self.assertFalse(modulestore()._block_matches({'a': 1, 'b': 2}, {'a': 2}))
         self.assertFalse(modulestore()._block_matches({'a': 1, 'b': 2}, {'c': 1}))

common/lib/xmodule/xmodule/modulestore/tests/test_store_utilities.py

+"""
+Tests for store_utilities.py
+"""
+import unittest
+from mock import Mock
+import ddt
+
+from xmodule.modulestore.store_utilities import (
+    get_draft_subtree_roots, draft_node_constructor
+)
+
+
+@ddt.ddt
+class TestUtils(unittest.TestCase):
+    """
+    Tests for store_utilities
+
+    ASCII trees for ONLY_ROOTS and SOME_TREES:
+
+    ONLY_ROOTS:
+    1)
+        vertical (not draft)
+          |
+        url1
+
+    2)
+        sequential (not draft)
+          |
+        url2
+
+    SOME_TREES:
+
+    1)
+            sequential_1 (not draft)
+                 |
+            vertical_1
+              /     \
+             /       \
+        child_1    child_2
+
+
+    2)
+        great_grandparent_vertical (not draft)
+                    |
+            grandparent_vertical
+                    |
+                vertical_2
+                 /      \
+                /        \
+            child_3    child_4
+    """
+
+    ONLY_ROOTS = [
+        draft_node_constructor(Mock(), 'url1', 'vertical'),
+        draft_node_constructor(Mock(), 'url2', 'sequential'),
+    ]
+    ONLY_ROOTS_URLS = ['url1', 'url2']
+
+    SOME_TREES = [
+        draft_node_constructor(Mock(), 'child_1', 'vertical_1'),
+        draft_node_constructor(Mock(), 'child_2', 'vertical_1'),
+        draft_node_constructor(Mock(), 'vertical_1', 'sequential_1'),
+
+        draft_node_constructor(Mock(), 'child_3', 'vertical_2'),
+        draft_node_constructor(Mock(), 'child_4', 'vertical_2'),
+        draft_node_constructor(Mock(), 'vertical_2', 'grandparent_vertical'),
+        draft_node_constructor(Mock(), 'grandparent_vertical', 'great_grandparent_vertical'),
+    ]
+
+    SOME_TREES_ROOTS_URLS = ['vertical_1', 'grandparent_vertical']
+
+    @ddt.data(
+        (ONLY_ROOTS, ONLY_ROOTS_URLS),
+        (SOME_TREES, SOME_TREES_ROOTS_URLS),
+    )
+    @ddt.unpack
+    def test_get_draft_subtree_roots(self, module_nodes, expected_roots_urls):
+        """tests for get_draft_subtree_roots"""
+        subtree_roots_urls = [root.url for root in get_draft_subtree_roots(module_nodes)]
+        # check that we return the expected urls
+        self.assertEqual(set(subtree_roots_urls), set(expected_roots_urls))

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -9,6 +9,7 @@ from xmodule.contentstore.content import StaticContent
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore import EdxJSONEncoder, ModuleStoreEnum
 from xmodule.modulestore.inheritance import own_metadata
+from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots
 from fs.osfs import OSFS
 from json import dumps
 import json
@@ -109,36 +110,55 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir):
         #### DRAFTS ####
         # xml backed courses don't support drafts!
         if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml:
-            # NOTE: this code assumes that verticals are the top most draftable container
-            # should we change the application, then this assumption will no longer be valid
             # NOTE: we need to explicitly implement the logic for setting the vertical's parent
             # and index here since the XML modulestore cannot load draft modules
             with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
-                draft_verticals = modulestore.get_items(
+                draft_modules = modulestore.get_items(
                     course_key,
-                    qualifiers={'category': 'vertical'},
+                    qualifiers={'category': {'$nin': DIRECT_ONLY_CATEGORIES}},
                     revision=ModuleStoreEnum.RevisionOption.draft_only
                 )
 
-                if len(draft_verticals) > 0:
+                if draft_modules:
                     draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
-                    for draft_vertical in draft_verticals:
+
+                    # accumulate tuples of draft_modules and their parents in
+                    # this list:
+                    draft_node_list = []
+
+                    for draft_module in draft_modules:
                         parent_loc = modulestore.get_parent_location(
-                            draft_vertical.location,
+                            draft_module.location,
                             revision=ModuleStoreEnum.RevisionOption.draft_preferred
                         )
                         # Don't try to export orphaned items.
                         if parent_loc is not None:
                             logging.debug('parent_loc = {0}'.format(parent_loc))
-                            if parent_loc.category in DIRECT_ONLY_CATEGORIES:
-                                draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string()
-                                sequential = modulestore.get_item(parent_loc)
-                                index = sequential.children.index(draft_vertical.location)
-                                draft_vertical.xml_attributes['index_in_children_list'] = str(index)
-                            draft_vertical.runtime.export_fs = draft_course_dir
-                            adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir)
-                            node = lxml.etree.Element('unknown')
-                            draft_vertical.add_xml_to_node(node)
+                            draft_node = draft_node_constructor(
+                                draft_module,
+                                location=draft_module.location,
+                                url=draft_module.location.to_deprecated_string(),
+                                parent_location=parent_loc,
+                                parent_url=parent_loc.to_deprecated_string(),
+                            )
+
+                            draft_node_list.append(draft_node)
+
+                    for draft_node in get_draft_subtree_roots(draft_node_list):
+                        # only export the roots of the draft subtrees
+                        # since export_from_xml (called by `add_xml_to_node`)
+                        # exports a whole tree
+
+                        draft_node.module.xml_attributes['parent_url'] = draft_node.parent_url
+                        parent = modulestore.get_item(draft_node.parent_location)
+                        index = parent.children.index(draft_node.module.location)
+                        draft_node.module.xml_attributes['index_in_children_list'] = str(index)
+
+                        draft_node.module.runtime.export_fs = draft_course_dir
+                        adapt_references(draft_node.module, xml_centric_course_key, draft_course_dir)
+                        node = lxml.etree.Element('unknown')
+
+                        draft_node.module.add_xml_to_node(node)
 
 
 def adapt_references(subtree, destination_course_key, export_fs):

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -42,7 +42,8 @@ from xmodule.modulestore.django import ASSET_IGNORE_REGEX
 from xmodule.modulestore.exceptions import DuplicateCourseError
 from xmodule.modulestore.mongo.base import MongoRevisionKey
 from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots
+
 
 log = logging.getLogger(__name__)
 
@@ -438,6 +439,8 @@ def _import_module_and_update_references(
                 value = field.read_from(module)
                 # remove any export/import only xml_attributes
                 # which are used to wire together draft imports
+                if 'parent_url' in value:
+                    del value['parent_url']
                 if 'parent_sequential_url' in value:
                     del value['parent_sequential_url']
 
@@ -500,28 +503,26 @@ def _import_course_draft(
         # to ensure that pure XBlock field data is updated correctly.
         _update_module_location(module, module_location.replace(revision=MongoRevisionKey.draft))
 
+        parent_url = get_parent_url(module)
+        index = index_in_children_list(module)
+
         # make sure our parent has us in its list of children
-        # this is to make sure private only verticals show up
+        # this is to make sure private only modules show up
         # in the list of children since they would have been
         # filtered out from the non-draft store export.
-        # Note though that verticals nested below the unit level will not have
-        # a parent_sequential_url and do not need special handling.
-        if module.location.category == 'vertical' and 'parent_sequential_url' in module.xml_attributes:
-            sequential_url = module.xml_attributes['parent_sequential_url']
-            index = int(module.xml_attributes['index_in_children_list'])
-
+        if parent_url is not None and index is not None:
             course_key = descriptor.location.course_key
-            seq_location = course_key.make_usage_key_from_deprecated_string(sequential_url)
+            parent_location = course_key.make_usage_key_from_deprecated_string(parent_url)
 
-            # IMPORTANT: Be sure to update the sequential in the NEW namespace
-            seq_location = seq_location.map_into_course(target_course_id)
+            # IMPORTANT: Be sure to update the parent in the NEW namespace
+            parent_location = parent_location.map_into_course(target_course_id)
 
-            sequential = store.get_item(seq_location, depth=0)
+            parent = store.get_item(parent_location, depth=0)
 
             non_draft_location = module.location.map_into_course(target_course_id)
-            if not any(child.block_id == module.location.block_id for child in sequential.children):
-                sequential.children.insert(index, non_draft_location)
-                store.update_item(sequential, user_id)
+            if not any(child.block_id == module.location.block_id for child in parent.children):
+                parent.children.insert(index, non_draft_location)
+                store.update_item(parent, user_id)
 
         _import_module_and_update_references(
             module, store, user_id,
@@ -537,8 +538,8 @@ def _import_course_draft(
 
     # First it is necessary to order the draft items by their desired index in the child list
     # (order os.walk returns them in is not guaranteed).
-    drafts = dict()
-    for dirname, _dirnames, filenames in os.walk(draft_dir + "/vertical"):
+    drafts = []
+    for dirname, _dirnames, filenames in os.walk(draft_dir):
         for filename in filenames:
             module_path = os.path.join(dirname, filename)
             with open(module_path, 'r') as f:
@@ -593,23 +594,27 @@ def _import_course_draft(
                         filename, __ = os.path.splitext(filename)
                         descriptor.location = descriptor.location.replace(name=filename)
 
-                        index = int(descriptor.xml_attributes['index_in_children_list'])
-                        if index in drafts:
-                            drafts[index].append(descriptor)
-                        else:
-                            drafts[index] = [descriptor]
+                        index = index_in_children_list(descriptor)
+                        parent_url = get_parent_url(descriptor, xml)
+                        draft_url = descriptor.location.to_deprecated_string()
+
+                        draft = draft_node_constructor(
+                            module=descriptor, url=draft_url, parent_url=parent_url, index=index
+                        )
+
+                        drafts.append(draft)
 
-                except Exception:
+                except Exception:  # pylint: disable=W0703
                     logging.exception('Error while parsing course xml.')
 
-        # For each index_in_children_list key, there is a list of vertical descriptors.
+    # sort drafts by `index_in_children_list` attribute
+    drafts.sort(key=lambda x: x.index)
 
-        for key in sorted(drafts.iterkeys()):
-            for descriptor in drafts[key]:
-                try:
-                    _import_module(descriptor)
-                except Exception:
-                    logging.exception('while importing draft descriptor %s', descriptor)
+    for draft in get_draft_subtree_roots(drafts):
+        try:
+            _import_module(draft.module)
+        except Exception:  # pylint: disable=W0703
+            logging.exception('while importing draft descriptor %s', draft.module)
 
 
 def allowed_metadata_by_category(category):
@@ -648,6 +653,56 @@ def check_module_metadata_editability(module):
     return err_cnt
 
 
+def get_parent_url(module, xml=None):
+    """
+    Get the parent_url, if any, from module using xml as an alternative source. If it finds it in
+    xml but not on module, it modifies module so that the next call to this w/o the xml will get the parent url
+    """
+    if hasattr(module, 'xml_attributes'):
+        return module.xml_attributes.get(
+            # handle deprecated old attr
+            'parent_url', module.xml_attributes.get('parent_sequential_url')
+        )
+    if xml is not None:
+        create_xml_attributes(module, xml)
+        return get_parent_url(module)  # don't reparse xml b/c don't infinite recurse but retry above lines
+    return None
+
+
+def index_in_children_list(module, xml=None):
+    """
+    Get the index_in_children_list, if any, from module using xml
+    as an alternative source. If it finds it in xml but not on module,
+    it modifies module so that the next call to this w/o the xml
+    will get the field.
+    """
+    if hasattr(module, 'xml_attributes'):
+        val = module.xml_attributes.get('index_in_children_list')
+        if val is not None:
+            return int(val)
+        return None
+    if xml is not None:
+        create_xml_attributes(module, xml)
+        return index_in_children_list(module)  # don't reparse xml b/c don't infinite recurse but retry above lines
+    return None
+
+
+def create_xml_attributes(module, xml):
+    """
+    Make up for modules which don't define xml_attributes by creating them here and populating
+    """
+    xml_attrs = {}
+    for attr, val in xml.attrib.iteritems():
+        if attr not in module.fields:
+            # translate obsolete attr
+            if attr == 'parent_sequential_url':
+                attr = 'parent_url'
+            xml_attrs[attr] = val
+
+    # now cache it on module where it's expected
+    setattr(module, 'xml_attributes', xml_attrs)
+
+
 def validate_no_non_editable_metadata(module_store, course_id, category):
     err_cnt = 0
     for module_loc in module_store.modules[course_id]:

common/lib/xmodule/xmodule/tests/xml/test_inheritance.py

@@ -39,7 +39,7 @@ class TestInheritedFieldParsing(XModuleXmlImportTest):
             parent=sequence,
             tag='video',
             attribs={
-                'parent_sequential_url': 'foo', 'garbage': 'asdlk',
+                'parent_url': 'foo', 'garbage': 'asdlk',
                 'download_video': 'true',
             }
         )