Merge branch 'main' into feature/rename-integration-settings-property

api/controller.py

@@ -1136,7 +1136,7 @@ def search(self, lane_identifier, feed_class=AcquisitionFeed):
 
         facets = self._load_search_facets(lane)
         if isinstance(facets, ProblemDetail):
-            return lane
+            return facets
 
         search_engine = self.search_engine
         if isinstance(search_engine, ProblemDetail):

core/lane.py

@@ -4,7 +4,7 @@
 import logging
 import time
 from collections import defaultdict
-from typing import TYPE_CHECKING, List, Optional
+from typing import TYPE_CHECKING, Any, List, Optional
 from urllib.parse import quote_plus
 
 from flask_babel import lazy_gettext as _
@@ -422,7 +422,7 @@ def available_facets(cls, config, facet_group_name):
         that might not be enabled in library configuration, but you
         can't make up totally new facets.
 
-        TODO: This sytem would make more sense if you _could_ make up
+        TODO: This system would make more sense if you _could_ make up
         totally new facets, maybe because each facet was represented
         as a policy object rather than a key to code implemented
         elsewhere in this class. Right now this method implies more
@@ -448,7 +448,9 @@ def default_facet(cls, config, facet_group_name):
         return config.default_facet(facet_group_name)
 
     @classmethod
-    def _values_from_request(cls, config, get_argument, get_header):
+    def _values_from_request(
+        cls, config, get_argument, get_header
+    ) -> dict[str, Any] | ProblemDetail:
         g = Facets.ORDER_FACET_GROUP_NAME
         order = get_argument(g, cls.default_facet(config, g))
         order_facets = cls.available_facets(config, g)
@@ -484,10 +486,14 @@ def _values_from_request(cls, config, get_argument, get_header):
         g = Facets.DISTRIBUTOR_FACETS_GROUP_NAME
         distributor = get_argument(g, cls.default_facet(config, g))
         distributor_facets = cls.available_facets(config, g)
-        if distributor and distributor not in distributor_facets:
+        if (
+            distributor
+            and distributor != "All"
+            and distributor not in distributor_facets
+        ):
             return INVALID_INPUT.detailed(
                 _(
-                    "I don't understand what '%(distributor)s' refers to.",
+                    "I don't understand which distributor '%(distributor)s' refers to.",
                     distributor=distributor,
                 ),
                 400,
@@ -496,10 +502,14 @@ def _values_from_request(cls, config, get_argument, get_header):
         g = Facets.COLLECTION_NAME_FACETS_GROUP_NAME
         collection_name = get_argument(g, cls.default_facet(config, g))
         collection_name_facets = cls.available_facets(config, g)
-        if collection_name and collection_name not in collection_name_facets:
+        if (
+            collection_name
+            and collection_name != "All"
+            and collection_name not in collection_name_facets
+        ):
             return INVALID_INPUT.detailed(
                 _(
-                    "I don't understand what '%(collection_name)s' refers to.",
+                    "I don't understand which collection '%(collection_name)s' refers to.",
                     collection_name=collection_name,
                 ),
                 400,

pyproject.toml

@@ -179,7 +179,7 @@ PyJWT = "2.6.0"
 PyLD = "2.0.3"
 pymarc = "5.1.0"
 pyOpenSSL = "^23.1.0"
-pyparsing = "3.1.0"
+pyparsing = "3.1.1"
 pyspellchecker = "0.7.2"
 python = ">=3.8,<4"
 python-dateutil = "2.8.2"

tests/api/test_controller_opdsfeed.py

@@ -567,7 +567,7 @@ def search(cls, **kwargs):
                 library_short_name=library.short_name,
                 **dict(list(facets.items())),
                 q=query,
-                _external=True
+                _external=True,
             )
         assert expect_url == kwargs.pop("url")
 
@@ -606,6 +606,61 @@ def search(cls, **kwargs):
             )
             assert self.called_with["facets"].search_type == "json"
 
+    def test_lane_search_params(
+        self,
+        circulation_fixture: CirculationControllerFixture,
+    ):
+        # Tests some of the lane search parameters.
+        # TODO: Add test for valid `distributor`.
+
+        valid_lane_id = circulation_fixture.english_adult_fiction.id
+        valid_collection_name = circulation_fixture.collection.name
+        invalid_collection_name = "__non-existent-collection__"
+        invalid_distributor = "__non-existent-distributor__"
+
+        with circulation_fixture.request_context_with_library("/?collectionName=All"):
+            response = circulation_fixture.manager.opds_feeds.search(valid_lane_id)
+            assert 200 == response.status_code
+            assert "application/opensearchdescription+xml" == response.headers.get(
+                "content-type"
+            )
+
+        with circulation_fixture.request_context_with_library(
+            f"/?collectionName={valid_collection_name}"
+        ):
+            response = circulation_fixture.manager.opds_feeds.search(valid_lane_id)
+            assert 200 == response.status_code
+            assert "application/opensearchdescription+xml" == response.headers.get(
+                "content-type"
+            )
+
+        with circulation_fixture.request_context_with_library(
+            f"/?collectionName={invalid_collection_name}"
+        ):
+            response = circulation_fixture.manager.opds_feeds.search(valid_lane_id)
+            assert 400 == response.status_code
+            assert (
+                f"I don't understand which collection '{invalid_collection_name}' refers to."
+                == response.detail
+            )
+
+        with circulation_fixture.request_context_with_library("/?distributor=All"):
+            response = circulation_fixture.manager.opds_feeds.search(valid_lane_id)
+            assert 200 == response.status_code
+            assert "application/opensearchdescription+xml" == response.headers.get(
+                "content-type"
+            )
+
+        with circulation_fixture.request_context_with_library(
+            f"/?distributor={invalid_distributor}"
+        ):
+            response = circulation_fixture.manager.opds_feeds.search(valid_lane_id)
+            assert 400 == response.status_code
+            assert (
+                f"I don't understand which distributor '{invalid_distributor}' refers to."
+                == response.detail
+            )
+
     def test_misconfigured_search(
         self, circulation_fixture: CirculationControllerFixture
     ):