Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items
Summary: Add the ability to prevent a patron from placing a hold on a record with avai...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 6837 9190 (view as bug list)
Depends on:
Blocks: 16404 18015
  Show dependency treegraph
 
Reported: 2016-01-09 11:19 UTC by Kyle M Hall
Modified: 2017-01-31 10:06 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (18.22 KB, patch)
2016-01-09 11:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (8.83 KB, patch)
2016-02-13 11:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (8.84 KB, patch)
2016-03-07 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (8.78 KB, patch)
2016-03-16 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (8.96 KB, patch)
2016-03-21 22:44 UTC, Andreas Hedström Mace
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Remove superfluous lines from unit tests (774 bytes, patch)
2016-04-18 13:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (9.03 KB, patch)
2016-04-19 06:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Remove superfluous lines from unit tests (847 bytes, patch)
2016-04-19 07:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus (908 bytes, patch)
2016-04-27 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Fix rule labels (2.48 KB, patch)
2016-04-27 14:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Fix rule labels (2.48 KB, patch)
2016-04-27 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (8.99 KB, patch)
2016-04-27 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Remove superfluous lines from unit tests (847 bytes, patch)
2016-04-27 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus (908 bytes, patch)
2016-04-27 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Fix rule labels (2.48 KB, patch)
2016-04-27 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15534 [QA Followup] - Fix rule labels (2.47 KB, patch)
2016-04-27 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 15534 - Add the ability to prevent a patron from placing a hold on a record with available items (9.06 KB, patch)
2016-04-27 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15534 [QA Followup] - Remove superfluous lines from unit tests (917 bytes, patch)
2016-04-27 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15534 [QA Followup] - Use IsItemOnHoldAndFound instead of GetReserveStatus (977 bytes, patch)
2016-04-27 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15534 [QA Followup] - Fix rule labels (2.54 KB, patch)
2016-04-27 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-01-09 11:19:36 UTC
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.
Comment 1 Kyle M Hall 2016-01-09 11:25:07 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2016-01-15 15:11:08 UTC
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?
Comment 3 Kyle M Hall 2016-01-16 12:30:43 UTC
(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?
Comment 4 Kyle M Hall 2016-01-19 14:28:57 UTC
*** Bug 6837 has been marked as a duplicate of this bug. ***
Comment 5 Owen Leonard 2016-01-20 14:51:23 UTC
(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.
Comment 6 Katrin Fischer 2016-01-20 21:43:30 UTC
I think a third option to the former 'allowonshelfholds' sounds good!
Comment 7 Andreas Hedström Mace 2016-02-09 12:29:07 UTC
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.
Comment 8 Andreas Hedström Mace 2016-02-09 12:37:04 UTC
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).
Comment 9 Kyle M Hall 2016-02-13 11:33:43 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2016-02-28 20:20:15 UTC
*** Bug 9190 has been marked as a duplicate of this bug. ***
Comment 11 Koha Team Lyon 3 2016-02-29 13:11:16 UTC
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
Comment 12 Koha Team Lyon 3 2016-02-29 14:46:43 UTC
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
Comment 13 Kyle M Hall 2016-03-07 20:01:43 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2016-03-07 20:07:28 UTC
> 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!
Comment 15 Katrin Fischer 2016-03-14 22:05:30 UTC
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...
Comment 16 Kyle M Hall 2016-03-16 13:25:07 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-03-16 14:33:39 UTC
(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.
Comment 18 Andreas Hedström Mace 2016-03-21 22:44:30 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-04-18 13:07:32 UTC
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 20 Jonathan Druart 2016-04-18 13:16:15 UTC
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.
Comment 21 Kyle M Hall 2016-04-18 13:19:46 UTC
(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!
Comment 22 Kyle M Hall 2016-04-18 13:22:03 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-04-18 13:34:04 UTC
(In reply to Jonathan Druart from comment #19)
> Should not we check the "Reserved" status?

And what about the "Reserved" status?
Comment 24 Kyle M Hall 2016-04-18 14:22:11 UTC
(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?
Comment 25 Jonathan Druart 2016-04-19 06:57:50 UTC
(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.
Comment 26 Jonathan Druart 2016-04-19 06:59:39 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-04-19 07:02:56 UTC
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.
Comment 28 Jonathan Druart 2016-04-19 07:03:19 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2016-04-27 13:01:22 UTC
Looking at this now!
Comment 30 Kyle M Hall 2016-04-27 13:31:47 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2016-04-27 14:08:09 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2016-04-27 14:15:04 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2016-04-27 14:16:24 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2016-04-27 14:16:32 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-04-27 14:16:35 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-04-27 14:16:38 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2016-04-27 14:47:06 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2016-04-27 15:59:03 UTC
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.
Comment 39 Katrin Fischer 2016-04-27 16:04:02 UTC
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>
Comment 40 Katrin Fischer 2016-04-27 16:04:06 UTC
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>
Comment 41 Katrin Fischer 2016-04-27 16:04:09 UTC
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>
Comment 42 Katrin Fischer 2016-04-27 16:04:13 UTC
Created attachment 50827 [details] [review]
[PASSED QA] Bug 15534 [QA Followup] - Fix rule labels

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 43 Kyle M Hall 2016-04-29 10:38:25 UTC
Pushed to master! Should be in the May 2016 release!