Improve test coverage

and fix found errors.

cms/djangoapps/contentstore/tests/test_crud.py

@@ -95,15 +95,20 @@ class TemplateTests(unittest.TestCase):
         """
         try saving temporary xblocks
         """
-        test_course = persistent_factories.PersistentCourseFactory.create(org='testx', prettyid='tempcourse',
+        test_course = persistent_factories.PersistentCourseFactory.create(
+            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)
         test_def_content = '<problem>boo</problem>'
         # create child
-        _ = self.load_from_json({'category': 'problem',
-            'fields': {'data': test_def_content}},
+        self.load_from_json({
+            'category': 'problem',
+            'fields': {
+                'data': test_def_content,
+                'display_name': 'problem'
+            }},
             test_course.system, parent_xblock=test_chapter)
         # better to pass in persisted parent over the subdag so
         # subdag gets the parent pointer (otherwise 2 ops, persist dag, update parent children,
@@ -117,6 +122,10 @@ class TemplateTests(unittest.TestCase):
         persisted_problem = persisted_chapter.get_children()[0]
         self.assertEqual(persisted_problem.category, 'problem')
         self.assertEqual(persisted_problem.data, test_def_content)
+        # update it
+        persisted_problem.display_name = 'altered problem'
+        persisted_problem = modulestore('split').persist_xblock_dag(persisted_problem, 'testbot')
+        self.assertEqual(persisted_problem.display_name, 'altered problem')
 
     def test_delete_course(self):
         test_course = persistent_factories.PersistentCourseFactory.create(

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -259,7 +259,9 @@ class LocMapperStore(object):
             # if there are few == org/course roots or their content is unrelated, this will work well.
             block_id = self._verify_uniqueness(location.category + location.name[:3], block_map)
         else:
-            block_id = location.name
+            # if 2 different category locations had same name, then they'll collide. Make the later
+            # mapped ones unique
+            block_id = self._verify_uniqueness(location.name, block_map)
         encoded_location_name = self._encode_for_mongo(location.name)
         block_map.setdefault(encoded_location_name, {})[location.category] = block_id
         self.location_map.update(location_id, {'$set': {'block_map': block_map}})

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

@@ -298,8 +298,8 @@ class CourseLocator(Locator):
         self._set_value(
             parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid))
         )
-        self._set_value(parse, 'course_id', lambda (new_id): self.set_course_id(new_id))
-        self._set_value(parse, 'branch', lambda (new_branch): self.set_branch(new_branch))
+        self._set_value(parse, 'course_id', self.set_course_id)
+        self._set_value(parse, 'branch', self.set_branch)
 
     def init_from_version_guid(self, version_guid):
         """
@@ -448,7 +448,7 @@ class BlockUsageLocator(CourseLocator):
                                      branch=self.branch,
                                      block_id=self.block_id)
 
-    def set_usage_id(self, new):
+    def set_block_id(self, new):
         """
         Initialize block_id to new value.
         If block_id has already been initialized to a different value, raise an exception.
@@ -457,12 +457,12 @@ class BlockUsageLocator(CourseLocator):
 
     def init_block_ref(self, block_ref):
         if isinstance(block_ref, LocalId):
-            self.set_usage_id(block_ref)
+            self.set_block_id(block_ref)
         else:
             parse = parse_block_ref(block_ref)
             if not parse:
                 raise ValueError('Could not parse "%s" as a block_ref' % block_ref)
-            self.set_usage_id(parse['block'])
+            self.set_block_id(parse['block'])
 
     def init_block_ref_from_str(self, value):
         """
@@ -476,17 +476,16 @@ class BlockUsageLocator(CourseLocator):
         parse = parse_url(value, tag_optional=True)
         if parse is None:
             raise ValueError('Could not parse "%s" as a url' % value)
-        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
+        self._set_value(parse, 'block', self.set_block_id)
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
-            # FIXME the parsed course_id should never contain a block ref
             course_id = course_id.course_id
             assert course_id, "%s does not have a valid course_id"
         parse = parse_course_id(course_id)
         if parse is None:
             raise ValueError('Could not parse "%s" as a course_id' % course_id)
-        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
+        self._set_value(parse, 'block', self.set_block_id)
 
     def __unicode__(self):
         """

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

@@ -945,9 +945,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             descriptor.definition_locator, descriptor.get_explicitly_set_fields_by_scope(Scope.content), user_id)
         # check children
         original_entry = original_structure['blocks'][descriptor.location.block_id]
-        if (not is_updated and descriptor.has_children
-            and not self._xblock_lists_equal(original_entry['fields']['children'], descriptor.children)):
-            is_updated = True
+        is_updated = is_updated or (
+            descriptor.has_children and original_entry['fields']['children'] != descriptor.children
+        )
         # check metadata
         if not is_updated:
             is_updated = self._compare_settings(
@@ -963,7 +963,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             block_data["definition"] = descriptor.definition_locator.definition_id
             block_data["fields"] = descriptor.get_explicitly_set_fields_by_scope(Scope.settings)
             if descriptor.has_children:
-                block_data['fields']["children"] = [self._block_id(child) for child in descriptor.children]
+                block_data['fields']["children"] = descriptor.children
 
             new_id = new_structure['_id']
             block_data['edit_info'] = {
@@ -1048,9 +1048,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         else:
             is_new = False
             block_id = xblock.location.block_id
-            if (not is_updated and xblock.has_children
-                and not self._xblock_lists_equal(structure_blocks[block_id]['fields']['children'], xblock.children)):
-                is_updated = True
+            is_updated = is_updated or (
+                xblock.has_children and structure_blocks[block_id]['fields']['children'] != xblock.children
+            )
 
         children = []
         if xblock.has_children:
@@ -1229,8 +1229,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             parent_block['edit_info']['previous_version'] = parent_block['edit_info']['update_version']
             parent_block['edit_info']['update_version'] = new_id
 
-        # remove subtree
         def remove_subtree(block_id):
+            """
+            Remove the subtree rooted at block_id
+            """
             for child in new_blocks[block_id]['fields'].get('children', []):
                 remove_subtree(child)
             del new_blocks[block_id]
@@ -1390,32 +1392,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         else:
             return criteria == target
 
-    def _xblock_lists_equal(self, lista, listb):
-        """
-        Do the 2 lists refer to the same xblocks in the same order (presumes they're from the
-        same course)
-
-        :param lista:
-        :param listb:
-        """
-        if len(lista) != len(listb):
-            return False
-        for ele_a, ele_b in zip(lista, listb):
-            if ele_a != ele_b:
-                if self._block_id(ele_a) != self._block_id(ele_b):
-                    return False
-        return True
-
-    def _block_id(self, xblock_or_id):
-        """
-        arg is either an xblock or an id. If an xblock, get the block_id from its location. Otherwise, return itself.
-        :param xblock_or_id:
-        """
-        if isinstance(xblock_or_id, XModuleDescriptor):
-            return xblock_or_id.location.block_id
-        else:
-            return xblock_or_id
-
     def _get_index_if_valid(self, locator, force=False, continue_version=False):
         """
         If the locator identifies a course and points to its draft (or plausibly its draft),

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

@@ -75,7 +75,6 @@ class TestLocationMapper(unittest.TestCase):
         self.assertEqual(entry['prod_branch'], 'live')
         self.assertEqual(entry['block_map'], block_map)
 
-
     def translate_n_check(self, location, old_style_course_id, new_style_course_id, block_id, branch, add_entry=False):
         """
         Request translation, check course_id, block_id, and branch
@@ -348,6 +347,25 @@ class TestLocationMapper(unittest.TestCase):
         reverted_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(location, reverted_location)
 
+    def test_name_collision(self):
+        """
+        Test dwim translation when the old name was not unique
+        """
+        org = "myorg"
+        course = "another_course"
+        name = "running_again"
+        course_location = Location('i4x', org, course, 'course', name)
+        course_xlate = loc_mapper().translate_location(None, course_location, add_entry_if_missing=True)
+        self.assertEqual(course_location, loc_mapper().translate_locator_to_location(course_xlate))
+        eponymous_block = course_location.replace(category='chapter')
+        chapter_xlate = loc_mapper().translate_location(None, eponymous_block, add_entry_if_missing=True)
+        self.assertEqual(course_location, loc_mapper().translate_locator_to_location(course_xlate))
+        self.assertEqual(eponymous_block, loc_mapper().translate_locator_to_location(chapter_xlate))
+        # and a non-existent one w/o add
+        eponymous_block = course_location.replace(category='problem')
+        with self.assertRaises(ItemNotFoundError):
+            chapter_xlate = loc_mapper().translate_location(None, eponymous_block, add_entry_if_missing=False)
+
 
 #==================================
 # functions to mock existing services

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

@@ -235,6 +235,16 @@ class SplitModuleCourseTests(SplitModuleTest):
                           modulestore().get_course,
                           CourseLocator(course_id='GreekHero', branch='published'))
 
+    def test_cache(self):
+        """
+        Test that the mechanics of caching work.
+        """
+        locator = CourseLocator(version_guid=self.GUID_D0)
+        course = modulestore().get_course(locator)
+        block_map = modulestore().cache_items(course.system, course.children, depth=3)
+        self.assertIn('chapter1', block_map)
+        self.assertIn('problem3_2', block_map)
+
     def test_course_successors(self):
         """
         get_course_successors(course_locator, version_history_depth=1)