From 74dfa54dde0245c3e73b87eae2fb633faef66dd7 Mon Sep 17 00:00:00 2001 From: Clay Fouts Date: Fri, 5 Nov 2010 19:37:40 +0000 Subject: [PATCH] Bug 5408 - Refactor GetIssuingRule and GetLoanLength 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