[PP-1274] Removes ODL Hold Reaper and replaces with ODL 2 Hold Reaper.

Also the dashboard active holds query has been modified to hide any
expired holds.

bin/odl2_hold_reaper

@@ -0,0 +1,8 @@
+#!/usr/bin/env python
+"""Check for ODL 2 holds that have expired and delete them."""
+
+
+from palace.manager.api.odl import ODL2HoldReaper
+from palace.manager.scripts.monitor import RunCollectionMonitorScript
+
+RunCollectionMonitorScript(ODL2HoldReaper).run()

docker/services/cron/cron.d/circulation

@@ -81,11 +81,11 @@ HOME=/var/www/circulation
 # OPDS 1.x + ODL
 #
 15 * * * * root bin/run odl_import_monitor >> /var/log/cron.log 2>&1
-0 */8 * * * root bin/run odl_hold_reaper >> /var/log/cron.log 2>&1
 
 # OPDS 2.x + ODL import
 #
 45 * * * * root bin/run odl2_import_monitor >> /var/log/cron.log 2>&1
+0 */8 * * * root bin/run odl2_hold_reaper >> /var/log/cron.log 2>&1
 
 # SAML
 #

src/palace/manager/api/admin/dashboard_stats.py

@@ -6,7 +6,7 @@
 from datetime import datetime
 from typing import Any
 
-from sqlalchemy import union
+from sqlalchemy import not_, union
 from sqlalchemy.orm import Session
 from sqlalchemy.sql import Select, func, select
 from sqlalchemy.sql.expression import and_, distinct, false, or_, true
@@ -285,6 +285,15 @@ def _loans_or_holds_query(loan_or_hold: type[Loan] | type[Hold]) -> Select:
 
         if issubclass(loan_or_hold, Loan):
             query = query.where(loan_or_hold.end >= datetime.now())
+        elif issubclass(loan_or_hold, Hold):
+            # active holds are holds where the position is greater than zero AND end is not before present.
+            # apparently a hold can have an end that is None when the estimate end has not yet been or
+            # cannot be calculated.
+            # Hold.position = 0 and Hold.end before present is the only state where we can definitively say the
+            # hold is not active so we exclude only those holds here.
+            query = query.where(
+                not_(and_(loan_or_hold.position > 0, loan_or_hold.end < datetime.now()))
+            )
 
         return query
 

src/palace/manager/api/odl.py

@@ -45,8 +45,7 @@
     LCPHashedPassphrase,
     LCPUnhashedPassphrase,
 )
-from palace.manager.core.metadata_layer import FormatData, LicenseData, TimestampData
-from palace.manager.core.monitor import CollectionMonitor
+from palace.manager.core.metadata_layer import FormatData, LicenseData
 from palace.manager.core.opds_import import (
     BaseOPDSImporter,
     OPDSImporter,
@@ -527,6 +526,7 @@ def _checkout(
         # We have successfully borrowed this book.
         if hold:
             _db.delete(hold)
+            # log circulation event:  hold converted to loan
         self.update_licensepool(licensepool)
         return loan
 
@@ -866,6 +866,8 @@ def _release_hold(self, hold: Hold) -> Literal[True]:
         _db = Session.object_session(hold)
         licensepool = hold.license_pool
         _db.delete(hold)
+
+        # log a circulation event : hold_released
         self.update_licensepool(licensepool)
         return True
 
@@ -899,6 +901,7 @@ def patron_activity(
         for hold in holds:
             if hold.end and hold.end < utc_now():
                 _db.delete(hold)
+                # log circulation event:  hold expired
                 self.update_licensepool(hold.license_pool)
             else:
                 self._update_hold_data(hold)
@@ -1327,48 +1330,3 @@ def __init__(
         super().__init__(
             _db, collection, import_class, force_reimport=True, **import_class_kwargs
         )
-
-
-class ODLHoldReaper(CollectionMonitor):
-    """Check for holds that have expired and delete them, and update
-    the holds queues for their pools."""
-
-    SERVICE_NAME = "ODL Hold Reaper"
-    PROTOCOL = ODLAPI.label()
-
-    def __init__(
-        self,
-        _db: Session,
-        collection: Collection,
-        api: ODLAPI | None = None,
-        **kwargs: Any,
-    ):
-        super().__init__(_db, collection, **kwargs)
-        self.api = api or ODLAPI(_db, collection)
-
-    def run_once(self, progress: TimestampData) -> TimestampData:
-        # Find holds that have expired.
-        expired_holds = (
-            self._db.query(Hold)
-            .join(Hold.license_pool)
-            .filter(LicensePool.collection_id == self.api.collection_id)
-            .filter(Hold.end < utc_now())
-            .filter(Hold.position == 0)
-        )
-
-        changed_pools = set()
-        total_deleted_holds = 0
-        for hold in expired_holds:
-            changed_pools.add(hold.license_pool)
-            self._db.delete(hold)
-            total_deleted_holds += 1
-
-        for pool in changed_pools:
-            self.api.update_licensepool(pool)
-
-        message = "Holds deleted: %d. License pools updated: %d" % (
-            total_deleted_holds,
-            len(changed_pools),
-        )
-        progress = TimestampData(achievements=message)
-        return progress

src/palace/manager/api/odl2.py

@@ -20,7 +20,8 @@
     ODLLibrarySettings,
     ODLSettings,
 )
-from palace.manager.core.metadata_layer import FormatData
+from palace.manager.core.metadata_layer import FormatData, TimestampData
+from palace.manager.core.monitor import CollectionMonitor
 from palace.manager.core.opds2_import import (
     OPDS2Importer,
     OPDS2ImporterSettings,
@@ -37,7 +38,7 @@
 from palace.manager.sqlalchemy.model.licensing import LicensePool, RightsStatus
 from palace.manager.sqlalchemy.model.resource import HttpResponseTuple
 from palace.manager.util import first_or_default
-from palace.manager.util.datetime_helpers import to_utc
+from palace.manager.util.datetime_helpers import to_utc, utc_now
 
 if TYPE_CHECKING:
     from webpub_manifest_parser.core.ast import Metadata
@@ -324,3 +325,49 @@ def __init__(
         super().__init__(
             _db, collection, import_class, force_reimport=True, **import_class_kwargs
         )
+
+
+class ODL2HoldReaper(CollectionMonitor):
+    """Check for holds that have expired and delete them, and update
+    the holds queues for their pools."""
+
+    SERVICE_NAME = "ODL2 Hold Reaper"
+    PROTOCOL = ODL2API.label()
+
+    def __init__(
+        self,
+        _db: Session,
+        collection: Collection,
+        api: ODL2API | None = None,
+        **kwargs: Any,
+    ):
+        super().__init__(_db, collection, **kwargs)
+        self.api = api or ODL2API(_db, collection)
+
+    def run_once(self, progress: TimestampData) -> TimestampData:
+        # Find holds that have expired.
+        expired_holds = (
+            self._db.query(Hold)
+            .join(Hold.license_pool)
+            .filter(LicensePool.collection_id == self.api.collection_id)
+            .filter(Hold.end < utc_now())
+            .filter(Hold.position == 0)
+        )
+
+        changed_pools = set()
+        total_deleted_holds = 0
+        for hold in expired_holds:
+            changed_pools.add(hold.license_pool)
+            self._db.delete(hold)
+            # log circulation event:  hold expired
+            total_deleted_holds += 1
+
+        for pool in changed_pools:
+            self.api.update_licensepool(pool)
+
+        message = "Holds deleted: %d. License pools updated: %d" % (
+            total_deleted_holds,
+            len(changed_pools),
+        )
+        progress = TimestampData(achievements=message)
+        return progress

tests/fixtures/api_odl.py

@@ -19,7 +19,6 @@
 
 if TYPE_CHECKING:
     from tests.fixtures.database import DatabaseTransactionFixture
-    from tests.fixtures.odl import ODLTestFixture
 
 
 class LicenseHelper:
@@ -133,27 +132,27 @@ def odl_mock_get() -> MockGet:
 @pytest.fixture()
 def odl_importer(
     db: DatabaseTransactionFixture,
-    odl_test_fixture: ODLTestFixture,
+    odl2_api_test_fixture,
     odl_mock_get: MockGet,
 ) -> ODLImporter:
-    library = odl_test_fixture.library()
+    library = odl2_api_test_fixture.library()
     return ODLImporter(
         db.session,
-        collection=odl_test_fixture.collection(library),
+        collection=odl2_api_test_fixture.collection(library),
         http_get=odl_mock_get.get,
     )
 
 
 @pytest.fixture()
 def odl2_importer(
     db: DatabaseTransactionFixture,
-    odl_test_fixture: ODLTestFixture,
+    odl2_api_test_fixture,
     odl_mock_get: MockGet,
 ) -> ODL2Importer:
-    library = odl_test_fixture.library()
+    library = odl2_api_test_fixture.library()
     return ODL2Importer(
         db.session,
-        collection=odl_test_fixture.collection(library, ODL2API),
+        collection=odl2_api_test_fixture.collection(library, ODL2API),
         http_get=odl_mock_get.get,
     )
 

tests/fixtures/odl.py

@@ -247,15 +247,15 @@ def checkout(
 
 
 @pytest.fixture(scope="function")
-def odl_api_test_fixture(odl_test_fixture: ODLTestFixture) -> ODLAPITestFixture:
-    library = odl_test_fixture.library()
-    collection = odl_test_fixture.collection(library)
-    work = odl_test_fixture.work(collection)
-    license = odl_test_fixture.license(work)
-    api = odl_test_fixture.api(collection)
-    patron = odl_test_fixture.db.patron()
+def odl_api_test_fixture(odl2_api_test_fixture) -> ODLAPITestFixture:
+    library = odl2_api_test_fixture.library()
+    collection = odl2_api_test_fixture.collection(library)
+    work = odl2_api_test_fixture.work(collection)
+    license = odl2_api_test_fixture.license(work)
+    api = odl2_api_test_fixture.api(collection)
+    patron = odl2_api_test_fixture.db.patron()
     return ODLAPITestFixture(
-        odl_test_fixture, library, collection, work, license, api, patron
+        odl2_api_test_fixture, library, collection, work, license, api, patron
     )
 
 

tests/manager/api/test_odl.py

@@ -22,10 +22,9 @@
     NotCheckedOut,
     NotOnHold,
 )
-from palace.manager.api.odl import ODLHoldReaper, ODLImporter, ODLSettings
+from palace.manager.api.odl import ODLImporter, ODLSettings
 from palace.manager.sqlalchemy.constants import MediaTypes
 from palace.manager.sqlalchemy.model.collection import Collection
-from palace.manager.sqlalchemy.model.datasource import DataSource
 from palace.manager.sqlalchemy.model.edition import Edition
 from palace.manager.sqlalchemy.model.licensing import (
     DeliveryMechanism,
@@ -1510,13 +1509,13 @@ def test_import(
         self,
         odl_importer: ODLImporter,
         odl_mock_get: MockGet,
-        odl_test_fixture: ODLTestFixture,
+        old_test_fixture: ODLTestFixture,
     ) -> None:
         """Ensure that ODLImporter correctly processes and imports the ODL feed encoded using OPDS 1.x.
 
         NOTE: `freeze_time` decorator is required to treat the licenses in the ODL feed as non-expired.
         """
-        feed = odl_test_fixture.files.sample_data("feedbooks_bibliographic.atom")
+        feed = old_test_fixture.files.sample_data("feedbooks_bibliographic.atom")
 
         warrior_time_limited = LicenseInfoHelper(
             license=LicenseHelper(
@@ -2046,56 +2045,3 @@ def test_odl_importer_reimport_multiple_licenses(
 
             # Two licenses expired
             assert sum(l.is_inactive for l in imported_pool.licenses) == 2
-
-
-class TestODLHoldReaper:
-    def test_run_once(
-        self, odl_test_fixture: ODLTestFixture, db: DatabaseTransactionFixture
-    ):
-        library = odl_test_fixture.library()
-        collection = odl_test_fixture.collection(library)
-        work = odl_test_fixture.work(collection)
-        license = odl_test_fixture.license(work)
-        api = odl_test_fixture.api(collection)
-        pool = odl_test_fixture.pool(license)
-
-        data_source = DataSource.lookup(db.session, "Feedbooks", autocreate=True)
-        DatabaseTransactionFixture.set_settings(
-            collection.integration_configuration,
-            **{Collection.DATA_SOURCE_NAME_SETTING: data_source.name},
-        )
-        reaper = ODLHoldReaper(db.session, collection, api=api)
-
-        now = utc_now()
-        yesterday = now - datetime.timedelta(days=1)
-
-        license.setup(concurrency=3, available=3)
-        expired_hold1, ignore = pool.on_hold_to(db.patron(), end=yesterday, position=0)
-        expired_hold2, ignore = pool.on_hold_to(db.patron(), end=yesterday, position=0)
-        expired_hold3, ignore = pool.on_hold_to(db.patron(), end=yesterday, position=0)
-        current_hold, ignore = pool.on_hold_to(db.patron(), position=3)
-        # This hold has an end date in the past, but its position is greater than 0
-        # so the end date is not reliable.
-        bad_end_date, ignore = pool.on_hold_to(db.patron(), end=yesterday, position=4)
-
-        progress = reaper.run_once(reaper.timestamp().to_data())
-
-        # The expired holds have been deleted and the other holds have been updated.
-        assert 2 == db.session.query(Hold).count()
-        assert [current_hold, bad_end_date] == db.session.query(Hold).order_by(
-            Hold.start
-        ).all()
-        assert 0 == current_hold.position
-        assert 0 == bad_end_date.position
-        assert current_hold.end > now
-        assert bad_end_date.end > now
-        assert 1 == pool.licenses_available
-        assert 2 == pool.licenses_reserved
-
-        # The TimestampData returned reflects what work was done.
-        assert "Holds deleted: 3. License pools updated: 1" == progress.achievements
-
-        # The TimestampData does not include any timing information --
-        # that will be applied by run().
-        assert None == progress.start
-        assert None == progress.finish