From 0d3225f95bc25a25088b176f4ad6930862396cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20V=C3=A9ron?= Date: Tue, 5 May 2015 16:24:10 +0200 Subject: [PATCH] Bug 14138 - Patroncard: POC for pre-testing parameters for create-pdf.pl If wrong parameters are given, create-pdf.pl displays a "Software error" or creates an invalid PDF (it is a text file containing "Software error...."). This patch is a proof of concept with the aim to prevent such a situation. It implements pre-testing the parameters provided for create-pdf.pl. If wrong parameters are given, a error message is displayed instead of creating the PDF. The user has then the possibility to go back to the page she came from. (Patron card tool, maybe later patron's detail page? ) To test: 1) Apply on top of Bug 14122 (otherwise it is not possible to test for borrower) 2) Test with links similar to the following ...cgi-bin/koha/patroncards/create-pdf.pl?borrower_number=61&template_id=24&layout_id=21&start_card=1 or: ...cgi-bin/koha/patroncards/create-pdf.pl?batch_id=8&template_id=24&layout_id=21&start_card=1 3) Change values for existing / not existing Ids. If a parameter is not valid, a new HTML page is displayed with a JavaScript alert. (This will need optimization. Ideas and/or patterns for doing this part are highly welcome). Comments are welcome - is this the way to go? Marc --- C4/Creators/Layout.pm | 1 + C4/Creators/Template.pm | 1 + patroncards/create-pdf.pl | 70 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/C4/Creators/Layout.pm b/C4/Creators/Layout.pm index a12532f..0a0eda4 100644 --- a/C4/Creators/Layout.pm +++ b/C4/Creators/Layout.pm @@ -108,6 +108,7 @@ sub retrieve { return -1; } my $self = $sth->fetchrow_hashref; + return -2 unless ( $self ); # Make it behave like C4::Patroncards::Batch->retrieve() bless ($self, $type); return $self; } diff --git a/C4/Creators/Template.pm b/C4/Creators/Template.pm index b0b7da0..d76120c 100644 --- a/C4/Creators/Template.pm +++ b/C4/Creators/Template.pm @@ -127,6 +127,7 @@ sub retrieve { return -1; } my $self = $sth->fetchrow_hashref; + return -2 unless ( $self ); # Make it behave like C4::Patroncards::Batch->retrieve() $self = _conv_points($self) if (($opts{convert} && $opts{convert} == 1) || $opts{profile_id}); $self = _apply_profile($self, $opts{'creator'}) if $opts{profile_id} && $self->{'profile_id'}; # don't bother if there is no profile_id $self->{'template_stat'} = 1; diff --git a/patroncards/create-pdf.pl b/patroncards/create-pdf.pl index 1df3e27..4b7631a 100755 --- a/patroncards/create-pdf.pl +++ b/patroncards/create-pdf.pl @@ -45,13 +45,76 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user({ }); -my $batch_id = $cgi->param('batch_id') if $cgi->param('batch_id'); +my $batch_id = $cgi->param('batch_id') || undef; my $template_id = $cgi->param('template_id') || undef; my $layout_id = $cgi->param('layout_id') || undef; -my $start_card = $cgi->param('start_card') || 1; +my $start_card = $cgi->param('start_card') || 1; my @label_ids = $cgi->param('label_id') if $cgi->param('label_id'); my @borrower_numbers = $cgi->param('borrower_number') if $cgi->param('borrower_number'); + +# Proof of concept pretesting see Bug 14138 +# Prepare for pre-tests + +my $batch_failed = ''; +my $patron_failed = ''; +my $template_failed = ''; +my $layout_failed = ''; + + +my $batch = C4::Patroncards::Batch->retrieve(batch_id => $batch_id) if ($batch_id); +# returns -1 or -2 if error occured +$batch_failed = $batch_id if ( ( $batch && $batch < 0 ) || ! $batch ); + +# Later add code to handle lists (Bug 14131) +# Later prevent that more than one of patron / batch / list is given + +if ( @borrower_numbers ) { + my $test_number = GetMember(borrowernumber => @borrower_numbers); + $patron_failed = @borrower_numbers unless ( $test_number ); +} + +my $pc_template; +my $template_failed = ''; +if ( $template_id ) { + $pc_template = C4::Patroncards::Template->retrieve(template_id => $template_id, profile_id => 1); + # returns -1 or -2 if error occured + $template_failed = $template_id if ( $pc_template && $pc_template < 0 ); +} else { + $template_failed = 'missing'; +} + +my $layout; +if ( $layout_id ) { + $layout = C4::Patroncards::Layout->retrieve(layout_id => $layout_id); + # returns -1 or -2 if error occured + $layout_failed = $layout_id if ( $layout && $layout < 0 ); +} else { + $layout_failed = 'missing'; +} + + +# Display the errors and go back to previous page +# Later: Fit better into Koha look and feel (Template?) / Translation issues (is hard coded) +if ( $batch_failed || $patron_failed || $template_failed || $layout_failed ) { + my $errmessage = ''; + $errmessage .= "ERROR "; + $errmessage .= "Template $template_failed " if ( $template_failed ); + $errmessage .= "Layout $layout_failed " if ( $layout_failed ); + $errmessage .= "Batch $batch_failed " if ( $batch_failed ); + $errmessage .= "Patron $patron_failed" if ( $patron_failed ); + + print $cgi->header( -type => 'text/html', -encoding => 'utf-8', ); + my $errtitle = 'Error preparing PDF document'; + my $errscript = "alert(\" $errmessage \"); window.history.back();"; + print $cgi-> start_html( -title => $errtitle, -onLoad => $errscript, ); + print $cgi->h1($errtitle); + print $cgi->end_html(); + exit 1; +} + + +# Do the work my $items = undef; # items = cards my $new_page = 0; @@ -62,9 +125,6 @@ print $cgi->header( -type => 'application/pdf', ); my $pdf = C4::Creators::PDF->new(InitVars => 0); -my $batch = C4::Patroncards::Batch->retrieve(batch_id => $batch_id); -my $pc_template = C4::Patroncards::Template->retrieve(template_id => $template_id, profile_id => 1); -my $layout = C4::Patroncards::Layout->retrieve(layout_id => $layout_id); $| = 1; -- 1.7.10.4