bug 11250 changed the holds message to be and if,elsif loop, falling through to catch the first appropriate message bug 13116 changed the reporting from CanItemBeReserved causing tooManyHolds to trigger when no holds are allowed A side effect is that 'maxreserves' is only passed to the template when there are too many reserves, but the too many reserves message is called when no holds are available To recreate: Set maxreserves to 50 Set some item type to allow 0 holds Attempt to place a hold on a record with that itemtype Message will be "Too many holds: [Firstname Surname] can only place a maximum of total holds." Message should be "[Firstname Surname] cannot place any holds on these items."
Created attachment 52652 [details] [review] Bug 16787 - 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds To test: 1 - Set an item type to allow no holds 2 - Attempt to place a hold for a patron 3 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 4 - Try placing a multihold with the record above and a holdable record, message should end "...cannot place holds on some of these titles' items"
needs unit tests
Tried to test in the sandbox but I kept getting an error message saying "CONFLICT (content): Merge conflict in C4/Reserves.pm" George
Nick have you made any further progress on this? This bug is definitely confusing and we'd love it resolved at my library.
Created attachment 68894 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds To test: 1 - Set an item type to allow no holds 2 - Attempt to place a hold for a patron 3 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 4 - Try placing a multihold with the record above and a holdable record, message should end "...cannot place holds on some of these titles' items"
Created attachment 68895 [details] [review] Bug 16787: Add tests
Is there a step missing form the test plan? I applied the patch, change my Circulation rules to set no holds for All/All (I have no other rules for my test patron's category) and tried to place a hold but got the message "No available items.", but I can see it as available on the OPAC detail page.
(In reply to David Bourgault from comment #7) > Is there a step missing form the test plan? I applied the patch, change my > Circulation rules to set no holds for All/All (I have no other rules for my > test patron's category) and tried to place a hold but got the message "No > available items.", but I can see it as available on the OPAC detail page. Sorry, I did miss a detail, this patch only affects the staff side, not the OPAC
conflicts: ~/d/koha ❯❯❯ git bz apply 16787 Bug 16787 - 'Too many holds' message appears inappropriately and is missing data 68894 - Bug 16787: 'Too many holds' message appears inappropriately and is missing data 68895 - Bug 16787: Add tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 16787: 'Too many holds' message appears inappropriately and is missing data Using index info to reconstruct a base tree... M C4/Reserves.pm M koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt M reserve/request.pl Falling back to patching base and 3-way merge... Auto-merging reserve/request.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Auto-merging C4/Reserves.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 16787: 'Too many holds' message appears inappropriately and is missing data The copy of the patch that failed is found in: .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-16787-Too-many-holds-message-appears-inappropr-CKsj5R.patch
Created attachment 72827 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds To test: 1 - Set an item type to allow no holds 2 - Attempt to place a hold for a patron 3 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 4 - Try placing a multihold with the record above and a holdable record, message should end "...cannot place holds on some of these titles' items"
Created attachment 72828 [details] [review] Bug 16787: Add tests
Created attachment 72889 [details] [review] Bug 16787: (follow-up) Return noReservesAllowed if 0 holds per record To test: 1 - Set 'Holds per record' to 0 for some itemtype/patorn combination 2 - Attempt to plac a hold 3 - Will see 'Too many..' 4 - Apply patch 5 - Try again 6 - Should now see 'No holds..' 7 - prove -v t/db_dependent/Holds.t
tested on biblibre sandbox 11 : - Set maxreserves to 50 - Set 'Holds per record' to 0 for some itemtype/patron combination - Attempt to place a hold No message in any case appears
i've tried to install it on a sandbox : CONFLICT (content): Merge conflict in reserve/request.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt The previous status was "failed QA" so there's also to review the patch as it doesn't seem to function correctly.
Created attachment 91146 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds To test: 1 - Set an item type to allow no holds 2 - Attempt to place a hold for a patron 3 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 4 - Try placing a multihold with the record above and a holdable record, message should end "...cannot place holds on some of these titles' items"
Created attachment 91147 [details] [review] Bug 16787: Add tests
Created attachment 91148 [details] [review] Bug 16787: (follow-up) Return noReservesAllowed if 0 holds per record To test: 1 - Set 'Holds per record' to 0 for some itemtype/patorn combination 2 - Attempt to plac a hold 3 - Will see 'Too many..' 4 - Apply patch 5 - Try again 6 - Should now see 'No holds..' 7 - prove -v t/db_dependent/Holds.t
Created attachment 91149 [details] [review] Bug 16787: Rebase fixes and updates
Patch doesn't apply: Applying: Bug 16787: 'Too many holds' message appears inappropriately and is missing data Applying: Bug 16787: Add tests Using index info to reconstruct a base tree... 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 error: Failed to merge in the changes. Patch failed at 0001 Bug 16787: Add tests
Created attachment 96152 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t
Created attachment 97403 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t CONFLICT (content): Merge conflict in C4/Reserves.pm
Looks like you will need to add the new statuses to the enum of club_holds_to_patron_holds.error_code
Created attachment 99949 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t
Created attachment 99950 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Missing signoff here.
Created attachment 100951 [details] [review] Bug 16787: DBIC changes
Adding the DBIC changes to help testing in a sandbox. AM123, Lyon3, are you still interested in this bugfix?
Created attachment 101473 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t
Created attachment 101474 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Created attachment 101475 [details] [review] Bug 16787: DO NOT PUSH - Shcema changes
Patch no longer applies.
Created attachment 101895 [details] [review] Bug 16787: DO NOT PUSH - Schema updates
Bug 23404 also fixed a itemtype issue in holds
> This patch also fixes a FIXME about using effective_itemtype to fetch item rules Looks like there are several places where GetBranchItemRule() is called with $item->itype instead of $item->effective_itemtype. Maybe we should create a new Bug for that.
> 4 - The message will be 'Too many holds' Is that valid if my messages are the following? > Too many holds: $EXAMPLE $USER can only place a maximum of 0 total holds. It's for the «itemtype to allow no holds» it's a book went to circ rules and added a rule: all branches, all categories, type books: Holds allowed (total): 0 The other record I get > Too many holds for this record: $EXAMPLE $USER can only place a maximum of 0 hold(s) on this record. It's for the «Holds per record' to 0 for another itemtype/patron combination» it for "music" and I added a rule; all branches, all categories, type music: Holds per record (count): 0
Patch doesn't apply.
Created attachment 107793 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "No holds allowed: [Firstname Surname] cannot place holds on any of these items" 7 - Try placing a multihold with either record above and a holdable record, message should end "...cannot place holds on some of these titles' items" 8 - prove -v t/db_dependent/Holds.t
Created attachment 107794 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Created attachment 107795 [details] [review] Bug 16787: DO NOT PUSH - Schema updates
I'm getting errors when trying to place club holds. - create rule that says 0 holds allowed for a specific category and item - create a club - add 2 patrons to club -- one of the category from your rule, one of a category that can hold that item type - attempt to place club hold for an item of the item type covered by your rule - get error "Something went wrong, check Koha logs for details." When placing individual holds, I'm not seeing a message of "No holds allowed: [Firstname Surname] cannot place holds on any of these items." Instead, I get "Cannot place hold: No items are available to be placed on hold." I'm happy enough with that message, but it's not what the test plan indicated.
Created attachment 114675 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t
Created attachment 114676 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Created attachment 114677 [details] [review] Bug 16787: DO NOT PUSH - Schema updates
Needs rebase once more... CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Auto-merging C4/Reserves.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 16787: 'Too many holds' message appears inappropriately and is missing data
Created attachment 119279 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t
Created attachment 119280 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Created attachment 119281 [details] [review] Bug 16787: DO NOT PUSH - Schema updates
Created attachment 119335 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t Rebase - Fix test expectation
Created attachment 119336 [details] [review] Bug 16787: Add noReservesAllowed to club holds
Created attachment 119337 [details] [review] Bug 16787: DO NOT PUSH - Schema updates
Created attachment 119616 [details] Screenshot showing the behavior before the patch I see a difference in how the holds process behaves before and after this patch and I'm not sure it's intentional. It seems to be a regression. It happens when I try to access the multi-hold process by linking directly to the hold page. The search results page will prevent me from submitting multiple titles for holds if one of them can't be placed on hold, but I can use a URL: /cgi-bin/koha/reserve/request.pl?biblionumbers=2329/2596/4796/4439/4896/2655/2740/ ...where one of those biblionumbers is a title with all non-holdable items. From that page, select a patron and submit. The following page will warn you that one of the titles you submitted has no items which can be placed on hold.
Created attachment 119617 [details] Screenshot showing the behavior after the patch After the patch, there is no warning. I know that I'm describing a workflow that isn't one which will be followed under regular circumstances (accessing the page via a handmade URL), but I think the page should still be performing this check.
(In reply to Owen Leonard from comment #52) > I see a difference in how the holds process behaves before and after this > patch and I'm not sure it's intentional. This is not a problem after all because I had my item-level_itypes preference switched and I didn't realize it. After testing with item-level_itypes ON and OFF, the behavior is correct.
Created attachment 119619 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t Rebase - Fix test expectation Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 119620 [details] [review] Bug 16787: Add noReservesAllowed to club holds Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 119621 [details] [review] Bug 16787: DO NOT PUSH - Schema updates Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 119622 [details] [review] Bug 16787: (follow-up) Fix check to ensure reasons are passed to template
Created attachment 119623 [details] [review] Bug 16787: (follow-up) Fix check to ensure reasons are passed to template Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 119738 [details] [review] Bug 16787: 'Too many holds' message appears inappropriately and is missing data This patch alters C4/Reserves.pm to pass back 'noReservesAllowed' when allowedreserves=0. This allows passing to the user an appropriate message about the availability of items for holds This patch also fixes a FIXME about using effective_itemtype to fetch item rules To test: 1 - Set one itemtype to allow no holds 2 - Set 'Holds per record' to 0 for another itemtype/patron combination 3 - Create or find 2 records, each with items only of the itemtypes above 3 - Attempt to place a hold for a patron on each record above 4 - The message will be 'Too many holds' 5 - Apply patch and repeat 6 - Message should be "Cannot place hold: no item are available to be placed on hold" 7 - Try placing a multihold with either record above and a holdable record, message should end "Cannot place hold on some items' 8 - prove -v t/db_dependent/Holds.t Rebase - Fix test expectation Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 119739 [details] [review] Bug 16787: Add noReservesAllowed to club holds Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 119740 [details] [review] Bug 16787: DO NOT PUSH - Schema updates Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 119741 [details] [review] Bug 16787: (follow-up) Fix check to ensure reasons are passed to template Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Tests pass, QA script is happy.. functionality is improved. Passing QA
At first glance I'd say noReservesAllowed is missing from ERROR_MAP, could you confirm?
Created attachment 119942 [details] [review] Bug 16787: (follow-up) Add noReservesAllowed to ERROR_MAP
Pushed to master for 21.05, thanks to everybody involved!
(In reply to Jonathan Druart from comment #67) > Pushed to master for 21.05, thanks to everybody involved! Delayed for a bit.
We have a test failing, please fix. # Failed test 'Too many reserves' # at t/db_dependent/ILSDI_Services.t line 372. # got: 'noReservesAllowed' # expected: 'tooManyReserves' # Looks like you failed 1 test of 8.
Created attachment 120009 [details] [review] Bug 16787: (follow-up) Add and adjust ILSDI tests
Follow-up pushed to master, thanks Nick!
Should we backported to 20.11.x ? Or is it to much behavior change for a stable version ? My opinion is more on no backport, reserves is really sensitive.