From: Bill Erickson <berickxx@gmail.com>
Date: Wed, 8 Jun 2016 16:57:28 +0000 (-0400)
Subject: hold targeter reify experiment
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=a3ba5b1051951f137b83d3c7159c1312f052e36e;p=working%2FEvergreen.git

hold targeter reify experiment

Signed-off-by: Bill Erickson <berickxx@gmail.com>
---

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm
index 14d91e863c..7167e28f24 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm
@@ -7,14 +7,11 @@ use OpenSRF::Utils::Logger qw(:logger);
 use OpenILS::Application::AppUtils;
 use OpenILS::Utils::CStoreEditor qw/:funcs/;
 
-my $U = "OpenILS::Application::AppUtils";
-my $dt_parser = DateTime::Format::ISO8601->new;
-
+# WIP notes:
 # avoid 'duplicate key value violates unique constraint "copy_once_per_hold"'
-# cache org unit settings per run
-# reduce memory requirements?
-# speed 
-#  -- better up-front copy filtering when finding potential copies
+
+our $U = "OpenILS::Application::AppUtils";
+our $dt_parser = DateTime::Format::ISO8601->new;
 
 sub new {
     my ($class, %args) = @_;
@@ -26,15 +23,18 @@ sub new {
     return bless($self, $class);
 }
 
-# Pre-fetch necessary data.
 sub init {
     my $self = shift;
 
-    $self->{org_closed_dates} =
-        $self->{editor}->search_actor_org_unit_closed_date({
-            close_start => {'<=', 'now'},
-            close_end => {'>=', 'now'}
+    my $closed = $self->{editor}->search_actor_org_unit_closed_date({
+        close_start => {'<=', 'now'},
+        close_end => {'>=', 'now'}
     });
+
+    # TODO: check expire / closed_at_next logic
+
+    # Map of org id to 1. Any org in the map is closed.
+    $self->{closed_orgs} = {map {$_->org_unit => 1} @$closed};
 }
 
 # Org unit setting fetch+cache
@@ -52,22 +52,136 @@ sub get_ou_setting {
     return $c->{$org_id}->{$setting};
 }
 
+sub target_hold {
+    my ($self, $hold_id) = @_;
+    my $targeter = OpenILS::Utils::HoldTargeter::Single->new(parent => $self);
+    $targeter->target($hold_id);
+    return $targeter->result;
+}
+
+
+# -----------------------------------------------------------------------
+# Knows how to target a single hold.
+# -----------------------------------------------------------------------
+package OpenILS::Utils::HoldTargeter::Single;
+use strict;
+use warnings;
+use DateTime;
+use OpenSRF::AppSession;
+use OpenSRF::Utils::Logger qw(:logger);
+use OpenILS::Application::AppUtils;
+use OpenILS::Utils::CStoreEditor qw/:funcs/;
+
+sub new {
+    my ($class, %args) = @_;
+    my $self = {
+        %args,
+        editor => new_editor(),
+        error => 0,
+        success => 0
+    };
+    return bless($self, $class);
+}
+
+# Parent targeter object.
+sub parent {
+    my ($self, $parent) = @_;
+    $self->{parent} = $parent if $parent;
+    return $self->{parent};
+}
+
+sub hold_id {
+    my ($self, $hold_id) = @_;
+    $self->{hold_id} = $hold_id if $hold_id;
+    return $self->{hold_id};
+}
+
+sub hold {
+    my ($self, $hold) = @_;
+    $self->{hold} = $hold if $hold;
+    return $self->{hold};
+}
+
+# Debug message
+sub message {
+    my ($self, $message) = @_;
+    $self->{message} = $message if $message;
+    return $self->{message} || '';
+}
+
+# True if the hold was successfully targeted.
+sub success {
+    my ($self, $success) = @_;
+    $self->{success} = $success if defined $success;
+    return $self->{success};
+}
+
+# True if targeting exited early on an unrecoverable error.
+sub error {
+    my ($self, $error) = @_;
+    $self->{error} = $error if defined $error;
+    return $self->{error};
+}
+
+sub editor {
+    my $self = shift;
+    return $self->{editor};
+}
+
+sub result {
+    my $self = shift;
+    return {
+        hold_id => $self->hold_id,
+        error   => $self->error,
+        success => $self->success,
+        message => $self->message
+    };
+}
+
+# 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};
+}
+
+# Maps copy ID's to their hold proximity
+sub copy_prox_map {
+    my ($self, $copy_prox_map) = @_;
+    $self->{copy_prox_map} = $copy_prox_map if $copy_prox_map;
+    return $self->{copy_prox_map};
+}
+
 sub exit_targeter {
-    my ($self, $msg) = @_;
-    $self->{exit} = 1;
-    $self->{exit_msg} = $msg;
-    $self->{editor}->rollback; # no-op if commit already occurred.
-    my $hold_id = $self->{hold_id};
-    $logger->info("targeter: exiting hold targeter for $hold_id : $msg");
+    my ($self, $msg, $is_error) = @_;
+    $self->message($msg);
+
+    # Force a rollback when exiting.
+    # This is a no-op if a commit or rollback have already occurred.
+    $self->editor->rollback; 
+
+    my $log = "targeter: exiting hold targeter on ".$self->hold_id." : $msg";
+
+    if ($is_error) {
+        $self->error($is_error);
+        $logger->error($log);
+    } else {
+        $logger->info($log);
+    }
+
     return 0;
 }
 
+sub create_prox_list {
+
+}
+
 # Cancel expired holds and kick off the A/T no-target event.  Returns
 # true (i.e. keep going) if the hold is not expired.  Returns false if
 # the hold is canceled or a non-recoverable error occcurred.
 sub handle_expired_hold {
     my $self = shift;
-    my $hold = $self->{hold};
+    my $hold = $self->hold;
 
     return 1 unless $hold->expire_time;
 
@@ -80,12 +194,13 @@ sub handle_expired_hold {
     $hold->cancel_time('now');
     $hold->cancel_cause(1); # == un-targeted expiration
 
-    if (!$self->{editor}->update_action_hold_request($hold)) {
-        my $evt = $self->{editor}->die_event;
-        return $self->exit_targeter("Error canceling hold: ".$evt->{textcode});
+    if (!$self->editor->update_action_hold_request($hold)) {
+        my $evt = $self->editor->die_event;
+        return $self->exit_targeter(
+            "Error canceling hold: ".$evt->{textcode}, 1);
     }
 
-    $self->{editor}->commit;
+    $self->editor->commit;
 
     # Fire the A/T handler, but don't wait for a response.
     OpenSRF::AppSession->create('open-ils.trigger')->request(
@@ -102,15 +217,15 @@ sub handle_expired_hold {
 # Returns true on success, false on error.
 sub remove_copy_maps {
     my $self = shift;
-    my $e = $self->{editor};
+    my $e = $self->editor;
     my $prev_maps = 
-        $e->search_action_hold_copy_map({hold => $self->{hold_id}});
+        $e->search_action_hold_copy_map({hold => $self->hold_id});
 
     for my $map (@$prev_maps) {
         if (!$e->delete_action_hold_copy_map($map)) {
             my $evt = $e->die_event;
             return $self->exit_targeter(
-                "Error deleting copy maps: ".$evt->{textcode});
+                "Error deleting copy maps: ".$evt->{textcode}, 1);
         }
     }
 
@@ -119,8 +234,8 @@ sub remove_copy_maps {
 
 sub get_hold_copies {
     my $self = shift;
-    my $e = $self->{editor};
-    my $hold = $self->{hold};
+    my $e = $self->editor;
+    my $hold = $self->hold;
 
     my $hold_target = $hold->target;
     my $hold_type   = $hold->hold_type;
@@ -128,8 +243,23 @@ sub get_hold_copies {
     my $org_depth   = $hold->selection_depth || 0;
 
     my $query = {
-        select => {acp => ['id']},
-        from => {acp => {}},
+        select => {acp => ['id', 'status', 'circ_lib']},
+        from => {
+            acp => {
+                # Exclude copies that are targeted by other active holds. 
+                # Include such copies that are targeted by the current hold.
+                ahr => {
+                    type => 'left',
+                    fkey => 'id', # acp.id
+                    field => 'current_copy',
+                    filter => {
+                        fulfillment_time => undef,
+                        cancel_time => undef,
+                        id => {'!=' => $self->hold_id}
+                    }
+                }
+            }
+        },
         where => {
             '+acp' => {
                 deleted => 'f',
@@ -146,8 +276,9 @@ sub get_hold_copies {
                         from => 'aou',
                         where => {id => $org_unit}
                     }
-                },
-            }
+                }
+            },
+            '+ahr' => {id => undef}
         }
     };
 
@@ -156,17 +287,16 @@ sub get_hold_copies {
         # we're processing a Recall or Force hold, which bypass most
         # holdability checks.
 
-        $query->{from}->{acp} = {
-            acpl => {
-                field => 'id',
-                filter => {holdable => 't', deleted => 'f'},
-                fkey => 'location'
-            },
-            ccs => {
-                field => 'id',
-                filter => {holdable => 't'},
-                fkey => 'status'
-            }
+        $query->{from}->{acp}->{acpl} = {
+            field => 'id',
+            filter => {holdable => 't', deleted => 'f'},
+            fkey => 'location'
+        };
+
+        $query->{from}->{acp}->{ccs} = {
+            field => 'id',
+            filter => {holdable => 't'},
+            fkey => 'status'
         };
 
         $query->{where}->{'+acp'}->{circulate} = 't';
@@ -249,26 +379,28 @@ sub get_hold_copies {
         };
     }
 
-    my $res = $e->json_query($query);
-    return map {$_->{id}} @$res;
+    $self->copy_hashes($e->json_query($query));
+    return $self->inspect_potential_copies;
 }
 
+# Returns true if we have copies to process, False if there are none.
 sub inspect_potential_copies {
-    my ($self, @copy_ids) = @_;
+    my $self = shift;
+    my @copy_hashes = @{$self->copy_hashes};
 
-    my $e = $self->{editor};
-    my $hold = $self->{hold};
-    my $hold_id = $self->{hold_id};
+    my $e = $self->editor;
+    my $hold = $self->hold;
+    my $hold_id = $self->hold_id;
 
     $logger->info("targeter: Hold $hold_id has ".
-        scalar(@copy_ids)." potential copies");
+        scalar(@copy_hashes)." potential copies");
     
     # Let the caller know we found the copy they were interested in.
     $self->{found_copy} = 1 if $self->{find_copy} 
-        && grep {$_ eq $self->{find_copy}} @copy_ids;
+        && grep {$_->{id} eq $self->{find_copy}} @copy_hashes;
 
     # We have copies.  Nothing left to do here.
-    return 1 if @copy_ids;
+    return 1 if @copy_hashes;
 
     $hold->prev_check_time('now');
     $hold->clear_current_copy;
@@ -276,7 +408,7 @@ sub inspect_potential_copies {
     if (!$e->update_action_hold_request($hold)) {
         my $evt = $e->die_event;
         return $self->exit_targeter(
-            "Error updating hold request: ".$evt->{textcode});
+            "Error updating hold request: ".$evt->{textcode}, 1);
     }
 
     $e->commit;
@@ -285,54 +417,174 @@ sub inspect_potential_copies {
 
 
 sub build_copy_maps {
-    my ($self, @copy_ids) = @_;
-    my $e = $self->{editor};
+    my $self = shift;
+    my $e = $self->editor;
 
-    for my $copy_id (@copy_ids) {
+    for my $copy_hash (@{$self->copy_hashes}) {
         my $map = Fieldmapper::action::hold_copy_map->new;
 
-        $map->hold($self->{hold_id});
-        $map->target_copy($copy_id);
+        $map->hold($self->hold_id);
+        $map->target_copy($copy_hash->{id});
 
         if (!$e->create_action_hold_copy_map($map)) {
             my $evt = $e->die_event;
             return $self->exit_targeter(
-                "Error creating hold copy map: ".$evt->{textcode});
+                "Error creating hold copy map: ".$evt->{textcode}, 1);
         }
     }
 
-    # TODO: collect proximities
+    # Collect copy proximity info (generated via DB trigger) from the 
+    # newly create copy maps.
+    my $proximities = $e->json_query({
+        select => {ahcm => ['target_copy', 'proximity']},
+        from => 'ahcm',
+        where => {hold => $self->hold_id}
+    });
+
+    $self->copy_prox_map({
+        map {$_->{target_copy} => $_->{proximity}} @$proximities
+    });
 
     return 1;
 }
 
+sub build_copy_prox_list {
+}
+
+# Returns true if filtering completed without error, false otherwise.
+sub filter_closed_date_copies {
+    my $self = shift;
+
+    my @filtered_copies;
+    for my $copy_hash (@{$self->copy_hashes}) {
+        my $clib = $copy_hash->{circ_lib};
 
-# Targets a single hold request
-sub target_hold {
+        if ($self->parent->{closed_orgs}->{$clib}) {
+            # Org unit is currently closed.  See if it matters.
+
+            my $ous = $self->hold->pickup_lib eq $clib ?
+                'circ.holds.target_when_closed_if_at_pickup_lib' :
+                'circ.holds.target_when_closed';
+
+            unless ($self->parent->get_ou_setting($clib, $ous)) {
+                # Targeting not allowed at this circ lib when its closed
+
+                $logger->info("targeter: skipping copy ".
+                    $copy_hash->{id}."at closed org $clib");
+
+                next;
+            }
+
+        }
+
+        push(@filtered_copies, $copy_hash);
+    }
+
+    # Update our in-progress list of copies to reflect the filtered set.
+    $self->copy_hashes(\@filtered_copies);
+
+    return 1;
+}
+
+# Limit the set of potential copies to those that are 
+# in a targetable status.
+# 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}
+    ]);
+    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};
+
+    # no previous target
+    return 1 unless my $prev_id = $hold->current_copy;
+
+    $self->{had_previous_copy} = 1;
+
+    # TODO: is this step really necessary here??  Not if we always set
+    # or clear the value later.  Confirm.
+    # Clear the previous copy regardless of 
+    # whether we can use it again later.
+    $self->clear_current_copy;
+
+    if (!$self->editor->update_action_hold_request($hold)) {
+        my $evt = $self->editor->die_event;
+        return $self->exit_targeter(
+            "Error updating hold request: ".$evt->{textcode}, 1);
+    }
+
+    # See if the previous copy is in our list of valid copies.
+    my ($prev) = grep {$_->{id} eq $prev_id} @copies;
+
+    # previous target is no longer valid.
+    return 1 unless $prev;
+
+    $self->{valid_previous_copy} = $prev;
+
+    # Remove the previous copy from the working set of potential copies
+    # if there are other copies we can focus on.  If there are no other
+    # copies, treat the previous copy like any other.
+    $self->copy_hashes([grep {$_->{id} ne $prev_id} @copies]) 
+        if scalar(@copies) > 1;
+
+    return 1;
+}
+
+sub check_no_copies {
+    my $self = shift;
+
+    return 1 if @{$self->copy_hashes};
+    
+    my $hold = $self->hold;
+    $hold->clear_current_copy;
+    $hold->prev_check_time('now');
+
+    if (!$self->editor->update_action_hold_request($hold)) {
+        my $evt = $self->editor->die_event;
+        return $self->exit_targeter(
+            "Error updating hold request: ".$evt->{textcode}, 1);
+    }
+
+    $self->editor->commit;
+    return $self->exit_targeter("Hold has no targetable copies");
+}
+
+# Target a single hold request
+sub target {
     my ($self, $hold_id) = @_;
-    my $e = $self->{editor};
 
-    # reset hold-specific toggles
-    $self->{exit} = 0;
-    $self->{hold_id} = $hold_id;
-    $self->{found_copy} = 0;
+    my $e = $self->editor;
+    $self->hold_id($hold_id);
+
+    $logger->info("Processing hold $hold_id");
 
     $e->xact_begin;
 
-    my $hold = $self->{hold} = 
-        $e->retrieve_action_hold_request($hold_id)
-            or return $self->exit_targeter("No hold found");
+    my $hold = $e->retrieve_action_hold_request($hold_id)
+        or return $self->exit_targeter("No hold found", 1);
+
+    $self->hold($hold);
 
     return $self->exit_targeter("Hold is not eligible for targeting")
         if $hold->capture_time || $hold->cancel_time;
 
     return unless $self->remove_copy_maps;
     return unless $self->handle_expired_hold;
-
-    my @copy_ids = $self->get_hold_copies;
-    return unless $self->inspect_potential_copies(@copy_ids);
-
-    return unless $self->build_copy_maps(@copy_ids);
+    return unless $self->get_hold_copies;
+    return unless $self->build_copy_maps;
+    return unless $self->inspect_previous_target;
+    return unless $self->filter_copies_by_status;
+    return unless $self->filter_closed_date_copies;
+    return unless $self->check_no_copies;
 
     $e->commit;
 }
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 c1b2acd45e..87367d92ea 100755
--- a/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl
+++ b/Open-ILS/src/support-scripts/test-scripts/hold_targeter.pl
@@ -8,6 +8,7 @@ use strict; use warnings;
 use OpenSRF::AppSession;
 use OpenILS::Utils::Fieldmapper;
 use OpenILS::Utils::HoldTargeter;
+use Data::Dumper;
 
 my $config = shift; # path to opensrf_core.xml
 osrf_connect($config); # connect to jabber
@@ -16,5 +17,7 @@ my $hold_id = shift;
 
 my $targeter = OpenILS::Utils::HoldTargeter->new;
 $targeter->init;
-$targeter->target_hold($hold_id);
+my $resp = $targeter->target_hold($hold_id);
+
+print Dumper($resp);