More updates to address PR feedback.

src/palace/manager/feed/serializer/opds.py

@@ -6,7 +6,6 @@
 from typing import Any, cast
 
 from lxml import etree
-from typing_extensions import override
 
 from palace.manager.core.facets import FacetConstants
 from palace.manager.feed.serializer.base import SerializerInterface
@@ -71,7 +70,7 @@ def is_sort_facet(link: Link) -> bool:
     )
 
 
-class BaseOPDS1Serializer(SerializerInterface[etree._Element], OPDSFeed):
+class BaseOPDS1Serializer(SerializerInterface[etree._Element], OPDSFeed, abc.ABC):
     def __init__(self) -> None:
         pass
 
@@ -84,7 +83,7 @@ def _tag(
 
     def _attr_name(self, attr_name: str, mapping: dict[str, str] | None = None) -> str:
         if not mapping:
-            mapping = V1_ATTRIBUTE_MAPPING
+            mapping = self._get_attribute_mapping()
         return mapping.get(attr_name, attr_name)
 
     def serialize_feed(
@@ -326,7 +325,9 @@ def _serialize_feed_entry(
 
     @abc.abstractmethod
     def _get_attribute_mapping(self) -> dict[str, str]:
-        pass
+        """This method should return a mapping of object attributes found on links and objects in the FeedData
+        to the related attribute names defined in the OPDS specification.
+        """
 
     def _serialize_author_tag(self, tag: str, author: Author) -> etree._Element:
         entry: etree._Element = self._tag(tag)
@@ -415,15 +416,15 @@ def to_string(cls, element: etree._Element) -> str:
 
     @abc.abstractmethod
     def content_type(self) -> str:
-        pass
+        """return the content type associated with the serialization. This value should include the api-version."""
 
     @abc.abstractmethod
     def _serialize_facet_links(self, feed: FeedData) -> list[Link]:
-        pass
+        """This method implements serialization of the facet_links from the feed data."""
 
     @abc.abstractmethod
     def _serialize_sort_links(self, feed: FeedData) -> list[Link]:
-        pass
+        """This method implements serialization of the sort links from the feed data."""
 
 
 class OPDS1Version1Serializer(BaseOPDS1Serializer):
@@ -432,25 +433,21 @@ class OPDS1Version1Serializer(BaseOPDS1Serializer):
     the http://palaceproject.io/terms/properties/default property indicating default facets
     """
 
-    @override
     def _serialize_facet_links(self, feed: FeedData) -> list[Link]:
         links = []
         if feed.facet_links:
             for link in feed.facet_links:
                 links.append(self._serialize_feed_entry("link", link))
         return links
 
-    @override
     def _serialize_sort_links(self, feed: FeedData) -> list[Link]:
         # Since this version of the serializer implements sort links as facets,
         # we return an empty list of sort links.
         return []
 
-    @override
     def _get_attribute_mapping(self) -> dict[str, str]:
         return V1_ATTRIBUTE_MAPPING
 
-    @override
     def content_type(self) -> str:
         return OPDSFeed.ACQUISITION_FEED_TYPE + "; api-version=1"
 
@@ -463,8 +460,8 @@ class OPDS1Version2Serializer(BaseOPDS1Serializer):
     inidcated by the http://palaceproject.io/terms/properties/default property.
     """
 
-    @override
     def _serialize_facet_links(self, feed: FeedData) -> list[Link]:
+        # serializes the non-sort related facets.
         links: list[Link] = []
         facet_links = feed.facet_links
         if facet_links:
@@ -474,13 +471,15 @@ def _serialize_facet_links(self, feed: FeedData) -> list[Link]:
                     links.append(self._serialize_feed_entry("link", link))
         return links
 
-    @override
     def _serialize_sort_links(self, feed: FeedData) -> list[Link]:
+        # this version of the feed filters out the sort facets and
+        # serializes them in a way that makes use of palace extensions.
         links: list[Link] = []
         facet_links = feed.facet_links
         if facet_links:
             for link in feed.facet_links:
                 # select only the sort facets for serialization
+
                 if is_sort_facet(link):
                     links.append(self._serialize_sort_link(link))
         return links
@@ -498,10 +497,8 @@ def _serialize_sort_link(self, link: Link) -> etree._Element:
 
         return self._serialize_feed_entry("link", sort_link)
 
-    @override
     def _get_attribute_mapping(self) -> dict[str, str]:
         return V2_ATTRIBUTE_MAPPING
 
-    @override
     def content_type(self) -> str:
         return OPDSFeed.ACQUISITION_FEED_TYPE + "; api-version=2"

src/palace/manager/feed/serializer/opds2.py

@@ -237,6 +237,8 @@ def _serialize_facet_links(self, feed: FeedData) -> list[dict[str, Any]]:
         results = []
         facet_links: dict[str, Any] = defaultdict(lambda: {"metadata": {}, "links": []})
         for link in feed.facet_links:
+            # TODO: When we remove the facet-based sort links [PP-1814],
+            # this check can be removed.
             if not is_sort_facet(link):
                 group = getattr(link, "facetGroup", None)
                 if group:
@@ -249,6 +251,8 @@ def _serialize_facet_links(self, feed: FeedData) -> list[dict[str, Any]]:
 
     def _serialize_sort_links(self, feed: FeedData) -> list[dict[str, Any]]:
         sort_links = []
+        # TODO: When we remove the facet-based sort links [PP-1814],
+        # we'll want to pull the sort link data from the feed.sort_links once that is in place.
         if feed.facet_links:
             for link in feed.facet_links:
                 if is_sort_facet(link):

tests/manager/api/controller/test_loan.py

@@ -131,10 +131,7 @@ class OPDSSerializationTestHelper:
             (None, OPDSFeed.ENTRY_TYPE),
             ("default-foo-bar", OPDSFeed.ENTRY_TYPE),
             (AtomFeed.ATOM_TYPE, OPDSFeed.ENTRY_TYPE),
-            (
-                OPDS2Serializer.CONTENT_TYPE,
-                OPDS2Serializer.CONTENT_TYPE,
-            ),
+            (OPDS2Serializer.CONTENT_TYPE, OPDS2Serializer.CONTENT_TYPE),
         ],
     )
 

tests/manager/feed/test_opds_base.py

@@ -59,6 +59,5 @@ class TestBaseOPDSFeed:
         ],
     )
     def test_get_serializer1(self, accept_header: str, serializer):
-        # The q-value should take priority
         request = Request.from_values(headers=dict(Accept=accept_header))
         assert isinstance(get_serializer(request.accept_mimetypes), serializer)

tests/manager/feed/test_opds_serializer.py

@@ -1,4 +1,5 @@
 import datetime
+from unittest.mock import patch
 
 import pytz
 from lxml import etree
@@ -292,6 +293,11 @@ def test_serialize_sort_link_v2(self):
             == "true"
         )
 
+        with patch.object(serializer, "_serialize_sort_links") as serialize_sort_links:
+
+            serializer.serialize_feed(feed)
+            assert serialize_sort_links.call_count == 1
+
     def test_serialize_non_sort_facetgroup_link_v2(self):
         facet_link = Link(href="test", rel="test_rel", title="text1")
         facet_link.add_attributes(