When circulation rules don't allow on shelf holds, the "Place hold" button still appears on the OPAC next to available items. 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record The Place hold button should not appear, but currently, it does. I think this may be related to Bug 3142? I'm not sure...
Created attachment 133497 [details] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds holds allowed is on "If all unavailable", the Place hold button is not showed. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there.
Created attachment 133608 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds holds allowed is on "If all unavailable", the Place hold button is not showed. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there.
Created attachment 134172 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there.
Created attachment 134420 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber
Created attachment 134421 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early
The last 2 patches are not directly related to this bug but they should have been there in the first place to not cause any errors when borrowernumber or patron are undefined.
Created attachment 134436 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 134437 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com>
Created attachment 134438 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com>
I noted when testing that the "Place hold" button is also not displayed when the "If any unavailable" option is selected. I assume that this is OK - I've never really got my head around this setting!
Created attachment 134617 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 134619 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 134620 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 134621 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Please don't forget to assign yourself to the bug :)
We can probably optimize this bug fix if https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 goes through.
This bug fix could be optimized if https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 goes through.
Sorry, sent the same message twice...
Created attachment 135395 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
(In reply to David Nind from comment #10) > I noted when testing that the "Place hold" button is also not displayed when > the "If any unavailable" option is selected. I assume that this is OK - I've > never really got my head around this setting! It's awkward language, it boils down to "If I could walk into the library and each of the items on this record are something I could check out right now, don't allow holds." So if I have 10 items on a record, and all down are able to be checked out, don't allow holds. But, if even one is checked out, allow holds.
Quick and dirty perf testing, one page load per: 500 Items Pre-patch ISBD: 2.5 MARC: 6.21 Details: 4.51 Post-patch ISBD: 2.84 MARC: 6.15 Details: 5.86 3000 Items Pre-patch ISBD: 6.28 MARC: 25.32 Details: 23.48 Post-patch ISBD: 6.07 MARC: 25.55 Details: 23.51
3000 Items Post-patch, rule = "If any unavailable" ISBD: 27.01 MARC: 44.76 Details: 43.51
It appears that this patch definitely has a noticeable effect on page loads at scale. The most immediate blocker at this point is a lack of unit tests for the changes to the Perl modules. Please provide unit tests for the changes to CanBookBeReserved and IsAvailableForItemLevelRequest.
Created attachment 137065 [details] [review] Bug 30556: (follow-up) Unit tests
Created attachment 139289 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 139290 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 139291 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 139292 [details] [review] Bug 30556: (follow-up) Unit tests
In your code I see: +my $currentbranch = C4::Context->userenv ? C4::Context->userenv->{branch} : undef; + $is_available = IsAvailableForItemLevelRequest($item, $patron, $currentbranch) + unless $is_available; But when I look at the POD for IsAvailableForItemLevelRequest the third parameter is supposed to be the pick-up location, not the logged-in user's branch: https://git.koha-community.org/Koha-community/Koha/src/commit/9408a443c24a5424828f62b48fa1dda04fdf3512/C4/Reserves.pm#L1323 my $is_available = IsAvailableForItemLevelRequest( $item_record, $borrower_record, $pickup_branchcode ); I believe this isn't correct. A system might allow you to place a hold for pickup at the item's home library, which might not be the patron's home library.
Created attachment 144429 [details] [review] Bug 30556: Changing the logged-in user's branch by pick-up location
Created attachment 147378 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 147379 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 147380 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 147381 [details] [review] Bug 30556: (follow-up) Unit tests
Created attachment 147382 [details] [review] Bug 30556: Changing the logged-in user's branch by pick-up location
Rebased
Created attachment 147441 [details] [review] Bug 30846: 'If any unavailable' doesn't consider negative notforlan values as unavailable When we set up a circulation rule where 'On shelf holds allowed' is 'If any unavailable' and we have a record with one 'Ordered' item, we cannot place this item on hold. This patch allows placing hold on item with negative not for loan values, when using rule with 'On shelf holds allowed' set to 'If any unavailable' To test: 1. Set up a circulation rule where on shelf holds are not allowed and force the choosing of an item (to facilitate the test) 1.1. Go to Administration > Circulation and fines rules 1.2. In the matrix, add a circulation like this - Patron category: All - Item type: Books - Current checkouts allowed: 10 - Current on-site checkouts allowed: 10 - Loan period: 21 - Holds allowed (total): 10 - Holds allowed (daily): 10 - Holds per record (count): 10 - On shelf holds allowed: If any unavailable - OPAC item level holds: Force 1.3. Click Save 2. Create a record with one 'Ordered' item (or any negative value not for loan status) 2.1. Go to Cataloging 2.2. Click New record 2.3. Fill out the mandatory fields (by default in MARC21: 000, 003, 005, 008, 040, 245, and 942 (942 should be set to Books)) 2.4. Click Save 2.5. Fill out the following item fields - Not for loan: Ordered - Koha item type: Books 2.6. Click Add item 2.7. Click Normal to go to the detailed record 3. Try to place a hold on the 'Ordered' item 3.1. From the detailed record, click OPAC view: Open in new window. --> Note that the 'Place hold' option is not present 4. Add a second 'Available' item 4.1. Back in the staff interface tab with the detailed record, click New > New item 4.2. Make sure the item type is set to Books 4.3. Add a barcode in p 4.4. Click Add item 5. Try again to place a hold on the 'Ordered' item 5.1. Go back to the OPAC tab and refresh the page --> Note that the 'Place hold' option is still not present 6. Check out the available item to a patron 6.1. In the staff interface tab, copy the barcode from the available item 6.2. Go to Patrons 6.3. Click on Search 6.4. Click Check out next to one of the patrons 6.5. Paste the barcode in the box and click Check out 7. Try again to place a hold on the 'Ordered' item 7.1. Go back to the OPAC tab and refresh the page --> Note that the 'Place hold' option is now present 7.2. Click Place hold --> Note that only the checked out item is available to place on hold, if you click Show unholdable items, it will show the Ordered item, but you can't place a hold on it. 8. Apply the patch 9. Go to the OPAC tab and click on the book title right next to 'Place a hold on' checkbox to go back to the record details. --> Note that the 'Place hold' option is still present 9.1. Click Place hold --> Note that you can now place a hold on the 'Checked out' or the 'Ordered' item. 10. Check in the item to make it available again 10.1. In the staff interface tab, click on 'Show checkouts' button 10.2. Select the Checked out item and click on 'Renew or check in selected items' button. 11. Try again to place a hold on the 'Ordered' item 11.1. Go back to the OPAC tab and click on the book title right next to 'Place a hold on' checkbox to go back to the record details. --> Note that the 'Place hold' option is still present 11.2. Click Place hold --> Note that only the 'Ordered' item is available to place on hold, if you click Show unholdable items, it will show the Available item and you can't place a hold on it. 12. Delete the available item to keep only the Ordered item 12.1 in the staff interface tab, click on 'Search catalog' and search for the record 12.2 click on 'Edit' then 'Edit items' 12.3 Delete the available item 13. Try to place a hold on the remain 'Ordered' item 13.1 Go back to the OPAC tab and click on the book title right next to 'Place a hold on' checkbox to go back to the record details. --> Note that the 'Place hold' option is present 13.2. Click Place hold --> Note that you can place a hold on the Ordered item. Signed-off-by: Amaury GAU <amaury.gau@bulac.fr>
Created attachment 147442 [details] [review] Bug 30846: (follow-up) Unit tests
Created attachment 150218 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150219 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150220 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150221 [details] [review] Bug 30556: (follow-up) Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 150222 [details] [review] Bug 30556: Changing the logged-in user's branch by pick-up location Signed-off-by: David Nind <david@davidnind.com>
1. + return { status => '' } unless $borrowernumber; + return 0 unless $patron; No, you can raise an exception if you want. 2. There is no explanation on the logic changes. Can you detail? 3. Code is duplicated in controller scripts, that feels wrong. We certainly want it in a module, covered by tests.
See last comment from Joubu.
Created attachment 156188 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156189 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156190 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156191 [details] [review] Bug 30556: (follow-up) Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156192 [details] Bug 30556: Changing the logged-in user's branch by pick-up location Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156429 [details] [review] Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved
I'm not getting the expected results for the 'On shelf holds allowed', both before and after the patch is applied. Maybe this is something to do with Bug 34886 - Regression in when hold button appears? Set up: - Changed the circulation and fines rules for all patron categories and item types (basically, just used one rule) - Used Mary Burton as the patron Before the patch is applied, I am getting this behavour: - On shelf holds allowed: Yes . Not logged in to OPAC: no 'Place hold' option . Logged in to OPAC: can place a hold - On shelf holds allowed: If any unavailable . Not logged in to OPAC: no 'Place hold' option . Logged in to OPAC: no 'Place hold' option - On shelf holds allowed: If all unavailable . Not logged in to OPAC: no 'Place hold' option . Logged in to OPAC: no 'Place hold' option After the patch is applied, I am getting this behavour: - On shelf holds allowed: Yes . Not logged in to OPAC: no 'Place hold' option . Logged in to OPAC: can place a hold - On shelf holds allowed: If any unavailable . Not logged in to OPAC: can place a hold . Logged in to OPAC: no 'Place hold' option - On shelf holds allowed: If all unavailable . Not logged in to OPAC: can place a hold . Logged in to OPAC: no 'Place hold' option
Hi David, I tested the patch with the setup you mentioned, and I didn't obtain the same result as you after applying the patch. Here are the results I obtained: - On-shelf holds allowed: Yes . Not logged in to OPAC: Can place a hold (we don’t have the same result here) . Logged in to OPAC: Can place a hold - On-shelf holds allowed: If any unavailable . Not logged in to OPAC: Can place a hold . Logged in to OPAC: No 'Place hold' option - On-shelf holds allowed: If all unavailable . Not logged in to OPAC: Can place a hold . Logged in to OPAC: No 'Place hold' option I conducted the tests twice and obtained the same results: - First time: I used a notice with only one item. This item had the "Book" itype, wasn't checked out, and had an available status. - Second time: I used a notice with two items. These items had the "Book" itype, were not checked out, and had an available status. Can you describe the notice and the item(s) (itype, status, checked out or not, availability) that you used, and I will attempt to reproduce the issue on my end?
Created attachment 157087 [details] [review] Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157088 [details] [review] Bug 30556: Fix IsAvailableForItemLevelRequest to return early Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157089 [details] [review] Bug 30556: Place hold button doesn't show when not allowed When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown. To test: 1. Create a circulation rule with on shelf holds set to "If all unavailable" 2. Create or modify a record to match the items itype to the circulation rule 3. Use a patron matching the circ rule category to log into the opac 4. Look for the record 5. Notice that the Place hold button is there, even thought it's not allowed 6. Apply the patch. 7. The button is not there. 8. Logout of the opac. 9. The button is here again since it should always show when not logged in. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157090 [details] [review] Bug 30556: (follow-up) Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157091 [details] [review] Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved
The patch has been rebased
The problem doesn't seem to happen anymore, now that Bug 30846 is on the stack. We can close this bug.