Attachment 104743 Details for Bug 25311 – [19.11.x] Bug 25311: Better error handling when creating a patron

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

Bug-25311-Better-error-handling-when-creating-a-pa.patch (text/plain), 7.19 KB, created by Jonathan Druart on 2020-05-12 08:48:27 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2020-05-12 08:48:27 UTC

Size: 7.19 KB

patch

obsolete

>From b9f2de194fb4b6e4c223fc0a6d0c2bfd41c219f9 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'"
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
>---
> .../prog/en/modules/members/memberentrygen.tt | 11 ++++++
> members/memberentry.pl                        | 39 +++++++++++--------
> 2 files changed, 34 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 1be9e1f95e..65775445e4 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
>@@ -34,6 +34,17 @@
> 
> <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 d5b0f08d55..6bcc6d44ff 100755
>--- a/members/memberentry.pl
>+++ b/members/memberentry.pl
>@@ -103,6 +103,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;
>@@ -439,6 +440,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;
>@@ -479,10 +482,10 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){
>             }
>         }
> 
>-        if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
>+        if ($patron && C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
>             C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $extended_patron_attributes);
>         }
>-        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'});
>         }
> 
>@@ -492,7 +495,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,
>@@ -549,25 +552,28 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){
>         }
> 
>         add_guarantors( $patron, $input );
>-        if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
>-            C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $extended_patron_attributes);
>-        }
>         if (C4::Context->preference('EnhancedMessagingPreferences') and $input->param('setting_messaging_prefs')) {
>             C4::Form::MessagingPreferences::handle_form_action($input, { borrowernumber => $borrowernumber }, $template);
>         }
> 	}
> 
>-    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 ( $patron ) {
>+        if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) {
>+            C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $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';
>+        }
>+        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){
>@@ -828,6 +834,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