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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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 4b5eb35..02f21e8 100644 (file)
--- 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 {
 
     <div class="main container-fluid">
         <div class="row">
+            [% IF messages %]
+                [% FOR message IN messages %]
+                    [% SWITCH message.error %]
+                        [% CASE 'error_on_insert_patron' %]
+                            <div class="dialog alert">Something went wrong when creating the patron. Check the logs.</div>
+                        [% CASE %]Unhandled error: [% message.error %]
+                    [% END %]
+                [% END %]
+            [% END %]
             [% IF ( opadd ) %]
                 <div class="col-md-10 col-md-offset-1 col-lg-8 col-lg-offset-2">
             [% ELSE %]

diff --git a/members/memberentry.pl b/members/memberentry.pl
index 72cbda5..35eabfb 100755 (executable)
--- 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 {