From 735e6cceb69e82139b64a1697b5ae35c43847632 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Fri, 27 Sep 2024 15:04:23 +0000
Subject: [PATCH] Bug 27123: (QA follow-up) Tidy modborrowers.pl

---
 tools/modborrowers.pl | 358 +++++++++++++++++++++---------------------
 1 file changed, 178 insertions(+), 180 deletions(-)

diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl
index babd3eb6d11..190fe94b2b7 100755
--- a/tools/modborrowers.pl
+++ b/tools/modborrowers.pl
@@ -26,13 +26,13 @@
 # And for patron attributes.
 
 use Modern::Perl;
-use CGI qw ( -utf8 );
+use CGI      qw ( -utf8 );
 use C4::Auth qw( get_template_and_user );
 use C4::Koha qw( GetAuthorisedValues );
 use C4::Members;
 use C4::Output qw( output_html_with_http_headers );
 use C4::Context;
-use Koha::DateUtils qw( dt_from_string );
+use Koha::DateUtils    qw( dt_from_string );
 use Koha::List::Patron qw( GetPatronLists );
 use Koha::Libraries;
 use Koha::Patron::Categories;
@@ -42,20 +42,21 @@ use List::MoreUtils qw(uniq);
 use Koha::Patron::Messages;
 
 my $input = CGI->new;
-my $op = $input->param('op') || 'show_form';
+my $op    = $input->param('op') || 'show_form';
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
-    {   template_name   => "tools/modborrowers.tt",
-        query           => $input,
-        type            => "intranet",
-        flagsrequired   => { tools => "edit_patrons" },
+    {
+        template_name => "tools/modborrowers.tt",
+        query         => $input,
+        type          => "intranet",
+        flagsrequired => { tools => "edit_patrons" },
     }
 );
 
-my $logged_in_user = Koha::Patrons->find( $loggedinuser );
+my $logged_in_user = Koha::Patrons->find($loggedinuser);
 
 $template->param( CanUpdatePasswordExpiration => 1 ) if $logged_in_user->is_superlibrarian;
 
-my $dbh       = C4::Context->dbh;
+my $dbh = C4::Context->dbh;
 
 # Show borrower informations
 if ( $op eq 'cud-show' || $op eq 'show' ) {
@@ -65,10 +66,12 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
     my $useborrowernumbers = 0;
 
     # Get cardnumbers from a file or the input area
-    if( my $cardnumberlist = $input->param('cardnumberlist') ){
+    if ( my $cardnumberlist = $input->param('cardnumberlist') ) {
+
         # User submitted a list of card numbers
         push @patronidnumbers, split( /\s\n/, $cardnumberlist );
-    } elsif ( my $cardnumberuploadfile = $input->param('cardnumberuploadfile') ){
+    } elsif ( my $cardnumberuploadfile = $input->param('cardnumberuploadfile') ) {
+
         # User uploaded a file of card numbers
         binmode $cardnumberuploadfile, ':encoding(UTF-8)';
         while ( my $content = <$cardnumberuploadfile> ) {
@@ -76,11 +79,13 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
             $content =~ s/[\r\n]*$//;
             push @patronidnumbers, $content if $content;
         }
-    } elsif ( my $borrowernumberlist = $input->param('borrowernumberlist') ){
+    } elsif ( my $borrowernumberlist = $input->param('borrowernumberlist') ) {
+
         # User submitted a list of borrowernumbers
         $useborrowernumbers = 1;
         push @patronidnumbers, split( /\s\n/, $borrowernumberlist );
-    } elsif ( my $borrowernumberuploadfile = $input->param('borrowernumberuploadfile') ){
+    } elsif ( my $borrowernumberuploadfile = $input->param('borrowernumberuploadfile') ) {
+
         # User uploaded a file of borrowernumbers
         $useborrowernumbers = 1;
         binmode $borrowernumberuploadfile, ':encoding(UTF-8)';
@@ -89,39 +94,40 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
             $content =~ s/[\r\n]*$//;
             push @patronidnumbers, $content if $content;
         }
-    } elsif ( my $patron_list_id = $input->param('patron_list_id') ){
+    } elsif ( my $patron_list_id = $input->param('patron_list_id') ) {
+
         # User selected a patron list
         my ($list) = GetPatronLists( { patron_list_id => $patron_list_id } );
         @patronidnumbers =
-          $list->patron_list_patrons()->search_related('borrowernumber')
-          ->get_column('cardnumber')->all();
+            $list->patron_list_patrons()->search_related('borrowernumber')->get_column('cardnumber')->all();
     }
 
     my $max_nb_attr = 0;
 
     # Make sure there is only one of each patron id number
-    @patronidnumbers = uniq( @patronidnumbers );
+    @patronidnumbers = uniq(@patronidnumbers);
 
-    for my $patronidnumber ( @patronidnumbers ) {
+    for my $patronidnumber (@patronidnumbers) {
         my $patron;
-        if( $useborrowernumbers == 1 ){
+        if ( $useborrowernumbers == 1 ) {
             $patron = Koha::Patrons->find( { borrowernumber => $patronidnumber } );
         } else {
             $patron = Koha::Patrons->find( { cardnumber => $patronidnumber } );
         }
-        if ( $patron ) {
-            if ( $logged_in_user->can_see_patron_infos( $patron ) ) {
-                my $borrower = $patron->unblessed;
-                my $attributes = $patron->extended_attributes;
+        if ($patron) {
+            if ( $logged_in_user->can_see_patron_infos($patron) ) {
+                my $borrower        = $patron->unblessed;
+                my $attributes      = $patron->extended_attributes;
                 my $patron_messages = Koha::Patron::Messages->search(
                     {
                         'me.borrowernumber' => $patron->borrowernumber,
                     }
                 );
-                $borrower->{patron_messages} = $patron_messages->as_list;
-                $borrower->{patron_attributes} = $attributes->as_list;
+                $borrower->{patron_messages}         = $patron_messages->as_list;
+                $borrower->{patron_attributes}       = $attributes->as_list;
                 $borrower->{patron_attributes_count} = $attributes->count;
-                $max_nb_attr = $borrower->{patron_attributes_count} if $borrower->{patron_attributes_count} > $max_nb_attr;
+                $max_nb_attr                         = $borrower->{patron_attributes_count}
+                    if $borrower->{patron_attributes_count} > $max_nb_attr;
                 push @borrowers, $borrower;
             } else {
                 push @notfoundcardnumbers, $patronidnumber;
@@ -132,26 +138,28 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
     }
 
     # Just for a correct display
-    for my $borrower ( @borrowers ) {
+    for my $borrower (@borrowers) {
         my $length = $borrower->{patron_attributes_count};
-        push @{ $borrower->{patron_attributes} }, {} for ( $length .. $max_nb_attr - 1);
+        push @{ $borrower->{patron_attributes} }, {} for ( $length .. $max_nb_attr - 1 );
     }
 
     # Construct the patron attributes list
     my @patron_attributes_values;
     my @patron_attributes_codes;
-    my $library_id = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef;
-    my $patron_attribute_types = Koha::Patron::Attribute::Types->search_with_library_limits({}, {}, $library_id);
-    my @patron_categories = Koha::Patron::Categories->search_with_library_limits({}, {order_by => ['description']})->as_list;
+    my $library_id             = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef;
+    my $patron_attribute_types = Koha::Patron::Attribute::Types->search_with_library_limits( {}, {}, $library_id );
+    my @patron_categories =
+        Koha::Patron::Categories->search_with_library_limits( {}, { order_by => ['description'] } )->as_list;
     while ( my $attr_type = $patron_attribute_types->next ) {
-        next if $attr_type->unique_id; # Don't display patron attributes that must be unqiue
-        my $options = $attr_type->authorised_value_category
+        next if $attr_type->unique_id;    # Don't display patron attributes that must be unqiue
+        my $options =
+            $attr_type->authorised_value_category
             ? GetAuthorisedValues( $attr_type->authorised_value_category )
             : undef;
         push @patron_attributes_values,
             {
-                attribute_code => $attr_type->code,
-                options        => $options,
+            attribute_code => $attr_type->code,
+            options        => $options,
             };
 
         my $category_code = $attr_type->category_code;
@@ -160,18 +168,18 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
             @patron_categories;
         push @patron_attributes_codes,
             {
-                attribute_code => $attr_type->code,
-                attribute_lib  => $attr_type->description,
-                category_lib   => $category_lib,
-                type           => $attr_type->authorised_value_category ? 'select' : 'text',
+            attribute_code => $attr_type->code,
+            attribute_lib  => $attr_type->description,
+            category_lib   => $category_lib,
+            type           => $attr_type->authorised_value_category ? 'select' : 'text',
             };
     }
 
     my @attributes_header = ();
-    for ( 1 .. scalar( $max_nb_attr ) ) {
+    for ( 1 .. scalar($max_nb_attr) ) {
         push @attributes_header, { attribute => "Attributes $_" };
     }
-    $template->param( borrowers => \@borrowers );
+    $template->param( borrowers         => \@borrowers );
     $template->param( attributes_header => \@attributes_header );
     @notfoundcardnumbers = map { { cardnumber => $_ } } @notfoundcardnumbers;
     $template->param( notfoundcardnumbers => \@notfoundcardnumbers )
@@ -179,7 +187,7 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
     $template->param( useborrowernumbers => $useborrowernumbers );
 
     # Construct drop-down list values
-    my $branches = Koha::Libraries->search({}, { order_by => ['branchname'] })->unblessed;
+    my $branches = Koha::Libraries->search( {}, { order_by => ['branchname'] } )->unblessed;
     my @branches_option;
     push @branches_option, { value => $_->{branchcode}, lib => $_->{branchname} } for @$branches;
     unshift @branches_option, { value => "", lib => "" };
@@ -201,158 +209,135 @@ if ( $op eq 'cud-show' || $op eq 'show' ) {
 
     my @fields = (
         {
-            name => "surname",
-            type => "text",
+            name      => "surname",
+            type      => "text",
             mandatory => ( grep /surname/, @mandatoryFields ) ? 1 : 0
-        }
-        ,
+        },
         {
-            name => "firstname",
-            type => "text",
+            name      => "firstname",
+            type      => "text",
             mandatory => ( grep /firstname/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "branchcode",
-            type => "select",
-            option => \@branches_option,
+            name      => "branchcode",
+            type      => "select",
+            option    => \@branches_option,
             mandatory => ( grep /branchcode/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "categorycode",
-            type => "select",
-            option => \@categories_option,
+            name      => "categorycode",
+            type      => "select",
+            option    => \@categories_option,
             mandatory => ( grep /categorycode/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "streetnumber",
-            type => "text",
+            name      => "streetnumber",
+            type      => "text",
             mandatory => ( grep /streetnumber/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "address",
-            type => "text",
+            name      => "address",
+            type      => "text",
             mandatory => ( grep /address/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "address2",
-            type => "text",
+            name      => "address2",
+            type      => "text",
             mandatory => ( grep /address2/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "city",
-            type => "text",
+            name      => "city",
+            type      => "text",
             mandatory => ( grep /city/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "state",
-            type => "text",
+            name      => "state",
+            type      => "text",
             mandatory => ( grep /state/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "zipcode",
-            type => "text",
+            name      => "zipcode",
+            type      => "text",
             mandatory => ( grep /zipcode/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "country",
-            type => "text",
+            name      => "country",
+            type      => "text",
             mandatory => ( grep /country/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "email",
-            type => "text",
+            name      => "email",
+            type      => "text",
             mandatory => ( grep /email/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "phone",
-            type => "text",
+            name      => "phone",
+            type      => "text",
             mandatory => ( grep /phone/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "mobile",
-            type => "text",
+            name      => "mobile",
+            type      => "text",
             mandatory => ( grep /mobile/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
             name      => "fax",
             type      => "text",
             mandatory => ( grep /fax/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "sort1",
-            type => @sort1_option ? "select" : "text",
-            option => \@sort1_option,
+            name      => "sort1",
+            type      => @sort1_option ? "select" : "text",
+            option    => \@sort1_option,
             mandatory => ( grep /sort1/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "sort2",
-            type => @sort2_option ? "select" : "text",
-            option => \@sort2_option,
+            name      => "sort2",
+            type      => @sort2_option ? "select" : "text",
+            option    => \@sort2_option,
             mandatory => ( grep /sort2/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "dateenrolled",
-            type => "date",
+            name      => "dateenrolled",
+            type      => "date",
             mandatory => ( grep /dateenrolled/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "dateexpiry",
-            type => "date",
+            name      => "dateexpiry",
+            type      => "date",
             mandatory => ( grep /dateexpiry/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "borrowernotes",
-            type => "text",
+            name      => "borrowernotes",
+            type      => "text",
             mandatory => ( grep /borrowernotes/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "opacnote",
-            type => "text",
+            name      => "opacnote",
+            type      => "text",
             mandatory => ( grep /opacnote/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "message",
-            type => "message_type",
+            name      => "message",
+            type      => "message_type",
             mandatory => ( grep /message/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "debarred",
-            type => "date",
+            name      => "debarred",
+            type      => "date",
             mandatory => ( grep /debarred/, @mandatoryFields ) ? 1 : 0,
-        }
-        ,
+        },
         {
-            name => "debarredcomment",
-            type => "text",
+            name      => "debarredcomment",
+            type      => "text",
             mandatory => ( grep /debarredcomment/, @mandatoryFields ) ? 1 : 0,
         },
     );
 
     push @fields, { name => "password_expiration_date", type => "date" } if $logged_in_user->is_superlibrarian;
 
-    $template->param('patron_attributes_codes', \@patron_attributes_codes);
-    $template->param('patron_attributes_values', \@patron_attributes_values);
+    $template->param( 'patron_attributes_codes',  \@patron_attributes_codes );
+    $template->param( 'patron_attributes_values', \@patron_attributes_values );
 
     $template->param( fields => \@fields );
     $op = 'show';
@@ -372,19 +357,21 @@ if ( $op eq 'cud-do' ) {
         $infos->{$field} = ""     if grep { $_ eq $field } @disabled;
     }
 
-    for my $field ( qw( dateenrolled dateexpiry debarred password_expiration_date ) ) {
-        $infos->{$field} = dt_from_string($infos->{$field}) if $infos->{$field};
+    for my $field (qw( dateenrolled dateexpiry debarred password_expiration_date )) {
+        $infos->{$field} = dt_from_string( $infos->{$field} ) if $infos->{$field};
     }
 
     delete $infos->{password_expiration_date} unless $logged_in_user->is_superlibrarian;
 
     my @errors;
     my @borrowernumbers = $input->multi_param('borrowernumber');
+
     # For each borrower selected
-    for my $borrowernumber ( @borrowernumbers ) {
+    for my $borrowernumber (@borrowernumbers) {
 
         # If at least one field are filled, we want to modify the borrower
         if ( defined $infos ) {
+
             # If a debarred date or debarred comment has been submitted make a new debarment
             if ( $infos->{debarred} || $infos->{debarredcomment} ) {
                 AddDebarment(
@@ -393,40 +380,46 @@ if ( $op eq 'cud-do' ) {
                         type           => 'MANUAL',
                         comment        => $infos->{debarredcomment},
                         expiration     => $infos->{debarred},
-                    });
+                    }
+                );
             }
 
             # If debarment date or debarment comment are disabled then remove all debarrments
-            my $patron = Koha::Patrons->find( $borrowernumber );
+            my $patron = Koha::Patrons->find($borrowernumber);
             if ( grep { /debarred/ } @disabled ) {
                 eval {
-                   my $debarrments = $patron->restrictions;
-                   while( my $debarment = $debarrments->next ) {
-                      DelDebarment( $debarment->borrower_debarment_id );
-                   }
+                    my $debarrments = $patron->restrictions;
+                    while ( my $debarment = $debarrments->next ) {
+                        DelDebarment( $debarment->borrower_debarment_id );
+                    }
                 };
             }
+
             # If 'message' or 'add_message_type' is defined then delete both at the same time
-            if ( grep { $_ eq 'message' } @disabled) {
-                my $patron = Koha::Patrons->find( $borrowernumber );
+            if ( grep { $_ eq 'message' } @disabled ) {
+                my $patron = Koha::Patrons->find($borrowernumber);
                 $patron->messages()->delete();
             }
             $infos->{borrowernumber} = $borrowernumber;
             eval { $patron->set($infos)->store; };
-            if ( $@ ) { # FIXME We could provide better error handling here
+            if ($@) {    # FIXME We could provide better error handling here
                 $infos->{cardnumber} = $patron ? $patron->cardnumber || '' : '';
-                push @errors, { error => "can_not_update", borrowernumber => $infos->{borrowernumber}, cardnumber => $infos->{cardnumber} };
+                push @errors,
+                    {
+                    error      => "can_not_update", borrowernumber => $infos->{borrowernumber},
+                    cardnumber => $infos->{cardnumber}
+                    };
             }
         }
 
-        my $patron = Koha::Patrons->find( $borrowernumber );
-        my @attributes = $input->multi_param('patron_attributes');
+        my $patron      = Koha::Patrons->find($borrowernumber);
+        my @attributes  = $input->multi_param('patron_attributes');
         my @attr_values = $input->multi_param('patron_attributes_value');
         my $attributes;
         my $i = 0;
-        for my $code ( @attributes ) {
-            push @{ $attributes->{$code}->{values} }, shift @attr_values; # Handling repeatables
-            $attributes->{$code}->{disabled} = grep { $_ eq sprintf("attr%s_value", ++$i) } @disabled;
+        for my $code (@attributes) {
+            push @{ $attributes->{$code}->{values} }, shift @attr_values;    # Handling repeatables
+            $attributes->{$code}->{disabled} = grep { $_ eq sprintf( "attr%s_value", ++$i ) } @disabled;
         }
 
         for my $code ( keys %$attributes ) {
@@ -438,24 +431,28 @@ if ( $op eq 'cud-do' ) {
             next if $attr_type->category_code and $attr_type->category_code ne $patron->categorycode;
 
             if ( $attributes->{$code}->{disabled} ) {
+
                 # The attribute is disabled, we remove it for this borrower !
                 eval {
-                    $patron->extended_attributes->search({'me.code' => $code})->filter_by_branch_limitations->delete;
+                    $patron->extended_attributes->search( { 'me.code' => $code } )
+                        ->filter_by_branch_limitations->delete;
                 };
                 push @errors, { error => $@ } if $@;
             } else {
                 eval {
-                    $patron->extended_attributes->search({'me.code' => $code})->filter_by_branch_limitations->delete;
-                    $patron->add_extended_attribute({ code => $code, attribute => $_ }) for @{$attributes->{$code}->{values}};
+                    $patron->extended_attributes->search( { 'me.code' => $code } )
+                        ->filter_by_branch_limitations->delete;
+                    $patron->add_extended_attribute( { code => $code, attribute => $_ } )
+                        for @{ $attributes->{$code}->{values} };
                 };
                 push @errors, { error => $@ } if $@;
             }
         }
 
         # Handle patron messages
-        my $message          = $input->param('message');
-        my $branchcode       = C4::Context::mybranch;
-        my $message_type     = $input->param('add_message_type');
+        my $message      = $input->param('message');
+        my $branchcode   = C4::Context::mybranch;
+        my $message_type = $input->param('add_message_type');
 
         Koha::Patron::Message->new(
             {
@@ -467,22 +464,22 @@ if ( $op eq 'cud-do' ) {
         )->store;
     }
 
-    $op = "show_results"; # We have process modifications, the user want to view its
+    $op = "show_results";    # We have process modifications, the user want to view its
 
     # Construct the results list
     my @borrowers;
     my $max_nb_attr = 0;
-    for my $borrowernumber ( @borrowernumbers ) {
-        my $patron = Koha::Patrons->find( $borrowernumber );
-        if ( $patron ) {
+    for my $borrowernumber (@borrowernumbers) {
+        my $patron = Koha::Patrons->find($borrowernumber);
+        if ($patron) {
             my $category_description = $patron->category->description;
-            my $borrower = $patron->unblessed;
-            my $patron_messages = Koha::Patron::Messages->search(
-                    {
-                        'me.borrowernumber' => $patron->borrowernumber,
-                    }
-                );
-            $borrower->{patron_messages} = $patron_messages->as_list;
+            my $borrower             = $patron->unblessed;
+            my $patron_messages      = Koha::Patron::Messages->search(
+                {
+                    'me.borrowernumber' => $patron->borrowernumber,
+                }
+            );
+            $borrower->{patron_messages}      = $patron_messages->as_list;
             $borrower->{category_description} = $category_description;
             my $attributes = $patron->extended_attributes;
             $borrower->{patron_attributes} = $attributes->as_list;
@@ -491,18 +488,19 @@ if ( $op eq 'cud-do' ) {
         }
     }
     my @patron_attributes_option;
-    for my $borrower ( @borrowers ) {
-        push @patron_attributes_option, { value => "$_->{code}", lib => $_->{code} } for @{ $borrower->{patron_attributes} };
+    for my $borrower (@borrowers) {
+        push @patron_attributes_option, { value => "$_->{code}", lib => $_->{code} }
+            for @{ $borrower->{patron_attributes} };
         my $length = scalar( @{ $borrower->{patron_attributes} } );
-        push @{ $borrower->{patron_attributes} }, {} for ( $length .. $max_nb_attr - 1);
+        push @{ $borrower->{patron_attributes} }, {} for ( $length .. $max_nb_attr - 1 );
     }
 
     my @attributes_header = ();
-    for ( 1 .. scalar( $max_nb_attr ) ) {
+    for ( 1 .. scalar($max_nb_attr) ) {
         push @attributes_header, { attribute => "Attributes $_" };
     }
 
-    $template->param( borrowers => \@borrowers );
+    $template->param( borrowers         => \@borrowers );
     $template->param( attributes_header => \@attributes_header );
 
     $template->param( errors => \@errors );
-- 
2.39.5 (Apple Git-154)