From e473c22cf5fa690abe38c010355373d13b02aa3b Mon Sep 17 00:00:00 2001 From: Srdjan Jankovic Date: Thu, 22 Dec 2011 13:33:33 +1300 Subject: [PATCH] bug_4530: Rework issuing rules Use only one set of functions Database efficient rules fetch - only one query Signed-off-by: Kyle M Hall --- C4/Circulation.pm | 318 +++++++++------------------- C4/Members.pm | 1 - C4/Overdues.pm | 42 +---- C4/Reserves.pm | 15 +-- t/db_dependent/lib/KohaTest/Circulation.pm | 4 +- t/db_dependent/lib/KohaTest/Overdues.pm | 1 - 6 files changed, 109 insertions(+), 272 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index e68db64..9363ad9 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -75,7 +75,6 @@ BEGIN { &GetItemIssue &GetItemIssues &GetIssuingCharges - &GetIssuingRule &GetBranchBorrowerCircRule &GetBranchItemRule &GetBiblioIssues @@ -357,12 +356,12 @@ sub TooMany { # given branch, patron category, and item type, determine # applicable issuing rule - my $issuing_rule = GetIssuingRule($cat_borrower, $type, $branch); + my $issuing_rule = GetIssuingRuleRecord($cat_borrower, $type, $branch, 'maxissueqty'); # if a rule is found and has a loan limit set, count # how many loans the patron already has that meet that # rule - if (defined($issuing_rule) and defined($issuing_rule->{'maxissueqty'})) { + if ( $issuing_rule ) { my @bind_params; my $count_query = "SELECT COUNT(*) FROM issues JOIN items USING (itemnumber) "; @@ -1094,56 +1093,8 @@ Get loan length for an itemtype, a borrower type and a branch 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; + return GetIssuingRuleValue( $borrowertype, $itemtype, $branchcode, 'issuelength' ) || 21; } @@ -1157,108 +1108,100 @@ Get the Hard Due Date and it's comparison for an itemtype, a borrower type and a sub GetHardDueDate { my ( $borrowertype, $itemtype, $branchcode ) = @_; - my $dbh = C4::Context->dbh; - my $sth = - $dbh->prepare( -"select hardduedate, hardduedatecompare from issuingrules where categorycode=? and itemtype=? and branchcode=?" - ); - $sth->execute( $borrowertype, $itemtype, $branchcode ); - my $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( $borrowertype, "*", $branchcode ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( "*", $itemtype, $branchcode ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( "*", "*", $branchcode ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( $borrowertype, $itemtype, "*" ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( $borrowertype, "*", "*" ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( "*", $itemtype, "*" ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - $sth->execute( "*", "*", "*" ); - $results = $sth->fetchrow_hashref; - return (C4::Dates->new($results->{hardduedate}, 'iso'),$results->{hardduedatecompare}) - if defined($results) && $results->{hardduedate} ne 'NULL'; - - # if no rule is set => return undefined - return (undef, undef); + my $issuing_rule = GetIssuingRuleRecord($borrowertype, $itemtype, $branchcode, 'hardduedate') + or return; + return (C4::Dates->new($issuing_rule->{hardduedate}, 'iso'),$issuing_rule->{hardduedatecompare}); } + + +=head2 GetIssuingRuleValue + + my $irule = &GetIssuingRuleValue($borrowertype,$itemtype,$branchcode, $rule_name) + +Get the most specific non-null value issuing rule for an itemtype, a borrower type and a branch -=head2 GetIssuingRule +=cut + +sub GetIssuingRuleValue { + my ( $borrowertype, $itemtype, $branchcode, $rule ) = @_; + my $irule = GetIssuingRuleRecord( $borrowertype, $itemtype, $branchcode, $rule ) + or return; + return $irule->{$rule}; +} - my $irule = &GetIssuingRule($borrowertype,$itemtype,branchcode) +=head2 GetIssuingRuleRecord -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. + my $irule = &GetIssuingRuleRecord($borrowertype,$itemtype,$branchcode, $rule_name) -Get the issuing rule for an itemtype, a borrower type and a branch -Returns a hashref from the issuingrules table. +Get the most specific non-null value issuing rule for an itemtype, a borrower type and a branch +If $rule_name is specified, only records with $rule_name not null are taken in account + +Order of preference is $branchcode, $borrowertype, $itemtype =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; +sub GetIssuingRuleRecord { + my ( $borrowertype, $itemtype, $branchcode, $rule ) = @_; - $sth->execute( $borrowertype, $itemtype, $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + my $irules = GetIssuingRules($borrowertype,$itemtype,$branchcode) + or return; - $sth->execute( $borrowertype, "*", $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + return unless @$irules; - $sth->execute( "*", $itemtype, $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + return $irules->[0] unless $rule; - $sth->execute( "*", "*", $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + die qq{Invalid rule (field) "$rule"} unless exists $irules->[0]->{$rule}; - $sth->execute( $borrowertype, $itemtype, "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + foreach (@$irules) { + return $_ if $_->{$rule}; # XXX defined $_->{$rule} + } + + return; +} - $sth->execute( $borrowertype, "*", "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +=head2 GetIssuingRules - $sth->execute( "*", $itemtype, "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + my $irules = &GetIssuingRules($borrowertype,$itemtype,$branchcode) + +Get issuing all rule records for an itemtype, a borrower type and a branch +in order of preference + +Order of preference is $branchcode, $borrowertype, $itemtype + +=cut + +sub GetIssuingRules { + my ( $borrowertype, $itemtype, $branchcode ) = @_; - $sth->execute( "*", "*", "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + my (@where, @params); - # if no rule matches, - return undef; + if ( $borrowertype ) { + push @where, "categorycode=? OR categorycode='*'"; + push @params, $borrowertype; + } else { + push @where, "categorycode='*'"; + } + + if ( $itemtype ) { + push @where, "itemtype=? OR itemtype='*'"; + push @params, $itemtype; + } else { + push @where, "itemtype='*'"; + } + + if ( $branchcode ) { + push @where, "branchcode=? OR branchcode='*'"; + push @params, $branchcode; + } else { + push @where, "branchcode='*'"; + } + + my $qry = "select * from issuingrules where " + .join( ' AND ', map "($_)", @where ) + ." order by branchcode desc, categorycode desc, itemtype desc"; + + my $dbh = C4::Context->dbh; + return $dbh->selectall_arrayref($qry, { Slice => {} }, @params); } =head2 GetBranchBorrowerCircRule @@ -1758,18 +1701,16 @@ sub _FixFineDaysOnReturn { } my $branchcode = _GetCircControlBranch( $item, $borrower ); + + my $finedays = GetIssuingRuleValue( $borrower->{categorycode}, $item->{itype}, $branchcode, 'finedays' ) + or return; + my $grace = GetIssuingRuleValue( $borrower->{categorycode}, $item->{itype}, $branchcode, 'firstremind' ); + my $calendar = C4::Calendar->new( branchcode => $branchcode ); my $today = C4::Dates->new(); - - my $deltadays = $calendar->daysBetween( $datedue, C4::Dates->new() ); + my $deltadays = $calendar->daysBetween( $datedue, $today ); my $circcontrol = C4::Context::preference('CircControl'); - my $issuingrule = GetIssuingRule( $borrower->{categorycode}, $item->{itype}, $branchcode ); - my $finedays = $issuingrule->{finedays}; - - # exit if no finedays defined - return unless $finedays; - my $grace = $issuingrule->{firstremind}; if ( $deltadays - $grace > 0 ) { my @newdate = Add_Delta_Days( Today(), $deltadays * $finedays ); @@ -2200,27 +2141,16 @@ sub CanBookBeRenewed { my $sthcount = $dbh->prepare(" SELECT - borrowers.categorycode, biblioitems.itemtype, issues.renewals, renewalsallowed, $controlbranch - FROM issuingrules, - issues + borrowers.categorycode, biblioitems.itemtype, issues.renewals, $controlbranch + FROM issues LEFT JOIN items USING (itemnumber) LEFT JOIN borrowers USING (borrowernumber) LEFT JOIN biblioitems USING (biblioitemnumber) WHERE - (issuingrules.categorycode = borrowers.categorycode OR issuingrules.categorycode = '*') - AND - (issuingrules.itemtype = $itype OR issuingrules.itemtype = '*') - AND - (issuingrules.branchcode = $controlbranch OR issuingrules.branchcode = '*') - AND borrowernumber = ? AND itemnumber = ? - ORDER BY - issuingrules.categorycode desc, - issuingrules.itemtype desc, - issuingrules.branchcode desc LIMIT 1; "); @@ -2239,8 +2169,17 @@ sub CanBookBeRenewed { $renewokay = 0; $error="on_reserve" } + elsif ( $override_limit ) { + $renewokay = 1; + } + elsif ( my $renewalsallowed = GetIssuingRuleValue($data1->{categorycode}, $itype, $data1->{$controlbranch}, 'renewalsallowed') ) { + if ( $renewalsallowed > $data1->{renewals} || $override_limit ) { + $renewokay = 1; + } + } + $error="too_many" unless $renewokay; + } - } return ($renewokay,$error); } @@ -2347,8 +2286,6 @@ sub GetRenewCount { my ( $bornum, $itemno ) = @_; my $dbh = C4::Context->dbh; my $renewcount = 0; - my $renewsallowed = 0; - my $renewsleft = 0; my $borrower = C4::Members::GetMember( borrowernumber => $bornum); my $item = GetItem($itemno); @@ -2369,10 +2306,8 @@ sub GetRenewCount { # $item and $borrower should be calculated my $branchcode = _GetCircControlBranch($item, $borrower); - my $issuingrule = GetIssuingRule($borrower->{categorycode}, $item->{itype}, $branchcode); - - $renewsallowed = $issuingrule->{'renewalsallowed'}; - $renewsleft = $renewsallowed - $renewcount; + my $renewsallowed = GetIssuingRuleValue($borrower->{categorycode}, $item->{itype}, $branchcode, 'renewalsallowed') || 0; + my $renewsleft = $renewsallowed - $renewcount; if($renewsleft < 0){ $renewsleft = 0; } return ( $renewcount, $renewsallowed, $renewsleft ); } @@ -2417,19 +2352,11 @@ sub GetIssuingCharges { $item_type = $item_data->{itemtype}; $charge = $item_data->{rentalcharge}; my $branch = C4::Branch::mybranch(); - my $discount_query = q|SELECT rentaldiscount, - issuingrules.itemtype, issuingrules.branchcode + my $borr_query = q|SELECT borrowers.categorycode FROM borrowers - LEFT JOIN issuingrules ON borrowers.categorycode = issuingrules.categorycode - WHERE borrowers.borrowernumber = ? - AND (issuingrules.itemtype = ? OR issuingrules.itemtype = '*') - AND (issuingrules.branchcode = ? OR issuingrules.branchcode = '*')|; - my $discount_sth = $dbh->prepare($discount_query); - $discount_sth->execute( $borrowernumber, $item_type, $branch ); - my $discount_rules = $discount_sth->fetchall_arrayref({}); - if (@{$discount_rules}) { - # We may have multiple rules so get the most specific - my $discount = _get_discount_from_rule($discount_rules, $branch, $item_type); + WHERE borrowers.borrowernumber = ?|; + my $borr_cat = $dbh->selectrow_array($borr_query, undef, $borrowernumber); + if ( my $discount = GetIssuingRuleValue($borr_cat, $item_type, $branch, 'rentaldiscount') ) { $charge = ( $charge * ( 100 - $discount ) ) / 100; } } @@ -2438,43 +2365,6 @@ sub GetIssuingCharges { return ( $charge, $item_type ); } -# Select most appropriate discount rule from those returned -sub _get_discount_from_rule { - my ($rules_ref, $branch, $itemtype) = @_; - my $discount; - - if (@{$rules_ref} == 1) { # only 1 applicable rule use it - $discount = $rules_ref->[0]->{rentaldiscount}; - return (defined $discount) ? $discount : 0; - } - # could have up to 4 does one match $branch and $itemtype - my @d = grep { $_->{branchcode} eq $branch && $_->{itemtype} eq $itemtype } @{$rules_ref}; - if (@d) { - $discount = $d[0]->{rentaldiscount}; - return (defined $discount) ? $discount : 0; - } - # do we have item type + all branches - @d = grep { $_->{branchcode} eq q{*} && $_->{itemtype} eq $itemtype } @{$rules_ref}; - if (@d) { - $discount = $d[0]->{rentaldiscount}; - return (defined $discount) ? $discount : 0; - } - # do we all item types + this branch - @d = grep { $_->{branchcode} eq $branch && $_->{itemtype} eq q{*} } @{$rules_ref}; - if (@d) { - $discount = $d[0]->{rentaldiscount}; - return (defined $discount) ? $discount : 0; - } - # so all and all (surely we wont get here) - @d = grep { $_->{branchcode} eq q{*} && $_->{itemtype} eq q{*} } @{$rules_ref}; - if (@d) { - $discount = $d[0]->{rentaldiscount}; - return (defined $discount) ? $discount : 0; - } - # none of the above - return 0; -} - =head2 AddIssuingCharge &AddIssuingCharge( $itemno, $borrowernumber, $charge ) diff --git a/C4/Members.pm b/C4/Members.pm index b81872c..f12c855 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -626,7 +626,6 @@ sub IsMemberBlocked { my $dbh = C4::Context->dbh; my $blockeddate = CheckBorrowerDebarred($borrowernumber); - return ( 1, $blockeddate ) if $blockeddate; # if he have late issues diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 660e10b..ef97f87 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -72,9 +72,6 @@ BEGIN { push @EXPORT, qw( &GetIssuesIteminfo ); - # - # &GetIssuingRules - delete. - # use C4::Circulation::GetIssuingRule instead. # subs to move to Members.pm push @EXPORT, qw( @@ -272,9 +269,6 @@ sub CalcFine { my $dbh = C4::Context->dbh; my $amount = 0; my $daystocharge; - # get issuingrules (fines part will be used) - $debug and warn sprintf("CalcFine calling GetIssuingRule(%s, %s, %s)", $bortype, $item->{'itemtype'}, $branchcode); - my $data = C4::Circulation::GetIssuingRule($bortype, $item->{'itemtype'}, $branchcode); if($difference) { # if $difference is supplied, the difference has already been calculated, but we still need to adjust for the calendar. # use copy-pasted functions from calendar module. (deprecated -- these functions will be removed from C4::Overdues ). @@ -292,6 +286,7 @@ sub CalcFine { } } # correct for grace period. + my $data = C4::Circulation::GetIssuingRuleRecord($bortype, $item->{'itemtype'}, $branchcode); my $days_minus_grace = $daystocharge - $data->{'firstremind'}; if ($data->{'chargeperiod'} > 0 && $days_minus_grace > 0 ) { $amount = int($daystocharge / $data->{'chargeperiod'}) * $data->{'fine'}; @@ -655,41 +650,6 @@ sub GetFine { return 0; } - -=head2 GetIssuingRules - -FIXME - This sub should be deprecated and removed. -It ignores branch and defaults. - - $data = &GetIssuingRules($itemtype,$categorycode); - -Looks up for all issuingrules an item info - -C<$itemnumber> is a reference-to-hash whose keys are all of the fields -from the borrowers and categories tables of the Koha database. Thus, - -C<$categorycode> contains information about borrowers category - -C<$data> contains all information about both the borrower and -category he or she belongs to. -=cut - -sub GetIssuingRules { - warn "GetIssuingRules is deprecated: use GetIssuingRule from C4::Circulation instead."; - my ($itemtype,$categorycode)=@_; - my $dbh = C4::Context->dbh(); - my $query=qq|SELECT * - FROM issuingrules - WHERE issuingrules.itemtype=? - AND issuingrules.categorycode=? - |; - my $sth = $dbh->prepare($query); - # print $query; - $sth->execute($itemtype,$categorycode); - return $sth->fetchrow_hashref; -} - - sub ReplacementCost2 { my ( $itemnum, $borrowernumber ) = @_; my $dbh = C4::Context->dbh(); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 359bbad..f23d63a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -418,18 +418,6 @@ sub CanItemBeReserved{ my $item = GetItem($itemnumber); my $borrower = C4::Members::GetMember('borrowernumber'=>$borrowernumber); - # we retrieve user rights on this itemtype and branchcode - my $sth = $dbh->prepare("SELECT categorycode, itemtype, branchcode, reservesallowed - FROM issuingrules - WHERE (categorycode in (?,'*') ) - AND (itemtype IN (?,'*')) - AND (branchcode IN (?,'*')) - ORDER BY - categorycode DESC, - itemtype DESC, - branchcode DESC;" - ); - my $querycount ="SELECT count(*) as count FROM reserves @@ -454,8 +442,7 @@ sub CanItemBeReserved{ } # we retrieve rights - $sth->execute($categorycode, $itemtype, $branchcode); - if(my $rights = $sth->fetchrow_hashref()){ + if(my $rights = C4::Circulation::GetIssuingRuleRecord($categorycode, $itemtype, $branchcode, 'reservesallowed')){ $itemtype = $rights->{itemtype}; $allowedreserves = $rights->{reservesallowed}; }else{ diff --git a/t/db_dependent/lib/KohaTest/Circulation.pm b/t/db_dependent/lib/KohaTest/Circulation.pm index b3a1ff8..106c1ba 100644 --- a/t/db_dependent/lib/KohaTest/Circulation.pm +++ b/t/db_dependent/lib/KohaTest/Circulation.pm @@ -20,7 +20,9 @@ sub methods : Test( 1 ) { CanBookBeIssued AddIssue GetLoanLength - GetIssuingRule + GetIssuingRules + GetIssuingRuleRecord + GetIssuingRuleValue GetBranchBorrowerCircRule AddReturn MarkIssueReturned diff --git a/t/db_dependent/lib/KohaTest/Overdues.pm b/t/db_dependent/lib/KohaTest/Overdues.pm index 13eb1f2..07d9d41 100644 --- a/t/db_dependent/lib/KohaTest/Overdues.pm +++ b/t/db_dependent/lib/KohaTest/Overdues.pm @@ -23,7 +23,6 @@ sub methods : Test( 1 ) { BorType ReplacementCost GetFine - GetIssuingRules ReplacementCost2 GetNextIdNotify NumberNotifyId -- 1.7.2.5