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';
Created attachment 111517 [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.
This patch breaks lots of tests in t/db_dependent/Circulation.t so needs more work.
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.
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.
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>
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
(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?
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.
Yes, there are scenarios when multiple reserves has to be made on one biblio, to put it short.
Could we have this ready for 20.11?
(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.
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.
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.
*** Bug 27065 has been marked as a duplicate of this bug. ***
This should be duplicate of bug 28013. Closing. *** This bug has been marked as a duplicate of bug 28013 ***