diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 4f8dcf3ccc..9bce00d650 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/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 = 'boo' # 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( diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 7be3806f5c..20d34d4607 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/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}}) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index bec4e55032..7e2854b042 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index b8770558c3..ff61571126 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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), diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 43d019a836..7783681262 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 49d592bfb5..e3fb461fbc 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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)