From 210c686f7bfc2fd4dc11bdd5fcd582eb9f8f4500 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Thu, 24 Feb 2022 08:01:53 -0300 Subject: [PATCH] Bug 29346: Refactor loop code into a subroutine The CreateQueue() method deletes the holds queue data, fetches some configuration (branches to use, transport cost matrix) and then loops through a list of biblionumbers, generating the tmp_holdsqueue and hold_fill_targets rows for the specified biblio. This patch simply moves that last bit that is run inside the biblios loop into a separate sub. The update_queue_for_biblio sub is designed so it does the exact same thing it did inside the loop, but also gets added the capability of querying those parameters if not passed, and it also gets a 'delete' parameter so it deletes the biblio-specific holds queue rows before starting to work. This way, it can be reused to write a background job for real-time holds queue update :-D To test: 1. Run: $ kshell k$ prove t/db_dependent/HoldsQueue.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! Behavior is kept! 4. Sign off :-D Sponsored-by: Montgomery County Public Libraries Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Signed-off-by: Fridolin Somers --- C4/HoldsQueue.pm | 115 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 97 insertions(+), 18 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index b7cf0c7027..7073760881 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -44,6 +44,8 @@ BEGIN { TransportCostMatrix UpdateTransportCostMatrix GetPendingHoldRequestsForBib + load_branches_to_pull_from + update_queue_for_biblio ); } @@ -204,27 +206,19 @@ sub CreateQueue { my $bibs_with_pending_requests = GetBibsWithPendingHoldRequests(); foreach my $biblionumber (@$bibs_with_pending_requests) { + $total_bibs++; - my $hold_requests = GetPendingHoldRequestsForBib($biblionumber); - my $available_items = GetItemsAvailableToFillHoldRequestsForBib($biblionumber, $branches_to_use); - $total_requests += scalar(@$hold_requests); - $total_available_items += scalar(@$available_items); - my $item_map = MapItemsToHoldRequests($hold_requests, $available_items, $branches_to_use, $transport_cost_matrix); - $item_map or next; - my $item_map_size = scalar(keys %$item_map) - or next; + my $result = update_queue_for_biblio( + { biblio_id => $biblionumber, + branches_to_use => $branches_to_use, + transport_cost_matrix => $transport_cost_matrix, + } + ); - $num_items_mapped += $item_map_size; - CreatePicklistFromItemMap($item_map); - AddToHoldTargetMap($item_map); - if (($item_map_size < scalar(@$hold_requests )) and - ($item_map_size < scalar(@$available_items))) { - # DOUBLE CHECK, but this is probably OK - unfilled item-level requests - # FIXME - #warn "unfilled requests for $biblionumber"; - #warn Dumper($hold_requests), Dumper($available_items), Dumper($item_map); - } + $total_requests += $result->{requests}; + $total_available_items += $result->{available_items}; + $num_items_mapped += $result->{mapped_items}; } } @@ -855,5 +849,90 @@ sub least_cost_branch { # return $branch[0] if @branch == 1; } +=head3 update_queue_for_biblio + + my $result = update_queue_for_biblio( + { + biblio_id => $biblio_id, + [ branches_to_use => $branches_to_use, + transport_cost_matrix => $transport_cost_matrix, + delete => $delete, ] + } + ); + +Given a I, this method calculates and sets the holds queue entries +for the biblio's holds, and the hold fill targets (items). + +=head4 Return value + +It return a hashref containing: + +=over + +=item I: the pending holds count for the biblio. + +=item I the count of items that are available to fill holds for the biblio. + +=item I the total items that got mapped. + +=back + +=head4 Optional parameters + +=over + +=item I a list of branchcodes to be used to restrict which items can be used. + +=item I is the output of C. + +=item I tells the method to delete prior entries on the related tables for the biblio_id. + +=back + +Note: All the optional parameters will be calculated in the method if omitted. They +are allowed to be passed to avoid calculating them many times inside loops. + +=cut + +sub update_queue_for_biblio { + my ($args) = @_; + + my $biblio_id = $args->{biblio_id}; + + my $branches_to_use = $args->{branches_to_use} // load_branches_to_pull_from( C4::Context->preference('UseTransportCostMatrix') ); + my $transport_cost_matrix; + + if ( !exists $args->{transport_cost_matrix} + && C4::Context->preference('UseTransportCostMatrix') ) { + $transport_cost_matrix = TransportCostMatrix(); + } else { + $transport_cost_matrix = $args->{transport_cost_matrix}; + } + + if ( $args->{delete} ) { + my $dbh = C4::Context->dbh; + + $dbh->do("DELETE FROM tmp_holdsqueue WHERE biblionumber=$biblio_id"); + $dbh->do("DELETE FROM hold_fill_targets WHERE biblionumber=$biblio_id"); + } + + my $hold_requests = GetPendingHoldRequestsForBib($biblio_id); + my $available_items = GetItemsAvailableToFillHoldRequestsForBib( $biblio_id, $branches_to_use ); + + my $result = { + requests => scalar( @{$hold_requests} ), + available_items => scalar( @{$available_items} ), + }; + + my $item_map = MapItemsToHoldRequests( $hold_requests, $available_items, $branches_to_use, $transport_cost_matrix ); + $result->{mapped_items} = scalar( keys %{$item_map} ); + + if ($item_map) { + CreatePicklistFromItemMap($item_map); + AddToHoldTargetMap($item_map); + } + + return $result; +} 1; -- 2.39.5