From fa95d323c68540c0e3018eeab688de5ab283aedd Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Thu, 2 Apr 2015 21:33:56 +0000 Subject: [PATCH] Tested version handling, modified modulestore when it didn't work. --- .../contentstore/tests/test_libraries.py | 39 +++++++++++++++++++ cms/djangoapps/contentstore/views/item.py | 2 +- common/lib/xmodule/xmodule/library_tools.py | 18 +++++++-- .../xmodule/modulestore/split_mongo/split.py | 15 +++---- .../modulestore/split_mongo/split_draft.py | 6 ++- 5 files changed, 67 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index b484c74a09..8417cd5ea8 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -747,6 +747,9 @@ class TestOverrides(LibraryTestCase): publish_item=False, ) + # Refresh library now that we've added something. + self.library = modulestore().get_library(self.lib_key) + # Also create a course: with modulestore().default_store(ModuleStoreEnum.Type.split): self.course = CourseFactory.create() @@ -862,6 +865,42 @@ class TestOverrides(LibraryTestCase): self.assertEqual(self.problem_in_course.weight, new_weight) self.assertEqual(self.problem_in_course.data, new_data_value) + def test_duplicated_version(self): + """ + Test that if a library is updated, and the content block is duplicated, + the new block will use the old library version and not the new one. + """ + self.assertEqual(len(self.library.children), 1) + self.assertEqual(len(self.lc_block.children), 1) + + # Create an additional problem block in the library: + self.problem = ItemFactory.create( + category="problem", + parent_location=self.library.location, + user_id=self.user.id, + publish_item=False, + ) + + # Refresh our reference to the library + store = modulestore() + self.library = store.get_library(self.lib_key) + + # Refresh our reference to the block + self.lc_block = store.get_item(self.lc_block.location) + + # The library has changed... + self.assertEqual(len(self.library.children), 2) + + # But the block hasn't. + self.assertEqual(len(self.lc_block.children), 1) + + duplicate = store.get_item( + _duplicate_item(self.course.location, self.lc_block.location, self.user) + ) + self.assertEqual(len(duplicate.children), 1) + self.assertTrue(self.lc_block.source_library_version) + self.assertEqual(self.lc_block.source_library_version, duplicate.source_library_version) + class TestIncompatibleModuleStore(LibraryTestCase): """ diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 36e1e877fb..10f2bfed56 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -612,7 +612,7 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ store.update_item(dest_module, user.id) # pylint: disable=protected-access - if ('detached' not in source_item.runtime.load_block_type(category)._class_tags): + if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: parent = store.get_item(parent_usage_key) # If source was already a child of the parent, add duplicate immediately afterward. # Otherwise, add child to end. diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 6aaf8e4135..8fcd31b443 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -16,7 +16,7 @@ class LibraryToolsService(object): def __init__(self, modulestore): self.store = modulestore - def _get_library(self, library_key): + def _get_library(self, library_key, version=None): """ Given a library key like "library-v1:ProblemX+PR0B", return the 'library' XBlock with meta-information about the library. @@ -28,8 +28,18 @@ class LibraryToolsService(object): if not isinstance(library_key, LibraryLocator): library_key = LibraryLocator.from_string(library_key) + assert library_key.version_guid is None + + if version: + library_key = library_key.for_version(version) + try: - return self.store.get_library(library_key, remove_version=False, remove_branch=False) + library = self.store.get_library( + library_key, remove_version=False, remove_branch=False, head_validation=False + ) + if version: + assert library_key.version_guid == library.location.version_guid + return library except ItemNotFoundError: return None @@ -123,8 +133,8 @@ class LibraryToolsService(object): return source_blocks = [] - library_key = dest_block.source_library_key.for_version(version) - library = self._get_library(library_key) + library_key = dest_block.source_library_key + library = self._get_library(library_key, version=version) if library is None: raise ValueError("Requested library not found.") if user_perms and not user_perms.can_read(library_key): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 631902c6ad..98a7692ee0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -790,7 +790,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): else: self.request_cache.data['course_cache'] = {} - def _lookup_course(self, course_key): + def _lookup_course(self, course_key, head_validation=True): """ Decode the locator into the right series of db access. Does not return the CourseDescriptor! It returns the actual db json from @@ -799,11 +799,12 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): Semantics: if course id and branch given, then it will get that branch. If also give a version_guid, it will see if the current head of that branch == that guid. If not it raises VersionConflictError (the version now differs from what it was when you got your - reference) + reference) unless you specify head_validation = False, in which case it will return the + revision (if specified) by the course_key. :param course_key: any subclass of CourseLocator """ - if course_key.org and course_key.course and course_key.run: + if head_validation and course_key.org and course_key.course and course_key.run: if course_key.branch is None: raise InsufficientSpecificationError(course_key) @@ -937,11 +938,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ return CourseLocator(org, course, run) - def _get_structure(self, structure_id, depth, **kwargs): + def _get_structure(self, structure_id, depth, head_validation=True, **kwargs): """ Gets Course or Library by locator """ - structure_entry = self._lookup_course(structure_id) + structure_entry = self._lookup_course(structure_id, head_validation=head_validation) root = structure_entry.structure['root'] result = self._load_items(structure_entry, [root], depth, **kwargs) return result[0] @@ -955,14 +956,14 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError(course_id) return self._get_structure(course_id, depth, **kwargs) - def get_library(self, library_id, depth=0, **kwargs): + def get_library(self, library_id, depth=0, head_validation=True, **kwargs): """ Gets the 'library' root block for the library identified by the locator """ if not isinstance(library_id, LibraryLocator): # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. raise ItemNotFoundError(library_id) - return self._get_structure(library_id, depth, **kwargs) + return self._get_structure(library_id, depth, head_validation=head_validation, **kwargs) def has_course(self, course_id, ignore_case=False, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 5830a64f01..64c74bde9f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -58,7 +58,11 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli course_id = self._map_revision_to_branch(course_id) return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs) - def get_library(self, library_id, depth=0, **kwargs): + def get_library(self, library_id, depth=0, head_validation=True, **kwargs): + if not head_validation and library_id.version_guid: + return SplitMongoModuleStore.get_library( + self, library_id, depth=depth, head_validation=head_validation, **kwargs + ) library_id = self._map_revision_to_branch(library_id) return super(DraftVersioningModuleStore, self).get_library(library_id, depth=depth, **kwargs)