Move code that uses the modulestore to an XBlock service

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -73,6 +73,19 @@ class TestLibraries(ModuleStoreTestCase):
         self.assertEqual(response.status_code, 200)
         return modulestore().get_item(lib_content_block.location)
 
+    def _update_item(self, usage_key, metadata):
+        """
+        Helper method: Uses the REST API to update the fields of an XBlock.
+        This will result in the XBlock's editor_saved() method being called.
+        """
+        update_url = reverse_usage_url("xblock_handler", usage_key)
+        return self.client.ajax_post(
+            update_url,
+            data={
+                'metadata': metadata,
+            }
+        )
+
     @ddt.data(
         (2, 1, 1),
         (2, 2, 2),
@@ -254,3 +267,39 @@ class TestLibraries(ModuleStoreTestCase):
 
         self.assertEqual(course_child_block.data, data_value)
         self.assertEqual(course_child_block.display_name, name_value)
+
+    def test_change_after_first_sync(self):
+        """
+        Check that nothing goes wrong if we (A) Set up a LibraryContent block
+        and use it successfully, then (B) Give it an invalid configuration.
+        No children should be deleted until the configuration is fixed.
+        """
+        # Add a block to the library:
+        data_value = "Hello world!"
+        ItemFactory.create(
+            category="html",
+            parent_location=self.library.location,
+            user_id=self.user.id,
+            publish_item=False,
+            display_name="HTML BLock",
+            data=data_value,
+        )
+        # Create a course:
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            course = CourseFactory.create()
+
+        # Add a LibraryContent block to the course:
+        lc_block = self._add_library_content_block(course, self.lib_key)
+        lc_block = self._refresh_children(lc_block)
+        self.assertEqual(len(lc_block.children), 1)
+
+        # Now, change the block settings to have an invalid library key:
+        resp = self._update_item(
+            lc_block.location,
+            {"source_libraries": [["library-v1:NOT+FOUND", None]]},
+        )
+        self.assertEqual(resp.status_code, 200)
+        lc_block = modulestore().get_item(lc_block.location)
+        self.assertEqual(len(lc_block.children), 1)  # Children should not be deleted due to a bad setting.
+        html_block = modulestore().get_item(lc_block.children[0])
+        self.assertEqual(html_block.data, data_value)

cms/djangoapps/contentstore/views/preview.py

@@ -14,6 +14,7 @@ from xmodule.x_module import PREVIEW_VIEWS, STUDENT_VIEW, AUTHOR_VIEW
 from xmodule.contentstore.django import contentstore
 from xmodule.error_module import ErrorDescriptor
 from xmodule.exceptions import NotFoundError, ProcessingError
+from xmodule.library_tools import LibraryToolsService
 from xmodule.modulestore.django import modulestore, ModuleI18nService
 from opaque_keys.edx.keys import UsageKey
 from xmodule.x_module import ModuleSystem
@@ -177,6 +178,7 @@ def _preview_module_system(request, descriptor, field_data):
         services={
             "i18n": ModuleI18nService(),
             "field-data": field_data,
+            "library_tools": LibraryToolsService(modulestore()),
         },
     )
 

common/lib/xmodule/xmodule/library_content_module.py

@@ -5,7 +5,6 @@ LibraryContent: The XBlock used to include blocks from a library in a course.
 from bson.objectid import ObjectId, InvalidId
 from collections import namedtuple
 from copy import copy
-import hashlib
 from .mako_module import MakoModuleDescriptor
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locator import LibraryLocator
@@ -14,7 +13,6 @@ from webob import Response
 from xblock.core import XBlock
 from xblock.fields import Scope, String, List, Integer, Boolean
 from xblock.fragment import Fragment
-from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.validation import StudioValidationMessage, StudioValidation
 from xmodule.x_module import XModule, STUDENT_VIEW
 from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor
@@ -163,29 +161,8 @@ class LibraryContentFields(object):
     has_children = True
 
 
-def _get_library(modulestore, library_key):
-    """
-    Given a library key like "library-v1:ProblemX+PR0B", return the
-    'library' XBlock with meta-information about the library.
-
-    Returns None on error.
-    """
-    if not isinstance(library_key, LibraryLocator):
-        library_key = LibraryLocator.from_string(library_key)
-    assert library_key.version_guid is None
-
-    # TODO: Is this too tightly coupled to split? May need to abstract this into a service
-    # provided by the CMS runtime.
-    try:
-        library = modulestore.get_library(library_key, remove_version=False)
-    except ItemNotFoundError:
-        return None
-    # We need to know the library's version so ensure it's set in library.location.library_key.version_guid
-    assert library.location.library_key.version_guid is not None
-    return library
-
-
 #pylint: disable=abstract-method
+@XBlock.wants('library_tools')  # Only needed in studio
 class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
     """
     An XBlock whose children are chosen dynamically from a content library.
@@ -289,10 +266,11 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
         else:
             # When shown on a unit page, don't show any sort of preview - just the status of this block.
             library_names = []
+            lib_tools = self.runtime.service(self, 'library_tools')
             for library_key, version in self.source_libraries:  # pylint: disable=unused-variable
-                library = _get_library(self.runtime.descriptor_runtime.modulestore, library_key)
-                if library is not None:
-                    library_names.append(library.display_name)
+                lib_name = lib_tools.get_library_display_name(library_key)
+                if lib_name is not None:
+                    library_names.append(lib_name)
 
             if library_names:
                 fragment.add_content(self.system.render_template('library-block-author-view.html', {
@@ -313,6 +291,7 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
 
 
 @XBlock.wants('user')
+@XBlock.wants('library_tools')  # Only needed in studio
 class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDescriptor, StudioEditableDescriptor):
     """
     Descriptor class for LibraryContentModule XBlock.
@@ -339,69 +318,10 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
         If update_db is True (default), this will explicitly persist the changes
         to the modulestore by calling update_item()
         """
+        lib_tools = self.runtime.service(self, 'library_tools')
         user_service = self.runtime.service(self, 'user')
         user_id = user_service.user_id if user_service else None  # May be None when creating bok choy test fixtures
-        root_children = []
-
-        store = self.system.modulestore
-        with store.bulk_operations(self.location.course_key):
-            # Currently, ALL children are essentially deleted and then re-added
-            # in a way that preserves their block_ids (and thus should preserve
-            # student data, grades, analytics, etc.)
-            # Once course-level field overrides are implemented, this will
-            # change to a more conservative implementation.
-
-            # First, delete all our existing children to avoid block_id conflicts when we add them:
-            for child in self.children:  # pylint: disable=access-member-before-definition
-                store.delete_item(child, user_id)
-
-            # Now add all matching children, and record the library version we use:
-            new_libraries = []
-            for library_key, old_version in self.source_libraries:  # pylint: disable=unused-variable
-                library = _get_library(self.system.modulestore, library_key)  # pylint: disable=protected-access
-                if library is None:
-                    raise ValueError("Required library not found.")
-
-                def copy_children_recursively(from_block):
-                    """
-                    Internal method to copy blocks from the library recursively
-                    """
-                    new_children = []
-                    for child_key in from_block.children:
-                        child = store.get_item(child_key, depth=9)
-                        # We compute a block_id for each matching child block found in the library.
-                        # block_ids are unique within any branch, but are not unique per-course or globally.
-                        # We need our block_ids to be consistent when content in the library is updated, so
-                        # we compute block_id as a hash of three pieces of data:
-                        unique_data = "{}:{}:{}".format(
-                            self.location.block_id,  # Must not clash with other usages of the same library in this course
-                            unicode(library_key.for_version(None)).encode("utf-8"),  # The block ID below is only unique within a library, so we need this too
-                            child_key.block_id,  # Child block ID. Should not change even if the block is edited.
-                        )
-                        child_block_id = hashlib.sha1(unique_data).hexdigest()[:20]
-                        fields = {}
-                        for field in child.fields.itervalues():
-                            if field.scope == Scope.settings and field.is_set_on(child):
-                                fields[field.name] = field.read_from(child)
-                        if child.has_children:
-                            fields['children'] = copy_children_recursively(from_block=child)
-                        new_child_info = store.create_item(
-                            user_id,
-                            self.location.course_key,
-                            child_key.block_type,
-                            block_id=child_block_id,
-                            definition_locator=child.definition_locator,
-                            runtime=self.system,
-                            fields=fields,
-                        )
-                        new_children.append(new_child_info.location)
-                    return new_children
-                root_children.extend(copy_children_recursively(from_block=library))
-                new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
-            self.source_libraries = new_libraries
-            self.children = root_children  # pylint: disable=attribute-defined-outside-init
-            if update_db:
-                self.system.modulestore.update_item(self, user_id)
+        lib_tools.update_children(self, user_id, update_db)
         return Response()
 
     def validate(self):
@@ -423,10 +343,10 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
                 )
             )
             return validation
+        lib_tools = self.runtime.service(self, 'library_tools')
         for library_key, version in self.source_libraries:
-            library = _get_library(self.runtime.modulestore, library_key)
-            if library is not None:
-                latest_version = library.location.library_key.version_guid
+            latest_version = lib_tools.get_library_version(library_key)
+            if latest_version is not None:
                 if version is None or version != latest_version:
                     validation.set_summary(
                         StudioValidationMessage(

common/lib/xmodule/xmodule/library_tools.py

+"""
+XBlock runtime services for LibraryContentModule
+"""
+import hashlib
+from opaque_keys.edx.locator import LibraryLocator
+from xblock.fields import Scope
+from xmodule.library_content_module import LibraryVersionReference
+from xmodule.modulestore.exceptions import ItemNotFoundError
+
+
+class LibraryToolsService(object):
+    """
+    Service that allows LibraryContentModule to interact with libraries in the
+    modulestore.
+    """
+    def __init__(self, modulestore):
+        self.store = modulestore
+
+    def _get_library(self, library_key):
+        """
+        Given a library key like "library-v1:ProblemX+PR0B", return the
+        'library' XBlock with meta-information about the library.
+
+        Returns None on error.
+        """
+        if not isinstance(library_key, LibraryLocator):
+            library_key = LibraryLocator.from_string(library_key)
+        assert library_key.version_guid is None
+
+        try:
+            return self.store.get_library(library_key, remove_version=False)
+        except ItemNotFoundError:
+            return None
+
+    def get_library_version(self, lib_key):
+        """
+        Get the version (an ObjectID) of the given library.
+        Returns None if the library does not exist.
+        """
+        library = self._get_library(lib_key)
+        if library:
+            # We need to know the library's version so ensure it's set in library.location.library_key.version_guid
+            assert library.location.library_key.version_guid is not None
+            return library.location.library_key.version_guid
+        return None
+
+    def get_library_display_name(self, lib_key):
+        """
+        Get the display_name of the given library.
+        Returns None if the library does not exist.
+        """
+        library = self._get_library(lib_key)
+        if library:
+            return library.display_name
+        return None
+
+    def update_children(self, dest_block, user_id, update_db=True):
+        """
+        This method is to be used when any of the libraries that a LibraryContentModule
+        references have been updated. It will re-fetch all matching blocks from
+        the libraries, and copy them as children of dest_block. The children
+        will be given new block_ids, but the definition ID used should be the
+        exact same definition ID used in the library.
+
+        This method will update dest_block's 'source_libraries' field to store
+        the version number of the libraries used, so we easily determine if
+        dest_block is up to date or not.
+
+        If update_db is True (default), this will explicitly persist the changes
+        to the modulestore by calling update_item(). Only set update_db False if
+        you know for sure that dest_block is about to be saved to the modulestore
+        anyways. Otherwise, orphaned blocks may be created.
+        """
+        root_children = []
+
+        with self.store.bulk_operations(dest_block.location.course_key):
+            # Currently, ALL children are essentially deleted and then re-added
+            # in a way that preserves their block_ids (and thus should preserve
+            # student data, grades, analytics, etc.)
+            # Once course-level field overrides are implemented, this will
+            # change to a more conservative implementation.
+
+            # First, load and validate the source_libraries:
+            libraries = []
+            for library_key, old_version in dest_block.source_libraries:  # pylint: disable=unused-variable
+                library = self._get_library(library_key)
+                if library is None:
+                    raise ValueError("Required library not found.")
+                libraries.append((library_key, library))
+
+            # Next, delete all our existing children to avoid block_id conflicts when we add them:
+            for child in dest_block.children:
+                self.store.delete_item(child, user_id)
+
+            # Now add all matching children, and record the library version we use:
+            new_libraries = []
+            for library_key, library in libraries:
+
+                def copy_children_recursively(from_block):
+                    """
+                    Internal method to copy blocks from the library recursively
+                    """
+                    new_children = []
+                    for child_key in from_block.children:
+                        child = self.store.get_item(child_key, depth=9)
+                        # We compute a block_id for each matching child block found in the library.
+                        # block_ids are unique within any branch, but are not unique per-course or globally.
+                        # We need our block_ids to be consistent when content in the library is updated, so
+                        # we compute block_id as a hash of three pieces of data:
+                        unique_data = "{}:{}:{}".format(
+                            dest_block.location.block_id,  # Must not clash with other usages of the same library in this course
+                            unicode(library_key.for_version(None)).encode("utf-8"),  # The block ID below is only unique within a library, so we need this too
+                            child_key.block_id,  # Child block ID. Should not change even if the block is edited.
+                        )
+                        child_block_id = hashlib.sha1(unique_data).hexdigest()[:20]
+                        fields = {}
+                        for field in child.fields.itervalues():
+                            if field.scope == Scope.settings and field.is_set_on(child):
+                                fields[field.name] = field.read_from(child)
+                        if child.has_children:
+                            fields['children'] = copy_children_recursively(from_block=child)
+                        new_child_info = self.store.create_item(
+                            user_id,
+                            dest_block.location.course_key,
+                            child_key.block_type,
+                            block_id=child_block_id,
+                            definition_locator=child.definition_locator,
+                            runtime=dest_block.system,
+                            fields=fields,
+                        )
+                        new_children.append(new_child_info.location)
+                    return new_children
+                root_children.extend(copy_children_recursively(from_block=library))
+                new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
+            dest_block.source_libraries = new_libraries
+            dest_block.children = root_children
+            if update_db:
+                self.store.update_item(dest_block, user_id)

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

@@ -6,6 +6,7 @@ from lazy import lazy
 from xblock.runtime import KvsFieldData
 from xblock.fields import ScopeIds
 from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator, LibraryLocator, DefinitionLocator
+from xmodule.library_tools import LibraryToolsService
 from xmodule.mako_module import MakoDescriptorSystem
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import exc_info_to_str
@@ -71,6 +72,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         self.module_data = module_data
         self.default_class = default_class
         self.local_modules = {}
+        self._services['library_tools'] = LibraryToolsService(modulestore)
 
     @lazy
     @contract(returns="dict(BlockKey: BlockKey)")