Merge pull request #1061 from bookwyrm-social/first-search-result-bug

Uses one set of search logic for all results and first result only functions

bookwyrm/connectors/connector_manager.py

@@ -19,7 +19,7 @@ class ConnectorException(HTTPError):
     """when the connector can't do what was asked"""
 
 
-def search(query, min_confidence=0.1):
+def search(query, min_confidence=0.1, return_first=False):
     """find books based on arbitary keywords"""
     if not query:
         return []
@@ -31,18 +31,15 @@ def search(query, min_confidence=0.1):
 
     for connector in get_connectors():
         result_set = None
-        if maybe_isbn:
+        if maybe_isbn and connector.isbn_search_url and connector.isbn_search_url == "":
             # Search on ISBN
-            if not connector.isbn_search_url or connector.isbn_search_url == "":
+            try:
-                result_set = []
+                result_set = connector.isbn_search(isbn)
-            else:
+            except Exception as e:  # pylint: disable=broad-except
-                try:
+                logger.exception(e)
-                    result_set = connector.isbn_search(isbn)
+                # if this fails, we can still try regular search
-                except Exception as e:  # pylint: disable=broad-except
-                    logger.exception(e)
-                    continue
 
-        # if no isbn search or results, we fallback to generic search
+        # if no isbn search results, we fallback to generic search
         if result_set in (None, []):
             try:
                 result_set = connector.search(query, min_confidence=min_confidence)
@@ -51,6 +48,10 @@ def search(query, min_confidence=0.1):
                 logger.exception(e)
                 continue
 
+        if return_first and result_set:
+            # if we found anything, return it
+            return result_set[0]
+
         results.append(
             {
                 "connector": connector,
@@ -58,6 +59,9 @@ def search(query, min_confidence=0.1):
             }
         )
 
+    if return_first:
+        return None
+
     return results
 
 
@@ -77,11 +81,7 @@ def isbn_local_search(query, raw=False):
 
 def first_search_result(query, min_confidence=0.1):
     """search until you find a result that fits"""
-    for connector in get_connectors():
+    return search(query, min_confidence=min_confidence, return_first=True) or None
-        result = connector.search(query, min_confidence=min_confidence)
-        if result:
-            return result[0]
-    return None
 
 
 def get_connectors():

bookwyrm/tests/connectors/test_connector_manager.py

@@ -1,5 +1,6 @@
 """ interface between the app and various connectors """
 from django.test import TestCase
+import responses
 
 from bookwyrm import models
 from bookwyrm.connectors import connector_manager
@@ -17,6 +18,9 @@ class ConnectorManager(TestCase):
         self.edition = models.Edition.objects.create(
             title="Example Edition", parent_work=self.work, isbn_10="0000000000"
         )
+        self.edition = models.Edition.objects.create(
+            title="Another Edition", parent_work=self.work, isbn_10="1111111111"
+        )
 
         self.connector = models.Connector.objects.create(
             identifier="test_connector",
@@ -29,6 +33,18 @@ class ConnectorManager(TestCase):
             isbn_search_url="http://test.com/isbn/",
         )
 
+        self.remote_connector = models.Connector.objects.create(
+            identifier="test_connector_remote",
+            priority=1,
+            local=False,
+            connector_file="bookwyrm_connector",
+            base_url="http://fake.ciom/",
+            books_url="http://fake.ciom/",
+            search_url="http://fake.ciom/search/",
+            covers_url="http://covers.fake.ciom/",
+            isbn_search_url="http://fake.ciom/isbn/",
+        )
+
     def test_get_or_create_connector(self):
         """loads a connector if the data source is known or creates one"""
         remote_id = "https://example.com/object/1"
@@ -42,23 +58,38 @@ class ConnectorManager(TestCase):
 
     def test_get_connectors(self):
         """load all connectors"""
-        remote_id = "https://example.com/object/1"
-        connector_manager.get_or_create_connector(remote_id)
         connectors = list(connector_manager.get_connectors())
         self.assertEqual(len(connectors), 2)
         self.assertIsInstance(connectors[0], SelfConnector)
         self.assertIsInstance(connectors[1], BookWyrmConnector)
 
+    @responses.activate
     def test_search(self):
         """search all connectors"""
+        responses.add(
+            responses.GET,
+            "http://fake.ciom/search/Example?min_confidence=0.1",
+            json={},
+        )
         results = connector_manager.search("Example")
         self.assertEqual(len(results), 1)
         self.assertIsInstance(results[0]["connector"], SelfConnector)
         self.assertEqual(len(results[0]["results"]), 1)
         self.assertEqual(results[0]["results"][0].title, "Example Edition")
 
+    def test_search_empty_query(self):
+        """don't panic on empty queries"""
+        results = connector_manager.search("")
+        self.assertEqual(results, [])
+
+    @responses.activate
     def test_search_isbn(self):
         """special handling if a query resembles an isbn"""
+        responses.add(
+            responses.GET,
+            "http://fake.ciom/isbn/0000000000",
+            json={},
+        )
         results = connector_manager.search("0000000000")
         self.assertEqual(len(results), 1)
         self.assertIsInstance(results[0]["connector"], SelfConnector)
@@ -75,8 +106,22 @@ class ConnectorManager(TestCase):
         """only get one search result"""
         result = connector_manager.first_search_result("Example")
         self.assertEqual(result.title, "Example Edition")
-        no_result = connector_manager.first_search_result("dkjfhg")
+
-        self.assertIsNone(no_result)
+    def test_first_search_result_empty_query(self):
+        """only get one search result"""
+        result = connector_manager.first_search_result("")
+        self.assertIsNone(result)
+
+    @responses.activate
+    def test_first_search_result_no_results(self):
+        """only get one search result"""
+        responses.add(
+            responses.GET,
+            "http://fake.ciom/search/dkjfhg?min_confidence=0.1",
+            json={},
+        )
+        result = connector_manager.first_search_result("dkjfhg")
+        self.assertIsNone(result)
 
     def test_load_connector(self):
         """load a connector object from the database entry"""