[Bug 26659] New: An item can be renewed even if there are holds waiting to be filled
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Bug ID: 26659 Summary: An item can be renewed even if there are holds waiting to be filled Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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';
-- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Severity|normal |major Assignee|koha-bugs@lists.koha-commun |joonas.kylmala@helsinki.fi |ity.org | Status|NEW |ASSIGNED Keywords| |rel_20_11_target -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #1 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 111517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111517&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED --- Comment #2 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- This patch breaks lots of tests in t/db_dependent/Circulation.t so needs more work. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111517|0 |1 is obsolete| | --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 111522 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111522&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=11634 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Hans Pålsson <hans.palsson@hkr.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hans.palsson@hkr.se -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111522|0 |1 is obsolete| | --- Comment #5 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 112402 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112402&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |nick@bywatersolutions.com --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #7 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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?
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #9 from Hans Pålsson <hans.palsson@hkr.se> --- Yes, there are scenarios when multiple reserves has to be made on one biblio, to put it short. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_11_target | --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Could we have this ready for 20.11? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_11_target -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|joonas.kylmala@helsinki.fi |koha-bugs@lists.koha-commun | |ity.org --- Comment #11 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #12 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 --- Comment #13 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27032 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 [Bug 27032] CanBookBeRenewed is not understandable and needs refactoring -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andreas.jonsson@kreablo.se --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- *** Bug 27065 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_11_target |rel_21_05_candidate -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28078 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |DUPLICATE Status|Failed QA |RESOLVED --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- This should be duplicate of bug 28013. Closing. *** This bug has been marked as a duplicate of bug 28013 *** -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|27032 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 [Bug 27032] CanBookBeRenewed is not understandable and needs refactoring -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_05_candidate | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org