From 2be036fe0e3ab0e98ce9fb6ec5bbc24368ed9097 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 6 Jan 2015 23:01:40 -0800
Subject: [PATCH] Simplifications and changes to reduce conflicts with PR 6399

---
 .../xmodule/xmodule/library_content_module.py | 11 ++----
 common/lib/xmodule/xmodule/library_tools.py   | 38 +++++++------------
 .../xmodule/tests/test_library_content.py     | 28 +++++++++-----
 .../studio/test_studio_library_container.py   | 23 +++++------
 4 files changed, 48 insertions(+), 52 deletions(-)

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("""
-                    <problem>
-                        <p>Dropdown</p>
-                        <optionresponse><optioninput label="Dropdown" options="('1', '2')" correct="'2'"></optioninput></optionresponse>
-                    </problem>
-                    """)
-            )
         )
 
     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("""
+                <problem>
+                    <p>Dropdown</p>
+                    <optionresponse><optioninput label="Dropdown" options="('1', '2')" correct="'2'"></optioninput></optionresponse>
+                </problem>
+                """)
+        ))
+
         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])