Bugzilla – Attachment 25004 Details for
Bug 5771
Better error handling for import borrowers - Provide a download file of errors
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 5771 : Adds a download link for big error reports to import_borrowers
Bug-5771--Adds-a-download-link-for-big-error-repor.patch (text/plain), 18.83 KB, created by
Jonathan Druart
on 2014-02-03 11:36:29 UTC
(
hide
)
Description:
Bug 5771 : Adds a download link for big error reports to import_borrowers
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2014-02-03 11:36:29 UTC
Size:
18.83 KB
patch
obsolete
>From f0da441b3ceb54107e840a633a0469ce197627c0 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@biblibre.com> >Date: Tue, 14 Dec 2010 22:58:19 +0100 >Subject: [PATCH] Bug 5771 : Adds a download link for big error reports to > import_borrowers > >If the import patrons tools produced a lot of errors(e.g. 1000), the >browser will crash or not be glad. > >This patch allow to download an error file for report producing at least >25 errors. > >Test plan: >Import patrons with at least 25 errors and check that a "Download >report" button appears and produce a text file to download. >--- > .../prog/en/modules/tools/import_borrowers.tt | 91 +++++++------ > tools/import_borrowers.pl | 137 +++++++++++++++----- > 2 files changed, 154 insertions(+), 74 deletions(-) > >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt >index 6744faf..6fb7308 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt >@@ -23,12 +23,12 @@ > [% IF ( uploadborrowers ) %] > <h5>Import results :</h5> > <ul> >- <li>[% imported %] imported records [% IF ( lastimported ) %](last was [% lastimported %])[% END %]</li> >- <li>[% overwritten %] overwritten [% IF ( lastoverwritten ) %](last was [% lastoverwritten %])[% END %]</li> >- <li>[% alreadyindb %] not imported because already in borrowers table and overwrite disabled [% IF ( lastalreadyindb ) %](last was [% lastalreadyindb %])[% END %]</li> >- <li>[% invalid %] not imported because they are not in the expected format [% IF ( lastinvalid ) %](last was [% lastinvalid %])[% END %]</li> >- <li>[% total %] records parsed</li> >- <li><a href="/cgi-bin/koha/tools/tools-home.pl">Back to Tools</a></li> >+ <li>[% imported %] imported records [% IF ( lastimported ) %](last was [% lastimported %])[% END %]</li> >+ <li>[% overwritten %] overwritten [% IF ( lastoverwritten ) %](last was [% lastoverwritten %])[% END %]</li> >+ <li>[% alreadyindb %] not imported because already in borrowers table and overwrite disabled [% IF ( lastalreadyindb ) %](last was [% lastalreadyindb %])[% END %]</li> >+ <li>[% invalid %] not imported because they are not in the expected format [% IF ( lastinvalid ) %](last was [% lastinvalid %])[% END %]</li> >+ <li>[% total %] records parsed</li> >+ <li><a href="/cgi-bin/koha/tools/tools-home.pl">Back to Tools</a></li> > </ul> > [% IF ( FEEDBACK ) %] > <br /><br /> >@@ -48,47 +48,54 @@ > </div> > [% END %] > [% IF ( ERRORS ) %] >- <br /><br /> > <div> > <h5>Error analysis:</h5> >- <ul> >- [% FOREACH ERROR IN ERRORS %] >+ [% IF download_errors %] >+ <form method="post"> >+ <input type="hidden" name="errors_filename" value="[% errors_filename %]" /> >+ <input type="hidden" name="report" value="report" /> >+ <input type="submit" value="Download report" /> >+ </form> >+ [% ELSE %] >+ <ul> >+ [% FOREACH ERROR IN ERRORS %] > [% IF ( ERROR.badheader ) %]<li>Header row could not be parsed</li>[% END %] >- [% FOREACH missing_critical IN ERROR.missing_criticals %] >- <li class="line_error"> >- Line <span class="linenumber">[% missing_critical.line %]</span> >- [% IF ( missing_critical.badparse ) %] >- could not be parsed! >- [% ELSIF ( missing_critical.bad_date ) %] >- has "[% missing_critical.key %]" in unrecognized format: "[% missing_critical.value %]" >- [% ELSE %] >- Critical field "[% missing_critical.key %]" >- [% IF ( missing_critical.branch_map ) %]has unrecognized value "[% missing_critical.value %]" >- [% ELSIF ( missing_critical.category_map ) %]has unrecognized value "[% missing_critical.value %]" >- [% ELSE %]missing >- [% END %] >- (borrowernumber: [% missing_critical.borrowernumber %]; surname: [% missing_critical.surname %]). >- [% END %] >- <br /><code>[% missing_critical.lineraw %]</code> >- </li> >+ [% FOREACH missing_critical IN ERROR.missing_criticals %] >+ <li class="line_error"> >+ Line <span class="linenumber">[% missing_critical.line %]</span> >+ [% IF ( missing_critical.badparse ) %] >+ could not be parsed! >+ [% ELSIF ( missing_critical.bad_date ) %] >+ has "[% missing_critical.key %]" in unrecognized format: "[% missing_critical.value %]" >+ [% ELSE %] >+ Critical field "[% missing_critical.key %]" >+ [% IF ( missing_critical.branch_map ) %]has unrecognized value "[% missing_critical.value %]" >+ [% ELSIF ( missing_critical.category_map ) %]has unrecognized value "[% missing_critical.value %]" >+ [% ELSE %]missing >+ [% END %] >+ (borrowernumber: [% missing_critical.borrowernumber %]; surname: [% missing_critical.surname %]). >+ [% END %] >+ <br /><code>[% missing_critical.lineraw %]</code> >+ </li> > [% END %] >- [% END %] >+ [% END %] > </ul> >+ [% END %] > </div> > [% END %] > [% ELSE %] > <ul> >- <li>Select a file to import into the borrowers table</li> >- <li>If a cardnumber exists in the table, you can choose whether to ignore the new one or overwrite the old one.</li> >+ <li>Select a file to import into the borrowers table</li> >+ <li>If a cardnumber exists in the table, you can choose whether to ignore the new one or overwrite the old one.</li> > </ul> > <form method="post" action="[% SCRIPT_NAME %]" enctype="multipart/form-data"> > <fieldset class="rows"> > <legend>Import into the borrowers table</legend> > <ol> >- <li> >- <label for="uploadborrowers">Select the file to import: </label> >- <input type="file" id="uploadborrowers" name="uploadborrowers" /> >- </li> >+ <li> >+ <label for="uploadborrowers">Select the file to import: </label> >+ <input type="file" id="uploadborrowers" name="uploadborrowers" /> >+ </li> > </ol></fieldset> > <fieldset class="rows"> > <legend>Field to use for record matching</legend> >@@ -107,25 +114,25 @@ > <legend>Default values</legend> > <ol> > [% FOREACH columnkey IN columnkeys %] >- <li> >+ <li> > <label class="widelabel" for="[% columnkey.key %]">[% columnkey.key %]: </label> >- <input id="[% columnkey.key %]" name="[% columnkey.key %]" /> >- </li> >+ <input id="[% columnkey.key %]" name="[% columnkey.key %]" /> >+ </li> > [% END %] > </ol></fieldset> >- <fieldset class="rows"> >- <legend>If matching record is already in the borrowers table:</legend> >+ <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> >+ <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> > [% IF ( ExtendedPatronAttributes ) %] >- <fieldset class="rows"> >- <legend>Patron Attributes</legend> >+ <fieldset class="rows"> >+ <legend>Patron Attributes</legend> > <ol><li class="radio"> > <input type="radio" id="ext_preserve_0" name="ext_preserve" value="0" checked="checked" /><label for="ext_preserve_0">Replace all Patron Attributes</label> > </li> >@@ -135,7 +142,7 @@ > </ol> > </fieldset> > [% END %] >- <fieldset class="action"><input type="submit" value="Import" /></fieldset> >+ <fieldset class="action"><input type="submit" value="Import" /></fieldset> > </form> > [% END %] > </div> >diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl >index 5f65c97..abf85ec 100755 >--- a/tools/import_borrowers.pl >+++ b/tools/import_borrowers.pl >@@ -1,7 +1,7 @@ > #!/usr/bin/perl > > # Copyright 2007 Liblime >-# Parts copyright 2010 BibLibre >+# Parts copyright 2013 BibLibre > # > # This file is part of Koha. > # >@@ -34,8 +34,7 @@ > # dates should be in the format you have set up Koha to expect > # branchcode and categorycode need to be valid > >-use strict; >-use warnings; >+use Modern::Perl; > > use C4::Auth; > use C4::Output; >@@ -46,8 +45,12 @@ use C4::Members; > use C4::Members::Attributes qw(:all); > use C4::Members::AttributeTypes; > use C4::Members::Messaging; >- >+use Koha::DateUtils; >+use Date::Calc qw(Today_and_Now); >+use Getopt::Long; >+use File::Temp; > use Text::CSV; >+ > # Text::CSV::Unicode, even in binary mode, fails to parse lines with these diacriticals: > # Ä > # Ä >@@ -75,10 +78,9 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user({ > 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' ? >@@ -88,6 +90,23 @@ if ($input->param('sample')) { > print $csv->string, "\n"; > exit 1; > } >+ >+if ($input->param('report')) { >+ print $input->header( >+ -type => 'text/plain', >+ -attachment => 'import_borrowers_report.txt' >+ ); >+ my $filename = $input->param('errors_filename'); >+ if ( -f $filename ) { >+ open my $fh, '<', $filename; >+ print <$fh>; >+ close $fh; >+ >+ unlink $filename; >+ exit 1; >+ } >+} >+ > my $uploadborrowers = $input->param('uploadborrowers'); > my $matchpoint = $input->param('matchpoint'); > if ($matchpoint) { >@@ -110,7 +129,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > my $alreadyindb = 0; > my $overwritten = 0; > my $invalid = 0; >- my $matchpoint_attr_type; >+ my $matchpoint_attr_type; > my %defaults = $input->Vars; > > # use header line to construct key to column map >@@ -121,11 +140,11 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > my %csvkeycol; > my $col = 0; > foreach my $keycol (@csvcolumns) { >- # columnkeys don't contain whitespace, but some stupid tools add it >- $keycol =~ s/ +//g; >+ # columnkeys don't contain whitespace, but some stupid tools add it >+ $keycol =~ s/ +//g; > $csvkeycol{$keycol} = $col++; > } >- #warn($borrowerline); >+ > my $ext_preserve = $input->param('ext_preserve') || 0; > if ($extended) { > $matchpoint_attr_type = C4::Members::AttributeTypes->fetch($matchpoint); >@@ -137,6 +156,8 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > my @bad_dates; # I've had a few. > my $date_re = C4::Dates->new->regexp('syspref'); > my $iso_re = C4::Dates->new->regexp('iso'); >+ my $lastalreadyindb; >+ my $lastinvalid; > LINE: while ( my $borrowerline = <$handle> ) { > my %borrower; > my @missing_criticals; >@@ -152,23 +173,23 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > if ($borrower{$key} !~ /\S/) { > $borrower{$key} = $defaults{$key}; > } >- } >+ } > } 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} = ''; >- } >+ 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} = ''; >+ } > } > } >- #warn join(':',%borrower); >+ > if ($borrower{categorycode}) { > push @missing_criticals, {key=>'categorycode', line=>$. , lineraw=>$borrowerline, value=>$borrower{categorycode}, category_map=>1} > unless GetBorrowercategory($borrower{categorycode}); >@@ -187,8 +208,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > $_->{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. >+ push @errors, {missing_criticals=>\@missing_criticals}; > next LINE; > } > if ($extended) { >@@ -197,9 +217,9 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > $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); >+ $patron_attributes = extended_attributes_code_value_arrayref($attr_str); > } >- # Popular spreadsheet applications make it difficult to force date outputs to be zero-padded, but we require it. >+ # Popular spreadsheet applications make it difficult to force date outputs to be zero-padded, but we require it. > foreach (qw(dateofbirth dateenrolled dateexpiry)) { > my $tempdate = $borrower{$_} or next; > if ($tempdate =~ /$date_re/) { >@@ -211,8 +231,8 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > push @missing_criticals, {key=>$_, line=>$. , lineraw=>$borrowerline, bad_date=>1}; > } > } >- $borrower{dateenrolled} = $today_iso unless $borrower{dateenrolled}; >- $borrower{dateexpiry} = GetExpiryDate($borrower{categorycode},$borrower{dateenrolled}) unless $borrower{dateexpiry}; >+ $borrower{dateenrolled} = $today_iso unless $borrower{dateenrolled}; >+ $borrower{dateexpiry} = GetExpiryDate($borrower{categorycode},$borrower{dateenrolled}) unless $borrower{dateexpiry}; > my $borrowernumber; > my $member; > if ( ($matchpoint eq 'cardnumber') && ($borrower{'cardnumber'}) ) { >@@ -231,12 +251,13 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > } > } > } >- >+ > if ($borrowernumber) { > # borrower exists > unless ($overwrite_cardnumber) { > $alreadyindb++; >- $template->param('lastalreadyindb'=>$borrower{'surname'}.' / '.$borrowernumber); >+ $lastalreadyindb = $borrower{'surname'}.' / '.$borrowernumber; >+ $template->param('lastalreadyindb'=>$lastalreadyindb); > next LINE; > } > $borrower{'borrowernumber'} = $borrowernumber; >@@ -255,7 +276,8 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > $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); >+ $lastinvalid = $borrower{'surname'}.' / '.$borrowernumber; >+ $template->param('lastinvalid'=>$lastinvalid); > next LINE; > } > if ($extended) { >@@ -301,6 +323,58 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > 'total' => $imported + $alreadyindb + $invalid + $overwritten, > ); > >+ my $max_errors = 25; >+ if (scalar(@errors) > $max_errors ) { >+ >+ my $total = $imported + $alreadyindb + $invalid + $overwritten; >+ my $output; >+ >+ my $timestamp = output_pref( dt_from_string ); >+ $output .= "Timestamp : $timestamp\n"; >+ $output .= "Import results\n"; >+ $output .= "$imported imported records\n"; >+ $output .= "$overwritten overwritten records\n"; >+ $output .= "$alreadyindb not imported because already in borrowers table and overwrite disabled\n"; >+ $output .= "(last was $lastalreadyindb)\n" if ($lastalreadyindb); >+ $output .= "$invalid not imported because they are not in the expected format\n"; >+ $output .= "(last was $lastinvalid)\n" if ($lastinvalid); >+ $output .= "$total records parsed\n"; >+ >+ >+ $output .= "\nError analysis\n"; >+ foreach my $error (@errors) { >+ $output .= "Header row could not be parsed" if ($error->{'badheader'}); >+ foreach my $array ($error->{'missing_criticals'}) { >+ foreach (@$array) { >+ $output .= "Line $_->{'line'}: "; >+ if ($error->{'badparse'}) { >+ $output .= "could not be parsed!"; >+ } elsif ($error->{'bad_date'}) { >+ $output .= "has $_->{'key'} in unrecognized format: $_->{'value'} "; >+ } else { >+ $output .= "Critical field $_->{'key'}: "; >+ if ($_->{'branch_map'} || $_->{'category_map'}) { >+ $output .= "has unrecognized value: $_->{'value'}"; >+ } else { >+ $output .= " missing"; >+ } >+ $output .= " (borrowernumber: $_->{'borrowernumber'}; surname: $_->{'surname'})"; >+ } >+ $output .= "\n"; >+ } >+ } >+ } >+ >+ my $tmpf = File::Temp->new(UNLINK => 0); >+ print $tmpf $output; >+ $template->param( >+ download_errors => 1, >+ errors_filename => $tmpf->filename >+ ); >+ close $tmpf; >+ >+ } >+ > } else { > if ($extended) { > my @matchpoints = (); >@@ -316,4 +390,3 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { > } > > output_html_with_http_headers $input, $cookie, $template->output; >- >-- >1.7.10.4
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=25004">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 5771
:
3177
|
4589
|
10023
|
19263
|
24858
|
25004
|
25019
|
26204
|
27278
|
27405