From 4d57823e559a6e7e7401c4c3b80791084c48edf7 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Thu, 30 May 2024 13:42:15 -0700 Subject: [PATCH] [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 | 8 +++ bin/odl_hold_reaper | 8 --- docker/services/cron/cron.d/circulation | 2 +- .../manager/api/admin/dashboard_stats.py | 11 +++- src/palace/manager/api/odl.py | 52 ++-------------- src/palace/manager/api/odl2.py | 51 +++++++++++++++- tests/fixtures/api_odl.py | 13 ++-- tests/fixtures/odl.py | 16 ++--- tests/manager/api/test_odl.py | 60 +------------------ tests/manager/api/test_odl2.py | 57 +++++++++++++++++- 10 files changed, 145 insertions(+), 133 deletions(-) create mode 100755 bin/odl2_hold_reaper delete mode 100755 bin/odl_hold_reaper diff --git a/bin/odl2_hold_reaper b/bin/odl2_hold_reaper new file mode 100755 index 0000000000..fbd9bc74cc --- /dev/null +++ b/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() diff --git a/bin/odl_hold_reaper b/bin/odl_hold_reaper deleted file mode 100755 index b2ef6552e6..0000000000 --- a/bin/odl_hold_reaper +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env python -"""Check for ODL holds that have expired and delete them.""" - - -from palace.manager.api.odl import ODLHoldReaper -from palace.manager.scripts.monitor import RunCollectionMonitorScript - -RunCollectionMonitorScript(ODLHoldReaper).run() diff --git a/docker/services/cron/cron.d/circulation b/docker/services/cron/cron.d/circulation index 645b0a323f..dcc894a81a 100644 --- a/docker/services/cron/cron.d/circulation +++ b/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 # diff --git a/src/palace/manager/api/admin/dashboard_stats.py b/src/palace/manager/api/admin/dashboard_stats.py index 5c2730f976..abc922b7c7 100644 --- a/src/palace/manager/api/admin/dashboard_stats.py +++ b/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 diff --git a/src/palace/manager/api/odl.py b/src/palace/manager/api/odl.py index b6e95a2fb5..6587983690 100644 --- a/src/palace/manager/api/odl.py +++ b/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 diff --git a/src/palace/manager/api/odl2.py b/src/palace/manager/api/odl2.py index 47e033f537..608574e9e9 100644 --- a/src/palace/manager/api/odl2.py +++ b/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 diff --git a/tests/fixtures/api_odl.py b/tests/fixtures/api_odl.py index c17fc23a18..93cfb58ee2 100644 --- a/tests/fixtures/api_odl.py +++ b/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,13 +132,13 @@ 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, ) @@ -147,13 +146,13 @@ def odl_importer( @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, ) diff --git a/tests/fixtures/odl.py b/tests/fixtures/odl.py index 378911b566..353b8725b9 100644 --- a/tests/fixtures/odl.py +++ b/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 ) diff --git a/tests/manager/api/test_odl.py b/tests/manager/api/test_odl.py index 582726141d..aec2d39a84 100644 --- a/tests/manager/api/test_odl.py +++ b/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 diff --git a/tests/manager/api/test_odl2.py b/tests/manager/api/test_odl2.py index 897b73aa82..f680d30df2 100644 --- a/tests/manager/api/test_odl2.py +++ b/tests/manager/api/test_odl2.py @@ -2,7 +2,7 @@ import pytest from freezegun import freeze_time -from webpub_manifest_parser.core.ast import PresentationMetadata +from webpub_manifest_parser.core.ast import Collection, PresentationMetadata from webpub_manifest_parser.odl.ast import ODLPublication from webpub_manifest_parser.odl.semantic import ( ODL_PUBLICATION_MUST_CONTAIN_EITHER_LICENSES_OR_OA_ACQUISITION_LINK_ERROR, @@ -12,7 +12,7 @@ PatronHoldLimitReached, PatronLoanLimitReached, ) -from palace.manager.api.odl2 import ODL2Importer +from palace.manager.api.odl2 import ODL2HoldReaper, ODL2Importer from palace.manager.core.coverage import CoverageFailure from palace.manager.sqlalchemy.constants import ( EditionConstants, @@ -20,6 +20,7 @@ MediaTypes, ) from palace.manager.sqlalchemy.model.contributor import Contribution, Contributor +from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.edition import Edition from palace.manager.sqlalchemy.model.licensing import ( DeliveryMechanism, @@ -30,6 +31,7 @@ from palace.manager.sqlalchemy.model.resource import Hyperlink from palace.manager.sqlalchemy.model.work import Work from palace.manager.sqlalchemy.util import create +from palace.manager.util.datetime_helpers import utc_now from tests.fixtures.api_odl import ( LicenseHelper, LicenseInfoHelper, @@ -437,3 +439,54 @@ def test_hold_limit( with pytest.raises(PatronHoldLimitReached) as exc: odl2api.api.place_hold(odl2api.patron, "pin", pool, "") assert exc.value.limit == 1 + + +class TestODL2HoldReaper: + def test_run_once(self, odl2_api_test_fixture, db: DatabaseTransactionFixture): + 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) + pool = odl2_api_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 = ODL2HoldReaper(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