Created attachment 63157 [details] [review] Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable", default hold policy "From home library" is ignored. Test plan: - Have a test user from one branch (eg Centerville) - Set "On shelf holds allowed" to "If all unavailable" for your patron and item category (or everyone and everything) - For "Default checkout, hold and return policy", set hold policy to "From home library" -> make sure there is no "Default holds policy by item type" to override the setting - Have two items for a record. 1. An item with home branch same as test user (eg Centerville) -> check this item out to somebody else 2. an item with a different home branch (eg Fairfield) -> available, not checked out - Try to place a hold for your test user. Does not work. - Apply the patch - Try to place a hold. Should work now.
Created attachment 63227 [details] [review] Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable", default hold policy "From home library" is ignored. Test plan: - Have a test user from one branch (eg Centerville) - Set "On shelf holds allowed" to "If all unavailable" for your patron and item category (or everyone and everything) - For "Default checkout, hold and return policy", set hold policy to "From home library" -> make sure there is no "Default holds policy by item type" to override the setting - Have two items for a record. 1. An item with home branch same as test user (eg Centerville) -> check this item out to somebody else 2. an item with a different home branch (eg Fairfield) -> available, not checked out - Try to place a hold for your test user. Does not work. - Apply the patch - Try to place a hold. Should work now. Followed test plan, worked as intended Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
I tested it in a local installation (16.11.07) but can not make it works... (see #18366)
(In reply to Alex Buckley from comment #2) > Created attachment 63227 [details] [review] [review] > Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default > hold policy > > If in the circ rules matrix you set "On shelf holds allowed" to "If all > unavailable", > default hold policy "From home library" is ignored. > > Test plan: > > - Have a test user from one branch (eg Centerville) > > - Set "On shelf holds allowed" to "If all unavailable" for your patron and > item > category (or everyone and everything) > - For "Default checkout, hold and return policy", set hold policy to "From > home library" > -> make sure there is no "Default holds policy by item type" to override > the setting > > - Have two items for a record. > 1. An item with home branch same as test user (eg Centerville) > -> check this item out to somebody else > 2. an item with a different home branch (eg Fairfield) > -> available, not checked out > - Try to place a hold for your test user. Does not work. > > - Apply the patch > - Try to place a hold. Should work now. > > Followed test plan, worked as intended > Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Alex, this is a great fix but does not work. I cannot figure the reason...
(In reply to Claudio from comment #4) > Alex, this is a great fix but does not work. I cannot figure the reason... We use this patch in production and it works. Maybe you have another problem related to this feature which is not covered by the patch.
(In reply to Mirko Tietgen from comment #5) > (In reply to Claudio from comment #4) > > > Alex, this is a great fix but does not work. I cannot figure the reason... > > We use this patch in production and it works. Maybe you have another problem > related to this feature which is not covered by the patch. Thanks Mirko! I noticed that changing ReservesControlBranch syspref to "item's home library" the fix works (in my Koha installation ReservesControlBranch was set to "patron's home library"). Is it right?
Any chances to see tests covering this change?
(In reply to Jonathan Druart from comment #7) > Any chances to see tests covering this change? Changing status.
Any chance to get this moving again? I think this is one of the things that a lot of libraries would like.
*** Bug 18366 has been marked as a duplicate of this bug. ***
Created attachment 66675 [details] [review] Add tests
(In reply to Jonathan Druart from comment #11) > Created attachment 66675 [details] [review] [review] > Add tests Make sure these tests make sense!
I'm not sure of the test plan and expected result. Do we have to place the hold from the OPAC or staff interface? And for the result, is it supposed to override the default holds policy? - I changed one of my circulation rules to If all unavailable for one of my patron categories (x all item types). - Fixed a record so that one of the items was in the home library of the user and one was in another branch (both current location and home library) - Check out the item that was in the same home library as the patron from the fiex patron category to another user - Tried to place a hold for my patron in the staff interface. It says "Not on hold OR Due DD/MM/AAAA Item level hold not allowed from OPAC. There is a little yellow "!" triangle icon next to BOTH items but I can still override the circ rule. I have a pop-up saying "This item normally cannot be put on hold except for patrons from (2nd branch), Place hold on this item?" - From the OPAC, I logged in as my user and tried to place the hold and it says "Sorry non of these items can be placed on hold." / "No available items". - After the patch, the yellow "!" triangle icon is ONLY next to the item from the 2nd location (not patron's home library) in the staff interface. So, no problem to reserve the checked out item, but I have to override to reserve the available item. - From the OPAC, I was able to place my hold on the bib record without problem. I went into the "Holds to pull" report and saw that the item from the 2nd location was there, waiting to be pulled from the shelf.
Created attachment 68854 [details] [review] Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable", default hold policy "From home library" is ignored. Test plan: - Have a test user from one branch (eg Centerville) - Set "On shelf holds allowed" to "If all unavailable" for your patron and item category (or everyone and everything) - For "Default checkout, hold and return policy", set hold policy to "From home library" -> make sure there is no "Default holds policy by item type" to override the setting - Have two items for a record. 1. An item with home branch same as test user (eg Centerville) -> check this item out to somebody else 2. an item with a different home branch (eg Fairfield) -> available, not checked out - Try to place a hold for your test user. Does not work. - Apply the patch - Try to place a hold. Should work now. Followed test plan, worked as intended Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Followed test plan in the intranet and OPAC, worked as intended. With few assumptions made: - when the hold works, should be on the item 1 (Centerville) item 2 is only there to trigger the bug - the circ rules were identically setup on each branch and in "Standard rules for all libraries" Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
@Caroline Cyr La Rose I got results consistent with yours. Thanks for raising the OPAC vs intranet issue, I didn't think of this when first trying the test plan. And it seems that the test plan was written for the OPAC as more details are needed for doing it in the intranet. (related the overriding the circ rule (disabling it or ignoring it))
Glad to see this one moving, thx for taking the time to test!
*** Bug 19543 has been marked as a duplicate of this bug. ***
Created attachment 69125 [details] [review] Add tests https://bugs.koha-community.org/show_bug.cgi?id=18547 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 69126 [details] [review] Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable", default hold policy "From home library" is ignored. Test plan: - Have a test user from one branch (eg Centerville) - Set "On shelf holds allowed" to "If all unavailable" for your patron and item category (or everyone and everything) - For "Default checkout, hold and return policy", set hold policy to "From home library" -> make sure there is no "Default holds policy by item type" to override the setting - Have two items for a record. 1. An item with home branch same as test user (eg Centerville) -> check this item out to somebody else 2. an item with a different home branch (eg Fairfield) -> available, not checked out - Try to place a hold for your test user. Does not work. - Apply the patch - Try to place a hold. Should work now. Followed test plan, worked as intended Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Followed test plan in the intranet and OPAC, worked as intended. With few assumptions made: - when the hold works, should be on the item 1 (Centerville) item 2 is only there to trigger the bug - the circ rules were identically setup on each branch and in "Standard rules for all libraries" Signed-off-by: Victor Grousset <victor.grousset@biblibre.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 69127 [details] [review] Bug 18547: QA Followup Added comments to make clearer Tests had some confusion with names and results, though they do seem to cover the changes here. I believe the tests highlight that ReserveControlBranch has no effect, and possibly it should? Beyond the scope here but left those tests for future work to be done Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 17.11, thanks to everybody involved!
I was looking at 4319 that also proposes changes for this sub in Reserves, and somehow came here. The changes here seem too selective to me. Why are you looking at $branchitemrule->{holdallowed} == 1 only? Why do you not check the 0 value (no holds allowed) ? And why do you add this test only for If all unavailable? Should it not be tested globally, over the different values of On shelf holds allowed? Does not look good to me. Do I overlook something ?
Removed Academy. Should (generally) not be added to Holds patches.
Comment on attachment 69126 [details] [review] Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy Review of attachment 69126 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ +1161,5 @@ > > foreach my $i (@items) { > + > + my $circ_control_branch = C4::Circulation::_GetCircControlBranch( $i->unblessed(), $borrower ); > + my $branchitemrule = C4::Circulation::GetBranchItemRule( $circ_control_branch, $i->itype ); Hum, should not $i->itype be $i->effective_itemtype?