From 7bd03c517bc01fa2739c3b4d7eea1de6869f3db3 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 16 Oct 2024 12:01:31 -0300 Subject: [PATCH] Updated --- bin/opds2_odl_hold_reaper | 6 - docker/services/cron/cron.d/circulation | 1 - src/palace/manager/api/circulation_manager.py | 1 - .../api/controller/odl_notification.py | 23 +- src/palace/manager/api/odl/api.py | 326 ++------ src/palace/manager/api/odl/reaper.py | 59 -- src/palace/manager/celery/tasks/opds.py | 220 ++++++ src/palace/manager/service/celery/celery.py | 12 + .../manager/sqlalchemy/model/collection.py | 9 +- .../manager/sqlalchemy/model/licensing.py | 37 +- tests/fixtures/odl.py | 32 +- .../manager/api/controller/test_odl_notify.py | 30 +- tests/manager/api/odl/test_api.py | 733 +----------------- tests/manager/api/odl/test_reaper.py | 59 -- tests/manager/celery/tasks/test_opds.py | 323 ++++++++ 15 files changed, 727 insertions(+), 1144 deletions(-) delete mode 100755 bin/opds2_odl_hold_reaper delete mode 100644 src/palace/manager/api/odl/reaper.py create mode 100644 src/palace/manager/celery/tasks/opds.py delete mode 100644 tests/manager/api/odl/test_reaper.py create mode 100644 tests/manager/celery/tasks/test_opds.py diff --git a/bin/opds2_odl_hold_reaper b/bin/opds2_odl_hold_reaper deleted file mode 100755 index daea01a48..000000000 --- a/bin/opds2_odl_hold_reaper +++ /dev/null @@ -1,6 +0,0 @@ -#!/usr/bin/env python -"""Check for ODL 2 holds that have expired and delete them.""" -from palace.manager.api.odl.reaper import OPDS2WithODLHoldReaper -from palace.manager.scripts.monitor import RunCollectionMonitorScript - -RunCollectionMonitorScript(OPDS2WithODLHoldReaper).run() diff --git a/docker/services/cron/cron.d/circulation b/docker/services/cron/cron.d/circulation index 10122136e..ef5632274 100644 --- a/docker/services/cron/cron.d/circulation +++ b/docker/services/cron/cron.d/circulation @@ -78,7 +78,6 @@ HOME=/var/www/circulation # OPDS 2.x + ODL import # 45 * * * * root bin/run opds2_odl_import_monitor >> /var/log/cron.log 2>&1 -0 */8 * * * root bin/run opds2_odl_hold_reaper >> /var/log/cron.log 2>&1 # SAML # diff --git a/src/palace/manager/api/circulation_manager.py b/src/palace/manager/api/circulation_manager.py index affdc6cb1..2c7b03246 100644 --- a/src/palace/manager/api/circulation_manager.py +++ b/src/palace/manager/api/circulation_manager.py @@ -359,7 +359,6 @@ def setup_one_time_controllers(self): self.version = ApplicationVersionController() self.odl_notification_controller = ODLNotificationController( self._db, - self, self.services.integration_registry.license_providers(), ) self.patron_auth_token = PatronAuthTokenController(self) diff --git a/src/palace/manager/api/controller/odl_notification.py b/src/palace/manager/api/controller/odl_notification.py index faea4135a..78983ae8f 100644 --- a/src/palace/manager/api/controller/odl_notification.py +++ b/src/palace/manager/api/controller/odl_notification.py @@ -1,7 +1,5 @@ from __future__ import annotations -from typing import TYPE_CHECKING - import flask from flask import Response from flask_babel import lazy_gettext as _ @@ -18,15 +16,12 @@ from palace.manager.service.integration_registry.license_providers import ( LicenseProvidersRegistry, ) -from palace.manager.sqlalchemy.model.library import Library from palace.manager.sqlalchemy.model.patron import Loan from palace.manager.sqlalchemy.util import get_one +from palace.manager.util.datetime_helpers import utc_now from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import ProblemDetail -if TYPE_CHECKING: - from palace.manager.api.circulation_manager import CirculationManager - class ODLNotificationController(LoggerMixin): """Receive notifications from an ODL distributor when the @@ -36,20 +31,12 @@ class ODLNotificationController(LoggerMixin): def __init__( self, db: Session, - manager: CirculationManager, registry: LicenseProvidersRegistry, ) -> None: self.db = db - self.manager = manager self.registry = registry - def get_api(self, library: Library, loan: Loan) -> OPDS2WithODLApi: - return self.manager.circulation_apis[library.id].api_for_license_pool( # type: ignore[no-any-return] - loan.license_pool - ) - def notify(self, loan_id: int) -> Response | ProblemDetail: - library = flask.request.library # type: ignore[attr-defined] status_doc_json = flask.request.data loan = get_one(self.db, Loan, id=loan_id) @@ -75,7 +62,11 @@ def notify(self, loan_id: int) -> Response | ProblemDetail: ): return INVALID_LOAN_FOR_ODL_NOTIFICATION - api = self.get_api(library, loan) - api.update_loan(loan, status_doc) + # TODO: This should really just trigger a celery task to do an availabilty sync on the + # license, since this is flagging that we might be out of sync with the distributor. + # Once we move the OPDS2WithODL scripts to celery this should be possible. + # For now we just mark the loan as expired. + if not status_doc.active: + loan.end = utc_now() return Response(status=204) diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index 2c0a30802..b151940ff 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -11,7 +11,6 @@ from dependency_injector.wiring import Provide, inject from flask import url_for from pydantic import ValidationError -from sqlalchemy import or_ from sqlalchemy.orm import Session from uritemplate import URITemplate @@ -22,7 +21,6 @@ Fulfillment, HoldInfo, LoanInfo, - PatronActivityCirculationAPI, RedirectFulfillment, UrlFulfillment, ) @@ -59,7 +57,6 @@ from palace.manager.service.container import Services from palace.manager.sqlalchemy.model.collection import Collection from palace.manager.sqlalchemy.model.datasource import DataSource -from palace.manager.sqlalchemy.model.library import Library from palace.manager.sqlalchemy.model.licensing import ( DeliveryMechanism, LicensePool, @@ -74,7 +71,7 @@ class OPDS2WithODLApi( OdlAuthenticatedRequest, - PatronActivityCirculationAPI[OPDS2WithODLSettings, OPDS2WithODLLibrarySettings], + BaseCirculationAPI[OPDS2WithODLSettings, OPDS2WithODLLibrarySettings], ): """ODL (Open Distribution to Libraries) is a specification that allows libraries to manage their own loans and holds. It offers a deeper level @@ -283,26 +280,17 @@ def checkout( raise AlreadyCheckedOut() if licensepool.open_access or licensepool.unlimited_access: - loan_start = None - loan_end = None - external_identifier = None + return LoanInfo.from_license_pool( + licensepool, + end_date=None, + ) else: hold = get_one(_db, Hold, patron=patron, license_pool_id=licensepool.id) - loan_obj = self._checkout(patron, licensepool, hold) - loan_start = loan_obj.start - loan_end = loan_obj.end - external_identifier = loan_obj.external_identifier - - return LoanInfo.from_license_pool( - licensepool, - start_date=loan_start, - end_date=loan_end, - external_identifier=external_identifier, - ) + return self._checkout(patron, licensepool, hold) def _checkout( self, patron: Patron, licensepool: LicensePool, hold: Hold | None = None - ) -> Loan: + ) -> LoanInfo: # If the loan limit is not None or 0 if self.loan_limit: loans = list( @@ -314,16 +302,14 @@ def _checkout( if len(loans) >= self.loan_limit: raise PatronLoanLimitReached(limit=self.loan_limit) - _db = Session.object_session(patron) + db = Session.object_session(patron) if not any(l for l in licensepool.licenses if not l.is_inactive): raise NoLicenses() # Make sure pool info is updated. - self.update_licensepool(licensepool) - - if hold: - self._update_hold_data(hold) + # Update the pool and the next holds in the queue when a license is reserved. + licensepool.update_availability_from_licenses() # If there's a holds queue, the patron must have a non-expired hold # with position 0 to check out the book. @@ -339,22 +325,18 @@ def _checkout( license = licensepool.best_available_license() if not license: raise NoAvailableCopies() - loan, ignore = license.loan_to(patron) - identifier = loan.license.identifier + identifier = license.identifier checkout_id = str(uuid.uuid4()) if self.collection is None: raise PalaceValueError(f"Collection not found: {self.collection_id}") - default_loan_period = self.collection.default_loan_period(loan.patron.library) + default_loan_period = self.collection.default_loan_period(patron.library) requested_expiry = utc_now() + datetime.timedelta(days=default_loan_period) patron_id = patron.identifier_to_remote_service(licensepool.data_source) - library_short_name = loan.patron.library.short_name + library_short_name = patron.library.short_name - db = Session.object_session(loan) - patron = loan.patron hasher = self._get_hasher() - unhashed_pass = self._credential_factory.get_patron_passphrase(db, patron) hashed_pass = unhashed_pass.hash(hasher) self._credential_factory.set_hashed_passphrase(db, patron, hashed_pass) @@ -363,7 +345,7 @@ def _checkout( notification_url = self._url_for( "odl_notify", library_short_name=library_short_name, - loan_id=loan.id, + loan_id=checkout_id, _external=True, ) @@ -390,26 +372,31 @@ def _checkout( "http://opds-spec.org/odl/error/checkout/unavailable" ], ) - except BadResponseException as e: - _db.delete(loan) - if isinstance(e, OpdsWithOdlException): - if e.type == "http://opds-spec.org/odl/error/checkout/unavailable": - # TODO: This would be a good place to do an async availability update, since we know - # the book is unavailable, when we thought it was available. For now, we know that - # the license has no checkouts_available, so we do that update. - license.checkouts_available = 0 - licensepool.update_availability_from_licenses() - raise NoAvailableCopies() + except OpdsWithOdlException as e: + if e.type == "http://opds-spec.org/odl/error/checkout/unavailable": + # TODO: This would be a good place to do an async availability update, since we know + # the book is unavailable, when we thought it was available. For now, we know that + # the license has no checkouts_available, so we do that update. + license.checkouts_available = 0 + if hold: + # If we have a hold, it means we thought the book was available, but it wasn't. + # So we need to update its position in the hold queue. We will put it at position + # 1, since the patron should be first in line. This may mean that there are two + # patrons in position 1 in the hold queue, but this will be resolved next time + # the hold queue is recalculated. + hold.position = 1 + # Update the pool and the next holds in the queue when a license is reserved. + licensepool.update_availability_from_licenses() + raise NoAvailableCopies() raise if not doc.active: # Something went wrong with this loan and we don't actually # have the book checked out. This should never happen. # Remove the loan we created. - _db.delete(loan) raise CannotLoan() - # We save the link to the loan status document in the loan's external_identifier field, so + # We save the link to the loan status document in the loan's status_url field, so # we are able to retrieve it later. loan_status_document_link: BaseLink | None = doc.links.get( rel="self", type=LoanStatus.content_type() @@ -436,23 +423,24 @@ def _checkout( ) if not loan_status_document_link: - _db.delete(loan) raise CannotLoan() - # We need to set the start and end dates on our local loan since - # the code that calls this only sets them when a new loan is created. - loan.start = utc_now() - loan.end = doc.potential_rights.end - loan.external_identifier = loan_status_document_link.href + loan = LoanInfo.from_license_pool( + licensepool, + end_date=doc.potential_rights.end, + external_identifier=loan_status_document_link.href, + ) # We also need to update the remaining checkouts for the license. - loan.license.checkout() + license.checkout() # We have successfully borrowed this book. if hold: - _db.delete(hold) + db.delete(hold) # log circulation event: hold converted to loan - self.update_licensepool(licensepool) + + # Update the pool to reflect the new loan. + licensepool.update_availability_from_licenses() return loan def fulfill( @@ -515,6 +503,8 @@ def _license_fulfill( # through the DRM system, and we didn't get a notification # from the distributor yet. self.update_loan(loan, doc) + db = Session.object_session(loan) + db.delete(loan) raise CannotFulfill() drm_scheme = delivery_mechanism.delivery_mechanism.drm_scheme @@ -592,162 +582,6 @@ def _fulfill( else: return self._license_fulfill(loan, delivery_mechanism) - def _count_holds_before(self, holdinfo: HoldInfo, pool: LicensePool) -> int: - # Count holds on the license pool that started before this hold and - # aren't expired. - _db = Session.object_session(pool) - return ( - _db.query(Hold) - .filter(Hold.license_pool_id == pool.id) - .filter(Hold.start < holdinfo.start_date) - .filter( - or_( - Hold.end == None, - Hold.end > utc_now(), - Hold.position > 0, - ) - ) - .count() - ) - - def _update_hold_data(self, hold: Hold) -> None: - pool: LicensePool = hold.license_pool - holdinfo = HoldInfo.from_license_pool( - pool, - start_date=hold.start, - end_date=hold.end, - hold_position=hold.position, - ) - library = hold.patron.library - self._update_hold_end_date(holdinfo, pool, library=library) - hold.end = holdinfo.end_date - hold.position = holdinfo.hold_position - - def _update_hold_end_date( - self, holdinfo: HoldInfo, pool: LicensePool, library: Library - ) -> None: - _db = Session.object_session(pool) - # First make sure the hold position is up-to-date, since we'll - # need it to calculate the end date. - original_position = holdinfo.hold_position - self._update_hold_position(holdinfo, pool) - assert holdinfo.hold_position is not None - - if self.collection is None: - raise ValueError(f"Collection not found: {self.collection_id}") - default_loan_period = self.collection.default_loan_period(library) - default_reservation_period = self.collection.default_reservation_period - - # If the hold was already to check out and already has an end date, - # it doesn't need an update. - if holdinfo.hold_position == 0 and original_position == 0 and holdinfo.end_date: - return - - # If the patron is in the queue, we need to estimate when the book - # will be available for check out. We can do slightly better than the - # default calculation since we know when all current loans will expire, - # but we're still calculating the worst case. - elif holdinfo.hold_position > 0: - # Find the current loans and reserved holds for the licenses. - current_loans = ( - _db.query(Loan) - .filter(Loan.license_pool_id == pool.id) - .filter(or_(Loan.end == None, Loan.end > utc_now())) - .order_by(Loan.start) - .all() - ) - current_holds = ( - _db.query(Hold) - .filter(Hold.license_pool_id == pool.id) - .filter( - or_( - Hold.end == None, - Hold.end > utc_now(), - Hold.position > 0, - ) - ) - .order_by(Hold.start) - .all() - ) - assert pool.licenses_owned is not None - licenses_reserved = min( - pool.licenses_owned - len(current_loans), len(current_holds) - ) - current_reservations = current_holds[:licenses_reserved] - - # The licenses will have to go through some number of cycles - # before one of them gets to this hold. This leavs out the first cycle - - # it's already started so we'll handle it separately. - cycles = ( - holdinfo.hold_position - licenses_reserved - 1 - ) // pool.licenses_owned - - # Each of the owned licenses is currently either on loan or reserved. - # Figure out which license this hold will eventually get if every - # patron keeps their loans and holds for the maximum time. - copy_index = ( - holdinfo.hold_position - licenses_reserved - 1 - ) % pool.licenses_owned - - # In the worse case, the first cycle ends when a current loan expires, or - # after a current reservation is checked out and then expires. - if len(current_loans) > copy_index: - next_cycle_start = current_loans[copy_index].end - else: - reservation = current_reservations[copy_index - len(current_loans)] - next_cycle_start = reservation.end + datetime.timedelta( - days=default_loan_period - ) - - # Assume all cycles after the first cycle take the maximum time. - cycle_period = default_loan_period + default_reservation_period - holdinfo.end_date = next_cycle_start + datetime.timedelta( - days=(cycle_period * cycles) - ) - - # If the end date isn't set yet or the position just became 0, the - # hold just became available. The patron's reservation period starts now. - else: - holdinfo.end_date = utc_now() + datetime.timedelta( - days=default_reservation_period - ) - - _db.expire_all() - - def _update_hold_position(self, holdinfo: HoldInfo, pool: LicensePool) -> None: - _db = Session.object_session(pool) - loans_count = ( - _db.query(Loan) - .filter( - Loan.license_pool_id == pool.id, - ) - .filter(or_(Loan.end == None, Loan.end > utc_now())) - .count() - ) - holds_count = self._count_holds_before(holdinfo, pool) - - assert pool.licenses_owned is not None - remaining_licenses = pool.licenses_owned - loans_count - - if remaining_licenses > holds_count: - # The hold is ready to check out. - holdinfo.hold_position = 0 - - else: - # Add 1 since position 0 indicates the hold is ready. - holdinfo.hold_position = holds_count + 1 - - def update_licensepool(self, licensepool: LicensePool) -> None: - # Update the pool and the next holds in the queue when a license is reserved. - licensepool.update_availability_from_licenses( - as_of=utc_now(), - ) - holds = licensepool.get_active_holds() - for hold in holds[: licensepool.licenses_reserved]: - if hold.position != 0: - # This hold just got a reserved license. - self._update_hold_data(hold) - def place_hold( self, patron: Patron, @@ -777,7 +611,8 @@ def _place_hold(self, patron: Patron, licensepool: LicensePool) -> HoldInfo: _db = Session.object_session(patron) # Make sure pool info is updated. - self.update_licensepool(licensepool) + # Update the pool and the next holds in the queue when a license is reserved. + licensepool.update_availability_from_licenses() if licensepool.licenses_available > 0: raise CurrentlyAvailable() @@ -803,10 +638,8 @@ def _place_hold(self, patron: Patron, licensepool: LicensePool) -> HoldInfo: licensepool, start_date=utc_now(), end_date=None, - hold_position=0, + hold_position=licensepool.patrons_in_hold_queue, ) - library = patron.library - self._update_hold_end_date(holdinfo, licensepool, library=library) return holdinfo @@ -828,73 +661,18 @@ def _release_hold(self, hold: Hold) -> Literal[True]: # of checking it out, but no one else had the book on hold, the # book is now available for anyone to check out. If someone else # had a hold, the license is now reserved for the next patron. - # If someone else had a hold, the license is now reserved for the - # next patron, and we need to update that hold. _db = Session.object_session(hold) licensepool = hold.license_pool _db.delete(hold) # log a circulation event : hold_released - self.update_licensepool(licensepool) + # Update the pool and the next holds in the queue when a license is reserved. + licensepool.update_availability_from_licenses() return True - def patron_activity( - self, patron: Patron, pin: str | None - ) -> list[LoanInfo | HoldInfo]: - """Look up non-expired loans for this collection in the database.""" - _db = Session.object_session(patron) - loans = ( - _db.query(Loan) - .join(Loan.license_pool) - .filter(LicensePool.collection_id == self.collection_id) - .filter(Loan.patron == patron) - .filter( - or_( - Loan.end >= utc_now(), - Loan.end == None, - ) - ) - ) - - # Get the patron's holds. If there are any expired holds, delete them. - # Update the end date and position for the remaining holds. - holds = ( - _db.query(Hold) - .join(Hold.license_pool) - .filter(LicensePool.collection_id == self.collection_id) - .filter(Hold.patron == patron) - ) - remaining_holds = [] - 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) - remaining_holds.append(hold) - - return [ - LoanInfo.from_license_pool( - loan.license_pool, - start_date=loan.start, - end_date=loan.end, - external_identifier=loan.external_identifier, - ) - for loan in loans - ] + [ - HoldInfo.from_license_pool( - hold.license_pool, - start_date=hold.start, - end_date=hold.end, - hold_position=hold.position, - ) - for hold in remaining_holds - ] - def update_loan(self, loan: Loan, status_doc: LoanStatus) -> None: - """Check a loan's status, and if it is no longer active, delete the loan - and update its pool's availability. + """ + Check a loan's status, and if it is no longer active, update its pool's availability. """ _db = Session.object_session(loan) @@ -906,8 +684,8 @@ def update_loan(self, loan: Loan, status_doc: LoanStatus) -> None: loan.license.checkin() # If there are holds, the license is reserved for the next patron. - _db.delete(loan) - self.update_licensepool(loan.license_pool) + # Update the pool and the next holds in the queue when a license is reserved. + loan.license_pool.update_availability_from_licenses() def update_availability(self, licensepool: LicensePool) -> None: pass diff --git a/src/palace/manager/api/odl/reaper.py b/src/palace/manager/api/odl/reaper.py deleted file mode 100644 index 554c99931..000000000 --- a/src/palace/manager/api/odl/reaper.py +++ /dev/null @@ -1,59 +0,0 @@ -from __future__ import annotations - -from typing import Any - -from sqlalchemy.orm import Session - -from palace.manager.api.odl.api import OPDS2WithODLApi -from palace.manager.core.metadata_layer import TimestampData -from palace.manager.core.monitor import CollectionMonitor -from palace.manager.sqlalchemy.model.collection import Collection -from palace.manager.sqlalchemy.model.licensing import LicensePool -from palace.manager.sqlalchemy.model.patron import Hold -from palace.manager.util.datetime_helpers import utc_now - - -class OPDS2WithODLHoldReaper(CollectionMonitor): - """Check for holds that have expired and delete them, and update - the holds queues for their pools.""" - - SERVICE_NAME = "ODL2 Hold Reaper" - PROTOCOL = OPDS2WithODLApi.label() - - def __init__( - self, - _db: Session, - collection: Collection, - api: OPDS2WithODLApi | None = None, - **kwargs: Any, - ): - super().__init__(_db, collection, **kwargs) - self.api = api or OPDS2WithODLApi(_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/src/palace/manager/celery/tasks/opds.py b/src/palace/manager/celery/tasks/opds.py new file mode 100644 index 000000000..d750c9851 --- /dev/null +++ b/src/palace/manager/celery/tasks/opds.py @@ -0,0 +1,220 @@ +import datetime +from logging import Logger + +from celery import shared_task +from sqlalchemy import delete, select +from sqlalchemy.orm import Session + +from palace.manager.api.odl.api import OPDS2WithODLApi +from palace.manager.celery.task import Task +from palace.manager.service.celery.celery import QueueNames +from palace.manager.service.redis.models.lock import RedisLock +from palace.manager.sqlalchemy.model.collection import Collection +from palace.manager.sqlalchemy.model.licensing import License, LicensePool +from palace.manager.sqlalchemy.model.patron import Hold +from palace.manager.util.datetime_helpers import utc_now + + +def remove_expired_holds_for_collection(db: Session, collection_id: int) -> int: + """ + Remove expired holds from the database for this collection. + """ + query = ( + delete(Hold) + .where( + Hold.position == 0, + Hold.end < utc_now(), + Hold.license_pool_id == LicensePool.id, + LicensePool.collection_id == collection_id, + ) + .execution_options(synchronize_session="fetch") + ) + result = db.execute(query) + # We need the type ignores here because result doesn't always have + # a rowcount, but the sqlalchemy docs swear it will in the case of + # a delete statement. + # https://docs.sqlalchemy.org/en/20/tutorial/data_update.html#getting-affected-row-count-from-update-delete + return result.rowcount # type: ignore[attr-defined,no-any-return] + + +def licensepool_ids_with_holds( + db: Session, collection_id: int, batch_size: int, after_id: int | None +) -> list[int]: + query = ( + select(LicensePool.id) + .join(Hold) + .where(LicensePool.collection_id == collection_id) + .order_by(LicensePool.id) + .limit(batch_size) + .distinct() + ) + + if after_id: + query = query.where(LicensePool.id > after_id) + + return db.scalars(query).all() + + +def lock_licenses(session: Session, license_pool_id: int) -> None: + """ + Acquire a row level lock on all the licenses for a license pool. + + WARNING: This function should be called within a transaction that + will be relatively short-lived. Since this will cause all the + licenses for the license pool to be locked, it could cause + contention or deadlocks if it is held for a long time. + """ + session.execute( + select(License.id) + .where(License.license_pool_id == license_pool_id) + .with_for_update() + ).all() + + +def recalculate_holds_for_licensepool( + session: Session, + log: Logger, + license_pool_id: int, + reservation_period: datetime.timedelta, +) -> None: + pool = ( + session.scalars(select(LicensePool).where(LicensePool.id == license_pool_id)) + .unique() + .one() + ) + if pool is None: + log.info( + f"Skipping license pool {license_pool_id} because it no longer exists." + ) + return + edition_title = ( + pool.presentation_edition.title if pool.presentation_edition else None + ) + author = pool.presentation_edition.author if pool.presentation_edition else None + + # We take out row level locks on all the licenses and holds for this license pool, so that + # everything is in a consistent state while we update the hold queue. This means we should be + # quickly committing the transaction, to avoid contention or deadlocks. + lock_licenses(session, license_pool_id) + holds = pool.get_active_holds(for_update=True) + + pool.update_availability_from_licenses() + reserved = pool.licenses_reserved + + ready = holds[:reserved] + waiting = holds[reserved:] + updated = 0 + + # These holds have a copy reserved for them. + for hold in ready: + # If this hold isn't already in position 0, the hold just became available. + # We need to set it to position 0 and set its end date. + if hold.position != 0 or hold.end is None: + hold.position = 0 + hold.end = utc_now() + reservation_period + updated += 1 + + # Update the position for the remaining holds. + for idx, hold in enumerate(waiting): + position = idx + 1 + if hold.position != position: + hold.position = position + hold.end = None + updated += 1 + + log.debug( + f"Updated hold queue for license pool {license_pool_id} ({edition_title} by {author}). " + f"{updated} holds out of date." + ) + + +@shared_task(queue=QueueNames.default, bind=True) +def remove_expired_holds(task: Task) -> None: + """ + Remove expired holds from the database. + """ + registry = task.services.integration_registry.license_providers() + protocols = registry.get_protocols(OPDS2WithODLApi, default=False) + with task.session() as session: + collections = [ + (collection.id, collection.name) + for collection in Collection.by_protocol(session, protocols) + if collection.id is not None + ] + for collection_id, collection_name in collections: + with task.transaction() as session: + removed = remove_expired_holds_for_collection(session, collection_id) + task.log.info( + f"Removed {removed} expired holds for collection {collection_name} ({collection_id})." + ) + + +@shared_task(queue=QueueNames.default, bind=True) +def recalculate_hold_queue(task: Task) -> None: + """ + Queue a task for each OPDS2WithODLApi integration to recalculate the hold queue. + """ + registry = task.services.integration_registry.license_providers() + protocols = registry.get_protocols(OPDS2WithODLApi, default=False) + with task.session() as session: + for collection in Collection.by_protocol(session, protocols): + recalculate_hold_queue_collection.delay(collection.id) + + +@shared_task(queue=QueueNames.default, bind=True) +def recalculate_hold_queue_collection( + task: Task, collection_id: int, batch_size: int = 100, after_id: int | None = None +) -> None: + """ + Recalculate the hold queue for a collection. + """ + lock = RedisLock( + task.services.redis.client(), + lock_name=[ + "RecalculateHolds", + Collection.redis_key_from_id(collection_id), + ], + ) + with lock.lock() as locked: + if not locked: + task.log.info( + f"Skipping collection {collection_id} because another task holds its lock." + ) + return + with task.transaction() as session: + collection = Collection.by_id(session, collection_id) + if collection is None: + task.log.info( + f"Skipping collection {collection_id} because it no longer exists." + ) + return + + collection_name = collection.name + reservation_period = datetime.timedelta( + days=collection.default_reservation_period + ) + task.log.info( + f"Recalculating hold queue for collection {collection_name} ({collection_id})." + ) + + license_pool_ids = licensepool_ids_with_holds( + session, collection_id, batch_size, after_id + ) + + for license_pool_id in license_pool_ids: + with task.transaction() as session: + recalculate_holds_for_licensepool( + session, task.log, license_pool_id, reservation_period + ) + + if len(license_pool_ids) == batch_size: + # We are done this batch, but there is probably more work to do, we queue up the next batch. + raise task.replace( + recalculate_hold_queue_collection.s( + collection_id, batch_size, license_pool_ids[-1] + ) + ) + + task.log.info( + f"Finished recalculating hold queue for collection {collection_name} ({collection_id})." + ) diff --git a/src/palace/manager/service/celery/celery.py b/src/palace/manager/service/celery/celery.py index b2796d3cc..1c2898371 100644 --- a/src/palace/manager/service/celery/celery.py +++ b/src/palace/manager/service/celery/celery.py @@ -50,6 +50,18 @@ def beat_schedule() -> dict[str, Any]: minute="0", ), # Run every day at 1:00 AM }, + "opds2_odl_remove_expired_holds": { + "task": "opds.remove_expired_holds", + "schedule": crontab( + minute="16", + ), # Run every hour at 16 minutes past the hour + }, + "opds2_odl_recalculate_hold_queue": { + "task": "opds.recalculate_hold_queue", + "schedule": crontab( + minute="31", + ), # Run every hour at 31 minutes past the hour + }, } diff --git a/src/palace/manager/sqlalchemy/model/collection.py b/src/palace/manager/sqlalchemy/model/collection.py index e59f646f1..d35ab3812 100644 --- a/src/palace/manager/sqlalchemy/model/collection.py +++ b/src/palace/manager/sqlalchemy/model/collection.py @@ -243,7 +243,9 @@ def by_name(cls, _db: Session, name: str) -> Collection | None: ).scalar_one_or_none() @classmethod - def by_protocol(cls, _db: Session, protocol: str | None) -> Query[Collection]: + def by_protocol( + cls, _db: Session, protocol: list[str] | str | None + ) -> Query[Collection]: """Query collections that get their licenses through the given protocol. Collections marked for deletion are not included. @@ -260,9 +262,12 @@ def by_protocol(cls, _db: Session, protocol: str | None) -> Query[Collection]: == Collection.integration_configuration_id, ) .filter(IntegrationConfiguration.goal == Goals.LICENSE_GOAL) - .filter(IntegrationConfiguration.protocol == protocol) .filter(Collection.marked_for_deletion == False) ) + if isinstance(protocol, str): + qu = qu.filter(IntegrationConfiguration.protocol == protocol) + else: + qu = qu.filter(IntegrationConfiguration.protocol.in_(protocol)) return qu diff --git a/src/palace/manager/sqlalchemy/model/licensing.py b/src/palace/manager/sqlalchemy/model/licensing.py index 120f7b02c..d34d3adde 100644 --- a/src/palace/manager/sqlalchemy/model/licensing.py +++ b/src/palace/manager/sqlalchemy/model/licensing.py @@ -5,14 +5,23 @@ import datetime import logging from enum import Enum as PythonEnum +from operator import and_ from typing import TYPE_CHECKING, Literal, overload from sqlalchemy import Boolean, Column, DateTime from sqlalchemy import Enum as AlchemyEnum -from sqlalchemy import ForeignKey, Index, Integer, String, Unicode, UniqueConstraint -from sqlalchemy.orm import Mapped, relationship +from sqlalchemy import ( + ForeignKey, + Index, + Integer, + String, + Unicode, + UniqueConstraint, + or_, + select, +) +from sqlalchemy.orm import Mapped, lazyload, relationship from sqlalchemy.orm.session import Session -from sqlalchemy.sql.expression import or_ from palace.manager.core.exceptions import BasePalaceException from palace.manager.sqlalchemy.constants import ( @@ -724,22 +733,26 @@ def update_availability_from_licenses( as_of=as_of, ) - def get_active_holds(self): + def get_active_holds(self, for_update: bool = False) -> list[Hold]: _db = Session.object_session(self) - return ( - _db.query(Hold) - .filter(Hold.license_pool_id == self.id) - .filter( + query = ( + select(Hold) + .where(Hold.license_pool_id == self.id) + .where( or_( - Hold.end == None, - Hold.end > utc_now(), - Hold.position > 0, + Hold.position != 0, + Hold.position == None, + and_(Hold.end > utc_now(), Hold.position == 0), ) ) .order_by(Hold.start) - .all() ) + if for_update: + query = query.options(lazyload(Hold.patron)).with_for_update() + + return _db.execute(query).scalars().all() + def update_availability( self, new_licenses_owned, diff --git a/tests/fixtures/odl.py b/tests/fixtures/odl.py index 7a124e455..2037f72df 100644 --- a/tests/fixtures/odl.py +++ b/tests/fixtures/odl.py @@ -11,7 +11,7 @@ from jinja2 import Template from requests import Response -from palace.manager.api.circulation import LoanInfo +from palace.manager.api.circulation import HoldInfo, LoanInfo from palace.manager.api.odl.api import OPDS2WithODLApi from palace.manager.api.odl.importer import OPDS2WithODLImporter from palace.manager.core.coverage import CoverageFailure @@ -26,7 +26,7 @@ LicensePool, LicensePoolDeliveryMechanism, ) -from palace.manager.sqlalchemy.model.patron import Loan, Patron +from palace.manager.sqlalchemy.model.patron import Patron from palace.manager.sqlalchemy.model.work import Work from palace.manager.util.datetime_helpers import utc_now from tests.fixtures.database import DatabaseTransactionFixture @@ -176,7 +176,8 @@ def checkout( loan_url: str | None = None, patron: Patron | None = None, pool: LicensePool | None = None, - ) -> tuple[LoanInfo, Loan]: + create_loan: bool = False, + ) -> LoanInfo: patron = patron or self.patron pool = pool or self.pool loan_url = loan_url or self.db.fresh_url() @@ -184,13 +185,24 @@ def checkout( self.mock_http.queue_response( 201, content=self.loan_status_document(self_link=loan_url).model_dump_json() ) - loan = self.api.checkout(patron, "pin", pool, MagicMock()) - loan_db = ( - self.db.session.query(Loan) - .filter(Loan.license_pool == pool, Loan.patron == patron) - .one() - ) - return loan, loan_db + loan_info = self.api_checkout(patron=patron, licensepool=pool) + if create_loan: + loan_info.create_or_update(patron, pool) + return loan_info + + def place_hold( + self, + patron: Patron | None = None, + pool: LicensePool | None = None, + create_hold: bool = False, + ) -> HoldInfo: + patron = patron or self.patron + pool = pool or self.pool + + hold_info = self.api.place_hold(patron, "pin", pool, "dummy@email.com") + if create_hold: + hold_info.create_or_update(patron, pool) + return hold_info @pytest.fixture(scope="function") diff --git a/tests/manager/api/controller/test_odl_notify.py b/tests/manager/api/controller/test_odl_notify.py index 34105df6a..4a57bb78e 100644 --- a/tests/manager/api/controller/test_odl_notify.py +++ b/tests/manager/api/controller/test_odl_notify.py @@ -1,7 +1,6 @@ -from unittest.mock import MagicMock - import pytest from flask import Response +from freezegun import freeze_time from palace.manager.api.controller.odl_notification import ODLNotificationController from palace.manager.api.odl.api import OPDS2WithODLApi @@ -10,13 +9,13 @@ NO_ACTIVE_LOAN, ) from palace.manager.core.problem_details import INVALID_INPUT +from palace.manager.util.datetime_helpers import utc_now from palace.manager.util.problem_detail import ProblemDetail from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.flask import FlaskAppFixture from tests.fixtures.odl import OPDS2WithODLApiFixture from tests.fixtures.services import ServicesFixture from tests.mocks.mock import MockHTTPClient -from tests.mocks.odl import MockOPDS2WithODLApi class ODLFixture: @@ -48,14 +47,7 @@ def __init__( self.pool.update_availability_from_licenses() self.patron = self.db.patron() self.http_client = MockHTTPClient() - self.api = MockOPDS2WithODLApi(db.session, self.collection, self.http_client) - self.mock_circulation_manager = MagicMock() - self.mock_circulation_manager.circulation_apis[ - self.library.id - ].api_for_license_pool.return_value = self.api - self.controller = ODLNotificationController( - db.session, self.mock_circulation_manager, self.registry - ) + self.controller = ODLNotificationController(db.session, self.registry) self.loan_status_document = OPDS2WithODLApiFixture.loan_status_document @@ -70,6 +62,7 @@ class TestODLNotificationController: """Test that an ODL distributor can notify the circulation manager when a loan's status changes.""" + @freeze_time() def test_notify_success( self, db: DatabaseTransactionFixture, @@ -82,6 +75,19 @@ def test_notify_success( assert odl_fixture.license.checkouts_available == 0 + status_doc = odl_fixture.loan_status_document("active") + with flask_app_fixture.test_request_context( + "/", + method="POST", + data=status_doc.model_dump_json(), + library=odl_fixture.library, + ): + assert loan.id is not None + response = odl_fixture.controller.notify(loan.id) + assert response.status_code == 204 + + assert loan.end != utc_now() + status_doc = odl_fixture.loan_status_document("revoked") with flask_app_fixture.test_request_context( "/", @@ -93,7 +99,7 @@ def test_notify_success( response = odl_fixture.controller.notify(loan.id) assert response.status_code == 204 - assert odl_fixture.license.checkouts_available == 1 + assert loan.end == utc_now() def test_notify_errors( self, diff --git a/tests/manager/api/odl/test_api.py b/tests/manager/api/odl/test_api.py index de5a95dd2..7f05d4d23 100644 --- a/tests/manager/api/odl/test_api.py +++ b/tests/manager/api/odl/test_api.py @@ -14,8 +14,6 @@ from palace.manager.api.circulation import ( DirectFulfillment, FetchFulfillment, - HoldInfo, - LoanInfo, RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( @@ -36,19 +34,17 @@ ) from palace.manager.api.odl.api import OPDS2WithODLApi from palace.manager.api.odl.constants import FEEDBOOKS_AUDIO -from palace.manager.api.odl.settings import OPDS2AuthType, OPDS2WithODLLibrarySettings +from palace.manager.api.odl.settings import OPDS2AuthType from palace.manager.opds.lcp.status import LoanStatus from palace.manager.sqlalchemy.constants import MediaTypes from palace.manager.sqlalchemy.model.licensing import ( DeliveryMechanism, - LicensePool, LicensePoolDeliveryMechanism, RightsStatus, ) from palace.manager.sqlalchemy.model.patron import Hold, Loan 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 datetime_utc, utc_now from palace.manager.util.http import BadResponseException, RemoteIntegrationException from tests.fixtures.database import DatabaseTransactionFixture @@ -65,10 +61,11 @@ def test_loan_limit(self, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture): response = opds2_with_odl_api_fixture.checkout( patron=opds2_with_odl_api_fixture.patron, pool=opds2_with_odl_api_fixture.work.active_license_pool(), + create_loan=True, ) # Did the loan take place correctly? assert ( - response[0].identifier + response.identifier == opds2_with_odl_api_fixture.work.presentation_edition.primary_identifier.identifier ) @@ -104,9 +101,7 @@ def test_hold_unlimited_access( pool.unlimited_access = unlimited_access with pytest.raises(HoldOnUnlimitedAccess): - opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, "pin", pool, "" - ) + opds2_with_odl_api_fixture.place_hold(pool=pool) def test_hold_limit( self, @@ -119,17 +114,20 @@ def test_hold_limit( # First checkout with patron1, then place a hold with the test patron pool = opds2_with_odl_api_fixture.work.active_license_pool() assert pool is not None - response = opds2_with_odl_api_fixture.checkout(patron=patron1, pool=pool) + loan_info = opds2_with_odl_api_fixture.checkout( + patron=patron1, pool=pool, create_loan=True + ) assert ( - response[0].identifier + loan_info.identifier == opds2_with_odl_api_fixture.work.presentation_edition.primary_identifier.identifier ) # Set the hold limit to zero (holds disallowed) and ensure hold fails. opds2_with_odl_api_fixture.api.hold_limit = 0 with pytest.raises(HoldsNotPermitted) as exc: - opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, "pin", pool, "" + opds2_with_odl_api_fixture.place_hold( + opds2_with_odl_api_fixture.patron, + pool, ) assert exc.value.problem_detail.title is not None assert exc.value.problem_detail.detail is not None @@ -139,17 +137,11 @@ def test_hold_limit( # Set the hold limit to 1. opds2_with_odl_api_fixture.api.hold_limit = 1 - hold_response = opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, "pin", pool, "" + hold_response = opds2_with_odl_api_fixture.place_hold( + opds2_with_odl_api_fixture.patron, pool, create_hold=True ) # Hold was successful assert hold_response.hold_position == 1 - create( - db.session, - Hold, - patron_id=opds2_with_odl_api_fixture.patron.id, - license_pool=pool, - ) # Second work should fail for the test patron due to the hold limit work2: Work = opds2_with_odl_api_fixture.create_work( @@ -161,31 +153,28 @@ def test_hold_limit( # Do the same, patron1 checkout and test patron hold pool = work2.active_license_pool() assert pool is not None - response = opds2_with_odl_api_fixture.checkout(patron=patron1, pool=pool) + response = opds2_with_odl_api_fixture.checkout( + patron=patron1, pool=pool, create_loan=True + ) assert ( - response[0].identifier + response.identifier == work2.presentation_edition.primary_identifier.identifier ) # Hold should fail with pytest.raises(PatronHoldLimitReached) as exc2: - opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, "pin", pool, "" + opds2_with_odl_api_fixture.place_hold( + opds2_with_odl_api_fixture.patron, pool ) assert exc2.value.limit == 1 # Set the hold limit to None (unlimited) and ensure hold succeeds. opds2_with_odl_api_fixture.api.hold_limit = None - hold_response = opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, "pin", pool, "" + hold_response = opds2_with_odl_api_fixture.place_hold( + opds2_with_odl_api_fixture.patron, pool, create_hold=True ) assert hold_response.hold_position == 1 - create( - db.session, - Hold, - patron_id=opds2_with_odl_api_fixture.patron.id, - license_pool=pool, - ) + # Verify that there are now two holds that our test patron has both of them. assert 2 == db.session.query(Hold).count() assert ( @@ -299,7 +288,6 @@ def test_checkin_success( # The pool's availability has increased assert 7 == opds2_with_odl_api_fixture.pool.licenses_available - assert 0 == db.session.query(Loan).count() # The license on the pool has also been updated assert 7 == opds2_with_odl_api_fixture.license.checkouts_available @@ -334,8 +322,6 @@ def test_checkin_success_with_holds_queue( assert 0 == opds2_with_odl_api_fixture.pool.licenses_available assert 1 == opds2_with_odl_api_fixture.pool.licenses_reserved assert 1 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - assert 0 == db.session.query(Loan).count() - assert 0 == hold.position def test_checkin_not_checked_out( self, @@ -444,25 +430,13 @@ def test_checkout_success( # A patron checks out the book successfully. loan_url = db.fresh_str() - loan, _ = opds2_with_odl_api_fixture.checkout(loan_url=loan_url) + loan = opds2_with_odl_api_fixture.checkout(loan_url=loan_url) assert opds2_with_odl_api_fixture.collection == loan.collection(db.session) assert opds2_with_odl_api_fixture.pool.identifier.type == loan.identifier_type assert opds2_with_odl_api_fixture.pool.identifier.identifier == loan.identifier - assert loan.start_date is not None - assert loan.start_date > utc_now() - datetime.timedelta(minutes=1) - assert loan.start_date < utc_now() + datetime.timedelta(minutes=1) assert datetime_utc(3017, 10, 21, 11, 12, 13) == loan.end_date assert loan_url == loan.external_identifier - assert 1 == db.session.query(Loan).count() - - # Now the patron has a loan in the database that matches the LoanInfo - # returned by the API. - db_loan = db.session.query(Loan).one() - assert opds2_with_odl_api_fixture.pool == db_loan.license_pool - assert opds2_with_odl_api_fixture.license == db_loan.license - assert loan.start_date == db_loan.start - assert loan.end_date == db_loan.end # The pool's availability and the license's remaining checkouts have decreased. assert 5 == opds2_with_odl_api_fixture.pool.licenses_available @@ -544,6 +518,7 @@ def test_checkout_success_with_hold( opds2_with_odl_api_fixture.pool.on_hold_to( opds2_with_odl_api_fixture.patron, start=utc_now() - datetime.timedelta(days=1), + end=utc_now() + datetime.timedelta(days=1), position=0, ) opds2_with_odl_api_fixture.setup_license(concurrency=1, available=1, left=5) @@ -554,23 +529,14 @@ def test_checkout_success_with_hold( # The patron checks out the book. loan_url = db.fresh_str() - loan, _ = opds2_with_odl_api_fixture.checkout(loan_url=loan_url) + loan = opds2_with_odl_api_fixture.checkout(loan_url=loan_url) # The patron gets a loan successfully. assert opds2_with_odl_api_fixture.collection == loan.collection(db.session) assert opds2_with_odl_api_fixture.pool.identifier.type == loan.identifier_type assert opds2_with_odl_api_fixture.pool.identifier.identifier == loan.identifier - assert loan.start_date is not None - assert loan.start_date > utc_now() - datetime.timedelta(minutes=1) - assert loan.start_date < utc_now() + datetime.timedelta(minutes=1) assert datetime_utc(3017, 10, 21, 11, 12, 13) == loan.end_date assert loan_url == loan.external_identifier - assert 1 == db.session.query(Loan).count() - - db_loan = db.session.query(Loan).one() - assert opds2_with_odl_api_fixture.pool == db_loan.license_pool - assert opds2_with_odl_api_fixture.license == db_loan.license - assert 4 == opds2_with_odl_api_fixture.license.checkouts_left # The book is no longer reserved for the patron, and the hold has been deleted. assert 0 == opds2_with_odl_api_fixture.pool.licenses_reserved @@ -614,10 +580,11 @@ def test_checkout_already_checked_out( opds2_with_odl_api_fixture.setup_license(concurrency=2, available=1) # Checkout succeeds the first time - opds2_with_odl_api_fixture.checkout() + opds2_with_odl_api_fixture.checkout(create_loan=True) # But raises an exception the second time - pytest.raises(AlreadyCheckedOut, opds2_with_odl_api_fixture.checkout) + with pytest.raises(AlreadyCheckedOut): + opds2_with_odl_api_fixture.checkout() assert 1 == db.session.query(Loan).count() @@ -886,7 +853,7 @@ def test_fulfill_success( ) -> None: # Fulfill a loan in a way that gives access to a license file. opds2_with_odl_api_fixture.setup_license(concurrency=1, available=1) - opds2_with_odl_api_fixture.checkout() + opds2_with_odl_api_fixture.checkout(create_loan=True) lpdm = MagicMock(spec=LicensePoolDeliveryMechanism) lpdm.delivery_mechanism = MagicMock(spec=DeliveryMechanism) @@ -1034,7 +1001,7 @@ def test_fulfill_cannot_fulfill( updated_availability: bool, ) -> None: opds2_with_odl_api_fixture.setup_license(concurrency=7, available=7) - opds2_with_odl_api_fixture.checkout() + opds2_with_odl_api_fixture.checkout(create_loan=True) assert 1 == db.session.query(Loan).count() assert 6 == opds2_with_odl_api_fixture.pool.licenses_available @@ -1057,519 +1024,36 @@ def test_fulfill_cannot_fulfill( assert 7 == opds2_with_odl_api_fixture.pool.licenses_available assert 0 == db.session.query(Loan).count() - def _holdinfo_from_hold(self, hold: Hold) -> HoldInfo: - pool: LicensePool = hold.license_pool - return HoldInfo.from_license_pool( - pool, - start_date=hold.start, - end_date=hold.end, - hold_position=hold.position, - ) - - def test_count_holds_before( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - now = utc_now() - yesterday = now - datetime.timedelta(days=1) - tomorrow = now + datetime.timedelta(days=1) - last_week = now - datetime.timedelta(weeks=1) - - hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - opds2_with_odl_api_fixture.patron, start=now - ) - - info = self._holdinfo_from_hold(hold) - assert 0 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - # A previous hold. - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - assert 1 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - # Expired holds don't count. - opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=last_week, end=yesterday, position=0 - ) - assert 1 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - # Later holds don't count. - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=tomorrow) - assert 1 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - # Holds on another pool don't count. - other_pool = db.licensepool(None) - other_pool.on_hold_to(opds2_with_odl_api_fixture.patron, start=yesterday) - assert 1 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - for i in range(3): - opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=yesterday, end=tomorrow, position=1 - ) - assert 4 == opds2_with_odl_api_fixture.api._count_holds_before( - info, hold.license_pool - ) - - def test_update_hold_end_date( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - now = utc_now() - tomorrow = now + datetime.timedelta(days=1) - yesterday = now - datetime.timedelta(days=1) - next_week = now + datetime.timedelta(days=7) - last_week = now - datetime.timedelta(days=7) - - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - opds2_with_odl_api_fixture.pool.licenses_reserved = 1 - - hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - opds2_with_odl_api_fixture.patron, start=now, position=0 - ) - info = self._holdinfo_from_hold(hold) - library = hold.patron.library - - # Set the reservation period and loan period. - db.integration_library_configuration( - opds2_with_odl_api_fixture.collection.integration_configuration, - library=library, - settings=OPDS2WithODLLibrarySettings( - default_reservation_period=3, ebook_loan_duration=6 - ), - ) - - # A hold that's already reserved and has an end date doesn't change. - info.end_date = tomorrow - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert tomorrow == info.end_date - info.end_date = yesterday - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert yesterday == info.end_date - - # Updating a hold that's reserved but doesn't have an end date starts the - # reservation period. - info.end_date = None - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert info.end_date is not None - assert info.end_date < next_week # type: ignore[unreachable] - assert info.end_date > now - - # Updating a hold that has an end date but just became reserved starts - # the reservation period. - info.end_date = yesterday - info.hold_position = 1 - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert info.end_date < next_week - assert info.end_date > now - - # When there's a holds queue, the end date is the maximum time it could take for - # a license to become available. - - # One copy, one loan, hold position 1. - # The hold will be available as soon as the loan expires. - opds2_with_odl_api_fixture.pool.licenses_available = 0 - opds2_with_odl_api_fixture.pool.licenses_reserved = 0 - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - loan, ignore = opds2_with_odl_api_fixture.license.loan_to( - db.patron(), end=tomorrow - ) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert tomorrow == info.end_date - - # One copy, one loan, hold position 2. - # The hold will be available after the loan expires + 1 cycle. - first_hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=last_week - ) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert tomorrow + datetime.timedelta(days=9) == info.end_date - - # Two copies, one loan, one reserved hold, hold position 2. - # The hold will be available after the loan expires. - opds2_with_odl_api_fixture.pool.licenses_reserved = 1 - opds2_with_odl_api_fixture.pool.licenses_owned = 2 - opds2_with_odl_api_fixture.license.checkouts_available = 2 - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert tomorrow == info.end_date - - # Two copies, one loan, one reserved hold, hold position 3. - # The hold will be available after the reserved hold is checked out - # at the latest possible time and that loan expires. - second_hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=yesterday - ) - first_hold.end = next_week - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=6) == info.end_date - - # One copy, no loans, one reserved hold, hold position 3. - # The hold will be available after the reserved hold is checked out - # at the latest possible time and that loan expires + 1 cycle. - db.session.delete(loan) - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=15) == info.end_date - - # One copy, no loans, one reserved hold, hold position 2. - # The hold will be available after the reserved hold is checked out - # at the latest possible time and that loan expires. - db.session.delete(second_hold) - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=6) == info.end_date - - db.session.delete(first_hold) - - # Ten copies, seven loans, three reserved holds, hold position 9. - # The hold will be available after the sixth loan expires. - opds2_with_odl_api_fixture.pool.licenses_owned = 10 - for i in range(5): - opds2_with_odl_api_fixture.pool.loan_to(db.patron(), end=next_week) - opds2_with_odl_api_fixture.pool.loan_to( - db.patron(), end=next_week + datetime.timedelta(days=1) - ) - opds2_with_odl_api_fixture.pool.loan_to( - db.patron(), end=next_week + datetime.timedelta(days=2) - ) - opds2_with_odl_api_fixture.pool.licenses_reserved = 3 - for i in range(3): - opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), - start=last_week + datetime.timedelta(days=i), - end=next_week + datetime.timedelta(days=i), - position=0, - ) - for i in range(5): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=1) == info.end_date - - # Ten copies, seven loans, three reserved holds, hold position 12. - # The hold will be available after the second reserved hold is checked - # out and that loan expires. - for i in range(3): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=7) == info.end_date - - # Ten copies, seven loans, three reserved holds, hold position 29. - # The hold will be available after the sixth loan expires + 2 cycles. - for i in range(17): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=19) == info.end_date - - # Ten copies, seven loans, three reserved holds, hold position 32. - # The hold will be available after the second reserved hold is checked - # out and that loan expires + 2 cycles. - for i in range(3): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_end_date( - info, hold.license_pool, library=library - ) - assert next_week + datetime.timedelta(days=25) == info.end_date - - def test_update_hold_position( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - now = utc_now() - yesterday = now - datetime.timedelta(days=1) - tomorrow = now + datetime.timedelta(days=1) - - hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - opds2_with_odl_api_fixture.patron, start=now - ) - info = self._holdinfo_from_hold(hold) - - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - - # When there are no other holds and no licenses reserved, hold position is 1. - loan, _ = opds2_with_odl_api_fixture.license.loan_to(db.patron()) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 1 == info.hold_position - - # When a license is reserved, position is 0. - db.session.delete(loan) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 0 == info.hold_position - - # If another hold has the reserved licenses, position is 2. - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 2 == info.hold_position - - # If another license is reserved, position goes back to 0. - opds2_with_odl_api_fixture.pool.licenses_owned = 2 - opds2_with_odl_api_fixture.license.checkouts_available = 2 - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 0 == info.hold_position - - # If there's an earlier hold but it expired, it doesn't - # affect the position. - opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=yesterday, end=yesterday, position=0 - ) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 0 == info.hold_position - - # Hold position is after all earlier non-expired holds... - for i in range(3): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=yesterday) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 5 == info.hold_position - - # and before any later holds. - for i in range(2): - opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), start=tomorrow) - opds2_with_odl_api_fixture.api._update_hold_position(info, hold.license_pool) - assert 5 == info.hold_position - - def test_update_hold_data( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - hold, is_new = opds2_with_odl_api_fixture.pool.on_hold_to( - opds2_with_odl_api_fixture.patron, - utc_now(), - utc_now() + datetime.timedelta(days=100), - 9, - ) - opds2_with_odl_api_fixture.api._update_hold_data(hold) - assert hold.position == 0 - assert hold.end.date() == (hold.start + datetime.timedelta(days=3)).date() - - def test_update_hold_queue( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - licenses = [opds2_with_odl_api_fixture.license] - - # If there's no holds queue when we try to update the queue, it - # will remove a reserved license and make it available instead. - opds2_with_odl_api_fixture.pool.licenses_owned = 1 - opds2_with_odl_api_fixture.pool.licenses_available = 0 - opds2_with_odl_api_fixture.pool.licenses_reserved = 1 - opds2_with_odl_api_fixture.pool.patrons_in_hold_queue = 0 - last_update = utc_now() - datetime.timedelta(minutes=5) - opds2_with_odl_api_fixture.work.last_update_time = last_update - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - assert 1 == opds2_with_odl_api_fixture.pool.licenses_available - assert 0 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 0 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - # The work's last update time is changed so it will be moved up in the crawlable OPDS feed. - assert opds2_with_odl_api_fixture.work.last_update_time > last_update - - # If there are holds, a license will get reserved for the next hold - # and its end date will be set. - hold, _ = opds2_with_odl_api_fixture.pool.on_hold_to( - opds2_with_odl_api_fixture.patron, start=utc_now(), position=1 - ) - later_hold, _ = opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), start=utc_now() + datetime.timedelta(days=1), position=2 - ) - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - - # The pool's licenses were updated. - assert 0 == opds2_with_odl_api_fixture.pool.licenses_available - assert 1 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 2 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - - # And the first hold changed. - assert 0 == hold.position - assert hold.end - utc_now() - datetime.timedelta(days=3) < datetime.timedelta( - hours=1 - ) - - # The later hold is the same. - assert 2 == later_hold.position - - # Now there's a reserved hold. If we add another license, it's reserved and, - # the later hold is also updated. - l = db.license( - opds2_with_odl_api_fixture.pool, terms_concurrency=1, checkouts_available=1 - ) - licenses.append(l) - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - - assert 0 == opds2_with_odl_api_fixture.pool.licenses_available - assert 2 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 2 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - assert 0 == later_hold.position - assert later_hold.end - utc_now() - datetime.timedelta( - days=3 - ) < datetime.timedelta(hours=1) - - # Now there are no more holds. If we add another license, - # it ends up being available. - l = db.license( - opds2_with_odl_api_fixture.pool, terms_concurrency=1, checkouts_available=1 - ) - licenses.append(l) - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - assert 1 == opds2_with_odl_api_fixture.pool.licenses_available - assert 2 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 2 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - - # License pool is updated when the holds are removed. - db.session.delete(hold) - db.session.delete(later_hold) - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - assert 3 == opds2_with_odl_api_fixture.pool.licenses_available - assert 0 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 0 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - - # We can also make multiple licenses reserved at once. - loans = [] - holds = [] - for i in range(3): - p = db.patron() - loan, _ = opds2_with_odl_api_fixture.checkout(patron=p) - loans.append((loan, p)) - assert 0 == opds2_with_odl_api_fixture.pool.licenses_available - assert 0 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 0 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - - l = db.license( - opds2_with_odl_api_fixture.pool, terms_concurrency=2, checkouts_available=2 - ) - licenses.append(l) - for i in range(3): - hold, ignore = opds2_with_odl_api_fixture.pool.on_hold_to( - db.patron(), - start=utc_now() - datetime.timedelta(days=3 - i), - position=i + 1, - ) - holds.append(hold) - - opds2_with_odl_api_fixture.api.update_licensepool( - opds2_with_odl_api_fixture.pool - ) - assert 2 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 0 == opds2_with_odl_api_fixture.pool.licenses_available - assert 3 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - assert 0 == holds[0].position - assert 0 == holds[1].position - assert 3 == holds[2].position - assert holds[0].end - utc_now() - datetime.timedelta( - days=3 - ) < datetime.timedelta(hours=1) - assert holds[1].end - utc_now() - datetime.timedelta( - days=3 - ) < datetime.timedelta(hours=1) - - # If there are more licenses that change than holds, some of them become available. - for i in range(2): - _, p = loans[i] - opds2_with_odl_api_fixture.checkin(patron=p) - assert 3 == opds2_with_odl_api_fixture.pool.licenses_reserved - assert 1 == opds2_with_odl_api_fixture.pool.licenses_available - assert 3 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue - for hold in holds: - assert 0 == hold.position - assert hold.end - utc_now() - datetime.timedelta( - days=3 - ) < datetime.timedelta(hours=1) - + @freeze_time() def test_place_hold_success( self, db: DatabaseTransactionFixture, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, ) -> None: - loan, _ = opds2_with_odl_api_fixture.checkout(patron=db.patron()) - - hold = opds2_with_odl_api_fixture.api.place_hold( - opds2_with_odl_api_fixture.patron, - "pin", - opds2_with_odl_api_fixture.pool, - "notifications@librarysimplified.org", - ) + loan = opds2_with_odl_api_fixture.checkout(patron=db.patron(), create_loan=True) + hold = opds2_with_odl_api_fixture.place_hold() assert 1 == opds2_with_odl_api_fixture.pool.patrons_in_hold_queue assert opds2_with_odl_api_fixture.collection == hold.collection(db.session) assert opds2_with_odl_api_fixture.pool.identifier.type == hold.identifier_type assert opds2_with_odl_api_fixture.pool.identifier.identifier == hold.identifier assert hold.start_date is not None - assert hold.start_date > utc_now() - datetime.timedelta(minutes=1) - assert hold.start_date < utc_now() + datetime.timedelta(minutes=1) - assert loan.end_date == hold.end_date + assert hold.start_date == utc_now() assert 1 == hold.hold_position def test_place_hold_already_on_hold( self, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture ) -> None: opds2_with_odl_api_fixture.setup_license(concurrency=1, available=0) - opds2_with_odl_api_fixture.pool.on_hold_to(opds2_with_odl_api_fixture.patron) - pytest.raises( - AlreadyOnHold, - opds2_with_odl_api_fixture.api.place_hold, - opds2_with_odl_api_fixture.patron, - "pin", - opds2_with_odl_api_fixture.pool, - "notifications@librarysimplified.org", - ) + opds2_with_odl_api_fixture.place_hold(create_hold=True) + with pytest.raises(AlreadyOnHold): + opds2_with_odl_api_fixture.place_hold() def test_place_hold_currently_available( self, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture ) -> None: - pytest.raises( - CurrentlyAvailable, - opds2_with_odl_api_fixture.api.place_hold, - opds2_with_odl_api_fixture.patron, - "pin", - opds2_with_odl_api_fixture.pool, - "notifications@librarysimplified.org", - ) + with pytest.raises(CurrentlyAvailable): + opds2_with_odl_api_fixture.place_hold() def test_release_hold_success( self, @@ -1630,141 +1114,6 @@ def test_release_hold_not_on_hold( opds2_with_odl_api_fixture.pool, ) - def test_patron_activity_loan( - self, - db: DatabaseTransactionFixture, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - ) -> None: - # No loans yet. - assert [] == opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - - # One loan. - _, loan = opds2_with_odl_api_fixture.checkout() - - [l1] = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert isinstance(l1, LoanInfo) - assert opds2_with_odl_api_fixture.collection == l1.collection(db.session) - assert opds2_with_odl_api_fixture.pool.identifier.type == l1.identifier_type - assert opds2_with_odl_api_fixture.pool.identifier.identifier == l1.identifier - assert loan.start == l1.start_date - assert loan.end == l1.end_date - assert loan.external_identifier == l1.external_identifier - - # Two loans. - pool2 = db.licensepool(None, collection=opds2_with_odl_api_fixture.collection) - license2 = db.license(pool2, terms_concurrency=1, checkouts_available=1) - _, loan2 = opds2_with_odl_api_fixture.checkout(pool=pool2) - - def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: - if activity.start_date is None: - raise TypeError("start_date is None") - return activity.start_date - - activity = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert 2 == len(activity) - [l1, l2] = sorted(activity, key=activity_sort_key) - assert isinstance(l1, LoanInfo) - assert isinstance(l2, LoanInfo) - - assert opds2_with_odl_api_fixture.collection == l1.collection(db.session) - assert opds2_with_odl_api_fixture.pool.identifier.type == l1.identifier_type - assert opds2_with_odl_api_fixture.pool.identifier.identifier == l1.identifier - assert loan.start == l1.start_date - assert loan.end == l1.end_date - assert loan.external_identifier == l1.external_identifier - - assert opds2_with_odl_api_fixture.collection == l2.collection(db.session) - assert pool2.identifier.type == l2.identifier_type - assert pool2.identifier.identifier == l2.identifier - assert loan2.start == l2.start_date - assert loan2.end == l2.end_date - assert loan2.external_identifier == l2.external_identifier - - # If a loan is expired already, it's left out. - loan2.end = utc_now() - datetime.timedelta(days=2) - [l1] = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert isinstance(l1, LoanInfo) - assert opds2_with_odl_api_fixture.pool.identifier.identifier == l1.identifier - opds2_with_odl_api_fixture.checkin(pool=pool2) - - # Open access loans are included. - oa_work = db.work( - with_open_access_download=True, - collection=opds2_with_odl_api_fixture.collection, - ) - pool3 = oa_work.license_pools[0] - loan3, ignore = pool3.loan_to(opds2_with_odl_api_fixture.patron) - - activity = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert 2 == len(activity) - [l1, l2] = sorted(activity, key=activity_sort_key) - assert isinstance(l1, LoanInfo) - assert isinstance(l2, LoanInfo) - - assert opds2_with_odl_api_fixture.collection == l1.collection(db.session) - assert opds2_with_odl_api_fixture.pool.identifier.type == l1.identifier_type - assert opds2_with_odl_api_fixture.pool.identifier.identifier == l1.identifier - assert loan.start == l1.start_date - assert loan.end == l1.end_date - assert loan.external_identifier == l1.external_identifier - - assert opds2_with_odl_api_fixture.collection == l2.collection(db.session) - assert pool3.identifier.type == l2.identifier_type - assert pool3.identifier.identifier == l2.identifier - assert loan3.start == l2.start_date - assert loan3.end == l2.end_date - assert loan3.external_identifier == l2.external_identifier - - # remove the open access loan - db.session.delete(loan3) - - # One hold. - other_patron = db.patron() - opds2_with_odl_api_fixture.checkout(patron=other_patron, pool=pool2) - hold, _ = pool2.on_hold_to(opds2_with_odl_api_fixture.patron) - hold.start = utc_now() - datetime.timedelta(days=2) - hold.end = hold.start + datetime.timedelta(days=3) - hold.position = 3 - activity = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert 2 == len(activity) - [h1, l1] = sorted(activity, key=activity_sort_key) - - assert isinstance(h1, HoldInfo) - - assert opds2_with_odl_api_fixture.collection == h1.collection(db.session) - assert pool2.identifier.type == h1.identifier_type - assert pool2.identifier.identifier == h1.identifier - assert hold.start == h1.start_date - assert hold.end == h1.end_date - # Hold position was updated. - assert 1 == h1.hold_position - assert 1 == hold.position - - # If the hold is expired, it's deleted right away and the license - # is made available again. - opds2_with_odl_api_fixture.checkin(patron=other_patron, pool=pool2) - hold.end = utc_now() - datetime.timedelta(days=1) - hold.position = 0 - activity = opds2_with_odl_api_fixture.api.patron_activity( - opds2_with_odl_api_fixture.patron, "pin" - ) - assert 1 == len(activity) - assert 0 == db.session.query(Hold).count() - assert 1 == pool2.licenses_available - assert 0 == pool2.licenses_reserved - def test_update_loan_still_active( self, db: DatabaseTransactionFixture, @@ -1788,7 +1137,7 @@ def test_update_loan_removes_loan( opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, ) -> None: opds2_with_odl_api_fixture.setup_license(concurrency=7, available=7) - _, loan = opds2_with_odl_api_fixture.checkout() + loan = opds2_with_odl_api_fixture.checkout() assert 6 == opds2_with_odl_api_fixture.pool.licenses_available assert 1 == db.session.query(Loan).count() @@ -1806,7 +1155,7 @@ def test_update_loan_removes_loan_with_hold_queue( db: DatabaseTransactionFixture, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, ) -> None: - _, loan = opds2_with_odl_api_fixture.checkout() + loan = opds2_with_odl_api_fixture.checkout() hold, _ = opds2_with_odl_api_fixture.pool.on_hold_to(db.patron(), position=1) opds2_with_odl_api_fixture.pool.update_availability_from_licenses() diff --git a/tests/manager/api/odl/test_reaper.py b/tests/manager/api/odl/test_reaper.py deleted file mode 100644 index 865ae6ad9..000000000 --- a/tests/manager/api/odl/test_reaper.py +++ /dev/null @@ -1,59 +0,0 @@ -from __future__ import annotations - -import datetime - -from palace.manager.api.odl.reaper import OPDS2WithODLHoldReaper -from palace.manager.sqlalchemy.model.patron import Hold -from palace.manager.util.datetime_helpers import utc_now -from tests.fixtures.database import DatabaseTransactionFixture -from tests.fixtures.odl import OPDS2WithODLApiFixture - - -class TestOPDS2WithODLHoldReaper: - def test_run_once( - self, - opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, - db: DatabaseTransactionFixture, - ): - collection = opds2_with_odl_api_fixture.collection - work = opds2_with_odl_api_fixture.work - license = opds2_with_odl_api_fixture.setup_license( - work, concurrency=3, available=3 - ) - api = opds2_with_odl_api_fixture.api - pool = license.license_pool - - reaper = OPDS2WithODLHoldReaper(db.session, collection, api=api) - - now = utc_now() - yesterday = now - datetime.timedelta(days=1) - - 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/celery/tasks/test_opds.py b/tests/manager/celery/tasks/test_opds.py new file mode 100644 index 000000000..e0caad5c7 --- /dev/null +++ b/tests/manager/celery/tasks/test_opds.py @@ -0,0 +1,323 @@ +from datetime import datetime, timedelta +from typing import cast +from unittest.mock import MagicMock + +import pytest +from freezegun import freeze_time +from sqlalchemy import func, select + +from palace.manager.api.odl.api import OPDS2WithODLApi +from palace.manager.api.overdrive import OverdriveAPI +from palace.manager.celery.tasks.opds import ( + licensepool_ids_with_holds, + recalculate_hold_queue_collection, + recalculate_holds_for_licensepool, + remove_expired_holds, + remove_expired_holds_for_collection, +) +from palace.manager.sqlalchemy.model.collection import Collection +from palace.manager.sqlalchemy.model.licensing import License, LicensePool +from palace.manager.sqlalchemy.model.patron import Hold, Patron +from palace.manager.sqlalchemy.util import create +from palace.manager.util.datetime_helpers import utc_now +from tests.fixtures.celery import CeleryFixture +from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.redis import RedisFixture + + +class OpdsTaskFixture: + def __init__(self, db: DatabaseTransactionFixture): + self.db = db + + self.two_weeks_ago = utc_now() - timedelta(weeks=2) + self.yesterday = utc_now() - timedelta(days=1) + self.tomorrow = utc_now() + timedelta(days=1) + + def hold( + self, + collection: Collection, + *, + start: datetime, + end: datetime, + position: int, + pool: LicensePool | None = None, + patron: Patron | None = None, + ) -> Hold: + if patron is None: + patron = self.db.patron() + if pool is None: + _, pool = self.db.edition(collection=collection, with_license_pool=True) + hold, _ = create( + self.db.session, + Hold, + patron=patron, + license_pool=pool, + start=start, + end=end, + position=position, + ) + return hold + + def holds( + self, collection: Collection, pool: LicensePool | None = None + ) -> tuple[set[int], set[int]]: + expired_holds = { + self.hold( + collection, + start=self.two_weeks_ago, + end=self.yesterday, + position=0, + pool=pool, + ).id + for idx in range(10) + } + ready_non_expired_holds = { + self.hold( + collection, + start=self.two_weeks_ago + timedelta(days=idx), + end=self.tomorrow, + position=0, + pool=pool, + ).id + for idx in range(10) + } + not_ready_non_expired_holds = { + self.hold( + collection, + start=self.yesterday, + end=self.tomorrow, + position=idx, + pool=pool, + ).id + for idx in range(10) + } + + return cast(set[int], expired_holds), cast( + set[int], ready_non_expired_holds | not_ready_non_expired_holds + ) + + def pool_with_licenses( + self, collection: Collection, num_licenses: int = 2, available: bool = False + ) -> tuple[LicensePool, list[License]]: + edition = self.db.edition(collection=collection) + pool = self.db.licensepool( + edition, open_access=False, unlimited_access=False, collection=collection + ) + licenses = [ + self.db.license( + pool=pool, + checkouts_available=idx + 1 if available else 0, + terms_concurrency=idx + 1, + ) + for idx in range(num_licenses) + ] + self.holds(collection, pool=pool) + return pool, licenses + + +@pytest.fixture +def opds_task_fixture(db: DatabaseTransactionFixture) -> OpdsTaskFixture: + return OpdsTaskFixture(db) + + +def _hold_sort_key(hold: Hold) -> int: + position = hold.position + assert position is not None + return position + + +def test_remove_expired_holds_for_collection( + db: DatabaseTransactionFixture, opds_task_fixture: OpdsTaskFixture +): + collection = db.collection(protocol=OPDS2WithODLApi) + decoy_collection = db.collection(protocol=OverdriveAPI) + + expired_holds, non_expired_holds = opds_task_fixture.holds(collection) + decoy_expired_holds, decoy_non_expired_holds = opds_task_fixture.holds( + decoy_collection + ) + + pools_before = db.session.scalars( + select(func.count()).select_from(LicensePool) + ).one() + + # Remove the expired holds + assert collection.id is not None + removed = remove_expired_holds_for_collection(db.session, collection.id) + + # Assert that the correct holds were removed + current_holds = {h.id for h in db.session.scalars(select(Hold))} + + assert expired_holds.isdisjoint(current_holds) + assert non_expired_holds.issubset(current_holds) + assert decoy_non_expired_holds.issubset(current_holds) + assert decoy_expired_holds.issubset(current_holds) + + assert removed == 10 + + pools_after = db.session.scalars( + select(func.count()).select_from(LicensePool) + ).one() + + # Make sure the license pools for those holds were not deleted + assert pools_before == pools_after + + +def test_licensepools_with_holds( + db: DatabaseTransactionFixture, opds_task_fixture: OpdsTaskFixture +): + collection1 = db.collection(protocol=OPDS2WithODLApi) + collection2 = db.collection(protocol=OPDS2WithODLApi) + + # create some holds on Collection2 to ensure that the query is correct + opds_task_fixture.holds(collection2) + + # Create some license pools + pools = [ + db.edition(collection=collection1, with_license_pool=True)[1] + for idx in range(10) + ] + + # Create holds for some of the license pools + for pool in pools[5:]: + opds_task_fixture.holds(collection1, pool=pool) + + queried_pools: list[int] = [] + iterations = 0 + + # Query the license pools with holds + assert collection1.id is not None + while license_pools := licensepool_ids_with_holds( + db.session, + collection1.id, + batch_size=2, + after_id=queried_pools[-1] if queried_pools else None, + ): + queried_pools.extend(license_pools) + iterations += 1 + + assert len(queried_pools) == 5 + assert iterations == 3 + assert queried_pools == [p.id for p in pools[5:]] + + +@freeze_time() +def test_recalculate_holds_for_licensepool( + db: DatabaseTransactionFixture, opds_task_fixture: OpdsTaskFixture +): + collection = db.collection(protocol=OPDS2WithODLApi) + pool, [license1, license2] = opds_task_fixture.pool_with_licenses(collection) + + # Recalculate the hold queue + assert pool.id is not None + recalculate_holds_for_licensepool( + db.session, MagicMock(), pool.id, timedelta(days=5) + ) + + current_holds = pool.get_active_holds() + assert len(current_holds) == 20 + assert current_holds[0].position == 1 + assert current_holds[-1].position == len(current_holds) + + # Make a couple of copies available and recalculate the hold queue + license1.checkouts_available = 1 + license2.checkouts_available = 2 + reservation_time = timedelta(days=5) + recalculate_holds_for_licensepool( + db.session, MagicMock(), pool.id, reservation_time + ) + + assert pool.licenses_reserved == 3 + assert pool.licenses_available == 0 + current_holds = pool.get_active_holds() + assert len(current_holds) == 20 + + reserved_holds = [h for h in current_holds if h.position == 0] + waiting_holds = [h for h in current_holds if h.position and h.position > 0] + + assert len(reserved_holds) == 3 + assert len(waiting_holds) == 17 + + assert all(h.end == utc_now() + reservation_time for h in reserved_holds) + assert all( + h.start and waiting_holds[0].start and h.start < waiting_holds[0].start + for h in reserved_holds + ) + + waiting_holds.sort(key=_hold_sort_key) + for idx, hold in enumerate(waiting_holds): + assert hold.position == idx + 1 + assert hold.end is None + + expected_start = ( + waiting_holds[idx - 1].start if idx else reserved_holds[-1].start + ) + assert hold.start and expected_start and hold.start >= expected_start + + +def test_remove_expired_holds( + celery_fixture: CeleryFixture, + db: DatabaseTransactionFixture, + opds_task_fixture: OpdsTaskFixture, +): + collection1 = db.collection(protocol=OPDS2WithODLApi) + collection2 = db.collection(protocol=OPDS2WithODLApi) + decoy_collection = db.collection(protocol=OverdriveAPI) + + expired_holds1, non_expired_holds1 = opds_task_fixture.holds(collection1) + expired_holds2, non_expired_holds2 = opds_task_fixture.holds(collection2) + decoy_expired_holds, decoy_non_expired_holds = opds_task_fixture.holds( + decoy_collection + ) + + # Remove the expired holds + remove_expired_holds.delay().wait() + + current_holds = {h.id for h in db.session.scalars(select(Hold))} + assert expired_holds1.isdisjoint(current_holds) + assert expired_holds2.isdisjoint(current_holds) + + assert decoy_non_expired_holds.issubset(current_holds) + assert decoy_expired_holds.issubset(current_holds) + assert non_expired_holds1.issubset(current_holds) + assert non_expired_holds2.issubset(current_holds) + + +def test_recalculate_hold_queue_collection( + celery_fixture: CeleryFixture, + redis_fixture: RedisFixture, + db: DatabaseTransactionFixture, + opds_task_fixture: OpdsTaskFixture, +): + collection = db.collection(protocol=OPDS2WithODLApi) + pools = [ + opds_task_fixture.pool_with_licenses(collection, num_licenses=1, available=True) + for idx in range(15) + ] + + # Do recalculation + recalculate_hold_queue_collection.delay(collection.id, batch_size=2).wait() + + for pool, [license] in pools: + current_holds = pool.get_active_holds() + assert len(current_holds) == 20 + [reserved_hold] = [h for h in current_holds if h.position == 0] + waiting_holds = [h for h in current_holds if h.position and h.position > 0] + + assert len(waiting_holds) == 19 + + assert reserved_hold.end is not None + assert reserved_hold.start is not None + assert waiting_holds[0].start is not None + assert reserved_hold.start < waiting_holds[0].start + + waiting_holds.sort(key=_hold_sort_key) + for idx, hold in enumerate(waiting_holds): + assert hold.position == idx + 1 + assert hold.end is None + assert hold.start is not None + expected_start = ( + waiting_holds[idx - 1].start if idx else reserved_hold.start + ) + assert expected_start is not None + assert hold.start >= expected_start