From 9b78d9c9f1db978142407283227d56fe9b4c7f18 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 24 Sep 2026 09:23:36 -0700 Subject: [PATCH 1/3] Count inventory report loans from Loan rows (PP-4552) shared_active_loan_count reported LicensePool.licenses_reserved, which is the number of copies held in reserve for patrons at the head of the hold queue, not a loan count. That mapping came from the original raw-SQL report and was carried over verbatim in the SQLAlchemy rewrite. Deriving the count from the availability counters instead does not work either, 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. A loan therefore shrinks the owned and available counts together and cancels itself out, so a license on its last checkout reports zero loans while a patron holds one. - An UNLIMITED pool holds all four counters at zero by design, so any count derived from them is always zero. Both pool types do record their loans, so count Loan rows instead. That is exact for every pool type and comes from the same source as library_active_loan_count, which guarantees the library count can never exceed the shared count. Fold both loan counts into one lateral helper so they share a definition of "active", and exclude expired loans from both. Expired loans are deleted by celery.tasks.reaper.loan_reaper, but only for metered pools and only when that task next runs, so the count otherwise drifts above what the Collection Manager shows -- and never settles for unlimited and open-access pools. Co-Authored-By: Claude Opus 5 --- .../generate_inventory_and_hold_reports.py | 44 +++-- ...est_generate_inventory_and_hold_reports.py | 165 +++++++++++++++++- 2 files changed, 197 insertions(+), 12 deletions(-) diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index 04ba8308d2..568de707f6 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -548,18 +548,38 @@ 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 pools and only when that + task next runs, so they have to be excluded here as well. + + :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 +744,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 +773,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 +808,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"), diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index b8e584abf2..0f7bb42fd6 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/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 ( @@ -1279,6 +1283,165 @@ def ratio_for(work: Work) -> float: assert ratio_for(no_copies_work) == -1 +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_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", [ From e8dd3d53d7a45b57bed63483c94e4c09d11eb008 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Fri, 25 Sep 2026 12:14:22 -0700 Subject: [PATCH 2/3] Address review nits in inventory report tests (PP-4552) Two test-file cleanups from review: - A comment still described shared_active_loan_count as coming from licenses_reserved, the mapping this PR removes. The assertion still holds, but now because no patron has a loan on that book. - test_inventory_activity_report_hold_ratio duplicated the CSV setup that _activity_report_rows already does. Use the helper, and move it above its first consumer. Co-Authored-By: Claude Opus 5 --- ...est_generate_inventory_and_hold_reports.py | 58 ++++++++----------- 1 file changed, 23 insertions(+), 35 deletions(-) diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index 0f7bb42fd6..25b717279c 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -990,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 ( @@ -1213,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, @@ -1253,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 @@ -1283,27 +1292,6 @@ def ratio_for(work: Work) -> float: assert ratio_for(no_copies_work) == -1 -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_loan_counts( db: DatabaseTransactionFixture, services_fixture: ServicesFixture, From 1105e5e9025dafe5b05a43cf00493236d817550e Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 28 Sep 2026 16:33:20 -0700 Subject: [PATCH 3/3] Correct reaper scope in active-loans docstring (PP-4552) Review feedback: loan_reaper filters on LicensePool.metered_or_equivalent_type, which covers AGGREGATED as well as METERED pools, so "only for metered pools" was wrong. Name the predicate and say which pools are genuinely never reaped. Co-Authored-By: Claude Opus 5 --- .../celery/tasks/generate_inventory_and_hold_reports.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index 568de707f6..fe4966f0dd 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -561,8 +561,11 @@ def _active_loans_lateral(*, this_library_only: bool) -> Lateral: 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 pools and only when that - task next runs, so they have to be excluded here as well. + ``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