Bugzilla – Attachment 7656 Details for
Bug 6875
de-nesting C4 packages
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[HAS_SOME_PROBLEM]bug 6875 de-nesting C4::Member
bug-6875-de-nesting-C4Member.patch (text/plain), 15.96 KB, created by
Paul Poulain
on 2012-02-15 14:45:54 UTC
(
hide
)
Description:
[HAS_SOME_PROBLEM]bug 6875 de-nesting C4::Member
Filename:
MIME Type:
Creator:
Paul Poulain
Created:
2012-02-15 14:45:54 UTC
Size:
15.96 KB
patch
obsolete
>From 4eea7044041acf2e52f0fa1041435c4d8e8fb9ae Mon Sep 17 00:00:00 2001 >From: Paul Poulain <paul.poulain@biblibre.com> >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 | 58 ++++++++++++-------------------- > 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(+), 44 deletions(-) > >diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm >index 067afd7..e33e0e7 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 b81872c..5ba6190 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 check_date Date_to_Days/; > 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); > >@@ -45,6 +41,7 @@ BEGIN { > push @EXPORT, qw( > &Search > &GetMemberDetails >+ &GetMemberFlags > &GetMemberRelatives > &GetMember > >@@ -285,19 +282,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<gt>{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<noissues> key >-with a true value. >- >-See patronflags for more details. >+borrowers table in the Koha database. > > C<$borrower-E<gt>{authflags}> is a hash giving more detailed information > about the top-level permissions flags set for the borrower. For example, >@@ -313,11 +298,17 @@ sub GetMemberDetails { > my $query; > my $sth; > if ($borrowernumber) { >- $sth = $dbh->prepare("SELECT borrowers.*,category_type,categories.description,reservefee,enrolmentperiod 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,enrolmentperiod 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 { >@@ -326,8 +317,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"); >@@ -337,7 +326,6 @@ sub GetMemberDetails { > $accessflagshash->{$flag} = 1; > } > } >- $borrower->{'flags'} = $flags; > $borrower->{'authflags'} = $accessflagshash; > > # For the purposes of making templates easier, we'll define a >@@ -353,11 +341,9 @@ sub GetMemberDetails { > return ($borrower); #, $flags, $accessflagshash); > } > >-=head2 patronflags >- >- $flags = &patronflags($patron); >+=head2 GetMemberFlags > >-This function is not exported. >+ $flags = &GetMemberFlags($patron); > > The following will be set where applicable: > $flags->{CHARGES}->{amount} Amount of debt >@@ -415,8 +401,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; >@@ -759,7 +746,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'}; >@@ -1113,8 +1101,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 ''){ >@@ -1126,11 +1116,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 >@@ -1703,7 +1688,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); > } > logaction("MEMBERS", "RENEW", $borrower->{'borrowernumber'}, "Membership renewed")if C4::Context->preference("BorrowersLog"); > return $date if ($sth); >diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm >index 95981fb..08b9854 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 e8d22cb..ecb2470 100755 >--- a/circ/circulation.pl >+++ b/circ/circulation.pl >@@ -527,7 +527,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#<br />#g; >@@ -599,7 +599,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 167bc5f..a6e49dc 100755 >--- a/circ/returns.pl >+++ b/circ/returns.pl >@@ -479,7 +479,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 924245a..253d36b 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 7e4f810..d9cb4e0 100755 >--- a/opac/sco/sco-main.pl >+++ b/opac/sco/sco-main.pl >@@ -113,6 +113,8 @@ if (C4::Context->preference('SelfCheckoutByLogin') && !$patronid) { > ($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 5646be1..f7bd522 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( Search > GetMemberDetails >- patronflags >+ GetMemberFlags > GetMember > GetMemberIssuesAndFines > ModMember >-- >1.7.5.4
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 6875
:
5442
|
5443
|
5496
|
5497
|
5498
|
5499
|
5500
|
5501
|
5502
|
5503
|
5504
|
5505
|
5506
|
5507
|
5508
|
5509
|
5510
|
5511
|
5512
|
7359
|
7360
|
7361
|
7362
|
7363
|
7364
|
7365
|
7652
|
7654
|
7655
| 7656 |
7657
|
7658
|
7770
|
7771
|
7808
|
7844
|
8095
|
8133
|
8134
|
8135