Updated to be a little easier to reason about

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, Literal
 
 from dependency_injector.wiring import Provide, inject
 from flask import url_for
@@ -54,6 +53,7 @@
 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
@@ -104,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)
 
@@ -348,70 +348,38 @@ def _checkout(
         encoded_pass = base64.b64encode(binascii.unhexlify(hashed_pass.hashed))
 
         licenses = licensepool.best_available_licenses()
-        license_: License | None = None
-        loan_status: LoanStatus | Literal[False] = False
-        while licenses and not loan_status:
-            license_ = licenses.pop()
-            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=requested_expiry.isoformat(),
-                notification_url=notification_url,
-                passphrase=encoded_pass,
-                hint=self.settings.passphrase_hint,
-                hint_url=self.settings.passphrase_hint_url,
-            )
 
+        license_: License | None = None
+        loan_status: LoanStatus | None = None
+        for license_ in licenses:
             try:
-                loan_status = self._request_loan_status(
-                    "POST",
-                    checkout_url,
-                    ignored_problem_types=[
-                        "http://opds-spec.org/odl/error/checkout/unavailable"
-                    ],
+                loan_status = self._checkout_license(
+                    license_,
+                    library_short_name,
+                    patron_id,
+                    requested_expiry.isoformat(),
+                    encoded_pass,
                 )
-            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 licenses:
-                        # There are more licenses we can try to loan from, so we continue the loop.
-                        continue
-                    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
-
-        if license_ is None or loan_status is False:
+                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 loan_status.active:
-            # Something went wrong with this loan and we don't actually
+            # 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
@@ -461,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

@@ -1101,20 +1101,20 @@ def on_hold_to(
         return hold, new
 
     class _LicensePriority(IntEnum):
-        LOAN_LIMITED = auto()
-        TIME_AND_LOAN_LIMITED = auto()
-        PERPETUAL = auto()
         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((utc_now() - license_.expires).total_seconds())
+        return int(license_.expires.timestamp())
 
     @staticmethod
     def _loan_limited_sort_key(license_: License) -> int:
-        return license_.checkouts_left or 0
+        return (license_.checkouts_left or 0) * -1
 
     @classmethod
     def _license_sort_func(cls, license_: License) -> tuple[_LicensePriority, int, int]:
@@ -1137,28 +1137,26 @@ def _license_sort_func(cls, license_: License) -> tuple[_LicensePriority, int, i
         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 for this pool.
+        """
+        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 reverse lending priority. The highest priority license is the last one in the
-        list, so that it can be popped off the end of the list.
+        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
+        which case it's better to loan the time-limited license so the perpetual one remains
         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
+        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. 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.
-
-        The worst option would be pay-per-use, but we don't yet support any distributors that
-        offer that model.
         """
         return sorted(
             (l for l in self.licenses if l.is_available_for_borrowing),

tests/manager/api/odl/test_api.py

@@ -764,8 +764,8 @@ def test_checkout_many_licenses(
         license_untouched = db.license(pool, checkouts_left=1, checkouts_available=1)
         license_lent = db.license(
             pool,
-            checkouts_left=1,
-            checkouts_available=1,
+            checkouts_left=4,
+            checkouts_available=4,
             expires=utc_now() + datetime.timedelta(weeks=1),
         )
         license_expired = db.license(
@@ -775,12 +775,13 @@ def test_checkout_many_licenses(
             expires=utc_now() - datetime.timedelta(weeks=1),
         )
         pool.update_availability_from_licenses()
+        assert pool.licenses_available == 8
 
         assert opds2_with_odl_api_fixture.pool.best_available_licenses() == [
-            license_untouched,
-            license_lent,
-            license_unavailable_2,
             license_unavailable_1,
+            license_unavailable_2,
+            license_lent,
+            license_untouched,
         ]
 
         # But the distributor says there are no available copies for license_unavailable_1
@@ -803,10 +804,10 @@ def test_checkout_many_licenses(
 
         loan_info = opds2_with_odl_api_fixture.api_checkout()
 
-        assert opds2_with_odl_api_fixture.pool.licenses_available == 1
+        assert opds2_with_odl_api_fixture.pool.licenses_available == 4
         assert license_unavailable_2.checkouts_available == 0
         assert license_unavailable_1.checkouts_available == 0
-        assert license_lent.checkouts_available == 0
+        assert license_lent.checkouts_available == 3
         assert license_untouched.checkouts_available == 1
 
         assert loan_info.license_identifier == license_lent.identifier

tests/manager/sqlalchemy/model/test_licensing.py

@@ -27,6 +27,7 @@
 )
 from palace.manager.sqlalchemy.model.resource import Hyperlink, Representation
 from palace.manager.sqlalchemy.util import create
+from palace.manager.util import first_or_default
 from palace.manager.util.datetime_helpers import utc_now
 from tests.fixtures.database import DatabaseTransactionFixture
 from tests.mocks.analytics_provider import MockAnalyticsProvider
@@ -481,44 +482,60 @@ def test_best_available_licenses(self, licenses: LicenseTestFixture):
 
         # First, make sure the overall order is correct
         assert licenses.pool.best_available_licenses() == [
-            loan_limited_2,
-            licenses.loan_limited,
-            licenses.time_and_loan_limited,
-            licenses.perpetual,
-            licenses.time_limited,
             time_limited_2,
+            licenses.time_limited,
+            licenses.perpetual,
+            licenses.time_and_loan_limited,
+            licenses.loan_limited,
+            loan_limited_2,
         ]
 
         # We use the time-limited license that's expiring first.
-        assert licenses.pool.best_available_licenses().pop() == time_limited_2
+        assert (
+            first_or_default(licenses.pool.best_available_licenses()) == time_limited_2
+        )
         time_limited_2.checkout()
 
         # When that's not available, we use the next time-limited license.
-        assert licenses.pool.best_available_licenses().pop() == licenses.time_limited
+        assert (
+            first_or_default(licenses.pool.best_available_licenses())
+            == licenses.time_limited
+        )
         licenses.time_limited.checkout()
 
         # Next is the perpetual license.
-        assert licenses.pool.best_available_licenses().pop() == licenses.perpetual
+        assert (
+            first_or_default(licenses.pool.best_available_licenses())
+            == licenses.perpetual
+        )
         licenses.perpetual.checkout()
 
         # The time-and-loan-limited license also counts as time-limited for this.
         assert (
-            licenses.pool.best_available_licenses().pop()
+            first_or_default(licenses.pool.best_available_licenses())
             == licenses.time_and_loan_limited
         )
         licenses.time_and_loan_limited.checkout()
 
         # Then the loan-limited license with the most remaining checkouts.
-        assert licenses.pool.best_available_licenses().pop() == licenses.loan_limited
+        assert (
+            first_or_default(licenses.pool.best_available_licenses())
+            == licenses.loan_limited
+        )
         licenses.loan_limited.checkout()
 
         # That license allows 2 concurrent checkouts, so it's still the
         # best license until it's checked out again.
-        assert licenses.pool.best_available_licenses().pop() == licenses.loan_limited
+        assert (
+            first_or_default(licenses.pool.best_available_licenses())
+            == licenses.loan_limited
+        )
         licenses.loan_limited.checkout()
 
         # There's one more loan-limited license.
-        assert licenses.pool.best_available_licenses().pop() == loan_limited_2
+        assert (
+            first_or_default(licenses.pool.best_available_licenses()) == loan_limited_2
+        )
         loan_limited_2.checkout()
 
         # Now all licenses are either loaned out or expired.