[Koha-patches] [PATCH] Bug 5408 - Refactor GetIssuingRule and GetLoanLength
Clay Fouts
cfouts at liblime.com
Mon Nov 15 09:17:20 CET 2010
This collapses the series of queries within GetIssuingRule into a single query,
the result of which is cached for future uses. Also refactors GetLoanLength to
call GetIssuingRule instead of submitting its own queries. The purpose of this
is to simplify and optimize the code. The API and behavior remain unchanged.
---
C4/Circulation.pm | 139 ++++++--------------
t/Circulation.t | 385 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 425 insertions(+), 99 deletions(-)
create mode 100755 t/Circulation.t
diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 10c3c11..9081c8e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1071,121 +1071,62 @@ sub AddIssue {
return ($datedue); # not necessarily the same as when it came in!
}
-=head2 GetLoanLength
-
- my $loanlength = &GetLoanLength($borrowertype,$itemtype,branchcode)
-
-Get loan length for an itemtype, a borrower type and a branch
-
-=cut
-
-sub GetLoanLength {
- my ( $borrowertype, $itemtype, $branchcode ) = @_;
- my $dbh = C4::Context->dbh;
- my $sth =
- $dbh->prepare(
-"select issuelength from issuingrules where categorycode=? and itemtype=? and branchcode=? and issuelength is not null"
- );
-# warn "in get loan lenght $borrowertype $itemtype $branchcode ";
-# try to find issuelength & return the 1st available.
-# check with borrowertype, itemtype and branchcode, then without one of those parameters
- $sth->execute( $borrowertype, $itemtype, $branchcode );
- my $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( $borrowertype, "*", $branchcode );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( "*", $itemtype, $branchcode );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( "*", "*", $branchcode );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( $borrowertype, $itemtype, "*" );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( $borrowertype, "*", "*" );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( "*", $itemtype, "*" );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- $sth->execute( "*", "*", "*" );
- $loanlength = $sth->fetchrow_hashref;
- return $loanlength->{issuelength}
- if defined($loanlength) && $loanlength->{issuelength} ne 'NULL';
-
- # if no rule is set => 21 days (hardcoded)
- return 21;
-}
-
=head2 GetIssuingRule
- my $irule = &GetIssuingRule($borrowertype,$itemtype,branchcode)
-
-FIXME - This is a copy-paste of GetLoanLength
-as a stop-gap. Do not wish to change API for GetLoanLength
-this close to release, however, Overdues::GetIssuingRules is broken.
-
Get the issuing rule for an itemtype, a borrower type and a branch
Returns a hashref from the issuingrules table.
+my $irule = &GetIssuingRule($categorycode, $itemtype, $branchcode)
+
=cut
-sub GetIssuingRule {
- my ( $borrowertype, $itemtype, $branchcode ) = @_;
- my $dbh = C4::Context->dbh;
- my $sth = $dbh->prepare( "select * from issuingrules where categorycode=? and itemtype=? and branchcode=? and issuelength is not null" );
- my $irule;
+our $irule_cache = undef;
+
+sub _populate_irule_cache() {
+ $irule_cache = C4::Context->dbh->selectall_hashref(
+ 'SELECT * FROM issuingrules WHERE issuelength IS NOT NULL',
+ ['categorycode', 'itemtype', 'branchcode']) or
+ die sprintf "Cannot get issuingrules: %s\n", $!;
+
+ return $irule_cache;
+}
- $sth->execute( $borrowertype, $itemtype, $branchcode );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+sub _clear_irule_cache {
+ return $irule_cache = undef;
+}
- $sth->execute( $borrowertype, "*", $branchcode );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+sub GetIssuingRule($$$) {
+ my ($categorycode, $itemtype, $branchcode) = @_;
+
+ $irule_cache //= _populate_irule_cache();
+ my $irule = $irule_cache->{$categorycode}{$itemtype}{$branchcode} //
+ $irule_cache->{$categorycode}{'*'}{$branchcode} //
+ $irule_cache->{'*'}{$itemtype}{$branchcode} //
+ $irule_cache->{'*'}{'*'}{$branchcode} //
+ $irule_cache->{$categorycode}{$itemtype}{'*'} //
+ $irule_cache->{$categorycode}{'*'}{'*'} //
+ $irule_cache->{'*'}{$itemtype}{'*'} //
+ $irule_cache->{'*'}{'*'}{'*'} //
+ undef;
+
+ return $irule;
+}
- $sth->execute( "*", $itemtype, $branchcode );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+=head2 GetLoanLength
- $sth->execute( "*", "*", $branchcode );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+Get loan length for an itemtype, a borrower type and a branch
- $sth->execute( $borrowertype, $itemtype, "*" );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+my $loanlength = &GetLoanLength($categorycode, $itemtype, $branchcode)
- $sth->execute( $borrowertype, "*", "*" );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+=cut
- $sth->execute( "*", $itemtype, "*" );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+sub GetLoanLength($$$) {
+ my ($categorycode, $itemtype, $branchcode) = @_;
- $sth->execute( "*", "*", "*" );
- $irule = $sth->fetchrow_hashref;
- return $irule if defined($irule) ;
+ my $irule = GetIssuingRule($categorycode, $itemtype, $branchcode);
+ my $loanlength = ($irule) ? $irule->{issuelength} : 21;
- # if no rule matches,
- return undef;
+ return $loanlength;
}
=head2 GetBranchBorrowerCircRule
diff --git a/t/Circulation.t b/t/Circulation.t
new file mode 100755
index 0000000..cd431d0
--- /dev/null
+++ b/t/Circulation.t
@@ -0,0 +1,385 @@
+#!/usr/bin/perl
+
+use vars qw($C4::Circulation::irule_cache);
+
+use strict;
+use warnings;
+
+use Test::More tests => 9;
+use YAML;
+
+use_ok('C4::Circulation');
+
+my $irules_yaml = q{
+---
+'':
+ '': {}
+ '*': {}
+'*':
+ '': {}
+ '*':
+ '*':
+ accountsent: ~
+ branchcode: '*'
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 1
+ holdallowed: 2
+ issuelength: 21
+ itemtype: '*'
+ max_fine: 0.000000
+ max_holds: ~
+ maxissueqty: 25
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ APL:
+ accountsent: ~
+ branchcode: APL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.100000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 21
+ itemtype: '*'
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 999
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ LML:
+ accountsent: ~
+ branchcode: LML
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 1
+ holdallowed: 2
+ issuelength: 14
+ itemtype: '*'
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 500
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ MSPL:
+ accountsent: ~
+ branchcode: MSPL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.100000
+ firstremind: 2
+ holdallowed: 2
+ issuelength: 21
+ itemtype: '*'
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 20
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ MVS:
+ accountsent: ~
+ branchcode: MVS
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 1
+ holdallowed: 2
+ issuelength: 14
+ itemtype: '*'
+ max_fine: 20.000000
+ max_holds: ~
+ maxissueqty: 5
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ DVD:
+ APL:
+ accountsent: ~
+ branchcode: APL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.250000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 7
+ itemtype: DVD
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 999
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ VHS:
+ APL:
+ accountsent: ~
+ branchcode: APL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.250000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 7
+ itemtype: VHS
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 999
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ BPL:
+ accountsent: ~
+ branchcode: BPL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.500000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 3
+ itemtype: VHS
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 2
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ MSPL:
+ accountsent: ~
+ branchcode: MSPL
+ categorycode: '*'
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.500000
+ firstremind: 2
+ holdallowed: 2
+ issuelength: 5
+ itemtype: VHS
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 5
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+STUD:
+ '*':
+ LML:
+ accountsent: ~
+ branchcode: LML
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 1
+ holdallowed: 2
+ issuelength: 28
+ itemtype: '*'
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 500
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ MVS:
+ accountsent: ~
+ branchcode: MVS
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 1
+ holdallowed: 2
+ issuelength: 7
+ itemtype: '*'
+ max_fine: 20.000000
+ max_holds: ~
+ maxissueqty: 2
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ WELD:
+ accountsent: ~
+ branchcode: WELD
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.000000
+ firstremind: 3
+ holdallowed: 2
+ issuelength: 7
+ itemtype: '*'
+ max_fine: 50.000000
+ max_holds: ~
+ maxissueqty: 2
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ YPL:
+ accountsent: ~
+ branchcode: YPL
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.150000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 21
+ itemtype: '*'
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 6
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ DVD:
+ YPL:
+ accountsent: ~
+ branchcode: YPL
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 1.000000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 7
+ itemtype: DVD
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 2
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ PERIODICAL:
+ YPL:
+ accountsent: ~
+ branchcode: YPL
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.150000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 7
+ itemtype: PERIODICAL
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 5
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ VHS:
+ YPL:
+ accountsent: ~
+ branchcode: YPL
+ categorycode: STUD
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 1.000000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 7
+ itemtype: VHS
+ max_fine: 500.000000
+ max_holds: ~
+ maxissueqty: 2
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+VISITOR:
+ '*':
+ MSPL:
+ accountsent: ~
+ branchcode: MSPL
+ categorycode: VISITOR
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.100000
+ firstremind: 2
+ holdallowed: 2
+ issuelength: 21
+ itemtype: '*'
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 3
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ DVD:
+ MSPL:
+ accountsent: ~
+ branchcode: MSPL
+ categorycode: VISITOR
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.500000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 5
+ itemtype: DVD
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 1
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+ VHS:
+ MSPL:
+ accountsent: ~
+ branchcode: MSPL
+ categorycode: VISITOR
+ chargename: ~
+ chargeperiod: 1
+ circ_termsets_id: ~
+ fine: 0.500000
+ firstremind: 0
+ holdallowed: 2
+ issuelength: 5
+ itemtype: VHS
+ max_fine: 5.000000
+ max_holds: ~
+ maxissueqty: 1
+ rentaldiscount: ~
+ reservecharge: ~
+ restrictedtype: ~
+};
+
+$C4::Circulation::irule_cache = Load($irules_yaml);
+
+is(C4::Circulation::GetLoanLength('VISITOR', 'DVD', 'MSPL'), 5, 'GetLoanLength 1');
+is(C4::Circulation::GetLoanLength('VISITOR', 'VHS', undef), 21, 'GetLoanLength 2');
+is(C4::Circulation::GetLoanLength('STUD', 'invalid', 'MVS'), 7, 'GetLoanLength 3');
+is(C4::Circulation::GetLoanLength('STUD', undef, 'MVS'), 7, 'GetLoanLength 4');
+is(C4::Circulation::GetLoanLength(undef, undef, 'APL'), 21, 'GetLoanLength 5');
+is(C4::Circulation::GetLoanLength(undef, 'DVD', 'APL'), 7, 'GetLoanLength 6');
+is(C4::Circulation::GetLoanLength(undef, undef, 'LML'), 14, 'GetLoanLength 7');
+is(C4::Circulation::GetLoanLength(undef, undef, undef), 21, 'GetLoanLength 8');
--
1.5.6.5
More information about the Koha-patches
mailing list