Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items
Summary: AutomaticItemReturn prevents holds queue from filling local holds with local ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-23 11:58 UTC by Kyle M Hall
Modified: 2014-12-07 20:03 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items (2.38 KB, patch)
2013-07-23 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items - Add unit test from least_cost_branch (1.27 KB, patch)
2013-07-23 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items - Switch grep to any (1.06 KB, patch)
2013-07-31 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items (4.55 KB, patch)
2013-08-01 12:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items (4.59 KB, patch)
2013-08-01 23:53 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items (4.76 KB, patch)
2013-08-20 06:58 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 2013-07-23 11:58:13 UTC
For some reason, C4::HoldsQueue::MapItemsToHoldRequests uses the system preference AutomaticItemReturn to decide if an attempt to fill local holds with local items. No explanation of this behavior is provided.
Comment 1 Kyle M Hall 2013-07-23 12:28:13 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-23 12:35:44 UTC
I could not come up with a reliable test plan for this bug. However, inspection of the code should be extremely simple. The patch actually contains two changes, either of which solves the problem, but I see no reason not to include both.

The first is is the removal of the use of AutomaticItemReturn to control if we should try filling local holds with local items.

The second is the rather strange situation where the least_cost_branch subroutine completely ignores the fact that the pickup branch might be in the list of branches that can be pulled from.

I suggest both be implemented, the former for the fact that I cannot find any reason for this behavior, and the latter ( though obviated by the former ) because in the future someone may again modify the holds queue system and not realize that some code in MapItemsToHoldRequests would make this addition to least_cost_branch unnecessary. It seems to me that a unit test for least_cost_branch should fail immediately in it's current state. However, there is, for some reason, no unit test for least_cost_branch itself in HoldsQueue.t
Comment 3 Kyle M Hall 2013-07-23 12:47:54 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2013-07-25 02:49:50 UTC
my ($self) = grep( /$to/, @$from );
looks a bit inefficient. Would you consider replacing with
my $self;
foreach (@$from) {
    if ($_ eq $to) {
        $self = $to;
        last;
    }
}

or maybe even better
List::MoreUtils::any()
Comment 5 Kyle M Hall 2013-07-31 15:19:32 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2013-08-01 00:28:59 UTC
#   Failed test 'take from holdingbranch AutomaticItemReturn on (use cost matrix) holding branch'
#   at t/db_dependent/HoldsQueue.t line 196.
#          got: 'CPL'
#     expected: 'IPT'
# Wrong pick-up/hold for first target (pick_branch, hold_branch, reserves, hold_fill_targets, tmp_holdsqueue): $VAR1 = 'CPL';
# $VAR2 = 'IPT';
# $VAR3 = [
#           {
#             'priority' => '1',
#             'reservenotes' => undef,
#             'reservedate' => '2013-08-00',
#             'found' => undef,
#             'expirationdate' => undef,
#             'suspend_until' => undef,
#             'reminderdate' => undef,
#             'biblionumber' => '92704',
#             'timestamp' => '2013-08-01 12:27:34',
#             'notificationdate' => undef,
#             'borrowernumber' => '1000022266',
#             'branchcode' => 'CPL',
#             'itemnumber' => undef,
#             'constrainttype' => '',
#             'lowestPriority' => '0',
#             'suspend' => '0',
#             'waitingdate' => undef,
#             'cancellationdate' => undef,
#             'reserve_id' => '4361'
#           }
#         ];
# $VAR4 = [
#           {
#             'itemnumber' => '68955',
#             'biblionumber' => '92704',
#             'source_branchcode' => 'CPL',
#             'borrowernumber' => '1000022266',
#             'item_level_request' => '0'
#           }
#         ];
# $VAR5 = [
#           {
#             'firstname' => 'my firstname',
#             'pickbranch' => 'CPL',
#             'itemnumber' => '68955',
#             'itemcallnumber' => undef,
#             'reservedate' => '2013-08-00',
#             'barcode' => '531703236591',
#             'phone' => undef,
#             'item_level_request' => '0',
#             'surname' => 'my surname',
#             'biblionumber' => '92704',
#             'holdingbranch' => 'CPL',
#             'cardnumber' => 'CARDNUMBER42',
#             'notes' => undef,
#             'title' => 'Test Holds Queue XXX',
#             'borrowernumber' => '1000022266'
#           }
#         ];
Illegal date specified (year = 2013, month = 8, day = 00) at C4/HoldsQueue.pm line 138
# Looks like you failed 1 test of 19.
Comment 7 Kyle M Hall 2013-08-01 12:26:46 UTC
At least the date error doesn't appear to be related to my patches. It appears that mysql doens't like subtracting from dates the day it's done on line 103 for the unit test. This will fail on the first of every month!
Comment 8 Kyle M Hall 2013-08-01 12:37:26 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2013-08-01 23:53:30 UTC Comment hidden (obsolete)
Comment 10 Srdjan Jankovic 2013-08-01 23:56:49 UTC
I'll make a mental note - MySQL is *NOT* Oracle.
Comment 11 Katrin Fischer 2013-08-20 06:42:28 UTC
Hi Kyle,

the code does look alright, but I tried testing it and it didn't give me the results I expected.

I turned on the cost matrix and entered and made it look something like this:

  A B
A _ 0
B 0 _

Now I am in A, putting a hold on a record with items from A and B, pickup location is B. I would expect the routine to pick up the available item from B, but it doesn't. I have reran build_holds_queue multiple times.

Any hints?
Comment 12 Katrin Fischer 2013-08-20 06:52:37 UTC
I made sure that there are no hold policies set in circulation rules, there are only items from A and B and they are all of the same itemtype.
Comment 13 Katrin Fischer 2013-08-20 06:57:28 UTC
I found it... my items were checked out...
Comment 14 Katrin Fischer 2013-08-20 06:58:16 UTC
Created attachment 20491 [details] [review]
[PASSED QA] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items

For some reason, C4::HoldsQueue::MapItemsToHoldRequests uses the system
preference AutomaticItemReturn to decide if an attempt to fill local
holds with local items. No explanation of this behavior is provided.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and adds unit tests.
Tested with some examples and those worked correctly.
Comment 15 Galen Charlton 2013-08-20 15:38:50 UTC
Pushed to master.  Thanks, Kyle!

By the way, Srdjan, since you had written the code that added the dependence on AutomaticItemReturn to MapItemsToHoldRequests(), I'm taking your participation in the bug discussion as tacit agreement that there isn't a known use case for that.
Comment 16 Tomás Cohen Arazi 2013-09-13 16:46:04 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Kyle!
Comment 17 Bernardo Gonzalez Kriegel 2013-09-15 01:46:34 UTC
Pushed to 3.10.x, will be in 3.10.11