Update ODL hold calculation logic (PP-1728) (#2117)

Completely re-work how we are calculating the hold queue for OPDS2 + ODL items.

All the work to calculate the holds is moved out to live in a Celery task. When a hold is invoked through the API the hold is always placed in the queue, and the queue is later recalculated by the celery task.

docker/services/cron/cron.d/circulation

@@ -78,7 +78,6 @@ HOME=/var/www/circulation
 # OPDS 2.x + ODL import
 #
 45 * * * * root bin/run opds2_odl_import_monitor >> /var/log/cron.log 2>&1
-0 */8 * * * root bin/run opds2_odl_hold_reaper >> /var/log/cron.log 2>&1
 
 # SAML
 #

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/circulation_manager.py

@@ -359,7 +359,6 @@ def setup_one_time_controllers(self):
         self.version = ApplicationVersionController()
         self.odl_notification_controller = ODLNotificationController(
             self._db,
-            self,
             self.services.integration_registry.license_providers(),
         )
         self.patron_auth_token = PatronAuthTokenController(self)

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

@@ -1,11 +1,10 @@
 from __future__ import annotations
 
-from typing import TYPE_CHECKING
-
 import flask
 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
@@ -18,14 +17,13 @@
 from palace.manager.service.integration_registry.license_providers import (
     LicenseProvidersRegistry,
 )
-from palace.manager.sqlalchemy.model.library import Library
-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
-
-if TYPE_CHECKING:
-    from palace.manager.api.circulation_manager import CirculationManager
+from palace.manager.util.problem_detail import ProblemDetailException
 
 
 class ODLNotificationController(LoggerMixin):
@@ -36,35 +34,58 @@ class ODLNotificationController(LoggerMixin):
     def __init__(
         self,
         db: Session,
-        manager: CirculationManager,
         registry: LicenseProvidersRegistry,
     ) -> None:
         self.db = db
-        self.manager = manager
         self.registry = registry
 
-    def get_api(self, library: Library, loan: Loan) -> OPDS2WithODLApi:
-        return self.manager.circulation_apis[library.id].api_for_license_pool(  # type: ignore[no-any-return]
-            loan.license_pool
-        )
+    def _get_loan(
+        self, patron_identifier: str | None, license_identifier: str | None
+    ) -> Loan | None:
+        if patron_identifier is None or license_identifier is None:
+            return 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, loan_id: int) -> Response | ProblemDetail:
-        library = flask.request.library  # type: ignore[attr-defined]
-        status_doc_json = flask.request.data
+    def notify(
+        self, patron_identifier: str | None, license_identifier: str | None
+    ) -> 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:
@@ -73,9 +94,13 @@ 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)
 
-            api = self.get_api(library, loan)
-            api.update_loan(loan, status_doc)
+            # 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.
+            if not status_doc.active:
+                loan.end = utc_now()
 
         return Response(status=204)