From b6e7369bdc1159cd75dd79eead6b6a97b4a4ab5b Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 3 Nov 2015 01:38:21 +0000 Subject: [PATCH] Bug 15103: Import Borrowers Performance Improvement Improves the performance of importing borrowers in batches from file by by switching out resultset calls for more direct SQL queries and using hash lookups instead of repeated DB lookups. 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) Download the PatronDataCSV1.csv and PatronDataCSV2.csv attachements from the bug page or organise your own csv's with data for 1000 or more patrons to import. 3) Navigate to Home > Tools > Import Patrons in the Koha Intranet (http://demo-intra.mykoha.co.nz/cgi-bin/koha/tools/import_borrowers.pl) 4) Click the "Browse..." button and select PatronDataCSV1.csv or your equilient. 5) 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. 6) Scroll to the bottom of the page and click the "Import" button (simultaneously start your stopwatch if using one) 7) When the page appears showing completion of the import, stop your stopwatch and check the time or navigate to the folder you have set NYTProf to output. 8) Record the runtime. This is the pre-optimisation time. 9) Apply this patch. 9) Repeat steps 2-7 of this testplan with the patch applied and use the PatronDataCSV2.csv this time around. This will yield the post-optimisation time. 10) Compare the pre-optimisation time and post optimisation time. The second post-optimisation time should be faster. --- C4/Members.pm | 89 +++++++++++++++++++++++++++++++-------------- tools/import_borrowers.pl | 10 +++-- 2 files changed, 68 insertions(+), 31 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 5fb1764..8ad9063 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -458,14 +458,12 @@ sub GetMember { $debug && warn $select, " ",values %information; my $sth = $dbh->prepare("$select"); $sth->execute(map{$information{$_}} keys %information); - my $data = $sth->fetchall_arrayref({}); - #FIXME interface to this routine now allows generation of a result set - #so whole array should be returned but bowhere in the current code expects this - if (@{$data} ) { - return $data->[0]; - } - - return; + my $data = $sth->fetchrow_hashref; + return $data if $data + # FIXME this routine can now support generation of a resultset + # but until that is needed only what is expected by current + # routines will be generated and returned for the sake of + # perfomance. } =head2 GetMemberRelatives @@ -747,13 +745,14 @@ sub AddMember { unless ( $data{'dateenrolled'} ) { $data{'dateenrolled'} = C4::Dates->new()->output("iso"); } - - my $patron_category = $schema->resultset('Category')->find( $data{'categorycode'} ); + + # Check the member category privacy level + my $patron_privacy = GetCategoryPrivacy( $data{'categorycode'} ); $data{'privacy'} = - $patron_category->default_privacy() eq 'default' ? 1 - : $patron_category->default_privacy() eq 'never' ? 2 - : $patron_category->default_privacy() eq 'forever' ? 0 - : undef; + $patron_privacy eq 'default' ? 1 + : $patron_privacy eq 'never' ? 2 + : $patron_privacy eq 'forever' ? 0 + : undef; # Make a copy of the plain text password for later use my $plain_text_password = $data{'password'}; @@ -811,17 +810,18 @@ sub Check_Userid { return 0 unless ($uid); # userid is a unique column, we should assume NULL is not unique - return 0 if ( $uid eq C4::Context->config('user') ); - - my $rs = Koha::Database->new()->schema()->resultset('Borrower'); - - my $params; - $params->{userid} = $uid; - $params->{borrowernumber} = { '!=' => $borrowernumber } if ($borrowernumber); - - my $count = $rs->count( $params ); - - return $count ? 0 : 1; + my $sql = 'SELECT borrowernumber FROM borrowers WHERE userid=?'; + $sql .= ' AND borrowernumber != ?' if $borrowernumber; + my $dbh = C4::Context->dbh; + my $sth = $dbh->prepare($sql); + if ($borrowernumber) { + $sth->execute( $uid, $borrowernumber ); + } + else { + $sth->execute($uid); + } + my $data = $sth->fetchrow; + return $data ? 0 : 1; } =head2 Generate_Userid @@ -1318,7 +1318,7 @@ sub checkcardnumber { return 0 unless defined $cardnumber; my $dbh = C4::Context->dbh; - my $query = "SELECT * FROM borrowers WHERE cardnumber=?"; + my $query = "SELECT borrowernumber FROM borrowers WHERE cardnumber=?"; $query .= " AND borrowernumber <> ?" if ($borrowernumber); my $sth = $dbh->prepare($query); $sth->execute( @@ -1326,7 +1326,7 @@ sub checkcardnumber { ( $borrowernumber ? $borrowernumber : () ) ); - return 1 if $sth->fetchrow_hashref; + return 1 if $sth->fetchrow; my ( $min_length, $max_length ) = get_cardnumber_length(); return 2 @@ -2535,6 +2535,41 @@ WHERE categorycode = ? AND DATEDIFF( NOW(), dateenrolled ) > ? |; return $cnt; } +=head2 GetBorrowecategoryHash + + $arrayref_hashref = &GetBorrowercateforyHash; + + This function returns a hash of borrower categories where the keys are category + codes and values are category descriptions. + +=cut + +sub GetBorrowercategoryHash { + my $categories_list = GetBorrowercategoryList(); + my %categories_hash = map { $_->{categorycode} => $_->{description} } @{$categories_list}; + return %categories_hash; +} + +=head2 GetCategoryPrivacyHash + + $hashref = &GetCategoryPrivacyHash + Returns the default privacy description of a patron's category given the category code. + +=cut + +sub GetCategoryPrivacy { + my ($categorycode) = @_; + my $dbh = C4::Context->dbh; + my $sth = $dbh->prepare( + qq{ + SELECT default_privacy + FROM categories + WHERE categorycode = ? + }); + $sth->execute($categorycode); + return $sth->fetchrow; +} + =head2 DeleteUnverifiedOpacRegistrations Delete all unverified self registrations in borrower_modifications, diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index ad29a66..98e68f8 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -40,7 +40,7 @@ use warnings; use C4::Auth; use C4::Output; use C4::Context; -use C4::Branch qw/GetBranchesLoop GetBranchName/; +use C4::Branch qw/GetBranchesLoop GetBranches/; use C4::Members; use C4::Members::Attributes qw(:all); use C4::Members::AttributeTypes; @@ -144,6 +144,8 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { my $today_iso = output_pref( { dt => dt_from_string, dateonly => 1, dateformat => 'iso' }); my @criticals = qw(surname branchcode categorycode); # there probably should be others my @bad_dates; # I've had a few. + my $all_branches = GetBranches(); + my %all_categories = C4::Members::GetBorrowercategoryHash(); LINE: while ( my $borrowerline = <$handle> ) { my %borrower; my @missing_criticals; @@ -178,13 +180,13 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { #warn join(':',%borrower); if ($borrower{categorycode}) { push @missing_criticals, {key=>'categorycode', line=>$. , lineraw=>$borrowerline, value=>$borrower{categorycode}, category_map=>1} - unless GetBorrowercategory($borrower{categorycode}); + unless $all_categories{ $borrower{categorycode} } } else { push @missing_criticals, {key=>'categorycode', line=>$. , lineraw=>$borrowerline}; } if ($borrower{branchcode}) { push @missing_criticals, {key=>'branchcode', line=>$. , lineraw=>$borrowerline, value=>$borrower{branchcode}, branch_map=>1} - unless GetBranchName($borrower{branchcode}); + unless $all_branches->{ $borrower{branchcode} }->{branchname}; } else { push @missing_criticals, {key=>'branchcode', line=>$. , lineraw=>$borrowerline}; } @@ -314,7 +316,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { $template->param('lastoverwritten'=>$borrower{'surname'}.' / '.$borrowernumber); } else { # FIXME: fixup_cardnumber says to lock table, but the web interface doesn't so this doesn't either. - # At least this is closer to AddMember than in members/memberentry.pl + # At least this is closer to AddMembes $all_categories{ $borrower{categorycode} } than in members/memberentry.pl if (!$borrower{'cardnumber'}) { $borrower{'cardnumber'} = fixup_cardnumber(undef); } -- 1.7.10.4