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 ;)

Created attachment 67337 [details]
displaying the error I got with the patch

forget those two last posts... my bad

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

(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!

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.

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.

(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!

Enjoy your holiday :)

Waiting for Tomas' follow-up

(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.

Which others do you mean? Meet me on IRC :)

(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 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 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...

(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.

(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.

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

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

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>

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.

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

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

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>

Created attachment 79323 [details] [review]
Bug 15486: DBIC update

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

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>

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

(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!

(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!

(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!

(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!

(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!

Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>

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>

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>

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>

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>

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>

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>

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>

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".

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>

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>

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>

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>

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>

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>

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>

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 ;)

Awesome work all!

Pushed to master for 18.11

Created attachment 81087 [details] [review]
Bug 15486: Fix failing test in GetHardDueDate.t

(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

Enhancement will not be backported to 18.05.x series.