Bugzilla – Attachment 44529 Details for
Bug 15106
Batch Patron Modification Performance Improvement
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15106:: Batch Patron Modification Performance Improvement
Bug-15106-Batch-Patron-Modification-Performance-Im.patch (text/plain), 12.25 KB, created by
cnorthcott.work
on 2015-11-05 23:58:24 UTC
(
hide
)
Description:
Bug 15106:: Batch Patron Modification Performance Improvement
Filename:
MIME Type:
Creator:
cnorthcott.work
Created:
2015-11-05 23:58:24 UTC
Size:
12.25 KB
patch
obsolete
>From d0baf9f8c99e14137ca06a859b62cd817aa7fd61 Mon Sep 17 00:00:00 2001 >From: Catherine <cnorthcott.work@gmail.com> >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 <number> 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' ) %] >+ <div> >+ <p> Successfully modified [% successes %] patrons. </p> >+ </div> > <p> > <a href="/cgi-bin/koha/tools/modborrowers.pl" title="New batch patrons modification">New batch patron modification</a> > </p> >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=44529">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 15106
: 44529 |
44530
|
44531