Bug 15524 - Set limit on maximum possible holds per patron by category
Summary: Set limit on maximum possible holds per patron by category
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 21415 (view as bug list)
Depends on: 20227
Blocks: 15521 18886 18887 21738
  Show dependency treegraph
 
Reported: 2016-01-07 22:31 UTC by Jesse Weaver
Modified: 2019-05-09 11:40 UTC (History)
21 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Sponsored by Northeast Kansas Library System, NEKLS (http://nekls.org/) This development adds the ability to set a maximum numbers of holds in the circulation rules that will limit the total number of holds a patron can have. This can be set as a default, or per library.
Version(s) released in:


Attachments
Bug 15524 - Set limit on maximum possible holds per patron by category (15.61 KB, patch)
2017-06-28 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524 - Update Schema (2.62 KB, patch)
2017-06-28 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524 - Set limit on maximum possible holds per patron by category (15.67 KB, patch)
2017-09-18 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15524 - Update Schema (2.67 KB, patch)
2017-09-18 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15524: (follow-up) Rename database update (787 bytes, patch)
2017-10-02 22:09 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15524 - Set limit on maximum possible holds per patron by category (15.58 KB, patch)
2018-01-09 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524 - Update Schema (2.67 KB, patch)
2018-01-09 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524: (follow-up) Rename database update (787 bytes, patch)
2018-01-09 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524 [QA Followup] - Change language used (1.73 KB, patch)
2018-01-09 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524 - Set limit on maximum possible holds per patron by category (15.62 KB, patch)
2018-02-16 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524 - Update Schema (2.71 KB, patch)
2018-02-16 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524 [QA Followup] - Change language used (1.77 KB, patch)
2018-02-16 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524 - Set limit on maximum possible holds per patron by category (15.68 KB, patch)
2018-02-16 11:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524 - Update Schema (2.77 KB, patch)
2018-02-16 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524 [QA Followup] - Change language used (1.83 KB, patch)
2018-02-16 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Fix meaningless test (1.39 KB, patch)
2018-02-16 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Pass max_holds at the right position in update (1.20 KB, patch)
2018-02-16 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Respect ReservesControlBranch (776 bytes, patch)
2018-08-10 12:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Respect ReservesControlBranch (782 bytes, patch)
2018-08-10 14:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template (22.18 KB, patch)
2018-08-10 14:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15524: Set limit on maximum possible holds per patron by category (15.66 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (QA Followup) - Change language used (1.80 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Fix meaningless test (1.35 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Pass max_holds at the right position in update (1.17 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Respect ReservesControlBranch (775 bytes, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template (22.13 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15524: (RM follow-up) Fix calls and add filter (4.16 KB, patch)
2018-08-24 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2016-01-07 22:31:05 UTC
It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
Comment 1 rkuiper 2017-02-22 14:39:12 UTC
We are very interested in this bug.  Nothing has happened on this for over a year. Would someone give me a status update on this?
Thanks,
Rhonda K.
Comment 2 Kyle M Hall 2017-06-28 15:30:17 UTC
Created attachment 64707 [details] [review]
Bug 15524 - Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 3 Kyle M Hall 2017-06-28 15:30:24 UTC
Created attachment 64708 [details] [review]
Bug 15524 - Update Schema
Comment 4 Baptiste 2017-06-30 10:37:10 UTC
5) Set the new max holds rule to X for "All libraries"

Do you mean in the first array ? I don't find any other, but if it is this is not really intuitive
Comment 5 Baptiste 2017-06-30 12:21:14 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2017-07-18 10:58:46 UTC
(In reply to Baptiste from comment #4)
> 5) Set the new max holds rule to X for "All libraries"
> 
> Do you mean in the first array ? I don't find any other, but if it is this
> is not really intuitive

I'm not sure what you mean, it's the last text box in this screenshot: https://monosnap.com/file/K2WEsjcCfRE5LF7TKO9xoYLbavdwjJ
Comment 7 Owen Leonard 2017-08-03 15:36:28 UTC
In my test I set the hold limit for DVDs to 10, and the "Maximum total holds allowed (count)" for patron type "Juvenile" to 5.

I placed holds for a patron with that patron category until the number exceeded 5 and then exceeded 10. Neither hold limit appears to be working.
Comment 8 Kyle M Hall 2017-08-09 12:34:42 UTC
(In reply to Owen Leonard from comment #7)
> In my test I set the hold limit for DVDs to 10, and the "Maximum total holds
> allowed (count)" for patron type "Juvenile" to 5.
> 
> I placed holds for a patron with that patron category until the number
> exceeded 5 and then exceeded 10. Neither hold limit appears to be working.

Just tested and everything is working for me! It's especially odd that the existing rule isn't working since I didn't touch that code.
Comment 9 Alex Buckley 2017-09-09 03:46:39 UTC
Hi Kyle

Can you please merge your patches with master branch as the patches attached to this bug report cannot be applied successfully:

Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
M	t/db_dependent/Holds.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Holds.t
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 15524 - Set limit on maximum possible holds per patron by category
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
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".
Patch left in /tmp/Bug-15524---Set-limit-on-maximum-possible-holds-pe-TzOohy.patch
Comment 10 Josef Moravec 2017-09-18 11:34:43 UTC
Created attachment 67180 [details] [review]
Bug 15524 - Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2017-09-18 11:34:46 UTC
Created attachment 67181 [details] [review]
Bug 15524 - Update Schema

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2017-09-18 11:35:38 UTC
I just had to fix small conflict in Holds.t (number of tests), so now this is rebased on master and signed off ;)
Comment 13 Jesse Weaver 2017-10-02 22:09:29 UTC
Created attachment 67531 [details] [review]
Bug 15524: (follow-up) Rename database update

This ensures that this update is applied in the correct order relative
to its dependent bugs.

Just a rename to a DB update so this bug works better with its dependencies; leaving as signed off.
Comment 14 Marcel de Rooy 2017-11-03 10:00:32 UTC
QA: Looking here now
Comment 15 Marcel de Rooy 2017-11-03 10:43:11 UTC
QA Comment
Interesting feature! Limited testing did not reveal problems.

Functionality issue:
In Administration, smart-rules, the section "Default checkout limit by patron category" contains the item "Maximum total holds allowed (count)".
Not sure if the label should contain total and count btw.
But the section should be renamed. This does not pertain any longer to checkout limits only.
Furthermore I would expect to have max_holds also under Default checkout, hold and return policy. It seems consistent to add it also without a patron category restriction.
Same as current checkouts etc.
Hard to move it to FQA for that reason. At least I want to have some additional opinions. Setting to In Discussion. Please ask for feedback via IRC or ML.
Comment 16 Kyle M Hall 2018-01-09 19:56:17 UTC
(In reply to Marcel de Rooy from comment #15)
> QA Comment
> Interesting feature! Limited testing did not reveal problems.
> 
> Functionality issue:
> In Administration, smart-rules, the section "Default checkout limit by
> patron category" contains the item "Maximum total holds allowed (count)".

I've submitted a patch to improve the section name.

> Not sure if the label should contain total and count btw.

Removed

> But the section should be renamed. This does not pertain any longer to
> checkout limits only.

Agreed.

> Furthermore I would expect to have max_holds also under Default checkout,
> hold and return policy. It seems consistent to add it also without a patron
> category restriction.

That behavior is controlled by the maxreserves system preference. We should move that out of a syspref and into the circ rules. I'd be happy to do that on another bug but it's definitely out of the scope of this bug.
Comment 17 Kyle M Hall 2018-01-09 19:57:31 UTC
Created attachment 70389 [details] [review]
Bug 15524 - Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Kyle M Hall 2018-01-09 19:57:39 UTC
Created attachment 70390 [details] [review]
Bug 15524 - Update Schema

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Kyle M Hall 2018-01-09 19:57:42 UTC
Created attachment 70391 [details] [review]
Bug 15524: (follow-up) Rename database update

This ensures that this update is applied in the correct order relative
to its dependent bugs.
Comment 20 Kyle M Hall 2018-01-09 19:57:47 UTC
Created attachment 70392 [details] [review]
Bug 15524 [QA Followup] - Change language used
Comment 21 Kyle M Hall 2018-01-09 20:05:09 UTC
(In reply to Marcel de Rooy from comment #15)
> QA Comment
> Interesting feature! Limited testing did not reveal problems.
> 
> Functionality issue:
> In Administration, smart-rules, the section "Default checkout limit by
> patron category" contains the item "Maximum total holds allowed (count)".

I've submitted a patch to improve the section name.

> Not sure if the label should contain total and count btw.

Removed

> But the section should be renamed. This does not pertain any longer to
> checkout limits only.

Agreed.

> Furthermore I would expect to have max_holds also under Default checkout,
> hold and return policy. It seems consistent to add it also without a patron
> category restriction.

That behavior is controlled by the maxreserves system preference. We should move that out of a syspref and into the circ rules. I'd be happy to do that on another bug but it's definitely out of the scope of this bug.
Comment 22 Marcel de Rooy 2018-02-16 07:13:16 UTC
QA: Looking here again
Comment 23 Marcel de Rooy 2018-02-16 07:16:26 UTC
 FAIL   installer/data/mysql/atomicupdate/max_holds.perl
   FAIL   git manipulation
                The file has been added and deleted in the same patchset
Comment 24 Marcel de Rooy 2018-02-16 07:24:43 UTC
Created attachment 71704 [details] [review]
Bug 15524 - Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Marcel de Rooy 2018-02-16 07:24:48 UTC
Created attachment 71705 [details] [review]
Bug 15524 - Update Schema

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Marcel de Rooy 2018-02-16 07:24:53 UTC
Created attachment 71706 [details] [review]
Bug 15524 [QA Followup] - Change language used
Comment 27 Marcel de Rooy 2018-02-16 07:25:19 UTC
Comment on attachment 70391 [details] [review]
Bug 15524: (follow-up) Rename database update

No longer needed.
Comment 28 Marcel de Rooy 2018-02-16 07:25:34 UTC
(In reply to Marcel de Rooy from comment #23)
>  FAIL   installer/data/mysql/atomicupdate/max_holds.perl
>    FAIL   git manipulation
>                 The file has been added and deleted in the same patchset

Fixed
Comment 29 Marcel de Rooy 2018-02-16 07:40:11 UTC
Do we need a specific coding guideline to exclude new DBIx constructions like

+    my $rule = $schema->resultset('BranchBorrowerCircRule')->find(
+        {
+            branchcode   => $borrower->{branchcode},
+            categorycode => $borrower->{categorycode},
+        }
+    );

while we could do the same by creating Koha objects ?
We do not have this rule now.
Comment 30 Marcel de Rooy 2018-02-16 11:19:59 UTC
Created attachment 71737 [details] [review]
Bug 15524 - Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2018-02-16 11:20:04 UTC
Created attachment 71738 [details] [review]
Bug 15524 - Update Schema

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2018-02-16 11:20:10 UTC
Created attachment 71739 [details] [review]
Bug 15524 [QA Followup] - Change language used

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2018-02-16 11:20:15 UTC
Created attachment 71740 [details] [review]
Bug 15524: (QA follow-up) Fix meaningless test

The last test claims to allow a hold when branch=5 and patron=5, but look
at the preceding statements:
    $rule_branch->max_holds(5);
    $rule_branch->update();
    $rule_branch->max_holds(5);
    $rule_branch->insert();
The last insert will not be done, since the record is already present.
A create should have triggered an error on the primary key.
Obviously, we should use $rule_all.

Test plan:
Run the test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2018-02-16 11:20:23 UTC
Created attachment 71741 [details] [review]
Bug 15524: (QA follow-up) Pass max_holds at the right position in update

When updating max_holds should be before the categorycode in the update
query.
Things that can be found by testing :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2018-02-16 11:27:20 UTC
3 remarks while passing QA (apart from 2 follow-ups):

[1] With reference to above comments:
>> Furthermore I would expect to have max_holds also under Default checkout,
>> hold and return policy. It seems consistent to add it also without a patron
>> category restriction.

> That behavior is controlled by the maxreserves system preference. We should 
> move that out of a syspref and into the circ rules. I'd be happy to do that 
> on another bug but it's definitely out of the scope of this bug.

Yes, sir. Please remove maxreserves on a new report. Note that you already added code for that by putting max_holds in default_circ_rules. At this moment it is actually not used by the interface.

[2] See my earlier comment about introducing new DBIx constructions. No blocker for now, but imo we should add a coding guideline.

[3] While staff shows too many reserves to me, opac does not. Simply no items available. This can be considered as outside the scope, since it should have been there already. But fixing that on yet another report would be so nice..

Thanks
Comment 36 Jonathan Druart 2018-02-16 16:22:22 UTC
(In reply to Marcel de Rooy from comment #35)
> [1] With reference to above comments:
> >> Furthermore I would expect to have max_holds also under Default checkout,
> >> hold and return policy. It seems consistent to add it also without a patron
> >> category restriction.
> 
> > That behavior is controlled by the maxreserves system preference. We should 
> > move that out of a syspref and into the circ rules. I'd be happy to do that 
> > on another bug but it's definitely out of the scope of this bug.
> 
> Yes, sir. Please remove maxreserves on a new report. Note that you already
> added code for that by putting max_holds in default_circ_rules. At this
> moment it is actually not used by the interface.
> 
> [2] See my earlier comment about introducing new DBIx constructions. No
> blocker for now, but imo we should add a coding guideline.

About 1 & 2:
And I think that is what is wrong here. We should have default_circ_rules.max_holds on the interface, then call GetBranchBorrowerCircRule from CanItemBeReserved
It sounds like a trivial change and so I think it should be added by this patch set.
Comment 37 Kyle M Hall 2018-03-23 15:49:47 UTC
(In reply to Jonathan Druart from comment #36)
> And I think that is what is wrong here. We should have
> default_circ_rules.max_holds on the interface, then call
> GetBranchBorrowerCircRule from CanItemBeReserved
> It sounds like a trivial change and so I think it should be added by this
> patch set.

I've been trying to understand your request and I just cannot figure it out. Considering it would be a trivial change, would you be able to submit the patch?
Comment 38 Jonathan Druart 2018-04-05 18:45:47 UTC
Kyle,

Go to "Default checkout, hold and return policy", you do not see "Total holds allowed", fill the other fields, save: default_circ_rules.holdallowed is filled.
We could add it to the interface and make it configurable, do not you think?

Also if we add "max_holds", maybe we should rename "holdallowed" then.

After that is done, the code in CanItemBeReserved could be replaced with a call to GetBranchBorrowerCircRule.

Without these changes we add more confusion to the code in my opinion.

I will not have the time to provide such change before the release.

Setting to in discussion to get other QAer opinions on these patches.

Feel free to provide a patch if you think my requests make sense to you.
Comment 39 Kyle M Hall 2018-06-29 15:17:25 UTC
(In reply to Jonathan Druart from comment #38)
> Kyle,
> 
> Go to "Default checkout, hold and return policy", you do not see "Total
> holds allowed", fill the other fields, save: default_circ_rules.holdallowed
> is filled.
> We could add it to the interface and make it configurable, do not you think?

holdallowed doesn't define how many holds may be placed, it defines what items a patron may place holds on. It's already revealed in the rules editor!

> 
> Also if we add "max_holds", maybe we should rename "holdallowed" then.

Again, not the same thing.

> 
> After that is done, the code in CanItemBeReserved could be replaced with a
> call to GetBranchBorrowerCircRule.
> 
> Without these changes we add more confusion to the code in my opinion.
> 
> I will not have the time to provide such change before the release.
> 
> Setting to in discussion to get other QAer opinions on these patches.
> 
> Feel free to provide a patch if you think my requests make sense to you.

I think your comment is based on a fundamental misunderstanding of what holdallowed does. If I'm wrong let me know!

We could definitely add max_holds to the default_circ_rules table, but that should be done on another bug as it is out of scope for this one.
Comment 40 Jonathan Druart 2018-07-24 13:44:33 UTC
Back to this.

(In reply to Kyle M Hall from comment #39)
> (In reply to Jonathan Druart from comment #38)
> > Kyle,
> > 
> > Go to "Default checkout, hold and return policy", you do not see "Total
> > holds allowed", fill the other fields, save: default_circ_rules.holdallowed
> > is filled.
> > We could add it to the interface and make it configurable, do not you think?
> 
> holdallowed doesn't define how many holds may be placed, it defines what
> items a patron may place holds on. It's already revealed in the rules editor!
> 
> > 
> > Also if we add "max_holds", maybe we should rename "holdallowed" then.
> 
> Again, not the same thing.

Well, I think you were right here. I do not understand what I wanted to tell. I guess I got holdallowed and max_holds mixed up. Sorry about that.


> > After that is done, the code in CanItemBeReserved could be replaced with a
> > call to GetBranchBorrowerCircRule.

This is, I think, still a valid request/question.
In CanItemBeReserved there are 2 Koha::CirculationRules->find
I am wondering why we do not call Koha::CirculationRules->get_effective_rule instead.
Moreover it seems that there is a mix between '*' and undef. The UI inserts '*' whereas these calls and the tests search for undef.
If we use undef we will be able to add FKs (branchcode,categorycode,itemtype) which can be great.
Comment 41 Jonathan Druart 2018-07-24 14:11:02 UTC
(In reply to Jonathan Druart from comment #40)
> > > After that is done, the code in CanItemBeReserved could be replaced with a
> > > call to GetBranchBorrowerCircRule.
> 
> This is, I think, still a valid request/question.
> In CanItemBeReserved there are 2 Koha::CirculationRules->find
> I am wondering why we do not call Koha::CirculationRules->get_effective_rule
> instead.
> Moreover it seems that there is a mix between '*' and undef. The UI inserts
> '*' whereas these calls and the tests search for undef.
> If we use undef we will be able to add FKs
> (branchcode,categorycode,itemtype) which can be great.

And this is for bug 18887.
Comment 42 Kyle M Hall 2018-08-10 12:21:50 UTC
Created attachment 77648 [details] [review]
Bug 15524: (QA follow-up) Respect ReservesControlBranch
Comment 43 Kyle M Hall 2018-08-10 14:37:23 UTC
Created attachment 77681 [details] [review]
Bug 15524: (QA follow-up) Respect ReservesControlBranch
Comment 44 Kyle M Hall 2018-08-10 14:37:40 UTC
Created attachment 77682 [details] [review]
Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template
Comment 45 Nick Clemens 2018-08-24 15:16:01 UTC
Created attachment 78134 [details] [review]
Bug 15524: Set limit on maximum possible holds per patron by category

It's possible to set a limit on the maximum number of holds for a particular branch/category/itemtype, but not on the total number of holds for a given patron (by branch/category).
This new rule works in conjunction with the existing branch/borrower/item rules in that Koha will use the lower of the two limits. This new rule counts all holds of all types, which prevents bib-level holds from not being counted for the purpose of these limits. This makes the most sense and was also requested by the sponsor.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Nick Clemens 2018-08-24 15:16:07 UTC
Created attachment 78135 [details] [review]
Bug 15524: (QA Followup) - Change language used

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Nick Clemens 2018-08-24 15:16:12 UTC
Created attachment 78136 [details] [review]
Bug 15524: (QA follow-up) Fix meaningless test

The last test claims to allow a hold when branch=5 and patron=5, but look
at the preceding statements:
    $rule_branch->max_holds(5);
    $rule_branch->update();
    $rule_branch->max_holds(5);
    $rule_branch->insert();
The last insert will not be done, since the record is already present.
A create should have triggered an error on the primary key.
Obviously, we should use $rule_all.

Test plan:
Run the test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Nick Clemens 2018-08-24 15:16:17 UTC
Created attachment 78137 [details] [review]
Bug 15524: (QA follow-up) Pass max_holds at the right position in update

When updating max_holds should be before the categorycode in the update
query.
Things that can be found by testing :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Nick Clemens 2018-08-24 15:16:22 UTC
Created attachment 78138 [details] [review]
Bug 15524: (QA follow-up) Respect ReservesControlBranch
Comment 50 Nick Clemens 2018-08-24 15:16:28 UTC
Created attachment 78139 [details] [review]
Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template
Comment 51 Nick Clemens 2018-08-24 15:16:34 UTC
Created attachment 78140 [details] [review]
Bug 15524: (RM follow-up) Fix calls and add filter
Comment 52 Nick Clemens 2018-08-24 16:31:44 UTC
Awesome work all!

Pushed to master for 18.11
Comment 53 Nick Clemens 2018-08-25 19:13:45 UTC
(In reply to Nick Clemens from comment #52)
> Awesome work all!
> 
> Pushed to master for 18.11

Pushed schema files as followup
Comment 54 Martin Renvoize 2018-09-05 07:55:21 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 55 Andrew Fuerste-Henry 2019-05-08 13:44:39 UTC
*** Bug 21415 has been marked as a duplicate of this bug. ***