Give OverdrivePatronRequests typed endpoint wrappers (PP-4938) - #3685
Give OverdrivePatronRequests typed endpoint wrappers (PP-4938)#3685jonathangreen wants to merge 4 commits into
Conversation
|
Claude finished @jonathangreen's task in 3m 43s —— View job SummaryClean, faithful refactor. I traced each migrated call site against its pre-change form and the requests on the wire are identical: DetailsNit:
|
Greptile SummaryThe PR moves fixed OverDrive patron operations behind typed request wrappers while retaining direct request handling for response-provided hypermedia links.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/integration/license/overdrive/api.py | Migrates patron business operations to typed request wrappers while preserving direct handling for hypermedia actions. |
| src/palace/manager/integration/license/overdrive/requests.py | Adds typed wrappers that encapsulate fixed patron endpoint construction, request payloads, and response validation. |
| tests/manager/integration/license/overdrive/test_api.py | Updates the token-refresh test to call the request-layer implementation directly. |
| tests/manager/integration/license/overdrive/test_requests.py | Adds focused coverage for every new wrapper’s verb, URL, payload, and validated response type. |
Sequence Diagram
sequenceDiagram
participant API as OverdriveAPI
participant Requests as OverdrivePatronRequests
participant Token as PatronTokenProvider
participant OD as OverDrive
API->>Requests: Typed patron operation(primitives)
Requests->>Token: Obtain patron access token
Requests->>OD: Constructed authenticated HTTP request
OD-->>Requests: Provider response
Requests-->>API: Validated OverDrive model
Reviews (6): Last reviewed commit: "Pin the empty hold email and the checkou..." | Re-trigger Greptile
59ac445 to
f8d7a31
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## chore/overdrive-request-spec #3685 +/- ##
=============================================================
Coverage 93.57% 93.57%
=============================================================
Files 514 514
Lines 46979 46992 +13
Branches 6407 6407
=============================================================
+ Hits 43962 43975 +13
Misses 1950 1950
Partials 1067 1067 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The fixed patron endpoints get named methods that own their URL and response type, so the API layer asks for a checkout or a hold rather than assembling one. Hypermedia actions stay on patron_request, since the URL comes from the document rather than from us. CHECKOUT_ENDPOINT comes back with them. It was dropped earlier in this series as an unused template; get_checkout is the caller it was missing, and using it keeps that method the same shape as delete_hold. follow_download_link is named for what it does rather than joining the endpoint group, because the link it follows comes from the loan document.
f8d7a31 to
5ad7d87
Compare
Call the hold identifier what its siblings call it. product_id was the name of the placeholder in the URL template, not of the thing callers pass, which is the same overdrive_id every other wrapper takes. Note the odd one out. Only get_checkout uppercases the identifier, which was invisible while these calls were spread across api.py and is obvious now they sit together. Nothing on record says why, so say that rather than letting a future reader make them uniform on a guess.
The branch tests truthiness, and the address is whatever Overdrive last reported for the patron, so an empty string suppresses the notice the same way a missing one does. Say that, so nobody adds a separate case for it. Narrow the field type to what the method builds. An int was never one of the values.
The previous commit documented that an empty address suppresses the hold notice, but only None and a real address were covered, so an is None refactor could have started sending an empty emailAddress with nothing to catch it. Assert the whole checkout path too, as the sibling tests do. Only the last segment was checked, which would not have caught a typo in the template this PR reintroduces.
1 participant
Description
Adds a named method per patron endpoint, each taking primitives and returning a validated model:
get_checkouts,get_checkout,create_checkout,get_holds,create_hold,delete_holdandget_patron_information.All the patron business methods in
OverdriveAPIare migrated onto them, and thepatron_requestdelegator on the API class is removed.Motivation and Context
Part of separating the Overdrive integration's HTTP concerns from its business logic (PP-4938). This is the step that makes the API class an adapter rather than a request builder: it no longer assembles URLs or request specs for fixed endpoints.
The hold notification email choice moves into
create_hold. Which field Overdrive expects,emailAddressorignoreHoldEmail, is a property of the endpoint rather than of our business logic, so it belongs with the request rather than at the call site.Hypermedia actions still go through
OverdrivePatronRequests.patron_requestdirectly, because the request they describe comes from the response document rather than from a fixed endpoint. That is the distinction the wrappers draw: fixed endpoints get a named method, and links the API document hands us do not.follow_download_linksits alongside them but is named for what it does, since the link it follows comes from the loan document rather than from a template we own.CHECKOUT_ENDPOINTreturns in this PR. It was dropped earlier in the series as a template with no callers;get_checkoutis the caller it was missing, and using it keeps that method the same shape asdelete_hold.flask.requestusage stays in the API layer, in_contentlink_fulfillment.How Has This Been Tested?
Adds coverage for each wrapper to the existing
TestOverdrivePatronRequests: the verb, the URL including the upper-casing of the identifier inget_checkout, and the serialized field payloads forcreate_checkoutand both branches ofcreate_hold.The business tests in
test_api.pysurvive nearly unchanged, which is the evidence that the requests on the wire are identical: they queue raw HTTP responses in strict order and assert on the recorded URLs, verbs and payloads.6002 passed.
mypyclean.Checklist