From 55fce59f71eec72ce133fbb0270443f17ec4238b Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 29 Apr 2020 14:38:41 +0200 Subject: [PATCH] Bug 25311: Better error handling when creating a patron This is still not ideal but brings a bit of enhancement. One possible problem is that the patron creation will fail if the streetnumber field is too long (borrowers.streetnumber is varchar(10). Test plan: 0. Don't apply this patch 1. Create a new patron with a streetnumber longer than 10 characters 2. Save => The patron has not been created and the app explodes The error is about extended_attributes and not meaningful Can't call method "extended_attributes" on an undefined value at /kohadevbox/koha/members/memberentry.pl line 560 3. Apply the patch 4. Repeat 1. and 2 => You get a warning on the interface and you still see the creation form 5. Check the logs => The error is meaningful "Data too long for column 'streetnumber'" Signed-off-by: Martin Renvoize --- .../prog/en/modules/members/memberentrygen.tt | 9 +++++ members/memberentry.pl | 38 +++++++++++-------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt index 4b5eb35d12..02f21e8e48 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -57,6 +57,15 @@ legend:hover {
+ [% IF messages %] + [% FOR message IN messages %] + [% SWITCH message.error %] + [% CASE 'error_on_insert_patron' %] +
Something went wrong when creating the patron. Check the logs.
+ [% CASE %]Unhandled error: [% message.error %] + [% END %] + [% END %] + [% END %] [% IF ( opadd ) %]
[% ELSE %] diff --git a/members/memberentry.pl b/members/memberentry.pl index 72cbda5895..35eabfbb8a 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -102,6 +102,7 @@ my @errors; my $borrower_data; my $NoUpdateLogin; my $userenv = C4::Context->userenv; +my @messages; ## Deal with guarantor stuff $template->param( relationships => scalar $patron->guarantor_relationships ) if $patron; @@ -444,6 +445,8 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ # FIXME Urgent error handling here, we cannot fail without relevant feedback # Lot of code will need to be removed from this script to handle exceptions raised by Koha::Patron->store warn "Patron creation failed! - $@"; # Maybe we must die instead of just warn + push @messages, {error => 'error_on_insert_patron'}; + $op = "add"; } else { add_guarantors( $patron, $input ); $borrowernumber = $patron->borrowernumber; @@ -484,7 +487,7 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ } } - if (C4::Context->preference('EnhancedMessagingPreferences') and $input->param('setting_messaging_prefs')) { + if ( $patron && (C4::Context->preference('EnhancedMessagingPreferences') and $input->param('setting_messaging_prefs')) ) { C4::Form::MessagingPreferences::handle_form_action($input, { borrowernumber => $borrowernumber }, $template, 1, $newdata{'categorycode'}); } @@ -494,7 +497,7 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ $hsbnd_chooser = 1 if $input->param('housebound_chooser'); $hsbnd_deliverer = 1 if $input->param('housebound_deliverer'); # Only create a HouseboundRole if patron has a role. - if ( $hsbnd_chooser || $hsbnd_deliverer ) { + if ( $patron && ( $hsbnd_chooser || $hsbnd_deliverer ) ) { Koha::Patron::HouseboundRole->new({ borrowernumber_id => $borrowernumber, housebound_chooser => $hsbnd_chooser, @@ -556,22 +559,24 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ } } - if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) { - $patron->extended_attributes->filter_by_branch_limitations->delete; - $patron->extended_attributes($extended_patron_attributes); - } + if ( $patron ) { + if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) { + $patron->extended_attributes->filter_by_branch_limitations->delete; + $patron->extended_attributes($extended_patron_attributes); + } - if ( $destination eq 'circ' and not C4::Auth::haspermission( C4::Context->userenv->{id}, { circulate => 'circulate_remaining_permissions' } ) ) { - # If we want to redirect to circulation.pl and need to check if the logged in user has the necessary permission - $destination = 'not_circ'; + if ( $destination eq 'circ' and not C4::Auth::haspermission( C4::Context->userenv->{id}, { circulate => 'circulate_remaining_permissions' } ) ) { + # If we want to redirect to circulation.pl and need to check if the logged in user has the necessary permission + $destination = 'not_circ'; + } + print scalar( $destination eq "circ" ) + ? $input->redirect( + "/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber") + : $input->redirect( + "/cgi-bin/koha/members/moremember.pl?borrowernumber=$borrowernumber" + ); + exit; # You can only send 1 redirect! After that, content or other headers don't matter. } - print scalar( $destination eq "circ" ) - ? $input->redirect( - "/cgi-bin/koha/circ/circulation.pl?borrowernumber=$borrowernumber") - : $input->redirect( - "/cgi-bin/koha/members/moremember.pl?borrowernumber=$borrowernumber" - ); - exit; # You can only send 1 redirect! After that, content or other headers don't matter. } if ($delete){ @@ -843,6 +848,7 @@ if ( C4::Context->preference('TranslateNotices') ) { $template->param( languages => $translated_languages ); } +$template->param( messages => \@messages ); output_html_with_http_headers $input, $cookie, $template->output; sub parse_extended_patron_attributes { -- 2.20.1