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.
I've tested this and the hold limit did not work for me either.
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.
Found flaws in my testing. I was able to get this to work as expected.
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.
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.
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.
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.
It's a long standing bug and it will need quite a lot of work to fix it.
(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?
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.
See bug 6976.
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!
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>
QA: Looking here
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>
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.
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
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?)
(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
Created attachment 83477 [details] [review] Bug 17978: Unit tests for changes
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>
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>
Created attachment 86831 [details] [review] Bug 17978: Unit tests for changes Signed-off-by: Liz Rea <wizzyrea@gmail.com>
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>
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>
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>
Worked well, no regressions found and the QA script is happy.. passing QA.
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.05
cant apply this cleanly to 18.05.x. if needed i'll need a rebase