Count inventory report loans from Loan rows (PP-4552)

src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

@@ -548,18 +548,41 @@ def _licenses_lateral() -> Lateral:
     )
 
 
-def _library_loans_lateral() -> Lateral:
-    """How many loans are active for this item in this library?"""
+def _active_loans_lateral(*, this_library_only: bool) -> Lateral:
+    """How many loans are active for this item?
+
+    Counts ``Loan`` rows rather than deriving a number from the pool's availability
+    counters, because those counters cannot express a loan count for every pool type.
+    An ``AGGREGATED`` (ODL) pool recomputes ``licenses_owned`` from its licenses as
+    ``min(checkouts_left, terms_concurrency)``, and a checkout decrements
+    ``checkouts_left``, so a loan shrinks the owned and available counts together and
+    cancels itself out. An ``UNLIMITED`` pool holds all four counters at zero by
+    design. In both cases the loans themselves are recorded, so counting them is both
+    exact and uniform across pool types.
+
+    A loan is active until it expires. Expired loans are deleted by
+    ``celery.tasks.reaper.loan_reaper``, but only for ``METERED`` and ``AGGREGATED``
+    pools (it filters on ``LicensePool.metered_or_equivalent_type``), and only when
+    that task next runs. So expired rows linger between runs, and on ``UNLIMITED``
+    and open-access pools they are never reaped at all; either way they have to be
+    excluded here.
+
+    :param this_library_only: Restrict the count to loans held by patrons of the
+        library the report is being generated for. When False, every library sharing
+        the collection is counted.
+    """
     loan_alias = aliased(Loan)
     patron_alias = aliased(Patron)
-    return lateral(
-        select(func.count(loan_alias.id).label("active_loan_count"))
-        .join(patron_alias, loan_alias.patron_id == patron_alias.id)
-        .where(
-            loan_alias.license_pool_id == LicensePool.id,
-            patron_alias.library_id == Library.id,
-        )
+    unexpired = loan_alias.end.is_(None) | (loan_alias.end > func.now())
+    query = select(func.count(loan_alias.id).label("active_loan_count")).where(
+        loan_alias.license_pool_id == LicensePool.id,
+        unexpired,
     )
+    if this_library_only:
+        query = query.join(patron_alias, loan_alias.patron_id == patron_alias.id).where(
+            patron_alias.library_id == Library.id
+        )
+    return lateral(query)
 
 
 def _library_hold_ratio(lib_holds: Lateral) -> ColumnElement[Any]:
@@ -724,7 +747,8 @@ def palace_inventory_activity_report_query() -> Select:
     wg_subquery = _comma_separated_sorted_work_genre_list_subquery()
     collection_sharing = _is_shared_collection_lateral()
     lib_holds = _library_holds_lateral()
-    lib_loans = _library_loans_lateral()
+    lib_loans = _active_loans_lateral(this_library_only=True)
+    shared_loans = _active_loans_lateral(this_library_only=False)
 
     return (
         select(
@@ -752,7 +776,7 @@ def palace_inventory_activity_report_query() -> Select:
             case(
                 (
                     collection_sharing.c.is_shared_collection,
-                    LicensePool.licenses_reserved,
+                    func.coalesce(shared_loans.c.active_loan_count, 0),
                 ),
                 else_=-1,
             ).label("shared_active_loan_count"),
@@ -787,6 +811,7 @@ def palace_inventory_activity_report_query() -> Select:
         .outerjoin(wg_subquery, Work.id == wg_subquery.c.work_id)
         .outerjoin(lib_holds, true())
         .outerjoin(lib_loans, true())
+        .outerjoin(shared_loans, true())
         .join(collection_sharing, true())
         .where(
             Library.id == bindparam("library_id"),

tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py

@@ -31,9 +31,13 @@
 from palace.manager.integration.license.opds.opds1.settings import OPDSImporterSettings
 from palace.manager.integration.license.overdrive.api import OverdriveAPI
 from palace.manager.sqlalchemy.model.classification import Genre, Subject
+from palace.manager.sqlalchemy.model.collection import Collection
 from palace.manager.sqlalchemy.model.identifier import Identifier
 from palace.manager.sqlalchemy.model.library import Library
-from palace.manager.sqlalchemy.model.licensing import LicensePoolStatus
+from palace.manager.sqlalchemy.model.licensing import (
+    LicensePoolStatus,
+    LicensePoolType,
+)
 from palace.manager.sqlalchemy.model.patron import Hold
 from palace.manager.sqlalchemy.model.work import Work
 from palace.manager.sqlalchemy.util import (
@@ -986,7 +990,7 @@ def find_entry(contains: str, extension: str) -> str:
                 # Activity report specific fields for book with holds
                 assert int(row["total_library_allowed_concurrent_users"]) == 1
                 assert int(row["library_active_loan_count"]) == 0
-                # Collection is shared (library2 was added), licenses_reserved defaults to 0
+                # Collection is shared (library2 was added), and no patron has a loan
                 assert int(row["shared_active_loan_count"]) == 0
                 assert int(row["library_active_hold_count"]) == 3
                 assert (
@@ -1209,6 +1213,27 @@ def row_for(work):
     assert suppressed_row["visibility_status"] == "manually suppressed"
 
 
+def _activity_report_rows(
+    db: DatabaseTransactionFixture, library: Library, *collections: Collection
+) -> list[dict[str, str]]:
+    """Run the activity report for a library and return its rows."""
+    csv_file = io.StringIO()
+    csv_file.name = "test_activity_report.csv"
+    generate_csv_report(
+        db=db.session,
+        csv_file=csv_file,
+        sql_params={
+            "library_id": library.id,
+            "integration_ids": tuple(
+                c.integration_configuration.id for c in collections
+            ),
+        },
+        query=palace_inventory_activity_report_query(),
+    )
+    csv_file.seek(0)
+    return list(csv.DictReader(csv_file))
+
+
 def test_inventory_activity_report_hold_ratio(
     db: DatabaseTransactionFixture,
     services_fixture: ServicesFixture,
@@ -1249,19 +1274,7 @@ def work_with(licenses_owned: int, holds: int) -> Work:
     no_holds_work = work_with(licenses_owned=4, holds=0)
     no_copies_work = work_with(licenses_owned=0, holds=2)
 
-    csv_file = io.StringIO()
-    csv_file.name = "test_activity_report.csv"
-    generate_csv_report(
-        db=db.session,
-        csv_file=csv_file,
-        sql_params={
-            "library_id": library.id,
-            "integration_ids": (collection.integration_configuration.id,),
-        },
-        query=palace_inventory_activity_report_query(),
-    )
-    csv_file.seek(0)
-    rows = list(csv.DictReader(csv_file))
+    rows = _activity_report_rows(db, library, collection)
 
     def ratio_for(work: Work) -> float:
         identifier_value = work.presentation_edition.primary_identifier.identifier
@@ -1279,6 +1292,144 @@ def ratio_for(work: Work) -> float:
     assert ratio_for(no_copies_work) == -1
 
 
+def test_inventory_activity_report_loan_counts(
+    db: DatabaseTransactionFixture,
+    services_fixture: ServicesFixture,
+):
+    """Loan counts come from Loan rows, and are scoped and filtered consistently."""
+    library = db.library(short_name="test_library")
+    other_library = db.library(short_name="other_library")
+    collection = create_test_opds_collection(
+        "Shared Collection", "SharedSource", db, library
+    )
+    # A second library on the collection is what makes it shared, which is what
+    # enables the shared_* columns.
+    collection.associated_libraries = [library, other_library]
+    ds = collection.data_source
+    assert ds is not None
+
+    work = db.work(
+        data_source_name=ds.name, collection=collection, with_license_pool=True
+    )
+    pool = work.license_pools[0]
+    start = utc_now() - timedelta(days=7)
+    unexpired = utc_now() + timedelta(days=1)
+
+    # This library: 2 active loans, plus an expired one that must not be counted.
+    pool.loan_to(db.patron(library=library), start=start, end=unexpired)
+    pool.loan_to(db.patron(library=library), start=start, end=None)
+    pool.loan_to(
+        db.patron(library=library), start=start, end=utc_now() - timedelta(minutes=1)
+    )
+    # The other library: 3 active loans, plus an expired one.
+    for _ in range(3):
+        pool.loan_to(db.patron(library=other_library), start=start, end=unexpired)
+    pool.loan_to(
+        db.patron(library=other_library),
+        start=start,
+        end=utc_now() - timedelta(minutes=1),
+    )
+
+    rows = _activity_report_rows(db, library, collection)
+    assert len(rows) == 1
+
+    assert int(rows[0]["library_active_loan_count"]) == 2
+    # The library's own loans are a subset of the shared count, never an addend.
+    assert int(rows[0]["shared_active_loan_count"]) == 5
+
+
+@pytest.mark.parametrize(
+    "pool_type",
+    [
+        pytest.param(LicensePoolType.AGGREGATED, id="odl"),
+        pytest.param(LicensePoolType.UNLIMITED, id="unlimited"),
+        pytest.param(LicensePoolType.METERED, id="metered"),
+    ],
+)
+def test_inventory_activity_report_loan_counts_by_pool_type(
+    db: DatabaseTransactionFixture,
+    services_fixture: ServicesFixture,
+    pool_type: LicensePoolType,
+):
+    """Loan counts are exact for every pool type.
+
+    The availability counters cannot express a loan count for all of these. An ODL
+    pool cancels a loan out of `licenses_owned` as `checkouts_left` falls, and an
+    unlimited pool holds every counter at zero. Counting Loan rows sidesteps both.
+    """
+    library = db.library(short_name="test_library")
+    other_library = db.library(short_name="other_library")
+    collection = create_test_opds_collection("Collection", "Source", db, library)
+    collection.associated_libraries = [library, other_library]
+    ds = collection.data_source
+    assert ds is not None
+
+    work = db.work(
+        data_source_name=ds.name, collection=collection, with_license_pool=True
+    )
+    pool = work.license_pools[0]
+    pool.type = pool_type
+
+    if pool_type == LicensePoolType.AGGREGATED:
+        # An ODL license with exactly one checkout left. Borrowing it drives
+        # licenses_owned, licenses_available and licenses_reserved all to zero.
+        license = db.license(
+            pool=pool,
+            status=LicenseStatus.available,
+            checkouts_left=1,
+            checkouts_available=1,
+            terms_concurrency=1,
+        )
+        license.checkout()
+        license.loan_to(db.patron(library=library), end=utc_now() + timedelta(days=1))
+        pool.update_availability_from_licenses()
+        assert pool.licenses_owned == 0
+    elif pool_type == LicensePoolType.UNLIMITED:
+        # Unlimited pools are imported with every counter pinned to zero.
+        pool.licenses_owned = 0
+        pool.licenses_available = 0
+        pool.licenses_reserved = 0
+        pool.patrons_in_hold_queue = 0
+        pool.loan_to(db.patron(library=library), end=utc_now() + timedelta(days=1))
+    else:
+        pool.licenses_owned = 5
+        pool.licenses_available = 4
+        pool.licenses_reserved = 0
+        pool.loan_to(db.patron(library=library), end=utc_now() + timedelta(days=1))
+
+    rows = _activity_report_rows(db, library, collection)
+    assert len(rows) == 1
+    # One patron holds a loan, whatever the counters say about it.
+    assert int(rows[0]["library_active_loan_count"]) == 1
+    assert int(rows[0]["shared_active_loan_count"]) == 1
+
+
+def test_inventory_activity_report_shared_loan_count_unshared_collection(
+    db: DatabaseTransactionFixture,
+    services_fixture: ServicesFixture,
+):
+    """A collection only this library uses reports the -1 sentinel, not a count."""
+    library = db.library(short_name="test_library")
+    collection = create_test_opds_collection(
+        "Unshared Collection", "UnsharedSource", db, library
+    )
+    ds = collection.data_source
+    assert ds is not None
+
+    work = db.work(
+        data_source_name=ds.name, collection=collection, with_license_pool=True
+    )
+    work.license_pools[0].loan_to(
+        db.patron(library=library), end=utc_now() + timedelta(days=1)
+    )
+
+    rows = _activity_report_rows(db, library, collection)
+    assert len(rows) == 1
+    assert int(rows[0]["library_active_loan_count"]) == 1
+    assert int(rows[0]["shared_active_loan_count"]) == -1
+    assert int(rows[0]["shared_active_hold_count"]) == -1
+
+
 @pytest.mark.parametrize(
     "status,licenses_owned,licenses_available,license_exception",
     [