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 2942d2c0f3..04ba8308d2 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 @@ -17,8 +17,10 @@ from openpyxl.cell.cell import ILLEGAL_CHARACTERS_RE from openpyxl.styles.numbers import FORMAT_TEXT from sqlalchemy import ( + Numeric, bindparam, case, + cast, exists, false, func, @@ -560,6 +562,28 @@ def _library_loans_lateral() -> Lateral: ) +def _library_hold_ratio(lib_holds: Lateral) -> ColumnElement[Any]: + """How many holds does this library have per owned copy? + + Returns -1 when the item has no owned copies, since the ratio is undefined. + + The numerator is cast to ``Numeric`` because both operands are otherwise integral, + and PostgreSQL's integer division discards the fractional part of the result at any + magnitude: 7 holds against 2 copies would report 3 rather than 3.5, and any ratio + below 1.0 would report 0. + """ + active_hold_count = func.coalesce(lib_holds.c.active_hold_count, 0) + return case( + ( + LicensePool.licenses_owned > 0, + func.round( + cast(active_hold_count, Numeric) / LicensePool.licenses_owned, 2 + ), + ), + else_=-1, + ) + + def inventory_report_query() -> Select: """A query for inventory report with license information. @@ -742,14 +766,7 @@ def palace_inventory_activity_report_query() -> Select: ), else_=-1, ).label("shared_active_hold_count"), - case( - ( - LicensePool.licenses_owned > 0, - func.coalesce(lib_holds.c.active_hold_count, 0) - / LicensePool.licenses_owned, - ), - else_=-1, - ).label("library_hold_ratio"), + _library_hold_ratio(lib_holds).label("library_hold_ratio"), ) .select_from(LicensePool) .join(Identifier, LicensePool.identifier_id == Identifier.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 f3dc0a4d23..b8e584abf2 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 @@ -35,6 +35,7 @@ from palace.manager.sqlalchemy.model.library import Library from palace.manager.sqlalchemy.model.licensing import LicensePoolStatus from palace.manager.sqlalchemy.model.patron import Hold +from palace.manager.sqlalchemy.model.work import Work from palace.manager.sqlalchemy.util import ( get_one_or_create, tuple_to_numericrange, @@ -1208,6 +1209,76 @@ def row_for(work): assert suppressed_row["visibility_status"] == "manually suppressed" +def test_inventory_activity_report_hold_ratio( + db: DatabaseTransactionFixture, + services_fixture: ServicesFixture, +): + """library_hold_ratio is a real ratio, not integer division. + + The numbers come from a title in a real report (PP-4552): 31 holds against + 61 owned copies, which was being reported as 0. + """ + library = db.library(short_name="test_library") + collection = create_test_opds_collection( + "Ratio Collection", "RatioSource", db, library + ) + ds = collection.data_source + assert ds is not None + + def work_with(licenses_owned: int, holds: int) -> Work: + work = db.work( + data_source_name=ds.name, collection=collection, with_license_pool=True + ) + pool = work.license_pools[0] + pool.licenses_owned = licenses_owned + for _ in range(holds): + get_one_or_create( + db.session, + Hold, + patron=db.patron(library=library), + license_pool=pool, + position=1, + start=utc_now(), + end=utc_now() + timedelta(days=1), + ) + return work + + fraction_below_one_work = work_with(licenses_owned=61, holds=31) + fraction_above_one_work = work_with(licenses_owned=2, holds=7) + whole_ratio_work = work_with(licenses_owned=2, holds=6) + 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)) + + def ratio_for(work: Work) -> float: + identifier_value = work.presentation_edition.primary_identifier.identifier + row = next(r for r in rows if r["identifier"] == identifier_value) + return float(row["library_hold_ratio"]) + + # 31 / 61 = 0.508..., rounded to two places. Integer division reported 0. + assert ratio_for(fraction_below_one_work) == pytest.approx(0.51) + # 7 / 2. Truncation discards the fraction above 1.0 too, reporting 3 rather than 3.5. + assert ratio_for(fraction_above_one_work) == pytest.approx(3.5) + # Only whole-number ratios came through the truncation unchanged. + assert ratio_for(whole_ratio_work) == pytest.approx(3.0) + assert ratio_for(no_holds_work) == pytest.approx(0.0) + # The ratio is undefined without owned copies, and reports the -1 sentinel. + assert ratio_for(no_copies_work) == -1 + + @pytest.mark.parametrize( "status,licenses_owned,licenses_available,license_exception", [