Fix library_hold_ratio integer division in inventory report (PP-4552)

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)

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",
     [