Merge pull request #14284 from open-craft/haikuginger/filter-by-block-name

Restrict block ID comparison slightly to avoid false positives

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

@@ -57,6 +57,7 @@ import copy
 import datetime
 import hashlib
 import logging
+import six
 from contracts import contract, new_contract
 from importlib import import_module
 from mongodb_proxy import autoretry_read
@@ -1202,9 +1203,17 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             block_name = qualifiers.pop('name')
             block_ids = []
             for block_id, block in course.structure['blocks'].iteritems():
-                # Do an in comparison on the name qualifier
-                # so that a list can be used to filter on block_id
-                if block_id.id in block_name and _block_matches_all(block):
+                # Don't do an in comparison blindly; first check to make sure
+                # that the name qualifier we're looking at isn't a plain string;
+                # if it is a string, then it should match exactly. If it's other
+                # than a string, we check whether it contains the block ID; this
+                # is so a list or other iterable can be passed with multiple
+                # valid qualifiers.
+                if isinstance(block_name, six.string_types):
+                    name_matches = block_id.id == block_name
+                else:
+                    name_matches = block_id.id in block_name
+                if name_matches and _block_matches_all(block):
                     block_ids.append(block_id)
 
             return self._load_items(course, block_ids, **kwargs)

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

@@ -267,6 +267,15 @@ class SplitModuleTest(unittest.TestCase):
                             },
                         },
                         {
+                            "id": "chap",
+                            "parent": "head12345",
+                            "parent_type": "course",
+                            "category": "chapter",
+                            "fields": {
+                                "display_name": "Buffalo buffalo Buffalo buffalo buffalo buffalo Buffalo buffalo"
+                            },
+                        },
+                        {
                             "id": "chapter2",
                             "parent": "head12345",
                             "parent_type": "course",
@@ -620,7 +629,7 @@ class SplitModuleCourseTests(SplitModuleTest):
             course.advertised_start, "Fall 2013",
             "advertised_start"
         )
-        self.assertEqual(len(course.children), 3, "children")
+        self.assertEqual(len(course.children), 4, "children")
         # check dates and graders--forces loading of descriptor
         self.assertEqual(course.edited_by, "testassist@edx.org")
         self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.45})
@@ -726,7 +735,7 @@ class SplitModuleCourseTests(SplitModuleTest):
         self.assertEqual(len(course.tabs), 6)
         self.assertEqual(course.display_name, "The Ancient Greek Hero")
         self.assertEqual(course.advertised_start, "Fall 2013")
-        self.assertEqual(len(course.children), 3)
+        self.assertEqual(len(course.children), 4)
         # check dates and graders--forces loading of descriptor
         self.assertEqual(course.edited_by, "testassist@edx.org")
         self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.45})
@@ -1094,7 +1103,7 @@ class SplitModuleItemTests(SplitModuleTest):
             self.assertEqual(len(block.tabs), 6, "wrong number of tabs")
             self.assertEqual(block.display_name, "The Ancient Greek Hero")
             self.assertEqual(block.advertised_start, "Fall 2013")
-            self.assertEqual(len(block.children), 3)
+            self.assertEqual(len(block.children), 4)
             # check dates and graders--forces loading of descriptor
             self.assertEqual(block.edited_by, "testassist@edx.org")
             self.assertDictEqual(
@@ -1189,13 +1198,14 @@ class SplitModuleItemTests(SplitModuleTest):
         locator = CourseLocator(org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_DRAFT)
         # get all modules
         matches = modulestore().get_items(locator)
-        self.assertEqual(len(matches), 7)
+        self.assertEqual(len(matches), 8)
         matches = modulestore().get_items(locator)
-        self.assertEqual(len(matches), 7)
+        self.assertEqual(len(matches), 8)
         matches = modulestore().get_items(locator, qualifiers={'category': 'chapter'})
-        self.assertEqual(len(matches), 3)
+        self.assertEqual(len(matches), 4)
         matches = modulestore().get_items(locator, qualifiers={'category': 'garbage'})
         self.assertEqual(len(matches), 0)
+        # Test that we don't accidentally get an item with a similar name.
         matches = modulestore().get_items(locator, qualifiers={'name': 'chapter1'})
         self.assertEqual(len(matches), 1)
         matches = modulestore().get_items(locator, qualifiers={'name': ['chapter1', 'chapter2']})
@@ -1209,7 +1219,7 @@ class SplitModuleItemTests(SplitModuleTest):
         matches = modulestore().get_items(locator, settings={'group_access': {'$exists': True}})
         self.assertEqual(len(matches), 1)
         matches = modulestore().get_items(locator, settings={'group_access': {'$exists': False}})
-        self.assertEqual(len(matches), 6)
+        self.assertEqual(len(matches), 7)
 
     def test_get_parents(self):
         '''
@@ -1243,7 +1253,7 @@ class SplitModuleItemTests(SplitModuleTest):
         block = modulestore().get_item(locator)
         children = block.get_children()
         expected_ids = [
-            "chapter1", "chapter2", "chapter3"
+            "chapter1", "chap", "chapter2", "chapter3"
         ]
         for child in children:
             self.assertEqual(child.category, "chapter")