From 9080b1c646c59ebc33444d0b5655d0b886a51e6f Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 29 May 2013 08:14:48 -0400 Subject: [PATCH] Bug 10374 - Clean up and unify CircControl related code As discussed in bug 10262, code related to the CircControl should be unified under C4::Circulation::GetCircControlBranch. Test Plan: 1) Apply this patch 2) Find some overdue items 2) Test the different settings for CircControl setting the system preference, returning an item, and verifying the correct rule was used. --- C4/Circulation.pm | 102 ++++++++++++++++++++++++------------------ C4/Reserves.pm | 2 +- misc/cronjobs/fines.pl | 13 +++--- reserve/request.pl | 2 +- t/db_dependent/Circulation.t | 24 +++++----- 5 files changed, 79 insertions(+), 64 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 30cbe2f..948afe6 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -94,6 +94,7 @@ BEGIN { push @EXPORT, qw( &AddReturn &MarkIssueReturned + &GetCircControlBranch ); # subs to deal with transfers @@ -375,7 +376,7 @@ sub TooMany { my $dbh = C4::Context->dbh; my $branch; # Get which branchcode we need - $branch = _GetCircControlBranch($item,$borrower); + $branch = GetCircControlBranch($item,$borrower); my $type = (C4::Context->preference('item-level_itypes')) ? $item->{'itype'} # item-level : $item->{'itemtype'}; # biblio-level @@ -432,13 +433,19 @@ sub TooMany { push @bind_params, $borrower->{'borrowernumber'}; my $rule_branch = $issuing_rule->{branchcode}; if ($rule_branch ne "*") { + # FIXME: This code replicates the behavior of GetCircControlBranch, + # but returns only the relevent field, rather than the value. It would be better + # to allow GetCircControl to be able to return the revelent field so we aren't repeating + # ourselves here. Or, possibly create GetCircControlBranchField which GetCircControlBranch + # would then use, and we could also call here. if (C4::Context->preference('CircControl') eq 'PickupLibrary') { $count_query .= " AND issues.branchcode = ? "; push @bind_params, $branch; } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') { ; # if branch is the patron's home branch, then count all loans by patron } else { - $count_query .= " AND items.homebranch = ? "; + my $branchfield = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; + $count_query .= " AND items.$branchfield = ? "; push @bind_params, $branch; } } @@ -462,13 +469,15 @@ sub TooMany { WHERE borrowernumber = ? "; push @bind_params, $borrower->{borrowernumber}; + # FIXME: As above, this code replicates the behavior of GetCircControlBranch. if (C4::Context->preference('CircControl') eq 'PickupLibrary') { $branch_count_query .= " AND issues.branchcode = ? "; push @bind_params, $branch; } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') { ; # if branch is the patron's home branch, then count all loans by patron } else { - $branch_count_query .= " AND items.homebranch = ? "; + my $branchfield = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; + $count_query .= " AND items.$branchfield = ? "; push @bind_params, $branch; } my $branch_count_sth = $dbh->prepare($branch_count_query); @@ -715,7 +724,7 @@ sub CanBookBeIssued { unless ( $duedate ) { my $issuedate = $now->clone(); - my $branch = _GetCircControlBranch($item,$borrower); + my $branch = GetCircControlBranch($item,$borrower); my $itype = ( C4::Context->preference('item-level_itypes') ) ? $item->{'itype'} : $biblioitem->{'itemtype'}; $duedate = CalcDateDue( $issuedate, $itype, $branch, $borrower ); @@ -1097,7 +1106,7 @@ sub CanBookBeReturned { sub checkHighHolds { my ( $item, $borrower ) = @_; my $biblio = GetBiblioFromItemNumber( $item->{itemnumber} ); - my $branch = _GetCircControlBranch( $item, $borrower ); + my $branch = GetCircControlBranch( $item, $borrower ); my $dbh = C4::Context->dbh; my $sth = $dbh->prepare( 'select count(borrowernumber) as num_holds from reserves where biblionumber=?' @@ -1188,7 +1197,7 @@ sub AddIssue { if ($borrower and $barcode and $barcodecheck ne '0'){#??? wtf # find which item we issue my $item = GetItem('', $barcode) or return; # if we don't get an Item, abort. - my $branch = _GetCircControlBranch($item,$borrower); + my $branch = GetCircControlBranch($item,$borrower); # get actual issuing if there is one my $actualissue = GetItemIssue( $item->{itemnumber}); @@ -1774,29 +1783,21 @@ sub AddReturn { if ($doreturn) { my $datedue = $issue->{date_due}; $borrower or warn "AddReturn without current borrower"; - my $circControlBranch; if ($dropbox) { - # define circControlBranch only if dropbox mode is set # don't allow dropbox mode to create an invalid entry in issues (issuedate > today) # FIXME: check issuedate > returndate, factoring in holidays - #$circControlBranch = _GetCircControlBranch($item,$borrower) unless ( $item->{'issuedate'} eq C4::Dates->today('iso') );; - $circControlBranch = _GetCircControlBranch($item,$borrower); - $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $today ) == -1 ? 1 : 0; + $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $today ) == -1 ? 1 : 0; } if ($borrowernumber) { - if( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'}){ - # we only need to calculate and change the fines if we want to do that on return - # Should be on for hourly loans - my $control = C4::Context->preference('CircControl'); - my $control_branchcode = - ( $control eq 'ItemHomeLibrary' ) ? $item->{homebranch} - : ( $control eq 'PatronLibrary' ) ? $borrower->{branchcode} - : $issue->{branchcode}; + my $circ_control_branchcode = + GetCircControlBranch( $item, $borrower, $issue ); + + if ( $issue->{'overdue'} ) { my ( $amount, $type, $unitcounttotal ) = C4::Overdues::CalcFine( $item, $borrower->{categorycode}, - $control_branchcode, $datedue, $today ); + $circ_control_branchcode, $datedue, $today ); $type ||= q{}; @@ -1810,7 +1811,7 @@ sub AddReturn { } MarkIssueReturned( $borrowernumber, $item->{'itemnumber'}, - $circControlBranch, '', $borrower->{'privacy'} ); + $circ_control_branchcode, '', $borrower->{'privacy'} ); # FIXME is the "= 1" right? This could be the borrower hash. $messages->{'WasReturned'} = 1; @@ -2019,7 +2020,7 @@ Should only be called for overdue returns sub _debar_user_on_return { my ( $borrower, $item, $dt_due, $dt_today ) = @_; - my $branchcode = _GetCircControlBranch( $item, $borrower ); + my $branchcode = GetCircControlBranch( $item, $borrower ); my $calendar = Koha::Calendar->new( branchcode => $branchcode ); # $deltadays is a DateTime::Duration object @@ -2215,40 +2216,55 @@ sub _FixAccountForLostAndReturned { return; } -=head2 _GetCircControlBranch - - my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower); +=head2 GetCircControlBranch -Internal function : + my $circ_control_branch = GetCircControlBranch( $item, $borrower[, $issue ] ); Return the library code to be used to determine which circulation policy applies to a transaction. Looks up the CircControl and HomeOrHoldingBranch system preferences. -C<$iteminfos> is a hashref to iteminfo. Only {homebranch or holdingbranch} is used. +C<$item> is a hashref for an item. Only {homebranch or holdingbranch} is used. + +C<$borrower> is a hashref for a borrower. Only {branchcode} is used. + +C<$issue> is a hashref for an issue. Only {branchcode} is used. -C<$borrower> is a hashref to borrower. Only {branchcode} is used. +If CircControl is set to "PickupLibrary", GetCircControlBranch returns the currently logged in library branchcode. + +If CircControl is set to 'PatronLibrary', GetCircControlBranch returns the patron's home library branchcode. + +If CircControl is set to 'ItemHomeLibrary', and $issue is defined, it will return the branchcode for the issue, +othereise it uses the system preference HomeOrHoldingBranch to return either the item's homebranch or its holdingbranch. =cut -sub _GetCircControlBranch { - my ($item, $borrower) = @_; +sub GetCircControlBranch { + my ( $item, $borrower, $issue ) = @_; my $circcontrol = C4::Context->preference('CircControl'); my $branch; - if ($circcontrol eq 'PickupLibrary' and (C4::Context->userenv and C4::Context->userenv->{'branch'}) ) { - $branch= C4::Context->userenv->{'branch'}; - } elsif ($circcontrol eq 'PatronLibrary') { - $branch=$borrower->{branchcode}; - } else { - my $branchfield = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; - $branch = $item->{$branchfield}; - # default to item home branch if holdingbranch is used - # and is not defined - if (!defined($branch) && $branchfield eq 'holdingbranch') { - $branch = $item->{homebranch}; + if ( $circcontrol eq 'PickupLibrary' + && C4::Context->userenv + && C4::Context->userenv->{branch} ) + { + $branch = C4::Context->userenv->{branch}; + } + elsif ( $circcontrol eq 'PatronLibrary' ) { + $branch = $borrower->{branchcode}; + } + elsif ( $circcontrol eq 'ItemHomeLibrary' ) { + if ($issue) { + $branch = $issue->{branchcode}; + } + else { + my $branch_field = C4::Context->preference('HomeOrHoldingBranch'); + $branch_field ||= 'homebranch'; + + $branch = $item->{$branch_field} || $item->{homebranch}; } } + return $branch; } @@ -2468,7 +2484,7 @@ sub CanBookBeRenewed { my $item = GetItem($itemnumber) or return; my $itemissue = GetItemIssue($itemnumber) or return; - my $branchcode = _GetCircControlBranch($item, $borrower); + my $branchcode = GetCircControlBranch( $item, $borrower ); my $issuingrule = GetIssuingRule($borrower->{categorycode}, $item->{itype}, $branchcode); @@ -2631,7 +2647,7 @@ sub GetRenewCount { $renewcount = $data->{'renewals'} if $data->{'renewals'}; $sth->finish; # $item and $borrower should be calculated - my $branchcode = _GetCircControlBranch($item, $borrower); + my $branchcode = GetCircControlBranch($item, $borrower); my $issuingrule = GetIssuingRule($borrower->{categorycode}, $item->{itype}, $branchcode); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 5ff12e4..0659313 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -863,7 +863,7 @@ sub CheckReserves { if ( $res->{'priority'} && $res->{'priority'} < $priority ) { my $borrowerinfo=C4::Members::GetMember(borrowernumber => $res->{'borrowernumber'}); my $iteminfo=C4::Items::GetItem($itemnumber); - my $branch=C4::Circulation::_GetCircControlBranch($iteminfo,$borrowerinfo); + my $branch=C4::Circulation::GetCircControlBranch($iteminfo,$borrowerinfo); my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$iteminfo->{'itype'}); next if ($branchitemrule->{'holdallowed'} == 0); next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $borrowerinfo->{'branchcode'})); diff --git a/misc/cronjobs/fines.pl b/misc/cronjobs/fines.pl index 02bde14..80bf9b9 100755 --- a/misc/cronjobs/fines.pl +++ b/misc/cronjobs/fines.pl @@ -32,6 +32,8 @@ use 5.010; use C4::Context; use C4::Overdues; +use C4::Circulation qw( GetCircControlBranch ); +use C4::Items qw( GetItem ); use Getopt::Long; use Carp; use File::Spec; @@ -75,7 +77,6 @@ my @borrower_fields = my @item_fields = qw(itemnumber barcode date_due); my @other_fields = qw(type days_overdue fine); my $libname = C4::Context->preference('LibraryName'); -my $control = C4::Context->preference('CircControl'); my $mode = C4::Context->preference('finesMode'); my $delim = "\t"; # ? C4::Context->preference('delimiter') || "\t"; @@ -100,13 +101,11 @@ for my $overdue ( @{$overdues} ) { "ERROR in Getoverdues : issues.borrowernumber IS NULL. Repair 'issues' table now! Skipping record.\n"; next; } - my $borrower = BorType( $overdue->{borrowernumber} ); - my $branchcode = - ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch} - : ( $control eq 'PatronLibrary' ) ? $borrower->{branchcode} - : $overdue->{branchcode}; -# In final case, CircControl must be PickupLibrary. (branchcode comes from issues table here). + my $borrower = BorType( $overdue->{borrowernumber} ); + my $item = GetItem( $overdue->{itemnumber} ); + my $branchcode = GetCircControlBranch( $item, $borrower, $overdue ); + if ( !exists $is_holiday{$branchcode} ) { $is_holiday{$branchcode} = set_holiday( $branchcode, $today ); } diff --git a/reserve/request.pl b/reserve/request.pl index a0cc055..0bd659c 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -436,7 +436,7 @@ foreach my $biblionumber (@biblionumbers) { } } - my $branch = C4::Circulation::_GetCircControlBranch($item, $borrowerinfo); + my $branch = C4::Circulation::GetCircControlBranch($item, $borrowerinfo); my $branchitemrule = GetBranchItemRule( $branch, $item->{'itype'} ); my $policy_holdallowed = 1; diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 31e5960..9bf813c 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -26,9 +26,9 @@ is( 'CircControl changed to PickupLibrary' ); is( - C4::Circulation::_GetCircControlBranch($item, $borrower), + C4::Circulation::GetCircControlBranch($item, $borrower), $item->{$HomeOrHoldingBranch}, - '_GetCircControlBranch returned item branch (no userenv defined)' + 'GetCircControlBranch returned item branch (no userenv defined)' ); # No userenv, PatronLibrary @@ -39,9 +39,9 @@ is( 'CircControl changed to PatronLibrary' ); is( - C4::Circulation::_GetCircControlBranch($item, $borrower), + C4::Circulation::GetCircControlBranch($item, $borrower), $borrower->{branchcode}, - '_GetCircControlBranch returned borrower branch' + 'GetCircControlBranch returned borrower branch' ); # No userenv, ItemHomeLibrary @@ -53,8 +53,8 @@ is( ); is( $item->{$HomeOrHoldingBranch}, - C4::Circulation::_GetCircControlBranch($item, $borrower), - '_GetCircControlBranch returned item branch' + C4::Circulation::GetCircControlBranch($item, $borrower), + 'GetCircControlBranch returned item branch' ); diag('Now, set a userenv'); @@ -70,9 +70,9 @@ is( 'CircControl changed to PickupLibrary' ); is( - C4::Circulation::_GetCircControlBranch($item, $borrower), + C4::Circulation::GetCircControlBranch($item, $borrower), 'CurrentBranch', - '_GetCircControlBranch returned current branch' + 'GetCircControlBranch returned current branch' ); # Userenv set, PatronLibrary @@ -83,9 +83,9 @@ is( 'CircControl changed to PatronLibrary' ); is( - C4::Circulation::_GetCircControlBranch($item, $borrower), + C4::Circulation::GetCircControlBranch($item, $borrower), $borrower->{branchcode}, - '_GetCircControlBranch returned borrower branch' + 'GetCircControlBranch returned borrower branch' ); # Userenv set, ItemHomeLibrary @@ -96,9 +96,9 @@ is( 'CircControl changed to ItemHomeLibrary' ); is( - C4::Circulation::_GetCircControlBranch($item, $borrower), + C4::Circulation::GetCircControlBranch($item, $borrower), $item->{$HomeOrHoldingBranch}, - '_GetCircControlBranch returned item branch' + 'GetCircControlBranch returned item branch' ); # Reset initial configuration -- 1.7.2.5