Merge pull request #23 from edx/dsego/SOL-875

handle case when exclude_dictionary is empty
openedx · Jun 8, 2015 · d97f602 · d97f602
2 parents ae459ea + 35e583a
commit d97f602
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 1 deletion.
diff --git a/search/elastic.py b/search/elastic.py
@@ -127,14 +127,23 @@ def filter_item(field):
 
 
 def _process_exclude_dictionary(exclude_dictionary):
-    """ We have a list of ids to exclude from the resultset """
+    """
+    Based on values in the exclude_dictionary generate a list of term queries that
+    will filter out unwanted results.
+    """
+    # not_properties will hold the generated term queries.
     not_properties = []
     for exclude_property in exclude_dictionary:
         exclude_values = exclude_dictionary[exclude_property]
         if not isinstance(exclude_values, list):
             exclude_values = [exclude_values]
         not_properties.extend([{"term": {exclude_property: exclude_value}} for exclude_value in exclude_values])
 
+    # Returning a query segment with an empty list freaks out ElasticSearch,
+    #   so just return an empty segment.
+    if not not_properties:
+        return {}
+
     return {
         "not": {
             "filter": {

diff --git a/search/tests/tests.py b/search/tests/tests.py
@@ -744,6 +744,35 @@ def test_exclude_filter_multiple(self):
         self.assertNotIn("FAKE_ID_4", result_ids)
         self.assertIn("FAKE_ID_5", result_ids)
 
+    def test_exclude_filter_empty(self):
+        """ Test that search works when exclude filter is an empty list """
+        self.searcher.index("test_doc", {"course": "ABC", "org": "edX", "id": "FAKE_ID_1"})
+        self.searcher.index("test_doc", {"course": "XYZ", "org": "edX", "id": "FAKE_ID_2"})
+        self.searcher.index("test_doc", {"course": "DEF", "org": "MITX", "id": "FAKE_ID_3"})
+        self.searcher.index("test_doc", {"course": "GHI", "org": "HarvardX", "id": "FAKE_ID_4"})
+        self.searcher.index("test_doc", {"course": "LMN", "org": "edX", "id": "FAKE_ID_5"})
+
+        response = self.searcher.search(exclude_dictionary={"org": []})
+        self.assertEqual(response["total"], 5)
+        result_ids = [r["data"]["id"] for r in response["results"]]
+        self.assertIn("FAKE_ID_1", result_ids)
+        self.assertIn("FAKE_ID_2", result_ids)
+        self.assertIn("FAKE_ID_3", result_ids)
+        self.assertIn("FAKE_ID_4", result_ids)
+        self.assertIn("FAKE_ID_5", result_ids)
+
+        response = self.searcher.search(
+            field_dictionary={"course": ["XYZ", "LMN", "DEF"]},
+            exclude_dictionary={"org": []}
+        )
+        self.assertEqual(response["total"], 3)
+        result_ids = [r["data"]["id"] for r in response["results"]]
+        self.assertNotIn("FAKE_ID_1", result_ids)
+        self.assertIn("FAKE_ID_2", result_ids)
+        self.assertIn("FAKE_ID_3", result_ids)
+        self.assertNotIn("FAKE_ID_4", result_ids)
+        self.assertIn("FAKE_ID_5", result_ids)
+
 
 @override_settings(SEARCH_ENGINE="search.tests.utils.ForceRefreshElasticSearchEngine")
 class ElasticSearchTests(MockSearchTests):