Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ekir 213 enable loan when not at loan limit #97

Merged
merged 22 commits into from
Sep 20, 2024

Conversation

natlibfi-kaisa
Copy link
Contributor

@natlibfi-kaisa natlibfi-kaisa commented Sep 13, 2024

Description

Circulation will give response 502 NoAvailableCopiesWhenReserved when a patron tries to checkout a loan that was "reserved" for them but turns out not to be available. Their hold is updated with a new end date.

Motivation and Context

When the patron was at the top of a license pool's hold queue and at their hold limit, circulation gave response 403 that they had reached their hold limit when they tried to checkout a presumably (and notified) available license. This was, based on comments and tests, a by design functionality.

When a patron tries to loan/hold a book, the request goes to route "/works/<identifier_type>/path:identifier/borrow" and the process loan.py <-> circulationapi.py <-> odl.py.

This PR changes functionality:

  • 403 PatronHoldLimitReached is not raised in enforce_limits() when a patron is not at their loan limit but at their hold limit so that we can check the situation more carefully later in borrow().

  • When checkout() (in odl.py which is the api we're using) in borrow() is run and gives a NoAvailableCopies, the code checks if the patron has an existing hold on that license at hold position 0, updates the hold with a new end date and updates the license's availability information. Finally, borrow() raises a NoAvailableCopiesWhenReserved exception at the end when it's been set to True during the hold update.

  • Borrow() is called in loans.py _checkout() which raises 502 NoAvailableCopiesWhenReserved except with a new NO_COPIES_WHEN_RESERVED problem detail to the client.

How Has This Been Tested?

This is purely based on unit tests that check hold limits and hold positions. This may be tricky to test in practice due to circulation not being fully in sync with the remote which causes this situation of a license being checked out by somebody else (most likely) == not available.

Checklist

  • Internal documentation updated.
  • All new and existing tests passed.

@natlibfi-kaisa natlibfi-kaisa force-pushed the EKIR-213-Enable-loan-when-not-at-loan-limit branch from 93cf672 to 8b69b4e Compare September 18, 2024 09:53
@natlibfi-kaisa natlibfi-kaisa marked this pull request as ready for review September 18, 2024 10:05
@natlibfi-kaisa natlibfi-kaisa force-pushed the EKIR-213-Enable-loan-when-not-at-loan-limit branch from 488df6d to de75560 Compare September 18, 2024 10:21
@natlibfi-kaisa natlibfi-kaisa force-pushed the EKIR-213-Enable-loan-when-not-at-loan-limit branch from de75560 to 2feba5b Compare September 18, 2024 10:24
Copy link
Contributor

@NatLibFi-JoonaKupe NatLibFi-JoonaKupe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but something to think about: How does this line up with recent changes by a distributor, are all checks still needed now or in the future? Handling the scenario seems to be a good idea anyway.

@natlibfi-kaisa
Copy link
Contributor Author

Hopefully, the situation improves but I guess we still want to inform the patrons of the potential error of an unavailable license. At least it will be less confusing to them compared to the current error message they're getting.

@natlibfi-kaisa natlibfi-kaisa merged commit d10ee45 into main Sep 20, 2024
24 checks passed
@natlibfi-kaisa natlibfi-kaisa deleted the EKIR-213-Enable-loan-when-not-at-loan-limit branch September 20, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants