Description
Caroline Cyr La Rose
2022-04-15 20:10:16 UTC
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 |