http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18345 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18345&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18345|0 |1 is obsolete| | --- Comment #3 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 18485 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18485&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18346|0 |1 is obsolete| | --- Comment #4 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 18486 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18486&action=edit Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - Tidy up CanItemBeReserved Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |chris@bigballofwax.co.nz --- Comment #5 from Chris Cormack <chris@bigballofwax.co.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20231 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20231&action=edit Bug 10314 - CanItemBeReserved does not respect the holds policies - Followup - QA Bugfix -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18485|0 |1 is obsolete| | Attachment #18486|0 |1 is obsolete| | Attachment #20231|0 |1 is obsolete| | --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20488 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20488&action=edit [PASSED QA] 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20490 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20490&action=edit [PASSED QA] 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |barton@bywatersolutions.com --- Comment #13 from Barton Chittenden <barton@bywatersolutions.com> --- This seems to be an issue with one of our partners running 3.14.05.000. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 --- Comment #15 from Christopher Brannon <cbrannon@cdalibrary.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Heather Braum <hbraum@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hbraum@nekls.org -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20488|0 |1 is obsolete| | Attachment #20489|0 |1 is obsolete| | Attachment #20490|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 40761 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40761&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Nicole C. Engard <nengard@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nengard@gmail.com Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10314 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |barton@bywatersolutions.com --- Comment #20 from Barton Chittenden <barton@bywatersolutions.com> --- I'll write the unit tests. -- You are receiving this mail because: You are watching all bug changes.