Bug 6976 - User hold limit can be avoided by users in OPAC
Summary: User hold limit can be avoided by users in OPAC
Status: REOPENED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high major (vote)
Target Milestone: ---
Assignee: Colin Campbell
QA Contact: Mason James
URL:
Whiteboard:
Keywords:
Depends on: 4045
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-05 16:41 UTC by Colin Campbell
Modified: 2018-12-24 14:59 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Proposed Patch (4.24 KB, patch)
2011-11-21 17:46 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 6976 - User hold limit can be avoided by users in OPAC (4.72 KB, patch)
2012-02-23 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Updated patch (1.93 KB, patch)
2012-08-28 15:13 UTC, Colin Campbell
Details | Diff | Splinter Review
Correct updated patch (6.89 KB, patch)
2012-08-28 15:49 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 6976 Close loophole allowing borrowers extra holds via opac (6.93 KB, patch)
2012-08-29 00:03 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 Colin Campbell 2011-10-05 16:41:00 UTC
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
Comment 1 Nicole C. Engard 2011-10-05 16:43:44 UTC

*** This bug has been marked as a duplicate of bug 4045 ***
Comment 2 Ian Walls 2011-10-13 14:24:34 UTC
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.
Comment 3 Colin Campbell 2011-10-13 15:51:53 UTC
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
Comment 4 Colin Campbell 2011-11-21 17:46:07 UTC Comment hidden (obsolete)
Comment 5 Colin Campbell 2011-11-21 17:48:13 UTC
Patch as deployed and tested on site. Reopening this bug as 4045 covers some other issues as well
Comment 6 Liz Rea 2011-11-21 18:48:17 UTC
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?
Comment 7 Liz Rea 2011-11-21 18:49:00 UTC
that should read, I was able to circumvent the limits.
Comment 8 Katrin Fischer 2011-11-22 08:24:43 UTC
Do the circulation matrix holds allowed work at all? (Bug 4045)
Comment 9 Chris Cormack 2011-12-01 18:48:24 UTC
Liz, should this be failed qa then? Judging by your comments
Comment 10 Melia Meggs 2011-12-08 23:56:35 UTC
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."
Comment 11 Liz Rea 2011-12-09 01:47:23 UTC
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.
Comment 12 Colin Campbell 2011-12-11 14:03:54 UTC
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
Comment 13 Kyle M Hall 2012-02-23 14:36:11 UTC Comment hidden (obsolete)
Comment 14 Jared Camins-Esakov 2012-03-17 09:52:30 UTC
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?
Comment 15 Kyle M Hall 2012-03-19 14:02:34 UTC
(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?
Comment 16 Francois Charbonnier 2012-06-10 09:09:27 UTC
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".
Comment 17 Liz Rea 2012-06-15 15:39:30 UTC
I want to see both patches get anot
Comment 18 Liz Rea 2012-06-15 15:41:29 UTC
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.
Comment 19 Chris Cormack 2012-07-01 00:53:03 UTC
Liz, did you get a chance to look?
Comment 20 Joy Nelson 2012-08-17 19:52:47 UTC
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
Comment 21 Colin Campbell 2012-08-28 15:13:52 UTC Comment hidden (obsolete)
Comment 22 Colin Campbell 2012-08-28 15:18:34 UTC
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
Comment 23 Colin Campbell 2012-08-28 15:49:41 UTC Comment hidden (obsolete)
Comment 24 Chris Cormack 2012-08-29 00:03:35 UTC
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>
Comment 25 Chris Cormack 2012-08-29 00:05:26 UTC
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
Comment 26 Mason James 2012-09-14 06:37:06 UTC
> 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
Comment 27 Paul Poulain 2012-09-14 13:27:45 UTC
Patch pushed to master

I had some trouble reproducing the behaviour (defining issuingrules correctly is sometimes tricky ;-) )
Comment 28 Chris Cormack 2012-09-16 08:01:03 UTC
Pushed to 3.8.x, will be in 3.8.5
Comment 29 Owen Leonard 2012-10-15 19:10:52 UTC
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.
Comment 30 Jonathan Druart 2015-07-14 11:38:15 UTC
Still valid.
Comment 31 Nick Clemens 2018-12-24 13:59:58 UTC
(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
Comment 32 Nick Clemens 2018-12-24 14:59:06 UTC
(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'