From d0baf9f8c99e14137ca06a859b62cd817aa7fd61 Mon Sep 17 00:00:00 2001 From: Catherine Date: Thu, 5 Nov 2015 23:18:02 +0000 Subject: [PATCH] Bug 15106:: Batch Patron Modification Performance Improvement Batch Patron Modification has a fairly long run/load time. I have attempted to improve the runtime of the Batch Patron Modification tool by removing unecessary reporting detail after large batch patron changes and altering some logic to avoid fetching and checking data that won't be needed. Test Plan: 1) Prepare NTYprof (http://wiki.koha-community.org/wiki/Profiling_with_Devel::NYTProf) or procure a stopwatch (stopwatch will be simpler but less accurate). 2) Ensure you have a decent number of patrons in your Koha system (>500) if you are lacking patrons to modify in your database you can import 1000 using the PatronDataCSV.csv attachement and the Import Patrons tool before moving to step 3. 3) Prepare a file with the cardnumber of borrowers to modify. If you imported patrons using PatronDataCSV.csv you can use the attached PatronCardNums.txt 4) Navigate to Home > Tools > Batch Patrons Modification in the Koha Intranet. 5) Click the "Browse..." button and select PatronCardNums.txt 6) Click the "Continue" button and scroll to the bottom of the page. 7) Enter text into the "Country" field box. 8) Enter text into the "Circulation note" field box. 9) If you are using a stopwatch, prepare your stopwatch so that you will start counting seconds from the point you click the button in the next step. 10) Click the "Save" button (simultaneously start your stopwatch if using one) 11) When the page appears showing completion of the change, stop your stopwatch and check the time or navigate to the folder you have set NYTProf to output. 12) Record the runtime. This is the pre-optimisation time. 13) If this is your pre-patch test, the success page will show all the patrons with the data changed, if this is your post-patch test it should only show a "Successfully changed patrons" message. 13) Apply this patch. 14) Repeat steps 2-12 of this testplan with the patch applied. This will yield the post-optimisation time. 15) Compare the pre-optimisation time and post optimisation time. The second post-optimisation time should be faster. --- C4/Members.pm | 26 ++++- .../prog/en/modules/tools/modborrowers.tt | 3 + tools/modborrowers.pl | 112 +++++++++++--------- 3 files changed, 84 insertions(+), 57 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 171f3c0..a41739b 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -656,11 +656,9 @@ sub ModMember { $data{password} = hash_password($data{password}); } } - my $old_categorycode = GetBorrowerCategorycode( $data{borrowernumber} ); # get only the columns of a borrower - my $schema = Koha::Database->new()->schema; - my @columns = $schema->source('Borrower')->columns; + my @columns = @{GetBorrowerColumns()}; my $new_borrower = { map { join(' ', @columns) =~ /$_/ ? ( $_ => $data{$_} ) : () } keys(%data) }; delete $new_borrower->{flags}; @@ -668,6 +666,8 @@ sub ModMember { $new_borrower->{dateenrolled} ||= undef if exists $new_borrower->{dateenrolled}; $new_borrower->{dateexpiry} ||= undef if exists $new_borrower->{dateexpiry}; $new_borrower->{debarred} ||= undef if exists $new_borrower->{debarred}; + + my $schema = Koha::Database->new()->schema; my $rs = $schema->resultset('Borrower')->search({ borrowernumber => $new_borrower->{borrowernumber}, }); @@ -683,7 +683,7 @@ sub ModMember { } # If the patron changes to a category with enrollment fee, we add a fee - if ( $data{categorycode} and $data{categorycode} ne $old_categorycode ) { + if ( $data{categorycode} and $data{categorycode} ne GetBorrowerCategoryCode($data{borrowernumber}) ) { if ( C4::Context->preference('FeeOnChangePatronCategory') ) { AddEnrolmentFeeIfNeeded( $data{categorycode}, $data{borrowernumber} ); } @@ -713,6 +713,20 @@ sub ModMember { return $execute_success; } +=head2 GetBorrowerColumns + + $columns_arrayref = &GetBorrowerColumns: + +Returns all borrower table columns. + +=cut + +sub GetBorrowerColumns { + my $schema = Koha::Database->new()->schema; + my @columns = $schema->source('Borrower')->columns; + return \@columns; +} + =head2 AddMember $borrowernumber = &AddMember(%borrower); @@ -1603,12 +1617,14 @@ Given the borrowernumber, the function returns the corresponding categorycode sub GetBorrowerCategorycode { my ( $borrowernumber ) = @_; my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare( qq{ + my $sth = $dbh->prepare_cached( qq{ SELECT categorycode FROM borrowers WHERE borrowernumber = ? } ); $sth->execute( $borrowernumber ); + my $data = $sth->fetchrow; + $sth->finish; return $sth->fetchrow; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt index 65ce440..623e431 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/modborrowers.tt @@ -372,6 +372,9 @@ [% END %] [% END %] [% IF ( op == 'show_results' ) %] +
+

Successfully modified [% successes %] patrons.

+

New batch patron modification

diff --git a/tools/modborrowers.pl b/tools/modborrowers.pl index 4172bc3..8bfaac8 100755 --- a/tools/modborrowers.pl +++ b/tools/modborrowers.pl @@ -27,7 +27,7 @@ use Modern::Perl; use CGI qw ( -utf8 ); -use C4::Auth; +use C4::Auth qw ( get_template_and_user ); use C4::Branch; use C4::Koha; use C4::Members; @@ -261,9 +261,7 @@ if ( $op eq 'do' ) { $infos->{$field} = "" if grep { /^$field$/ } @disabled; } - my @attributes = $input->param('patron_attributes'); - my @attr_values = $input->param('patron_attributes_value'); - + my $successes = 0; my @errors; my @borrowernumbers = $input->param('borrowernumber'); # For each borrower selected @@ -277,67 +275,77 @@ if ( $op eq 'do' ) { $infos->{cardnumber} = $borrowerinfo->{cardnumber} || ''; push @errors, { error => "can_not_update", borrowernumber => $infos->{borrowernumber}, cardnumber => $infos->{cardnumber} }; } + else { + $successes++; + } } # - my $borrower_categorycode = GetBorrowerCategorycode $borrowernumber; - my $i=0; - for ( @attributes ) { - my $attribute; - $attribute->{code} = $_; - $attribute->{attribute} = $attr_values[$i]; - my $attr_type = C4::Members::AttributeTypes->fetch( $_ ); - # If this borrower is not in the category of this attribute, we don't want to modify this attribute - ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode; - my $valuename = "attr" . $i . "_value"; - if ( grep { /^$valuename$/ } @disabled ) { - # The attribute is disabled, we remove it for this borrower ! - eval { - C4::Members::Attributes::DeleteBorrowerAttribute( $borrowernumber, $attribute ); - }; - push @errors, { error => $@ } if $@; - } else { - # Attribute's value is empty, we don't want to modify it - ++$i and next if not $attribute->{attribute}; - - eval { - C4::Members::Attributes::UpdateBorrowerAttribute( $borrowernumber, $attribute ); - }; - push @errors, { error => $@ } if $@; + if (@disabled) { + my @attributes = $input->param('patron_attributes'); + my @attr_values = $input->param('patron_attributes_value'); + my $borrower_categorycode = GetBorrowerCategorycode $borrowernumber; + my $i=0; + for ( @attributes ) { + my $attribute; + $attribute->{code} = $_; + $attribute->{attribute} = $attr_values[$i]; + my $attr_type = C4::Members::AttributeTypes->fetch( $_ ); + # If this borrower is not in the category of this attribute, we don't want to modify this attribute + ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode; + my $valuename = "attr" . $i . "_value"; + if ( grep { /^$valuename$/ } @disabled ) { + # The attribute is disabled, we remove it for this borrower ! + eval { + C4::Members::Attributes::DeleteBorrowerAttribute( $borrowernumber, $attribute ); + }; + push @errors, { error => $@ } if $@; + } else { + # Attribute's value is empty, we don't want to modify it + ++$i and next if not $attribute->{attribute}; + + eval { + C4::Members::Attributes::UpdateBorrowerAttribute( $borrowernumber, $attribute ); + }; + push @errors, { error => $@ } if $@; + } + $i++; } - $i++; } } $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 $borrower = GetBorrowerInfos( borrowernumber => $borrowernumber ); - if ( $borrower ) { - $max_nb_attr = scalar( @{ $borrower->{patron_attributes} } ) - if scalar( @{ $borrower->{patron_attributes} } ) > $max_nb_attr; - push @borrowers, $borrower; + # Construct a detailed list if there are fewer than 20 patrons + if (scalar @borrowernumbers < 20) { + my @borrowers; + my $max_nb_attr = 0; + for my $borrowernumber ( @borrowernumbers ) { + my $borrower = GetBorrowerInfos( borrowernumber => $borrowernumber ); + if ( $borrower ) { + $max_nb_attr = scalar( @{ $borrower->{patron_attributes} } ) + if scalar( @{ $borrower->{patron_attributes} } ) > $max_nb_attr; + push @borrowers, $borrower; + } + } + my @patron_attributes_option; + 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); } - } - my @patron_attributes_option; - 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); - } - my @attributes_header = (); - for ( 1 .. scalar( $max_nb_attr ) ) { - push @attributes_header, { attribute => "Attributes $_" }; - } + my @attributes_header = (); + for ( 1 .. scalar( $max_nb_attr ) ) { + push @attributes_header, { attribute => "Attributes $_" }; + } - $template->param( borrowers => \@borrowers ); - $template->param( attributes_header => \@attributes_header ); + $template->param( borrowers => \@borrowers ); + $template->param( attributes_header => \@attributes_header ); + } - $template->param( borrowers => \@borrowers ); + $template->param( successes => $successes ); $template->param( errors => \@errors ); + } else { $template->param( patron_lists => [ GetPatronLists() ] ); -- 1.7.10.4