Some libraries would like to prevent patrons from placing holds on records/items where there are other items available for the patron to check out.
Created attachment 46463 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on records/items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new rule "Allow hold if others available" 5) Set the rule to "No" 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the patron 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron
I don't think this is the right solution for what is currently a messy problem. As I understand it, the current "allow on shelf holds" feature will let you place a hold if *any* of multiple items is unavailable. I understand that this is The Way It Has Been, but the description of the option in and the documentation do NOT make it explicit that this is the behavior. My initial reaction to this patch is that it's trying to make "allow on shelf holds" work the way it is actually described rather than how it really works. Do libraries who use the "allow on shelf holds" feature *want* it to work that way, or do they use it because it's better than nothing? I think having two settings, "allow on shelf holds" and "allow hold if others available" is needlessly confusing. On the face of it they both look like they do exactly the same thing, and I have no idea how they're supposed to work in conjunction with each other. In my test of this patch I set a global all item types/all patron categories rule with both "allow on shelf holds" and "allow hold if others available" set to "no." However, I was able to place a hold on a title which has one checked-out item and three available. I don't think that's right. There have been other discussions of this issue too which should be taken into account: Bug 6837 and Bug 9190. If we bring it all together is there a single solution which can accommodate everyone's needs?
(In reply to Owen Leonard from comment #2) I think we can establish right now that we can't change the shelf holds behavior. It has been this way so long that it is a fundamental part of how Koha works. We can however, add an alternative. I think the basic issue is that shelfholds = no and allow_hold_if_items_available are fundamentally at odss with each other and should be mutually exclusive. I had though to somehow extend shelfholds, but it's inner workings are completely different than what is needed for allow_hold_if_items_available. It *would* be possible for an additional option in on shelf holds allowed, so the choices would be: Yes Only if at least one item is unavailable Only if all itmes are unavailble This would require a bit more code, changing all the shelfhold checks from if $shelfholds to if $shelfholds == 1, but that isn't too onerous a change. Then I could remove my new rule and use the existing shelfholds rule for the new logic code. How does that sound Owen?
*** Bug 6837 has been marked as a duplicate of this bug. ***
(In reply to Kyle M Hall from comment #3) > It *would* be possible for an additional option in on shelf holds allowed, > so the choices would be: > Yes > Only if at least one item is unavailable > Only if all itmes are unavailble This sounds like a good solution based on what I've heard and read in other bug reports. However since my library doesn't use the feature at all I think it would be good to seek out opinions from people who do.
I think a third option to the former 'allowonshelfholds' sounds good!
Last week I wrote a rather lengthy comment about how I don't see how AllowOnShelfHolds and the proposed AllowHoldIfOtherAvailable are necessarily the same thing and should be kept as two separate sysprefs. But in writing I got unsure if my original concerns are correct. I went through some various setting on the two sysprefs and how I envision they would work together with some tests of how they are handled by the patch now (pasted at the bottom for reference, for those interested - everyone else can skip them). My basic reservation against placing a third option on AllowOnShelfHolds is that there are libraries who don't want to allow on shelf holds that might want to only allow holds when all items are checked out. But I came to realize that the third option would in effect means no on shelf holds. What I wonder though is how a third option for AllowOnShelfHolds would affect connected patches, like bug 15532, and other hold options? I'm having a hard time envisioning the extended effects of such a change. Any thought's there is greatly appreciated! A use case to centre this discussion around: at Stockholm University Library the general rule is that placing holds is only possible when all item are checked out; and no shelf holds allowed. Holds are limited per branch: only holds that have the same pick-up branch as the holding library fills the holds (implemented in bug 15532). However, researchers have special privileges and are allowed to place on shelf holds. So, I guess my basic question is - can a third option on AllowOnShelfHolds work together with the ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold (bug 15532)? ----- How would the two sysprefs work together? I'll shorten "Allow on shelf holds" and "Allow hold if items available" as shelf_holds and allow_if_available here, for easier reference.) Say we have one bib, with two items. To me, it should work this way, if shelf_holds = no and allow_if_available = yes: a) No items checked out - no hold allowed. b) One item checked out (or more) - the checkout item(s) fills the hold when it is returned, the other items are unaffected. Testing his patch now this seem to be the case. The other way around is a little bit more problematic: shelf_holds = yes, allow_if_available = no. a) No items checked out - holds allowed. b) One item checked out – only the item available on the shelf should fill the hold. (A bit unsure about this one though, might require more thought.) Testing the patch, neither of these work – no holds are allowed until they are all checked out. The example Owen mentioned, when both sysprefs are set to no, and being able to place a hold when there are items available seems like a bug to me. I tried the same setting and was not allowed to place a hold... Setting both sysprefs = yes, should work as shelf_holds does today.
I'm wondering if the third option on AllowOnShelfHolds is supposed to fix a technical issue or a pedagogical one? If it's the first it might be the way to go, but if it's the second I'm not so sure it helps (as I mentioned in my previous comment).
Created attachment 48039 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new option for "On shelf holds allowed" 5) Set the rule to "If all unavailable", set "item level holds" to allow 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the item 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron
*** Bug 9190 has been marked as a duplicate of this bug. ***
I've tried to install it on a sandbox : The sandbox you've requested is not ready. Some problems occurred applying patches from bug 15534: <h1>Something went wrong !</h1>Applying: Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Failed to merge in the changes. Patch failed at 0001 Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items
I'm really interested by this bug and I can try to give you my opinion and my feedback. From what I've read here and in the other bugs, I think that the proposition of the syspref allowonshelfholds with 3 option sounds good. I agree that we can keep the current behaviour that allows to put holds if only one item at least is borrowed (but I must say that patrons in my library don't really understand that their hold are not put on available items but only on the borrowed one). I'm looking forward to the new option to be implemented because it's really the behaviour that we expect : that patrons can put holds only if all items that can be hold are checked out. It isn't specified here but if there's items with different item types and that some of them are not defined as "holdable" in issuing rules and are on shelves but that all the "holdable" one are checked out, we should have the possibility to place an hold. And it should be the same if we have items from different branches : one that allows holds and the other not. If there's items available in the branch that doesn't allow holds, but that all the items from the branch that allows holds are checked out, patrons should be able to place a hold. I hope that I'm clear in my explanations... Sonia BOUIS
Created attachment 48764 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new option for "On shelf holds allowed" 5) Set the rule to "If all unavailable", set "item level holds" to allow 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the item 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron
> It isn't specified here but if there's items with different item types and > that some of them are not defined as "holdable" in issuing rules and are on > shelves but that all the "holdable" one are checked out, we should have the > possibility to place an hold. > And it should be the same if we have items from different branches : one > that allows holds and the other not. If there's items available in the > branch that doesn't allow holds, but that all the items from the branch that > allows holds are checked out, patrons should be able to place a hold. Those behaviors are beyond the scope of this enhancement, but further enhancements would be welcome!
Hi Kyle, this explodes right now :( When trying to log in: Can't locate Koha/Itemtypes.pm in @INC (you may need to install the Koha::Itemtypes module) (@INC contains: /home/katrin/kohaclone /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) at /home/katrin/kohaclone/C4/Reserves.pm line 45...
Created attachment 49223 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new option for "On shelf holds allowed" 5) Set the rule to "If all unavailable", set "item level holds" to allow 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the item 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron
(In reply to Katrin Fischer from comment #15) > Hi Kyle, this explodes right now :( When trying to log in: > > Can't locate Koha/Itemtypes.pm in @INC (you may need to install the > Koha::Itemtypes module) (@INC contains: /home/katrin/kohaclone /etc/perl > /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 > /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 > /usr/local/lib/site_perl .) at /home/katrin/kohaclone/C4/Reserves.pm line > 45... Should be fixed. Looks like it was a capitalization problem.
Created attachment 49406 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new option for "On shelf holds allowed" 5) Set the rule to "If all unavailable", set "item level holds" to allow 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the item 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se> Works as intended!
Kyle, Not sure the following makes sense :) Should not we check the "Reserved" status? Why don't we call CanItemBeReserved in addition of the existing checks? The test coverage is quite light, but it is not easy to provide a full test coverage for this kind of changes, there are too many possibilities.
Comment on attachment 49406 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Review of attachment 49406 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ +12,5 @@ > +use t::lib::TestBuilder; > + > +BEGIN { > + use FindBin; > + use lib $FindBin::Bin; These 2 lines are not needed.
(In reply to Jonathan Druart from comment #19) > Kyle, > Not sure the following makes sense :) > Should not we check the "Reserved" status? > Why don't we call CanItemBeReserved in addition of the existing checks? IsAvailableForItemLevelRequest does not currently call CanItemBeReserved, so I did not implement it specifically for this feature. A check for CanItemBeReserved would be better dealt with at the beginning of IsAvailableForItemLevelRequest and would be outside the scope of this specific enhancement. That being said, it does make sense that we should be calling CanItemBeReserved from IsAvailableForItemLevelRequest and I think we should file a bug report for that!
Created attachment 50326 [details] [review] Bug 15534 [QA Followup] - Remove superfluous lines from unit tests
(In reply to Jonathan Druart from comment #19) > Should not we check the "Reserved" status? And what about the "Reserved" status?
(In reply to Jonathan Druart from comment #23) > (In reply to Jonathan Druart from comment #19) > > Should not we check the "Reserved" status? > > And what about the "Reserved" status? The code already checks to see if the item is a waiting hold. Is this not what you are referring to?
(In reply to Kyle M Hall from comment #24) > (In reply to Jonathan Druart from comment #23) > > (In reply to Jonathan Druart from comment #19) > > > Should not we check the "Reserved" status? > > > > And what about the "Reserved" status? > > The code already checks to see if the item is a waiting hold. Is this not > what you are referring to? I was thinking about the "Reserved" status returned by GetReserveStatus, but after a night's sleep, I don't think it's needed actually.
Created attachment 50389 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Browse to the circulation rules 4) Note the new option for "On shelf holds allowed" 5) Set the rule to "If all unavailable", set "item level holds" to allow 6) Find a patron/branch/itemtype applicable to this rule 7) Ensure at least one item on the record is available for the patron to check out 8) Attempt to place a hold for the item 9) Note you cannot place the hold 10) Check the available item out to another patron 11) Note you can now place a hold for the first patron Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se> Works as intended! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Everything looks good to me but I'd like to get another signoff or to see a QAer test this patch deeply. I may have missed something or a tricky situation may not be taken into account. Looking at the discussions on duplicate bugs I guess it will be easy to find someone interested in testing this patch.
Created attachment 50390 [details] [review] Bug 15534 [QA Followup] - Remove superfluous lines from unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Looking at this now!
Created attachment 50802 [details] [review] Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus
Created attachment 50804 [details] [review] Bug 15534 [QA Followup] - Fix rule labels
Created attachment 50805 [details] [review] Bug 15534 [QA Followup] - Fix rule labels
Created attachment 50806 [details] [review] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Browse to the circulation rules 3) Note the new option for "On shelf holds allowed" 4) Set the rule to "If all unavailable", set "item level holds" to allow 5) Find a patron/branch/itemtype applicable to this rule 6) Ensure at least one item on the record is available for the patron to check out 7) Attempt to place a hold for the item 8) Note you cannot place the hold 9) Check the available item out to another patron 10) Note you can now place a hold for the first patron Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se> Works as intended! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 50807 [details] [review] Bug 15534 [QA Followup] - Remove superfluous lines from unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 50808 [details] [review] Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus
Created attachment 50809 [details] [review] Bug 15534 [QA Followup] - Fix rule labels
Created attachment 50813 [details] [review] Bug 15534 [QA Followup] - Fix rule labels
1 = YES (unchanged, we don't care about availability) 2 = If all unavailable (new behaviour) 0 = If any unavailable (former 'no' = old behaviour) Note for 0: old behaviour is also, that item level holds can only be placed on checked out items. Testing with a all-all-all rule at first: 1 item: 1 checked out - Yes: Can place hold - If all unavailable: Can place hold - If any unavailable: Can place hold 1 item: 1 available - Yes: Can place hold - If all unavailable: No hold - If any unavailable: No hold 2 items: 1 checked out, 1 available - Yes: Can place hold, item holds can be placed on both items - If all unavailable: No hold - If any unavailable: Can place hold, item hold only on checked out item 2 items: 1 checked out, 1 available and damaged AllowHoldsOnDamagedItems = No holds - Yes: Can place hold, item hold only on checked out item - If all unavailable: damaged item is not available = can place hold, item level on checked out - If any unavailable: Can place hold, item hold only on cheked out item AllowHoldsOnDamagedItems = Holds allowed - If all unavailable: No hold, damaged item is counted available 2 items: both available, different itemtypes A and B A B 1:0 can place holds, item level only on A 1:1 can place holds, item level on both 1:2 can place holds, item level only on A 2:2 no hold 2:0 no hold 0:0 no hold 2 items: both available, different itemtypes A and B, B checked out A B 1:0 can place holds, item level on both 1:1 can place holds, item level on both 1:2 can place holds, item level only on A 2:2 no hold. 2:0 can place holds, item level only on B 0:0 can place holds, item level only on B and this time also: 0:1 can place holds, item level only on B 2:1 can place holds, item level only on B 0:2 no hold Note: for 2 all items on the record are taken into account, independent of the rule applying to them. More tests could be done, but looking a the code and the results I believe this to be working as intended.
Created attachment 50824 [details] [review] [PASSED QA] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items Some libraries would like to prevent patrons from placing holds on items where there are other items available for the patron to check out. Test Plan: 1) Apply this patch 2) Browse to the circulation rules 3) Note the new option for "On shelf holds allowed" 4) Set the rule to "If all unavailable", set "item level holds" to allow 5) Find a patron/branch/itemtype applicable to this rule 6) Ensure at least one item on the record is available for the patron to check out 7) Attempt to place a hold for the item 8) Note you cannot place the hold 9) Check the available item out to another patron 10) Note you can now place a hold for the first patron Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se> Works as intended! Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 50825 [details] [review] [PASSED QA] Bug 15534 [QA Followup] - Remove superfluous lines from unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 50826 [details] [review] [PASSED QA] Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 50827 [details] [review] [PASSED QA] Bug 15534 [QA Followup] - Fix rule labels Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master! Should be in the May 2016 release!