[PP-1509] Add support for default facet #2142

dbernstein · 2024-10-28T22:25:20Z

Description

With this update, the "default" facet or sort link will have a http://palaceproject.io/terms/properties/default property to indicate that it is a default property. This default property will be present in both OPDS1 and OPDS2 feeds. However in order to see them, clients must specify an api-version of 2 in the mimetype.

For OPDS 1:
application/atom+xml;api-version=2
For OPDS 2:
application/opds+json;api-version=2

Otherwise version 1 of both formats will be delivered. Version 1 is what is currently deployed in production.

Here is an update to the documentation that covers these updates.

I do have an open question about whether or not a default facet that happens to be active should have an "activeFacet" or "active-sort" set to true.

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-1506

How Has This Been Tested?

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-10-29T17:06:54Z

Codecov Report

Attention: Patch coverage is 93.38235% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.78%. Comparing base (7411b60) to head (dfa6472).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/palace/manager/feed/serializer/opds.py	89.47%	2 Missing and 4 partials ⚠️
src/palace/manager/feed/serializer/opds2.py	94.91%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2142   +/-   ##
=======================================
  Coverage   90.78%   90.78%           
=======================================
  Files         350      350           
  Lines       40608    40691   +83     
  Branches     8796     8822   +26     
=======================================
+ Hits        36864    36940   +76     
- Misses       2439     2441    +2     
- Partials     1305     1310    +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonathangreen

Just a few comments to look at before this one goes in.

Can you also update the PR with a small example of what these links look like in the different feed versions?

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

jonathangreen · 2024-10-30T14:27:36Z

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

-                # TODO: When we remove the facet-based sort links [PP-1814],
-                # this code path will be removed and we'll want to pull the sort
-                # link data from the feed.sort_links once that is in place.
-                link_data["links"].append(self._serialize_sort_link(link))


Should this TODO get relocated rather then removed?

src/palace/manager/feed/opds.py

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

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

tests/manager/feed/test_opds_base.py

jonathangreen · 2024-10-30T15:00:03Z

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

+            serialized.append(link)
+
+        for link in self._serialize_sort_links(feed.facet_links):
+            serialized.append(link)


Seems like it would be good to have a test that covers this line, to make sure we are getting the expected sort links in the OPDS1Version2 feed.

Still doesn't look like there is any coverage here?

dbernstein · 2024-10-30T15:39:19Z

@jonathangreen : thanks for the review. All your suggests look reasonable to me. I'll update the PR today.

jonathangreen

A couple more comments here that would be nice to get resolved before merging @dbernstein.

jonathangreen · 2024-11-05T14:23:20Z

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

-class OPDS1Serializer(SerializerInterface[etree._Element], OPDSFeed):
-    """An OPDS 1.2 Atom feed serializer"""
-
+class BaseOPDS1Serializer(SerializerInterface[etree._Element], OPDSFeed):


You also need to inherent from ABC in order to make this an abstract base class.

See:
https://docs.python.org/3/library/abc.html#abc.ABC

jonathangreen · 2024-11-05T14:24:41Z

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

@@ -79,7 +84,7 @@ def _tag(

    def _attr_name(self, attr_name: str, mapping: dict[str, str] | None = None) -> str:
        if not mapping:
-            mapping = ATTRIBUTE_MAPPING
+            mapping = V1_ATTRIBUTE_MAPPING


Since we defined _get_attribute_mapping, shouldn't this be calling that function?

jonathangreen · 2024-11-05T14:25:12Z

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

+            serialized.append(link)
+
+        for link in self._serialize_sort_links(feed.facet_links):
+            serialized.append(link)


Still doesn't look like there is any coverage here?

jonathangreen · 2024-11-05T14:26:40Z

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

                        value if value is not None else "",
                    )
        return entry

+    @abc.abstractmethod
+    def _get_attribute_mapping(self) -> dict[str, str]:


Minor: Since subclasses are expected to implement these, its would be nice to have a doc comment with a small bit of explanation of what an implementation should provide for this function.

jonathangreen · 2024-11-05T14:28:57Z

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

@@ -405,13 +413,95 @@ def _serialize_data_entry(self, entry: DataEntry) -> etree._Element:
    def to_string(cls, element: etree._Element) -> str:
        return cast(str, etree.tostring(element, encoding="unicode"))

+    @abc.abstractmethod
    def content_type(self) -> str:


Minor: Since subclasses are expected to implement these, its would be nice to have a doc comment with a small bit of explanation of what an implementation should provide for this function.

jonathangreen · 2024-11-05T14:29:01Z

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

+        pass
+
+    @abc.abstractmethod
+    def _serialize_facet_links(self, feed: FeedData) -> list[Link]:


Minor: Since subclasses are expected to implement these, its would be nice to have a doc comment with a small bit of explanation of what an implementation should provide for this function.

jonathangreen · 2024-11-05T14:29:05Z

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

+        pass
+
+    @abc.abstractmethod
+    def _serialize_sort_links(self, feed: FeedData) -> list[Link]:


Minor: Since subclasses are expected to implement these, its would be nice to have a doc comment with a small bit of explanation of what an implementation should provide for this function.

jonathangreen · 2024-11-05T14:33:27Z

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

+                # select only the sort facets for serialization
+                if is_sort_facet(link):
+                    links.append(self._serialize_sort_link(link))
+        return links

    def _serialize_sort_link(self, link: Link) -> etree._Element:


We aren't using the @override decorator anywhere else in the codebase currently. I think if we're going to apply it in these classes we should apply it consistently, so it should get added to this method as well.

jonathangreen · 2024-11-05T14:42:42Z

tests/manager/feed/test_opds_base.py

+        ],
+    )
+    def test_get_serializer1(self, accept_header: str, serializer):
+        # The q-value should take priority


Minor: Seems like this comment is outdated.

jonathangreen · 2024-11-05T14:49:26Z

tests/manager/api/controller/test_loan.py

+            (
+                OPDS2Serializer.CONTENT_TYPE,
+                OPDS2Serializer.CONTENT_TYPE,
+            ),


Suggested change

(

OPDS2Serializer.CONTENT_TYPE,

OPDS2Serializer.CONTENT_TYPE,

),

(OPDS2Serializer.CONTENT_TYPE, OPDS2Serializer.CONTENT_TYPE),

Minor: This change seems unnecessary. Its nice to have these be consistent, instead of having this one entry on multiple lines.

…when retrieving opds1 and opds2 client feeds.

…mat. Move support for sort links to OPD1Version2 and OPDS2Version2.

…erty on the sort and facet relations in OPDS1V2 and OPDS2V2 feeds.

dbernstein marked this pull request as draft October 28, 2024 22:25

dbernstein marked this pull request as ready for review October 29, 2024 21:18

dbernstein force-pushed the PP-1509-add-support-for-default-facet branch from 292d66b to 8ff807c Compare October 29, 2024 21:18

dbernstein requested a review from a team October 29, 2024 21:20

jonathangreen self-assigned this Oct 30, 2024

jonathangreen added the feature New feature label Oct 30, 2024

jonathangreen reviewed Oct 30, 2024

View reviewed changes

jonathangreen removed their assignment Oct 30, 2024

dbernstein force-pushed the PP-1509-add-support-for-default-facet branch 5 times, most recently from e03180c to a23686b Compare November 4, 2024 16:39

dbernstein requested a review from jonathangreen November 4, 2024 16:57

jonathangreen approved these changes Nov 5, 2024

View reviewed changes

dbernstein added 11 commits November 5, 2024 11:45

Add support for specifying the Palace api-version in mimetype header …

3d09af9

…when retrieving opds1 and opds2 client feeds.

Revert OPDS1Version1 and OPDS2Version1 to current production feed for…

a261cfd

…mat. Move support for sort links to OPD1Version2 and OPDS2Version2.

Add support for http://palaceproject.io/terms/properties/default prop…

2113e32

…erty on the sort and facet relations in OPDS1V2 and OPDS2V2 feeds.

Fix broken tests.

5da996e

Fix remaining tests.

fe6d961

Fix more tests.

2635755

Add test to ensure OPDS2Version1Serializer has not changed.

a8eac9b

Addresses PR feedaback.

dd312b9

Fix mypy and other compile errors.

af8ae67

Fix broken tests.

fb91e75

More updates to address PR feedback.

dfa6472

dbernstein force-pushed the PP-1509-add-support-for-default-facet branch from 2bce84e to dfa6472 Compare November 5, 2024 19:45

dbernstein merged commit c4ebcb7 into main Nov 5, 2024
21 checks passed

dbernstein deleted the PP-1509-add-support-for-default-facet branch November 5, 2024 19:58

tdilauro mentioned this pull request Dec 4, 2024

Better handle OPDS server root content types. (PP-1959) ThePalaceProject/library-registry#718

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PP-1509] Add support for default facet #2142

[PP-1509] Add support for default facet #2142

dbernstein commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

jonathangreen left a comment

jonathangreen Oct 30, 2024

jonathangreen Oct 30, 2024

jonathangreen Nov 5, 2024

dbernstein commented Oct 30, 2024

jonathangreen left a comment

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

jonathangreen Nov 5, 2024

[PP-1509] Add support for default facet #2142

[PP-1509] Add support for default facet #2142

Conversation

dbernstein commented Oct 28, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

jonathangreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbernstein commented Oct 30, 2024

jonathangreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbernstein commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading