More updates

src/palace/manager/api/circulation.py

@@ -52,6 +52,7 @@
 from palace.manager.sqlalchemy.model.library import Library
 from palace.manager.sqlalchemy.model.licensing import (
     DeliveryMechanism,
+    License,
     LicensePool,
     LicensePoolDeliveryMechanism,
     RightsStatus,
@@ -350,6 +351,7 @@ class LoanInfo(LoanAndHoldInfoMixin):
     fulfillment: Fulfillment | None = None
     external_identifier: str | None = None
     locked_to: DeliveryMechanismInfo | None = None
+    license_identifier: str | None = None
 
     @classmethod
     def from_license_pool(
@@ -361,6 +363,7 @@ def from_license_pool(
         fulfillment: Fulfillment | None = None,
         external_identifier: str | None = None,
         locked_to: DeliveryMechanismInfo | None = None,
+        license_identifier: str | None = None,
     ) -> Self:
         collection_id = license_pool.collection_id
         assert collection_id is not None
@@ -377,6 +380,7 @@ def from_license_pool(
             fulfillment=fulfillment,
             external_identifier=external_identifier,
             locked_to=locked_to,
+            license_identifier=license_identifier,
         )
 
     def __repr__(self) -> str:
@@ -397,13 +401,25 @@ def create_or_update(
     ) -> tuple[Loan, bool]:
         session = Session.object_session(patron)
         license_pool = license_pool or self.license_pool(session)
-        loan, is_new = license_pool.loan_to(
+
+        if self.license_identifier is not None:
+            loanable = session.execute(
+                select(License).where(
+                    License.identifier == self.license_identifier,
+                    License.license_pool == license_pool,
+                )
+            ).scalar_one()
+        else:
+            loanable = license_pool
+
+        loan, is_new = loanable.loan_to(
             patron,
             start=self.start_date,
             end=self.end_date,
             fulfillment=self.fulfillment,
             external_identifier=self.external_identifier,
         )
+
         if self.locked_to:
             # The loan source is letting us know that the loan is
             # locked to a specific delivery mechanism. Even if

src/palace/manager/api/controller/odl_notification.py

@@ -4,6 +4,7 @@
 from flask import Response
 from flask_babel import lazy_gettext as _
 from pydantic import ValidationError
+from sqlalchemy import select
 from sqlalchemy.orm import Session
 
 from palace.manager.api.odl.api import OPDS2WithODLApi
@@ -16,11 +17,13 @@
 from palace.manager.service.integration_registry.license_providers import (
     LicenseProvidersRegistry,
 )
-from palace.manager.sqlalchemy.model.patron import Loan
+from palace.manager.sqlalchemy.model.credential import Credential
+from palace.manager.sqlalchemy.model.licensing import License
+from palace.manager.sqlalchemy.model.patron import Loan, Patron
 from palace.manager.sqlalchemy.util import get_one
 from palace.manager.util.datetime_helpers import utc_now
 from palace.manager.util.log import LoggerMixin
-from palace.manager.util.problem_detail import ProblemDetail
+from palace.manager.util.problem_detail import ProblemDetailException
 
 
 class ODLNotificationController(LoggerMixin):
@@ -36,22 +39,47 @@ def __init__(
         self.db = db
         self.registry = registry
 
-    def notify(self, loan_id: int) -> Response | ProblemDetail:
-        status_doc_json = flask.request.data
+    def _get_loan(self, patron_identifier: str, license_identifier: str) -> Loan | None:
+        return self.db.execute(
+            select(Loan)
+            .join(License)
+            .join(Patron)
+            .join(Credential)
+            .where(
+                License.identifier == license_identifier,
+                Credential.credential == patron_identifier,
+                Credential.type == Credential.IDENTIFIER_TO_REMOTE_SERVICE,
+            )
+        ).scalar_one_or_none()
+
+    def notify(self, patron_identifier: str, license_identifier: str) -> Response:
+        loan = self._get_loan(patron_identifier, license_identifier)
+        return self._process_notification(loan)
+
+    # TODO: This method is deprecated and should be removed once all the loans
+    #   created using the old endpoint have expired.
+    def notify_deprecated(self, loan_id: int) -> Response:
         loan = get_one(self.db, Loan, id=loan_id)
+        return self._process_notification(loan)
+
+    def _process_notification(self, loan: Loan | None) -> Response:
+        status_doc_json = flask.request.data
 
         try:
             status_doc = LoanStatus.model_validate_json(status_doc_json)
         except ValidationError as e:
             self.log.exception(f"Unable to parse loan status document. {e}")
-            return INVALID_INPUT
+            raise ProblemDetailException(INVALID_INPUT) from e
 
         # We don't have a record of this loan. This likely means that the loan has been returned
         # and our local record has been deleted. This is expected, except in the case where the
         # distributor thinks the loan is still active.
         if loan is None and status_doc.active:
-            return NO_ACTIVE_LOAN.detailed(
-                _("No loan was found for this identifier."), status_code=404
+            self.log.error(
+                f"No loan found for active OPDS + ODL Notification. Document: {status_doc.model_dump_json()}"
+            )
+            raise ProblemDetailException(
+                NO_ACTIVE_LOAN.detailed(_("No loan was found."), status_code=404)
             )
 
         if loan:
@@ -60,9 +88,9 @@ def notify(self, loan_id: int) -> Response | ProblemDetail:
                 not integration.protocol
                 or self.registry.get(integration.protocol) != OPDS2WithODLApi
             ):
-                return INVALID_LOAN_FOR_ODL_NOTIFICATION
+                raise ProblemDetailException(INVALID_LOAN_FOR_ODL_NOTIFICATION)
 
-            # TODO: This should really just trigger a celery task to do an availabilty sync on the
+            # TODO: This should really just trigger a celery task to do an availability sync on the
             #   license, since this is flagging that we might be out of sync with the distributor.
             #   Once we move the OPDS2WithODL scripts to celery this should be possible.
             #   For now we just mark the loan as expired.

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

@@ -6,7 +6,7 @@
 import uuid
 from collections.abc import Callable
 from functools import cached_property, partial
-from typing import Any, Literal
+from typing import Any
 
 from dependency_injector.wiring import Provide, inject
 from flask import url_for
@@ -217,22 +217,23 @@ def checkin(self, patron: Patron, pin: str, licensepool: LicensePool) -> None:
 
         self._checkin(loan_result)
 
-    def _checkin(self, loan: Loan) -> bool:
+    def _checkin(self, loan: Loan) -> None:
         _db = Session.object_session(loan)
         if loan.external_identifier is None:
             # We can't return a loan that doesn't have an external identifier. This should never happen
-            # but if it does, we log an error and delete the loan, so it doesn't stay on the patrons
+            # but if it does, we log an error and continue on, so it doesn't stay on the patrons
             # bookshelf forever.
             self.log.error(f"Loan {loan.id} has no external identifier.")
-            return False
+            return
 
         doc = self._request_loan_status("GET", loan.external_identifier)
         if not doc.active:
             self.log.warning(
                 f"Loan {loan.id} was already returned early, revoked by the distributor, or it expired."
             )
-            self.update_loan(loan, doc)
-            return False
+            loan.license.checkin()
+            loan.license_pool.update_availability_from_licenses()
+            return
 
         return_link = doc.links.get(rel="return", type=LoanStatus.content_type())
         if not return_link:
@@ -248,7 +249,7 @@ def _checkin(self, loan: Loan) -> bool:
         return_url = return_link.href_templated({"name": "Palace Manager"})
 
         # Hit the distributor's return link, and if it's successful, update the pool
-        # availability and delete the local loan.
+        # availability.
         doc = self._request_loan_status("PUT", return_url)
         if doc.active:
             # If the distributor says the loan is still active, we didn't return it, and
@@ -258,8 +259,8 @@ def _checkin(self, loan: Loan) -> bool:
                 f"Loan {loan.id} was not returned. The distributor says it's still active. {doc.model_dump_json()}"
             )
             raise CannotReturn()
-        self.update_loan(loan, doc)
-        return True
+        loan.license.checkin()
+        loan.license_pool.update_availability_from_licenses()
 
     def checkout(
         self,
@@ -343,9 +344,10 @@ def _checkout(
         encoded_pass = base64.b64encode(binascii.unhexlify(hashed_pass.hashed))
 
         notification_url = self._url_for(
-            "odl_notify",
+            "opds2_with_odl_notification",
             library_short_name=library_short_name,
-            loan_id=checkout_id,
+            patron_id=patron_id,
+            license_id=license.identifier,
             _external=True,
         )
 
@@ -396,7 +398,7 @@ def _checkout(
             # Remove the loan we created.
             raise CannotLoan()
 
-        # We save the link to the loan status document in the loan's status_url field, so
+        # We save the link to the loan status document in the loan's external_identifier field, so
         # we are able to retrieve it later.
         loan_status_document_link: BaseLink | None = doc.links.get(
             rel="self", type=LoanStatus.content_type()
@@ -429,18 +431,14 @@ def _checkout(
             licensepool,
             end_date=doc.potential_rights.end,
             external_identifier=loan_status_document_link.href,
+            license_identifier=license.identifier,
         )
 
         # We also need to update the remaining checkouts for the license.
         license.checkout()
 
-        # We have successfully borrowed this book.
-        if hold:
-            db.delete(hold)
-            # log circulation event:  hold converted to loan
-
         # Update the pool to reflect the new loan.
-        licensepool.update_availability_from_licenses()
+        licensepool.update_availability_from_licenses(holds_adjustment=1 if hold else 0)
         return loan
 
     def fulfill(
@@ -489,6 +487,7 @@ def _unlimited_access_fulfill(
     def _license_fulfill(
         self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism
     ) -> Fulfillment:
+        # We are unable to fulfill a loan that doesn't have its external identifier set,
         # We are unable to fulfill a loan that doesn't have its external identifier set,
         # since we use this to get to the checkout link. It shouldn't be possible to get
         # into this state.
@@ -502,7 +501,6 @@ def _license_fulfill(
             # the distributor revoked it or the patron already returned it
             # through the DRM system, and we didn't get a notification
             # from the distributor yet.
-            self.update_loan(loan, doc)
             db = Session.object_session(loan)
             db.delete(loan)
             raise CannotFulfill()
@@ -653,38 +651,9 @@ def release_hold(self, patron: Patron, pin: str, licensepool: LicensePool) -> No
         )
         if not hold:
             raise NotOnHold()
-        self._release_hold(hold)
-
-    def _release_hold(self, hold: Hold) -> Literal[True]:
-        # If the book was ready and the patron revoked the hold instead
-        # of checking it out, but no one else had the book on hold, the
-        # book is now available for anyone to check out. If someone else
-        # had a hold, the license is now reserved for the next patron.
-        _db = Session.object_session(hold)
-        licensepool = hold.license_pool
-        _db.delete(hold)
-
-        # log a circulation event : hold_released
-        # Update the pool and the next holds in the queue when a license is reserved.
-        licensepool.update_availability_from_licenses()
-        return True
-
-    def update_loan(self, loan: Loan, status_doc: LoanStatus) -> None:
-        """
-        Check a loan's status, and if it is no longer active, update its pool's availability.
-        """
-        _db = Session.object_session(loan)
 
-        if not status_doc.active:
-            # This loan is no longer active. Update the pool's availability
-            # and delete the loan.
-
-            # Update the license
-            loan.license.checkin()
-
-            # If there are holds, the license is reserved for the next patron.
-            # Update the pool and the next holds in the queue when a license is reserved.
-            loan.license_pool.update_availability_from_licenses()
+        # Update the license pool to reflect the released hold.
+        hold.license_pool.update_availability_from_licenses(holds_adjustment=1)
 
     def update_availability(self, licensepool: LicensePool) -> None:
         pass

src/palace/manager/api/routes.py

@@ -5,7 +5,11 @@
 from flask_cors.core import get_cors_options, set_cors_headers
 
 from palace.manager.api.app import app
-from palace.manager.core.app_server import compressible, returns_problem_detail
+from palace.manager.core.app_server import (
+    compressible,
+    raises_problem_detail,
+    returns_problem_detail,
+)
 from palace.manager.sqlalchemy.hassessioncache import HasSessionCache
 from palace.manager.util.problem_detail import ProblemDetail
 
@@ -568,12 +572,26 @@ def saml_callback():
     )
 
 
-# Loan notifications for ODL distributors, eg. Feedbooks
+# TODO: This is a deprecated route that will be removed in a future release of the code,
+#       its left here for now to provide a dummy endpoint for existing ODL loans. All
+#       new loans will use the new endpoint.
 @library_route("/odl_notify/<loan_id>", methods=["GET", "POST"])
 @has_library
-@returns_problem_detail
-def odl_notify(loan_id):
-    return app.manager.odl_notification_controller.notify(loan_id)
+@raises_problem_detail
+def odl_notify(loan_id) -> Response:
+    return app.manager.odl_notification_controller.notify_deprecated(loan_id)
+
+
+# Loan notifications for OPDS + ODL distributors
+@library_route("/odl/notify/<patron_identifier>/<license_identifier>", methods=["POST"])
+@has_library
+@raises_problem_detail
+def opds2_with_odl_notification(
+    patron_identifier: str, license_identifier: str
+) -> Response:
+    return app.manager.odl_notification_controller.notify(
+        patron_identifier, license_identifier
+    )
 
 
 # Controllers used for operations purposes

src/palace/manager/core/app_server.py

@@ -3,9 +3,10 @@
 from __future__ import annotations
 
 import gzip
+from collections.abc import Callable
 from functools import wraps
 from io import BytesIO
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, ParamSpec, TypeVar
 
 import flask
 from flask import Response, make_response, url_for
@@ -91,6 +92,21 @@ def decorated(*args, **kwargs):
     return decorated
 
 
+P = ParamSpec("P")
+T = TypeVar("T")
+
+
+def raises_problem_detail(f: Callable[P, T]) -> Callable[P, T | Response]:
+    @wraps(f)
+    def decorated(*args: P.args, **kwargs: P.kwargs) -> T | Response:
+        try:
+            return f(*args, **kwargs)
+        except BaseProblemDetailException as e:
+            return make_response(e.problem_detail.response)
+
+    return decorated
+
+
 def compressible(f):
     """Decorate a function to make it transparently handle whatever
     compression the client has announced it supports.