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.
Created attachment 19862 [details] [review] 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.
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
Created attachment 19863 [details] [review] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items - Add unit test from least_cost_branch
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()
Created attachment 20010 [details] [review] Bug 10628 - AutomaticItemReturn prevents holds queue from filling local holds with local items - Switch grep to any
# 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.
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!
Created attachment 20036 [details] [review] 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.
Created attachment 20052 [details] [review] [SIGNED-OFF] 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>
I'll make a mental note - MySQL is *NOT* Oracle.
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?
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.
I found it... my items were checked out...
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.
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.
This patch has been pushed to 3.12.x, will be in 3.12.5. Thanks Kyle!
Pushed to 3.10.x, will be in 3.10.11