Merge pull request #97 from NatLibFi/EKIR-213-Enable-loan-when-not-at…

…-loan-limit

Ekir 213 enable loan when not at loan limit

api/circulation.py

@@ -926,7 +926,7 @@ def borrow(
         delivery_mechanism: LicensePoolDeliveryMechanism,
         hold_notification_email: str | None = None,
     ) -> tuple[Loan | None, Hold | None, bool]:
-        """Either borrow a book or put it on hold. Don't worry about fulfilling
+        """Either borrow a book or put it or leave it on hold. Don't worry about fulfilling
         the loan yet.
 
         :return: A 3-tuple (`Loan`, `Hold`, `is_new`). Either `Loan`
@@ -964,6 +964,15 @@ def borrow(
             on_multiple="interchangeable",
         )
 
+        # Or maybe we have it on hold?
+        existing_hold = get_one(
+            self._db,
+            Hold,
+            patron=patron,
+            license_pool=licensepool,
+            on_multiple="interchangeable",
+        )
+
         loan_info = None
         hold_info = None
         if existing_loan and isinstance(api, PatronActivityCirculationAPI):
@@ -1002,6 +1011,11 @@ def borrow(
         # Enforce any library-specific limits on loans or holds.
         self.enforce_limits(patron, licensepool)
 
+        # When the patron is at the top of the hold queue ("reverved")
+        # but the loan fails, we want to raise an exception  when that
+        # happens.
+        reserved_license_exception = False
+
         # Since that didn't raise an exception, we don't know of any
         # reason why the patron shouldn't be able to get a loan or a
         # hold. There are race conditions that will allow someone to
@@ -1069,6 +1083,24 @@ def borrow(
                 raise CannotRenew(
                     _("You cannot renew a loan if other patrons have the work on hold.")
                 )
+
+            # The patron had a hold and was in the hold queue's 0th position believing
+            # there were copies available for them to checkout.
+            if existing_hold and existing_hold.position == 0:
+                # Update the hold so the patron doesn't lose their hold. Extend the hold to expire in the
+                # next 3 days.
+                hold_info = HoldInfo(
+                    licensepool.collection,
+                    licensepool.data_source,
+                    licensepool.identifier.type,
+                    licensepool.identifier.identifier,
+                    existing_hold.start,
+                    datetime.datetime.now() + datetime.timedelta(days=3),
+                    existing_hold.position,
+                )
+                # Update availability information
+                api.update_availability(licensepool)
+                reserved_license_exception = True
             else:
                 # That's fine, we'll just (try to) place a hold.
                 #
@@ -1131,35 +1163,42 @@ def borrow(
         # Checking out a book didn't work, so let's try putting
         # the book on hold.
         if not hold_info:
-            try:
-                hold_info = api.place_hold(
-                    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,
-                )
-            except CurrentlyAvailable:
-                if loan_exception:
-                    # We tried to take out a loan and got an
-                    # exception.  But we weren't sure whether the real
-                    # problem was the exception we got or the fact
-                    # that the book wasn't available. Then we tried to
-                    # place a hold, which didn't work because the book
-                    # is currently available. That answers the
-                    # question: we should have let the first exception
-                    # go through.  Raise it now.
-                    raise loan_exception
-
-                # This shouldn't normally happen, but if it does,
-                # treat it as any other exception.
-                raise
+            # At this point, we need to check to see if the patron
+            # has reached their hold limit since we did not raise
+            # an exception for it earlier when limits were enforced.
+            at_hold_limit = self.patron_at_hold_limit(patron)
+            if not at_hold_limit:
+                try:
+                    hold_info = api.place_hold(
+                        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,
+                    )
+                except CurrentlyAvailable:
+                    if loan_exception:
+                        # We tried to take out a loan and got an
+                        # exception.  But we weren't sure whether the real
+                        # problem was the exception we got or the fact
+                        # that the book wasn't available. Then we tried to
+                        # place a hold, which didn't work because the book
+                        # is currently available. That answers the
+                        # question: we should have let the first exception
+                        # go through.  Raise it now.
+                        raise loan_exception
+
+                    # This shouldn't normally happen, but if it does,
+                    # treat it as any other exception.
+                    raise
+            else:
+                raise PatronHoldLimitReached
 
         # 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.
@@ -1186,6 +1225,12 @@ def borrow(
         if existing_loan:
             self._db.delete(existing_loan)
         __transaction.commit()
+
+        # Raise the exception of failed loan when the patron falsely believed
+        # there was an available licanse at the top of the hold queue.
+        if reserved_license_exception:
+            raise NoAvailableCopiesWhenReserved
+
         return None, hold, is_new
 
     def enforce_limits(self, patron: Patron, pool: LicensePool) -> None:
@@ -1214,6 +1259,11 @@ def enforce_limits(self, patron: Patron, pool: LicensePool) -> None:
             # limits don't apply.
             return
 
+        if not at_loan_limit and at_hold_limit:
+            # This patron can take out a loan, but not a hold. This is relevant when
+            # the patron can't place a hold but can take out a loan.
+            return
+
         if at_loan_limit and at_hold_limit:
             # This patron can neither take out a loan or place a hold.
             # Raise PatronLoanLimitReached for the most understandable

api/circulation_exceptions.py

@@ -217,6 +217,17 @@ class CannotRenew(CirculationException):
     status_code = 400
 
 
+class NoAvailableCopiesWhenReserved(CannotLoan):
+    """The patron can't check this book out because all available
+    copies are already checked out. The book is "reserved" to the
+    patron.
+    """
+
+    def as_problem_detail_document(self, debug=False):
+        """Return a suitable problem detail document."""
+        return NO_COPIES_WHEN_RESERVED
+
+
 class NoAvailableCopies(CannotLoan):
     """The patron can't check this book out because all available
     copies are already checked out.

api/controller/loan.py

@@ -22,6 +22,7 @@
     DeliveryMechanismError,
     FormatNotAvailable,
     NoActiveLoan,
+    NoAvailableCopiesWhenReserved,
     NoOpenAccessDownload,
     NotFoundOnRemote,
     OutstandingFines,
@@ -203,6 +204,11 @@ def _borrow(self, patron, credential, pool, mechanism):
             result = e.as_problem_detail_document(debug=False)
         except CannotLoan as e:
             result = CHECKOUT_FAILED.with_debug(str(e))
+        except CannotLoan as e:
+            if isinstance(e, NoAvailableCopiesWhenReserved):
+                result = e.as_problem_detail_document()
+            else:
+                result = CHECKOUT_FAILED.with_debug(str(e))
         except CannotHold as e:
             result = HOLD_FAILED.with_debug(str(e))
         except CannotRenew as e:

api/problem_details.py

@@ -56,6 +56,14 @@
     _("All licenses for this book are loaned out."),
 )
 
+# E-kirjasto
+NO_COPIES_WHEN_RESERVED = pd(
+    "http://librarysimplified.org/terms/problem/cannot-issue-loan",
+    502,
+    _("No available license."),
+    _("All copies of this book are loaned out after all. You are still next in line."),
+)
+
 NO_ACCEPTABLE_FORMAT = pd(
     "http://librarysimplified.org/terms/problem/no-acceptable-format",
     400,

tests/api/controller/test_loan.py

@@ -21,6 +21,7 @@
 from api.circulation_exceptions import (
     AlreadyOnHold,
     NoAvailableCopies,
+    NoAvailableCopiesWhenReserved,
     NoLicenses,
     NotFoundOnRemote,
     PatronHoldLimitReached,
@@ -30,6 +31,7 @@
     CANNOT_RELEASE_HOLD,
     HOLD_LIMIT_REACHED,
     NO_ACTIVE_LOAN,
+    NO_COPIES_WHEN_RESERVED,
     NOT_FOUND_ON_REMOTE,
     OUTSTANDING_FINES,
 )
@@ -1091,6 +1093,26 @@ def test_hold_fails_when_patron_is_at_hold_limit(self, loan_fixture: LoanFixture
             assert isinstance(response, ProblemDetail)
             assert HOLD_LIMIT_REACHED.uri == response.uri
 
+    def test_loan_fails_when_patron_is_at_hold_limit_and_hold_position_zero(
+        self, loan_fixture: LoanFixture
+    ):
+        edition, pool = loan_fixture.db.edition(with_license_pool=True)
+        pool.open_access = False
+        with loan_fixture.request_context_with_library(
+            "/", headers=dict(Authorization=loan_fixture.valid_auth)
+        ):
+            patron = loan_fixture.manager.loans.authenticated_patron_from_request()
+            loan_fixture.manager.d_circulation.queue_checkout(pool, NoAvailableCopies())
+            loan_fixture.manager.d_circulation.queue_hold(
+                pool, NoAvailableCopiesWhenReserved()
+            )
+            response = loan_fixture.manager.loans.borrow(
+                pool.identifier.type, pool.identifier.identifier
+            )
+            assert isinstance(response, ProblemDetail)
+            assert NO_COPIES_WHEN_RESERVED.uri == response.uri
+            assert 502 == response.status_code
+
     def test_borrow_fails_with_outstanding_fines(
         self, loan_fixture: LoanFixture, library_fixture: LibraryFixture
     ):

tests/api/test_circulationapi.py

@@ -864,30 +864,40 @@ def patron_at_hold_limit(self, patron):
         assert patron == circulation.patron_at_hold_limit_calls.pop()
         assert pool == api.availability_updated.pop()
 
-        # Sub-test 3: patron is at hold limit but not loan limit
+        # Sub-test 4: patron is at hold limit but not loan limit
         #
         circulation.at_loan_limit = False
         circulation.at_hold_limit = True
 
-        # If the book is not available, we get PatronHoldLimitReached
+        # Test updated: If the book is not available, we should NOT yet
+        # raise PatronHoldLimitReached. The patron is at their hold limit
+        # but they may be trying to take out a loan on a book that is
+        # reserved for them (hold position 0). We want to let them proceed
+        # at this point.
         pool.licenses_available = 0
-        with pytest.raises(PatronHoldLimitReached) as hold_limit_info:
+        try:
             circulation.enforce_limits(patron, pool)
-        assert 12 == hold_limit_info.value.limit
+        except PatronHoldLimitReached:
+            assert False, "PatronHoldLimitReached is raised when it shouldn't"
+        else:
+            # If no exception is raised, the test should pass
+            assert True
 
         # Reaching this conclusion required checking both patron
-        # limits and asking the remote API for updated availability
+        # limits. The remote API isn't queried for updated availability
         # information for this LicensePool.
         assert patron == circulation.patron_at_loan_limit_calls.pop()
         assert patron == circulation.patron_at_hold_limit_calls.pop()
-        assert pool == api.availability_updated.pop()
+        assert [] == api.availability_updated
 
         # If the book is available, we're fine -- we're not at our loan limit.
+        # The remote API isn't queried for updated availability
+        # information for this LicensePool.
         pool.licenses_available = 1
         circulation.enforce_limits(patron, pool)
         assert patron == circulation.patron_at_loan_limit_calls.pop()
         assert patron == circulation.patron_at_hold_limit_calls.pop()
-        assert pool == api.availability_updated.pop()
+        assert [] == api.availability_updated
 
     def test_borrow_hold_limit_reached(
         self, circulation_api: CirculationAPIFixture, library_fixture: LibraryFixture
@@ -902,16 +912,14 @@ def test_borrow_hold_limit_reached(
         other_pool = circulation_api.db.licensepool(None)
         other_pool.open_access = False
         other_pool.on_hold_to(circulation_api.patron)
+        # The patron wants to take out a loan on another title which is not available.
+        circulation_api.remote.queue_checkout(NoAvailableCopies())
 
-        # The patron wants to take out a loan on an unavailable title.
-        circulation_api.pool.licenses_available = 0
         try:
             self.borrow(circulation_api)
         except Exception as e:
-            # The result is a PatronHoldLimitReached configured with the
-            # library's hold limit.
+            # The result is a PatronHoldLimitReached.
             assert isinstance(e, PatronHoldLimitReached)
-            assert 1 == e.limit
 
         # If we increase the limit, borrow succeeds.
         library_fixture.settings(circulation_api.patron.library).hold_limit = 2
@@ -930,6 +938,49 @@ def test_borrow_hold_limit_reached(
         loan, hold, is_new = self.borrow(circulation_api)
         assert hold != None
 
+    def test_borrow_no_available_copies_and_update_existing_hold(
+        self, circulation_api: CirculationAPIFixture, library_fixture: LibraryFixture
+    ):
+        """
+        The hold limit is 1, and the patron has a hold with position 0 in the hold queue on a book they're
+        trying to checkout. When the patron tries to borrow the book but it turns out to not be available, the
+        hold is updated to have a new end date with all other properties unchanged.
+        The circulation information for the book is immediately updated, to reduce the risk that other patrons
+        would encounter the same problem. Finally, the patron gets a NoAvailableCopiesWhenReserved exception.
+        """
+
+        # The hold limit is 1
+        library_fixture.settings(circulation_api.patron.library).hold_limit = 1
+
+        # The patron has a hold with position 0 in the hold queue
+        existing_hold, is_new = circulation_api.pool.on_hold_to(
+            circulation_api.patron,
+            start=self.YESTERDAY,
+            end=self.TOMORROW,
+            position=0,
+        )
+        original_hold_end_date = existing_hold.end
+
+        # The patron wants to take out their hold for loan but which turns out to not be available.
+        circulation_api.remote.queue_checkout(NoAvailableCopies())
+
+        # The patron tries to borrow the book but gets a NoAvailableCopiesWhenReserved exception
+        try:
+            self.borrow(circulation_api)
+        except Exception as e:
+            assert isinstance(e, NoAvailableCopiesWhenReserved)
+
+        # Nonetheless, the hold is updated to have a new extended end date.
+        assert existing_hold.position == 0
+        assert (
+            existing_hold.end != original_hold_end_date
+        )  # The updated hold should have a new end date.
+        # When NoAvailableCopies was raised, the circulation
+        # information for the book was immediately updated, to reduce
+        # the risk that other patrons would encounter the same
+        # problem.
+        assert [circulation_api.pool] == circulation_api.remote.availability_updated_for
+
     def test_fulfill_errors(self, circulation_api: CirculationAPIFixture):
         # Here's an open-access title.
         collection = circulation_api.db.collection(