Fix various Traditional and holds-go-home best-hold sort orders

approx is a more correct determinant to give the behavior sites are used
to.  hprox can cause copies to be too eager to go home when there are
holds with that copy's circ lib as its request lib (if that's what you
want, then you do pick or create a sort-order with hprox near the top).

Address a problem in the copy_has_not_been_home CTE. This expression was
always meant to provide a TRUE or FALSE value as its lone result, but
would return NULL in cases where copies had no transit history.

Also use pickup_lib, not request_lib, as the determinant of
nearness-to-home.  request_lib was used with the thinking that an item's
"owning" patrons should have their wishes favored at holds-go-home time,
even if where they wanted to send the copy was not actually home, but
that's neither necessarily desired nor very intuitive.

Signed-off-by: Lebbeous Fogle-Weekley <lebbeous@esilibrary.com>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
index 9ba8272..fc43e1d 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
@@ -3007,6 +3007,10 @@ sub find_nearest_permitted_hold {
 
     my $fifo = $U->ou_ancestor_setting_value($user->ws_ou, 'circ.holds_fifo');
 
+    # the nearest_hold API call now needs this
+    $copy->call_number($editor->retrieve_asset_call_number($copy->call_number))
+        unless ref $copy->call_number;
+
        # search for what should be the best holds for this copy to fulfill
        my $best_holds = $U->storagereq(
         "open-ils.storage.action.hold_request.nearest_hold.atomic", 

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm
index c0c953b..d7bc4b9 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm
@@ -17,10 +17,12 @@ use OpenILS::Application::Circ::CircCommon;
 use OpenILS::Application::AppUtils;
 my $U = "OpenILS::Application::AppUtils";
 
-# used in build_hold_sort_clause()
+# Used in build_hold_sort_clause().  See the hash %order_by_sprintf_args in
+# that sub to confirm what gets used to replace the formatters, and see
+# nearest_hold() for the main body of the SQL query that these go into.
 my %HOLD_SORT_ORDER_BY = (
     pprox => 'p.prox',
-    hprox => 'actor.org_unit_proximity(%d, h.request_lib)',  # $cp->circ_lib
+    hprox => 'actor.org_unit_proximity(%d, h.pickup_lib)',  # $cp->call_number->owning_lib
     aprox => 'COALESCE(hm.proximity, p.prox)',
     approx => 'action.hold_copy_calculated_proximity(h.id, %d, %d)', # $cp,$here
     priority => 'pgt.hold_priority',
@@ -30,17 +32,17 @@ my %HOLD_SORT_ORDER_BY = (
     htime => q!
         CASE WHEN
             copy_has_not_been_home.result
-        THEN actor.org_unit_proximity(%d, acn.owning_lib)
+        THEN actor.org_unit_proximity(%d, h.pickup_lib)
         ELSE 999
         END
-    !,
+    !,  # $cp->call_number->owning_lib
     shtime => q!
         CASE WHEN
             copy_has_not_been_home_even_to_idle.result
-        THEN actor.org_unit_proximity(%d, acn.owning_lib)
+        THEN actor.org_unit_proximity(%d, h.pickup_lib)
         ELSE 999
         END
-    !,
+    !,  # $cp->call_number->owning_lib
 );
 
 
@@ -349,10 +351,10 @@ sub build_hold_sort_clause {
     my ($columns, $cp, $here) = @_;
 
     my %order_by_sprintf_args = (
-        hprox => [$cp->circ_lib],
+        hprox => [$cp->call_number->owning_lib],
         approx => [$cp->id, $here],
-        htime => [$cp->circ_lib],
-        shtime => [$cp->circ_lib]
+        htime => [$cp->call_number->owning_lib],
+        shtime => [$cp->call_number->owning_lib]
     );
 
     my @clauses;
@@ -402,22 +404,21 @@ sub build_hold_sort_clause {
             circ.xact_start >= NOW() - go_home_interval.value
     ) IS NULL AND (
         -- part 2
-        SELECT atc.dest <> %d FROM action.transit_copy atc
-        JOIN go_home_interval ON (true)
-        WHERE
+        SELECT COALESCE(atc.dest <> %d, TRUE)
+        FROM go_home_interval
+        LEFT JOIN action.transit_copy atc ON (
             atc.id = (
                 SELECT MAX(id) FROM action.transit_copy atc_inner
                 WHERE
                     atc_inner.target_copy = %d AND
                     atc_inner.source_send_time < NOW() - go_home_interval.value
             )
+        )
     ) AS result
 ) !, $cp->id, $cp->circ_lib, $cp->circ_lib, $cp->id);
 
         $joins .= q!
             JOIN copy_has_not_been_home ON (true)
-            JOIN asset.copy acp ON (hm.target_copy = acp.id)
-            JOIN asset.call_number acn ON (acn.id = acp.call_number)
         !;
     }
 
@@ -469,7 +470,8 @@ sub nearest_hold {
        my $self = shift;
        my $client = shift;
        my $here = shift;   # just the ID
-       my $cp = shift;     # now an object, formerly just the ID
+       my $cp = shift;     # now an object with call_number fleshed,
+                        # formerly just copy ID
        my $limit = int(shift()) || 10;
        my $age = shift() || '0 seconds';
        my $fifo = shift();

diff --git a/Open-ILS/src/sql/Pg/950.data.seed-values.sql b/Open-ILS/src/sql/Pg/950.data.seed-values.sql
index d693e6c..a8ddf6a 100644 (file)
--- a/Open-ILS/src/sql/Pg/950.data.seed-values.sql
+++ b/Open-ILS/src/sql/Pg/950.data.seed-values.sql
@@ -12489,15 +12489,15 @@ INSERT INTO config.org_unit_setting_type (
 
 INSERT INTO config.best_hold_order (
     name,
-    pprox, aprox, priority, cut, depth, rtime, htime, hprox
+    approx, pprox, aprox, priority, cut, depth, rtime
 ) VALUES (
     'Traditional',
-    1, 2, 3, 4, 5, 6, 7, 8
+    1, 2, 3, 4, 5, 6, 7
 );
 
 INSERT INTO config.best_hold_order (
     name,
-    hprox, pprox, aprox, priority, cut, depth, rtime, htime
+    hprox, approx, pprox, aprox, priority, cut, depth, rtime
 ) VALUES (
     'Traditional with Holds-always-go-home',
     1, 2, 3, 4, 5, 6, 7, 8
@@ -12505,7 +12505,7 @@ INSERT INTO config.best_hold_order (
 
 INSERT INTO config.best_hold_order (
     name,
-    htime, hprox, pprox, aprox, priority, cut, depth, rtime
+    htime, approx, pprox, aprox, priority, cut, depth, rtime
 ) VALUES (
     'Traditional with Holds-go-home',
     1, 2, 3, 4, 5, 6, 7, 8

diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql
new file mode 100644 (file)
index 0000000..ddd525d
--- /dev/null
+++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql
@@ -0,0 +1,67 @@
+BEGIN;
+
+-- INSERT INTO config.upgrade_log (version) VALUES ('XXXX');
+
+UPDATE config.best_hold_order
+SET
+    approx = 1,
+    pprox = 2,
+    aprox = 3,
+    priority = 4,
+    cut = 5,
+    depth = 6,
+    rtime = 7,
+    hprox = NULL,
+    htime = NULL
+WHERE name = 'Traditional' AND
+    pprox = 1 AND
+    aprox = 2 AND
+    priority = 3 AND
+    cut = 4 AND
+    depth = 5 AND
+    rtime = 6 ;
+
+UPDATE config.best_hold_order
+SET
+    hprox = 1,
+    approx = 2,
+    pprox = 3,
+    aprox = 4,
+    priority = 5,
+    cut = 6,
+    depth = 7,
+    rtime = 8,
+    htime = NULL
+WHERE name = 'Traditional with Holds-always-go-home' AND
+    hprox = 1 AND
+    pprox = 2 AND
+    aprox = 3 AND
+    priority = 4 AND
+    cut = 5 AND
+    depth = 6 AND
+    rtime = 7 AND
+    htime = 8;
+
+UPDATE config.best_hold_order
+SET
+    htime = 1,
+    approx = 2,
+    pprox = 3,
+    aprox = 4,
+    priority = 5,
+    cut = 6,
+    depth = 7,
+    rtime = 8,
+    hprox = NULL
+WHERE name = 'Traditional with Holds-go-home' AND
+    htime = 1 AND
+    hprox = 2 AND
+    pprox = 3 AND
+    aprox = 4 AND
+    priority = 5 AND
+    cut = 6 AND
+    depth = 7 AND
+    rtime = 8 ;
+
+
+COMMIT;