[Koha-patches] [PATCH] [SIGNED OFF] Bug 5379 - import_borrowers.pl fails with db insert/update errors

[Liz Rea]

From: Chris Nighswonger <cnighswonger at foundations.edu>

Some spreadsheet programs use smart quotes which causes the db to throw
an error when an insert/update is attempted due to improper processing
of the CSV file. This patch adds code to check for smart quotes and change
them to "dumb" quotes.

This patch also adds more logging of errors and a notice to the user to check
the logs for errors when they occur.

Signed-off-by: Liz Rea <lrea at nekls.org>
---
 C4/Members.pm             |   29 ++++++++++++++++++-----------
 C4/Members/Attributes.pm  |    5 +++++
 tools/import_borrowers.pl |   10 ++++++++--
 3 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index e03b40b..7cd88af 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -772,17 +772,17 @@ sub ModMember {
         }
     }
 	my $execute_success=UpdateInTable("borrowers",\%data);
-# ok if its an adult (type) it may have borrowers that depend on it as a guarantor
-# so when we update information for an adult we should check for guarantees and update the relevant part
-# of their records, ie addresses and phone numbers
-    my $borrowercategory= GetBorrowercategory( $data{'category_type'} );
-    if ( exists  $borrowercategory->{'category_type'} && $borrowercategory->{'category_type'} eq ('A' || 'S') ) {
-        # is adult check guarantees;
-        UpdateGuarantees(%data);
+    if ($execute_success) { # only proceed if the update was a success
+        # ok if its an adult (type) it may have borrowers that depend on it as a guarantor
+        # so when we update information for an adult we should check for guarantees and update the relevant part
+        # of their records, ie addresses and phone numbers
+        my $borrowercategory= GetBorrowercategory( $data{'category_type'} );
+        if ( exists  $borrowercategory->{'category_type'} && $borrowercategory->{'category_type'} eq ('A' || 'S') ) {
+            # is adult check guarantees;
+            UpdateGuarantees(%data);
+        }
+        logaction("MEMBERS", "MODIFY", $data{'borrowernumber'}, "UPDATE (executed w/ arg: $data{'borrowernumber'})") if C4::Context->preference("BorrowersLog");
     }
-    logaction("MEMBERS", "MODIFY", $data{'borrowernumber'}, "UPDATE (executed w/ arg: $data{'borrowernumber'})") 
-        if C4::Context->preference("BorrowersLog");
-
     return $execute_success;
 }
 
@@ -792,7 +792,9 @@ sub ModMember {
   $borrowernumber = &AddMember(%borrower);
 
 insert new borrower into table
-Returns the borrowernumber
+Returns the borrowernumber upon success
+
+Returns as undef upon any db error without further processing
 
 =cut
 
@@ -810,10 +812,15 @@ sub AddMember {
     my $sth = $dbh->prepare("SELECT enrolmentfee FROM categories WHERE categorycode=?");
     $sth->execute($data{'categorycode'});
     my ($enrolmentfee) = $sth->fetchrow;
+    if ($sth->err) {
+        warn sprintf('Database returned the following error: %s', $sth->errstr);
+        return;
+    }
     if ($enrolmentfee && $enrolmentfee > 0) {
         # insert fee in patron debts
         manualinvoice($data{'borrowernumber'}, '', '', 'A', $enrolmentfee);
     }
+
     return $data{'borrowernumber'};
 }
 
diff --git a/C4/Members/Attributes.pm b/C4/Members/Attributes.pm
index 24c6f71..35d6702 100644
--- a/C4/Members/Attributes.pm
+++ b/C4/Members/Attributes.pm
@@ -203,7 +203,12 @@ sub SetBorrowerAttributes {
     foreach my $attr (@$attr_list) {
         $attr->{password} = undef unless exists $attr->{password};
         $sth->execute($borrowernumber, $attr->{code}, $attr->{value}, $attr->{password});
+        if ($sth->err) {
+            warn sprintf('Database returned the following error: %s', $sth->errstr);
+            return; # bail immediately on errors
+        }
     }
+    return 1; # borower attributes successfully set
 }
 
 =head2 extended_attributes_code_value_arrayref 
diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl
index 6141998..74a5889 100755
--- a/tools/import_borrowers.pl
+++ b/tools/import_borrowers.pl
@@ -66,7 +66,7 @@ my $columnkeystpl = [ map { {'key' => $_} }  grep {$_ ne 'borrowernumber' && $_
 
 my $input = CGI->new();
 our $csv  = Text::CSV->new({binary => 1});  # binary needed for non-ASCII Unicode
-# push @feedback, {feedback=>1, name=>'backend', value=>$csv->backend, backend=>$csv->backend};
+#push @feedback, {feedback=>1, name=>'backend', value=>$csv->backend, backend=>$csv->backend}; #XXX
 
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user({
         template_name   => "tools/import_borrowers.tmpl",
@@ -193,6 +193,9 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) {
         }
         if ($extended) {
             my $attr_str = $borrower{patron_attributes};
+            $attr_str =~ s/\xe2\x80\x9c/"/g; # fixup double quotes in case we are passed smart quotes
+            $attr_str =~ s/\xe2\x80\x9d/"/g;
+            push @feedback, {feedback=>1, name=>'attribute string', value=>$attr_str, filename=>$uploadborrowers};
             delete $borrower{patron_attributes};    # not really a field in borrowers, so we don't want to pass it to ModMember.
             $patron_attributes = extended_attributes_code_value_arrayref($attr_str); 
         }
@@ -246,6 +249,8 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) {
             }
             unless (ModMember(%borrower)) {
                 $invalid++;
+                # untill we have better error trapping, we have no way of knowing why ModMember errored out...
+                push @errors, {unknown_error => 1};
                 $template->param('lastinvalid'=>$borrower{'surname'}.' / '.$borrowernumber);
                 next LINE;
             }
@@ -254,7 +259,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) {
                     my $old_attributes = GetBorrowerAttributes($borrowernumber);
                     $patron_attributes = extended_attributes_merge($old_attributes, $patron_attributes);  #TODO: expose repeatable options in template
                 }
-                SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes);
+                push @errors, {unknown_error => 1} unless SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes);
             }
             $overwritten++;
             $template->param('lastoverwritten'=>$borrower{'surname'}.' / '.$borrowernumber);
@@ -276,6 +281,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) {
                 $template->param('lastimported'=>$borrower{'surname'}.' / '.$borrowernumber);
             } else {
                 $invalid++;
+                push @errors, {unknown_error => 1};
                 $template->param('lastinvalid'=>$borrower{'surname'}.' / AddMember');
             }
         }
-- 
1.7.2.5