OPDS ODL API changes for UL Feed (PP-1769) #2095

jonathangreen · 2024-10-01T23:11:06Z

Description

Some major updates to the OPDS2 + ODL API code.

The highlights are:

Switch the validation of the status documents we get back to use Pydantic
- There is some work here to start building up models of OPDS2 and ODL that can be the basis of replacing webpub manifest parser when we get to it.
Update the checkout / checkin / fulfillment logic, to reflect the differences between the documents we get back from DeMarque and those returned by the Open Source LCP server stack.
- While I was in here, I updated the fulfillment logic to support ODL without LCP encryption.
Using the correct HTTP methods as defined in the spec, rather then using GET for everything
- DeMarque supports using GET, but LCP server doesn't, and we should be following the spec

Motivation and Context

Starting to pull in ODL feeds from other distributors.

JIRA: PP-1769

How Has This Been Tested?

Running unit tests
Manually testing all the different DRM / content type interactions from marketplace via the app
- This is complete, the interactions were tested in Cerberus.
  - LCP Ebook
  - LCP Audiobook
  - Adobe Ebook
  - DeMarque Audiobook
  - UL Abook
  - UL Ebook

Checklist

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

codecov · 2024-10-01T23:19:27Z

Codecov Report

Attention: Patch coverage is 96.10656% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.69%. Comparing base (4f4295e) to head (c0b4b90).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
src/palace/manager/api/odl/api.py	94.28%	2 Missing and 4 partials ⚠️
src/palace/manager/opds/authentication.py	89.74%	2 Missing and 2 partials ⚠️
src/palace/manager/opds/lcp/license.py	92.45%	2 Missing and 2 partials ⚠️
src/palace/manager/opds/odl/info.py	92.85%	3 Missing ⚠️
src/palace/manager/opds/base.py	97.22%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2095      +/-   ##
==========================================
+ Coverage   90.65%   90.69%   +0.03%     
==========================================
  Files         344      351       +7     
  Lines       40580    40878     +298     
  Branches     8822     8858      +36     
==========================================
+ Hits        36787    37073     +286     
- Misses       2485     2493       +8     
- Partials     1308     1312       +4

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

tests/manager/scripts/test_playtime_entries.py

src/palace/manager/api/odl/api.py

jonathangreen · 2024-10-02T00:22:22Z

src/palace/manager/api/odl/api.py

+        # an extra request to try to get the information we need from the 'status' link in the license
+        # document, which the LCP server does provide.
+        # TODO: Raise this issue with LCP server maintainers, and try to get a fix in place.
+        #   once that is done, we should be able to remove this fallback.


Started a slack thread about this here: https://readium.slack.com/archives/C0SGTMMR6/p1727814143241779

src/palace/manager/api/odl/api.py

jonathangreen · 2024-10-02T15:11:39Z

Okay I finished the testing on this one, so this one is ready for code review / merge.

tdilauro

This looks good to me. 🍄

A couple small comments below.

tdilauro · 2024-10-05T01:42:53Z

src/palace/manager/api/odl/api.py

+            )
+            fulfill_cls = RedirectFulfillment
+        elif drm_scheme == DeliveryMechanism.FEEDBOOKS_AUDIOBOOK_DRM:
+            # For DeMarque audiobook content, the link we are looking is stored in the 'manifest' rel.


Minor: missing word in the comment? "...link we are looking [up? for? is..."

tdilauro · 2024-10-05T01:46:00Z

tests/manager/api/controller/test_odl_notify.py

+            library=odl_fixture.library,
+            data=odl_fixture.loan_status_document("returned").model_dump_json(),
+        ):
+            response = odl_fixture.controller.notify(-55)


I think this -55 is meant to represent a loan that can't exist, but it's not super clear in this context. Would be useful to add a comment here (and a few lines down) making that explicit. Or we could use a constant named something like "NON_EXISTENT_LOAN".

This reverts commit 197f070.

jonathangreen requested a review from a team October 1, 2024 23:55

jonathangreen commented Oct 2, 2024

View reviewed changes

jonathangreen requested review from a team and removed request for a team October 2, 2024 00:26

jonathangreen marked this pull request as ready for review October 2, 2024 15:09

jonathangreen force-pushed the feature/odl-update branch 2 times, most recently from 9dbbccb to ff3bc90 Compare October 4, 2024 12:23

tdilauro approved these changes Oct 5, 2024

View reviewed changes

jonathangreen added 16 commits October 5, 2024 14:14

Update ODL code

5c94178

Updated

706f239

Udpated

eb369b2

New models with tests

23e8b3f

Update tests

2183bc6

Updates

5354f69

Fix mypy issues

ab24911

Fix playtime entries scripts tests

61e8050

Fix issue with backports import

2857bf8

Add an additional test

6a82f78

Add helpful comment

b064b3d

Add additional book medium

58578db

Add a couple more example documents, and update the logic a bit

01eae20

Update test fixture

7fa960b

Update the notification controller

5742061

Revert "Fix playtime entries scripts tests"

9a86853

This reverts commit 197f070.

jonathangreen force-pushed the feature/odl-update branch from ff3bc90 to 9a86853 Compare October 5, 2024 17:17

Code review feedback

c0b4b90

jonathangreen merged commit ea60979 into main Oct 5, 2024
20 checks passed

jonathangreen deleted the feature/odl-update branch October 5, 2024 21:47

dbernstein added the feature New feature label Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPDS ODL API changes for UL Feed (PP-1769) #2095

OPDS ODL API changes for UL Feed (PP-1769) #2095

jonathangreen commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

jonathangreen Oct 2, 2024

jonathangreen commented Oct 2, 2024

tdilauro left a comment

tdilauro Oct 5, 2024

tdilauro Oct 5, 2024

OPDS ODL API changes for UL Feed (PP-1769) #2095

OPDS ODL API changes for UL Feed (PP-1769) #2095

Conversation

jonathangreen commented Oct 1, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

jonathangreen Oct 2, 2024

Choose a reason for hiding this comment

jonathangreen commented Oct 2, 2024

tdilauro left a comment

Choose a reason for hiding this comment

tdilauro Oct 5, 2024

Choose a reason for hiding this comment

tdilauro Oct 5, 2024

Choose a reason for hiding this comment

jonathangreen commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading