Attachment 103930 Details for Bug 25311 – Bug 25311: Better error handling when creating a patron

[patch] Bug 25311: Better error handling when creating a patron

Bug-25311-Better-error-handling-when-creating-a-pa.patch (text/plain), 6.55 KB, created by Jonathan Druart on 2020-04-29 12:49:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2020-04-29 12:49:53 UTC

Size: 6.55 KB

patch

obsolete

>From 18479b4a7aa14201b7ae81d3275441e70e405807 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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'"
>---
> .../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 {
> 
>     <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 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

Actions: View | Diff | Splinter Review

Attachments on bug 25311: 103930 | 103931 | 103939 | 103940 | 104154 | 104155 | 104156 | 104157 | 104158 | 104743 | 104744 | 104745