Bug 26659 - An item can be renewed even if there are holds waiting to be filled
Summary: An item can be renewed even if there are holds waiting to be filled
Status: RESOLVED DUPLICATE of bug 28013
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
: 27065 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-10-12 11:49 UTC by Joonas Kylmälä
Modified: 2021-04-21 13:49 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26659: Fix incorrect comparison in checkout renewability (1.93 KB, patch)
2020-10-12 15:25 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26659: Fix incorrect comparison in checkout renewability (2.42 KB, patch)
2020-10-13 08:24 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26659: Fix incorrect comparison in checkout renewability (2.48 KB, patch)
2020-10-24 21:22 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2020-10-12 11:49:27 UTC
I'm not quite sure how to exactly reproduce the issue but if you have

- AllowRenewalIfOtherItemsAvailable enabled 
- all the books are checked out
- there is a hold to the biblio

then the people having checked out items can renew their loans for the item they have even though it should not be possible because when there is a hold and no available items then no renewals should be allowed in order to make the patrons return the item and let the person having the hold be able to checkout the item. 

The culprit seems to be the following line in C4/Circulation.pm:

> next unless CanItemBeReserved($borrowernumber,$itemnumber);

It should be corrected to

> next unless CanItemBeReserved($borrowernumber,$itemnumber)->{status} eq 'OK';
Comment 1 Joonas Kylmälä 2020-10-12 15:25:17 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2020-10-12 15:30:36 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2020-10-13 08:24:18 UTC
Created attachment 111522 [details] [review]
Bug 26659: Fix incorrect comparison in checkout renewability

The syntax used here was wrong and it allowed incorrectly to renew
checkouts which should not have been allowed because there were
holds waiting to be filled. This scenario can be reproduced at least
with using local hold groups.

To test:

1. In the circ rules for all libraries under "Default holds policy by item type" section
   select for itemtype Book the policies "From local hold group" and "patron's hold group"
2. Enable AllowRenewalIfOtherItemsAvailable
3. Create local hold group for branch A and branch B (/cgi-bin/koha/admin/library_groups.pl)
4. Create biblio with 2 items of itemtype Book, one in branch A and other in B
5. Checkout item from branch A to patron PA
6. Create bib level hold for patron PB whose home library is branch A.
7. Try go to renew now the loan for patron PA and notice it would work
8. Apply patch
9. Try to go renew the loan for patron PA and notice it doesn't work.
Comment 4 Joonas Kylmälä 2020-10-13 09:04:19 UTC
This bug seem to have been already existing when the AllowRenewalIfOtherItemsAvailable syspref was introduced in bug 11634. Already back then there was incorrect check

> if ( CanItemBeReserved( $b, $i ) ) {

and that was partly due to rebase issue caused by bug 13116 being pushed on the same day and even if there was no rebase issue there would have been the issue that if the maxreserves limit were hit but not exceeded this would have allowed the renewal incorrectly. So just adding the ->{status} eq 'OK'; fix would have fixed the rebase issue that happened many years ago but in addition to that I decided in this patch now also fix the issue if for example maxreserves is 1 and patron has one hold it should now block the renewal for the other patron.
Comment 5 Chris Cormack 2020-10-24 21:22:35 UTC
Created attachment 112402 [details] [review]
Bug 26659: Fix incorrect comparison in checkout renewability

The syntax used here was wrong and it allowed incorrectly to renew
checkouts which should not have been allowed because there were
holds waiting to be filled. This scenario can be reproduced at least
with using local hold groups.

To test:

1. In the circ rules for all libraries under "Default holds policy by item type" section
   select for itemtype Book the policies "From local hold group" and "patron's hold group"
2. Enable AllowRenewalIfOtherItemsAvailable
3. Create local hold group for branch A and branch B (/cgi-bin/koha/admin/library_groups.pl)
4. Create biblio with 2 items of itemtype Book, one in branch A and other in B
5. Checkout item from branch A to patron PA
6. Create bib level hold for patron PB whose home library is branch A.
7. Try go to renew now the loan for patron PA and notice it would work
8. Apply patch
9. Try to go renew the loan for patron PA and notice it doesn't work.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 6 Nick Clemens 2020-10-25 01:12:28 UTC
Trying to read the code, but there is a lot happening here, I feel this needs unit tests

The check for 'OK' makes sense, but some of the other statuses like 'itemAlreadyOnHold' short circuit other checks, so while it saves us time in CanItemBeReserved to bail out, I think in this instance we are actually trying to determine "CanItemFillHold", CheckReserves seems to cover some, but not all cases
Comment 7 Joonas Kylmälä 2020-10-26 09:37:52 UTC
(In reply to Nick Clemens from comment #6)
> Trying to read the code, but there is a lot happening here, I feel this
> needs unit tests
> 
> The check for 'OK' makes sense, but some of the other statuses like
> 'itemAlreadyOnHold' short circuit other checks, so while it saves us time in
> CanItemBeReserved to bail out, I think in this instance we are actually
> trying to determine "CanItemFillHold", CheckReserves seems to cover some,
> but not all cases

To do the tests and new sub CanItemFillHold I need to move some code out of CanBookBeRenewed. This week is busy so it might take me a while to get the new patches for this done, I also wouldn't mind anybody else taking in on this.

The code I identified as potential candidate for CanItemFillHold is the code in the if block that starts after the comment

>    # This item can fill one or more unfilled reserve, can those unfilled reserves
>    # all be filled by other available items?
Comment 8 Joonas Kylmälä 2020-10-26 10:16:03 UTC
While doing the refactoring I came across this comment

> # If the count of the union of the lists of reservable items for each borrower
> # is equal or greater than the number of borrowers, we know that all reserves
> # can be filled with available items. We can get the union of the sets simply
> # by pushing all the elements onto an array and removing the duplicates.

which seems to describe incorrect behaviour. This only assumes that there is one reserve per patron but there can be multiple reserves per patron to the same biblio. Please let me know if you think this behaviour of only taking into account one reserve needs to be kept intact.
Comment 9 Hans Pålsson 2020-10-26 10:39:49 UTC
Yes, there are scenarios when multiple reserves has to be made on one biblio, to put it short.
Comment 10 Jonathan Druart 2020-11-12 10:58:36 UTC
Could we have this ready for 20.11?
Comment 11 Joonas Kylmälä 2020-11-12 11:10:38 UTC
(In reply to Jonathan Druart from comment #10)
> Could we have this ready for 20.11?

It's gonna be tight if I'm to do the refactored version which fixes both of the bugs discovered here. Marking the bug report unassigned until I actually start coding this again so anybody else is free to take this meanwhile and hopefully increase the chances of getting this in for 20.11.
Comment 12 Joonas Kylmälä 2020-11-16 11:05:05 UTC
The current code also doesn't check properly whether the other items can fill hold based on the transfer rules / pickup branch so gotta fix that too.
Comment 13 Joonas Kylmälä 2020-11-16 11:45:22 UTC
It appears I cannot fix this bug in efficient manner until C4/Circulation.pm and C4/Reserves.pm are refactored to be more understandable. I will open new bug reports for the different functions refactoring. I suggest we take the patch attached to this report for 20.11 to fix one of these bugs right now since it seems to affect multiple libraries.
Comment 14 Jonathan Druart 2020-11-20 13:44:57 UTC
*** Bug 27065 has been marked as a duplicate of this bug. ***
Comment 15 Joonas Kylmälä 2021-04-08 06:53:17 UTC
This should be duplicate of bug 28013. Closing.

*** This bug has been marked as a duplicate of bug 28013 ***