Attachment 2752 Details for Bug 5408 – Patch to refactor GetIssuingRule

Patch to refactor GetIssuingRule

0001-Bug-5408-Refactor-GetIssuingRule-and-GetLoanLength.patch (text/plain), 15.20 KB, created by Clay Fouts on 2010-11-15 08:23:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Clay Fouts

Created: 2010-11-15 08:23:46 UTC

Size: 15.20 KB

patch

obsolete

>From 74dfa54dde0245c3e73b87eae2fb633faef66dd7 Mon Sep 17 00:00:00 2001
>From: Clay Fouts <cfouts@liblime.com>
>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
>

Actions: View

Attachments on bug 5408: 2752