Bugzilla – Attachment 172122 Details for
Bug 27123
Add messages to batch patron modification
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 27123: (QA follow-up) Tidy modborrowers.pl
Bug-27123-QA-follow-up-Tidy-modborrowerspl.patch (text/plain), 25.25 KB, created by
Kyle M Hall (khall)
on 2024-09-27 15:08:22 UTC
(
hide
)
Description:
Bug 27123: (QA follow-up) Tidy modborrowers.pl
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2024-09-27 15:08:22 UTC
Size:
25.25 KB
patch
obsolete
>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)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 27123
:
131015
|
131042
|
132923
|
132924
|
132925
|
132926
|
132927
|
134065
|
172012
|
172013
|
172053
|
172086
|
172087
|
172120
|
172121
| 172122