Fix broken ISBN cleaning, handle missing keys

Previous attempt at ISBN cleaning was broken. This approach
is not broken.

If we find no valid ISBN and no UPC values in the record,
do not attempt to call an AC handler. This will need
to change when we have AC handlers that rely on other
possible keys.

Caching improvement: serve from cache before fetching keys
from the bib record in the database.

Also:
* Fixed a stray "next" left over from an earlier re-factor.
* Removed some debugging.

Signed-off-by: Jeff Godin <jgodin@tadl.org>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/WWW/AddedContent.pm b/Open-ILS/src/perlmods/lib/OpenILS/WWW/AddedContent.pm
index 90a9de2..169a183 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/WWW/AddedContent.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/WWW/AddedContent.pm
@@ -20,6 +20,8 @@ use OpenILS::Utils::CStoreEditor;
 use LWP::UserAgent;
 use MIME::Base64;
 
+use Business::ISBN;
+
 my $AC = __PACKAGE__;
 
 
@@ -114,44 +116,44 @@ sub handler {
     my $keyhash;
     my $cachekey;
 
+    $cachekey = ($keytype eq "isbn") ? $keyvalue : $keytype . '_' . $keyvalue;
+
+    child_init() unless $handler;
+
+    return Apache2::Const::NOT_FOUND unless $handler and $type and $format and $cachekey;
+
+    my $err;
+    my $data;
+    my $method = "${type}_${format}";
+
+    return Apache2::Const::NOT_FOUND unless $handler->can($method);
+    return $res if defined($res = $AC->serve_from_cache($type, $format, $cachekey));
+    return Apache2::Const::NOT_FOUND unless $AC->lookups_enabled;
+
     if ($keytype eq "isbn") { # if this request uses isbn for the key
         # craft a hash with the single isbn, because that's all we will have
         $keyhash = {};
         $keyhash->{"isbn"} = [$keyvalue];
-        $cachekey = $keyvalue;
     } else {
         my $key_data = get_rec_keys($keyvalue);
-        my @raw_isbns = grep {$_->{tag} eq '020'} @$key_data;
+        my @isbns = grep {$_->{tag} eq '020'} @$key_data;
         my @upcs = grep {$_->{tag} eq '024'} @$key_data;
 
-        my @isbns = [map {
-            my $isbn_obj = Business::ISBN->new($_);
-            return $isbn_obj->as_string([]);
-        } @raw_isbns];
+        map {
+            my $isbn_obj = Business::ISBN->new($_->{value});
+            my $isbn_str;
+            $isbn_str = $isbn_obj->as_string([]) if defined($isbn_obj);
+            $_->{value} = $isbn_str;
+            undef $_ if !defined($_->{value});
+        } @isbns;
 
         $keyhash = {
             isbn => [map {$_->{value}} @isbns],
             upc => [map {$_->{value}} @upcs]
         };
-
-        $cachekey = $keytype . '_' . $keyvalue;
     }
 
-    child_init() unless $handler;
-
-    return Apache2::Const::NOT_FOUND unless $handler and $type and $format and $keyhash;
-
-    my $err;
-    my $data;
-    my $method = "${type}_${format}";
-
-    return Apache2::Const::NOT_FOUND unless $handler->can($method);
-    return $res if defined($res = $AC->serve_from_cache($type, $format, $cachekey));
-    return Apache2::Const::NOT_FOUND unless $AC->lookups_enabled;
-
-    # XXX DEBUG
-    use OpenSRF::Utils::JSON;
-    $logger->info("Added Content Keys: " . OpenSRF::Utils::JSON->perl2JSON($keyhash));
+    return Apache2::Const::NOT_FOUND unless @{$keyhash->{isbn}} || @{$keyhash->{upc}};
 
     try {
         $data = $handler->$method($keyhash);
@@ -162,7 +164,6 @@ sub handler {
     };
 
     return Apache2::Const::NOT_FOUND if $err;
-    next unless $data;
 
     if(!$data) {
         # if the AC lookup found no corresponding data, cache that information