diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py
index 7918c54ecc..218272de8a 100644
--- a/common/lib/xmodule/xmodule/library_content_module.py
+++ b/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, suffix, update_db=True): # pylint: disable=unused-argument
+ def refresh_children(self, request=None, suffix=None, update_db=True): # pylint: disable=unused-argument
"""
Refresh children:
This method is to be used when any of the libraries that this block
@@ -402,17 +402,12 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
)
return validation
lib_tools = self.runtime.service(self, 'library_tools')
- matching_children_count = 0
for library_key, version in self.source_libraries:
if not self._validate_library_version(validation, lib_tools, version, library_key):
break
- library = lib_tools.get_library(library_key)
- children_matching_filter = lib_tools.get_filtered_children(library, self.capa_type)
- # get_filtered_children returns generator, so can't use len.
- # And we don't actually need those children, so no point of constructing a list
- matching_children_count += sum(1 for child in children_matching_filter)
-
+ # Note: we assume refresh_children() has been called since the last time fields like source_libraries or capa_types were changed.
+ matching_children_count = len(self.children) # pylint: disable=no-member
if matching_children_count == 0:
self._set_validation_error_if_empty(
validation,
diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py
index 94d4e0fdf4..f9b9f3edab 100644
--- a/common/lib/xmodule/xmodule/library_tools.py
+++ b/common/lib/xmodule/xmodule/library_tools.py
@@ -18,7 +18,7 @@ class LibraryToolsService(object):
def __init__(self, modulestore):
self.store = modulestore
- def get_library(self, library_key):
+ 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.
@@ -39,39 +39,26 @@ class LibraryToolsService(object):
Get the version (an ObjectID) of the given library.
Returns None if the library does not exist.
"""
- library = self.get_library(lib_key)
+ 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 _filter_child(self, capa_type, child_descriptor):
+ def _filter_child(self, usage_key, capa_type):
"""
Filters children by CAPA problem type, if configured
"""
if capa_type == ANY_CAPA_TYPE_VALUE:
return True
- if not isinstance(child_descriptor, CapaDescriptor):
+ if usage_key.block_type != "problem":
return False
- return capa_type in child_descriptor.problem_types
-
- def get_filtered_children(self, from_block, capa_type=ANY_CAPA_TYPE_VALUE):
- """
- Filters children of `from_block` that satisfy filter criteria
- Returns generator containing (child_key, child) for all children matching filter criteria
- """
- children = (
- (child_key, self.store.get_item(child_key, depth=None))
- for child_key in from_block.children
- )
- return (
- (child_key, child)
- for child_key, child in children
- if self._filter_child(capa_type, child)
- )
+ descriptor = self.store.get_item(usage_key, depth=0)
+ 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):
"""
@@ -104,7 +91,7 @@ class LibraryToolsService(object):
# 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)
+ 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):
@@ -124,9 +111,12 @@ class LibraryToolsService(object):
Internal method to copy blocks from the library recursively
"""
new_children = []
- target_capa_type = dest_block.capa_type if filter_problem_type else ANY_CAPA_TYPE_VALUE
- filtered_children = self.get_filtered_children(from_block, target_capa_type)
- for child_key, child in filtered_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
diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py
index 5ab2eaa575..fec957d0cd 100644
--- a/common/lib/xmodule/xmodule/tests/test_library_content.py
+++ b/common/lib/xmodule/xmodule/tests/test_library_content.py
@@ -138,9 +138,10 @@ class TestLibraries(MixedSplitTestCase):
# Check that get_content_titles() doesn't return titles for hidden/unused children
self.assertEqual(len(self.lc_block.get_content_titles()), 1)
- def test_validation(self):
+ def test_validation_of_course_libraries(self):
"""
- Test that the validation method of LibraryContent blocks is working.
+ Test that the validation method of LibraryContent blocks can validate
+ the source_libraries setting.
"""
# When source_libraries is blank, the validation summary should say this block needs to be configured:
self.lc_block.source_libraries = []
@@ -166,11 +167,17 @@ class TestLibraries(MixedSplitTestCase):
self.assertIn("out of date", result.summary.text)
# Now if we update the block, all validation should pass:
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertTrue(self.lc_block.validate())
+ def test_validation_of_matching_blocks(self):
+ """
+ Test that the validation method of LibraryContent blocks can warn
+ the user about problems with other settings (max_count and capa_type).
+ """
# Set max_count to higher value than exists in library
self.lc_block.max_count = 50
+ self.lc_block.refresh_children() # In the normal studio editing process, editor_saved() calls refresh_children at this point
result = self.lc_block.validate()
self.assertFalse(result) # Validation fails due to at least one warning/message
self.assertTrue(result.summary)
@@ -180,17 +187,19 @@ class TestLibraries(MixedSplitTestCase):
# Add some capa problems so we can check problem type validation messages
self.lc_block.max_count = 1
self._create_capa_problems()
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertTrue(self.lc_block.validate())
# Existing problem type should pass validation
self.lc_block.max_count = 1
self.lc_block.capa_type = 'multiplechoiceresponse'
+ self.lc_block.refresh_children()
self.assertTrue(self.lc_block.validate())
# ... unless requested more blocks than exists in library
self.lc_block.max_count = 10
self.lc_block.capa_type = 'multiplechoiceresponse'
+ self.lc_block.refresh_children()
result = self.lc_block.validate()
self.assertFalse(result) # Validation fails due to at least one warning/message
self.assertTrue(result.summary)
@@ -200,6 +209,7 @@ class TestLibraries(MixedSplitTestCase):
# Missing problem type should always fail validation
self.lc_block.max_count = 1
self.lc_block.capa_type = 'customresponse'
+ self.lc_block.refresh_children()
result = self.lc_block.validate()
self.assertFalse(result) # Validation fails due to at least one warning/message
self.assertTrue(result.summary)
@@ -213,21 +223,21 @@ class TestLibraries(MixedSplitTestCase):
self._create_capa_problems()
self.assertEqual(len(self.lc_block.children), 0) # precondition check
self.lc_block.capa_type = "multiplechoiceresponse"
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertEqual(len(self.lc_block.children), 1)
self.lc_block.capa_type = "optionresponse"
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertEqual(len(self.lc_block.children), 3)
self.lc_block.capa_type = "coderesponse"
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertEqual(len(self.lc_block.children), 2)
self.lc_block.capa_type = "customresponse"
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertEqual(len(self.lc_block.children), 0)
self.lc_block.capa_type = ANY_CAPA_TYPE_VALUE
- self.lc_block.refresh_children(None, None)
+ self.lc_block.refresh_children()
self.assertEqual(len(self.lc_block.children), len(self.lib_blocks) + 4)
diff --git a/common/test/acceptance/tests/studio/test_studio_library_container.py b/common/test/acceptance/tests/studio/test_studio_library_container.py
index ac385eb7ce..ce04643745 100644
--- a/common/test/acceptance/tests/studio/test_studio_library_container.py
+++ b/common/test/acceptance/tests/studio/test_studio_library_container.py
@@ -43,16 +43,6 @@ class StudioLibraryContainerTest(StudioLibraryTest, UniqueCourseTest):
XBlockFixtureDesc("html", "Html1"),
XBlockFixtureDesc("html", "Html2"),
XBlockFixtureDesc("html", "Html3"),
-
- XBlockFixtureDesc(
- "problem", "Dropdown",
- data=textwrap.dedent("""
-
- Dropdown
-
-
- """)
- )
)
def populate_course_fixture(self, course_fixture):
@@ -189,9 +179,20 @@ class StudioLibraryContainerTest(StudioLibraryTest, UniqueCourseTest):
When I go to studio unit page for library content block
And I set Problem Type selector so that no libraries have matching content
Then I can see that "No matching content" warning is shown
- When I set Problem Type selector so that there are matching content
+ When I set Problem Type selector so that there is matching content
Then I can see that warning messages are not shown
"""
+ # Add a single "Dropdown" type problem to the library (which otherwise has only HTML blocks):
+ self.library_fixture.create_xblock(self.library_fixture.library_location, XBlockFixtureDesc(
+ "problem", "Dropdown",
+ data=textwrap.dedent("""
+
+ Dropdown
+
+
+ """)
+ ))
+
expected_text = 'There are no matching problem types in the specified libraries. Select another problem type'
library_container = self._get_library_xblock_wrapper(self.unit_page.xblocks[0])