From 2c67648093b9e7479b1dd1135c80ada2d46aee7c Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Thu, 22 Dec 2016 14:00:37 +0000 Subject: [PATCH] Bug 17781 - Improper branchcode set during renewal For no discernable reason, when AddIssue calls AddRenewal, it passes the branchcode generated from _GetCircControlBranch. Assume _GetCircControlBranch is set to return items.homebranch. So: 1) If an item owned by LibraryA is checked out at LibraryB, the statistic line branchcode will be LibraryB 2) If an item is renewed via the ajax datatables renewal function, the statistic line branchcode will be LibraryB the 3) If an item is renewed via scanning the item into the checkout again, statistic line branchcode will be *LibraryA* This is clearly improper behavior. The renewal is taking place at LibraryB, so the branchcode passed to AddRenewal should be LibraryB, the logged in library. This also jives with the documentation for the subroutine. Test Plan: 1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary ) 2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch ) 3) Create item with homebranch of LibraryA and holdingbranch of LibraryB 4) Set the logged in library to LibraryB 4) Check the item out to a patron at LibraryB 5) Note the statistics line has a branchcode of LibraryB 6) Check the item out again to trigger a renewal, renew the item 7) Note the statistic line has a branchcode of LibraryA! 8) Apply this patch 9) Repeat step 6 10) Note the statistics line has a branchcode of LibraryB! Signed-off-by: Kyle M Hall Signed-off-by: David Kuhn Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- C4/Circulation.pm | 22 +++++++++++++--------- t/db_dependent/Circulation/issue.t | 26 +++++++++++++++++++++----- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 6e6f4c14a0..f1c9c2526d 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2953,15 +2953,19 @@ sub AddRenewal { } # Log the renewal - UpdateStats({branch => $branch, - type => 'renew', - amount => $charge, - itemnumber => $itemnumber, - itemtype => $item->{itype}, - borrowernumber => $borrowernumber, - ccode => $item->{'ccode'}} - ); - return $datedue; + UpdateStats( + { + branch => C4::Context->userenv ? C4::Context->userenv->{branch} : $branch, + type => 'renew', + amount => $charge, + itemnumber => $itemnumber, + itemtype => $item->{itype}, + borrowernumber => $borrowernumber, + ccode => $item->{'ccode'} + } + ); + + return $datedue; } sub GetRenewCount { diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t index 1de08bbdb7..e959ae81f4 100644 --- a/t/db_dependent/Circulation/issue.t +++ b/t/db_dependent/Circulation/issue.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 32; +use Test::More tests => 33; use DateTime::Duration; use t::lib::Mocks; @@ -67,6 +67,9 @@ $dbh->do(q|DELETE FROM branches|); $dbh->do(q|DELETE FROM categories|); $dbh->do(q|DELETE FROM accountlines|); $dbh->do(q|DELETE FROM issuingrules|); +$dbh->do(q|DELETE FROM reserves|); +$dbh->do(q|DELETE FROM old_reserves|); +$dbh->do(q|DELETE FROM statistics|); # Generate sample datas my $itemtype = $builder->build( @@ -76,6 +79,7 @@ my $itemtype = $builder->build( )->{itemtype}; my $branchcode_1 = $builder->build({ source => 'Branch' })->{branchcode}; my $branchcode_2 = $builder->build({ source => 'Branch' })->{branchcode}; +my $branchcode_3 = $builder->build({ source => 'Branch' })->{branchcode}; my $categorycode = $builder->build({ source => 'Category', value => { enrolmentfee => undef } @@ -159,6 +163,11 @@ my @USERENV = ( $branchcode_1, 'email@example.org' ); +my @USERENV_DIFFERENT_LIBRARY = ( + $borrower_id1, 'test', 'MASTERTEST', 'firstname', $branchcode_3, + $branchcode_3, 'email@example.org' +); + C4::Context->_new_userenv('DUMMY_SESSION_ID'); C4::Context->set_userenv(@USERENV); @@ -206,16 +215,23 @@ $sth->execute; $countaccount = $sth -> fetchrow_array; is ($countaccount,1,"1 accountline has been added"); -#Test AddRenewal -my $datedue3 = - AddRenewal( $borrower_id1, $item_id1, $branchcode_1, - $datedue1, $daysago10 ); +# Test AddRenewal + +# Let's renew this one at a different library for statistical purposes to test Bug 17781 +C4::Context->set_userenv(@USERENV_DIFFERENT_LIBRARY); +my $datedue3 = AddRenewal( $borrower_id1, $item_id1, $branchcode_1, $datedue1, $daysago10 ); +C4::Context->set_userenv(@USERENV); + like( $datedue3, qr/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/, "AddRenewal returns a date" ); +my $stat = $dbh->selectrow_hashref("SELECT * FROM statistics WHERE type = 'renew' AND borrowernumber = ? AND itemnumber = ? AND branch = ?", undef, $borrower_id1, $item_id1, $branchcode_3 ); +ok( $stat, "Bug 17781 - 'Improper branchcode set during renewal' still fixed" ); + + #Test GetBiblioIssues is( GetBiblioIssues(), undef, "GetBiblio Issues without parameters" ); -- 2.39.5