[Koha-patches] [PATCH] Patron import reform - bug 2287 - expanded error catching and feedback
Joe Atzberger
joe.atzberger at liblime.com
Wed Aug 6 10:44:46 CEST 2008
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).
---
C4/Members.pm | 45 +++--
.../prog/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 1f03282..385faf9 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(',', at 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 1718439..e6d6c82 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 @@
<!-- TMPL_INCLUDE NAME="doc-head-open.inc" -->
<title>Koha › Cataloging › Import Patrons <!-- TMPL_IF NAME="uploadborrowers" -->› Results<!-- /TMPL_IF --></title>
<!-- TMPL_INCLUDE NAME="doc-head-close.inc" -->
+<style type="text/css">
+ .yui-u fieldset.rows label.widelabel { width: 12em; }
+ code { background-color: yellow; }
+</style>
</head>
<body>
<!-- TMPL_INCLUDE NAME="header.inc" -->
<!-- TMPL_INCLUDE NAME="patron-search.inc"-->
-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> › <!-- TMPL_IF name="uploadborrowers" --><a href="/cgi-bin/koha/tools/import_borrowers.pl">Import Patrons</a> › Results<!-- TMPL_ELSE -->Import Patrons<!-- /TMPL_IF --></div>
+<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> › <a href="/cgi-bin/koha/tools/import_borrowers.pl">Import Patrons</a><!-- TMPL_IF name="uploadborrowers" --> › Results<!-- /TMPL_IF --></div>
<div id="doc3" class="yui-t2">
@@ -17,15 +21,37 @@
<div class="yui-u first">
<h1>Import Patrons</h1>
<!-- TMPL_IF name="uploadborrowers" -->
-<p>Import results :</p>
+<h5>Import results :</h5>
<ul>
- <li><!-- TMPL_VAR name="imported" --> imported records</li>
- <li><!-- TMPL_VAR name="overwritten" --> overwritten</li>
- <li><!-- TMPL_VAR name="alreadyindb" --> not imported because already in borrowers table and overwrite disabled</li>
- <li><!-- TMPL_VAR name="invalid" --> not imported because they are not in the expected format</li>
- <li><!-- TMPL_VAR name="total" --> records parsed</li>
- <li><a href="/cgi-bin/koha/tools/tools-home.pl">Back</a></li>
+ <li><!-- TMPL_VAR name="imported" --> imported records <!-- TMPL_IF name="lastimported" -->(last was <!-- TMPL_VAR name="lastimported" -->)<!-- /TMPL_IF --></li>
+ <li><!-- TMPL_VAR name="overwritten" --> overwritten <!-- TMPL_IF name="lastoverwritten" -->(last was <!-- TMPL_VAR name="lastoverwritten" -->)<!-- /TMPL_IF --></li>
+ <li><!-- TMPL_VAR name="alreadyindb" --> not imported because already in borrowers table and overwrite disabled <!-- TMPL_IF name="lastalreadyindb" -->(last was <!-- TMPL_VAR name="lastalreadyindb" -->)<!-- /TMPL_IF --></li>
+ <li><!-- TMPL_VAR name="invalid" --> not imported because they are not in the expected format <!-- TMPL_IF name="lastinvalid" -->(last was <!-- TMPL_VAR name="lastinvalid" -->)<!-- /TMPL_IF --></li>
+ <li><!-- TMPL_VAR name="total" --> records parsed</li>
+ <li><a href="/cgi-bin/koha/tools/tools-home.pl">Back to Tools</a></li>
</ul>
+ <!-- TMPL_IF NAME="ERRORS" -->
+ <br /><br />
+ <div>
+ <h5>Error analysis:</h5>
+ <ul>
+ <!-- TMPL_LOOP NAME="ERRORS" -->
+ <!-- TMPL_IF NAME="badheader" --><li>Header row could not be parsed</li><!-- /TMPL_IF -->
+ <!-- TMPL_LOOP NAME="missing_criticals" -->
+ <li>
+ <!-- TMPL_IF NAME="badparse" -->
+ Line <span class="linenumber"><!-- TMPL_VAR NAME="line" --></span> could not be parsed!
+ <!-- TMPL_ELSE -->
+ Critical field "<!-- TMPL_VAR NAME="key" -->" missing on line <span class="linenumber"><!-- TMPL_VAR NAME="line" --></span>
+ (borrowernumber: <!-- TMPL_VAR NAME="borrowernumber" -->; surname: <!-- TMPL_VAR NAME="surname" -->).
+ <!-- /TMPL_IF -->
+ <br /><code><!-- TMPL_VAR NAME="lineraw" --></code>
+ </li>
+ <!-- /TMPL_LOOP -->
+ <!-- /TMPL_LOOP -->
+ </ul>
+ </div>
+ <!-- /TMPL_IF -->
<!-- TMPL_ELSE -->
<ul>
<li>Select a file to import into the borrowers table</li>
@@ -53,42 +79,43 @@
</li>
</ol>
</fieldset>
+<fieldset class="rows">
+<legend>Default values</legend>
+<ol>
+ <!-- TMPL_LOOP NAME="columnkeys" -->
+ <li>
+ <label class="widelabel" for="<!-- TMPL_VAR NAME="key" -->"><!-- TMPL_VAR NAME="key" --></label>
+ <input id="<!-- TMPL_VAR NAME="key" -->" name="<!-- TMPL_VAR NAME="key" -->" />
+ </li>
+ <!-- /TMPL_LOOP -->
+</ol></fieldset>
<fieldset class="rows">
<legend>If matching record is already in the borrowers table:</legend><ol><li class="radio">
<input type="radio" id="overwrite_cardnumberno" name="overwrite_cardnumber" value="0" checked="checked" /><label for="overwrite_cardnumberno">Ignore this one, keep the existing one</label></li>
<li class="radio">
<input type="radio" id="overwrite_cardnumberyes" name="overwrite_cardnumber" value="1" /><label for="overwrite_cardnumberyes">Overwrite the existing one with this</label>
- </li></ol></fieldset><fieldset class="action"><input type="submit" value="Import" /></fieldset>
+ </li></ol></fieldset>
+ <fieldset class="action"><input type="submit" value="Import" /></fieldset>
</form>
<!-- /TMPL_IF -->
</div>
<div class="yui-u">
<h2>Notes:</h2>
<ul>
-<li>Format the file in CSV format with the following fields:</li>
-<li>
- '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'<!-- TMPL_IF NAME="ExtendedPatronAttributes" -->, 'patron_attributes'<!-- /TMPL_IF -->
-</li>
+<li><b>Download a starter CSV file with all the columns <a href="?sample=1">here</a>.</b> Values are comma-separated.</li>
+<li>OR format your file in CSV format with the following fields:</li>
+<ul><li>
+ <!-- TMPL_LOOP name="columnkeys" -->'<!-- TMPL_VAR name="key" -->', <!-- /TMPL_LOOP -->
+</li></ul>
<!-- TMPL_IF NAME="ExtendedPatronAttributes" -->
<li>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.
<li>
<!-- /TMPL_IF -->
-<li>Please make sure the 'branchcode' and 'categorycode' are valid entries in your database.</li>
-<li>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).</li>
+<li>The fields 'branchcode' and 'categorycode' are <b>required</b> and <b>must match</b> valid entries in your database.</li>
+<li>'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).</li>
</ul>
</div>
</div>
diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl
index a58e0da..d8c4068 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) {
--
1.5.5.GIT
More information about the Koha-patches
mailing list