From e668b02e5964d643ef59e5e61604bfacf9465004 Mon Sep 17 00:00:00 2001 From: Paul Poulain Date: Sat, 17 Sep 2011 00:19:05 +0200 Subject: [PATCH] bug 6875 de-nesting C4::Member * working on GetMemberDetails. This sub must be optimized, as it is called on each page by C4::Auth. The sub call everytime patronflags, that need to load Reserves and Overdues. The patronflag returns a hash with various flags (overdues, fines, ...). It's not related to permissionflags, that are loaded elsewhere. Most of the time, we do nothing with those flags. The strategy is to split the GetMemberDetails in 2 subs : * GetMemberDetails, same as previously, without the flags * GetMemberFlags, that will return just the flags when needed (and will load when needed Reserves package) The following scripts call GetMemberDetails : * admin/aqbudget.pl = no use is done with flags, the splitting changes nothing * C4/SIP/ILS/Patron.pm = the flags are used later, so we call GetMemberFlags as well * C4/SIP/interactive_members_dump.pl = nothing is done with the flags, we change nothing * C4/Print.pm = used by printslip, that don't do anything with flags, we change nothing * C4/VirtualShelves = used to check permission to access to a virtualshelf, nothing done with flags, we change nothing * C4/Reserves = used in CheckReserves. With the result, we call _GetCircControlBranch only, that does nothing with flags, we change nothing * C4/Auth = used in get_template_and_user. The return from GetMemberDetails is stored in the session, I don't see any change, flags are useless * C4/Circulation.pm = used many times * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already issued to someone else * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already reserved by someone else * in AddReturn, the result of GetMemberDetails is returned. It's then used in return.pl, to display patron flags. Retrieving flags with GetMemberFlags in return.pl * in AddRenewal, to retrieve some information, nothing done with flags, we change nothing * in GetRenewCount, to retrieve name & categorycode, nothing done with flags, we change nothing * in ILSDI/Services.pm, GetMemberDetails is called many times. Only once we check for charges, that is in the flags, so retrieving flags with GetMemberFlags * in catalogue/detail.pl, GetMemberDetails is called to retrieve the name of a patron that has placed a hold on a book, we change nothing * in circ/transferstoreceive, GetMemberDetails is called to retrieve the name of a patron, (and address), we change nothing * in circ/circulation.pl, we must retrieve the flags to display them (like for return.pl) * in circ/waitingreserves.pl, GetMemberDetails is called to retrieve the name of the patron, nothing to change * in members/moremember.pl, GetMemberDetails is called to retrieve name, surname, borrowernumber, we change nothing * in members/deletemem.pl, GetMemberDetails is followed by a $flag = $bor->{flags}, replacing by a call to GetMemberDetails * in members/messaging.pl, GetMemberDetails is called to retrieve name, category, nothing to change * members/member-flags.pl is here for permissions, nothing to deal with patron flags, nothing to change * opac/opac-passwd.pl, nothing related to flags, nothing to change * opac/opac-ics.pl, nothing related to flags, nothing to change * opac/opac-renew.pl, used to retrieve only branchcode, nothing to change * opac/opac-account.pl, nothing to change, the core part of this script is to display fines that are retrieved by GetMemberAccountRecords * opac/opac-reserve.pl, nothing related to flags, nothing to change * opac/opac-userdetail.pl, script that retrieve only a few dates and other infos from GetMemberDetails, nothing to change * opac/opac-showreviews.pl, retrieve only name & email from GetMemberDetails, nothing to change * opac/SCO/sco-main.pl, calls canbookbeissued, that need flags, so retrieving them * opac/opac-readingrecord.pl, use GetMemberDetails to retrieve patron name, address... nothing to change * opac/opac-messaging.pl, use GetMemberDetails to retrieve sms informations, nothing to change * opac/opac-userupdate.pl, use GetMemberDetails to display member information that can be updated by the patron, nothing related to flags, nothing to change * opac/opac-privacy.pl, loaded just to retrieve name & privacy flag info, nothing to change * opac/opac-user.pl, use GetMemberDetails to display patron information, but not flags one, nothing to change * opac/opac-suggestion.pl use GetMemberDetails to retrieve the branchcode, nothing to change * reserve/request.pl use GetMemberDetails to retrieve many informations displayed (name, branchcode,...), but none related to flags. Nothing to change GetMemberDetails also launches 2 SQL queries, the 2nd being superfluous : the enrolmentperiod is already available in the 1st query, just return it ! => C4::Reserves & C4::Overdues are now used only in GetMemberFlags sub, that is not always called, so switching from a use to a require inside the sub C4::Accounts is used only in AddMember and ExtendMemberSubscriptionTo, and only if there is a fee for subscription, switching from use to require sub GetMemberAccountRecords the SQL query has been completed. With a LEFT JOIN, we retrieve directly the title & biblionumber. Thus the call to GetBiblioByItemNumber is now useless and the use C4::Biblio can be removed --- C4/ILSDI/Services.pm | 4 +- C4/Members.pm | 66 +++++++++++--------------------- C4/SIP/ILS/Patron.pm | 2 +- circ/circulation.pl | 4 +- circ/returns.pl | 2 +- members/deletemem.pl | 2 +- opac/sco/sco-main.pl | 2 + t/db_dependent/lib/KohaTest/Members.pm | 2 +- 8 files changed, 32 insertions(+), 52 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 90f741a..762a3fb 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -356,12 +356,12 @@ sub GetPatronInfo { # Get Member details my $borrowernumber = $cgi->param('patron_id'); my $borrower = GetMemberDetails( $borrowernumber ); + my $flags = GetMemberFlags($borrower); return { code => 'PatronNotFound' } unless $$borrower{borrowernumber}; # Cleaning the borrower hashref - $borrower->{'charges'} = $borrower->{'flags'}->{'CHARGES'}->{'amount'}; + $borrower->{'charges'} = $flags->{'CHARGES'}->{'amount'}; $borrower->{'branchname'} = GetBranchName( $borrower->{'branchcode'} ); - delete $borrower->{'flags'}; delete $borrower->{'userid'}; delete $borrower->{'password'}; diff --git a/C4/Members.pm b/C4/Members.pm index 8d4a6c5..781dfc8 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -27,10 +27,6 @@ use C4::Dates qw(format_date_in_iso); use Digest::MD5 qw(md5_base64); use Date::Calc qw/Today Add_Delta_YM/; use C4::Log; # logaction -use C4::Overdues; -use C4::Reserves; -use C4::Accounts; -use C4::Biblio; use C4::SQLHelper qw(InsertInTable UpdateInTable SearchInTable); use C4::Members::Attributes qw(SearchIdMatchingAttribute); @@ -46,6 +42,7 @@ BEGIN { &Search &SearchMember &GetMemberDetails + &GetMemberFlags &GetMemberRelatives &GetMember @@ -325,19 +322,7 @@ up the borrower by number; otherwise, it looks up the borrower by card number. C<$borrower> is a reference-to-hash whose keys are the fields of the -borrowers table in the Koha database. In addition, -C<$borrower-E{flags}> is a hash giving more detailed information -about the patron. Its keys act as flags : - - if $borrower->{flags}->{LOST} { - # Patron's card was reported lost - } - -If the state of a flag means that the patron should not be -allowed to borrow any more books, then it will have a C key -with a true value. - -See patronflags for more details. +borrowers table in the Koha database. C<$borrower-E{authflags}> is a hash giving more detailed information about the top-level permissions flags set for the borrower. For example, @@ -353,11 +338,17 @@ sub GetMemberDetails { my $query; my $sth; if ($borrowernumber) { - $sth = $dbh->prepare("SELECT borrowers.*,category_type,categories.description,reservefee FROM borrowers LEFT JOIN categories ON borrowers.categorycode=categories.categorycode WHERE borrowernumber=?"); + $sth = $dbh->prepare("SELECT borrowers.*,category_type,categories.description,reservefee, enrolmentperiod + FROM borrowers + LEFT JOIN categories ON borrowers.categorycode=categories.categorycode, enrolmentperiod + WHERE borrowernumber=?"); $sth->execute($borrowernumber); } elsif ($cardnumber) { - $sth = $dbh->prepare("SELECT borrowers.*,category_type,categories.description,reservefee FROM borrowers LEFT JOIN categories ON borrowers.categorycode=categories.categorycode WHERE cardnumber=?"); + $sth = $dbh->prepare("SELECT borrowers.*,category_type,categories.description,reservefee, enrolmentperiod + FROM borrowers + LEFT JOIN categories ON borrowers.categorycode=categories.categorycode + WHERE cardnumber=?"); $sth->execute($cardnumber); } else { @@ -366,8 +357,6 @@ sub GetMemberDetails { my $borrower = $sth->fetchrow_hashref; my ($amount) = GetMemberAccountRecords( $borrowernumber); $borrower->{'amountoutstanding'} = $amount; - # FIXME - patronflags calls GetMemberAccountRecords... just have patronflags return $amount - my $flags = patronflags( $borrower); my $accessflagshash; $sth = $dbh->prepare("select bit,flag from userflags"); @@ -377,25 +366,14 @@ sub GetMemberDetails { $accessflagshash->{$flag} = 1; } } - $borrower->{'flags'} = $flags; $borrower->{'authflags'} = $accessflagshash; - - # find out how long the membership lasts - $sth = - $dbh->prepare( - "select enrolmentperiod from categories where categorycode = ?"); - $sth->execute( $borrower->{'categorycode'} ); - my $enrolment = $sth->fetchrow; - $borrower->{'enrolmentperiod'} = $enrolment; return ($borrower); #, $flags, $accessflagshash); } -=head2 patronflags +=head2 GetMemberFlags - $flags = &patronflags($patron); - -This function is not exported. + $flags = &GetMemberFlags($patron); The following will be set where applicable: $flags->{CHARGES}->{amount} Amount of debt @@ -453,8 +431,9 @@ The "message" field that comes from the DB is OK. =cut # TODO: use {anonymous => hashes} instead of a dozen %flaginfo -# FIXME rename this function. -sub patronflags { +sub GetMemberFlags { + require C4::Reserves; + require C4::Overdues; my %flags; my ( $patroninformation) = @_; my $dbh=C4::Context->dbh; @@ -822,7 +801,8 @@ sub AddMember { } if ($enrolmentfee && $enrolmentfee > 0) { # insert fee in patron debts - manualinvoice($data{'borrowernumber'}, '', '', 'A', $enrolmentfee); + require C4::Accounts; + C4::Accounts::manualinvoice($data{'borrowernumber'}, '', '', 'A', $enrolmentfee); } return $data{'borrowernumber'}; @@ -1172,8 +1152,10 @@ sub GetMemberAccountRecords { my @acctlines; my $numlines = 0; my $strsth = qq( - SELECT * + SELECT accountlines.*,biblio.biblionumber,biblio.title FROM accountlines + LEFT JOIN items USING(itemnumber) + LEFT JOIN biblio USING (biblionumber) WHERE borrowernumber=?); my @bind = ($borrowernumber); if ($date && $date ne ''){ @@ -1185,11 +1167,6 @@ sub GetMemberAccountRecords { $sth->execute( @bind ); my $total = 0; while ( my $data = $sth->fetchrow_hashref ) { - if ( $data->{itemnumber} ) { - my $biblio = GetBiblioFromItemNumber( $data->{itemnumber} ); - $data->{biblionumber} = $biblio->{biblionumber}; - $data->{title} = $biblio->{title}; - } $acctlines[$numlines] = $data; $numlines++; $total += int(1000 * $data->{'amountoutstanding'}); # convert float to integer to avoid round-off errors @@ -1763,7 +1740,8 @@ EOF my ($enrolmentfee) = $sth->fetchrow; if ($enrolmentfee && $enrolmentfee > 0) { # insert fee in patron debts - manualinvoice($borrower->{'borrowernumber'}, '', '', 'A', $enrolmentfee); + require C4::Accounts; + C4::Accounts::manualinvoice($borrower->{'borrowernumber'}, '', '', 'A', $enrolmentfee); } return $date if ($sth); return 0; diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 2b0b443..53cac35 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -47,7 +47,7 @@ sub new { $kp = GetMemberDetails(undef,$patron_id); $debug and warn "new Patron (GetMemberDetails): " . Dumper($kp); my $pw = $kp->{password}; ### FIXME - md5hash -- deal with . - my $flags = $kp->{flags}; # or warn "Warning: No flags from patron object for '$patron_id'"; + my $flags = GetMemberFlags($kp); my $debarred = $kp->{debarred}; # 1 if ($kp->{flags}->{DBARRED}->{noissues}); $debug and warn sprintf("Debarred = %s : ", ($debarred||'undef')) . Dumper(%{$kp->{flags}}); my ($day, $month, $year) = (localtime)[3,4,5]; diff --git a/circ/circulation.pl b/circ/circulation.pl index da4878f..9eac4fa 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -517,7 +517,7 @@ if ($borrowerslist) { } #title -my $flags = $borrower->{'flags'}; +my $flags = GetMemberFlags($borrower); foreach my $flag ( sort keys %$flags ) { $template->param( flagged=> 1); $flags->{$flag}->{'message'} =~ s#\n#
#g; @@ -589,7 +589,7 @@ foreach my $flag ( sort keys %$flags ) { } } -my $amountold = $borrower->{flags}->{'CHARGES'}->{'message'} || 0; +my $amountold = $flags->{'CHARGES'}->{'message'} || 0; $amountold =~ s/^.*\$//; # remove upto the $, if any my ( $total, $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); diff --git a/circ/returns.pl b/circ/returns.pl index 6826d7c..6b7eb0a 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -470,7 +470,7 @@ $template->param( errmsgloop => \@errmsgloop ); # patrontable .... if ($borrower) { - my $flags = $borrower->{'flags'}; + my $flags = GetMemberFlags($borrower); my @flagloop; my $flagset; foreach my $flag ( sort keys %$flags ) { diff --git a/members/deletemem.pl b/members/deletemem.pl index b1ba709..1e17f36 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -48,7 +48,7 @@ my $issues = GetPendingIssues($member); # FIXME: wasteful call when really, my $countissues = scalar(@$issues); my ($bor)=GetMemberDetails($member,''); -my $flags=$bor->{flags}; +my $flags=GetMemberFlags($bor); my $userenv = C4::Context->userenv; diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index 2488f67..821d09f 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -95,6 +95,8 @@ if (C4::Context->preference('SelfCheckoutByLogin') && !$patronid) { my ($resval, $patronid) = checkpw($dbh, $patronlogin, $patronpw); } my $borrower = GetMemberDetails(undef,$patronid); +# retrieve flags, they're needed by canbookbeissued sub +$borrower->{flags} = GetMemberFlags($borrower); my $branch = $issuer->{branchcode}; diff --git a/t/db_dependent/lib/KohaTest/Members.pm b/t/db_dependent/lib/KohaTest/Members.pm index ff18869..2362272 100644 --- a/t/db_dependent/lib/KohaTest/Members.pm +++ b/t/db_dependent/lib/KohaTest/Members.pm @@ -14,7 +14,7 @@ sub methods : Test( 1 ) { my $self = shift; my @methods = qw( SearchMember GetMemberDetails - patronflags + GetMemberFlags GetMember GetMemberIssuesAndFines ModMember -- 1.7.4.1