Support for overriding Scope.settings values when library content is used in a course

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -683,3 +683,130 @@ class TestLibraryAccess(LibraryTestCase):
         self._bind_module(lc_block, user=self.non_staff_user)  # We must use the CMS's module system in order to get permissions checks.
         lc_block = self._refresh_children(lc_block, status_code_expected=200 if expected_result else 403)
         self.assertEqual(len(lc_block.children), 1 if expected_result else 0)
+
+
+class TestOverrides(LibraryTestCase):
+    """
+    Test that overriding block Scope.settings fields from a library in a specific course works
+    """
+    def setUp(self):
+        super(TestOverrides, self).setUp()
+        self.original_display_name = "A Problem Block"
+        self.original_weight = 1
+
+        # Create a problem block in the library:
+        self.problem = ItemFactory.create(
+            category="problem",
+            parent_location=self.library.location,
+            display_name=self.original_display_name,  # display_name is a Scope.settings field
+            weight=self.original_weight,  # weight is also a Scope.settings field
+            user_id=self.user.id,
+            publish_item=False,
+        )
+
+        # Also create a course:
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            self.course = CourseFactory.create()
+
+        # Add a LibraryContent block to the course:
+        self.lc_block = self._add_library_content_block(self.course, self.lib_key)
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.lc_block.children[0])
+
+    def test_overrides(self):
+        """
+        Test that we can override Scope.settings values in a course.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 10
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+        modulestore().update_item(self.problem_in_course, self.user.id)
+
+        # Add a second LibraryContent block to the course, with no override:
+        lc_block2 = self._add_library_content_block(self.course, self.lib_key)
+        lc_block2 = self._refresh_children(lc_block2)
+        # Re-load the two problem blocks - one with and one without an override:
+        self.problem_in_course = modulestore().get_item(self.lc_block.children[0])
+        problem2_in_course = modulestore().get_item(lc_block2.children[0])
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        self.assertEqual(problem2_in_course.display_name, self.original_display_name)
+        self.assertEqual(problem2_in_course.weight, self.original_weight)
+
+    def test_reset_override(self):
+        """
+        If we override a setting and then reset it, we should get the library value.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 10
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        # Reset:
+        for field_name in ["display_name", "weight"]:
+            self.problem_in_course.fields[field_name].delete_from(self.problem_in_course)
+
+        # Save, reload, and verify:
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, self.original_display_name)
+        self.assertEqual(self.problem_in_course.weight, self.original_weight)
+
+    def test_consistent_definitions(self):
+        """
+        Make sure that the new child of the LibraryContent block
+        shares its definition with the original (self.problem).
+
+        This test is specific to split mongo.
+        """
+        definition_id = self.problem.definition_locator.definition_id
+        self.assertEqual(self.problem_in_course.definition_locator.definition_id, definition_id)
+
+        # Now even if we change some Scope.settings fields and refresh, the definition should be unchanged
+        self.problem.weight = 20
+        self.problem.display_name = "NEW"
+        modulestore().update_item(self.problem, self.user.id)
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem.definition_locator.definition_id, definition_id)
+        self.assertEqual(self.problem_in_course.definition_locator.definition_id, definition_id)
+
+    def test_persistent_overrides(self):
+        """
+        Test that when we override Scope.settings values in a course,
+        the override values persist even when the block is refreshed
+        with updated blocks from the library.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 15
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        # Change the settings in the library version:
+        self.problem.display_name = "X"
+        self.problem.weight = 99
+        new_data_value = "<problem><p>We change the data as well to check that non-overriden fields do get updated.</p></problem>"
+        self.problem.data = new_data_value
+        modulestore().update_item(self.problem, self.user.id)
+
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+        self.assertEqual(self.problem_in_course.data, new_data_value)

common/lib/xmodule/xmodule/library_content_module.py

@@ -323,7 +323,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
     js_module_name = "VerticalDescriptor"
 
     @XBlock.handler
-    def refresh_children(self, request=None, suffix=None, update_db=True):  # pylint: disable=unused-argument
+    def refresh_children(self, request=None, suffix=None):  # pylint: disable=unused-argument
         """
         Refresh children:
         This method is to be used when any of the libraries that this block
@@ -335,15 +335,12 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
         This method will update this block's 'source_libraries' field to store
         the version number of the libraries used, so we easily determine if
         this 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()
         """
         lib_tools = self.runtime.service(self, 'library_tools')
         user_service = self.runtime.service(self, 'user')
         user_perms = self.runtime.service(self, 'studio_user_permissions')
         user_id = user_service.user_id if user_service else None  # May be None when creating bok choy test fixtures
-        lib_tools.update_children(self, user_id, user_perms, update_db)
+        lib_tools.update_children(self, user_id, user_perms)
         return Response()
 
     def _validate_library_version(self, validation, lib_tools, version, library_key):
@@ -451,7 +448,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
         if (set(old_source_libraries) != set(self.source_libraries) or
                 old_metadata.get('capa_type', ANY_CAPA_TYPE_VALUE) != self.capa_type):
             try:
-                self.refresh_children(None, None, update_db=False)  # update_db=False since update_item() is about to be called anyways
+                self.refresh_children()
             except ValueError:
                 pass  # The validation area will display an error message, no need to do anything now.
 

common/lib/xmodule/xmodule/library_tools.py

 """
 XBlock runtime services for LibraryContentModule
 """
-import hashlib
 from django.core.exceptions import PermissionDenied
 from opaque_keys.edx.locator import LibraryLocator
-from xblock.fields import Scope
 from xmodule.library_content_module import LibraryVersionReference, ANY_CAPA_TYPE_VALUE
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.capa_module import CapaDescriptor
@@ -60,7 +58,7 @@ class LibraryToolsService(object):
         assert isinstance(descriptor, CapaDescriptor)
         return capa_type in descriptor.problem_types
 
-    def update_children(self, dest_block, user_id, user_perms=None, update_db=True):
+    def update_children(self, dest_block, user_id, user_perms=None):
         """
         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
@@ -71,82 +69,28 @@ class LibraryToolsService(object):
         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 = []
         if user_perms and not user_perms.can_write(dest_block.location.course_key):
             raise PermissionDenied()
 
-        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.")
-                if user_perms and not user_perms.can_read(library_key):
-                    raise PermissionDenied()
-                libraries.append((library_key, library))
+        new_libraries = []
+        source_blocks = []
+        for library_key, __ in dest_block.source_libraries:
+            library = self._get_library(library_key)
+            if library is None:
+                raise ValueError("Required library not found.")
+            if user_perms and not user_perms.can_read(library_key):
+                raise PermissionDenied()
+            filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE)
+            if filter_children:
+                # Apply simple filtering based on CAPA problem types:
+                source_blocks.extend([key for key in library.children if self._filter_child(key, dest_block.capa_type)])
+            else:
+                source_blocks.extend(library.children)
+            new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
 
-            # 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, filter_problem_type=False):
-                    """
-                    Internal method to copy blocks from the library recursively
-                    """
-                    new_children = []
-                    if filter_problem_type:
-                        filtered_children = [key for key in from_block.children if self._filter_child(key, dest_block.capa_type)]
-                    else:
-                        filtered_children = from_block.children
-                    for child_key in filtered_children:
-                        child = self.store.get_item(child_key, depth=None)
-                        # 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, filter_problem_type=True))
-                new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
+        with self.store.bulk_operations(dest_block.location.course_key):
             dest_block.source_libraries = new_libraries
-            dest_block.children = root_children
-            if update_db:
-                self.store.update_item(dest_block, user_id)
+            self.store.update_item(dest_block, user_id)
+            dest_block.children = self.store.copy_from_template(source_blocks, dest_block.location, user_id)
+            # ^-- copy_from_template updates the children in the DB but we must also set .children here to avoid overwriting the DB again

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -283,6 +283,8 @@ class InheritanceKeyValueStore(KeyValueStore):
 
     def default(self, key):
         """
-        Check to see if the default should be from inheritance rather than from the field's global default
+        Check to see if the default should be from inheritance. If not
+        inheriting, this will raise KeyError which will cause the caller to use
+        the field's global default.
         """
         return self.inherited_settings[key.field_name]

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

@@ -677,6 +677,14 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         return store.import_xblock(user_id, course_key, block_type, block_id, fields, runtime)
 
     @strip_key
+    def copy_from_template(self, source_keys, dest_key, user_id, **kwargs):
+        """
+        See :py:meth `SplitMongoModuleStore.copy_from_template`
+        """
+        store = self._verify_modulestore_support(dest_key.course_key, 'copy_from_template')
+        return store.copy_from_template(source_keys, dest_key, user_id)
+
+    @strip_key
     def update_item(self, xblock, user_id, allow_not_found=False, **kwargs):
         """
         Update the xblock persisted to be the same as the given for all types of fields

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

@@ -169,16 +169,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         if block_key is None:
             block_key = BlockKey(json_data['block_type'], LocalId())
 
+        convert_fields = lambda field: self.modulestore.convert_references_to_keys(
+            course_key, class_, field, self.course_entry.structure['blocks'],
+        )
+
         if definition_id is not None and not json_data.get('definition_loaded', False):
             definition_loader = DefinitionLazyLoader(
                 self.modulestore,
                 course_key,
                 block_key.type,
                 definition_id,
-                lambda fields: self.modulestore.convert_references_to_keys(
-                    course_key, self.load_block_type(block_key.type),
-                    fields, self.course_entry.structure['blocks'],
-                )
+                convert_fields,
             )
         else:
             definition_loader = None
@@ -193,9 +194,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
             block_id=block_key.id,
         )
 
-        converted_fields = self.modulestore.convert_references_to_keys(
-            block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry.structure['blocks'],
-        )
+        converted_fields = convert_fields(json_data.get('fields', {}))
+        converted_defaults = convert_fields(json_data.get('defaults', {}))
         if block_key in self._parent_map:
             parent_key = self._parent_map[block_key]
             parent = course_key.make_usage_key(parent_key.type, parent_key.id)
@@ -204,6 +204,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         kvs = SplitMongoKVS(
             definition_loader,
             converted_fields,
+            converted_defaults,
             parent=parent,
             field_decorator=kwargs.get('field_decorator')
         )

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

@@ -93,6 +93,26 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
             # version_agnostic b/c of above assumption in docstring
             self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
 
+    def copy_from_template(self, source_keys, dest_key, user_id, **kwargs):
+        """
+        See :py:meth `SplitMongoModuleStore.copy_from_template`
+        """
+        source_keys = [self._map_revision_to_branch(key) for key in source_keys]
+        dest_key = self._map_revision_to_branch(dest_key)
+        new_keys = super(DraftVersioningModuleStore, self).copy_from_template(source_keys, dest_key, user_id)
+        if dest_key.branch == ModuleStoreEnum.BranchName.draft:
+            # Check if any of new_keys or their descendants need to be auto-published.
+            # We don't use _auto_publish_no_children since children may need to be published.
+            with self.bulk_operations(dest_key.course_key):
+                keys_to_check = list(new_keys)
+                while keys_to_check:
+                    usage_key = keys_to_check.pop()
+                    if usage_key.category in DIRECT_ONLY_CATEGORIES:
+                        self.publish(usage_key.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
+                        children = getattr(self.get_item(usage_key, **kwargs), "children", [])
+                        keys_to_check.extend(children)  # e.g. if usage_key is a chapter, it may have an auto-publish sequential child
+        return new_keys
+
     def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs):
         old_descriptor_locn = descriptor.location
         descriptor.location = self._map_revision_to_branch(old_descriptor_locn)

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

@@ -19,17 +19,20 @@ class SplitMongoKVS(InheritanceKeyValueStore):
     """
 
     @contract(parent="BlockUsageLocator | None")
-    def __init__(self, definition, initial_values, parent, field_decorator=None):
+    def __init__(self, definition, initial_values, default_values, parent, field_decorator=None):
         """
 
         :param definition: either a lazyloader or definition id for the definition
         :param initial_values: a dictionary of the locally set values
+        :param default_values: any Scope.settings field defaults that are set locally
+            (copied from a template block with copy_from_template)
         """
         # deepcopy so that manipulations of fields does not pollute the source
         super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values))
         self._definition = definition  # either a DefinitionLazyLoader or the db id of the definition.
         # if the db id, then the definition is presumed to be loaded into _fields
 
+        self._defaults = default_values
         # a decorator function for field values (to be called when a field is accessed)
         if field_decorator is None:
             self.field_decorator = lambda x: x
@@ -110,6 +113,16 @@ class SplitMongoKVS(InheritanceKeyValueStore):
         # if someone changes it so that they do, then change any tests of field.name in xx._field_data
         return key.field_name in self._fields
 
+    def default(self, key):
+        """
+        Check to see if the default should be from the template's defaults (if any)
+        rather than the global default or inheritance.
+        """
+        if self._defaults and key.field_name in self._defaults:
+            return self._defaults[key.field_name]
+        # If not, try inheriting from a parent, then use the XBlock type's normal default value:
+        return super(SplitMongoKVS, self).default(key)
+
     def _load_definition(self):
         """
         Update fields w/ the lazily loaded definitions

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

@@ -10,8 +10,9 @@ from mock import patch
 from opaque_keys.edx.locator import LibraryLocator
 from xblock.fragment import Fragment
 from xblock.runtime import Runtime as VanillaRuntime
-from xmodule.modulestore.exceptions import DuplicateCourseError
-from xmodule.modulestore.tests.factories import LibraryFactory, ItemFactory, check_mongo_calls
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError
+from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory, ItemFactory, check_mongo_calls
 from xmodule.modulestore.tests.utils import MixedSplitTestCase
 from xmodule.x_module import AUTHOR_VIEW
 
@@ -217,3 +218,142 @@ class TestLibraries(MixedSplitTestCase):
             modulestore=self.store,
         )
         self.assertFalse(self.store.has_published_version(block))
+
+
+@ddt.ddt
+class TestSplitCopyTemplate(MixedSplitTestCase):
+    """
+    Test for split's copy_from_template method.
+    Currently it is only used for content libraries.
+    However for this test, we make sure it also works when copying from course to course.
+    """
+    @ddt.data(
+        LibraryFactory,
+        CourseFactory,
+    )
+    def test_copy_from_template(self, source_type):
+        """
+        Test that the behavior of copy_from_template() matches its docstring
+        """
+        source_container = source_type.create(modulestore=self.store)  # Either a library or a course
+        course = CourseFactory.create(modulestore=self.store)
+        # Add a vertical with a capa child to the source library/course:
+        vertical_block = ItemFactory.create(
+            category="vertical",
+            parent_location=source_container.location,
+            user_id=self.user_id,
+            publish_item=False,
+            modulestore=self.store,
+        )
+        problem_library_display_name = "Problem Library Display Name"
+        problem_block = ItemFactory.create(
+            category="problem",
+            parent_location=vertical_block.location,
+            user_id=self.user_id,
+            publish_item=False,
+            modulestore=self.store,
+            display_name=problem_library_display_name,
+            markdown="Problem markdown here"
+        )
+
+        if source_type == LibraryFactory:
+            source_container = self.store.get_library(source_container.location.library_key, remove_version=False, remove_branch=False)
+        else:
+            source_container = self.store.get_course(source_container.location.course_key, remove_version=False, remove_branch=False)
+
+        # Inherit the vertical and the problem from the library into the course:
+        source_keys = [source_container.children[0]]
+        new_blocks = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id)
+        self.assertEqual(len(new_blocks), 1)
+
+        course = self.store.get_course(course.location.course_key)  # Reload from modulestore
+
+        self.assertEqual(len(course.children), 1)
+        vertical_block_course = self.store.get_item(course.children[0])
+        self.assertEqual(new_blocks[0], vertical_block_course.location)
+        problem_block_course = self.store.get_item(vertical_block_course.children[0])
+        self.assertEqual(problem_block_course.display_name, problem_library_display_name)
+
+        # Override the display_name and weight:
+        new_display_name = "The Trouble with Tribbles"
+        new_weight = 20
+        problem_block_course.display_name = new_display_name
+        problem_block_course.weight = new_weight
+        self.store.update_item(problem_block_course, self.user_id)
+
+        # Test that "Any previously existing children of `dest_usage` that haven't been replaced/updated by this copy_from_template operation will be deleted."
+        extra_block = ItemFactory.create(
+            category="html",
+            parent_location=vertical_block_course.location,
+            user_id=self.user_id,
+            publish_item=False,
+            modulestore=self.store,
+        )
+
+        # Repeat the copy_from_template():
+        new_blocks2 = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id)
+        self.assertEqual(new_blocks, new_blocks2)
+        # Reload problem_block_course:
+        problem_block_course = self.store.get_item(problem_block_course.location)
+        self.assertEqual(problem_block_course.display_name, new_display_name)
+        self.assertEqual(problem_block_course.weight, new_weight)
+
+        # Ensure that extra_block was deleted:
+        vertical_block_course = self.store.get_item(new_blocks2[0])
+        self.assertEqual(len(vertical_block_course.children), 1)
+        with self.assertRaises(ItemNotFoundError):
+            self.store.get_item(extra_block.location)
+
+    def test_copy_from_template_auto_publish(self):
+        """
+        Make sure that copy_from_template works with things like 'chapter' that
+        are always auto-published.
+        """
+        source_course = CourseFactory.create(modulestore=self.store)
+        course = CourseFactory.create(modulestore=self.store)
+        make_block = lambda category, parent: ItemFactory.create(category=category, parent_location=parent.location, user_id=self.user_id, modulestore=self.store)
+
+        # Populate the course:
+        about = make_block("about", source_course)
+        chapter = make_block("chapter", source_course)
+        sequential = make_block("sequential", chapter)
+        # And three blocks that are NOT auto-published:
+        vertical = make_block("vertical", sequential)
+        make_block("problem", vertical)
+        html = make_block("html", source_course)
+
+        # Reload source_course since we need its branch and version to use copy_from_template:
+        source_course = self.store.get_course(source_course.location.course_key, remove_version=False, remove_branch=False)
+
+        # Inherit the vertical and the problem from the library into the course:
+        source_keys = [block.location for block in [about, chapter, html]]
+        block_keys = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id)
+        self.assertEqual(len(block_keys), len(source_keys))
+
+        # Build dict of the new blocks in 'course', keyed by category (which is a unique key in our case)
+        new_blocks = {}
+        block_keys = set(block_keys)
+        while block_keys:
+            key = block_keys.pop()
+            block = self.store.get_item(key)
+            new_blocks[block.category] = block
+            block_keys.update(set(getattr(block, "children", [])))
+
+        # Check that auto-publish blocks with no children are indeed published:
+        def published_version_exists(block):
+            """ Does a published version of block exist? """
+            try:
+                self.store.get_item(block.location.for_branch(ModuleStoreEnum.BranchName.published))
+                return True
+            except ItemNotFoundError:
+                return False
+
+        # Check that the auto-publish blocks have been published:
+        self.assertFalse(self.store.has_changes(new_blocks["about"]))
+        self.assertTrue(published_version_exists(new_blocks["chapter"]))  # We can't use has_changes because it includes descendants
+        self.assertTrue(published_version_exists(new_blocks["sequential"]))  # Ditto
+        # Check that non-auto-publish blocks and blocks with non-auto-publish descendants show changes:
+        self.assertTrue(self.store.has_changes(new_blocks["html"]))
+        self.assertTrue(self.store.has_changes(new_blocks["problem"]))
+        self.assertTrue(self.store.has_changes(new_blocks["chapter"]))  # Will have changes since a child block has changes.
+        self.assertFalse(published_version_exists(new_blocks["vertical"]))  # Verify that our published_version_exists works

common/lib/xmodule/xmodule/tests/test_library_content.py

@@ -117,7 +117,8 @@ class TestLibraries(MixedSplitTestCase):
         # is updated, but the way we do it through a factory doesn't do that.
         self.assertEqual(len(self.lc_block.children), 0)
         # Update the LibraryContent module:
-        self.lc_block.refresh_children(None, None)
+        self.lc_block.refresh_children()
+        self.lc_block = self.store.get_item(self.lc_block.location)
         # Check that all blocks from the library are now children of the block:
         self.assertEqual(len(self.lc_block.children), len(self.lib_blocks))
 
@@ -125,7 +126,7 @@ class TestLibraries(MixedSplitTestCase):
         """
         Test that each student sees only one block as a child of the LibraryContent block.
         """
-        self.lc_block.refresh_children(None, None)
+        self.lc_block.refresh_children()
         self.lc_block = self.store.get_item(self.lc_block.location)
         self._bind_course_module(self.lc_block)
         # Make sure the runtime knows that the block's children vary per-user:

common/lib/xmodule/xmodule/x_module.py

@@ -1250,6 +1250,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # p
         :param xblock:
         :param field:
         """
+        # pylint: disable=protected-access
         # in runtime b/c runtime contains app-specific xblock behavior. Studio's the only app
         # which needs this level of introspection right now. runtime also is 'allowed' to know
         # about the kvs, dbmodel, etc.
@@ -1257,12 +1258,8 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # p
         result = {}
         result['explicitly_set'] = xblock._field_data.has(xblock, field.name)
         try:
-            block_inherited = xblock.xblock_kvs.inherited_settings
-        except AttributeError:  # if inherited_settings doesn't exist on kvs
-            block_inherited = {}
-        if field.name in block_inherited:
-            result['default_value'] = block_inherited[field.name]
-        else:
+            result['default_value'] = xblock._field_data.default(xblock, field.name)
+        except KeyError:
             result['default_value'] = field.to_json(field.default)
         return result