Summary: | An item can be renewed even if there are holds waiting to be filled | ||
---|---|---|---|
Product: | Koha | Reporter: | Joonas Kylmälä <joonas.kylmala> |
Component: | Circulation | Assignee: | Bugs List <koha-bugs> |
Status: | RESOLVED DUPLICATE | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | andreas.jonsson, gmcharlt, hans.palsson, jonathan.druart, kyle.m.hall, nick, nugged |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15524 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11634 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28078 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Bug 26659: Fix incorrect comparison in checkout renewability
Bug 26659: Fix incorrect comparison in checkout renewability Bug 26659: Fix incorrect comparison in checkout renewability |
Description
Joonas Kylmälä
2020-10-12 11:49:27 UTC
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. *** |