From 49e4e7020d0947ce5f436fe32bbd3342730348c3 Mon Sep 17 00:00:00 2001 From: Joe Atzberger Date: Wed, 6 Aug 2008 03:44:46 -0500 Subject: [PATCH] Patron import reform - bug 2287 - expanded error catching and feedback This incorporates and extends the patch from MJ Ray attached to bug 2287. Added feedback of up to 25 lines, including for errors at the Text::CSV parsing level. This allows feedback for problems than involve encoding. Added link to download "starter" CSV file (with all the columns). Signed-off-by: Joshua Ferraro --- C4/Members.pm | 45 ++-- .../en/modules/tools/import_borrowers.tmpl | 79 ++++--- tools/import_borrowers.pl | 211 +++++++++++------- 3 files changed, 209 insertions(+), 126 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 1f0328225a..385faf9a4c 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -590,6 +590,10 @@ sub GetMemberIssuesAndFines { return ($overdue_count, $issue_count, $total_fines); } +sub columns(;$) { + return @{C4::Context->dbh->selectcol_arrayref("SHOW columns from borrowers")}; +} + =head2 =head2 ModMember @@ -607,7 +611,6 @@ true on success, or false on failure =cut -#' sub ModMember { my (%data) = @_; my $dbh = C4::Context->dbh; @@ -616,41 +619,43 @@ sub ModMember { if (my $tempdate = $data{$_}) { # assignment, not comparison ($tempdate =~ /$iso_re/) and next; # Congatulations, you sent a valid ISO date. warn "ModMember given $_ not in ISO format ($tempdate)"; - if (my $tempdate2 = format_date_in_iso($tempdate)) { # assignment, not comparison - $data{$_} = $tempdate2; - } else { - warn "ModMember cannot convert '$tempdate' (from syspref)"; + my $tempdate2 = format_date_in_iso($tempdate); + if (!$tempdate2 or $tempdate2 eq '0000-00-00') { + warn "ModMember cannot convert '$tempdate' (from syspref to ISO)"; + next; } + $data{$_} = $tempdate2; } } if (!$data{'dateofbirth'}){ delete $data{'dateofbirth'}; } - my $qborrower=$dbh->prepare("SHOW columns from borrowers"); - $qborrower->execute; - my %hashborrowerfields; - while (my ($field)=$qborrower->fetchrow){ - $hashborrowerfields{$field}=1; - } + my @columns = &columns; + my %hashborrowerfields = (map {$_=>1} @columns); my $query = "UPDATE borrowers SET \n"; my $sth; my @parameters; # test to know if you must update or not the borrower password - if ( exists $data{'password'} ) { - if ( $data{'password'} eq '****' ) { - delete $data{'password'}; + if (exists $data{password}) { + if ($data{password} eq '****' or $data{password} eq '') { + delete $data{password}; } else { - $data{'password'} = md5_base64( $data{'password'} ) if ( $data{'password'} ne "" ); - delete $data{'password'} if ( $data{password} eq "" ); + $data{password} = md5_base64($data{password}); } } - foreach (keys %data){ - if ($_ ne 'borrowernumber' and $_ ne 'flags' and $hashborrowerfields{$_}){ - $query .= " $_=?, "; - push @parameters,$data{$_}; + my @badkeys; + foreach (keys %data) { + next if ($_ eq 'borrowernumber' or $_ eq 'flags'); + if ($hashborrowerfields{$_}){ + $query .= " $_=?, "; + push @parameters,$data{$_}; + } else { + push @badkeys, $_; + delete $data{$_}; } } + (@badkeys) and warn scalar(@badkeys) . " Illegal key(s) passed to ModMember: " . join(',',@badkeys); $query =~ s/, $//; $query .= " WHERE borrowernumber=?"; push @parameters, $data{'borrowernumber'}; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tmpl b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tmpl index 17184392f4..e6d6c82f0e 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tmpl +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tmpl @@ -1,12 +1,16 @@ Koha › Cataloging › Import Patrons <!-- TMPL_IF NAME="uploadborrowers" -->› Results<!-- /TMPL_IF --> + - +
@@ -17,15 +21,37 @@

Import Patrons

-

Import results :

+
Import results :
    -
  • imported records
    • -
  • overwritten
    • -
  • not imported because already in borrowers table and overwrite disabled
    • -
  • not imported because they are not in the expected format
    • -
  • records parsed
    • -
  • Back
    • +
  • imported records (last was )
    • +
  • overwritten (last was )
    • +
  • not imported because already in borrowers table and overwrite disabled (last was )
    • +
  • not imported because they are not in the expected format (last was )
    • +
  • records parsed
    • +
  • Back to Tools
+ +

+
+
Error analysis:
+
    + +
  • Header row could not be parsed
    • + +
  • + + Line could not be parsed! + + Critical field "" missing on line + (borrowernumber: ; surname: ). + +
    +
    • + + +
+
+
  • Select a file to import into the borrowers table
    • @@ -53,42 +79,43 @@ +
    +Default values +
      + +
    1. + + " name="" /> +
      2. + +
    If matching record is already in the borrowers table:
    2. -
    + +

Notes:

    -
  • Format the file in CSV format with the following fields:
    • -
  • - 'cardnumber', 'surname', 'firstname', 'title', - 'othernames', 'initials', 'streetnumber', 'streettype', - 'address', 'address2', 'city', 'zipcode', - 'email', 'phone', 'mobile', 'fax', - 'emailpro', 'phonepro', 'B_streetnumber', 'B_streettype', - 'B_address', 'B_city', 'B_zipcode', 'B_email', - 'B_phone', 'dateofbirth', 'branchcode', 'categorycode', - 'dateenrolled', 'dateexpiry', 'gonenoaddress', 'lost', - 'debarred', 'contactname', 'contactfirstname', 'contacttitle', - 'borrowernotes', 'relationship', 'ethnicity', 'ethnotes', - 'sex', 'userid', 'opacnote', 'contactnote', - 'password', 'sort1', 'sort2', 'patron_attributes' -
    • +
  • Download a starter CSV file with all the columns here. Values are comma-separated.
    • +
  • OR format your file in CSV format with the following fields:
    • +
    • + '', +
  • If loading patron attributes, the 'patron_attributes' field should contain a comma-separated list of attribute types and values. The attribute type code and a ':' should precede each value. For example: "INSTID:12345,LANG:fr". This means that if an input record has more than one attribute, the 'patron_attributes' field must be wrapped in double quotation marks.
  • -
  • Please make sure the 'branchcode' and 'categorycode' are valid entries in your database.
    • -
  • password should be stored in plaintext, and will be converted to a md5 hash (if your passwords are already encrypted, talk to your systems administrator about options).
    • +
  • The fields 'branchcode' and 'categorycode' are required and must match valid entries in your database.
    • +
  • 'password' should be stored in plaintext, and will be converted to a md5 hash (if your passwords are already encrypted, talk to your systems administrator about options).
diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index a58e0da4f7..d8c4068bd6 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -45,37 +45,37 @@ use C4::Members::AttributeTypes; use Text::CSV; use CGI; -my @columnkeys = ( - 'cardnumber', 'surname', 'firstname', 'title', - 'othernames', 'initials', 'streetnumber', 'streettype', - 'address', 'address2', 'city', 'zipcode', - 'email', 'phone', 'mobile', 'fax', - 'emailpro', 'phonepro', 'B_streetnumber', 'B_streettype', - 'B_address', 'B_city', 'B_zipcode', 'B_email', - 'B_phone', 'dateofbirth', 'branchcode', 'categorycode', - 'dateenrolled', 'dateexpiry', 'gonenoaddress', 'lost', - 'debarred', 'contactname', 'contactfirstname', 'contacttitle', - 'borrowernotes', 'relationship', 'ethnicity', 'ethnotes', - 'sex', 'userid', 'opacnote', 'contactnote', - 'password', 'sort1', 'sort2' -); -if (C4::Context->preference('ExtendedPatronAttributes')) { +my @errors; +my $extended = C4::Context->preference('ExtendedPatronAttributes'); +my @columnkeys = C4::Members->columns; +if ($extended) { push @columnkeys, 'patron_attributes'; } +my $columnkeystpl = [ map { {'key' => $_} } @columnkeys ]; # ref. to array of hashrefs. -my $input = new CGI; +my $input = CGI->new(); +my $csv = Text::CSV->new(); -my ( $template, $loggedinuser, $cookie ) = get_template_and_user( - { +my ( $template, $loggedinuser, $cookie ) = get_template_and_user({ template_name => "tools/import_borrowers.tmpl", query => $input, type => "intranet", authnotrequired => 0, flagsrequired => { tools => 'import_patrons' }, debug => 1, - } -); +}); + +$template->param(columnkeys => $columnkeystpl); +if ($input->param('sample')) { + print $input->header( + -type => 'application/vnd.sun.xml.calc', # 'application/vnd.ms-excel' ? + -attachment => 'patron_import.csv', + ); + $csv->combine(@columnkeys); + print $csv->string, "\n"; + exit 1; +} my $uploadborrowers = $input->param('uploadborrowers'); my $matchpoint = $input->param('matchpoint'); if ($matchpoint) { @@ -85,89 +85,140 @@ my $overwrite_cardnumber = $input->param('overwrite_cardnumber'); $template->param( SCRIPT_NAME => $ENV{'SCRIPT_NAME'} ); -if (C4::Context->preference('ExtendedPatronAttributes')) { - $template->param(ExtendedPatronAttributes => 1); -} +($extended) and $template->param(ExtendedPatronAttributes => 1); if ( $uploadborrowers && length($uploadborrowers) > 0 ) { - my $csv = Text::CSV->new(); my $imported = 0; my $alreadyindb = 0; my $overwritten = 0; my $invalid = 0; my $matchpoint_attr_type; + my %defaults = $input->Vars; + + # use header line to construct key to column map + my $borrowerline = <$uploadborrowers>; + my $status = $csv->parse($borrowerline); + ($status) or push @errors, {badheader=>1,line=>$., lineraw=>$borrowerline}; + my @csvcolumns = $csv->fields(); + my %csvkeycol; + my $col = 0; + foreach my $keycol (@csvcolumns) { + # columnkeys don't contain whitespace, but some stupid tools add it + $keycol =~ s/ +//g; + $csvkeycol{$keycol} = $col++; + } + #warn($borrowerline); - if (C4::Context->preference('ExtendedPatronAttributes')) { + if ($extended) { $matchpoint_attr_type = C4::Members::AttributeTypes->fetch($matchpoint); } - while ( my $borrowerline = <$uploadborrowers> ) { - my $status = $csv->parse($borrowerline); - my @columns = $csv->fields(); + my @criticals = qw(surname); # there probably should be others + my @errors; + LINE: while ( my $borrowerline = <$uploadborrowers> ) { my %borrower; + my @missing_criticals; my $patron_attributes; - if ( @columns == @columnkeys ) { + my $status = $csv->parse($borrowerline); + my @columns = $csv->fields(); + if (! $status) { + push @missing_criticals, {badparse=>1, line=>$., lineraw=>$borrowerline}; + } elsif (@columns == @columnkeys) { @borrower{@columnkeys} = @columns; - my @attrs; - if (C4::Context->preference('ExtendedPatronAttributes')) { - my $attr_str = $borrower{patron_attributes}; - delete $borrower{patron_attributes}; - my $ok = $csv->parse($attr_str); - my @list = $csv->fields(); - # FIXME error handling - $patron_attributes = [ map { map { my @arr = split /:/, $_, 2; { code => $arr[0], value => $arr[1] } } $_ } @list ]; + } else { + # MJR: try to recover gracefully by using default values + foreach my $key (@columnkeys) { + if (defined($csvkeycol{$key}) and $columns[$csvkeycol{$key}] =~ /\S/) { + $borrower{$key} = $columns[$csvkeycol{$key}]; + } elsif ( $defaults{$key} ) { + $borrower{$key} = $defaults{$key}; + } elsif ( scalar grep {$key eq $_} @criticals ) { + # a critical field is undefined + push @missing_criticals, {key=>$key, line=>$., lineraw=>$borrowerline}; + } else { + $borrower{$key} = ''; + } } - foreach (qw(dateofbirth dateenrolled dateexpiry)) { - my $tempdate = $borrower{$_} or next; - $borrower{$_} = format_date_in_iso($tempdate) || ''; - } - my $borrowernumber; - if ($matchpoint eq 'cardnumber') { - my $member = GetMember( $borrower{'cardnumber'}, 'cardnumber' ); - if ($member) { - $borrowernumber = $member->{'borrowernumber'}; - } - } elsif (C4::Context->preference('ExtendedPatronAttributes')) { - if (defined($matchpoint_attr_type)) { - foreach my $attr (@$patron_attributes) { - if ($attr->{code} eq $matchpoint and $attr->{value} ne '') { - my @borrowernumbers = $matchpoint_attr_type->get_patrons($attr->{value}); - $borrowernumber = $borrowernumbers[0] if scalar(@borrowernumbers) == 1; - last; - } + } + #warn join(':',%borrower); + if (@missing_criticals) { + foreach (@missing_criticals) { + $_->{borrowernumber} = $borrower{borrowernumber} || 'UNDEF'; + $_->{surname} = $borrower{surname} || 'UNDEF'; + } + $invalid++; + (25 > scalar @errors) and push @errors, {missing_criticals=>\@missing_criticals}; + # The first 25 errors are enough. Keeping track of 30,000+ would destroy performance. + next LINE; + } + my @attrs; + if ($extended) { + my $attr_str = $borrower{patron_attributes}; + delete $borrower{patron_attributes}; + my $ok = $csv->parse($attr_str); + my @list = $csv->fields(); + # FIXME error handling + $patron_attributes = [ map { map { my @arr = split /:/, $_, 2; { code => $arr[0], value => $arr[1] } } $_ } @list ]; + } + foreach (qw(dateofbirth dateenrolled dateexpiry)) { + my $tempdate = $borrower{$_} or next; + $borrower{$_} = format_date_in_iso($tempdate) || ''; + } + my $borrowernumber; + if ($matchpoint eq 'cardnumber') { + my $member = GetMember( $borrower{'cardnumber'}, 'cardnumber' ); + if ($member) { + $borrowernumber = $member->{'borrowernumber'}; + } + } elsif ($extended) { + if (defined($matchpoint_attr_type)) { + foreach my $attr (@$patron_attributes) { + if ($attr->{code} eq $matchpoint and $attr->{value} ne '') { + my @borrowernumbers = $matchpoint_attr_type->get_patrons($attr->{value}); + $borrowernumber = $borrowernumbers[0] if scalar(@borrowernumbers) == 1; + last; } } } + } - if ( $borrowernumber) - { - # borrower exists - if ($overwrite_cardnumber) { - $borrower{'borrowernumber'} = $borrowernumber; - ModMember(%borrower); - if (C4::Context->preference('ExtendedPatronAttributes')) { - C4::Members::Attributes::SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes); - } - $overwritten++; - } else { - $alreadyindb++; - } + if ($borrowernumber) { + # borrower exists + unless ($overwrite_cardnumber) { + $alreadyindb++; + $template->param('lastalreadyindb'=>$borrower{'surname'}.' / '.$borrowernumber); + next LINE; } - else { - if ($borrowernumber = AddMember(%borrower)) { - if (C4::Context->preference('ExtendedPatronAttributes')) { - C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $patron_attributes); - } - $imported++; - } else { - $invalid++; # was just "$invalid", I assume incrementing was the point --atz - } + $borrower{'borrowernumber'} = $borrowernumber; + unless (ModMember(%borrower)) { + $invalid++; + $template->param('lastinvalid'=>$borrower{'surname'}.' / '.$borrowernumber); + next LINE; } + if ($extended) { + C4::Members::Attributes::SetBorrowerAttributes($borrower{'borrowernumber'}, $patron_attributes); + } + $overwritten++; + $template->param('lastoverwritten'=>$borrower{'surname'}.' / '.$borrowernumber); } else { - $invalid++; + # FIXME: fixup_cardnumber says to lock table, but the web interface doesn't so this doesn't either. + # At least this is closer to AddMember than in members/memberentry.pl + if (!$borrower{'cardnumber'}) { + $borrower{'cardnumber'} = fixup_cardnumber(''); + } + if ($borrowernumber = AddMember(%borrower)) { + if ($extended) { + C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $patron_attributes); + } + $imported++; + $template->param('lastimported'=>$borrower{'surname'}.' / '.$borrowernumber); + } else { + $invalid++; # was just "$invalid", I assume incrementing was the point --atz + $template->param('lastinvalid'=>$borrower{'surname'}.' / AddMember'); + } } } - $template->param( 'uploadborrowers' => 1 ); + (@errors) and $template->param(ERRORS=>\@errors); $template->param( 'uploadborrowers' => 1, 'imported' => $imported, @@ -178,7 +229,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { ); } else { - if (C4::Context->preference('ExtendedPatronAttributes')) { + if ($extended) { my @matchpoints = (); my @attr_types = C4::Members::AttributeTypes::GetAttributeTypes(); foreach my $type (@attr_types) { -- 2.39.5