Merge pull request #2918 from edx/dhm/meld_pretty_factory

Dhm/meld pretty factory

cms/djangoapps/contentstore/tests/test_crud.py

 import unittest
-from django.conf import settings
 
 from xmodule import templates
 from xmodule.modulestore.tests import persistent_factories
@@ -10,8 +9,6 @@ from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, LocalId
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError
 from xmodule.html_module import HtmlDescriptor
-from xmodule.modulestore import inheritance
-from xblock.core import XBlock
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
@@ -57,14 +54,14 @@ class TemplateTests(unittest.TestCase):
 
     def test_factories(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+            course_id='testx.tempcourse', org='testx',
             display_name='fun test course', user_id='testbot'
         )
         self.assertIsInstance(test_course, CourseDescriptor)
         self.assertEqual(test_course.display_name, 'fun test course')
         index_info = modulestore('split').get_course_index_info(test_course.location)
         self.assertEqual(index_info['org'], 'testx')
-        self.assertEqual(index_info['prettyid'], 'tempcourse')
+        self.assertEqual(index_info['_id'], 'testx.tempcourse')
 
         test_chapter = persistent_factories.ItemFactory.create(display_name='chapter 1',
             parent_location=test_course.location)
@@ -75,31 +72,31 @@ class TemplateTests(unittest.TestCase):
 
         with self.assertRaises(DuplicateCourseError):
             persistent_factories.PersistentCourseFactory.create(
-                course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+                course_id='testx.tempcourse', org='testx', 
                 display_name='fun test course', user_id='testbot'
             )
 
     def test_temporary_xblocks(self):
         """
-        Test using load_from_json to create non persisted xblocks
+        Test create_xblock to create non persisted xblocks
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+            course_id='testx.tempcourse', org='testx',
             display_name='fun test course', user_id='testbot'
         )
 
-        test_chapter = self.load_from_json({'category': 'chapter',
-            'fields': {'display_name': 'chapter n'}},
-            test_course.system, parent_xblock=test_course)
+        test_chapter = modulestore('split').create_xblock(
+            test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course
+        )
         self.assertIsInstance(test_chapter, SequenceDescriptor)
         self.assertEqual(test_chapter.display_name, 'chapter n')
         self.assertIn(test_chapter, test_course.get_children())
 
         # test w/ a definition (e.g., a problem)
         test_def_content = '<problem>boo</problem>'
-        test_problem = self.load_from_json({'category': 'problem',
-            'fields': {'data': test_def_content}},
-            test_course.system, parent_xblock=test_chapter)
+        test_problem = modulestore('split').create_xblock(
+            test_course.system, 'problem', {'data': test_def_content}, parent_xblock=test_chapter
+        )
         self.assertIsInstance(test_problem, CapaDescriptor)
         self.assertEqual(test_problem.data, test_def_content)
         self.assertIn(test_problem, test_chapter.get_children())
@@ -111,20 +108,22 @@ class TemplateTests(unittest.TestCase):
         try saving temporary xblocks
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
-            display_name='fun test course', user_id='testbot')
-        test_chapter = self.load_from_json({'category': 'chapter',
-            'fields': {'display_name': 'chapter n'}},
-            test_course.system, parent_xblock=test_course)
+            course_id='testx.tempcourse', org='testx', 
+            display_name='fun test course', user_id='testbot'
+        )
+        test_chapter = modulestore('split').create_xblock(
+            test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course
+        )
+        self.assertEqual(test_chapter.display_name, 'chapter n')
         test_def_content = '<problem>boo</problem>'
         # create child
-        new_block = self.load_from_json({
-            'category': 'problem',
-            'fields': {
+        new_block = modulestore('split').create_xblock(
+            test_course.system,
+            'problem',
+            fields={
                 'data': test_def_content,
                 'display_name': 'problem'
-            }},
-            test_course.system,
+            },
             parent_xblock=test_chapter
         )
         self.assertIsNotNone(new_block.definition_locator)
@@ -149,7 +148,6 @@ class TemplateTests(unittest.TestCase):
     def test_delete_course(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
             course_id='edu.harvard.history.doomed', org='testx',
-            prettyid='edu.harvard.history.doomed',
             display_name='doomed test course',
             user_id='testbot')
         persistent_factories.ItemFactory.create(display_name='chapter 1',
@@ -173,9 +171,9 @@ class TemplateTests(unittest.TestCase):
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
             course_id='edu.harvard.history.hist101', org='testx',
-            prettyid='edu.harvard.history.hist101',
             display_name='history test course',
-            user_id='testbot')
+            user_id='testbot'
+        )
         chapter = persistent_factories.ItemFactory.create(display_name='chapter 1',
             parent_location=test_course.location, user_id='testbot')
         sub = persistent_factories.ItemFactory.create(display_name='subsection 1',
@@ -242,44 +240,3 @@ class TemplateTests(unittest.TestCase):
         mapper = loc_mapper()
         self.assertEqual(modulestore('split').loc_mapper, mapper)
 
-    # ================================= JSON PARSING ===========================
-    # These are example methods for creating xmodules in memory w/o persisting them.
-    # They were in x_module but since xblock is not planning to support them but will
-    # allow apps to use this type of thing, I put it here.
-    @staticmethod
-    def load_from_json(json_data, system, default_class=None, parent_xblock=None):
-        """
-        This method instantiates the correct subclass of XModuleDescriptor based
-        on the contents of json_data. It does not persist it and can create one which
-        has no usage id.
-
-        parent_xblock is used to compute inherited metadata as well as to append the new xblock.
-
-        json_data:
-        - 'location' : must have this field
-        - 'category': the xmodule category (required or location must be a Location)
-        - 'metadata': a dict of locally set metadata (not inherited)
-        - 'children': a list of children's usage_ids w/in this course
-        - 'definition':
-        - '_id' (optional): the usage_id of this. Will generate one if not given one.
-        """
-        class_ = XBlock.load_class(
-            json_data.get('category', json_data.get('location', {}).get('category')),
-            default_class,
-            select=settings.XBLOCK_SELECT_FUNCTION
-        )
-        usage_id = json_data.get('_id', None)
-        if not '_inherited_settings' in json_data and parent_xblock is not None:
-            json_data['_inherited_settings'] = parent_xblock.xblock_kvs.inherited_settings.copy()
-            json_fields = json_data.get('fields', {})
-            for field_name in inheritance.InheritanceMixin.fields:
-                if field_name in json_fields:
-                    json_data['_inherited_settings'][field_name] = json_fields[field_name]
-
-        new_block = system.xblock_from_json(class_, usage_id, json_data)
-        if parent_xblock is not None:
-            parent_xblock.children.append(new_block.scope_ids.usage_id)
-            # decache pending children field settings
-            parent_xblock.save()
-        return new_block
-

cms/djangoapps/contentstore/views/course.py

@@ -97,7 +97,7 @@ def course_handler(request, tag=None, package_id=None, branch=None, version_guid
         index entry.
     PUT
         json: update this course (index entry not xblock) such as repointing head, changing display name, org,
-        package_id, prettyid. Return same json as above.
+        package_id. Return same json as above.
     DELETE
         json: delete this branch from this course (leaving off /branch/draft would imply delete the course)
     """

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -22,12 +22,14 @@ log = logging.getLogger(__name__)
 
 class LocalId(object):
     """
-    Class for local ids for non-persisted xblocks
-
-    Should be hashable and distinguishable, but nothing else
+    Class for local ids for non-persisted xblocks (which can have hardcoded block_ids if necessary)
     """
+    def __init__(self, block_id=None):
+        self.block_id = block_id
+        super(LocalId, self).__init__()
+
     def __str__(self):
-        return "localid_{}".format(id(self))
+        return "localid_{}".format(self.block_id or id(self))
 
 
 class Locator(object):
@@ -358,8 +360,7 @@ class CourseLocator(Locator):
         Generate a discussion group id based on course
 
         To make compatible with old Location object functionality. I don't believe this behavior fits at this
-        place, but I have no way to override. If this is really needed, it should probably use the pretty_id to seed
-        the name although that's mutable. We should also clearly define the purpose and restrictions of this
+        place, but I have no way to override. We should clearly define the purpose and restrictions of this
         (e.g., I'm assuming periods are fine).
         """
         return self.package_id

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -282,7 +282,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param fields: a dict of xblock field name - value pairs for the course module.
         :param metadata: the old way of setting fields by knowing which ones are scope.settings v scope.content
         :param definition_data: the complement to metadata which is also a subset of fields
-        :param pretty_id: a field split.create_course uses and may quit using
         :returns: course xblock
         """
         store = self.modulestores[store_name]
@@ -297,11 +296,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 org = None
 
             org = kwargs.pop('org', org)
-            pretty_id = kwargs.pop('pretty_id', course_id)
             fields = kwargs.pop('fields', {})
             fields.update(kwargs.pop('metadata', {}))
             fields.update(kwargs.pop('definition_data', {}))
-            course = store.create_course(course_id, org, pretty_id, user_id, fields=fields, **kwargs)
+            course = store.create_course(course_id, org, user_id, fields=fields, **kwargs)
         else:  # assume mongo
             course = store.create_course(course_id, **kwargs)
 

common/lib/xmodule/xmodule/modulestore/split_migrator.py

@@ -47,7 +47,7 @@ class SplitMigrator(object):
         original_course = self.direct_modulestore.get_item(course_location)
         new_course_root_locator = self.loc_mapper.translate_location(old_course_id, course_location)
         new_course = self.split_modulestore.create_course(
-            new_package_id, course_location.org, original_course.display_name,
+            new_package_id, course_location.org, 
             user.id,
             fields=self._get_json_fields_translate_children(original_course, old_course_id, True),
             root_block_id=new_course_root_locator.block_id,

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -5,7 +5,6 @@ Representation:
 * course_index: a dictionary:
     ** '_id': package_id (e.g., myu.mydept.mycourse.myrun),
     ** 'org': the org's id. Only used for searching not identity,
-    ** 'prettyid': a vague to-be-determined field probably more useful to storing searchable tags,
     ** 'edited_by': user_id of user who created the original entry,
     ** 'edited_on': the datetime of the original creation,
     ** 'versions': versions_dict: {branch_id: structure_id, ...}
@@ -99,6 +98,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
     A Mongodb backed ModuleStore supporting versions, inheritance,
     and sharing.
     """
+
+    SCHEMA_VERSION = 1
     reference_type = Locator
     def __init__(self, doc_store_config, fs_root, render_template,
                  default_class=None,
@@ -468,7 +469,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         heads. This function is primarily for test verification but may serve some
         more general purpose.
         :param course_locator: must have a package_id set
-        :return {'org': , 'prettyid': ,
+        :return {'org': string,
             versions: {'draft': the head draft version id,
                 'published': the head published version id if any,
             },
@@ -618,7 +619,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 "edited_on": datetime.datetime.now(UTC),
                 "previous_version": None,
                 "original_version": new_id,
-            }
+            },
+            'schema_version': self.SCHEMA_VERSION,
         }
         self.db_connection.insert_definition(document)
         definition_locator = DefinitionLocator(new_id)
@@ -654,6 +656,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC)
             # previous version id
             old_definition['edit_info']['previous_version'] = definition_locator.definition_id
+            old_definition['schema_version'] = self.SCHEMA_VERSION
             self.db_connection.insert_definition(old_definition)
             return DefinitionLocator(old_definition['_id']), True
         else:
@@ -811,7 +814,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         return self.get_item(item_loc)
 
     def create_course(
-        self, course_id, org, prettyid, user_id, fields=None,
+        self, course_id, org, user_id, fields=None,
         master_branch='draft', versions_dict=None, root_category='course',
         root_block_id='course'
     ):
@@ -870,7 +873,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                     'edited_on': datetime.datetime.now(UTC),
                     'previous_version': None,
                     'original_version': definition_id,
-                }
+                },
+                'schema_version': self.SCHEMA_VERSION,
             }
             self.db_connection.insert_definition(definition_entry)
 
@@ -904,6 +908,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                     definition['edit_info']['edited_by'] = user_id
                     definition['edit_info']['edited_on'] = datetime.datetime.now(UTC)
                     definition['_id'] = ObjectId()
+                    definition['schema_version'] = self.SCHEMA_VERSION
                     self.db_connection.insert_definition(definition)
                     root_block['definition'] = definition['_id']
                     root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC)
@@ -917,10 +922,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         index_entry = {
             '_id': course_id,
             'org': org,
-            'prettyid': prettyid,
             'edited_by': user_id,
             'edited_on': datetime.datetime.now(UTC),
-            'versions': versions_dict}
+            'versions': versions_dict,
+            'schema_version': self.SCHEMA_VERSION,
+        }
         self.db_connection.insert_course_index(index_entry)
         return self.get_course(CourseLocator(package_id=course_id, branch=master_branch))
 
@@ -947,7 +953,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # check children
         original_entry = self._get_block_from_structure(original_structure, descriptor.location.block_id)
         is_updated = is_updated or (
-            descriptor.has_children and original_entry['fields']['children'] != descriptor.children
+            descriptor.has_children and original_entry['fields'].get('children', []) != descriptor.children
         )
         # check metadata
         if not is_updated:
@@ -986,6 +992,40 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             # nothing changed, just return the one sent in
             return descriptor
 
+    def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None):
+        """
+        This method instantiates the correct subclass of XModuleDescriptor based
+        on the contents of json_data. It does not persist it and can create one which
+        has no usage id.
+
+        parent_xblock is used to compute inherited metadata as well as to append the new xblock.
+
+        json_data:
+        - 'category': the xmodule category
+        - 'fields': a dict of locally set fields (not inherited) in json format not pythonic typed format!
+        - 'definition': the object id of the existing definition
+        """
+        xblock_class = runtime.load_block_type(category)
+        json_data = {
+            'category': category,
+            'fields': fields or {},
+        }
+        if definition_id is not None:
+            json_data['definition'] = definition_id
+        if parent_xblock is not None:
+            json_data['_inherited_settings'] = parent_xblock.xblock_kvs.inherited_settings.copy()
+            if fields is not None:
+                for field_name in inheritance.InheritanceMixin.fields:
+                    if field_name in fields:
+                        json_data['_inherited_settings'][field_name] = fields[field_name]
+
+        new_block = runtime.xblock_from_json(xblock_class, block_id, json_data)
+        if parent_xblock is not None:
+            parent_xblock.children.append(new_block.scope_ids.usage_id)
+            # decache pending children field settings
+            parent_xblock.save()
+        return new_block
+
     def persist_xblock_dag(self, xblock, user_id, force=False):
         """
         create or update the xblock and all of its children. The xblock's location must specify a course.
@@ -1044,8 +1084,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             # generate an id
             is_new = True
             is_updated = True
-            block_id = self._generate_block_id(structure_blocks, xblock.category)
-            encoded_block_id = block_id
+            block_id = getattr(xblock.scope_ids.usage_id.block_id, 'block_id', None)
+            if block_id is None:
+                block_id = self._generate_block_id(structure_blocks, xblock.category)
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id)
             xblock.scope_ids.usage_id.block_id = block_id
         else:
             is_new = False
@@ -1448,6 +1490,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         new_structure['previous_version'] = structure['_id']
         new_structure['edited_by'] = user_id
         new_structure['edited_on'] = datetime.datetime.now(UTC)
+        new_structure['schema_version'] = self.SCHEMA_VERSION
         return new_structure
 
     def _find_local_root(self, element_to_find, possibility, tree):
@@ -1508,7 +1551,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             'original_version': new_id,
             'edited_by': user_id,
             'edited_on': datetime.datetime.now(UTC),
-            'blocks': blocks
+            'blocks': blocks,
+            'schema_version': self.SCHEMA_VERSION,
         }
 
     def _get_parents_from_structure(self, block_id, structure):

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

@@ -24,7 +24,6 @@ class PersistentCourseFactory(SplitFactory):
     keywords: any xblock field plus (note, the below are filtered out; so, if they
     become legitimate xblock fields, they won't be settable via this factory)
     * org: defaults to textX
-    * prettyid: defaults to 999
     * master_branch: (optional) defaults to 'draft'
     * user_id: (optional) defaults to 'test_user'
     * display_name (xblock field): will default to 'Robot Super Course' unless provided
@@ -33,14 +32,14 @@ class PersistentCourseFactory(SplitFactory):
 
     # pylint: disable=W0613
     @classmethod
-    def _create(cls, target_class, course_id='testX.999', org='testX', prettyid='999', user_id='test_user',
+    def _create(cls, target_class, course_id='testX.999', org='testX', user_id='test_user',
                 master_branch='draft', **kwargs):
 
         modulestore = kwargs.pop('modulestore')
         root_block_id = kwargs.pop('root_block_id', 'course')
         # Write the data to the mongo datastore
         new_course = modulestore.create_course(
-            course_id, org, prettyid, user_id, fields=kwargs,
+            course_id, org, user_id, fields=kwargs,
             master_branch=master_branch, root_block_id=root_block_id
         )
 

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

@@ -114,7 +114,7 @@ class TestOrphan(unittest.TestCase):
         fields.update(data)
         # split requires the course to be created separately from creating items
         self.split_mongo.create_course(
-            self.split_package_id, 'test_org', 'my course', self.userid, fields=fields, root_block_id='runid'
+            self.split_package_id, 'test_org', self.userid, fields=fields, root_block_id='runid'
         )
         self.course_location = Location('i4x', 'test_org', 'test_course', 'course', 'runid')
         self.old_mongo.create_and_save_xmodule(self.course_location, data, metadata)

common/test/data/splitmongo_json/active_versions.json

-[{"_id" : "GreekHero", 
-  "org" : "testx", 
-  "prettyid" : "test_course", 
-  "versions" : {
-    "draft" : { "$oid" : "1d00000000000000dddd0000" }
-   },
-  "edited_on" : {"$date" : 1364481713238},
-  "edited_by" : "test@edx.org"},
-
- {"_id" : "wonderful", 
-  "org" : "testx", 
-  "prettyid" : "another_course", 
-  "versions" : {
-    "draft" : { "$oid" : "1d00000000000000dddd2222" },
-    "published" : { "$oid" : "1d00000000000000eeee0000" }
-    },
-  "edited_on" : {"$date" : 1364481313238},
-  "edited_by" : "test@edx.org"},
-  
- {"_id" : "contender", 
-  "org" : "guestx", 
-  "prettyid" : "test_course", 
-  "versions" : {
-        "draft" : { "$oid" : "1d00000000000000dddd5555" }},
-  "edited_on" : {"$date" : 1364491313238},
-  "edited_by" : "test@guestx.edu"}
-]