Simplified Hold Pull List: Fix several sorting bugs

First of all, sorting on most columns was broken due to a bug in the way
that the flattener methods of the open-ils.fielder service were
constructing their SQL JOINs.  We were coming up with way too many
joins, and then losing track of which JOIN's alias to refer to when
building the ORDER BY clause later.  This is fixed.

Secondly, the shelving location column now sorts automatically by the
shelving location *ordering* values, when avaiable.  These are the
values that you set up in the drag-and-drop staff client interface
titled "Copy Location Order."  When these values are not set for the org
unit whose pull list you're viewing, the sorting will fall back to
alphabetical.

Signed-off-by: Lebbeous Fogle-Weekley <lebbeous@esilibrary.com>
Signed-off-by: Mike Rylander <mrylander@gmail.com>

diff --git a/Open-ILS/examples/fm_IDL.xml b/Open-ILS/examples/fm_IDL.xml
index eb75532..739bf83 100644 (file)
--- a/Open-ILS/examples/fm_IDL.xml
+++ b/Open-ILS/examples/fm_IDL.xml
@@ -5031,7 +5031,7 @@ SELECT  usr,
                <oils_persist:source_definition><![CDATA[
                SELECT
                        ahr.*,
-                       COALESCE(acplo.position, 999) AS
+                       COALESCE(acplo.position, acpl_ordered.fallback_position) AS
                                copy_location_order_position,
                        CASE WHEN au.alias IS NOT NULL THEN
                                au.alias
@@ -5055,6 +5055,10 @@ SELECT  usr,
                JOIN asset.call_number_prefix acnp ON (acn.prefix = acnp.id)
                JOIN asset.call_number_suffix acns ON (acn.suffix = acns.id)
                JOIN actor.usr au ON (au.id = ahr.usr)
+               JOIN (
+                       SELECT *, (ROW_NUMBER() OVER (ORDER BY name) + 1000000) AS fallback_position
+                       FROM asset.copy_location
+               ) acpl_ordered ON (acpl_ordered.id = acp.location)
                LEFT JOIN actor.usr_standing_penalty ausp 
                        ON (ahr.usr = ausp.usr AND (ausp.stop_date IS NULL OR ausp.stop_date > NOW()))
                LEFT JOIN config.standing_penalty csp

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Fielder.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Fielder.pm
index 343c285..610a86b 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Fielder.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Fielder.pm
@@ -24,6 +24,9 @@ use XML::LibXML::XPathContext;
 use XML::LibXSLT;
 
 use OpenILS::Application::Flattener;
+use Data::Dumper;
+
+$Data::Dumper::Indent = 0;
 
 our %namespace_map = (
     oils_persist=> {ns => 'http://open-ils.org/spec/opensrf/IDL/persistence/v1'},

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Flattener.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Flattener.pm
index ff232a9..8bc8eda 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Flattener.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Flattener.pm
@@ -13,6 +13,10 @@ use OpenSRF::Utils::Logger qw/:logger/;
 use OpenILS::Utils::CStoreEditor q/:funcs/;
 use OpenSRF::Utils::JSON;
 
+use Data::Dumper;
+
+$Data::Dumper::Indent = 0;
+
 sub _fm_link_from_class {
     my ($class, $field) = @_;
 
@@ -255,6 +259,11 @@ sub process_map {
         join => {}
     };
 
+    # Here's a hash where we'll keep track of whether we've already provided
+    # a join to cover a given hash.  It seems that without this we build
+    # redundant joins.
+    my $join_coverage = {};
+
     foreach my $k (keys %$map) {
         my $column = $map->{$k} =
             _flattened_search_normalize_map_column($map->{$k});
@@ -269,8 +278,23 @@ sub process_map {
 
         # For filter or sort columns, we'll need joining.
         if ($column->{filter} or $column->{sort}) {
-            my ($clause, $last_join_alias) =
-                _flattened_search_single_join_clause($k,$hint,$column->{path});
+            my @path = @{ $column->{path} };
+            pop @path; # discard last part (field)
+            my $joinkey = join(",", @path);
+
+            my ($clause, $last_join_alias);
+
+            # Skip joins that are already covered. We shouldn't need more than
+            # one join for the same path
+            if ($join_coverage->{$joinkey}) {
+                ($clause, $last_join_alias) = @{ $join_coverage->{$joinkey} };
+            } else {
+                ($clause, $last_join_alias) =
+                    _flattened_search_single_join_clause(
+                        $k, $hint, $column->{path}
+                    );
+                $join_coverage->{$joinkey} = [$clause, $last_join_alias];
+            }
 
             $map->{$k}{last_join_alias} = $last_join_alias;
             _flattened_search_merge_join_clause($jffolo->{join}, $clause);
@@ -334,6 +358,7 @@ sub finish_jffolo {
 
         if ($map->{$key}) {
             my $class = $map->{$key}{last_join_alias} || $core_hint;
+
             push @{ $jffolo->{order_by} }, {
                 class => $class,
                 field => $map->{$key}{path}[-1],
@@ -355,7 +380,7 @@ sub process_result {
 
     if (not ref $fmobj) {
         throw OpenSRF::EX::ERROR(
-            "process_result() was passed an inappropriate second argument"
+            "process_result() was passed an inappropriate second argument ($fmobj)"
         );
     }
 

diff --git a/Open-ILS/src/templates/circ/hold_pull_list.tt2 b/Open-ILS/src/templates/circ/hold_pull_list.tt2
index fc497b9..8355ffe 100644 (file)
--- a/Open-ILS/src/templates/circ/hold_pull_list.tt2
+++ b/Open-ILS/src/templates/circ/hold_pull_list.tt2
@@ -71,9 +71,9 @@
         editStyle="pane"
         showLoadFilter="true"
         fmClass="'ahopl'"
-        defaultSort="['copy_location_sort_order','call_number_sort_key']"
+        defaultSort="['copy_location_order_position','call_number_sort_key']"
         mapExtras="map_extras"
-        sortFieldReMap="{call_number_label: 'call_number_sort_key'}"
+        sortFieldReMap="{call_number_label:'call_number_sort_key',shelving_loc:'copy_location_order_position'}"
         fetchLock="true"
         query="{}">
         <thead>