Bug 17978 - Include 'Next available'/title level holds in holds count when placing holds (opac and staff)
Summary: Include 'Next available'/title level holds in holds count when placing holds ...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-23 23:56 UTC by Christopher Brannon
Modified: 2019-04-16 19:57 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05


Attachments
Bug 17978 - Circ rules will not limit holds (1.29 KB, patch)
2017-12-19 19:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17978 - Circ rules will not limit holds (1.35 KB, patch)
2018-03-09 07:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17978 - Circ rules will not limit holds (1.44 KB, patch)
2018-03-23 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17978: Check if hold can be placed before placing on placerequest.pl (2.29 KB, patch)
2018-12-24 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17978: Unit tests for changes (2.19 KB, patch)
2018-12-24 15:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17978 - Circ rules will not limit holds (1.46 KB, patch)
2019-03-20 18:27 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17978: Check if hold can be placed before placing on placerequest.pl (2.33 KB, patch)
2019-03-20 18:27 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17978: Unit tests for changes (2.23 KB, patch)
2019-03-20 18:27 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17978: Circ rules will not limit holds (1.52 KB, patch)
2019-04-01 14:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17978: Check if hold can be placed before placing on placerequest.pl (2.40 KB, patch)
2019-04-01 14:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17978: Unit tests for changes (2.29 KB, patch)
2019-04-01 14:15 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2017-01-23 23:56:48 UTC
It appears that the holds limit in circ rules is broken.  I can set a limit of 0 on holds and it will block the hold, but if I set it to anything higher than 0, it ignores the value altogether and gives the patron unlimited holds.

Maxreserves works as expected.
Comment 1 tmcmahon 2017-01-24 00:06:19 UTC
I've tested this and the hold limit did not work for me either.
Comment 2 tmcmahon 2017-01-24 00:18:26 UTC
At least for me, this setting doesn't work only if I do a bulk hold by checking the boxes in search results.  If I reserve one item at a time, it will stop me at the limit.
Comment 3 Christopher Brannon 2017-01-24 19:02:54 UTC
Found flaws in my testing.  I was able to get this to work as expected.
Comment 4 tmcmahon 2017-01-24 19:43:53 UTC
Circ rules will still not limit holds if multiples are added from search results.

To repeat this:
1. Select a patron who has less than the maximum holds.
2. Search for items to hold and select multiple check boxes to bring the hold count over the set limit.
3. Select the "Place hold for (patron)" from the "Place hold" button at the top of the list.

It will let you place holds on as many as you select.  It will not let you add more holds if you've already reached the limit before this.
Comment 5 tmcmahon 2017-01-25 17:40:39 UTC
My tests were from the staff interface.  It's not likely the staff will be reserving in bulk for a patron, but the bug still exists there if someone feels the need to work on this.

If a patron selects a group of items to reserve, it will add items up to the limit, but not give a warning that not all items were added.
Comment 6 Jonathan Druart 2017-01-27 15:01:33 UTC
Technically: placerequest.pl is not checking if a patron can place reserve.
The check is only done in reserve.pl
That means that if a patron can place 1 hold, he will not be blocked to place X holds.
Comment 7 Viccy Kemp 2017-01-30 21:08:46 UTC
Flower Mound Public Library can verify this is, in fact, happening. I have a hold limit placed of one particular item type, but the same card is able to place multiple holds. This is a real problem because we do not want patrons checking out more than one of our STEM kits at a time or placing holds on all of them.
Comment 8 Jonathan Druart 2017-02-09 07:19:36 UTC
It's a long standing bug and it will need quite a lot of work to fix it.
Comment 9 Christopher Brannon 2017-04-13 14:47:08 UTC
(In reply to Jonathan Druart from comment #8)
> It's a long standing bug and it will need quite a lot of work to fix it.

I assume that if this is a long standing bug, there must be another ticket somewhere.  Do you know of one?
Comment 10 Christopher Brannon 2017-04-13 14:48:52 UTC
Finding that next available holds are ignored when it comes to hold limits on specific rules.  Item specific holds are adhering to the limits set.
Comment 11 Jonathan Druart 2017-04-17 14:25:34 UTC
See bug 6976.
Comment 12 Kyle M Hall 2017-12-19 19:48:26 UTC
Created attachment 69927 [details] [review]
Bug 17978 - Circ rules will not limit holds

The crux of this issue is that all those holds are unfilled record level holds. The code that checks for the number of holds the user already has *always* uses a branchcode of some form. Because of this, any open record level holds are completely ignored for this purpose.

Test Plan:
1) Set ReservesControlBranch to "item's home library"
2) Set an All libraries/All Patrons/All itemtypes rules limiting a patron to 2 holds
3) Place 3 record level holds, you can but you should not be able to!
4) Delete the holds
5) Apply this patch
6) Attempt to place 3 record level holds
7) Note that you cannot!
Comment 13 Josef Moravec 2018-03-09 07:02:30 UTC
Created attachment 72547 [details] [review]
Bug 17978 - Circ rules will not limit holds

The crux of this issue is that all those holds are unfilled record level holds. The code that checks for the number of holds the user already has *always* uses a branchcode of some form. Because of this, any open record level holds are completely ignored for this purpose.

Test Plan:
1) Set ReservesControlBranch to "item's home library"
2) Set an All libraries/All Patrons/All itemtypes rules limiting a patron to 2 holds
3) Place 3 record level holds, you can but you should not be able to!
4) Delete the holds
5) Apply this patch
6) Attempt to place 3 record level holds
7) Note that you cannot!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Marcel de Rooy 2018-03-23 07:01:12 UTC
QA: Looking here
Comment 15 Marcel de Rooy 2018-03-23 07:43:44 UTC
Created attachment 73161 [details] [review]
Bug 17978 - Circ rules will not limit holds

The crux of this issue is that all those holds are unfilled record level holds. The code that checks for the number of holds the user already has *always* uses a branchcode of some form. Because of this, any open record level holds are completely ignored for this purpose.

Test Plan:
1) Set ReservesControlBranch to "item's home library"
2) Set an All libraries/All Patrons/All itemtypes rules limiting a patron to 2 holds
3) Place 3 record level holds, you can but you should not be able to!
4) Delete the holds
5) Apply this patch
6) Attempt to place 3 record level holds
7) Note that you cannot!

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Jonathan Druart 2018-03-26 17:41:19 UTC
OPAC, Staff or both?

How did you test this patch without the fix on bug 18474?

Are we going to fix bug 6976 as well, i.e. we are going to fix the long standing bug, or it's a regression fix?

Please improve the bug report title.

We should also provide test coverage for this patch.
Comment 17 Nick Clemens 2018-12-24 14:52:04 UTC
Created attachment 83476 [details] [review]
Bug 17978: Check if hold can be placed before placing on placerequest.pl

To test:
 1 - Set an All/All/All rule with reserves limited 2 to
 2 - Search in the staff side
 3 - Select all records (or more than 2) from the results
 4 - Click 'Place hold'
 5 - Find a patron, place holds
 6 - You get more holds than you should
 7 - Delete those holds
 8 - Apply patch
 9 - Search and select more than 2 records
10 - Find patron, place holds
11 - Only 2 holds are placed
Comment 18 Nick Clemens 2018-12-24 14:54:15 UTC
This doesn't give feedback on why some holds were not placed, however, this is similar to how the opac functions, holds are placed until they being to violate the circ rules, then they are not - this patch solves the problem, I think improving the feedback can be on a future bug (combine request.pl and placerequest.pl?)
Comment 19 Nick Clemens 2018-12-24 15:21:06 UTC
(In reply to Nick Clemens from comment #18)
> This doesn't give feedback on why some holds were not placed, however, this
> is similar to how the opac functions, holds are placed until they being to
> violate the circ rules, then they are not - this patch solves the problem, I
> think improving the feedback can be on a future bug (combine request.pl and
> placerequest.pl?)

Ah, i missed the reservescontrolbrnach setting - restoring Kyles patch and will followup when I can
Comment 20 Nick Clemens 2018-12-24 15:59:52 UTC
Created attachment 83477 [details] [review]
Bug 17978: Unit tests for changes
Comment 21 Liz Rea 2019-03-20 18:27:31 UTC
Created attachment 86829 [details] [review]
Bug 17978 - Circ rules will not limit holds

The crux of this issue is that all those holds are unfilled record level holds. The code that checks for the number of holds the user already has *always* uses a branchcode of some form. Because of this, any open record level holds are completely ignored for this purpose.

Test Plan:
1) Set ReservesControlBranch to "item's home library"
2) Set an All libraries/All Patrons/All itemtypes rules limiting a patron to 2 holds
3) Place 3 record level holds, you can but you should not be able to!
4) Delete the holds
5) Apply this patch
6) Attempt to place 3 record level holds
7) Note that you cannot!

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 22 Liz Rea 2019-03-20 18:27:35 UTC
Created attachment 86830 [details] [review]
Bug 17978: Check if hold can be placed before placing on placerequest.pl

To test:
 1 - Set an All/All/All rule with reserves limited 2 to
 2 - Search in the staff side
 3 - Select all records (or more than 2) from the results
 4 - Click 'Place hold'
 5 - Find a patron, place holds
 6 - You get more holds than you should
 7 - Delete those holds
 8 - Apply patch
 9 - Search and select more than 2 records
10 - Find patron, place holds
11 - Only 2 holds are placed

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 23 Liz Rea 2019-03-20 18:27:39 UTC
Created attachment 86831 [details] [review]
Bug 17978: Unit tests for changes

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 24 Martin Renvoize 2019-04-01 14:15:51 UTC
Created attachment 87280 [details] [review]
Bug 17978: Circ rules will not limit holds

The crux of this issue is that all those holds are unfilled record level holds. The code that checks for the number of holds the user already has *always* uses a branchcode of some form. Because of this, any open record level holds are completely ignored for this purpose.

Test Plan:
1) Set ReservesControlBranch to "item's home library"
2) Set an All libraries/All Patrons/All itemtypes rules limiting a patron to 2 holds
3) Place 3 record level holds, you can but you should not be able to!
4) Delete the holds
5) Apply this patch
6) Attempt to place 3 record level holds
7) Note that you cannot!

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2019-04-01 14:15:55 UTC
Created attachment 87281 [details] [review]
Bug 17978: Check if hold can be placed before placing on placerequest.pl

To test:
 1 - Set an All/All/All rule with reserves limited 2 to
 2 - Search in the staff side
 3 - Select all records (or more than 2) from the results
 4 - Click 'Place hold'
 5 - Find a patron, place holds
 6 - You get more holds than you should
 7 - Delete those holds
 8 - Apply patch
 9 - Search and select more than 2 records
10 - Find patron, place holds
11 - Only 2 holds are placed

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2019-04-01 14:15:59 UTC
Created attachment 87282 [details] [review]
Bug 17978: Unit tests for changes

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2019-04-01 14:16:29 UTC
Worked well, no regressions found and the QA script is happy.. passing QA.
Comment 28 Nick Clemens 2019-04-10 20:12:06 UTC
Awesome work all!

Pushed to master for 19.05
Comment 29 Martin Renvoize 2019-04-15 11:00:23 UTC
Pushed to 18.11.x for 18.11.05
Comment 30 Lucas Gass 2019-04-16 19:57:49 UTC
cant apply this cleanly to 18.05.x. if needed i'll need a rebase