Bug 4045 - No check for maximum number of allowed holds from OPAC.
Summary: No check for maximum number of allowed holds from OPAC.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 25643 6976 7030 12197
  Show dependency treegraph
 
Reported: 2010-01-14 04:02 UTC by Owen Leonard
Modified: 2020-06-01 15:11 UTC (History)
29 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 4054 - Double-clicking the 'place hold' button can result in duplicate holds (2.45 KB, patch)
2012-02-14 15:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (5.57 KB, patch)
2012-02-23 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (9.93 KB, patch)
2012-02-23 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (5.63 KB, patch)
2012-03-22 16:34 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug-4045---No-check-for-maximum-number-of-allowed-.patch (5.63 KB, patch)
2012-03-22 16:35 UTC, delaye
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds (9.93 KB, patch)
2012-06-21 14:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 4045 - Followup: No check for maximum number of allowed holds. (1.13 KB, patch)
2012-06-21 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
System preference maxreserves appears twice (87.87 KB, image/jpeg)
2012-06-22 12:16 UTC, Marc Véron
Details
Bug 4045 - No check for maximum number of allowed holds. (1.32 KB, patch)
2012-08-07 13:43 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (8.17 KB, patch)
2013-09-18 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (8.21 KB, patch)
2013-10-02 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4045: Followup for optional insertion of maxreserves (1.45 KB, patch)
2013-10-02 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (8.17 KB, patch)
2013-11-15 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045: Followup for optional insertion of maxreserves (1.45 KB, patch)
2013-11-15 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - QA Followup (683 bytes, patch)
2013-11-15 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (12.38 KB, patch)
2014-05-27 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (10.23 KB, patch)
2014-05-27 13:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (10.23 KB, patch)
2014-05-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (10.28 KB, patch)
2014-07-24 22:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 4045 - No check for maximum number of allowed holds. (10.38 KB, patch)
2014-08-01 10:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:22:31 UTC


---- Reported by oleonard@myacpl.org 2010-01-14 16:02:20 ----

At some point updatedatabase removed the "maxreserves" system preference:

$dbh->do('DELETE FROM systempreferences WHERE variable = "maxreserves";');

But the opac and staff client still checks for that preference when placing holds. From request.pl:

    if ( $number_reserves > C4::Context->preference('maxreserves') ) {
	$warnings = 1;
        $maxreserves = 1;
    }

chris fixed this in the OPAC (Bug by allowing holds if maxreserves is not set, which we could apply to the staff client:

    if ( C4::Context->preference('maxreserves') && $number_reserves > C4::Context->preference('maxreserves') ) {
        $warnings = 1;
        $maxreserves = 1;
    }

...but that is just bypasses the problem: There is no check of the "Holds Allowed" value specified in the issuingrules table.



---- Additional Comments From cnighswonger@foundations.edu 2010-02-01 17:14:34 ----

Reassigning to myself.



---- Additional Comments From cnighswonger@foundations.edu 2010-02-10 17:26:20 ----

Work from Biblibre addresses this, but will not make it into 3.2. This work should be included in 3.4.



---- Additional Comments From oleonard@myacpl.org 2010-02-10 18:01:40 ----

If the maxreserves preference stays deleted and the limits defined in issuing rules are ignored, 3.2 will ship with no way for librarians to limit the number of holds a patron can place. 



---- Additional Comments From jwagner@ptfs.com 2010-02-10 18:24:04 ----

We have some development (not yet submitted) that requires maxreserves, so if it's deleted we'd have to add it back in.  Apart from that, I really believe libraries need the option to limit number of holds.  I'd strongly suggest this syspref be retained.



---- Additional Comments From henridamien@koha-fr.org 2010-02-11 00:30:31 ----

In a development we did, the maxreserves (and delay, and holdspolicy) is included in issuingrules.
So that max reserves as such would not be required any longer.
Maybe we could share our devs and see if we can reconcile them.
ours is on a public branch on git.biblibre.com



---- Additional Comments From jwagner@ptfs.com 2010-02-11 13:06:05 ----

We'll take a look at your github and also at latest head -- we're going to have to reconcile our code to all the updates since we froze our development level. 



---- Additional Comments From jwagner@ptfs.com 2010-05-12 13:22:49 ----

Updating, see http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3507 -- I think that uses maxreserves as a backup setting.  It could probably be reconciled.





---- Additional Comments From oleonard@myacpl.org 2010-05-12 14:11:17 ----

There is still one check of C4::Context->preference("maxreserves") that I can find, in opac-reserve.pl (excluding one in Utility.pm which doesn't seem to be used)

Hold limits work inconsistently in the OPAC:

  -- If the patron has not yet exceeded the maximum allowed number of holds, it is possible to place an unlimited number of multiple holds in one transaction. Koha seems to say, "Patron has not reached the limit of allowable holds, so allow this holds transaction to take place." The system should instead allow holds only up to the number of allowable holds. This would require the interface to know the number of available holds and keep track of how many of multiple holds were selected.

  -- If the patron has reached or exceeded the maximum allowed number of holds, the OPAC will *not* block the transaction with the built in warning, "Sorry, you cannot place more than <!-- TMPL_VAR NAME="too_many_reserves" --> holds" because that warning only works with the "maxreserves" system preference. Instead the user will be shown the title(s) he has chosen, but the title(s) will be shown with the message "This title cannot be requested." The wording of this message implies that there is a problem with that particular title, not a problem with the number of holds the user has.



---- Additional Comments From oleonard@myacpl.org 2010-05-12 14:21:59 ----

Testing holds limits in the staff client:

-- When placing a single hold for a patron who has exceeded their maximum allowed number of holds, Koha warns that the users has too many holds, but does not block the transaction.

-- When placing multiple holds for a patron who has exceeded their maximum allowed number of holds, Koha reports "Cannot place hold on some items," but does not block the transaction. 



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:22 UTC  ---

This bug was previously known as _bug_ 4045 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4045

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
CC member jwagner@ptfs.com does not have an account here

Comment 1 Owen Leonard 2010-07-09 19:14:25 UTC
> There is still one check of C4::Context->preference("maxreserves") that I can
> find, in opac-reserve.pl (excluding one in Utility.pm which doesn't seem to be
> used)

Correction: request.pl in the staff client also checks for this now-defunct system preference.
Comment 2 Nicole C. Engard 2011-01-04 13:59:53 UTC
Just wanted to give this a little bump and see if the BibLibre or PTFS development to fix this is ready for QA - or close to ready yet?
Comment 3 Vitor Fernandes 2011-01-12 15:06:28 UTC
I'm another person that would like to know if this bug will corrected in near future versions of Koha?
Comment 4 Vitor Fernandes 2011-01-12 15:07:59 UTC
I'm another person that would like to know if this bug will corrected in near future versions of Koha?(In reply to comment #2)
Comment 5 Nicole C. Engard 2011-09-28 16:24:43 UTC
I just got another report of this issue so it still exists ... any ideas?
Comment 6 Nicole C. Engard 2011-10-05 16:43:44 UTC
*** Bug 6976 has been marked as a duplicate of this bug. ***
Comment 7 Nicole C. Engard 2011-10-14 13:18:42 UTC
*** Bug 7030 has been marked as a duplicate of this bug. ***
Comment 8 Kyle M Hall 2012-02-10 16:23:27 UTC
This is a strange situation we have. 

The system pref maxreserves is created during installation by installer/data/mysql/sysprefs.sql 

However, if your installation is older than 3.01.00.066, it will be removed during an upgrade.

So, if you perform a clean install a version of Koha newer than 3.01.00.066, you will still have the maxreserves syspref. But if you upgrade from below 3.01.00.066 to a version above it, you will not!

Proposed Solutions:
Option A) Add maxreserves back in via updatedatabase.pl
Option B) Find all references to maxreserves in the code, and use issuingrules.reservesallowed.

B is probably better in the long run, but the extra work may be rendered superfluous by the new holds system coming down the pipe.
Comment 9 Katrin Fischer 2012-02-10 16:49:53 UTC
I am not sure the new maxreserves from circulation rules is used anywhere - so if we are not reapairing it, we should perhaps remove it. Repairing is of course better. Do we know where/how it works exactly?
Comment 10 Kyle M Hall 2012-02-10 17:09:28 UTC
(In reply to comment #9)

I assume you are referring to issuingrules.reservesallowed? It is being used in C4::Reserves::CanItemBeReserved(), so pulling it out is not a good idea. 

The maxreserves system pref is not used anywhere in C4::Reserves at the moment. It *is* being used in reserve/request.pl, and opac/opac-reserve, and that appears to be it.

I'm thinking option B is the better option at the moment. Strip out all references to maxreserves, replace with the new issuingrules reserves, and get rid of the maxreserves line in the systemprefs sql file.

> I am not sure the new maxreserves from circulation rules is used anywhere - so
> if we are not reapairing it, we should perhaps remove it. Repairing is of
> course better. Do we know where/how it works exactly?
Comment 11 Katrin Fischer 2012-02-10 17:57:50 UTC
From the comments I got the impression it was not working at all, but seems it's not that bad. So making it work is really the better option. 
Perhaps maxreserves could be resused as a global max value?
Comment 12 Kyle M Hall 2012-02-10 18:02:04 UTC
(In reply to comment #11)

While that is certainly a possibility, I don't know if its usefulness would outweigh the extra confusion and complexity it may add. It would be great to get some more opinions.

> From the comments I got the impression it was not working at all, but seems
> it's not that bad. So making it work is really the better option. 
> Perhaps maxreserves could be resused as a global max value?
Comment 13 Liz Rea 2012-02-10 18:05:45 UTC
I think we should make maxreserves go away and use the issuingrules one - it makes no sense to me to keep maxreserves if we don't actually intend for it to be used.
Comment 14 Katrin Fischer 2012-02-10 18:47:40 UTC
I agree, make it go away and use the circulation rules in all places. 
Keeping it was only an idea, really not insisting on it.
Comment 15 Kyle M Hall 2012-02-10 19:05:54 UTC
(In reply to comment #14)

Sounds like we have a plan. It never hurts to throw ideas out there. Discussion is important to making Koha as good as possible!

> I agree, make it go away and use the circulation rules in all places. 
> Keeping it was only an idea, really not insisting on it.
Comment 16 Kyle M Hall 2012-02-14 12:52:01 UTC
I've been looking at the code, and there seems to be a fundamental flaw with having having the the maximum reserves in the issuingrules table. Issuing rules are defined by a combination of Branch Code, Patron Category, and Item Type. 

The problem is the item type. Most holds are placed at a bibliographic level, and bibs in and of themselves, have no item type. Each bib can have attached items of multiple item types. 

Furthermore, there was also a new system pref added, ReservesControlBranch, which is supposed to define whether to use the patron's home library or the item's home library, again, there can be multiple items each with a different home library attached to a bib.

So, unless all item's attached to a bib are not hold-able based on the issuing rules, we cannot disallow a bib level hold to be placed on the record.

Of Note: It does appear that the smart rules correctly disallows the placing of item level holds, but does nothing for bib level holds. Even if all the items on a record disallow item level holds, if a borrower has a bib level hold on the record, and one of those items is returned, Koha will try to use it to fulfill the hold ( even though it was specifically not hold-able ).

I am reticent to remove issuingrules.holdsallowed, as that would be going backwards, not forwards. Removing this would probably cause serious problems for any library that has been using it.

Here is the best solution I can come up with:
1) Re-add maxreserves ( renamed to MaxReserves ) to updatadatabase.pl

2) Use the hybrid approach that was previously decided against. Really, there should be a way to define a maximum total number of holds, even if there is a specific holds limit for a given Library/Patron/Item Type combination.

3) When an item was returned, and is about to fill a hold for a borrower, check to see if that item was specifically hold-able for that person or not. If not, either skip trying to use it to fulfill a hold, or keep going down the list of people with holds for that bib until we find one that could have put an item level hold on it, or until there is no one left.

The commit for this was added on Aug 24, 2009, in commit 3c741d2376e939dea0554a05eddd4f9e9b2d9449. It's amazing that this went unreported for a year, and went three years without a solution.
Comment 17 Katrin Fischer 2012-02-14 14:19:40 UTC
Hi Kyle, 

I can only suspect that not a lof of people have used it, because they didn't trust it to work.

I think things are even a bit more complicated here:

We have an itemtype on biblio level, but I think it should only be checked for holds, when the system preference item-level_itypes is set to biblio record.
So in that case it can work correctly for biblio level holds, because all items on the record are of the same item type. So both item level and biblio level hold will work here.

When you use item level itemtypes, things get a lot more complicated.
I started testing the patch that moves AllowOnShelfHolds to issuingrules and the problems here are similar I think. We allow a biblio level hold, if at least one of the items can be put on hold for the patron. If he missed items on the shelf and the library doesn't offer to get those items for the patrons, it will mean the patron has to wait longer, but the hold will still be filled at some point (If I didn't miss something here, and I feel like I did).

If we follow the same approach for maxreserves it means that as long as at least one item is 'holdable' for the patron, it will be possible to place an item level hold on that item and a biblio level hold on the record. 

The problem with 3) is, that at the time the item will be checked in, the conditions can have changed totally. The numbers of holds for that patron might look totally different at the time one item is returned. While it was holdable when the patron placed the hold, it might no longer be holdable when it's returnd (and the other way round). 

Holds may have been filled in the meantime and other holds placed or cancelled. For this I think we can't really check on return - we must determine when placing the hold and then it's ok or not. Perhaps for use with item level itypes the current approach is too granular?

I am thinking about a solution similar to "Total Current Checkouts Allowed". That would mean combinations of branch and patron category. And make the branch be the branch specified in ReservesControlBranch. 
I think that could work, but I might be wrong and missing some major things.

Hope that makes sense.
Comment 18 Kyle M Hall 2012-02-14 14:37:33 UTC
(In reply to comment #17)

I was actually thinking a new table using only patron category and branch would be the ideal solution. I was hoping to fix it without removing holdsallowed from the issuingrules table, but I think that approach is just untenable at this point. I should be able to create the new table/columns and import the issuingrules.holdsallowed where possible.
Comment 19 Katrin Fischer 2012-02-14 14:42:09 UTC
Perhaps maxreserves in issuingrules could be dependent on the item-level_itypes setting? I am not sure how much of it currently works or doesn't work, but a bit worried about breaking it for libraries.
Comment 20 Kyle M Hall 2012-02-14 14:56:49 UTC
(In reply to comment #19)

That's not a bad idea. How about, if item-level_itypes is set to biblio level, use issuingrules.holdsallowed, otherwise use maxreserves?

A holds rewrite is somewhere in the future, so this may be the quickest solution until then. There is no use writing a ton of new code that will be tossed out in a short time period.

> Perhaps maxreserves in issuingrules could be dependent on the item-level_itypes
> setting? I am not sure how much of it currently works or doesn't work, but a
> bit worried about breaking it for libraries.
Comment 21 Katrin Fischer 2012-02-14 15:00:24 UTC
Is there a timeline for the development?
Comment 22 Kyle M Hall 2012-02-14 15:34:02 UTC
(In reply to comment #21)

I sent an email to Chris Cormack to find out. I believe he is the person working on it.

> Is there a timeline for the development?
Comment 23 Kyle M Hall 2012-02-14 15:56:24 UTC Comment hidden (obsolete)
Comment 24 Chris Cormack 2012-02-14 17:33:35 UTC
(In reply to comment #22)
> (In reply to comment #21)
> 
> I sent an email to Chris Cormack to find out. I believe he is the person
> working on it.
> 
> > Is there a timeline for the development?

Timelines for software development, bwahaha

But seriously look at http://bugs.koha-community.org/bugzilla3/showdependencytree.cgi?id=5609&hide_resolved=0
Comment 25 Kyle M Hall 2012-02-23 13:42:32 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-02-23 14:23:20 UTC Comment hidden (obsolete)
Comment 27 Liz Rea 2012-03-02 21:20:00 UTC
So to test this we need to 

* make sure multi holds beyond maxreserves are not allowed
* make sure single holds that would put you over maxreserves are disallowed

in both intranet and OPAC

is that all?
Comment 28 Laurence Rault 2012-03-21 09:06:26 UTC
Tested in following conditions : 

maxreserves = 3

reservation rules for my library : 
2 serials
2 books

impossible to put a hold unless some rules are defined for default library, so rule added :  default library / default itemtype / default patron : 5 

holds made : 
staff : 
2 serials OK 
+ 1 book : OK (total = 3)
+ 1 book  : alert message 'cannot place hold' (coming from maxreserves)  without explanation, and I am able to force and validate the 4th hold
When the limit comes from issuingrules, hold can't be forced

opac : 
multiple holds at once : 2 serials + 2 books from basket : no alert at all

So I don't think it is what is wanted.
Comment 29 Leila 2012-03-21 10:01:28 UTC
Hello

This can't be try with a sandbox : we have to modify the database.
Sorry...
Comment 30 delaye 2012-03-22 16:31:41 UTC
I have test in a sandbox (test2)

With 3 patrons :
ADH1, ADH2 and ADH3

The Holds rules :
2 books
3 DVD

System preference maxreserve = 3

the patron ADH1 have ckeck out the items (2 books an 3 DVD)

with this items
holds made : 
In staff with the patron ADH2: 
2 book OK 
+ 1 DVD : OK (total = 3)
+ 1 DVD  : alert message 'cannot place hold' (coming from maxreserves)  without explanation, and I am able to force and validate the 4th hold
When the limit comes from issuingrules, hold can't be forced

In opac : 
with the patron ADH3: 
2 DVD OK 
+ 1 book : OK (total = 3)
+ 1 book  : alert message 'cannot place hold' (coming from maxreserves)  without explanation, and I am able to force and validate the 4th hold
When the limit comes from issuingrules, hold can't be forced

so it's ok for me
Comment 31 Paul Poulain 2012-03-22 16:34:17 UTC Comment hidden (obsolete)
Comment 32 delaye 2012-03-22 16:35:43 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2012-03-23 10:29:03 UTC
Whow, it seems, there was a problem on the signed off patch.

Appearly, the tested patch is 

  Bug 4045 - No check for maximum number of allowed holds. (9.93 KB, patch)
  2012-02-23 14:23 UTC, Kyle M Hall (in comment 25)

This is not the one which has been signed off by delaye.

I think the right patch to test is :
  Bug 4045 - No check for maximum number of allowed holds. (5.57 KB, patch)
  2012-02-23 13:42 UTC, Kyle M Hall (in comment 26)

QA Comments: An entry in admin/preferences/ is missing for the new system preference.

Marked as Failed QA.

(Moreover, this patch (comment 25) should be rebased, there is a conflict in updatedatabase.pl)
Comment 34 Jonathan Druart 2012-06-21 14:51:37 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2012-06-21 14:55:24 UTC Comment hidden (obsolete)
Comment 36 Marc Véron 2012-06-22 12:16:08 UTC
Created attachment 10430 [details]
System preference maxreserves appears twice

With the second patch, the system preference maxreserve appears twice.
Comment 37 Jonathan Druart 2012-06-25 07:33:59 UTC
I mark it as obsolete
Comment 38 Jonathan Druart 2012-06-25 07:34:28 UTC
Comment on attachment 10412 [details] [review]
Bug 4045 - Followup: No check for maximum number of allowed holds.

>From a9fa989ebc8a95508309d9b77d1ac9a392bec06c Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@biblibre.com>
>Date: Thu, 21 Jun 2012 16:55:41 +0200
>Subject: [PATCH] Bug 4045 - Followup: No check for maximum number of allowed
> holds.
>
>Adds the systempreference in .pref file
>---
> .../en/modules/admin/preferences/circulation.pref  |    4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>index a54e3c5..2341133 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>@@ -370,6 +370,10 @@ Circulation:
>                   yes: Allow
>                   no: "Don't allow"
>             - holds to be suspended from the OPAC.
>+        -
>+            - pref: maxreserves
>+              class: integer
>+            - System-wide maximum number of holds a patron can place
>     Fines Policy:
>         -
>             - Calculate fines based on days overdue
>-- 
>1.7.7.3
Comment 39 Liz Rea 2012-08-07 13:43:37 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2012-08-09 12:34:18 UTC
Liz, the patch you submitted only removes an empty line. Is it the patch you wanted to submit?
Comment 41 Joy Nelson 2012-08-17 21:20:04 UTC
I was able to apply this patch (a merge conflict with updatedatabase.pl to resolve first)

In testing, the maxreserves value was set to 9.  

I was not able to place holds for over 9 items.  
I was not able to place holds for exactly 9 items.
I could place holds for 8 items.

I attempted to place a hold on one more item and received two messages on one screen:

message1:
Sorry, you cannot place holds on all these items. You can only place 1 more hold(s). Please choose the items you wish to be held.

message2:
Sorry, none of these items can be placed on hold.
Comment 42 Mason James 2012-09-14 06:48:48 UTC
(In reply to comment #41)
> I was able to apply this patch (a merge conflict with updatedatabase.pl to
> resolve first)
> 
> In testing, the maxreserves value was set to 9.  
> 
> I was not able to place holds for over 9 items.  
> I was not able to place holds for exactly 9 items.
> I could place holds for 8 items.
> 
> I attempted to place a hold on one more item and received two messages on
> one screen:
> 
> message1:
> Sorry, you cannot place holds on all these items. You can only place 1 more
> hold(s). Please choose the items you wish to be held.
> 
> message2:
> Sorry, none of these items can be placed on hold.

ok, so the messages combination is glitchy :/

if 'none of these items can be placed on hold.' 
there should be no message to 'choose the items you wish to be held.'

everyone, changing status from 'failed qa' to 'in discussion'
Comment 43 Owen Leonard 2013-05-06 13:27:49 UTC
Why "In discussion?" Sounds to me like a correction is needed?
Comment 44 Kyle M Hall 2013-09-18 12:35:08 UTC Comment hidden (obsolete)
Comment 45 Bernardo Gonzalez Kriegel 2013-09-19 11:33:00 UTC
In my case I have maxreserves preference, so updatedatabase gives

   Duplicate entry 'maxreserves' for key 'PRIMARY'

I think that an INSERT IGNORE is better.
Comment 46 I'm just a bot 2013-09-24 05:47:32 UTC
Patch applied cleanly, go forth and signoff
Comment 47 Marcel de Rooy 2013-10-02 12:04:13 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2013-10-02 12:04:38 UTC
trivial rebase
Comment 49 Marcel de Rooy 2013-10-02 12:10:44 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2013-10-02 12:19:03 UTC
Global symbol "$warnings" requires explicit package name at /usr/share/koha/testclone/reserve/request.pl line 132.
Global symbol "$messages" requires explicit package name at /usr/share/koha/testclone/reserve/request.pl line 166.
Global symbol "$warnings" requires explicit package name at /usr/share/koha/testclone/reserve/request.pl line 168.
Execution of /usr/share/koha/testclone/reserve/request.pl aborted due to compilation errors.

Failed QA
Comment 51 Marcel de Rooy 2013-10-02 14:37:10 UTC
Just another question in this regard:
dbrev 3.01.00.066: should the delete statement of maxreserves here be undone? 

Or would that present problems for those upgrading from below this dbrev to some version without this patch? Looks like we offer the choice bad or worse??
Comment 52 mshafer@ckls.org 2013-11-05 20:27:28 UTC
With 43 libraries that have 43 different circ rules and holds limits, I am encouraging the further development of this issue. Some libraries need to limit the holds their patrons can place more than other libraries.

M Shafer
Central Kansas Library System
Comment 53 Kyle M Hall 2013-11-15 14:48:08 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2013-11-15 14:48:16 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2013-11-15 14:48:22 UTC Comment hidden (obsolete)
Comment 56 Owen Leonard 2013-11-15 15:39:15 UTC
I'm not sure what the test plan is for this, but here's what I just tried in the staff client:

 - Circ rule: NPL patrons of type J limited to 1 hold

   - Can place multiple holds at once which exceed circ rule
   - Can place multiple holds at once which exceed maxreserves
   - Single holds not blocked until maxreserves limit is reached

- Circ rule: All patrons limited to 1 DVD hold

   - Can place multiple holds at once which exceed circ rule
   - Can place multiple holds at once which exceed maxreserves
   - Single holds not blocked until maxreserves limit is reached

- Circ rule: Staff patrons limited to 1 Audio book hold

   - Can place multiple holds at once which exceed circ rule
   - Can place multiple holds at once which exceed maxreserves
   - Single holds not blocked until maxreserves limit is reached

In summary: Circulation rule hold limits by patron type, item type, and/or branch do not work. The maxreserves limit rule only works if you place holds one at a time.
Comment 57 Kyle M Hall 2014-01-09 19:24:40 UTC
Owen, how do these results differ from the same tests pre-patch? I think it's safe to say hold rules are generally on the fritz. The issue is that the code between opac and intranet holds is not unified. It needs to be done, but it's gonna be a lot of work. If this patch corrects the holds behavior in at least one case and doesn't break it in others, I think it should still be pushed.

(In reply to Owen Leonard from comment #56)
> I'm not sure what the test plan is for this, but here's what I just tried in
> the staff client:
> 
>  - Circ rule: NPL patrons of type J limited to 1 hold
> 
>    - Can place multiple holds at once which exceed circ rule
>    - Can place multiple holds at once which exceed maxreserves
>    - Single holds not blocked until maxreserves limit is reached
> 
> - Circ rule: All patrons limited to 1 DVD hold
> 
>    - Can place multiple holds at once which exceed circ rule
>    - Can place multiple holds at once which exceed maxreserves
>    - Single holds not blocked until maxreserves limit is reached
> 
> - Circ rule: Staff patrons limited to 1 Audio book hold
> 
>    - Can place multiple holds at once which exceed circ rule
>    - Can place multiple holds at once which exceed maxreserves
>    - Single holds not blocked until maxreserves limit is reached
> 
> In summary: Circulation rule hold limits by patron type, item type, and/or
> branch do not work. The maxreserves limit rule only works if you place holds
> one at a time.
Comment 58 Owen Leonard 2014-01-10 19:22:02 UTC
(In reply to Kyle M Hall from comment #57)
> Owen, how do these results differ from the same tests pre-patch?

I think a better question from the tester's point of view is: What problem is this patch supposed to fix? Any small improvement is good, and a test plan will tell me how to verify it.
Comment 59 Kyle M Hall 2014-05-27 13:11:38 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2014-05-27 13:19:30 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2014-05-27 13:25:37 UTC Comment hidden (obsolete)
Comment 62 Bernardo Gonzalez Kriegel 2014-05-27 20:05:16 UTC
Testing:

> Test Plan:
> 1) Apply this patch
> 2) Run updatedatabase
> 3) Set maxreserves to 3, set opactheme to bootstrap
> 4) Log into the opac as a patron
> 5) Place 3 holds
> 6) Attempt to place a 4th hold
> 7) Note you get an error message and cannot place a forth hold
> 8) Delete two of those holds
> 9) Attempt to place 3 or more holds as a multi-hold
> 10) You should see a warning that you cannot place this many holds
All Ok up to this point

> 11) Try to anyway
> 12) You should see an alert to tell you to reduce the number of holds
>     you are placing.
Here I don't see an alert, but "ERROR: Internal error: incomplete hold request."

On logs:
opac-reserve.pl: Use of uninitialized value $biblionumbers in split at /home/bgkriegel/kohaclone/opac/opac-reserve.pl line 110

No holds placed

> 13) Reduce the number for holds you are placing to 2
> 14) Your holds should now be placed
Final two Ok

Is that error message the expected result?
Comment 63 Christopher Brannon 2014-05-28 21:22:39 UTC
Tried applying this to a sandbox.  Got the following errors:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 4045:
<h1>Something went wrong !</h1>Applying: Bug 4045 - No check for maximum number of allowed holds.
fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 4045 - No check for maximum number of allowed holds.
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 4045 - No check for maximum number of allowed holds from OPAC.

28492 - Bug 4045 - No check for maximum number of allowed holds.

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-4045---No-check-for-maximum-number-of-allowed--QeoM3b.patch .


Christopher
Comment 64 Bernardo Gonzalez Kriegel 2014-05-28 23:11:06 UTC
(In reply to Christopher Brannon from comment #63)
> Tried applying this to a sandbox.  Got the following errors:
> 
> The sandbox you've requested is not ready.
...
> Apply? [(y)es, (n)o, (i)nteractive] Patch left in
> /tmp/Bug-4045---No-check-for-maximum-number-of-allowed--QeoM3b.patch .
> 

Hi, seems to be a problem with the sandbox

~/kohaclone$ git checkout master
Switched to branch 'master'
~/kohaclone$ git pull
Already up-to-date.
~/kohaclone$ git bz apply 4045
Bug 4045 - No check for maximum number of allowed holds from OPAC.

28492 - Bug 4045 - No check for maximum number of allowed holds.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 4045 - No check for maximum number of allowed holds.
~/kohaclone$

Switch again to Needs signoff, waiting Kyle's comment
Comment 65 Christopher Brannon 2014-06-03 23:06:28 UTC
Got the sandbox to work this time around.

The limit works great for individually placed holds, but still gives no warning and places holds over limit if holding multiple items.

Christopher
Comment 66 Kyle M Hall 2014-07-01 16:48:03 UTC
I don't think this should fail qa for not being a complete fix. A partial fix is better than no fix at all!

Kyle

(In reply to Christopher Brannon from comment #65)
> Got the sandbox to work this time around.
> 
> The limit works great for individually placed holds, but still gives no
> warning and places holds over limit if holding multiple items.
> 
> Christopher
Comment 67 Christopher Brannon 2014-07-01 16:59:59 UTC
(In reply to Kyle M Hall from comment #66)
> I don't think this should fail qa for not being a complete fix. A partial
> fix is better than no fix at all!
> 
> Kyle
> 
> (In reply to Christopher Brannon from comment #65)
> > Got the sandbox to work this time around.
> > 
> > The limit works great for individually placed holds, but still gives no
> > warning and places holds over limit if holding multiple items.
> > 
> > Christopher

I respectfully disagree.  You placed the multi-hold as a test condition.

> 9) Attempt to place 3 or more holds as a multi-hold

The patch does not pass your test.  If the patch is only to test items placed on hold one at a time, then it works just fine.  I don't think this should pass, based on your parameters.

Christopher
Comment 68 Kyle M Hall 2014-07-09 13:32:50 UTC
I see now that it did work for multi-holds in the past. Considering I started this patch 2 years ago I'm not surprised I forgot ; )

(In reply to Christopher Brannon from comment #67)
> (In reply to Kyle M Hall from comment #66)
> > I don't think this should fail qa for not being a complete fix. A partial
> > fix is better than no fix at all!
> > 
> > Kyle
> > 
> > (In reply to Christopher Brannon from comment #65)
> > > Got the sandbox to work this time around.
> > > 
> > > The limit works great for individually placed holds, but still gives no
> > > warning and places holds over limit if holding multiple items.
> > > 
> > > Christopher
> 
> I respectfully disagree.  You placed the multi-hold as a test condition.
> 
> > 9) Attempt to place 3 or more holds as a multi-hold
> 
> The patch does not pass your test.  If the patch is only to test items
> placed on hold one at a time, then it works just fine.  I don't think this
> should pass, based on your parameters.
> 
> Christopher
Comment 69 Chris Cormack 2014-07-24 22:16:52 UTC Comment hidden (obsolete)
Comment 70 Marcel de Rooy 2014-08-01 10:29:56 UTC
Test results:
Hopefully understandable enough (bl=bib level; il=item level; numbers are bibs)
Will refer to this in my QA comment.

First test (biblevel maxreserves=3 issuingrules=10 on all itypes)
bl 668 OK
bl 1178 OK
req on two biblios: get warning when submitting: ERROR: Internal error: incomplete hold request.
So both holds are blocked. But the error message is kind of vague. Result of bad_data flag in opac-reserve.pl
No blocker, but this could still be improved.
bl 1234 on third OK
try no 4 (1235) message fine OK
try 4 via staff warning, but hold is placed [staff overrules: design??] Note that I had AllowHoldPolicyOverride=Do not allow override! 
try 5 via opac: You cannot place more than 4 holds. (Oops: actually more than 3 should not be allowed..)

Second test (itemlevel maxres=3 issu=2 on all itypes)
il 668
il 1178
req on two: Funny: Four warnings: None can be placed on hold (Fine). Sorry, you can only place 1 more holds (Small spelling issue too! But it is not true! I have gone above the number of 2 in issuingrules. Please uncheck boxes (Impossible: there are no boxes to uncheck..)
Per biblio a warning: No available items.
il 1234 None can be placed on hold (OK)
Changing issu=2 to issu=3: repeat il 1234 (OK)
try no 4 (1235): You cannot place more than 3 holds (OK)
try 4 via staff Cannot place hold on some items (No items are available) So, this time I cannot overrule it.

Third test (item level maxres=3 issu=2 on BK only)
il 668 BK
il 1178 BK
try il 1234 BK None can be placed (OK)
try il 1331 CF Sorry none can be placed (Not true: I just have 2 holds; For itype CF I should have one to go?)
try il 1331 CF on staff: Cannot place a hold. No items av. (Not true?)
delete two reserves, try il 1331 CF: Still impossible.

Fourth test (item level issu BK=2 issu CF=4) 
il 668 BK
il 1178 BK
try il 1234 BK None placed: OK
try il 1331 CF Can be done: OK
retry il 1331 CF No more than 3: OK (maxreserves overrules the CF=4 rule here.. Debatable?)
try il 1235 BK You cannot place more than 3: OK
Comment 71 Marcel de Rooy 2014-08-01 10:43:38 UTC
Still another test after deleting the reserves from the fourth run: I select four biblios and place hold. I get a warning to uncheck.
I uncheck one biblio and select three items (BK) with a hold limit on BK=2.
Without a warning I had two holds placed.
Comment 72 Marcel de Rooy 2014-08-01 10:51:28 UTC
QA Comment

If maxreserves would have been a new development and Holds was a perfect module, this should have failed QA :)
But we are in another situation. The missing maxreserves for long time already is really bad.
My test results show me that there are still some discrepancies and points to resolve.
We could see this patch as the starting point for that.
Since this report already has quite a history, I am passing QA on it in the perspective noted. Leaving final judgement to RM or perhaps a second QA opinion?
Comment 73 Marcel de Rooy 2014-08-01 10:52:16 UTC
Created attachment 30448 [details] [review]
Bug 4045 - No check for maximum number of allowed holds.

Re-add the system preference maxreserves.

All the code using maxreserves is still in place. Though it
is not used in the Reserves module, it is used in all the
scripts where holds are placed.

Also adds a check so that a borrower cannot exceed the maximum
number of allowed holds by using the multi-hold feature via
the opac.

Test Plan:
1) Apply this patch
2) Run updatedatabase
3) Set maxreserves to 3, set opactheme to bootstrap
4) Log into the opac as a patron
5) Place 3 holds
6) Attempt to place a 4th hold
7) Note you get an error message and cannot place a forth hold
8) Delete two of those holds
9) Attempt to place 3 or more holds as a multi-hold
10) You should see a warning that you cannot place this many holds
11) Try to anyway
12) You should see an alert to tell you to reduce the number of holds
    you are placing.
13) Reduce the number for holds you are placing to 2
14) Your holds should now be placed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Tomás Cohen Arazi 2014-08-07 20:00:43 UTC
Patch pushed to master.

Thanks Kyle!