Bug 15486 - Restrict number of holds placed by day
Summary: Restrict number of holds placed by day
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL: http://devs.bywatersolutions.com/2016...
Keywords:
Depends on: 19539
Blocks: 15565 25652
  Show dependency treegraph
 
Reported: 2016-01-06 16:05 UTC by Nicole C. Engard
Modified: 2022-08-21 21:08 UTC (History)
13 users (show)

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


Attachments
Bug 15486: DB structure change (3.14 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.60 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (4.20 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (3.39 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.61 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.65 KB, patch)
2017-07-19 22:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.14 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.60 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (4.20 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (3.39 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.61 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.65 KB, patch)
2017-09-19 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
displaying the error I got with the patch (25.31 KB, image/png)
2017-09-22 20:29 UTC, Dominic Pichette
Details
Bug 15486: DB structure change (3.13 KB, patch)
2017-10-09 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.60 KB, patch)
2017-10-09 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (4.19 KB, patch)
2017-10-09 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (3.38 KB, patch)
2017-10-09 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.61 KB, patch)
2017-10-09 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.64 KB, patch)
2017-10-09 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.13 KB, patch)
2017-10-27 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.60 KB, patch)
2017-10-27 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (5.91 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (5.74 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.61 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (1.76 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.64 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: (followup) Tidy table code for readability (13.49 KB, patch)
2017-10-27 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.69 KB, patch)
2017-10-27 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.64 KB, patch)
2017-10-27 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: (followup) Tidy table code for readability (13.49 KB, patch)
2017-10-27 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18282: operationId must be unique (2.14 KB, patch)
2017-10-27 20:10 UTC, Dominic Pichette
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.14 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.60 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (5.74 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (2.69 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.65 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (6.59 KB, patch)
2018-02-07 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (6.65 KB, patch)
2018-03-12 15:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.12 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Unit tests (6.19 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (3.04 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.62 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (6.99 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.65 KB, patch)
2018-09-24 15:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: (follow-up) Default to unlimited (1.14 KB, patch)
2018-09-24 15:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.27 KB, patch)
2018-10-01 09:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: Unit tests (6.34 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (3.17 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.76 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (7.14 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.79 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: (follow-up) Default to unlimited (1.33 KB, patch)
2018-10-01 09:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 15486: DB structure change (3.33 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: Unit tests (6.47 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits (3.23 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: Modify request.tt to handle the new error string (1.82 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: Make circ rules UI handle holds_per_day (7.19 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: DBIC update (1.85 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: (follow-up) Default to unlimited (1.39 KB, patch)
2018-10-12 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15486: Fix failing test in GetHardDueDate.t (861 bytes, patch)
2018-10-24 20:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2016-01-06 16:05:43 UTC
For example:
A patron is allowed to place 5 holds (or other variable number set by the library) per day but can not exceed 30 holds (set by circ rules).

Work to be done:
1) Add new database column reservesallowed_daily to the table issuingrules
2) Add new database column “created_on” to the table reserves
3) Modify C4::Reserves::AddReserve to populate “created_on” with the current timestamp
4) Add new column “Holds allowed (daily)” to the circulation and fine rules.
5) Modify “Holds allowed (count)” to “Holds allowed (total)” for clarity
6) Modify CanItemBeReserved to check if max daily holds has been exceeded
If so, it must return new error code ‘TooManyReservesToday’
7) Modify all code calling CanItemBeReserved to handle this new error code
8) Add unit tests for new CanItemBeReserved behavior


Seeking Co-Sponsors: http://devs.bywatersolutions.com/projects/restrict-number-of-holds-placed-by-day/
Comment 1 Tomás Cohen Arazi 2017-07-19 22:43:20 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-07-19 22:43:23 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-07-19 22:43:26 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2017-07-19 22:43:30 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2017-07-19 22:43:33 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2017-07-19 22:43:36 UTC Comment hidden (obsolete)
Comment 7 Alex Buckley 2017-09-17 02:38:49 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2017-09-19 12:31:01 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2017-09-19 12:31:08 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2017-09-19 12:31:13 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2017-09-19 12:31:20 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2017-09-19 12:31:26 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2017-09-19 12:31:37 UTC Comment hidden (obsolete)
Comment 14 Dominic Pichette 2017-09-22 20:27:44 UTC
Just tried it. Can you look at error.png and tell me if this is the error I am suppose to get? If it is I'll sign you off ;)
Comment 15 Dominic Pichette 2017-09-22 20:29:34 UTC
Created attachment 67337 [details]
displaying the error I got with the patch
Comment 16 Dominic Pichette 2017-09-22 20:30:51 UTC
forget those two last posts... my bad
Comment 17 Katrin Fischer 2017-10-07 22:18:03 UTC
1) Translations in mind, I suggest to make the message a bit more specific:
Daily limit reached for patron >
Daily hold limit reached for patron

2) If nothing is entered, it appears to default to 0 (instead of unlimited). So I think this will break existing installations. I'd suggest to copy the max holds total into max holds daily.

3) The tests are giving me trouble:
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Holds.t
t/db_dependent/Holds.t .. 26/59 # Looks like you planned 59 tests but ran 56.
t/db_dependent/Holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/59 subtests 

Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 65280 Tests: 56 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 59 tests but ran 56.
Files=1, Tests=56,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.70 cusr  0.34 csys =  2.07 CPU)
Result: FAIL
Comment 18 Tomás Cohen Arazi 2017-10-09 13:39:38 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2017-10-09 13:39:45 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2017-10-09 13:39:56 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2017-10-09 13:40:03 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2017-10-09 13:40:10 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2017-10-09 13:40:16 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2017-10-09 13:44:33 UTC
(In reply to Katrin Fischer from comment #17)
> 1) Translations in mind, I suggest to make the message a bit more specific:
> Daily limit reached for patron >
> Daily hold limit reached for patron

Fixed, thanks!

> 2) If nothing is entered, it appears to default to 0 (instead of unlimited).
> So I think this will break existing installations. I'd suggest to copy the
> max holds total into max holds daily.

This is intended, on purpose! Its the same behaviour allowedreserves has. 0 and undef mean the same in the code. If no daily (or zero) limit is defined, then the global one is used 

> 3) The tests are giving me trouble:
> kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Holds.t
> t/db_dependent/Holds.t .. 26/59 # Looks like you planned 59 tests but ran 56.
> t/db_dependent/Holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
> Failed 3/59 subtests 

Fixed it, I guess some recent changes in Holds.t made this. Tests should pass noW! Thanks for the feedback!
Comment 25 Katrin Fischer 2017-10-09 15:44:04 UTC
Hi Tomas,

what I meant is that the table displays dail limit = 0 for all circulation conditions existing pre-update. 
If 0 means unlimited it should display as such. 
If 0 means 0 we should fix the existing rules on update so they still work.
Comment 26 Katrin Fischer 2017-10-14 22:13:27 UTC
Can you take another look at my last comment and meaning of 0 please?

I retested and after installing the patch the daily column is empty. Later it's not possible to safe it empty, as empty jumps to 0. It doesn't feel quite right.
Comment 27 Tomás Cohen Arazi 2017-10-15 12:14:11 UTC
(In reply to Katrin Fischer from comment #26)
> Can you take another look at my last comment and meaning of 0 please?
> 
> I retested and after installing the patch the daily column is empty. Later
> it's not possible to safe it empty, as empty jumps to 0. It doesn't feel
> quite right.

0 and undef have the same practical behavior. I will make it undef so it makes more sense to you. Will do it on Tuesday because we have a holiday tomorrow!
Comment 28 Katrin Fischer 2017-10-15 19:58:40 UTC
Enjoy your holiday :)
Comment 29 Katrin Fischer 2017-10-22 08:43:45 UTC
Waiting for Tomas' follow-up
Comment 30 Tomás Cohen Arazi 2017-10-26 18:11:45 UTC
(In reply to Katrin Fischer from comment #25)
> Hi Tomas,
> 
> what I meant is that the table displays dail limit = 0 for all circulation
> conditions existing pre-update. 
> If 0 means unlimited it should display as such. 
> If 0 means 0 we should fix the existing rules on update so they still work.

I'm looking at this, and it still bugs me that this is already the behaviour for other 4 fields.
Comment 31 Katrin Fischer 2017-10-26 18:17:46 UTC
Which others do you mean? Meet me on IRC :)
Comment 32 Tomás Cohen Arazi 2017-10-27 14:44:54 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2017-10-27 14:44:59 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2017-10-27 14:45:04 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2017-10-27 14:45:07 UTC Comment hidden (obsolete)
Comment 36 Tomás Cohen Arazi 2017-10-27 14:45:11 UTC Comment hidden (obsolete)
Comment 37 Tomás Cohen Arazi 2017-10-27 14:45:15 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2017-10-27 14:45:19 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2017-10-27 14:45:23 UTC Comment hidden (obsolete)
Comment 40 Tomás Cohen Arazi 2017-10-27 15:16:30 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2017-10-27 15:16:42 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2017-10-27 15:16:52 UTC Comment hidden (obsolete)
Comment 43 Dominic Pichette 2017-10-27 20:10:14 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2017-10-30 13:57:03 UTC
(In reply to Dominic Pichette from comment #43)
> Created attachment 68791 [details] [review] [review]
> Bug 18282: operationId must be unique
> 
> operationId has the following documentation:
>  "Unique string used to identify the operation. The id MUST be unique among
> all
>   operations described in the API."
> 
> This patch modifies operationIds to be unique accross our API operations.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
> Signed-off-by: Dominic Pichette <dominic@inlibro.com>

Dominique, I think you submitted a signed patch to the wrong bug report. I obsoleted it and marked the bug back as Needs Signoff to avoid confusion.
Comment 45 Katrin Fischer 2017-12-28 07:11:16 UTC Comment hidden (obsolete)
Comment 46 Tomás Cohen Arazi 2018-02-07 19:18:05 UTC Comment hidden (obsolete)
Comment 47 Tomás Cohen Arazi 2018-02-07 19:18:10 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2018-02-07 19:18:15 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2018-02-07 19:18:20 UTC Comment hidden (obsolete)
Comment 50 Tomás Cohen Arazi 2018-02-07 19:18:25 UTC Comment hidden (obsolete)
Comment 51 Tomás Cohen Arazi 2018-02-07 19:18:30 UTC Comment hidden (obsolete)
Comment 52 Tomás Cohen Arazi 2018-03-12 15:34:06 UTC Comment hidden (obsolete)
Comment 53 Mark Tompsett 2018-03-12 17:57:05 UTC
Comment on attachment 71317 [details] [review]
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits

Review of attachment 71317 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +300,4 @@
>      my $ruleitemtype;    # itemtype of the matching issuing rule
>      my $allowedreserves  = 0; # Total number of holds allowed across all records
>      my $holds_per_record = 1; # Total number of holds allowed for this one given record
> +    my $holds_per_day    = 0; # Total number of holds allowed per day for the given patron

Shouldn't this default to undef? (i.e. unlimited)
Comment 54 Mark Tompsett 2018-03-12 17:57:40 UTC
Comment on attachment 71314 [details] [review]
Bug 15486: DB structure change

Review of attachment 71314 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug_15486_max_holds_per_day.perl
@@ +1,2 @@
> +$DBversion = "XXX";
> +if ( CheckVersion($DBversion) ) {

no reserve date?!

::: installer/data/mysql/kohastructure.sql
@@ +1883,4 @@
>  CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha
>    `reserve_id` int(11) NOT NULL auto_increment, -- primary key
>    `borrowernumber` int(11) NOT NULL default 0, -- foreign key from the borrowers table defining which patron this hold is for
> +  `reservedate` date default NULL, -- the date the hold was placed

reservedate here...
Comment 55 Tomás Cohen Arazi 2018-03-12 18:29:04 UTC
(In reply to M. Tompsett from comment #54)
> Comment on attachment 71314 [details] [review] [review]
> Bug 15486: DB structure change
> 
> Review of attachment 71314 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/atomicupdate/bug_15486_max_holds_per_day.perl
> @@ +1,2 @@
> > +$DBversion = "XXX";
> > +if ( CheckVersion($DBversion) ) {
> 
> no reserve date?!
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +1883,4 @@
> >  CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha
> >    `reserve_id` int(11) NOT NULL auto_increment, -- primary key
> >    `borrowernumber` int(11) NOT NULL default 0, -- foreign key from the borrowers table defining which patron this hold is for
> > +  `reservedate` date default NULL, -- the date the hold was placed
> 
> reservedate here...

This is probably some formatting change. I'm not adding that column at all.
Comment 56 Tomás Cohen Arazi 2018-03-12 18:29:53 UTC
(In reply to M. Tompsett from comment #53)
> Comment on attachment 71317 [details] [review] [review]
> Bug 15486: Extend CanItemBeReserved so it handles daily holds limits
> 
> Review of attachment 71317 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Reserves.pm
> @@ +300,4 @@
> >      my $ruleitemtype;    # itemtype of the matching issuing rule
> >      my $allowedreserves  = 0; # Total number of holds allowed across all records
> >      my $holds_per_record = 1; # Total number of holds allowed for this one given record
> > +    my $holds_per_day    = 0; # Total number of holds allowed per day for the given patron
> 
> Shouldn't this default to undef? (i.e. unlimited)

While it could make sense, I'm just following what's done already in the code.
Comment 57 Lucie Gay 2018-03-14 16:02:36 UTC
patch works well, but messages are not clear :
in staff user : "None of these items can normally be put on hold for this patron"
opac : "Sorry, none of these items can be placed on hold."
patrons doesn't know that it's because there is a limit per day
Comment 58 Séverine Queune 2018-04-24 09:50:50 UTC
I can't apply the patch on master branch 17.12.00.037 :

Bug 15486 - Restrict number of holds placed by day

71314 - Bug 15486: DB structure change
71315 - Bug 15486: DBIC update
71316 - Bug 15486: Unit tests
71317 - Bug 15486: Extend CanItemBeReserved so it handles daily holds limits
71318 - Bug 15486: Modify request.tt to handle the new error string
72709 - Bug 15486: Make circ rules UI handle holds_per_day

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15486: DB structure change
Applying: Bug 15486: DBIC update
Using index info to reconstruct a base tree...
M       Koha/Schema/Result/Issuingrule.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Issuingrule.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Issuingrule.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 15486: DBIC update
The copy of the patch that failed is found in:
   /var/repositories/koha/.git/rebase-apply/patch
Comment 59 Tomás Cohen Arazi 2018-09-24 15:51:21 UTC
Created attachment 79318 [details] [review]
Bug 15486: DB structure change

This patch adds a new column __max_holds_per_day__ to the issuingrules table.
It's going to be used to set a daily limit  for holds.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2018-09-24 15:51:27 UTC
Created attachment 79319 [details] [review]
Bug 15486: Unit tests

This patch introduces unit tests for the new circulation rules option
that allows setting a max holds per day limit.

To test:
- Apply the patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: CanItemBeReserved doesn't check the amount of holds per day
   and the introduced error code is not returned. OK is returned
   instead.
Comment 61 Tomás Cohen Arazi 2018-09-24 15:51:34 UTC
Created attachment 79320 [details] [review]
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits

This patch implements the required changes in
C4::Reserves::CanItemBeReserved so it implements a daily limit on holds.

It returns the 'tooManyReservesToday' string if the policy doesn't allow
placing the hold. It returns 'OK' (current behaviour) otherwise.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: Tests fail because the error condition is not making
CanItemBeReserved return the desired error code.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Holds.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 62 Tomás Cohen Arazi 2018-09-24 15:51:40 UTC
Created attachment 79321 [details] [review]
Bug 15486: Modify request.tt to handle the new error string

This patch makes reserve/request.pl display a convenient error
description when a hold cannot be placed due to the new daily limit
configuration.

To test:
- Apply this patchset
- Upgrade
- Configure your circulation rules so there's a daily limit for holds
- Place holds so the patron reaches the maximum
- Place one more hold
- Notice the hold cannot be placed and a convenient error message is
displayed.
- Sign off :-D
Comment 63 Tomás Cohen Arazi 2018-09-24 15:51:47 UTC
Created attachment 79322 [details] [review]
Bug 15486: Make circ rules UI handle holds_per_day

This patch makes the staff UI correctly handle the holds_per_day
configuration.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2018-09-24 15:51:54 UTC
Created attachment 79323 [details] [review]
Bug 15486: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Tomás Cohen Arazi 2018-09-24 15:56:44 UTC
Created attachment 79324 [details] [review]
Bug 15486: (follow-up) Default to unlimited

While it doesn't have a practical effect (the variable is reset several
lines below) I agree this should be explicitly set to its default.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2018-09-24 15:58:51 UTC
I have:
- Rebased against master
- Adjusted to the new return values for C4::Reserves::CanItemBeReserved
- Fixed the tests
- Deal with Katrin's highlighted issue with 0 vs. unlimited.
- Fixed Mark's remark on default value
Comment 67 Tomás Cohen Arazi 2018-09-24 15:59:09 UTC
(In reply to Katrin Fischer from comment #26)
> Can you take another look at my last comment and meaning of 0 please?
> 
> I retested and after installing the patch the daily column is empty. Later
> it's not possible to safe it empty, as empty jumps to 0. It doesn't feel
> quite right.

Done!
Comment 68 Tomás Cohen Arazi 2018-09-24 15:59:29 UTC
(In reply to M. Tompsett from comment #53)
> Comment on attachment 71317 [details] [review] [review]
> Bug 15486: Extend CanItemBeReserved so it handles daily holds limits
> 
> Review of attachment 71317 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Reserves.pm
> @@ +300,4 @@
> >      my $ruleitemtype;    # itemtype of the matching issuing rule
> >      my $allowedreserves  = 0; # Total number of holds allowed across all records
> >      my $holds_per_record = 1; # Total number of holds allowed for this one given record
> > +    my $holds_per_day    = 0; # Total number of holds allowed per day for the given patron
> 
> Shouldn't this default to undef? (i.e. unlimited)

Done!
Comment 69 Tomás Cohen Arazi 2018-09-24 16:00:16 UTC
(In reply to M. Tompsett from comment #54)
> Comment on attachment 71314 [details] [review] [review]
> Bug 15486: DB structure change
> 
> Review of attachment 71314 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/atomicupdate/bug_15486_max_holds_per_day.perl
> @@ +1,2 @@
> > +$DBversion = "XXX";
> > +if ( CheckVersion($DBversion) ) {
> 
> no reserve date?!
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +1883,4 @@
> >  CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha
> >    `reserve_id` int(11) NOT NULL auto_increment, -- primary key
> >    `borrowernumber` int(11) NOT NULL default 0, -- foreign key from the borrowers table defining which patron this hold is for
> > +  `reservedate` date default NULL, -- the date the hold was placed
> 
> reservedate here...

I just fixed a typo on the field description. No need to change the atomic update file!
Comment 70 Tomás Cohen Arazi 2018-09-24 16:02:03 UTC
(In reply to Lucie Gay from comment #57)
> patch works well, but messages are not clear :
> in staff user : "None of these items can normally be put on hold for this
> patron"
> opac : "Sorry, none of these items can be placed on hold."
> patrons doesn't know that it's because there is a limit per day

Hi Lucie, thanks for testing it and helping me improve it! I agree the OPAC 'could' give better information to the OPAC users when placing a hold is not possible. But that's already the case for existing constraints, and this bug is not about it. I would encourage you to file a new one, with a proposal to give end users more information!
Comment 71 Tomás Cohen Arazi 2018-09-24 16:02:45 UTC
(In reply to Séverine Queune from comment #58)
> I can't apply the patch on master branch 17.12.00.037 :

Hola Séverine, the new patches apply on master. Thanks!
Comment 72 Biblibre Sandboxes 2018-10-01 09:56:14 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 73 Biblibre Sandboxes 2018-10-01 09:57:57 UTC
Created attachment 79696 [details] [review]
Bug 15486: DB structure change

This patch adds a new column __max_holds_per_day__ to the issuingrules table.
It's going to be used to set a daily limit  for holds.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 74 Biblibre Sandboxes 2018-10-01 09:58:02 UTC
Created attachment 79697 [details] [review]
Bug 15486: Unit tests

This patch introduces unit tests for the new circulation rules option
that allows setting a max holds per day limit.

To test:
- Apply the patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: CanItemBeReserved doesn't check the amount of holds per day
   and the introduced error code is not returned. OK is returned
   instead.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 75 Biblibre Sandboxes 2018-10-01 09:58:07 UTC
Created attachment 79698 [details] [review]
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits

This patch implements the required changes in
C4::Reserves::CanItemBeReserved so it implements a daily limit on holds.

It returns the 'tooManyReservesToday' string if the policy doesn't allow
placing the hold. It returns 'OK' (current behaviour) otherwise.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: Tests fail because the error condition is not making
CanItemBeReserved return the desired error code.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 76 Biblibre Sandboxes 2018-10-01 09:58:12 UTC
Created attachment 79699 [details] [review]
Bug 15486: Modify request.tt to handle the new error string

This patch makes reserve/request.pl display a convenient error
description when a hold cannot be placed due to the new daily limit
configuration.

To test:
- Apply this patchset
- Upgrade
- Configure your circulation rules so there's a daily limit for holds
- Place holds so the patron reaches the maximum
- Place one more hold
- Notice the hold cannot be placed and a convenient error message is
displayed.
- Sign off :-D

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 77 Biblibre Sandboxes 2018-10-01 09:58:16 UTC
Created attachment 79700 [details] [review]
Bug 15486: Make circ rules UI handle holds_per_day

This patch makes the staff UI correctly handle the holds_per_day
configuration.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 78 Biblibre Sandboxes 2018-10-01 09:58:21 UTC
Created attachment 79701 [details] [review]
Bug 15486: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 79 Biblibre Sandboxes 2018-10-01 09:58:26 UTC
Created attachment 79702 [details] [review]
Bug 15486: (follow-up) Default to unlimited

While it doesn't have a practical effect (the variable is reset several
lines below) I agree this should be explicitly set to its default.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 80 Séverine Queune 2018-10-01 10:02:25 UTC
Thanks Tomás !
This is a great enhancement, it works as excepted !

I just have suggestion about the error messages that don't explain the reason hold can't be place :
   - opac : There are no items that can be placed on hold.
   - staff client : Cannot place hold - No items are available to be placed on hold.
I let the QA team decide, but I think it could be clearer for users (pro + patrons) if we could add something like "You've reached the daily limit for holds".
Comment 81 Josef Moravec 2018-10-12 13:44:00 UTC
Created attachment 80478 [details] [review]
Bug 15486: DB structure change

This patch adds a new column __max_holds_per_day__ to the issuingrules table.
It's going to be used to set a daily limit  for holds.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 82 Josef Moravec 2018-10-12 13:44:06 UTC
Created attachment 80479 [details] [review]
Bug 15486: Unit tests

This patch introduces unit tests for the new circulation rules option
that allows setting a max holds per day limit.

To test:
- Apply the patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: CanItemBeReserved doesn't check the amount of holds per day
   and the introduced error code is not returned. OK is returned
   instead.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 83 Josef Moravec 2018-10-12 13:44:12 UTC
Created attachment 80480 [details] [review]
Bug 15486: Extend CanItemBeReserved so it handles daily holds limits

This patch implements the required changes in
C4::Reserves::CanItemBeReserved so it implements a daily limit on holds.

It returns the 'tooManyReservesToday' string if the policy doesn't allow
placing the hold. It returns 'OK' (current behaviour) otherwise.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Holds.t
=> FAIL: Tests fail because the error condition is not making
CanItemBeReserved return the desired error code.
- Apply this patch
- Run:
 k$ prove t/db_dependent/Holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 84 Josef Moravec 2018-10-12 13:44:18 UTC
Created attachment 80481 [details] [review]
Bug 15486: Modify request.tt to handle the new error string

This patch makes reserve/request.pl display a convenient error
description when a hold cannot be placed due to the new daily limit
configuration.

To test:
- Apply this patchset
- Upgrade
- Configure your circulation rules so there's a daily limit for holds
- Place holds so the patron reaches the maximum
- Place one more hold
- Notice the hold cannot be placed and a convenient error message is
displayed.
- Sign off :-D

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 85 Josef Moravec 2018-10-12 13:44:24 UTC
Created attachment 80482 [details] [review]
Bug 15486: Make circ rules UI handle holds_per_day

This patch makes the staff UI correctly handle the holds_per_day
configuration.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 86 Josef Moravec 2018-10-12 13:44:29 UTC
Created attachment 80483 [details] [review]
Bug 15486: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 87 Josef Moravec 2018-10-12 13:44:36 UTC
Created attachment 80484 [details] [review]
Bug 15486: (follow-up) Default to unlimited

While it doesn't have a practical effect (the variable is reset several
lines below) I agree this should be explicitly set to its default.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 88 Josef Moravec 2018-10-12 13:44:52 UTC
This patchset looks OK for me:

there are some glitches:
1) The error messages on hold request - but they behave this way in current master and could be improved

2) The limit can be override by placing multiple hold at once from search results (syspref  DisplayMultiPlaceHold ) in staff client
in opac it does not add holds over limmit, but does not inform patron in any way that some of his/her hold will not be placed.

So, passing QA this one, and adding these to reports to bugzilla ;)
Comment 89 Nick Clemens 2018-10-24 17:25:09 UTC
Awesome work all!

Pushed to master for 18.11
Comment 90 Jonathan Druart 2018-10-24 20:08:34 UTC
Created attachment 81087 [details] [review]
Bug 15486: Fix failing test in GetHardDueDate.t
Comment 91 Nick Clemens 2018-10-25 12:39:06 UTC
(In reply to Jonathan Druart from comment #90)
> Created attachment 81087 [details] [review] [review]
> Bug 15486: Fix failing test in GetHardDueDate.t

Pushed to master for 18.11
Comment 92 Martin Renvoize 2018-11-06 10:32:21 UTC
Enhancement will not be backported to 18.05.x series.