Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy
Summary: Holds queue is making transfer requests that contradict the branch holds policy
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
: 9906 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-05-14 19:53 UTC by Kyle M Hall
Modified: 2015-06-04 23:23 UTC (History)
4 users (show)

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


Attachments
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy (2.45 KB, patch)
2013-05-14 20:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy - Alternate solution (1.65 KB, patch)
2013-05-14 20:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed off] Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy (2.51 KB, patch)
2013-05-15 17:19 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy (2.55 KB, patch)
2013-06-18 08:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy - Unit Tests (5.54 KB, patch)
2013-09-16 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy - QA Followup (1.00 KB, patch)
2013-10-09 18:03 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-14 19:53:58 UTC
For some reason MapItemsToHoldRequests will, as a last ditch effort, grab what seems to be an arbitrary available item to fill a hold request, even if it will violate the circulation rules for holds.

In other words, even if an item matches a "Holds policy by item type" that says "From home library", the a request to transfer that item to another library will be added to the holds queue!
Comment 1 Kyle M Hall 2013-05-14 20:02:40 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-14 20:05:36 UTC
This patch is tested and working, but I wonder if we should simply delete lines 470-471 instead of using the code I wrote. I cannot understand the justification for what 470-471 do, and there are no comments about why it's there. 

I'll post an alterative that just deletes those lines, either one will resolve this bug.
Comment 3 Kyle M Hall 2013-05-14 20:09:38 UTC Comment hidden (obsolete)
Comment 4 Melia Meggs 2013-05-15 17:19:16 UTC Comment hidden (obsolete)
Comment 5 HB-NEKLS 2013-05-15 17:45:55 UTC
I, Heather Braum, from NEKLS sign off on this patch. I have tested it on our production server and it works.
Comment 6 Melia Meggs 2013-05-16 16:53:46 UTC
*** Bug 9906 has been marked as a duplicate of this bug. ***
Comment 7 Chris Cormack 2013-06-18 08:48:47 UTC
Created attachment 19097 [details] [review]
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy

For some reason MapItemsToHoldRequests will, as a last ditch effort,
grab what seems to be an arbitrary available item to fill a hold request,
even if it will violate the circulation rules for holds.

In other words, even if an item matches a "Holds policy by item type"
that says "From home library", a request to transfer that item to
another library will be added to the holds queue!

Test Plan:
1) Create a record with a an item at BranchA of item type BOOK
2) Set the holds policy such that itemtype BOOK for BranchA is set
   to "From home library"
3) Place a bib-level hold request for a patron with a pickup at BranchB
4) Run build_holds_queue.pl
5) You should now see a request for that item to be transfered to
   BranchB, even though the rules should not allow this.
6) Apply this patch
7) Run build_holds_queue.pl again
8) View the holds queue again, that request should no longer exist

Signed-off-by: Heather Braum <hbraum@nekls.org>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 8 Galen Charlton 2013-06-18 18:45:06 UTC
Would you mind adding test cases for this situation to t/db_dependent/HoldsQueue.t?  Since HoldsQueue.t is pretty well fleshed out already, it should be easier to add regression tests for this than might be the case with other modules.
Comment 9 Kyle M Hall 2013-09-16 15:31:40 UTC
Created attachment 21121 [details] [review]
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy - Unit Tests
Comment 10 Kyle M Hall 2013-10-09 18:03:59 UTC
Created attachment 21927 [details] [review]
Bug 10243 - Holds queue is making transfer requests that contradict the branch holds policy - QA Followup

Using the TransportCostMatrix can cause the same issue. Removing the
last ditch use of the first item causes the the subroutine to continue
with the traditional matching, which will respect the hold policies.
Comment 11 Galen Charlton 2013-10-30 03:23:41 UTC
Pushed to master.  Thanks, Kyle!
Comment 12 Tomás Cohen Arazi 2013-12-05 18:34:26 UTC
This patch has been pushed to 3.12.x, will be in 3.12.8.

Thanks Kyle!