From 0558db79825caa79a350ab77f54fd4127372d2fe Mon Sep 17 00:00:00 2001 From: Bill Erickson Date: Thu, 9 Jun 2016 17:42:29 -0400 Subject: [PATCH] hold targeter reify experiment Signed-off-by: Bill Erickson --- .../src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm | 170 ++++++++++++--------- .../sql/Pg/upgrade/XXXX.schema.hold_targeter.sql | 2 +- .../support-scripts/test-scripts/hold_targeter.pl | 3 +- 3 files changed, 104 insertions(+), 71 deletions(-) diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm index 497d888d12..4a3ea9cddb 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm @@ -47,7 +47,8 @@ sub init { # if it's closed both now and at the next re-target date. my $next_check_time = - DateTime->now->add(seconds => $self->{retarget_interval}); + DateTime->now->add(seconds => $self->{retarget_interval}) + ->strftime('%F %T%z'); $closed_orgs_query = { '-and' => [ @@ -93,11 +94,14 @@ sub target_hold { sub target_all { my $self = shift; my @responses; - push(@responses, $self->target_hold($_)) for $self->collect_hold_ids; + for my $hold_id ($self->find_holds_to_target) { + my $resp = $self->target_hold($hold_id); + push(@responses, $resp); + } return \@responses; } -sub collect_hold_ids { +sub find_holds_to_target { my $self = shift; my $date = DateTime->now->subtract(seconds => $self->{retarget_interval}); @@ -202,20 +206,32 @@ sub result { my $self = shift; return { - hold_id => $self->hold_id, + hold => $self->hold_id, error => $self->error, success => $self->success, message => $self->message, - targeted_copy => $self->{targeted_copy}, - found_copy => $self->{found_copy} + target => $self->hold ? $self->hold->current_copy : undef, + old_target => $self->{previous_copy_id}, + found_copy => $self->{found_copy}, + eligible_copies => $self->{eligible_copy_count} }; } # List of potential copies in the form of slim hashes. -sub copy_hashes { - my ($self, $copy_hashes) = @_; - $self->{copy_hashes} = $copy_hashes if $copy_hashes; - return $self->{copy_hashes}; +# This is a working list of copies that evolves as copies +# are filtered for various reasons or deemed non-targetable. +sub copies { + my ($self, $copies) = @_; + $self->{copies} = $copies if $copies; + return $self->{copies}; +} + +# Final set of targetable ("good") copies that may be eligible for +# recall processing. +sub recall_copies { + my ($self, $recall_copies) = @_; + $self->{recall_copies} = $recall_copies if $recall_copies; + return $self->{recall_copies}; } # Maps copy ID's to their hold proximity @@ -293,11 +309,14 @@ sub get_hold_copies { my $org_depth = $hold->selection_depth || 0; my $query = { - select => {acp => ['id', 'status', 'circ_lib']}, + select => { + acp => ['id', 'status', 'circ_lib'], + ahr => ['current_copy'] + }, from => { acp => { - # Exclude copies that are targeted by other active holds. - # Include such copies that are targeted by the current hold. + # Tag copies that are in use by other holds so we don't + # try to target them for our hold. ahr => { type => 'left', fkey => 'id', # acp.id @@ -327,8 +346,7 @@ sub get_hold_copies { where => {id => $org_unit} } } - }, - '+ahr' => {id => undef} + } } }; @@ -428,43 +446,21 @@ sub get_hold_copies { }; } - $self->copy_hashes($e->json_query($query)); - return $self->inspect_potential_copies; -} + my $copies = $e->json_query($query); + $self->{eligible_copy_count} = scalar(@$copies); -# Returns true if we have copies to process, False if there are none. -sub inspect_potential_copies { - my $self = shift; - my @copy_hashes = @{$self->copy_hashes}; + $logger->info("targeter: Hold ".$self->hold_id." has ". + $self->{eligible_copy_count}." potential copies"); - my $e = $self->editor; - my $hold = $self->hold; - my $hold_id = $self->hold_id; - - $logger->info("targeter: Hold $hold_id has ". - scalar(@copy_hashes)." potential copies"); - - # Let the caller know we found the copy they were interested in. + # Let the caller know we encountered the copy they were interested in. $self->{found_copy} = 1 if $self->{find_copy} - && grep {$_->{id} eq $self->{find_copy}} @copy_hashes; + && grep {$_->{id} eq $self->{find_copy}} @$copies; - # We have copies. Nothing left to do here. - return 1 if @copy_hashes; + $self->copies($copies); - $hold->prev_check_time('now'); - $hold->clear_current_copy; - - if (!$e->update_action_hold_request($hold)) { - my $evt = $e->die_event; - return $self->exit_targeter( - "Error updating hold request: ".$evt->{textcode}, 1); - } - - $e->commit; - return $self->exit_targeter("No copies available for targeting"); + return 1; } - # Delete and rebuild copy maps sub update_copy_maps { my $self = shift; @@ -473,7 +469,7 @@ sub update_copy_maps { $e->json_query({from => [ 'action.hold_request_regen_copy_maps', $self->hold_id, - map {$_->{id}} @{$self->copy_hashes} # @array / variadic + '{' . join(',', map {$_->{id}} @{$self->copies}) . '}' ]}); return 1; @@ -498,7 +494,7 @@ sub compile_weighted_proximity_map { map {$_->{target_copy} => $_->{proximity}} @$hold_copy_maps; my %prox_map; - for my $copy_hash (@{$self->copy_hashes}) { + for my $copy_hash (@{$self->copies}) { my $prox = $copy_prox_map{$copy_hash->{id}}; $prox_map{$prox} ||= []; @@ -518,7 +514,7 @@ sub filter_closed_date_copies { my $self = shift; my @filtered_copies; - for my $copy_hash (@{$self->copy_hashes}) { + for my $copy_hash (@{$self->copies}) { my $clib = $copy_hash->{circ_lib}; if ($self->parent->{closed_orgs}->{$clib}) { @@ -543,7 +539,7 @@ sub filter_closed_date_copies { } # Update our in-progress list of copies to reflect the filtered set. - $self->copy_hashes(\@filtered_copies); + $self->copies(\@filtered_copies); return 1; } @@ -553,19 +549,30 @@ sub filter_closed_date_copies { # Returns true if filtering completes without error, false otherwise. sub filter_copies_by_status { my $self = shift; - $self->copy_hashes([ - grep - {$_->{status} == 0 || $_->{status} == 7} - @{$self->copy_hashes} + $self->copies([ + grep {$_->{status} == 0 || $_->{status} == 7} @{$self->copies} ]); return 1; } +# Remove copies that are currently targeted by other holds. +# Returns true if filtering completes without error, false otherwise. +sub filter_copies_in_use { + my $self = shift; + + # A copy with a 'current_copy' value means it's in use by another hold. + $self->copies([ + grep {!$_->{current_copy}} @{$self->copies} + ]); + + return 1; +} + # Returns true if inspection completed without error, false otherwise. sub inspect_previous_target { my $self = shift; my $hold = $self->hold; - my @copies = @{$self->copy_hashes}; + my @copies = @{$self->copies}; # no previous target return 1 unless my $prev_id = $hold->current_copy; @@ -578,11 +585,12 @@ sub inspect_previous_target { # previous target is no longer valid. return 1 unless $prev; + # Previous copy is targetable. Keep it around for later. $self->{valid_previous_copy} = $prev; # Remove the previous copy from the working set of potential copies. # It will be revisited later if needed. - $self->copy_hashes([grep {$_->{id} ne $prev_id} @copies]); + $self->copies([grep {$_->{id} ne $prev_id} @copies]); return 1; } @@ -595,9 +603,9 @@ sub handle_no_copies { my $force = shift; if (!$force) { - # Force just says don't bother checking the copies, because - # other code already has. - return 1 if @{$self->copy_hashes} || $self->{valid_previous_copy}; + # Force just says don't bother checking the copies, + # because other code already has. + return 1 if @{$self->copies} || $self->{valid_previous_copy}; } my $hold = $self->hold; @@ -619,7 +627,7 @@ sub handle_no_copies { # F or R hold is encountered. Returns undef otherwise. sub attempt_force_recall_target { my $self = shift; - return $self->copy_hashes->[0] if + return $self->copies->[0] if $self->hold->hold_type eq 'R' || $self->hold->hold_type eq 'F'; return undef; } @@ -630,7 +638,7 @@ sub attempt_local_copy_target { my $self = shift; my $pu_lib = $self->hold->pickup_lib; - my @copies = @{$self->copy_hashes}; + my @copies = @{$self->copies}; my @locals = grep {$_->{circ_lib} eq $pu_lib} @copies; return undef unless @locals; @@ -639,13 +647,19 @@ sub attempt_local_copy_target { return $copy if $self->copy_is_permitted($copy); } + $logger->info( + "targeter: no local targetable copies found for hold ".$self->hold_id); + return undef; } sub attempt_remote_copy_target { my $self = shift; - return undef unless @{$self->copy_hashes}; + $logger->info("targeter: attempting to target a ". + "remote copy for hold ".$self->hold_id); + + return undef unless @{$self->copies}; return undef unless $self->trim_copies_by_target_loop; @@ -662,9 +676,9 @@ sub trim_copies_by_target_loop { 'circ.holds.max_org_unit_target_loops' ); - return unless defined $max_loops; + return 1 unless defined $max_loops; - my @copies = @{$self->copy_hashes}; + my @copies = @{$self->copies}; my $e = $self->editor; my %circ_lib_map = map {$_->{circ_lib} => 1} @copies; @@ -719,7 +733,7 @@ sub trim_copies_by_target_loop { if grep {$copy->{circ_lib} eq $_} @keep_libs; } - $self->copy_hashes(\@new_copies); + $self->copies(\@new_copies); return 1; } @@ -761,6 +775,9 @@ sub attempt_prev_copy_retarget { my $prev_copy = $self->{valid_previous_copy}; return undef unless $prev_copy; + $logger->info("targeter: attempting to re-target ". + "previously targeted copy for hold ".$self->hold_id); + if ($self->copy_is_permitted($prev_copy)) { $logger->debug("targeter: retargeting the ". "previously targeted copy [".$prev_copy->{id}."]" ); @@ -820,8 +837,8 @@ sub copy_is_permitted { # Copy is confirmed non-viable. # Remove it from our potentials list. - $self->copy_hashes([ - grep {$_->{id} ne $copy->{id}} @{$self->copy_hashes} + $self->copies([ + grep {$_->{id} ne $copy->{id}} @{$self->copies} ]); return 0; @@ -843,7 +860,6 @@ sub apply_copy_target { $e->commit; $self->{success} = 1; - $self->{targeted_copy} = $hold->current_copy; return $self->exit_targeter("Hold successfully targeted"); } @@ -880,6 +896,10 @@ sub log_unfulfilled_hold { return 1; } +sub process_recalls { + my $self = shift; +} + # Target a single hold request sub target { my ($self, $hold_id) = @_; @@ -905,11 +925,23 @@ sub target { return unless $self->handle_expired_hold; return unless $self->get_hold_copies; return unless $self->update_copy_maps; - return unless $self->inspect_previous_target; + + # Confirm that we have something to work on. + return unless $self->handle_no_copies; + return unless $self->filter_copies_by_status; + return unless $self->filter_copies_in_use; return unless $self->filter_closed_date_copies; + return unless $self->inspect_previous_target; + + # Confirm again we have something to work on. return unless $self->handle_no_copies; + # At this point, we have trimmed the working set of copies + # down to those that are currently targetable. Clone this + # list for later use by recalls processing if needed. + $self->recall_copies([@{$self->copies}]); + my $copy = $self->attempt_force_recall_target || $self->attempt_local_copy_target || $self->attempt_remote_copy_target || @@ -926,7 +958,7 @@ sub target { # No targetable copy was found. Fire the no-copy # handler to update the hold accordingly. - # TODO: process_recall() + return unless $self->process_recalls; $self->handle_no_copies(1); } diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.hold_targeter.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.hold_targeter.sql index 43983daa77..914ad40b78 100644 --- a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.hold_targeter.sql +++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.hold_targeter.sql @@ -4,7 +4,7 @@ BEGIN; CREATE OR REPLACE FUNCTION action.hold_request_regen_copy_maps( - hold_id INTEGER, copy_ids VARIADIC INTEGER[]) RETURNS VOID AS $$ + hold_id INTEGER, copy_ids INTEGER[]) RETURNS VOID AS $$ DELETE FROM action.hold_copy_map WHERE hold = $1; INSERT INTO action.hold_copy_map (hold, target_copy) SELECT $1, UNNEST($2); $$ LANGUAGE SQL; diff --git a/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl b/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl index 0191b8e2d9..75a7fca75c 100755 --- a/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl +++ b/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl @@ -20,5 +20,6 @@ my $targeter = OpenILS::Utils::HoldTargeter->new( $targeter->init; my $responses = $targeter->target_all; -#print Dumper($responses); +#my $responses = $targeter->target_hold(80); +print Dumper($responses); print "Processed " . scalar(@$responses) . " holds\n"; -- 2.11.0