Bugzilla – Attachment 18462 Details for
Bug 10374
Clean up and unify CircControl related code
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 10374 - Clean up and unify CircControl related code
Bug-10374---Clean-up-and-unify-CircControl-related.patch (text/plain), 16.58 KB, created by
Kyle M Hall (khall)
on 2013-05-29 12:16:48 UTC
(
hide
)
Description:
Bug 10374 - Clean up and unify CircControl related code
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2013-05-29 12:16:48 UTC
Size:
16.58 KB
patch
obsolete
>From 9080b1c646c59ebc33444d0b5655d0b886a51e6f Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=18462">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 10374
:
18462
|
18463
|
18467
|
18468
|
18469
|
18470
|
18471