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

[dbernstein]

Stacked on #3762 — review and merge that one first. Only the last commit belongs to this PR.

Description

shared_active_loan_count reported LicensePool.licenses_reserved — the copies held in reserve for patrons at the head of the hold queue, not a loan count. The mapping came from the original raw-SQL report (#1761) and was carried over verbatim into the SQLAlchemy rewrite.

Both loan counts now come from Loan rows, via a single lateral helper that takes a library filter or not, so the two columns share one definition of "active".

The change also excludes expired loans, which library_active_loan_count was counting.

Motivation and Context

PP-4552.

Why not derive the count from the availability counters. The obvious fix is licenses_owned - licenses_available - licenses_reserved. That was this PR's first approach, and review (#3762) correctly rejected it — the counters cannot express a loan count for every pool type:

• AGGREGATED (ODL) pools recompute licenses_owned from their licenses as min(checkouts_left, terms_concurrency), and License.checkout() decrements checkouts_left. A loan therefore shrinks the owned and available counts together and cancels itself out. Verified against a live pool: a license with concurrency 1 and 1 checkout left goes to owned 0 / available 0 / reserved 0 the moment it is borrowed, reporting 0 while a patron holds the loan. With concurrency 5 and 3 checkouts left, 2 copies out reports 0 instead of 2.
• UNLIMITED pools are imported with all four counters pinned to zero (data_layer/circulation.py), so any derived count is always zero. Verified: a pool with 3 active loans reported 0.

Both produced a self-contradicting row, with the library's own loans exceeding the supposedly-broader shared count:

library_active_loan_count = 3     shared_active_loan_count = 0

Counting Loan rows is exact for all three pool types and comes from the same source as library_active_loan_count, so library <= shared holds by construction. It also matches the column's own scope: is_shared_collection means "more than one library on this CM's integration", and the loans table covers exactly those libraries.

Expired loans. _library_loans_lateral had no filter on Loan.end, unlike the holds lateral beside it. loan_reaper deletes expired loans, but only for metered pools and only when it next runs, so the count drifted above the Collection Manager between runs — and for UNLIMITED and open-access pools was never corrected at all.

How Has This Been Tested?

Three new tests:

• test_inventory_activity_report_loan_counts — library and shared scoping on one shared collection, with expired loans in both scopes that must not be counted.
• test_inventory_activity_report_loan_counts_by_pool_type — parametrized over AGGREGATED, UNLIMITED and METERED, building real License rows for the ODL case so the counter-cancellation path is actually exercised.
• test_inventory_activity_report_shared_loan_count_unshared_collection — the -1 sentinel for unshared collections.

All three were confirmed to fail against the original licenses_reserved query. The pool-type test was additionally run against the rejected subtraction formula, where odl and unlimited fail and metered passes — reproducing the review findings exactly.

24 tests pass. mypy and pre-commit clean.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 1m 51s —— View job

---

Code review

• Gather context (diff, surrounding code)
• Review query changes (loan model indexes, reaper scope, report consumers)
• Review tests (fixture helpers, ODL/unlimited setup paths)
• Post review

Summary

This fix looks correct and is ready to merge. Both loan columns now count unexpired Loan rows through one helper, so they use the same definition of "active" and are backed by the loans.license_pool_id index. The unexpired filter matches the one in the holds lateral beside it. The docstring's description of the reaper is accurate: loan_reaper filters on LicensePool.metered_or_equivalent_type, which covers METERED and AGGREGATED, so tdilauro's note is addressed in 1105e5e. The ODL test path is real, not mocked: after License.checkout(), checkouts_left is 0, which makes is_inactive true, so total_remaining_loans is 0 and licenses_owned is 0. That is the counter-cancellation case the PR describes. There are no correctness issues. The only finding is a small efficiency nit.

Details

Nit: src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py:574-585

Calling the helper twice makes Postgres run two separate lateral scans of loans for every pool row in the report. For unshared collections, the second scan's result is discarded by case(..., else_=-1). The library-scoped lateral already visits every loan on the pool and joins patrons to filter by library, so one lateral with an aggregate FILTER can return both counts in a single pass. That keeps one definition of "active" and makes library <= shared hold by construction:

select(
    func.count(loan_alias.id).label("shared_active_loan_count"),
    func.count(loan_alias.id)
    .filter(patron_alias.library_id == Library.id)
    .label("library_active_loan_count"),
)
.join(patron_alias, loan_alias.patron_id == patron_alias.id)
.where(loan_alias.license_pool_id == LicensePool.id, unexpired)

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

Lines 574 to 585 in 1105e5e

	loan_alias = aliased(Loan)
	patron_alias = aliased(Patron)
	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)

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Changes how the inventory report counts active loans.

The PR appears safe to merge based on the reviewed changes.

Summary

The PR counts active loans from Loan rows rather than availability counters, excludes expired loans, and adds tests for library and shared counts across pool types.

• The previously reported missing helper annotations are no longer present in the current changeset; that thread is resolved.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  L[Loan rows] --> U[Exclude expired loans]
  U --> A[Library-filtered count]
  U --> B[All-library count]
  A --> R[Activity report]
  B --> S{Collection shared?}
  S -->|Yes| R
  S -->|No| N[-1 sentinel]
  N --> R

Loading

_{Reviews (9) · Last reviewed commit: "Correct reaper scope in active-loans doc..."}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (e9bfc35) to head (1105e5e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3763      +/-   ##
==========================================
- Coverage   93.74%   93.74%   -0.01%     
==========================================
  Files         510      510              
  Lines       46412    46417       +5     
  Branches     6299     6300       +1     
==========================================
+ Hits        43510    43514       +4     
- Misses       1874     1875       +1     
  Partials     1028     1028              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbernstein]

Addressed in 501d50c — dropped the untyped add_loan helper in favor of LicensePool.loan_to, as suggested. That removes the untyped function entirely rather than annotating it.

I checked loan_to before swapping it in to be sure the test still exercises the same thing: it creates the Loan via get_one_or_create with the same start/end and does not recalculate pool availability or emit analytics, so there is no behavior change. The end=None case still lands as NULL (it goes through create_method_kwargs, not the if end: branch, which only applies to pre-existing loans).

Re-verified that the test still fails against the pre-fix query, counting 3 instead of 2. mypy and pre-commit clean.

Note for anyone running the suite locally right now: tox -e py312-docker is currently broken on an unrelated infra issue — quay.io/minio/minio:latest returns unauthorized, so tox-docker cannot build the minio image. I ran these tests against a standalone postgres:16 container instead. Unrelated to this PR, but it will bite anyone else who tries.

[tdilauro]

A couple small comments, but this one is mostly ready to go. 🥇

[tdilauro]

Minor - Both metered and aggregated pool loans get this treatment.

[dbernstein]

Correct — fixed in 1105e5e. loan_reaper filters on LicensePool.metered_or_equivalent_type, which is METERED or AGGREGATED, so "only for metered pools" was wrong. The docstring now names the predicate and says which pools genuinely never get reaped (UNLIMITED and open-access).

[tdilauro]

Noting here that shared_active_loan_count includes only loans visible in the PM, while shared_active_hold_count comes from the distributor in some cases (e.g., OverDrive and Bibliotheca) and might include holds created outside of Palace.

I’m not sure how big a deal it is, but I think it’s worth mentioning and understanding the distinction.

[dbernstein]

I agree. I have another ticket open that calls out the need for just this kind of distinction. I will create a separate ticket that mirrors that one for holds.