From f85f505815eb6db69b02d3c05c2a10801238f3e8 Mon Sep 17 00:00:00 2001 From: Srdjan Jankovic Date: Wed, 7 Sep 2011 13:50:48 +1200 Subject: [PATCH] bug_4530: Rework issuing rules Use only one set of functions Database efficient rules fetch - only one query --- C4/Circulation.pm | 312 +++++++++------------------- C4/Members.pm | 41 ++-- C4/Overdues.pm | 40 +---- C4/Reserves.pm | 15 +-- t/db_dependent/lib/KohaTest/Circulation.pm | 4 +- t/db_dependent/lib/KohaTest/Overdues.pm | 1 - 6 files changed, 125 insertions(+), 288 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 47068f9..fb7c4a6 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -74,7 +74,6 @@ BEGIN { &GetItemIssues &GetBorrowerIssues &GetIssuingCharges - &GetIssuingRule &GetBranchBorrowerCircRule &GetBranchItemRule &GetBiblioIssues @@ -356,12 +355,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) "; @@ -1119,56 +1118,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; } @@ -1182,108 +1133,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 + +=cut + +sub GetIssuingRuleValue { + my ( $borrowertype, $itemtype, $branchcode, $rule ) = @_; + my $irule = GetIssuingRuleRecord( $borrowertype, $itemtype, $branchcode, $rule ) + or return; + return $irule->{$rule}; } -=head2 GetIssuingRule +=head2 GetIssuingRuleRecord - my $irule = &GetIssuingRule($borrowertype,$itemtype,branchcode) + my $irule = &GetIssuingRuleRecord($borrowertype,$itemtype,$branchcode, $rule_name) -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 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 -Get the issuing rule for an itemtype, a borrower type and a branch -Returns a hashref from the issuingrules table. +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 ) = @_; + + my $irules = GetIssuingRules($borrowertype,$itemtype,$branchcode) + or return; + + return unless @$irules; + + return $irules->[0] unless $rule; + + die qq{Invalid rule (field) "$rule"} unless exists $irules->[0]->{$rule}; - $sth->execute( $borrowertype, $itemtype, $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + foreach (@$irules) { + return $_ if $_->{$rule}; # XXX defined $_->{$rule} + } - $sth->execute( $borrowertype, "*", $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + return; +} - $sth->execute( "*", $itemtype, $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +=head2 GetIssuingRules - $sth->execute( "*", "*", $branchcode ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; + my $irules = &GetIssuingRules($borrowertype,$itemtype,$branchcode) - $sth->execute( $borrowertype, $itemtype, "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +Get issuing all rule records for an itemtype, a borrower type and a branch +in order of preference - $sth->execute( $borrowertype, "*", "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +Order of preference is $branchcode, $borrowertype, $itemtype - $sth->execute( "*", $itemtype, "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +=cut - $sth->execute( "*", "*", "*" ); - $irule = $sth->fetchrow_hashref; - return $irule if defined($irule) ; +sub GetIssuingRules { + my ( $borrowertype, $itemtype, $branchcode ) = @_; + + 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 @@ -2159,47 +2102,37 @@ 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; "); $sthcount->execute( $borrowernumber, $itemnumber ); if ( my $data1 = $sthcount->fetchrow_hashref ) { - - if ( ( $data1->{renewalsallowed} && $data1->{renewalsallowed} > $data1->{renewals} ) || $override_limit ) { - $renewokay = 1; - } - else { - $error="too_many"; - } - my ( $resfound, $resrec ) = C4::Reserves::CheckReserves($itemnumber); if ($resfound) { $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); } @@ -2306,8 +2239,6 @@ sub GetRenewCount { my ( $bornum, $itemno ) = @_; my $dbh = C4::Context->dbh; my $renewcount = 0; - my $renewsallowed = 0; - my $renewsleft = 0; my $borrower = C4::Members::GetMemberDetails($bornum); my $item = GetItem($itemno); @@ -2328,10 +2259,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 ); } @@ -2376,19 +2305,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; } } @@ -2397,43 +2318,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 dcc168b..8cbfbe2 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -660,28 +660,31 @@ sub IsMemberBlocked { my $dbh = C4::Context->dbh; # does patron have current fine days? - my $strsth=qq{ - SELECT - ADDDATE(returndate, finedays * DATEDIFF(returndate,date_due) ) AS blockingdate, - DATEDIFF(ADDDATE(returndate, finedays * DATEDIFF(returndate,date_due)),NOW()) AS blockedcount - FROM old_issues - }; + my $strsth=<preference("item-level_itypes")){ - $strsth.= - qq{ LEFT JOIN items ON (items.itemnumber=old_issues.itemnumber) - LEFT JOIN issuingrules ON (issuingrules.itemtype=items.itype)} + $strsth.=<prepare($strsth); $sth->execute($borrowernumber); my $row = $sth->fetchrow_hashref; diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 28b135c..a8bfe31 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( @@ -246,7 +243,6 @@ sub CalcFine { 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 ). @@ -264,6 +260,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'}; @@ -624,41 +621,6 @@ sub GetFine { return ( $data->{'sum(amountoutstanding)'} ); } - -=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 8373840..d9b1ea1 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -410,18 +410,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 @@ -446,8 +434,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 7d5e69d..59238b1 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 949c670..02b907e 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.6.5