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..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 @@ -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"), 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..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 @@ -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", [