lp1863252 toward geosort
authorGalen Charlton <gmc@equinoxinitiative.org>
Tue, 15 Dec 2020 23:07:00 +0000 (18:07 -0500)
committerBill Erickson <berickxx@gmail.com>
Thu, 11 Mar 2021 21:00:52 +0000 (16:00 -0500)
* fix swap of lat/lon that broke Math::Trig-based distance calculations
  TODO: might want to drop that entirely in favor of delegating to a DB
  call that uses earthdistance
* first pass at implementing display of distance in the OPAC
* include "Distance" label on mobile view of copy table
* display warning if address input is not translated to coordinates
* add distance column to TPAC copy table
* add default item sort button to both TPAC and Bootstrap
* changes to open-ils.geo registration
  - don't register with the public router
  - add to param redaction list
* add temporary caching of address => coordinates results
  By default, coordinates are cached in memcached for 5 minutes. The
  cache key is derived from a SHA-2 hash of the input address.
* implement query parameter log redaction as a PerLogHandler
* Don't need perl-script for a PerlLogHandler

Signed-off-by: Galen Charlton <gmc@equinoxinitiative.org>
Signed-off-by: Terran McCanna <tmccanna@georgialibraries.org>
Open-ILS/examples/apache_24/eg_vhost.conf.in
Open-ILS/examples/opensrf.xml.example
Open-ILS/examples/opensrf_core.xml.example
Open-ILS/src/perlmods/lib/OpenILS/Application/Geo.pm
Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Record.pm
Open-ILS/src/perlmods/lib/OpenILS/WWW/EGWeb.pm
Open-ILS/src/templates-bootstrap/opac/css/mediaQuery.css.tt2
Open-ILS/src/templates-bootstrap/opac/css/style.css.tt2
Open-ILS/src/templates-bootstrap/opac/parts/record/copy_table.tt2
Open-ILS/src/templates/opac/parts/record/copy_table.tt2

index 2a78307..53f411d 100644 (file)
@@ -45,6 +45,15 @@ OSRFTranslatorConfig @sysconfdir@/opensrf_core.xml
 # Translator memcache server.  Default is localhost
 # OSRFTranslatorCacheServer 127.0.0.1:11211
 
+# ----------------------------------------------------------------------------------
+# Log redaction
+# ----------------------------------------------------------------------------------
+<Location />
+    # handler for redacting URL query parameters from
+    # the access log
+    PerlLogHandler OpenILS::WWW::EGWeb::log_handler
+    PerlAddVar OILSUrlParamToRedact "geographic-location"
+</Location>
 
 # ----------------------------------------------------------------------------------
 # Added content plugin
index 8d26f16..c713fa3 100644 (file)
@@ -753,6 +753,7 @@ vim:et:ts=4:sw=4:
                     <max_spare_children>5</max_spare_children>
                 </unix_config>
                 <app_settings>
+                  <cache_timeout>300</cache_timeout>
                 </app_settings>
             </open-ils.geo>
 
index dfa0c3b..aeba4ad 100644 (file)
@@ -30,7 +30,6 @@ Example OpenSRF bootstrap configuration file for Evergreen
           <service>open-ils.courses</service>
           <service>open-ils.curbside</service>
           <service>open-ils.fielder</service>
-          <service>open-ils.geo</service>
           <service>open-ils.pcrud</service>
           <service>open-ils.permacrud</service>
           <service>open-ils.reporter</service>
@@ -194,6 +193,8 @@ Example OpenSRF bootstrap configuration file for Evergreen
       <match_string>open-ils.cstore.direct.actor.user.update</match_string>
       <match_string>open-ils.cstore.direct.actor.user.delete</match_string>
       <match_string>open-ils.search.z3950.apply_credentials</match_string>
+      <match_string>open-ils.geo</match_string>
+      <match_string>open-ils.actor.geo</match_string>
     </log_protect>
   </shared>
 </config>
index 0d2ba8c..05874de 100644 (file)
@@ -7,8 +7,10 @@ use OpenSRF::AppSession;
 use OpenILS::Application;
 use base qw/OpenILS::Application/;
 
+use OpenSRF::Utils::SettingsClient;
 use OpenILS::Utils::CStoreEditor qw/:funcs/;
 use OpenILS::Utils::Fieldmapper;
+use OpenSRF::Utils::Cache;
 use OpenILS::Application::AppUtils;
 my $U = "OpenILS::Application::AppUtils";
 
@@ -19,6 +21,20 @@ use Geo::Coder::OSM;
 use Geo::Coder::Google;
 
 use Math::Trig qw(great_circle_distance deg2rad);
+use Digest::SHA qw(sha256_base64);
+
+my $cache;
+my $cache_timeout;
+
+sub initialize {
+    my $conf = OpenSRF::Utils::SettingsClient->new;
+
+    $cache_timeout = $conf->config_value(
+            "apps", "open-ils.geo", "app_settings", "cache_timeout" ) || 300;
+}
+sub child_init {
+    $cache = OpenSRF::Utils::Cache->new('global');
+}
 
 sub calculate_distance {
     my ($self, $conn, $pointA, $pointB) = @_;
@@ -28,7 +44,7 @@ sub calculate_distance {
     return new OpenILS::Event("BAD_PARAMS", "desc" => "Malformed coordinates") unless scalar(@{ $pointA }) == 2;
     return new OpenILS::Event("BAD_PARAMS", "desc" => "Malformed coordinates") unless scalar(@{ $pointB }) == 2;
 
-    sub NESW { deg2rad($_[0]), deg2rad(90 - $_[1]) }
+    sub NESW { deg2rad($_[1]), deg2rad(90 - $_[0]) } # longitude, latitude
     my @A = NESW( $pointA->[0], $pointA->[1] );
     my @B = NESW( $pointB->[0], $pointB->[1] );
     my $km = great_circle_distance(@A, @B, 6378);
@@ -131,7 +147,18 @@ sub retrieve_coordinates { # invoke 3rd party API for latitude/longitude lookup
     my $service = $e->retrieve_config_geolocation_service($service_id);
     return new OpenILS::Event("GEOCODING_NOT_ALLOWED") unless ($U->is_true($service));
 
+    $address =~ s/^\s+//;
+    $address =~ s/\s+$//;
     return new OpenILS::Event("BAD_PARAMS", "desc" => "No address supplied") unless $address;
+
+    # Return cached coordinates if available. We're assuming that any
+    # geolocation service will give roughly equivalent results, so we're
+    # using a hash of the user-supplied address as the cache key, not
+    # address + OU.
+    my $cache_key = 'geo.address.' . sha256_base64($address);
+    my $coords = OpenSRF::Utils::JSON->JSON2perl($cache->get_cache($cache_key));
+    return $coords if $coords;
+
     my $geo_coder;
     eval {
         if ($service->service_code eq 'Free') {
@@ -170,8 +197,10 @@ sub retrieve_coordinates { # invoke 3rd party API for latitude/longitude lookup
        $latitude = $location->{lat};
        $longitude = $location->{lon};
     }
+    $coords = { latitude => $latitude, longitude => $longitude };
+    $cache->put_cache($cache_key, OpenSRF::Utils::JSON->perl2JSON($coords), $cache_timeout);
 
-    return { latitude => $latitude, longitude => $longitude }
+    return $coords;
 }
 __PACKAGE__->register_method(
     method   => "retrieve_coordinates",
index ca5b34e..0fa267b 100644 (file)
@@ -7,6 +7,7 @@ use OpenILS::Utils::Fieldmapper;
 use OpenILS::Application::AppUtils;
 use Net::HTTP::NB;
 use IO::Select;
+use List::MoreUtils qw(uniq);
 my $U = 'OpenILS::Application::AppUtils';
 
 our $ac_types = ['toc',  'anotes', 'excerpt', 'summary', 'reviews'];
@@ -59,12 +60,32 @@ sub load_record {
         $self->timelog("load user lists and settings");
     }
 
+    # fetch geographic coordinates if user supplied an
+    # address
+    my $gl = $self->cgi->param('geographic-location');
+    my $coords;
+    if ($gl) {
+        my $geo = OpenSRF::AppSession->create("open-ils.geo");
+        $coords = $geo
+            ->request('open-ils.geo.retrieve_coordinates', $org, scalar $gl)
+            ->gather(1);
+        $geo->kill_me;
+    }
+    $ctx->{has_valid_coords} = 0;
+    if ($coords
+        && ref($coords)
+        && $$coords{latitude}
+        && $$coords{longitude}
+    ) {
+        $ctx->{has_valid_coords} = 1;
+    }
+
     # run copy retrieval in parallel to bib retrieval
     # XXX unapi
     my $cstore = OpenSRF::AppSession->create('open-ils.cstore');
     my $copy_rec = $cstore->request(
         'open-ils.cstore.json_query.atomic', 
-        $self->mk_copy_query($rec_id, $org, $copy_depth, $copy_limit, $copy_offset, $pref_ou)
+        $self->mk_copy_query($rec_id, $org, $copy_depth, $copy_limit, $copy_offset, $pref_ou, $coords)
     );
 
     if ($self->cgi->param('badges')) {
@@ -107,6 +128,24 @@ sub load_record {
         $ctx->{course_module_opt_in} = 1;
     }
 
+    $ctx->{ou_distances} = {};
+    if ($ctx->{has_valid_coords}) {
+        my $circ_libs = [ uniq map { $_->{circ_lib} } @{$ctx->{copies}} ];
+        my $foreign_copy_circ_libs = [ 
+            map { $_->target_copy()->circ_lib() }
+            map { @{ $_->foreign_copy_maps() } }
+            @{ $ctx->{foreign_copies} }
+        ];
+        push @{ $circ_libs }, @$foreign_copy_circ_libs; # some overlap is OK here
+        my $ou_distance_list = $U->simplereq(
+            'open-ils.geo',
+            'open-ils.geo.sort_orgs_by_distance_from_coordinate.include_distances',
+            [ $coords->{latitude}, $coords->{longitude} ],
+            $circ_libs
+        );
+        $ctx->{ou_distances} = { map { $_->[0] => $_->[1] } @$ou_distance_list };
+    }
+
     # Add public copy notes to each copy - and while we're in there, grab peer bib records
     # and copy tags. Oh and if we're working with course materials, those too.
     my %cached_bibs = ();
@@ -310,6 +349,7 @@ sub mk_copy_query {
     my $copy_limit = shift;
     my $copy_offset = shift;
     my $pref_ou = shift;
+    my $coords = shift;
 
     my $query = $U->basic_opac_copy_query(
         $rec_id, undef, undef, $copy_limit, $copy_offset, $self->ctx->{is_staff}
@@ -341,21 +381,14 @@ sub mk_copy_query {
         }};
     };
 
-       my $ou_sort_param = [$org, $pref_ou ];
-       my $gl = $self->cgi->param('geographic-location');
-       if ($gl) {
-               my $geo = OpenSRF::AppSession->create("open-ils.geo");
-               my $coords = $geo
-                       ->request('open-ils.geo.retrieve_coordinates', $org, scalar $gl)
-                       ->gather(1);
-               if ($coords
-                       && ref($coords)
-                       && $$coords{latitude}
-                       && $$coords{longitude}
-               ) {
-                       push(@$ou_sort_param, $$coords{latitude}, $$coords{longitude});
-               }
-       }
+    my $ou_sort_param = [$org, $pref_ou ];
+    if ($coords
+        && ref($coords)
+        && $$coords{latitude}
+        && $$coords{longitude}
+    ) {
+        push(@$ou_sort_param, $$coords{latitude}, $$coords{longitude});
+    }
 
     # Unsure if we want these in the shared function, leaving here for now
     unshift(@{$query->{order_by}},
index 0d2fe26..8d48deb 100644 (file)
@@ -49,6 +49,27 @@ sub child_init {
     return Apache2::Const::OK;
 }
 
+sub log_handler {
+    my $r = shift;
+
+    my @params_to_redact = uniq $r->dir_config->get('OILSUrlParamToRedact');
+    my $re = '('. join('|', map { quotemeta($_) } @params_to_redact) . ')=(?:[^&;]*)';
+
+    my $args = $r->args();
+    $args =~ s/$re/$1=[REDACTED]/g;
+    $r->args($args);
+    my $req = $r->the_request(); # munging args doesn't update the
+                                 # original requested URI
+    $req =~ s/$re/$1=[REDACTED]/g;
+    $r->the_request($req);
+
+    if ($r->headers_in->{Referer}) {
+        $r->headers_in->{Referer} =~ s/$re/$1=[REDACTED]/g;
+    }
+
+    return Apache2::Const::OK;
+}
+
 sub handler {
     my $r = shift;
     my $stat = handler_guts($r);
index 66439e7..6105543 100644 (file)
@@ -66,6 +66,7 @@ only screen and (max-width: 650px)  {
     .copyTable td:nth-of-type(4):before { content: "Shelving Location"; display: flex;}
     .copyTable td:nth-of-type(5):before { content: "Status"; display: flex;}
     .copyTable td:nth-of-type(6):before { content: "Due Date"; display: flex;}
+    .copyTable td:nth-of-type(7):before { content: "[% l('Distance') %]"; display: flex;}
 
     .holdingsTable tr:nth-of-type(1):before { content: "Copy #1"; display: block; text-align:center; }
     .holdingsTable tr:nth-of-type(2):before { content: "Copy #2"; display: block; text-align:center;}
index 77c6bc2..74faa1e 100755 (executable)
@@ -87,6 +87,7 @@ only screen and (max-width: 650px)  {
     .copyTable td:nth-of-type(4):before { content: "Shelving Location"; display: flex;}
     .copyTable td:nth-of-type(5):before { content: "Status"; display: flex;}
     .copyTable td:nth-of-type(6):before { content: "Due Date"; display: flex;}
+    .copyTable td:nth-of-type(7):before { content: "[% l('Distance') %]"; display: flex;}
 
     .holdingsTable tr:nth-of-type(1):before { content: "Copy #1"; display: block; text-align:center; }
     .holdingsTable tr:nth-of-type(2):before { content: "Copy #2"; display: block; text-align:center;}
index 9bf4fd9..6851879 100755 (executable)
@@ -25,6 +25,20 @@ FOREACH copy_info IN copies;
     END;
 END;
 -%]
+[%- MACRO display_ou_distance(ou) BLOCK;
+    km = ctx.ou_distances.$ou;
+    IF km && km != '-1';
+        IF ctx.get_org_setting(ctx.physical_loc || ctx.search_ou, 'opac.geographic_proximity_in_miles');
+            distance = l('[_1] mi', POSIX.sprintf('%.01f', km / 1.609));
+        ELSE;
+            distance = l('[_1] km', POSIX.sprintf('%.01f', km));
+        END;
+    ELSE;
+        distance = '-';
+    END;
+%]
+[% distance %]
+[%- END %]
 [%-
 IF has_copies or ctx.foreign_copies;
   depth = CGI.param('copy_depth').defined ? CGI.param('copy_depth') : CGI.param('depth').defined ? CGI.param('depth') : ctx.copy_summary.last.depth;
@@ -40,8 +54,19 @@ IF has_copies or ctx.foreign_copies;
     <input type="text" id="geographic-location-box" name="geographic-location" aria-label="[% l('Enter address or postal code') %]" placeholder="[% l('Enter address/postal code') %]" class="search-box" x-webkit-speech="" value="[% p = 'geographic-location'; CGI.params.$p %]"></input>
     <button type="submit" class="btn btn-confirm">[% l('Go') %]</button>
 </span>
+[% p = 'geographic-location'; IF CGI.params.$p && !ctx.has_valid_coords %]
+<span class="opac-alert">[% l('Sorry, your address is not recognized') %]</span>
+[% END %]
+</form>
+[% p = 'geographic-location'; IF CGI.params.$p && ctx.has_valid_coords %]
+<form method="GET">
+[% FOREACH p IN CGI.params.keys; NEXT IF p == 'geographic-location' %]
+  <input type="hidden" name="[% p | html %]" value="[% CGI.params.$p | html %]"/>
+[% END %]
+   <button type="submit" class="opac-button">[% l('Use default item sort') %]</button>
 </form>
 [% END %]
+[% END %]
 <table class="container-fluid table table-hover mt-4 miniTable copyTable w-100" >
     <thead>
         <tr>
@@ -70,6 +95,8 @@ IF has_copies or ctx.foreign_copies;
             <th scope='col'>[% l("Due Date") %]</th>
             [%- IF ctx.use_courses %]
             <th scope='col'>[% l("Courses") %]</th>
+            [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+            <th scope='col'>[% l("Distance") %]</th>
             [%- END %]
         </tr>
     </thead>
@@ -92,6 +119,9 @@ IF has_copies or ctx.foreign_copies;
     <td>[% bib.target_copy.location.name | html %]</td>
     <td>[% bib.target_copy.status.name | html %]</td>
     <td>[% date.format(ctx.parse_datetime(copy_info.due_date, copy_info.circ_circ_lib),DATE_FORMAT) %]</td>
+    [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+    <td>[% display_ou_distance(bib.target_copy.circ_lib) %]</td>
+    [%- END %]
 </tr>
    [%- END; # FOREACH peer
 END; # FOREACH bib
@@ -250,6 +280,9 @@ END; # FOREACH bib
                 <div>[% course.course_number %]</div>
             [% END %]</td>
             [% END %]
+            [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+                <td>[% display_ou_distance(copy_info.circ_lib) %]</td>
+            [%- END %]
         </tr>
 
         [% IF copy_info.notes; %]
index f62e96e..dc47ab7 100644 (file)
@@ -25,6 +25,20 @@ FOREACH copy_info IN copies;
     END;
 END;
 -%]
+[%- MACRO display_ou_distance(ou) BLOCK;
+    km = ctx.ou_distances.$ou;
+    IF km && km != '-1';
+        IF ctx.get_org_setting(ctx.physical_loc || ctx.search_ou, 'opac.geographic_proximity_in_miles');
+            distance = l('[_1] mi', POSIX.sprintf('%.01f', km / 1.609));
+        ELSE;
+            distance = l('[_1] km', POSIX.sprintf('%.01f', km));
+        END;
+    ELSE;
+        distance = '-';
+    END;
+%]
+[% distance %]
+[%- END %]
 [%-
 IF has_copies or ctx.foreign_copies;
   depth = CGI.param('copy_depth').defined ? CGI.param('copy_depth') : CGI.param('depth').defined ? CGI.param('depth') : ctx.copy_summary.last.depth;
@@ -42,7 +56,18 @@ IF has_copies or ctx.foreign_copies;
     <input type="text" id="geographic-location-box" name="geographic-location" aria-label="[% l('Enter address or postal code') %]" placeholder="[% l('Enter address/postal code') %]" class="search-box" x-webkit-speech="" value="[% p = 'geographic-location'; CGI.params.$p %]"></input>
     <button type="submit" class="opac-button">[% l('Go') %]</button>
 </span>
+[% p = 'geographic-location'; IF CGI.params.$p && !ctx.has_valid_coords %]
+<span class="alert">[% l('Sorry, your address is not recognized') %]</span>
+[% END %]
 </form>
+[% p = 'geographic-location'; IF CGI.params.$p && ctx.has_valid_coords %]
+<form method="GET">
+[% FOREACH p IN CGI.params.keys; NEXT IF p == 'geographic-location' %]
+  <input type="hidden" name="[% p | html %]" value="[% CGI.params.$p | html %]"/>
+[% END %]
+   <button type="submit" class="opac-button">[% l('Use default item sort') %]</button>
+</form>
+[% END %]
 [% END %]
 <table class="table_no_border_space table_no_cell_pad table_no_border" width="100%" id="rdetails_status">
     <thead>
@@ -73,6 +98,9 @@ IF has_copies or ctx.foreign_copies;
             [%- IF use_courses %]
             <th scope='col'>[% l("Courses") %]</th>
             [%- END %]
+            [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+            <th scope='col'>[% l("Distance") %]</th>
+            [%- END %]
         </tr>
     </thead>
     <tbody class="copy_details_table">
@@ -98,6 +126,9 @@ IF has_copies or ctx.foreign_copies;
         OR use_courses %]
     <td></td>
     [%- END %]
+    [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+    <td>[% display_ou_distance(bib.target_copy.circ_lib) %]</td>
+    [%- END %]
 </tr>
    [%- END; # FOREACH peer
 END; # FOREACH bib
@@ -256,6 +287,9 @@ END; # FOREACH bib
                 <div>[% course.course_number %]</div>
             [% END %]</td>
             [% END %]
+            [%- IF ctx.geo_sort && ctx.has_valid_coords %]
+               <td>[% display_ou_distance(copy_info.circ_lib) %]</td>
+            [%- END %]
         </tr>
 
         [% IF copy_info.notes; %]