diff --git a/alembic/versions/20241015_1938277e993f_remove_hold_external_identifier.py b/alembic/versions/20241015_1938277e993f_remove_hold_external_identifier.py new file mode 100644 index 000000000..06a05c0aa --- /dev/null +++ b/alembic/versions/20241015_1938277e993f_remove_hold_external_identifier.py @@ -0,0 +1,33 @@ +"""Remove Hold.external_identifier + +Revision ID: 1938277e993f +Revises: 87901a6323d6 +Create Date: 2024-10-15 19:47:55.697280+00:00 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "1938277e993f" +down_revision = "87901a6323d6" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_constraint("holds_external_identifier_key", "holds", type_="unique") + op.drop_column("holds", "external_identifier") + + +def downgrade() -> None: + op.add_column( + "holds", + sa.Column( + "external_identifier", sa.VARCHAR(), autoincrement=False, nullable=True + ), + ) + op.create_unique_constraint( + "holds_external_identifier_key", "holds", ["external_identifier"] + ) diff --git a/src/palace/manager/api/axis.py b/src/palace/manager/api/axis.py index 8fe987349..9a3683fcd 100644 --- a/src/palace/manager/api/axis.py +++ b/src/palace/manager/api/axis.py @@ -45,7 +45,6 @@ AlreadyCheckedOut, AlreadyOnHold, CannotFulfill, - CannotHold, CannotLoan, CurrentlyAvailable, InvalidInputException, @@ -415,9 +414,7 @@ def checkin(self, patron: Patron, pin: str, licensepool: LicensePool) -> None: patron_id = patron.authorization_identifier response = self._checkin(title_id, patron_id) try: - CheckinResponseParser(licensepool.collection).process_first( - response.content - ) + CheckinResponseParser().process_first(response.content) except etree.XMLSyntaxError: raise RemoteInitiatedServerError(response.text, self.label()) @@ -454,12 +451,8 @@ def checkout( title_id, patron_id, self.internal_format(delivery_mechanism) ) try: - loan_info = CheckoutResponseParser(licensepool.collection).process_first( - response.content - ) - if loan_info is None: - raise CannotLoan() - return loan_info + expiration_date = CheckoutResponseParser().process_first(response.content) + return LoanInfo.from_license_pool(licensepool, end_date=expiration_date) except etree.XMLSyntaxError: raise RemoteInitiatedServerError(response.text, self.label()) @@ -525,17 +518,12 @@ def place_hold( titleId=title_id, patronId=patron_id, email=hold_notification_email ) response = self.request(url, params=params) - hold_info = HoldResponseParser(licensepool.collection).process_first( - response.content + hold_position = HoldResponseParser().process_first(response.content) + hold_info = HoldInfo.from_license_pool( + licensepool, + start_date=utc_now(), + hold_position=hold_position, ) - if not hold_info: - raise CannotHold() - if not hold_info.identifier: - # The Axis 360 API doesn't return the identifier of the - # item that was placed on hold, so we have to fill it in - # based on our own knowledge. - hold_info.identifier_type = identifier.type - hold_info.identifier = identifier.identifier return hold_info def release_hold(self, patron: Patron, pin: str, licensepool: LicensePool) -> None: @@ -546,9 +534,7 @@ def release_hold(self, patron: Patron, pin: str, licensepool: LicensePool) -> No params = dict(titleId=title_id, patronId=patron_id) response = self.request(url, params=params) try: - HoldReleaseResponseParser(licensepool.collection).process_first( - response.content - ) + HoldReleaseResponseParser().process_first(response.content) except NotOnHold: # Fine, it wasn't on hold and now it's still not on hold. pass @@ -1423,14 +1409,6 @@ def _raise_exception_on_error( class XMLResponseParser(ResponseParser, Axis360Parser[T], ABC): - def __init__(self, collection: Collection): - """Constructor. - - :param collection: A Collection, in case parsing this document - results in the creation of LoanInfo or HoldInfo objects. - """ - self.collection = collection - def raise_exception_on_error( self, e: _Element, @@ -1477,47 +1455,37 @@ def process_one( return True -class CheckoutResponseParser(XMLResponseParser[LoanInfo]): +class CheckoutResponseParser(XMLResponseParser[datetime.datetime | None]): @property def xpath_expression(self) -> str: return "//axis:checkoutResult" - def process_one(self, e: _Element, namespaces: dict[str, str] | None) -> LoanInfo: - """Either turn the given document into a LoanInfo - object, or raise an appropriate exception. + def process_one( + self, e: _Element, namespaces: dict[str, str] | None + ) -> datetime.datetime | None: + """Either turn the given document into a datetime representing the + loan's expiration date, or raise an appropriate exception. """ self.raise_exception_on_error(e, namespaces) # If we get to this point it's because the checkout succeeded. expiration_date = self._xpath1(e, "//axis:expirationDate", namespaces) - fulfillment_url = self._xpath1(e, "//axis:url", namespaces) - if fulfillment_url is not None: - fulfillment_url = fulfillment_url.text if expiration_date is not None: expiration_date = expiration_date.text expiration_date = self._pd(expiration_date) - loan_start = utc_now() - loan = LoanInfo( - collection=self.collection, - data_source_name=DataSource.AXIS_360, - identifier_type=self.id_type, - identifier=None, - start_date=loan_start, - end_date=expiration_date, - ) - return loan + return expiration_date # type: ignore[no-any-return] -class HoldResponseParser(XMLResponseParser[HoldInfo]): +class HoldResponseParser(XMLResponseParser[int | None], LoggerMixin): @property def xpath_expression(self) -> str: return "//axis:addtoholdResult" - def process_one(self, e: _Element, namespaces: dict[str, str] | None) -> HoldInfo: - """Either turn the given document into a HoldInfo - object, or raise an appropriate exception. + def process_one(self, e: _Element, namespaces: dict[str, str] | None) -> int | None: + """Either turn the given document into an int representing the hold position, + or raise an appropriate exception. """ self.raise_exception_on_error(e, namespaces, {3109: AlreadyOnHold}) @@ -1529,22 +1497,10 @@ def process_one(self, e: _Element, namespaces: dict[str, str] | None) -> HoldInf try: queue_position = int(queue_position.text) except ValueError: - print("Invalid queue position: %s" % queue_position) + self.log.warning("Invalid queue position: %s" % queue_position) queue_position = None - hold_start = utc_now() - # NOTE: The caller needs to fill in Collection -- we have no idea - # what collection this is. - hold = HoldInfo( - collection=self.collection, - data_source_name=DataSource.AXIS_360, - identifier_type=self.id_type, - identifier=None, - start_date=hold_start, - end_date=None, - hold_position=queue_position, - ) - return hold + return queue_position class HoldReleaseResponseParser(XMLResponseParser[Literal[True]]): @@ -1575,11 +1531,12 @@ def __init__(self, api: Axis360API, internal_format: str | None = None) -> None: """ self.api = api self.internal_format = internal_format - if api.collection is None: + if api.collection_id is None: raise ValueError( - "Cannot use an Axis360AvailabilityResponseParser without a Collection." + "Cannot use an Axis360AvailabilityResponseParser without a collection_id." ) - super().__init__(api.collection) + self.collection_id = api.collection_id + super().__init__() @property def xpath_expression(self) -> str: @@ -1643,8 +1600,7 @@ def process_one( # We're out of luck -- we can't fulfill this loan. fulfillment = None info = LoanInfo( - collection=self.collection, - data_source_name=DataSource.AXIS_360, + collection_id=self.collection_id, identifier_type=self.id_type, identifier=axis_identifier, start_date=start_date, @@ -1655,11 +1611,9 @@ def process_one( elif reserved: end_date = self._xpath1_date(availability, "axis:reservedEndDate", ns) info = HoldInfo( - collection=self.collection, - data_source_name=DataSource.AXIS_360, + collection_id=self.collection_id, identifier_type=self.id_type, identifier=axis_identifier, - start_date=None, end_date=end_date, hold_position=0, ) @@ -1668,12 +1622,9 @@ def process_one( availability, "axis:holdsQueuePosition", ns ) info = HoldInfo( - collection=self.collection, - data_source_name=DataSource.AXIS_360, + collection_id=self.collection_id, identifier_type=self.id_type, identifier=axis_identifier, - start_date=None, - end_date=None, hold_position=position, ) return info diff --git a/src/palace/manager/api/bibliotheca.py b/src/palace/manager/api/bibliotheca.py index 1a9259739..fe1aaee69 100644 --- a/src/palace/manager/api/bibliotheca.py +++ b/src/palace/manager/api/bibliotheca.py @@ -472,12 +472,8 @@ def checkout( # At this point we know we have a loan. loan_expires = CheckoutResponseParser().process_first(response.content) - loan = LoanInfo( - licensepool.collection, - DataSource.BIBLIOTHECA, - licensepool.identifier.type, - licensepool.identifier.identifier, - start_date=None, + loan = LoanInfo.from_license_pool( + licensepool, end_date=loan_expires, ) return loan @@ -561,11 +557,8 @@ def place_hold(self, patron, pin, licensepool, hold_notification_email=None): if response.status_code in (200, 201): start_date = utc_now() end_date = HoldResponseParser().process_first(response_content) - return HoldInfo( - licensepool.collection, - DataSource.BIBLIOTHECA, - licensepool.identifier.type, - licensepool.identifier.identifier, + return HoldInfo.from_license_pool( + licensepool, start_date=start_date, end_date=end_date, hold_position=None, @@ -1079,21 +1072,16 @@ def datevalue(key): identifier = self.text_of_subtag(tag, "ItemId") start_date = datevalue("EventStartDateInUTC") end_date = datevalue("EventEndDateInUTC") - a = [ - self.collection, - DataSource.BIBLIOTHECA, - self.id_type, - identifier, - start_date, - end_date, - ] + kwargs = { + "collection_id": self.collection.id, + "identifier_type": self.id_type, + "identifier": identifier, + "start_date": start_date, + "end_date": end_date, + } if source_class is HoldInfo: - hold_position = self.int_of_subtag(tag, "Position") - a.append(hold_position) - else: - # Fulfillment info -- not available from this API - a.append(None) - return source_class(*a) + kwargs["hold_position"] = self.int_of_subtag(tag, "Position") + return source_class(**kwargs) class DateResponseParser(BibliothecaParser[Optional[datetime]], ABC): diff --git a/src/palace/manager/api/circulation.py b/src/palace/manager/api/circulation.py index 634556a68..faab1351b 100644 --- a/src/palace/manager/api/circulation.py +++ b/src/palace/manager/api/circulation.py @@ -14,6 +14,7 @@ from pydantic import PositiveInt from sqlalchemy import select from sqlalchemy.orm import Session +from typing_extensions import Self from palace.manager.api.circulation_exceptions import ( AlreadyCheckedOut, @@ -35,6 +36,7 @@ PatronLoanLimitReached, ) from palace.manager.api.util.patron import PatronUtility +from palace.manager.core.exceptions import PalaceValueError from palace.manager.integration.base import HasLibraryIntegrationConfiguration from palace.manager.integration.settings import ( BaseSettings, @@ -98,31 +100,6 @@ def __init__( self.identifier_type = identifier_type self.identifier = identifier - def collection(self, _db: Session) -> Collection | None: - """Find the Collection to which this object belongs.""" - if self.collection_id is None: - return None - return Collection.by_id(_db, self.collection_id) - - def license_pool(self, _db: Session) -> LicensePool: - """Find the LicensePool model object corresponding to this object.""" - collection = self.collection(_db) - pool, is_new = LicensePool.for_foreign_id( - _db, - self.data_source_name, - self.identifier_type, - self.identifier, - collection=collection, - ) - return pool - - def fd(self, d: datetime.datetime | None) -> str | None: - # Stupid method to format a date - if not d: - return None - else: - return datetime.datetime.strftime(d, "%Y/%m/%d %H:%M:%S") - class DeliveryMechanismInfo(CirculationInfo): """A record of a technique that must be (but is not, currently, being) @@ -334,53 +311,111 @@ def response(self) -> Response: ) -class LoanInfo(CirculationInfo): +class LoanAndHoldInfoMixin: + collection_id: int + identifier_type: str + identifier: str + + def collection(self, _db: Session) -> Collection: + """Find the Collection to which this object belongs.""" + collection = Collection.by_id(_db, self.collection_id) + if collection is None: + raise PalaceValueError( + f"collection_id {self.collection_id} could not be found." + ) + return collection + + def license_pool(self, _db: Session) -> LicensePool: + """Find the LicensePool model object corresponding to this object.""" + collection = self.collection(_db) + pool, is_new = LicensePool.for_foreign_id( + _db, + collection.data_source, + self.identifier_type, + self.identifier, + collection=collection, + ) + return pool + + +@dataclasses.dataclass(kw_only=True) +class LoanInfo(LoanAndHoldInfoMixin): """A record of a loan.""" - def __init__( - self, - collection: Collection | int, - data_source_name: str | DataSource | None, - identifier_type: str | None, - identifier: str | None, - start_date: datetime.datetime | None, + collection_id: int + identifier_type: str + identifier: str + start_date: datetime.datetime | None = None + end_date: datetime.datetime | None + fulfillment: Fulfillment | None = None + external_identifier: str | None = None + locked_to: DeliveryMechanismInfo | None = None + + @classmethod + def from_license_pool( + cls, + license_pool: LicensePool, + *, + start_date: datetime.datetime | None = None, end_date: datetime.datetime | None, fulfillment: Fulfillment | None = None, external_identifier: str | None = None, locked_to: DeliveryMechanismInfo | None = None, - ): - """Constructor. - - :param start_date: A datetime reflecting when the patron borrowed the book. - :param end_date: A datetime reflecting when the checked-out book is due. - :param fulfillment: A Fulfillment object representing an - active attempt to fulfill the loan. - :param locked_to: A DeliveryMechanismInfo object representing the - delivery mechanism to which this loan is 'locked'. - """ - super().__init__(collection, data_source_name, identifier_type, identifier) - self.start_date = start_date - self.end_date = end_date - self.fulfillment = fulfillment - self.locked_to = locked_to - self.external_identifier = external_identifier + ) -> Self: + collection_id = license_pool.collection_id + assert collection_id is not None + identifier_type = license_pool.identifier.type + assert identifier_type is not None + identifier = license_pool.identifier.identifier + assert identifier is not None + return cls( + collection_id=collection_id, + identifier_type=identifier_type, + identifier=identifier, + start_date=start_date, + end_date=end_date, + fulfillment=fulfillment, + external_identifier=external_identifier, + locked_to=locked_to, + ) def __repr__(self) -> str: if self.fulfillment: fulfillment = " Fulfilled by: " + repr(self.fulfillment) else: fulfillment = "" - f = "%Y/%m/%d" return "{}".format( self.identifier_type, self.identifier, - self.fd(self.start_date), - self.fd(self.end_date), + self.start_date.isoformat() if self.start_date else self.start_date, + self.end_date.isoformat() if self.end_date else self.end_date, fulfillment, ) - -class HoldInfo(CirculationInfo): + def create_or_update( + self, patron: Patron, license_pool: LicensePool | None = None + ) -> tuple[Loan, bool]: + session = Session.object_session(patron) + license_pool = license_pool or self.license_pool(session) + loan, is_new = license_pool.loan_to( + patron, + start=self.start_date, + end=self.end_date, + fulfillment=self.fulfillment, + external_identifier=self.external_identifier, + ) + if self.locked_to: + # The loan source is letting us know that the loan is + # locked to a specific delivery mechanism. Even if + # this is the first we've heard of this loan, + # it may have been created in another app or through + # a library-website integration. + self.locked_to.apply(loan) + return loan, is_new + + +@dataclasses.dataclass(kw_only=True) +class HoldInfo(LoanAndHoldInfoMixin): """A record of a hold. :param identifier_type: Ex. Identifier.BIBLIOTHECA_ID. @@ -392,32 +427,58 @@ class HoldInfo(CirculationInfo): default to be passed is None, which is equivalent to "first in line". """ - def __init__( - self, - collection: Collection | int, - data_source_name: str | DataSource | None, - identifier_type: str | None, - identifier: str | None, - start_date: datetime.datetime | None, - end_date: datetime.datetime | None, + collection_id: int + identifier_type: str + identifier: str + start_date: datetime.datetime | None = None + end_date: datetime.datetime | None = None + hold_position: int | None + + @classmethod + def from_license_pool( + cls, + license_pool: LicensePool, + *, + start_date: datetime.datetime | None = None, + end_date: datetime.datetime | None = None, hold_position: int | None, - external_identifier: str | None = None, - ): - super().__init__(collection, data_source_name, identifier_type, identifier) - self.start_date = start_date - self.end_date = end_date - self.hold_position = hold_position - self.external_identifier = external_identifier + ) -> Self: + collection_id = license_pool.collection_id + assert collection_id is not None + identifier_type = license_pool.identifier.type + assert identifier_type is not None + identifier = license_pool.identifier.identifier + assert identifier is not None + return cls( + collection_id=collection_id, + identifier_type=identifier_type, + identifier=identifier, + start_date=start_date, + end_date=end_date, + hold_position=hold_position, + ) def __repr__(self) -> str: return "".format( self.identifier_type, self.identifier, - self.fd(self.start_date), - self.fd(self.end_date), + self.start_date.isoformat() if self.start_date else self.start_date, + self.end_date.isoformat() if self.end_date else self.end_date, self.hold_position, ) + def create_or_update( + self, patron: Patron, license_pool: LicensePool | None = None + ) -> tuple[Hold, bool]: + session = Session.object_session(patron) + license_pool = license_pool or self.license_pool(session) + return license_pool.on_hold_to( # type: ignore[no-any-return] + patron, + start=self.start_date, + end=self.end_date, + position=self.hold_position, + ) + class BaseCirculationEbookLoanSettings(BaseSettings): """A mixin for settings that apply to ebook loans.""" @@ -746,31 +807,7 @@ def sync_loans( ) -> None: # Update the local loans and to match the remote loans for identifier, loan in remote_loans.items(): - pool = loan.license_pool(self._db) - start = loan.start_date - end = loan.end_date - - if identifier in local_loans: - # We already have the Loan object, we don't need to look - # it up again. - local_loan = local_loans[identifier] - - # But maybe the remote's opinions as to the loan's - # start or end date have changed. - if start: - local_loan.start = start - if end: - local_loan.end = end - else: - local_loan, _ = pool.loan_to(patron, start, end) - - if loan.locked_to: - # The loan source is letting us know that the loan is - # locked to a specific delivery mechanism. Even if - # this is the first we've heard of this loan, - # it may have been created in another app or through - # a library-website integration. - loan.locked_to.apply(local_loan) + loan.create_or_update(patron) loans_to_delete = [ local_loans[i] for i in local_loans.keys() - remote_loans.keys() @@ -785,21 +822,7 @@ def sync_holds( ) -> None: # Update the local holds to match the remote holds for identifier, hold in remote_holds.items(): - pool = hold.license_pool(self._db) - start = hold.start_date - end = hold.end_date - position = hold.hold_position - - if identifier in local_holds: - # We already have the Hold object, we don't need to look - # it up again. - local_hold = local_holds[identifier] - - # But maybe the remote's opinions as to the hold's - # start or end date have changed. - local_hold.update(start, end, position) - else: - local_hold, _ = pool.on_hold_to(patron, start, end, position) + hold.create_or_update(patron) holds_to_delete = [ local_holds[i] for i in local_holds.keys() - remote_holds.keys() @@ -1026,14 +1049,14 @@ def borrow( # available -- someone else may have checked it in since we # last looked. try: - loan_info = api.checkout( + checkout_result = api.checkout( patron, pin, licensepool, delivery_mechanism=delivery_mechanism # type: ignore[arg-type] ) - if isinstance(loan_info, HoldInfo): + if isinstance(checkout_result, HoldInfo): # If the API couldn't give us a loan, it may have given us # a hold instead of raising an exception. - hold_info = loan_info + hold_info = checkout_result loan_info = None else: # We asked the API to create a loan and it gave us a @@ -1045,30 +1068,24 @@ def borrow( # API does something unusual like return LoanInfo instead # of raising AlreadyCheckedOut. new_loan = True + loan_info = checkout_result + hold_info = None except AlreadyCheckedOut: # This is good, but we didn't get the real loan info. # Just fake it. - identifier = licensepool.identifier - loan_info = LoanInfo( - licensepool.collection, - licensepool.data_source, - identifier.type, - identifier.identifier, + loan_info = LoanInfo.from_license_pool( + licensepool, start_date=None, end_date=now + datetime.timedelta(hours=1), + external_identifier=( + existing_loan.external_identifier if existing_loan else None + ), ) - if existing_loan: - loan_info.external_identifier = existing_loan.external_identifier except AlreadyOnHold: # We're trying to check out a book that we already have on hold. - hold_info = HoldInfo( - licensepool.collection, - licensepool.data_source, - licensepool.identifier.type, - licensepool.identifier.identifier, - None, - None, - None, + hold_info = HoldInfo.from_license_pool( + licensepool, + hold_position=None, ) except NoAvailableCopies: if existing_loan: @@ -1104,12 +1121,7 @@ def borrow( # We successfully secured a loan. Now create it in our # database. __transaction = self._db.begin_nested() - loan, new_loan_record = licensepool.loan_to( - patron, - start=loan_info.start_date or now, - end=loan_info.end_date, - external_identifier=loan_info.external_identifier, - ) + loan, new_loan_record = loan_info.create_or_update(patron, licensepool) if must_set_delivery_mechanism: loan.fulfillment = delivery_mechanism @@ -1144,14 +1156,9 @@ def borrow( patron, pin, licensepool, hold_notification_email ) except AlreadyOnHold as e: - hold_info = HoldInfo( - licensepool.collection, - licensepool.data_source, - licensepool.identifier.type, - licensepool.identifier.identifier, - None, - None, - None, + hold_info = HoldInfo.from_license_pool( + licensepool, + hold_position=None, ) except CurrentlyAvailable: if loan_exception: @@ -1172,13 +1179,7 @@ def borrow( # It's pretty rare that we'd go from having a loan for a book # to needing to put it on hold, but we do check for that case. __transaction = self._db.begin_nested() - hold, is_new = licensepool.on_hold_to( - patron, - hold_info.start_date or now, - hold_info.end_date, - hold_info.hold_position, - hold_info.external_identifier, - ) + hold, is_new = hold_info.create_or_update(patron, licensepool) if hold and is_new: # Send out an analytics event to record the fact that diff --git a/src/palace/manager/api/enki.py b/src/palace/manager/api/enki.py index 25ac8b017..ed57de9de 100644 --- a/src/palace/manager/api/enki.py +++ b/src/palace/manager/api/enki.py @@ -428,14 +428,9 @@ def checkout( expires = self._epoch_to_struct(due_date) # Create the loan info. - loan = LoanInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - None, - expires, - None, + loan = LoanInfo.from_license_pool( + licensepool, + end_date=expires, ) return loan @@ -572,16 +567,14 @@ def parse_patron_loans(self, checkout_data: Mapping[str, Any]) -> LoanInfo: enki_id = checkout_data["id"] start_date = self._epoch_to_struct(checkout_data["checkoutdate"]) end_date = self._epoch_to_struct(checkout_data["duedate"]) - if self.collection is None: - raise ValueError("Collection is None") + if self.collection_id is None: + raise ValueError("Collection_id is None") return LoanInfo( - self.collection, - DataSource.ENKI, - Identifier.ENKI_ID, - enki_id, + collection_id=self.collection_id, + identifier_type=Identifier.ENKI_ID, + identifier=enki_id, start_date=start_date, end_date=end_date, - fulfillment=None, ) def parse_patron_holds(self, hold_data: Mapping[str, Any]) -> HoldInfo | None: diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index 37f6acf11..e215fd64c 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -293,13 +293,10 @@ def checkout( loan_end = loan_obj.end external_identifier = loan_obj.external_identifier - return LoanInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - loan_start, - loan_end, + return LoanInfo.from_license_pool( + licensepool, + start_date=loan_start, + end_date=loan_end, external_identifier=external_identifier, ) @@ -615,14 +612,11 @@ def _count_holds_before(self, holdinfo: HoldInfo, pool: LicensePool) -> int: def _update_hold_data(self, hold: Hold) -> None: pool: LicensePool = hold.license_pool - holdinfo = HoldInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - hold.start, - hold.end, - hold.position, + 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) @@ -805,14 +799,10 @@ def _place_hold(self, patron: Patron, licensepool: LicensePool) -> HoldInfo: else 0 ) licensepool.patrons_in_hold_queue = patrons_in_hold_queue + 1 - holdinfo = HoldInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - utc_now(), - None, - 0, + holdinfo = HoldInfo.from_license_pool( + licensepool, + start_date=utc_now(), + hold_position=0, ) library = patron.library self._update_hold_end_date(holdinfo, licensepool, library=library) @@ -884,22 +874,16 @@ def patron_activity( remaining_holds.append(hold) return [ - LoanInfo( - loan.license_pool.collection, - loan.license_pool.data_source.name, - loan.license_pool.identifier.type, - loan.license_pool.identifier.identifier, - loan.start, - loan.end, + 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( - hold.license_pool.collection, - hold.license_pool.data_source.name, - hold.license_pool.identifier.type, - hold.license_pool.identifier.identifier, + HoldInfo.from_license_pool( + hold.license_pool, start_date=hold.start, end_date=hold.end, hold_position=hold.position, diff --git a/src/palace/manager/api/opds_for_distributors.py b/src/palace/manager/api/opds_for_distributors.py index 721652982..a177ff3c2 100644 --- a/src/palace/manager/api/opds_for_distributors.py +++ b/src/palace/manager/api/opds_for_distributors.py @@ -275,11 +275,8 @@ def checkout( delivery_mechanism: LicensePoolDeliveryMechanism, ) -> LoanInfo: now = utc_now() - return LoanInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, + return LoanInfo.from_license_pool( + licensepool, start_date=now, end_date=None, ) diff --git a/src/palace/manager/api/overdrive.py b/src/palace/manager/api/overdrive.py index c5b6fe172..dc6aa1a86 100644 --- a/src/palace/manager/api/overdrive.py +++ b/src/palace/manager/api/overdrive.py @@ -1017,14 +1017,9 @@ def checkout( expires = self.extract_expiration_date(data) # Create the loan info. - loan = LoanInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - None, - expires, - None, + loan = LoanInfo.from_license_pool( + licensepool, + end_date=expires, ) return loan @@ -1057,14 +1052,9 @@ def _process_checkout_error(self, patron, pin, licensepool, error): # then immediately borrows the same book through SimplyE. loan = self.get_loan(patron, pin, identifier.identifier) expires = self.extract_expiration_date(loan) - return LoanInfo( - licensepool.collection, - licensepool.data_source.name, - identifier.type, - identifier.identifier, - None, - expires, - None, + return LoanInfo.from_license_pool( + licensepool, + end_date=expires, ) if code in self.ERROR_MESSAGE_TO_EXCEPTION: @@ -1441,7 +1431,7 @@ def patron_activity( self, patron: Patron, pin: str | None ) -> Iterable[LoanInfo | HoldInfo]: collection = self.collection - if collection is None: + if collection is None or collection.id is None: raise BasePalaceException( "No collection available for Overdrive patron activity." ) @@ -1465,7 +1455,7 @@ def patron_activity( holds = {} for checkout in loans.get("checkouts", []): - loan_info = self.process_checkout_data(checkout, collection) + loan_info = self.process_checkout_data(checkout, collection.id) if loan_info is not None: yield loan_info @@ -1482,10 +1472,9 @@ def patron_activity( # 0, not whatever position Overdrive had for them. position = 0 yield HoldInfo( - collection, - DataSource.OVERDRIVE, - Identifier.OVERDRIVE_ID, - overdrive_identifier, + collection_id=collection.id, + identifier_type=Identifier.OVERDRIVE_ID, + identifier=overdrive_identifier, start_date=start, end_date=end, hold_position=position, @@ -1493,7 +1482,7 @@ def patron_activity( @classmethod def process_checkout_data( - cls, checkout: dict[str, Any], collection: Collection + cls, checkout: dict[str, Any], collection_id: int ) -> LoanInfo | None: """Convert one checkout from Overdrive's list of checkouts into a LoanInfo object. @@ -1555,10 +1544,9 @@ def process_checkout_data( ) return LoanInfo( - collection, - DataSource.OVERDRIVE, - Identifier.OVERDRIVE_ID, - overdrive_identifier, + collection_id=collection_id, + identifier_type=Identifier.OVERDRIVE_ID, + identifier=overdrive_identifier, start_date=start, end_date=end, locked_to=locked_to, @@ -1642,13 +1630,9 @@ def make_holdinfo(hold_response): # (either creating a new hold or fetching an existing # one). position, start_date = self.extract_data_from_hold_response(hold_response) - return HoldInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, + return HoldInfo.from_license_pool( + licensepool, start_date=start_date, - end_date=None, hold_position=position, ) diff --git a/src/palace/manager/core/opds_import.py b/src/palace/manager/core/opds_import.py index c11a0480f..88e70bf4d 100644 --- a/src/palace/manager/core/opds_import.py +++ b/src/palace/manager/core/opds_import.py @@ -344,7 +344,7 @@ def checkout( licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, ) -> LoanInfo: - return LoanInfo(licensepool.collection, None, None, None, None, None) + return LoanInfo.from_license_pool(licensepool, end_date=None) def can_fulfill_without_loan( self, diff --git a/src/palace/manager/sqlalchemy/model/licensing.py b/src/palace/manager/sqlalchemy/model/licensing.py index a391455ad..120f7b02c 100644 --- a/src/palace/manager/sqlalchemy/model/licensing.py +++ b/src/palace/manager/sqlalchemy/model/licensing.py @@ -1053,6 +1053,10 @@ def loan_to( loan.fulfillment = fulfillment if external_identifier: loan.external_identifier = external_identifier + if start: + loan.start = start + if end: + loan.end = end return loan, is_new def on_hold_to( @@ -1061,7 +1065,6 @@ def on_hold_to( start=None, end=None, position=None, - external_identifier=None, ): _db = Session.object_session(patron) if not patron.library.settings.allow_holds: @@ -1069,8 +1072,6 @@ def on_hold_to( start = start or utc_now() hold, new = get_one_or_create(_db, Hold, patron=patron, license_pool=self) hold.update(start, end, position) - if external_identifier: - hold.external_identifier = external_identifier return hold, new def best_available_license(self) -> License | None: diff --git a/src/palace/manager/sqlalchemy/model/patron.py b/src/palace/manager/sqlalchemy/model/patron.py index 5300579e0..118a7b782 100644 --- a/src/palace/manager/sqlalchemy/model/patron.py +++ b/src/palace/manager/sqlalchemy/model/patron.py @@ -567,7 +567,6 @@ class Hold(Base, LoanAndHoldMixin): start = Column(DateTime(timezone=True), index=True) end = Column(DateTime(timezone=True), index=True) position = Column(Integer, index=True) - external_identifier = Column(Unicode, unique=True, nullable=True) patron_last_notified = Column(DateTime, nullable=True) patron: Mapped[Patron] = relationship( diff --git a/tests/manager/api/controller/test_loan.py b/tests/manager/api/controller/test_loan.py index 7ac1e726c..4948b44bb 100644 --- a/tests/manager/api/controller/test_loan.py +++ b/tests/manager/api/controller/test_loan.py @@ -271,13 +271,10 @@ def test_borrow_success( pool = work.license_pools[0] loan_fixture.manager.d_circulation.queue_checkout( pool, - LoanInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), + LoanInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), ), ) @@ -494,13 +491,10 @@ def test_borrow_and_fulfill_with_streaming_delivery_mechanism( loan_fixture.manager.loans.authenticated_patron_from_request() loan_fixture.manager.d_circulation.queue_checkout( pool, - LoanInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), + LoanInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), ), ) with patch( @@ -673,14 +667,11 @@ def test_borrow_creates_hold_when_no_available_copies( loan_fixture.manager.d_circulation.queue_checkout(pool, NoAvailableCopies()) loan_fixture.manager.d_circulation.queue_hold( pool, - HoldInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), - 1, + HoldInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), + hold_position=1, ), ) response = loan_fixture.manager.loans.borrow( @@ -739,14 +730,11 @@ def test_borrow_creates_local_hold_if_remote_hold_exists( loan_fixture.manager.d_circulation.queue_checkout(pool, AlreadyOnHold()) loan_fixture.manager.d_circulation.queue_hold( pool, - HoldInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), - 1, + HoldInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), + hold_position=1, ), ) response = loan_fixture.manager.loans.borrow( @@ -1347,13 +1335,10 @@ def test_borrow_fails_with_outstanding_fines( patron.fines = Decimal("0.49") loan_fixture.manager.d_circulation.queue_checkout( pool, - LoanInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), + LoanInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), ), ) response = loan_fixture.manager.loans.borrow( @@ -1706,13 +1691,10 @@ def create_work_and_return_license_pool_and_loan_info(**kwargs): ) license_pool = work.license_pools[0] - loan_info = LoanInfo( - license_pool.collection, - license_pool.data_source.name, - license_pool.identifier.type, - license_pool.identifier.identifier, - loan_start, - loan_end, + loan_info = LoanInfo.from_license_pool( + license_pool, + start_date=loan_start, + end_date=loan_end, ) return license_pool, loan_info diff --git a/tests/manager/api/controller/test_work.py b/tests/manager/api/controller/test_work.py index df76536d5..b4ee11410 100644 --- a/tests/manager/api/controller/test_work.py +++ b/tests/manager/api/controller/test_work.py @@ -423,13 +423,10 @@ def test_permalink_returns_fulfillment_links_for_authenticated_patrons_with_fulf pool = work.license_pools[0] [delivery_mechanism] = pool.delivery_mechanisms - loan_info = LoanInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - utc_now() + datetime.timedelta(seconds=3600), + loan_info = LoanInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=utc_now() + datetime.timedelta(seconds=3600), ) work_fixture.manager.d_circulation.queue_checkout(pool, loan_info) diff --git a/tests/manager/api/odl/test_api.py b/tests/manager/api/odl/test_api.py index 058ae542f..de5a95dd2 100644 --- a/tests/manager/api/odl/test_api.py +++ b/tests/manager/api/odl/test_api.py @@ -447,7 +447,6 @@ def test_checkout_success( 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.data_source.name == loan.data_source_name 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 @@ -559,7 +558,6 @@ def test_checkout_success_with_hold( # The patron gets a loan successfully. assert opds2_with_odl_api_fixture.collection == loan.collection(db.session) - assert opds2_with_odl_api_fixture.pool.data_source.name == loan.data_source_name 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 @@ -1061,14 +1059,11 @@ def test_fulfill_cannot_fulfill( def _holdinfo_from_hold(self, hold: Hold) -> HoldInfo: pool: LicensePool = hold.license_pool - return HoldInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - hold.start, - hold.end, - hold.position, + return HoldInfo.from_license_pool( + pool, + start_date=hold.start, + end_date=hold.end, + hold_position=hold.position, ) def test_count_holds_before( @@ -1542,7 +1537,6 @@ def test_place_hold_success( 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.data_source.name == hold.data_source_name 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 @@ -1649,28 +1643,16 @@ def test_patron_activity_loan( # One loan. _, loan = opds2_with_odl_api_fixture.checkout() - activity = opds2_with_odl_api_fixture.api.patron_activity( + [l1] = opds2_with_odl_api_fixture.api.patron_activity( opds2_with_odl_api_fixture.patron, "pin" ) - assert 1 == len(activity) - assert opds2_with_odl_api_fixture.collection == activity[0].collection( - db.session - ) - assert ( - opds2_with_odl_api_fixture.pool.data_source.name - == activity[0].data_source_name - ) - assert ( - opds2_with_odl_api_fixture.pool.identifier.type - == activity[0].identifier_type - ) - assert ( - opds2_with_odl_api_fixture.pool.identifier.identifier - == activity[0].identifier - ) - assert loan.start == activity[0].start_date - assert loan.end == activity[0].end_date - assert loan.external_identifier == activity[0].external_identifier + 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) @@ -1687,9 +1669,10 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: ) 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.data_source.name == l1.data_source_name 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 @@ -1697,7 +1680,6 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: assert loan.external_identifier == l1.external_identifier assert opds2_with_odl_api_fixture.collection == l2.collection(db.session) - assert pool2.data_source.name == l2.data_source_name assert pool2.identifier.type == l2.identifier_type assert pool2.identifier.identifier == l2.identifier assert loan2.start == l2.start_date @@ -1706,14 +1688,11 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: # If a loan is expired already, it's left out. loan2.end = utc_now() - datetime.timedelta(days=2) - activity = opds2_with_odl_api_fixture.api.patron_activity( + [l1] = opds2_with_odl_api_fixture.api.patron_activity( opds2_with_odl_api_fixture.patron, "pin" ) - assert 1 == len(activity) - assert ( - opds2_with_odl_api_fixture.pool.identifier.identifier - == activity[0].identifier - ) + 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. @@ -1729,9 +1708,10 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: ) 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.data_source.name == l1.data_source_name 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 @@ -1739,7 +1719,6 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: assert loan.external_identifier == l1.external_identifier assert opds2_with_odl_api_fixture.collection == l2.collection(db.session) - assert pool3.data_source.name == l2.data_source_name assert pool3.identifier.type == l2.identifier_type assert pool3.identifier.identifier == l2.identifier assert loan3.start == l2.start_date @@ -1765,7 +1744,6 @@ def activity_sort_key(activity: LoanInfo | HoldInfo) -> datetime.datetime: assert isinstance(h1, HoldInfo) assert opds2_with_odl_api_fixture.collection == h1.collection(db.session) - assert pool2.data_source.name == h1.data_source_name assert pool2.identifier.type == h1.identifier_type assert pool2.identifier.identifier == h1.identifier assert hold.start == h1.start_date diff --git a/tests/manager/api/test_axis.py b/tests/manager/api/test_axis.py index 6bed8f8ca..654d52383 100644 --- a/tests/manager/api/test_axis.py +++ b/tests/manager/api/test_axis.py @@ -1397,7 +1397,7 @@ def axis360parsers( class TestRaiseExceptionOnError: def test_internal_server_error(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("internal_server_error.xml") - parser = HoldReleaseResponseParser(MagicMock()) + parser = HoldReleaseResponseParser() with pytest.raises(RemoteInitiatedServerError) as excinfo: parser.process_first(data) assert "Internal Server Error" in str(excinfo.value) @@ -1416,19 +1416,19 @@ def process_one(self, e, namespaces): # Unlike in test_internal_server_error, no exception is # raised, because we told the parser to ignore this particular # error code. - parser = IgnoreISE(MagicMock()) + parser = IgnoreISE() assert retval == parser.process_first(data) def test_internal_server_error2(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("invalid_error_code.xml") - parser = HoldReleaseResponseParser(MagicMock()) + parser = HoldReleaseResponseParser() with pytest.raises(RemoteInitiatedServerError) as excinfo: parser.process_first(data) assert "Invalid response code from Axis 360: abcd" in str(excinfo.value) def test_missing_error_code(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("missing_error_code.xml") - parser = HoldReleaseResponseParser(MagicMock()) + parser = HoldReleaseResponseParser() with pytest.raises(RemoteInitiatedServerError) as excinfo: parser.process_first(data) assert "No status code!" in str(excinfo.value) @@ -1442,70 +1442,56 @@ def test_parse_checkin_success(self, axis360parsers: Axis360FixturePlusParsers): # "Book is not on loan" is not treated as a problem. for filename in ("checkin_success.xml", "checkin_not_checked_out.xml"): data = axis360parsers.sample_data(filename) - parser = CheckinResponseParser(axis360parsers.default_collection) + parser = CheckinResponseParser() parsed = parser.process_first(data) assert parsed is True def test_parse_checkin_failure(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("checkin_failure.xml") - parser = CheckinResponseParser(axis360parsers.default_collection) + parser = CheckinResponseParser() pytest.raises(NotFoundOnRemote, parser.process_first, data) class TestCheckoutResponseParser: def test_parse_checkout_success(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("checkout_success.xml") - parser = CheckoutResponseParser(axis360parsers.default_collection) + parser = CheckoutResponseParser() parsed = parser.process_first(data) - assert isinstance(parsed, LoanInfo) - assert axis360parsers.default_collection.id == parsed.collection_id - assert DataSource.AXIS_360 == parsed.data_source_name - assert Identifier.AXIS_360_ID == parsed.identifier_type - assert datetime_utc(2015, 8, 11, 18, 57, 42) == parsed.end_date - - # There is no Fulfillment associated with the LoanInfo, - # because we don't need it (checkout and fulfillment are - # separate steps). - assert parsed.fulfillment == None + assert datetime_utc(2015, 8, 11, 18, 57, 42) == parsed def test_parse_already_checked_out(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("already_checked_out.xml") - parser = CheckoutResponseParser(MagicMock()) + parser = CheckoutResponseParser() pytest.raises(AlreadyCheckedOut, parser.process_first, data) def test_parse_not_found_on_remote(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("not_found_on_remote.xml") - parser = CheckoutResponseParser(MagicMock()) + parser = CheckoutResponseParser() pytest.raises(NotFoundOnRemote, parser.process_first, data) class TestHoldResponseParser: def test_parse_hold_success(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("place_hold_success.xml") - parser = HoldResponseParser(axis360parsers.default_collection) + parser = HoldResponseParser() parsed = parser.process_first(data) - assert isinstance(parsed, HoldInfo) - assert 1 == parsed.hold_position - - # The HoldInfo is given the Collection object we passed into - # the HoldResponseParser. - assert axis360parsers.default_collection.id == parsed.collection_id + assert 1 == parsed def test_parse_already_on_hold(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("already_on_hold.xml") - parser = HoldResponseParser(MagicMock()) + parser = HoldResponseParser() pytest.raises(AlreadyOnHold, parser.process_first, data) class TestHoldReleaseResponseParser: def test_success(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("release_hold_success.xml") - parser = HoldReleaseResponseParser(MagicMock()) + parser = HoldReleaseResponseParser() assert True == parser.process_first(data) def test_failure(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("release_hold_failure.xml") - parser = HoldReleaseResponseParser(MagicMock()) + parser = HoldReleaseResponseParser() pytest.raises(NotOnHold, parser.process_first, data) diff --git a/tests/manager/api/test_bibliotheca.py b/tests/manager/api/test_bibliotheca.py index 79c09c1d5..8d279e183 100644 --- a/tests/manager/api/test_bibliotheca.py +++ b/tests/manager/api/test_bibliotheca.py @@ -820,7 +820,6 @@ def test_parse(self, bibliotheca_fixture: BibliothecaAPITestFixture): # This is the book on reserve. assert collection.id == h1.collection_id - assert DataSource.BIBLIOTHECA == h1.data_source_name assert "9wd8" == h1.identifier expect_hold_start = datetime_utc(2015, 5, 25, 17, 5, 34) expect_hold_end = datetime_utc(2015, 5, 27, 17, 5, 34) @@ -831,7 +830,6 @@ def test_parse(self, bibliotheca_fixture: BibliothecaAPITestFixture): # This is the book on hold. assert "d4o8r9" == h2.identifier assert collection.id == h2.collection_id - assert DataSource.BIBLIOTHECA == h2.data_source_name expect_hold_start = datetime_utc(2015, 3, 24, 15, 6, 56) expect_hold_end = datetime_utc(2015, 3, 24, 15, 7, 51) assert expect_hold_start == h2.start_date diff --git a/tests/manager/api/test_circulationapi.py b/tests/manager/api/test_circulationapi.py index 8437b1822..f27358cf8 100644 --- a/tests/manager/api/test_circulationapi.py +++ b/tests/manager/api/test_circulationapi.py @@ -8,7 +8,9 @@ import flask import pytest from flask import Flask +from freezegun import freeze_time +from palace.manager.api.bibliotheca import BibliothecaAPI from palace.manager.api.circulation import ( BaseCirculationAPI, CirculationAPI, @@ -114,13 +116,10 @@ def test_circulationinfo_collection_id( def test_borrow_sends_analytics_event(self, circulation_api: CirculationAPIFixture): now = utc_now() - loaninfo = LoanInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.pool.identifier.type, - circulation_api.pool.identifier.identifier, - now, - now + timedelta(seconds=3600), + loaninfo = LoanInfo.from_license_pool( + circulation_api.pool, + start_date=now, + end_date=now + timedelta(seconds=3600), external_identifier=circulation_api.db.fresh_str(), ) circulation_api.remote.queue_checkout(loaninfo) @@ -165,6 +164,7 @@ def test_borrow_sends_analytics_event(self, circulation_api: CirculationAPIFixtu loan, hold, is_new = self.borrow(circulation_api) assert 3 == circulation_api.analytics.count + @freeze_time() def test_attempt_borrow_with_existing_remote_loan( self, circulation_api: CirculationAPIFixture ): @@ -187,8 +187,8 @@ def test_attempt_borrow_with_existing_remote_loan( # but didn't give us any useful information on when that loan # was created. We've faked it with values that should be okay # until the next sync. - assert abs((loan.start - now).seconds) < 2 - assert 3600 == (loan.end - loan.start).seconds + assert (loan.start - now).seconds == 0 + assert (loan.end - loan.start).seconds == 3600 def test_attempt_borrow_with_existing_remote_hold( self, circulation_api: CirculationAPIFixture @@ -262,14 +262,9 @@ def test_loan_becomes_hold_if_no_available_copies( ): # We want to borrow this book but there are no copies. circulation_api.remote.queue_checkout(NoAvailableCopies()) - holdinfo = HoldInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.identifier.type, - circulation_api.identifier.identifier, - None, - None, - 10, + holdinfo = HoldInfo.from_license_pool( + circulation_api.pool, + hold_position=10, ) circulation_api.remote.queue_hold(holdinfo) @@ -287,14 +282,9 @@ def test_borrow_creates_hold_if_api_returns_hold_info( # There are no available copies, but the remote API # places a hold for us right away instead of raising # an error. - holdinfo = HoldInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.identifier.type, - circulation_api.identifier.identifier, - None, - None, - 10, + holdinfo = HoldInfo.from_license_pool( + circulation_api.pool, + hold_position=10, ) circulation_api.remote.queue_checkout(holdinfo) @@ -315,14 +305,9 @@ def test_vendor_side_loan_limit_allows_for_hold_placement( # But the point is moot because the book isn't even available. # Attempting to place a hold will succeed. - holdinfo = HoldInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.identifier.type, - circulation_api.identifier.identifier, - None, - None, - 10, + holdinfo = HoldInfo.from_license_pool( + circulation_api.pool, + hold_position=10, ) circulation_api.remote.queue_hold(holdinfo) @@ -362,14 +347,9 @@ def test_loan_exception_reraised_if_hold_placement_fails( def test_hold_sends_analytics_event(self, circulation_api: CirculationAPIFixture): circulation_api.remote.queue_checkout(NoAvailableCopies()) - holdinfo = HoldInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.identifier.type, - circulation_api.identifier.identifier, - None, - None, - 10, + holdinfo = HoldInfo.from_license_pool( + circulation_api.pool, + hold_position=10, ) circulation_api.remote.queue_hold(holdinfo) @@ -401,13 +381,10 @@ def test_borrow_with_expired_card_fails( # We use local time here, rather than UTC time, because we use # local time when checking for expired cards in authorization_is_active. now = datetime.datetime.now() - loaninfo = LoanInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.pool.identifier.type, - circulation_api.pool.identifier.identifier, - now, - now + timedelta(seconds=3600), + loaninfo = LoanInfo.from_license_pool( + circulation_api.pool, + start_date=now, + end_date=now + timedelta(seconds=3600), ) circulation_api.remote.queue_checkout(loaninfo) @@ -424,13 +401,10 @@ def test_borrow_with_outstanding_fines( ): # This checkout would succeed... now = utc_now() - loaninfo = LoanInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.pool.identifier.type, - circulation_api.pool.identifier.identifier, - now, - now + timedelta(seconds=3600), + loaninfo = LoanInfo.from_license_pool( + circulation_api.pool, + start_date=now, + end_date=now + timedelta(seconds=3600), ) circulation_api.remote.queue_checkout(loaninfo) @@ -457,13 +431,10 @@ def test_borrow_with_outstanding_fines( def test_borrow_with_block_fails(self, circulation_api: CirculationAPIFixture): # This checkout would succeed... now = utc_now() - loaninfo = LoanInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.pool.identifier.type, - circulation_api.pool.identifier.identifier, - now, - now + timedelta(seconds=3600), + loaninfo = LoanInfo.from_license_pool( + circulation_api.pool, + start_date=now, + end_date=now + timedelta(seconds=3600), ) circulation_api.remote.queue_checkout(loaninfo) @@ -787,14 +758,11 @@ def test_borrow_hold_limit_reached( library_fixture.settings(circulation_api.patron.library).hold_limit = 2 circulation_api.remote.queue_checkout(NoAvailableCopies()) now = utc_now() - holdinfo = HoldInfo( - circulation_api.pool.collection, - circulation_api.pool.data_source, - circulation_api.pool.identifier.type, - circulation_api.pool.identifier.identifier, - now, - now + timedelta(seconds=3600), - 10, + holdinfo = HoldInfo.from_license_pool( + circulation_api.pool, + start_date=now, + end_date=now + timedelta(seconds=3600), + hold_position=10, ) circulation_api.remote.queue_hold(holdinfo) loan, hold, is_new = self.borrow(circulation_api) @@ -1060,7 +1028,7 @@ class PatronActivityCirculationAPIFixture: def __init__(self, db: DatabaseTransactionFixture) -> None: self.db = db self.patron = db.patron() - self.collection = db.collection() + self.collection = db.collection(protocol=BibliothecaAPI) edition, self.pool = db.edition( data_source_name=DataSource.BIBLIOTHECA, identifier_type=Identifier.BIBLIOTHECA_ID, @@ -1191,13 +1159,10 @@ def test_sync_patron_activity_updates_local_loan_and_hold_with_modified_timestam # But the remote thinks the loan runs from today until two # weeks from today. patron_activity_circulation_api.api.add_remote_loan( - LoanInfo( - patron_activity_circulation_api.pool.collection, - patron_activity_circulation_api.pool.data_source, - patron_activity_circulation_api.identifier.type, - patron_activity_circulation_api.identifier.identifier, - patron_activity_circulation_api.now, - patron_activity_circulation_api.in_two_weeks, + LoanInfo.from_license_pool( + patron_activity_circulation_api.pool, + start_date=patron_activity_circulation_api.now, + end_date=patron_activity_circulation_api.in_two_weeks, ) ) @@ -1215,14 +1180,11 @@ def test_sync_patron_activity_updates_local_loan_and_hold_with_modified_timestam hold.position = 10 patron_activity_circulation_api.api.add_remote_hold( - HoldInfo( - pool2.collection, - pool2.data_source, - pool2.identifier.type, - pool2.identifier.identifier, - patron_activity_circulation_api.now, - patron_activity_circulation_api.in_two_weeks, - 0, + HoldInfo.from_license_pool( + pool2, + start_date=patron_activity_circulation_api.now, + end_date=patron_activity_circulation_api.in_two_weeks, + hold_position=0, ) ) patron_activity_circulation_api.sync_patron_activity() @@ -1246,15 +1208,14 @@ def test_sync_patron_activity_applies_locked_delivery_mechanism_to_loan( mechanism = DeliveryMechanismInfo( Representation.TEXT_HTML_MEDIA_TYPE, DeliveryMechanism.NO_DRM ) - pool = db.licensepool(None) + data_source = db.default_collection().data_source + assert data_source is not None + pool = db.licensepool(None, data_source_name=data_source.name) patron_activity_circulation_api.api.add_remote_loan( - LoanInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - utc_now(), - None, + LoanInfo.from_license_pool( + pool, + start_date=utc_now(), + end_date=None, locked_to=mechanism, ) ) diff --git a/tests/manager/api/test_enki.py b/tests/manager/api/test_enki.py index ce768126b..154bfecc4 100644 --- a/tests/manager/api/test_enki.py +++ b/tests/manager/api/test_enki.py @@ -403,7 +403,6 @@ def test_checkout_open_access_parser(self, enki_test_fixture: EnkiTestFixure): loan = enki_test_fixture.api.parse_patron_loans( result["result"]["checkedOutItems"][0] ) - assert loan.data_source_name == DataSource.ENKI assert loan.identifier_type == Identifier.ENKI_ID assert loan.identifier == "2" assert loan.start_date == datetime_utc(2017, 8, 23, 19, 31, 58, 0) @@ -416,7 +415,6 @@ def test_checkout_acs_parser(self, enki_test_fixture: EnkiTestFixure): loan = enki_test_fixture.api.parse_patron_loans( result["result"]["checkedOutItems"][0] ) - assert loan.data_source_name == DataSource.ENKI assert loan.identifier_type == Identifier.ENKI_ID assert loan.identifier == "3334" assert loan.start_date == datetime_utc(2017, 8, 23, 19, 42, 35, 0) @@ -603,7 +601,6 @@ def test_patron_activity(self, enki_test_fixture: EnkiTestFixure): # The result is a single LoanInfo. assert isinstance(loan, LoanInfo) assert Identifier.ENKI_ID == loan.identifier_type - assert DataSource.ENKI == loan.data_source_name assert "231" == loan.identifier assert enki_test_fixture.collection == loan.collection(db.session) assert datetime_utc(2017, 8, 15, 14, 56, 51) == loan.start_date diff --git a/tests/manager/api/test_opds_for_distributors.py b/tests/manager/api/test_opds_for_distributors.py index 56be5cfec..06efe1a4e 100644 --- a/tests/manager/api/test_opds_for_distributors.py +++ b/tests/manager/api/test_opds_for_distributors.py @@ -421,7 +421,6 @@ def test_checkout(self, opds_dist_api_fixture: OPDSForDistributorsAPIFixture): patron, "1234", pool, MagicMock() ) assert opds_dist_api_fixture.collection.id == loan_info.collection_id - assert data_source.name == loan_info.data_source_name assert Identifier.URI == loan_info.identifier_type assert pool.identifier.identifier == loan_info.identifier diff --git a/tests/manager/api/test_overdrive.py b/tests/manager/api/test_overdrive.py index c23793901..c49b3cf39 100644 --- a/tests/manager/api/test_overdrive.py +++ b/tests/manager/api/test_overdrive.py @@ -866,7 +866,6 @@ def _process_checkout_error(self, patron, pin, licensepool, data): # The return value is a LoanInfo object with all relevant info. assert isinstance(loan, LoanInfo) assert pool.collection.id == loan.collection_id - assert pool.data_source.name == loan.data_source_name assert identifier.type == loan.identifier_type assert identifier.identifier == loan.identifier assert None == loan.start_date @@ -995,7 +994,6 @@ def with_error_code(code): # And a LoanInfo was created with all relevant information. assert isinstance(loan, LoanInfo) assert pool.collection.id == loan.collection_id - assert pool.data_source.name == loan.data_source_name assert identifier.type == loan.identifier_type assert identifier.identifier == loan.identifier assert None == loan.start_date @@ -1177,7 +1175,6 @@ def process_error_response(message): def assert_correct_holdinfo(x): assert isinstance(x, HoldInfo) assert licensepool.collection == x.collection(db.session) - assert licensepool.data_source.name == x.data_source_name assert identifier.identifier == x.identifier assert identifier.type == x.identifier_type assert datetime_utc(2015, 3, 26, 11, 30, 29) == x.start_date diff --git a/tests/manager/celery/tasks/test_patron_activity.py b/tests/manager/celery/tasks/test_patron_activity.py index a1d58e676..bc169d00d 100644 --- a/tests/manager/celery/tasks/test_patron_activity.py +++ b/tests/manager/celery/tasks/test_patron_activity.py @@ -166,28 +166,26 @@ def test_not_supported( def test_success( self, sync_task_fixture: SyncTaskFixture, db: DatabaseTransactionFixture ): - loan_pool = db.licensepool(None, collection=sync_task_fixture.collection) - hold_pool = db.licensepool(None, collection=sync_task_fixture.collection) + collection = sync_task_fixture.collection + assert collection.data_source is not None + data_source_name = collection.data_source.name + loan_pool = db.licensepool( + None, collection=collection, data_source_name=data_source_name + ) + hold_pool = db.licensepool( + None, collection=collection, data_source_name=data_source_name + ) sync_task_fixture.mock_collection_api.add_remote_loan( - LoanInfo( - collection=sync_task_fixture.collection, - data_source_name=loan_pool.data_source, - identifier_type=loan_pool.identifier.type, - identifier=loan_pool.identifier.identifier, - start_date=None, + LoanInfo.from_license_pool( + loan_pool, end_date=None, ) ) sync_task_fixture.mock_collection_api.add_remote_hold( - HoldInfo( - collection=sync_task_fixture.collection, - data_source_name=hold_pool.data_source, - identifier_type=hold_pool.identifier.type, - identifier=hold_pool.identifier.identifier, + HoldInfo.from_license_pool( + hold_pool, hold_position=1, - start_date=None, - end_date=None, ) ) diff --git a/tests/manager/core/test_opds_import.py b/tests/manager/core/test_opds_import.py index 41362dcf2..1641006d8 100644 --- a/tests/manager/core/test_opds_import.py +++ b/tests/manager/core/test_opds_import.py @@ -3,7 +3,7 @@ import random from functools import partial from io import StringIO -from unittest.mock import MagicMock, PropertyMock, patch +from unittest.mock import MagicMock, patch import pytest import requests_mock @@ -2123,10 +2123,6 @@ def test_update_availability(self, opds_api_fixture: OPDSAPIFixture) -> None: def test_checkout(self, opds_api_fixture: OPDSAPIFixture) -> None: # Make sure checkout returns a LoanInfo object with the correct # collection id. - mock_collection_property = PropertyMock( - return_value=opds_api_fixture.collection - ) - type(opds_api_fixture.mock_licensepool).collection = mock_collection_property delivery_mechanism = MagicMock(spec=LicensePoolDeliveryMechanism) loan = opds_api_fixture.api.checkout( opds_api_fixture.mock_patron, @@ -2135,8 +2131,7 @@ def test_checkout(self, opds_api_fixture: OPDSAPIFixture) -> None: delivery_mechanism, ) assert isinstance(loan, LoanInfo) - assert mock_collection_property.call_count == 1 - assert loan.collection_id == opds_api_fixture.collection.id + assert loan.collection_id == opds_api_fixture.mock_licensepool.collection_id def test_can_fulfill_without_loan(self, opds_api_fixture: OPDSAPIFixture) -> None: # This should always return True. diff --git a/tests/manager/sqlalchemy/model/test_licensing.py b/tests/manager/sqlalchemy/model/test_licensing.py index 0670451aa..b328d356e 100644 --- a/tests/manager/sqlalchemy/model/test_licensing.py +++ b/tests/manager/sqlalchemy/model/test_licensing.py @@ -1174,13 +1174,11 @@ def test_on_hold_to_patron(self, db: DatabaseTransactionFixture): fulfillment = pool.delivery_mechanisms[0] position = 99 - external_identifier = db.fresh_str() hold, is_new = pool.on_hold_to( patron, start=yesterday, end=tomorrow, position=position, - external_identifier=external_identifier, ) assert is_new is True @@ -1190,7 +1188,6 @@ def test_on_hold_to_patron(self, db: DatabaseTransactionFixture): assert yesterday == hold.start assert tomorrow == hold.end assert position == hold.position - assert external_identifier == hold.external_identifier # 'Creating' a hold that already exists returns the existing hold. hold2, is_new = pool.on_hold_to( @@ -1198,7 +1195,6 @@ def test_on_hold_to_patron(self, db: DatabaseTransactionFixture): start=yesterday, end=tomorrow, position=position, - external_identifier=external_identifier, ) assert is_new is False assert hold == hold2