Refactor LoanInfo and HoldInfo to be dataclasses (PP-1728) (#2113)

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"]
+    )

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

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):