Bug 10314 - CanItemBeReserved does not respect the holds policies
Summary: CanItemBeReserved does not respect the holds policies
Status: RESOLVED DUPLICATE of bug 13687
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Barton Chittenden
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-23 11:15 UTC by Kyle M Hall
Modified: 2015-10-15 17:32 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10314 - CanItemBeReserved does not respect the holds policies (2.84 KB, patch)
2013-05-23 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - Tidy up CanItemBeReserved (5.65 KB, patch)
2013-05-23 11:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies (2.90 KB, patch)
2013-05-30 03:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - Tidy up CanItemBeReserved (5.70 KB, patch)
2013-05-30 04:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - QA Bugfix (913 bytes, patch)
2013-08-09 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10314 - CanItemBeReserved does not respect the holds policies (3.00 KB, patch)
2013-08-20 05:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - Tidy up CanItemBeReserved (5.66 KB, patch)
2013-08-20 05:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - QA Bugfix (1.12 KB, patch)
2013-08-20 05:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies (3.15 KB, patch)
2015-07-02 11:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - QA Bugfix (1.11 KB, patch)
2015-07-02 11:50 UTC, Kyle M Hall
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 2013-05-23 11:15:43 UTC
CanItemBeReserved will return 1 as long as the circulation rules have a non-zero number of holds allowed and the patron hasn't reached his or her "Holds allowed" count. However, the subroutine completely ignores the holds policies which would contravene reserving the item even if the patron has not reached the maximum holds allowed count.
Comment 1 Kyle M Hall 2013-05-23 11:20:41 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-23 11:21:32 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2013-05-30 03:59:23 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2013-05-30 04:00:02 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2013-07-14 07:30:09 UTC
Dunno how I managed to sign off on this, 

But I am now getting 

Software error:

Undefined subroutine &C4::Reserves::GetBranchItemRule called at /home/chrisc/git/catalyst-koha/C4/Reserves.pm line 437.

When trying to place a hold in the opac
Comment 6 Kyle M Hall 2013-08-09 14:30:43 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2013-08-20 05:51:22 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2013-08-20 05:51:37 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-08-20 05:51:45 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-08-20 05:53:30 UTC
Passing QA as this is kind of a bad bug - we shouldn't offer configuration options that are not taken into account. But some unit tests would be really nice to have...
Comment 11 Galen Charlton 2013-08-20 16:20:17 UTC
(In reply to Kyle M Hall from comment #1)
> 5) Create a hold policy rule that would stop this patron from placing
>    a hold on this item
> 6) Attempt to place an item-level hold on this item/patron, it should
>    fail, but instead it will succeed

How exactly are you managing to place the item-level hold?  Both request.pl and opac-request.pl do call GetBranchItemRule(), and any items that cannot be requested because of the policy are prevented from being selected:

http://screencast.com/t/pOF9zM4IChk

Of course, maybe I'm just missing another avenue for a patron or staff user to manage to place such requests.

To be clear, I'm not opposed to the general idea of the change -- for one thing, the ILS-DI and SIP hold placement code currently doesn't do enough checking, so moving checks into CanItemBeReserved() can certainly simplify things.  Also, it looks like it might be useful as a way of preventing patrons from placing forbidden hold requests by manipulating the parameters passed to opac-request.pl.

That said, unless there's more of a visible way for users to bypass the library/item-type hold policies, I think this patch can be held back a bit longer for:

- unit tests
- removing calls to GetBranchItemRule() that would be made redundant by this change
- (maybe) tweaking the return of CanItemBeReserved() so that higher-level code can distinguish between an item not being requestable due to a patron exceeding the limit and to it not being requestable because of library/item-type policy.

Per a discussion I had with Katrin, I'm setting this to in discussion so that she can review her testing notes.
Comment 12 Katrin Fischer 2013-08-21 05:46:45 UTC
I have done some more testing and it looks like this already works ok on master. I think I must have missed to test it thorougly without the patches applied. I am not sure if there is a condition where the exsiting code wouldn't work. 
Kyle - can you please take a look at this?
Comment 13 Barton Chittenden 2014-06-04 18:16:45 UTC
This seems to be an issue with one of our partners running 3.14.05.000.
Comment 14 Christopher Brannon 2014-06-04 19:28:43 UTC
I can confirm, our consortium is the partner Barton mentions above.  Specifically, rules are honored for item-level holds, but are not being honored for next available holds.

I've made a short video to demonstrate the problem.

http://screencast.com/t/HUUvnOGceCz

Christopher
Comment 15 Christopher Brannon 2014-06-04 20:20:23 UTC
Suggested Test Scenario:

1) Clear any existing hold policies
2) Set one circ rule to allow 1 or more holds on an item type (like Books).
3) Set a second rule to allow 0 holds on another item type (like computer file)
4) Create or modify a record to have more than one item.  One item should have the first item type (book), and another should have the second, non-holdable item type (computer file).
5) Check out the item that can be held (book) to a patron.
6) Try to place an item level hold on non-holdable item type (computer file).  See that you can't.
7) Place next available hold on the title instead.  Theoretically, the book type will be the only item that can fill the hold.
8) Check in the computer file item.  WHAT?  The item is trying to fill the hold!

Hope this helps.

Christopher
Comment 16 Barton Chittenden 2014-06-20 13:55:03 UTC
I believe that the ReservesControlBranch syspref is at the root of this problem. Consider:

Title "The Hobbit, first edition"
Item type "Valuable Book"

Patron "Candice (Candy) Fingers" comes from "Park Place" branch, which allows 10 holds on items of type "Valuable Book". Candice has 9 valuable books checked out.

The Park Place branch already has its copy of "The Hobbit, First Edition" checked out, but there is a copy at the Baltic Avenue branch. The Baltic Avenue branch only allows one hold on items of type "Valuable Book".

So:

If ReservesControlBranch is set to "Patron's Home Branch", Candy Fingers will be able to reserve "The Hobbit" from Baltic Avenue, which violates Baltic Avenue's hold limits for that item.

Now let's say that Branch "Boardwalk", allows 14 items of type "Valuable Book" to be checked out.

If ReservesControlBranch is set to "Item's Home Branch", Candy Fingers may then reserve the first editions of "The Hobbit", "The Fellowship of the Rings", "The TWo Towers" and "The Return of the King" (all of which have item type "Valuable Book"). She has 13 Valuable books on hold, and the circ staff at "Park Place" is overwhelmed cleaning sticky fingerprints off of valuable books, and complains that Candice violated their circ rules by reserving more valuable books than their circ rules allow.

There *must* be a ReservesControlBranch option that allows *both* sets of reserve rules to be checked.

Whether or not it even makes sense to restrict ReservesControlBranch to either "It's Home Branch" or "Patron's Home Branch" is a matter that can be debated by the community, but the option to observe both sets of rules *must* be available.
Comment 17 Kyle M Hall 2015-07-02 11:50:32 UTC
Created attachment 40761 [details] [review]
Bug 10314 - CanItemBeReserved does not respect the holds policies

CanItemBeReserved will return 1 as long as the circulation rules have a
non-zero number of holds allowed and the patron hasn't reached his or
her "Holds allowed" count. However, the subroutine completely ignores
the holds policies which would contravene reserving the item even if the
patron has not reached the maximum holds allowed count.

Test Plan:
1) Place an item on hold for a patron
2) Clear any existing hold policies
3) Set the appropriate circ rule so that the patron should be allowed
4) Attempt to place am item-level hold for this item/patron, it should succeed
5) Create a hold policy rule that would stop this patron from placing
   a hold on this item
6) Attempt to place an item-level hold on this item/patron, it should
   fail, but instead it will succeed
7) Apply this patch
8) Repeat step 6, this time you should be unable to place a specific
   hold on this item

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Testing notes on last patch in series.
Comment 18 Kyle M Hall 2015-07-02 11:50:38 UTC
Created attachment 40762 [details] [review]
Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - QA Bugfix

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Note: Unit tests to ensure the fix has the desired effect and
behaviour is not accidentally changed in the future should be
added.
Comment 19 Jonathan Druart 2015-08-18 14:19:04 UTC
Kyle, could you please provide tests?
Comment 20 Barton Chittenden 2015-08-18 14:30:20 UTC
I'll write the unit tests.
Comment 21 Kyle M Hall 2015-10-15 17:32:01 UTC
I believe that bug 13687 has resolved this issue.

*** This bug has been marked as a duplicate of bug 13687 ***