LP#1511828 - There were 3 instances of using a lexical sort instead

  of a numeric sort.

Two of the instances affected hold targeting if proximity values were over 9.
Proximity values were being sorted lexically. For instance,
1,2,5,11,15,100,120 was sorted as 1,100,11,120,15,2,5, causing interesting
hold targeting results. Normally proximity doesn't go that high so it isn't
a problem, but we were using proximity adjustments to strictly order locations
so each location had a different proximity, driving the numbers above 9.

One instance wasn't causing any current issue but could cause problems in
the future if more best hold selection sort options are added to bring the
total number of sort options over 9.

Also included are some changes to reduce warnings in the logs
suggested by Bill Erickson.  The sorting problem was found by
Galen Charlton, thanks Galen.

Testing Notes - to trigger this issue you need to have adjusted
  proximities over 9.

1. Use proximity adjustment rules to add a +10 proximity adjustment to
  one item circ lib, Branch A and a +2 proximity adjustment to the
  item circ lib Branch B.

2. Find a title with a copy at Branch A and B. Place a hold with a
  pickup location of Branch B.

3. The copy at Branch A should get targeted since it has a proximity of
  12-14 (10+normal proximity) and Branch B has a proximity of 2
  (2+normal proximity).  12 gets sorted before 2 lexically.

4. After the fix the copy at Branch B should get targeted since 2
  is sorted before 12.

Signed-off-by: Josh Stompro <stomproj@larl.org>

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 3d592d6..652acc1 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
@@ -363,8 +363,8 @@ sub get_hold_sort_order {
     # Return only the keys of our hash, sorted by value,
     # keys for null values omitted.
     return [
-        grep { defined $row->{$_} } (
-            sort {$row->{$a} cmp $row->{$b}} keys %$row
+        sort { $row->{$a} <=> $row->{$b} } (
+          grep { defined $row->{$_} } keys %$row
         )
     ];
 }
@@ -1433,11 +1433,11 @@ sub new_hold_copy_targeter {
 
             $all_copies = [ grep { ''.$_->circ_lib ne $pu_lib && ( $_->status == 0 || $_->status == 7 ) } @good_copies ];
 
-            my $min_prox = [ sort keys %$prox_list ]->[0];
+            my $min_prox = [ sort {$a<=>$b} keys %$prox_list ]->[0];
             my $best;
             if  ($hold->hold_type eq 'R' || $hold->hold_type eq 'F') { # Recall/Force holds bypass hold rules.
                 $best = $good_copies[0] if(scalar @good_copies);
-            } else {
+            } elsif (defined $min_prox) {
                 $best = choose_nearest_copy($hold, { $min_prox => delete($$prox_list{$min_prox}) });
             }
 
@@ -1989,7 +1989,7 @@ sub choose_nearest_copy {
     my $hold = shift;
     my $prox_list = shift;
 
-    for my $p ( sort keys %$prox_list ) {
+    for my $p ( sort {$a<=>$b} keys %$prox_list ) {
         next unless (ref $$prox_list{$p});
 
         my @capturable = @{ $$prox_list{$p} };