"Could not place hold" error with multiple licenses (PP-1902) (#2173)

When we have multiple licenses, it was possible for us to get out of sync with the provider, so we would try to place a hold, when a different license could have been tried instead. This updates the checkout to try all available licenses, in priority order, before resorting to a hold.

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

@@ -6,7 +6,6 @@
 import uuid
 from collections.abc import Callable
 from functools import cached_property, partial
-from typing import Any
 
 from dependency_injector.wiring import Provide, inject
 from flask import url_for
@@ -54,11 +53,13 @@
 from palace.manager.opds.base import BaseLink
 from palace.manager.opds.lcp.license import LicenseDocument
 from palace.manager.opds.lcp.status import LoanStatus
+from palace.manager.service.analytics.analytics import Analytics
 from palace.manager.service.container import Services
 from palace.manager.sqlalchemy.model.collection import Collection
 from palace.manager.sqlalchemy.model.datasource import DataSource
 from palace.manager.sqlalchemy.model.licensing import (
     DeliveryMechanism,
+    License,
     LicensePool,
     LicensePoolDeliveryMechanism,
 )
@@ -103,7 +104,7 @@ def __init__(
         self,
         _db: Session,
         collection: Collection,
-        analytics: Any = Provide[Services.analytics.analytics],
+        analytics: Analytics = Provide[Services.analytics.analytics],
     ) -> None:
         super().__init__(_db, collection)
 
@@ -334,85 +335,56 @@ def _checkout(
         ) and licensepool.licenses_available < 1:
             raise NoAvailableCopies()
 
-        # Create a local loan so its database id can be used to
-        # receive notifications from the distributor.
-        license = licensepool.best_available_license()
-        if not license:
-            raise NoAvailableCopies()
-
-        identifier = str(license.identifier)
-        checkout_id = str(uuid.uuid4())
         if self.collection is None:
             raise PalaceValueError(f"Collection not found: {self.collection_id}")
         default_loan_period = self.collection.default_loan_period(patron.library)
-
         requested_expiry = utc_now() + datetime.timedelta(days=default_loan_period)
         patron_id = patron.identifier_to_remote_service(licensepool.data_source)
         library_short_name = patron.library.short_name
-
         hasher = self._get_hasher()
         unhashed_pass = self._credential_factory.get_patron_passphrase(db, patron)
         hashed_pass = unhashed_pass.hash(hasher)
         self._credential_factory.set_hashed_passphrase(db, patron, hashed_pass)
         encoded_pass = base64.b64encode(binascii.unhexlify(hashed_pass.hashed))
 
-        notification_url = self._notification_url(
-            library_short_name,
-            patron_id,
-            identifier,
-        )
-
-        # We should never be able to get here if the license doesn't have a checkout_url, but
-        # we assert it anyway, to be sure we fail fast if it happens.
-        assert license.checkout_url is not None
-        url_template = URITemplate(license.checkout_url)
-        checkout_url = url_template.expand(
-            id=identifier,
-            checkout_id=checkout_id,
-            patron_id=patron_id,
-            expires=requested_expiry.isoformat(),
-            notification_url=notification_url,
-            passphrase=encoded_pass,
-            hint=self.settings.passphrase_hint,
-            hint_url=self.settings.passphrase_hint_url,
-        )
-
-        try:
-            doc = self._request_loan_status(
-                "POST",
-                checkout_url,
-                ignored_problem_types=[
-                    "http://opds-spec.org/odl/error/checkout/unavailable"
-                ],
-            )
-        except OpdsWithOdlException as e:
-            if e.type == "http://opds-spec.org/odl/error/checkout/unavailable":
-                # TODO: This would be a good place to do an async availability update, since we know
-                #   the book is unavailable, when we thought it was available. For now, we know that
-                #   the license has no checkouts_available, so we do that update.
-                license.checkouts_available = 0
-                if hold:
-                    # If we have a hold, it means we thought the book was available, but it wasn't.
-                    # So we need to update its position in the hold queue. We will put it at position
-                    # 1, since the patron should be first in line. This may mean that there are two
-                    # patrons in position 1 in the hold queue, but this will be resolved next time
-                    # the hold queue is recalculated.
-                    hold.position = 1
-                    hold.end = None
-                # Update the pool and the next holds in the queue when a license is reserved.
-                licensepool.update_availability_from_licenses()
-                raise NoAvailableCopies()
-            raise
+        licenses = licensepool.best_available_licenses()
+
+        license_: License | None = None
+        loan_status: LoanStatus | None = None
+        for license_ in licenses:
+            try:
+                loan_status = self._checkout_license(
+                    license_,
+                    library_short_name,
+                    patron_id,
+                    requested_expiry.isoformat(),
+                    encoded_pass,
+                )
+                break
+            except NoAvailableCopies:
+                # This license had no available copies, so we try the next one.
+                ...
+
+        if license_ is None or loan_status is None:
+            if hold:
+                # If we have a hold, it means we thought the book was available, but it wasn't.
+                # So we need to update its position in the hold queue. We will put it at position
+                # 1, since the patron should be first in line. This may mean that there are two
+                # patrons in position 1 in the hold queue, but this will be resolved next time
+                # the hold queue is recalculated.
+                hold.position = 1
+                hold.end = None
+            licensepool.update_availability_from_licenses()
+            raise NoAvailableCopies()
 
-        if not doc.active:
-            # Something went wrong with this loan and we don't actually
+        if not loan_status.active:
+            # Something went wrong with this loan, and we don't actually
             # have the book checked out. This should never happen.
-            # Remove the loan we created.
             raise CannotLoan()
 
         # 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(
+        loan_status_document_link: BaseLink | None = loan_status.links.get(
             rel="self", type=LoanStatus.content_type()
         )
 
@@ -424,7 +396,7 @@ def _checkout(
         # 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.
         if not loan_status_document_link:
-            license_document_link = doc.links.get(
+            license_document_link = loan_status.links.get(
                 rel="license", type=LicenseDocument.content_type()
             )
             if license_document_link:
@@ -441,13 +413,13 @@ def _checkout(
 
         loan = LoanInfo.from_license_pool(
             licensepool,
-            end_date=doc.potential_rights.end,
+            end_date=loan_status.potential_rights.end,
             external_identifier=loan_status_document_link.href,
-            license_identifier=license.identifier,
+            license_identifier=license_.identifier,
         )
 
         # We also need to update the remaining checkouts for the license.
-        license.checkout()
+        license_.checkout()
 
         # If there was a hold CirculationAPI will take care of deleting it. So we just need to
         # update the license pool to reflect the loan. Since update_availability_from_licenses
@@ -457,6 +429,55 @@ def _checkout(
         )
         return loan
 
+    def _checkout_license(
+        self,
+        license_: License,
+        library_short_name: str | None,
+        patron_id: str,
+        expiry: str,
+        encoded_pass: str,
+    ) -> LoanStatus:
+        identifier = str(license_.identifier)
+        checkout_id = str(uuid.uuid4())
+
+        notification_url = self._notification_url(
+            library_short_name,
+            patron_id,
+            identifier,
+        )
+
+        # We should never be able to get here if the license doesn't have a checkout_url, but
+        # we assert it anyway, to be sure we fail fast if it happens.
+        assert license_.checkout_url is not None
+        url_template = URITemplate(license_.checkout_url)
+        checkout_url = url_template.expand(
+            id=identifier,
+            checkout_id=checkout_id,
+            patron_id=patron_id,
+            expires=expiry,
+            notification_url=notification_url,
+            passphrase=encoded_pass,
+            hint=self.settings.passphrase_hint,
+            hint_url=self.settings.passphrase_hint_url,
+        )
+
+        try:
+            return self._request_loan_status(
+                "POST",
+                checkout_url,
+                ignored_problem_types=[
+                    "http://opds-spec.org/odl/error/checkout/unavailable"
+                ],
+            )
+        except OpdsWithOdlException as e:
+            if e.type == "http://opds-spec.org/odl/error/checkout/unavailable":
+                # TODO: This would be a good place to do an async availability update, since we know
+                #   the book is unavailable, when we thought it was available. For now, we know that
+                #   the license has no checkouts_available, so we do that update.
+                license_.checkouts_available = 0
+                raise NoAvailableCopies() from e
+            raise
+
     def fulfill(
         self,
         patron: Patron,

src/palace/manager/sqlalchemy/model/licensing.py

@@ -5,6 +5,7 @@
 import datetime
 import logging
 from enum import Enum as PythonEnum
+from enum import IntEnum, auto
 from operator import and_
 from typing import TYPE_CHECKING, Literal, overload
 
@@ -1099,44 +1100,68 @@ def on_hold_to(
         hold.update(start, end, position)
         return hold, new
 
-    def best_available_license(self) -> License | None:
-        """Determine the next license that should be lent out for this pool.
+    class _LicensePriority(IntEnum):
+        TIME_LIMITED = auto()
+        PERPETUAL = auto()
+        TIME_AND_LOAN_LIMITED = auto()
+        LOAN_LIMITED = auto()
+
+    @staticmethod
+    def _time_limited_sort_key(license_: License) -> int:
+        if license_.expires is None:
+            return 0
+        return int(license_.expires.timestamp())
+
+    @staticmethod
+    def _loan_limited_sort_key(license_: License) -> int:
+        return (license_.checkouts_left or 0) * -1
+
+    @classmethod
+    def _license_sort_func(cls, license_: License) -> tuple[_LicensePriority, int, int]:
+        time_limited_key = cls._time_limited_sort_key(license_)
+        loan_limited_key = cls._loan_limited_sort_key(license_)
+
+        if license_.is_time_limited and license_.is_loan_limited:
+            return (
+                cls._LicensePriority.TIME_AND_LOAN_LIMITED,
+                time_limited_key,
+                loan_limited_key,
+            )
+
+        if license_.is_time_limited:
+            return cls._LicensePriority.TIME_LIMITED, time_limited_key, loan_limited_key
+
+        if license_.is_loan_limited:
+            return cls._LicensePriority.LOAN_LIMITED, time_limited_key, loan_limited_key
+
+        return cls._LicensePriority.PERPETUAL, time_limited_key, loan_limited_key
+
+    def best_available_licenses(self) -> list[License]:
+        """
+        Determine the next license that should be lent out from this pool.
+
+        This function returns a list of licenses that are available for lending, sorted
+        by priority. The highest priority license (the one that the next loan should be made from)
+        is the first one in the list.
 
         Time-limited licenses and perpetual licenses are the best. It doesn't matter which
         is used first, unless a time-limited license would expire within the loan period, in
-        which case it's better to loan the time-limited license so the perpetual one is still
-        available. We can handle this by always loaning the time-limited one first, followed
-        by perpetual. If there is more than one time-limited license, it's better to use the one
+        which case it's better to loan the time-limited license so the perpetual one remains
+        available.
+
+        We handle this by always loaning the time-limited one first, followed by the perpetual
+        one. If there is more than one time-limited license, it's better to use the one
         expiring soonest.
 
         If no time-limited or perpetual licenses are available, the next best is a loan-limited
-        license. We should choose the license with the most remaining loans, so that we'll
-        maximize the number of concurrent checkouts available in the future.
-
-        The worst option would be pay-per-use, but we don't yet support any distributors that
-        offer that model.
+        license. If a license is both time-limited and loan-limited, it's better to use it before
+        a license that is only loan-limited. We should choose the license with the most remaining
+        loans, so that we'll maximize the number of concurrent checkouts available in the future.
         """
-        best: License | None = None
-
-        for license in (l for l in self.licenses if l.is_available_for_borrowing):
-            if (
-                not best
-                or (license.is_time_limited and not best.is_time_limited)
-                or (
-                    license.is_time_limited
-                    and best.is_time_limited
-                    and license.expires < best.expires  # type: ignore[operator]
-                )
-                or (license.is_perpetual and not best.is_time_limited)
-                or (
-                    license.is_loan_limited
-                    and best.is_loan_limited
-                    and license.checkouts_left > best.checkouts_left  # type: ignore[operator]
-                )
-            ):
-                best = license
-
-        return best
+        return sorted(
+            (l for l in self.licenses if l.is_available_for_borrowing),
+            key=self._license_sort_func,
+        )
 
     @classmethod
     def consolidate_works(cls, _db, batch_size=10):