Problem as reported: borrowers are only allowed 5 holds. If holds are done individually, it is obeying that law i.e. X has held 5 separate books – when I tried to do a 6th book it told me that I could not place a hold. However, if a borrower checks the catalogue, gets the intial list up and clicks items on the “results” box and then clicks “place hold” they can hold more than 5 items – I have been able to hold 8 (and I suspect could do more) Looks like the logic is a bit flawed when placing multiple holds as the variable canreserve is not reset for each iteration of the loop. Testing fix on site patch to follow
*** This bug has been marked as a duplicate of bug 4045 ***
Colin, Any word on the fix for this? I don't mean to be a nag, just curious if it was still on your radar after this was marked as a duplicate.
The person who was going to confirm the fix works in production was away. They are now back and I'll submit the patch on confirmation
Created attachment 6371 [details] [review] Proposed Patch
Patch as deployed and tested on site. Reopening this bug as 4045 covers some other issues as well
It looks like this uses MaxReserves syspref, but not the per itemtype/patron category rules that are defined in the circ matrix. I was able to circumvent the issue with a maxreserves set to 100, but the circ rules set to 5 - I wonder if a check needs to be added to verify the limit specified in the circ rules as well?
that should read, I was able to circumvent the limits.
Do the circulation matrix holds allowed work at all? (Bug 4045)
Liz, should this be failed qa then? Judging by your comments
With maxreserves sys pref set to allow 100 holds and circ rules set to allow 5 holds (as in Liz's example), I was also able to place 6 holds. So I have changed the status to "Failed QA."
I think failed QA is ok - except this *is* a partial fix to the problem - neither setting works to limit holds, and both should. So, the question for me becomes - accept this as a partial fix and address the circ rules later (as we i'm sure intended) or forego this patch for a potential future, better patch. I'm not sure what's best.
Katrin's comment above is valid as this script has always checked the circ matrix limits via the C4::reserves routines but this does not restrict the user from placing the hold. The patch only addresses the users ability to circumvent the maxreserves setting
Created attachment 7832 [details] [review] Bug 6976 - User hold limit can be avoided by users in OPAC Adds a check to see if a multi-hold request would possibly exceed the maximum number of allowed holds. If it is possible, then we display an error message if the borrower has more checkboxes checked than new holds that can be placed.
Kyle, As far as I can tell, your patch just a reimplementation of the patch that Melia marked as failed QA for only addressing maxreserves. Am I missing something?
(In reply to comment #14) I cannot really comment on the previous patch, but in my opinion, adding the max holds to the circulation rules has broken the max holds system. Circ rules are a combination of Library, Patron Type and Item Type. When using item level Item Types, we cannot know if placing the hold will violate these rules in advance, as a given record could have items with different item types. The holds rules should really be pulled out of the circ rules and given a separate table where the are set only by Library and Patron Category. This patch does not address this problem. Since we cannot know if the Holds Allowed rules will be violated in advance, we must simply rely on maxreserves. If I am mistaken, or you have an idea that would rectify this situation, please let me know! > Kyle, > > As far as I can tell, your patch just a reimplementation of the patch that > Melia marked as failed QA for only addressing maxreserves. Am I missing > something?
I'm not sure I have been able to reproduce this bug. I didn't know the set up so, here are the set ups I used to test this : The count of "Holds allowed" within the circulation matrix (Library / Category code / Item type) is : All / All / All = 3 All / Librarian / Book = 3 Library A / All / All = 3 Library A / Librarian / Book = 3 I worked with 4 system preferences : * maxreserves * item-level_itypes * OPACItemHolds * ReservesControlBranch The maxreserves syspref is set up to : maxreserves = 4 Then I tried every set up possible with the three other sysprefs. I worked with 4 differents items that have : biblioitems.itemtype = Book items.itype = Book The patron I used to place hold is a Librarian from Library A. When I tested the set ups with OPACItemHolds = yes, every holds placed were on the item level. If OPACItemHolds was set up to "no", every holds placed were on the biblio level. What I noticed is that : * if koha knows the patron category code, the patron or the item library and the itemtype, the limit used is the one from the circulation matrix _BUT_ I have the wrong message : "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." * if koha doesn't know the item's library ou the item's item type, I can't place more than 4 holds. So the limit comes from the maxreserves syspref. It's logical but I think, we should use the "default" rule from the circulation matrix instead to be more flexible. The maxreserves syspref should be used only when nothing else apply. Or maybe, we could get rid of the maxreserves syspref, saying that the default rule (Library =all / Category code = all / Item type = all) is the one koha should use when nothing apply. Since, the documentation says this rule is required, I think it would logical to give it a use. Set ups and test results : 1/ item-level_itypes = biblio OPACItemHolds = no ReservesControlBranch = patron's branch -> it looks like it works but I have the wrong message "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." 2/ item-level_itypes = item OPACItemHolds = no ReservesControlBranch = patron's branch -> The control looks like it comes from "maxreserves". 3/ item-level_itypes = item OPACItemHolds = yes ReservesControlBranch = patron's branch -> it looks like it works but I have the wrong message "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." 4/ item-level_itypes = biblio OPACItemHolds = no ReservesControlBranch = item's branch -> The control looks like it comes from "maxreserves". 5/ item-level_itypes = biblio OPACItemHolds = yes ReservesControlBranch = item's branch -> it looks like it works but I have the wrong message "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." 6/ item-level_itypes = item OPACItemHolds = yes ReservesControlBranch = item's branch -> it looks like it works but I have the wrong message "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." 7/ item-level_itypes = biblio OPACItemHolds = yes ReservesControlBranch = patron's branch -> it looks like it works but I have the wrong message "Sorry, none of these items can be placed on hold". I should have "Sorry, you cannot place more than X holds." 8/ item-level_itypes = item OPACItemHolds = no ReservesControlBranch = item's branch -> The control looks like it comes from "maxreserves".
I want to see both patches get anot
Lol dangit. I want to see these patches get another look - this is no trivial issue for libraries - we can't have patrons requesting and getting items over the limits libraries have set in policy. At this point, I am convinced that it is better to look at a partial fix to get SOME limit, SOMEWHERE working. I will look at both fixes by Monday, june 18 2012.
Liz, did you get a chance to look?
Marking as Does Not Apply Auto-merging opac/opac-reserve.pl CONFLICT (content): Merge conflict in opac/opac-reserve.pl Failed to merge in the changes. Patch failed at 0001 Bug 6976 Close loophole allowing borrowers extra holds
Created attachment 11872 [details] [review] Updated patch Updated patch against current master
Have updated original patch against current master. Tested by staff at Staffordshire University Incidentally in testing found that if the patch is not in place it only took a couple of days for students to start using the loophole to abuse the system placing greater than the maxholds value
Created attachment 11874 [details] [review] Correct updated patch uploaded wrong patch this is the one on current master
Created attachment 11876 [details] [review] Bug 6976 Close loophole allowing borrowers extra holds via opac The check that the max number of reserves has not been reached needs to take account of the fact that we may have added enouch reserves to reach the limit while this script is running. Add a check against the incrementing count The variable canreserve was only used when looping through multiple holds but was not being reinitialized on every iteration make variable local to loop it is used in and initialize to zero Have used perltidy to correct the indention of the while loop as it was misleading to the reader Some numeric comparisons were using a string operator fix them before strange bugs manifest themselves The loophole manifests thus: borrowers are only allowed Y holds. If holds are done individually, it is obeying that law i.e. X has held Y separate books – when I tried to do a 6th book it told me that I could not place a hold. However, if a borrower checks the catalogue, gets the intial list up and clicks items on the “results” box and then clicks “place hold” they can hold more than Y items Please enter the commit message for your changes. Lines starting Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
I have signed off on this, as it fixes one big loophole. There is no reason not to get this QAed and into master and the other loophole can be closed on bug 4045
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> patch looks good, passing QA... $ koha-qa.pl testing 1 commit(s) (applied to commit 60790d0) * 3ebd669 Bug 6976 Close loophole allowing borrowers extra holds via opac opac/opac-reserve.pl * opac/opac-reserve.pl OK
Patch pushed to master I had some trouble reproducing the behaviour (defining issuingrules correctly is sometimes tricky ;-) )
Pushed to 3.8.x, will be in 3.8.5
This doesn't appear to be fixed to me. Here's how I tested: - I have a rule set up to limit holds on the DVD item type to 5. - Log in to the OPAC as a user with no holds. - Search by item type for DVDs. - Select all results from the first page of search results - Click 'Place hold' - Click 'Place hold' on the hold confirmation page. Result: The patron has a hold on 20 different DVDs even though the limit should be 5.
Still valid.
(In reply to Jonathan Druart from comment #30) > Still valid. I cannot recreate. Set an All/All/All rule for limit 2 holds, do a search, select all, place hold - user gets two holds Cancel those Set maxreserves to 1 Search - select all - place hold - warning that I can only select one - error if I try to place all holds
(In reply to Nick Clemens from comment #31) > (In reply to Jonathan Druart from comment #30) > > Still valid. > > I cannot recreate. > > Set an All/All/All rule for limit 2 holds, do a search, select all, place > hold - user gets two holds > > Cancel those > > Set maxreserves to 1 > Search - select all - place hold - warning that I can only select one - > error if I try to place all holds NVM, set ReservesControlBranch to 'Items home library'
(In reply to Nick Clemens from comment #32) > (In reply to Nick Clemens from comment #31) > > (In reply to Jonathan Druart from comment #30) > > > Still valid. > > > > I cannot recreate. > > > > Set an All/All/All rule for limit 2 holds, do a search, select all, place > > hold - user gets two holds > > > > Cancel those > > > > Set maxreserves to 1 > > Search - select all - place hold - warning that I can only select one - > > error if I try to place all holds > > NVM, set ReservesControlBranch to 'Items home library' This may be fixed in 19.05+, I cannot recreate, can someone confirm?
(In reply to Nick Clemens from comment #33) > (In reply to Nick Clemens from comment #32) > > (In reply to Nick Clemens from comment #31) > > > (In reply to Jonathan Druart from comment #30) > > > > Still valid. > > > > > > I cannot recreate. > > > > > > Set an All/All/All rule for limit 2 holds, do a search, select all, place > > > hold - user gets two holds > > > > > > Cancel those > > > > > > Set maxreserves to 1 > > > Search - select all - place hold - warning that I can only select one - > > > error if I try to place all holds > > > > NVM, set ReservesControlBranch to 'Items home library' > > This may be fixed in 19.05+, I cannot recreate, can someone confirm? I cannot either. Closing. New bug report must be opened with steps to recreate if the bug pops up again.