From 9af6c4e34bc41616c03bb786201a9c10ebf13dab Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 10 Aug 2017 14:44:50 -0300 Subject: [PATCH] Bug 19080: Handle non-existing patrons gratefully This is a recurrent bug we have over the last years. When a script is called with non-existent borrowernumber it will crashes. We need to handle this gracefully instead of letting the script crashes. On bug 18403 a new subroutine is added to the codebase (output_and_exit_if_error) to handle this kind of errors correctly. Since it is not pushed yet, I propose to just redirect to a script that handle it correctly (circulation.pl) instead of adding this message to all these scripts. Test plan: Hit different scripts from the members module and pass a non-existent borrowernumber. You must be redirected to circulation.pl with a friendly message. Signed-off-by: Josef Moravec Signed-off-by: Julian Maurice Signed-off-by: Jonathan Druart --- circ/circulation.pl | 8 +- .../prog/en/modules/members/statistics.tt | 8 +- members/boraccount.pl | 4 + members/deletemem.pl | 4 + members/discharge.pl | 143 +++++++++--------- members/files.pl | 5 + members/mancredit.pl | 4 + members/maninvoice.pl | 5 + members/member-flags.pl | 5 + members/member-password.pl | 5 + members/memberentry.pl | 5 + members/notices.pl | 4 + members/pay.pl | 4 + members/paycollect.pl | 4 + members/printfeercpt.pl | 4 + members/printinvoice.pl | 4 + members/purchase-suggestions.pl | 5 +- members/readingrec.pl | 10 +- members/routing-lists.pl | 57 +++---- members/statistics.pl | 3 +- members/summary-print.pl | 4 + members/update-child.pl | 4 + tools/viewlog.pl | 4 + 23 files changed, 188 insertions(+), 115 deletions(-) diff --git a/circ/circulation.pl b/circ/circulation.pl index 6fd4006717..130d350288 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -261,8 +261,8 @@ if ($findborrower) { } # get the borrower information..... -if ($borrowernumber) { - $patron = Koha::Patrons->find( $borrowernumber ); +$patron ||= Koha::Patrons->find( $borrowernumber ) if $borrowernumber; +if ($patron) { my $overdues = $patron->get_overdues; my $issues = $patron->checkouts; my $balance = $patron->account->balance; @@ -443,8 +443,8 @@ if (@$barcodes) { ################################################################################## # BUILD HTML # show all reserves of this borrower, and the position of the reservation .... -if ($borrowernumber) { - my $holds = Koha::Holds->search( { borrowernumber => $borrowernumber } ); +if ($patron) { + my $holds = Koha::Holds->search( { borrowernumber => $borrowernumber } ); # FIXME must be Koha::Patron->holds my $waiting_holds = $holds->waiting; $template->param( holds_count => $holds->count(), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/statistics.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/statistics.tt index ae5c276dc0..7397339eb1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/statistics.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/statistics.tt @@ -3,11 +3,7 @@ [% USE Branches %] [% INCLUDE 'doc-head-open.inc' %] Koha › Patrons › -[% IF ( unknowuser ) %] - Patron does not exist -[% ELSE %] - Statistics for [% INCLUDE 'patron-title.inc' %] -[% END %] +Statistics for [% INCLUDE 'patron-title.inc' %] [% INCLUDE 'doc-head-close.inc' %] @@ -30,7 +26,7 @@
diff --git a/members/boraccount.pl b/members/boraccount.pl index 8abb373f2e..0cc9487f39 100755 --- a/members/boraccount.pl +++ b/members/boraccount.pl @@ -54,6 +54,10 @@ my $action = $input->param('action') || ''; #get patron details my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} if ( $action eq 'reverse' ) { ReversePayment( $input->param('accountlines_id') ); diff --git a/members/deletemem.pl b/members/deletemem.pl index 5ea015ada7..b03b5c8d90 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -75,6 +75,10 @@ my $issues = GetPendingIssues($member); # FIXME: wasteful call when really, my $countissues = scalar(@$issues); my $patron = Koha::Patrons->find( $member ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $flags = C4::Members::patronflags( $patron->unblessed ); my $userenv = C4::Context->userenv; diff --git a/members/discharge.pl b/members/discharge.pl index b24c41b1db..a0af828363 100755 --- a/members/discharge.pl +++ b/members/discharge.pl @@ -58,85 +58,86 @@ unless ( C4::Context->preference('useDischarge') ) { exit; } -if ( $input->param('borrowernumber') ) { - $borrowernumber = $input->param('borrowernumber'); +$borrowernumber = $input->param('borrowernumber'); - # Getting member data - my $patron = Koha::Patrons->find( $borrowernumber ); +my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} - my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged({ - borrowernumber => $borrowernumber - }); +my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged({ + borrowernumber => $borrowernumber +}); - my $holds = $patron->holds; - my $has_reserves = $holds->count; +my $holds = $patron->holds; +my $has_reserves = $holds->count; - # Generating discharge if needed - if ( $input->param('discharge') and $can_be_discharged ) { - my $is_discharged = Koha::Patron::Discharge::is_discharged({ - borrowernumber => $borrowernumber, +# Generating discharge if needed +if ( $input->param('discharge') and $can_be_discharged ) { + my $is_discharged = Koha::Patron::Discharge::is_discharged({ + borrowernumber => $borrowernumber, + }); + unless ($is_discharged) { + Koha::Patron::Discharge::discharge({ + borrowernumber => $borrowernumber }); - unless ($is_discharged) { - Koha::Patron::Discharge::discharge({ - borrowernumber => $borrowernumber - }); - } - eval { - my $pdf_path = Koha::Patron::Discharge::generate_as_pdf( - { borrowernumber => $borrowernumber, branchcode => $patron->branchcode } ); - - binmode(STDOUT); - print $input->header( - -type => 'application/pdf', - -charset => 'utf-8', - -attachment => "discharge_$borrowernumber.pdf", - ); - open my $fh, '<', $pdf_path; - my @lines = <$fh>; - close $fh; - print @lines; - exit; - }; - if ( $@ ) { - carp $@; - $template->param( messages => [ {type => 'error', code => 'unable_to_generate_pdf'} ] ); - } } + eval { + my $pdf_path = Koha::Patron::Discharge::generate_as_pdf( + { borrowernumber => $borrowernumber, branchcode => $patron->branchcode } ); + + binmode(STDOUT); + print $input->header( + -type => 'application/pdf', + -charset => 'utf-8', + -attachment => "discharge_$borrowernumber.pdf", + ); + open my $fh, '<', $pdf_path; + my @lines = <$fh>; + close $fh; + print @lines; + exit; + }; + if ( $@ ) { + carp $@; + $template->param( messages => [ {type => 'error', code => 'unable_to_generate_pdf'} ] ); + } +} - # Already generated discharges - my $validated_discharges = Koha::Patron::Discharge::get_validated({ - borrowernumber => $borrowernumber, - }); +# Already generated discharges +my $validated_discharges = Koha::Patron::Discharge::get_validated({ + borrowernumber => $borrowernumber, +}); - $template->param( picture => 1 ) if $patron->image; - - $template->param( - # FIXME The patron object should be passed to the template - borrowernumber => $borrowernumber, - title => $patron->title, - initials => $patron->initials, - surname => $patron->surname, - borrowernumber => $borrowernumber, - firstname => $patron->firstname, - cardnumber => $patron->cardnumber, - categorycode => $patron->categorycode, - category_type => $patron->category->category_type, - categoryname => $patron->category->description, - address => $patron->address, - streetnumber => $patron->streetnumber, - streettype => $patron->streettype, - address2 => $patron->address2, - city => $patron->city, - zipcode => $patron->zipcode, - country => $patron->country, - phone => $patron->phone, - email => $patron->email, - branchcode => $patron->branchcode, - has_reserves => $has_reserves, - can_be_discharged => $can_be_discharged, - validated_discharges => $validated_discharges, - ); -} +$template->param( picture => 1 ) if $patron->image; + +$template->param( + # FIXME The patron object should be passed to the template + borrowernumber => $borrowernumber, + title => $patron->title, + initials => $patron->initials, + surname => $patron->surname, + borrowernumber => $borrowernumber, + firstname => $patron->firstname, + cardnumber => $patron->cardnumber, + categorycode => $patron->categorycode, + category_type => $patron->category->category_type, + categoryname => $patron->category->description, + address => $patron->address, + streetnumber => $patron->streetnumber, + streettype => $patron->streettype, + address2 => $patron->address2, + city => $patron->city, + zipcode => $patron->zipcode, + country => $patron->country, + phone => $patron->phone, + email => $patron->email, + branchcode => $patron->branchcode, + has_reserves => $has_reserves, + can_be_discharged => $can_be_discharged, + validated_discharges => $validated_discharges, +); $template->param( dischargeview => 1, ); diff --git a/members/files.pl b/members/files.pl index 0070fcf071..8cc25a918d 100755 --- a/members/files.pl +++ b/members/files.pl @@ -64,6 +64,11 @@ if ( $op eq 'download' ) { } else { my $patron = Koha::Patrons->find( $borrowernumber ); + unless ( $patron ) { + print $cgi->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; + } + my $patron_category = $patron->category; $template->param(%{ $patron->unblessed}); diff --git a/members/mancredit.pl b/members/mancredit.pl index 4855c24cde..5a93e956be 100755 --- a/members/mancredit.pl +++ b/members/mancredit.pl @@ -43,6 +43,10 @@ my $flagsrequired = { borrowers => 1, updatecharges => 1 }; my $borrowernumber=$input->param('borrowernumber'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $add=$input->param('add'); if ($add){ diff --git a/members/maninvoice.pl b/members/maninvoice.pl index 2c1c05f01f..bd24a10fd8 100755 --- a/members/maninvoice.pl +++ b/members/maninvoice.pl @@ -43,6 +43,11 @@ my $flagsrequired = { borrowers => 1 }; my $borrowernumber=$input->param('borrowernumber'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} + my $add=$input->param('add'); if ($add){ if ( checkauth( $input, 0, $flagsrequired, 'intranet' ) ) { diff --git a/members/member-flags.pl b/members/member-flags.pl index e7c24e9e04..158074a4fc 100755 --- a/members/member-flags.pl +++ b/members/member-flags.pl @@ -27,6 +27,11 @@ my $input = new CGI; my $flagsrequired = { permissions => 1 }; my $member=$input->param('member'); my $patron = Koha::Patrons->find( $member ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$member"); + exit; +} + my $category_type = $patron->category->category_type; my $bor = $patron->unblessed; if( $category_type eq 'S' ) { diff --git a/members/member-password.pl b/members/member-password.pl index 4cf00bfde4..cb922da43c 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -49,6 +49,11 @@ my $newpassword2 = $input->param('newpassword2'); my @errors; my $patron = Koha::Patrons->find( $member ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$member"); + exit; +} + my $category_type = $patron->category->category_type; my $bor = $patron->unblessed; diff --git a/members/memberentry.pl b/members/memberentry.pl index 050153442c..c58b2a9bff 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -155,6 +155,11 @@ $template->param( "duplicate" => 1 ) if ( $op eq 'duplicate' ); $template->param( "checked" => 1 ) if ( defined($nodouble) && $nodouble eq 1 ); if ( $op eq 'modify' or $op eq 'save' or $op eq 'duplicate' ) { my $patron = Koha::Patrons->find( $borrowernumber ); + unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; + } + $borrower_data = $patron->unblessed; $borrower_data->{category_type} = $patron->category->category_type; } diff --git a/members/notices.pl b/members/notices.pl index 977c921e89..9f0ab11623 100755 --- a/members/notices.pl +++ b/members/notices.pl @@ -34,6 +34,10 @@ my $input=new CGI; my $borrowernumber = $input->param('borrowernumber'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $borrower = $patron->unblessed; my ($template, $loggedinuser, $cookie) diff --git a/members/pay.pl b/members/pay.pl index e4d0a955d3..7c3f6ad8ae 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -68,6 +68,10 @@ if ( !$borrowernumber ) { # get borrower details my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $category = $patron->category; our $borrower = $patron->unblessed; $borrower->{description} = $category->description; diff --git a/members/paycollect.pl b/members/paycollect.pl index 3b8b9228a5..3576d10315 100755 --- a/members/paycollect.pl +++ b/members/paycollect.pl @@ -50,6 +50,10 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( # get borrower details my $borrowernumber = $input->param('borrowernumber'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $borrower = $patron->unblessed; my $category = $patron->category; $borrower->{description} = $category->description; diff --git a/members/printfeercpt.pl b/members/printfeercpt.pl index 8863d1c373..f5933f16ab 100755 --- a/members/printfeercpt.pl +++ b/members/printfeercpt.pl @@ -51,6 +51,10 @@ my $action = $input->param('action') || ''; my $accountlines_id = $input->param('accountlines_id'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $category = $patron->category; my $data = $patron->unblessed; $data->{description} = $category->description; diff --git a/members/printinvoice.pl b/members/printinvoice.pl index ad823d5005..ff2c0d1ed6 100755 --- a/members/printinvoice.pl +++ b/members/printinvoice.pl @@ -50,6 +50,10 @@ my $action = $input->param('action') || ''; my $accountlines_id = $input->param('accountlines_id'); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $category = $patron->category; my $data = $patron->unblessed; $data->{description} = $category->description; diff --git a/members/purchase-suggestions.pl b/members/purchase-suggestions.pl index 9e352590e1..5acc3cc2e6 100755 --- a/members/purchase-suggestions.pl +++ b/members/purchase-suggestions.pl @@ -42,8 +42,11 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( my $borrowernumber = $input->param('borrowernumber'); -# Set informations for the patron my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $category = $patron->category; my $data = $patron->unblessed; $data->{description} = $category->description; diff --git a/members/readingrec.pl b/members/readingrec.pl index b0b563c816..ba5e17497d 100755 --- a/members/readingrec.pl +++ b/members/readingrec.pl @@ -55,15 +55,19 @@ my $patron; if ($input->param('cardnumber')) { $cardnumber = $input->param('cardnumber'); $patron = Koha::Patrons->find( { cardnumber => $cardnumber } ); - $data = $patron->unblessed; - $borrowernumber = $data->{'borrowernumber'}; # we must define this as it is used to retrieve other data about the patron } if ($input->param('borrowernumber')) { $borrowernumber = $input->param('borrowernumber'); $patron = Koha::Patrons->find( $borrowernumber ); - $data = $patron->unblessed; } +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} +$data = $patron->unblessed; +$borrowernumber = $patron->borrowernumber; + my $order = 'date_due desc'; my $limit = 0; my $issues = (); diff --git a/members/routing-lists.pl b/members/routing-lists.pl index 6b024bdc16..5726a20ea3 100755 --- a/members/routing-lists.pl +++ b/members/routing-lists.pl @@ -48,37 +48,38 @@ my $borrowernumber = $query->param('borrowernumber'); my $branch = C4::Context->userenv->{'branch'}; -# get the borrower information..... -my ( $patron, $patron_info ); -if ($borrowernumber) { - $patron = Koha::Patrons->find( $borrowernumber ); - my $category = $patron->category; - $patron_info = $patron->unblessed; - $patron_info->{description} = $category->description; - $patron_info->{category_type} = $category->category_type; - - my $count; - my @borrowerSubscriptions; - ($count, @borrowerSubscriptions) = GetSubscriptionsFromBorrower($borrowernumber ); - my @subscripLoop; - - foreach my $num_res (@borrowerSubscriptions) { - my %getSubscrip; - $getSubscrip{subscriptionid} = $num_res->{'subscriptionid'}; - $getSubscrip{title} = $num_res->{'title'}; - $getSubscrip{borrowernumber} = $num_res->{'borrowernumber'}; - push( @subscripLoop, \%getSubscrip ); - } - - $template->param( - countSubscrip => scalar @subscripLoop, - subscripLoop => \@subscripLoop, - routinglistview => 1 - ); +my $patron = Koha::Patrons->find( $borrowernumber ) if $borrowernumber; +unless ( $patron ) { + print $query->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} - $template->param( adultborrower => 1 ) if ( $patron_info->{category_type} =~ /^(A|I)$/ ); +my $category = $patron->category; +my $patron_info = $patron->unblessed; +$patron_info->{description} = $category->description; +$patron_info->{category_type} = $category->category_type; + +my $count; +my @borrowerSubscriptions; +($count, @borrowerSubscriptions) = GetSubscriptionsFromBorrower($borrowernumber ); +my @subscripLoop; + +foreach my $num_res (@borrowerSubscriptions) { + my %getSubscrip; + $getSubscrip{subscriptionid} = $num_res->{'subscriptionid'}; + $getSubscrip{title} = $num_res->{'title'}; + $getSubscrip{borrowernumber} = $num_res->{'borrowernumber'}; + push( @subscripLoop, \%getSubscrip ); } +$template->param( + countSubscrip => scalar @subscripLoop, + subscripLoop => \@subscripLoop, + routinglistview => 1 +); + +$template->param( adultborrower => 1 ) if ( $patron_info->{category_type} =~ /^(A|I)$/ ); + ################################################################################## $template->param(%$patron_info); diff --git a/members/statistics.pl b/members/statistics.pl index 5979e7f1dd..55d5ed02b5 100755 --- a/members/statistics.pl +++ b/members/statistics.pl @@ -50,8 +50,7 @@ my $borrowernumber = $input->param('borrowernumber'); # Set informations for the patron my $patron = Koha::Patrons->find( $borrowernumber ); unless ( $patron ) { - $template->param (unknowuser => 1); - output_html_with_http_headers $input, $cookie, $template->output; + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); exit; } diff --git a/members/summary-print.pl b/members/summary-print.pl index 546e93121b..a6a0e36194 100755 --- a/members/summary-print.pl +++ b/members/summary-print.pl @@ -44,6 +44,10 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( ); my $patron = Koha::Patrons->find( $borrowernumber ); +unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; +} my $category = $patron->category; my $data = $patron->unblessed; $data->{description} = $category->description; diff --git a/members/update-child.pl b/members/update-child.pl index bedf9b5fb3..c14db87ea7 100755 --- a/members/update-child.pl +++ b/members/update-child.pl @@ -73,6 +73,10 @@ if ( $op eq 'multi' ) { elsif ( $op eq 'update' ) { my $patron = Koha::Patrons->find( $borrowernumber ); + unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; + } my $member = $patron->unblessed; $member->{'guarantorid'} = 0; $member->{'categorycode'} = $catcode; diff --git a/tools/viewlog.pl b/tools/viewlog.pl index 9af64d5fb6..00469ba845 100755 --- a/tools/viewlog.pl +++ b/tools/viewlog.pl @@ -74,6 +74,10 @@ if ( $src eq 'circ' ) { use C4::Members::Attributes qw(GetBorrowerAttributes); my $borrowernumber = $object; my $patron = Koha::Patrons->find( $borrowernumber ); + unless ( $patron ) { + print $input->redirect("/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber"); + exit; + } $template->param( picture => 1 ) if $patron->image; my $data = $patron->unblessed; -- 2.39.5