From 86a2b89a5a7aa783cc77ea44d18fddd49b5a9d32 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Fri, 28 Feb 2020 13:09:29 +0000
Subject: [PATCH] Bug 24759: Move OpacRenewalBranch code to Koha::Item
This patchset moves all code to calculate the correct renewal branch into Koha::Item.pm
When interface is opac we follow the syspref, otherwise we use the current userenv, or pass through
a defined branch
To test:
1 - Check out an item to a patron
2 - Set allowed renewals in the circ rules to 100 (just so you can keep testing)
3 - Renew the item in staff interface, confirm it is recorded correctly in statistics table (as signed in branch)
4 - Renew via the opac, testing with each setting of OpacRenewalbranch
5 - prove -v t/db_dependent/Koha/Item.t
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
---
C4/Circulation.pm | 6 +-----
Koha/Item.pm | 39 ++++++++++++++++++++++++++++++++++
opac/opac-renew.pl | 21 +-----------------
t/db_dependent/Koha/Item.t | 53 +++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 93 insertions(+), 26 deletions(-)
diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 2c75ec68c4..0013e17b17 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3049,14 +3049,10 @@ sub AddRenewal {
DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
}
- unless ( C4::Context->interface eq 'opac' ) { #if from opac we are obeying OpacRenewalBranch as calculated in opac-renew.pl
- $branch = ( C4::Context->userenv && defined C4::Context->userenv->{branch} ) ? C4::Context->userenv->{branch} : $branch;
- }
-
# Add the renewal to stats
UpdateStats(
{
- branch => $branch,
+ branch => $item_object->renewalbranch({branch => $branch}),
type => 'renew',
amount => $charge,
itemnumber => $itemnumber,
diff --git a/Koha/Item.pm b/Koha/Item.pm
index d2b1b9d154..11866921ce 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -490,6 +490,45 @@ sub as_marc_field {
return $field;
}
+=head3 renewalbranch
+
+Returns the branch to be recorded in statistics renewal of the item
+
+=cut
+
+sub renewalbranch {
+
+ my ($self, $params ) = @_;
+
+ my $interface = C4::Context->interface;
+ my $branchcode;
+ if ( $interface eq 'opac' ){
+ my $renewalbranch = C4::Context->preference('OpacRenewalBranch');
+ if( !defined $renewalbranch ){
+ $branchcode = 'OPACRenew';
+ }
+ elsif ( $renewalbranch eq 'itemhomebranch' ) {
+ $branchcode = $self->homebranch;
+ }
+ elsif ( $renewalbranch eq 'patronhomebranch' ) {
+ $branchcode = $self->checkout->patron->branchcode;
+ }
+ elsif ( $renewalbranch eq 'checkoutbranch' ) {
+ $branchcode = $self->checkout->branchcode;
+ }
+ elsif ( $renewalbranch eq 'NULL' ) {
+ $branchcode = '';
+ }
+ else {
+ $branchcode = 'OPACRenew';
+ }
+ } else {
+ $branchcode = ( C4::Context->userenv && defined C4::Context->userenv->{branch} )
+ ? C4::Context->userenv->{branch} : $params->{branch};
+ }
+ return $branchcode;
+}
+
=head3 to_api_mapping
This method returns the mapping for representing a Koha::Item object
diff --git a/opac/opac-renew.pl b/opac/opac-renew.pl
index 85570d54ad..cb9043d9ad 100755
--- a/opac/opac-renew.pl
+++ b/opac/opac-renew.pl
@@ -63,26 +63,7 @@ else {
my ( $status, $error ) =
CanBookBeRenewed( $borrowernumber, $itemnumber );
if ( $status == 1 && $opacrenew == 1 ) {
- my $renewalbranch = C4::Context->preference('OpacRenewalBranch');
- my $branchcode;
- if ( $renewalbranch eq 'itemhomebranch' ) {
- my $item = Koha::Items->find($itemnumber);
- $branchcode = $item->homebranch;
- }
- elsif ( $renewalbranch eq 'patronhomebranch' ) {
- $branchcode = Koha::Patrons->find( $borrowernumber )->branchcode;
- }
- elsif ( $renewalbranch eq 'checkoutbranch' ) {
- my $issue = GetOpenIssue($itemnumber); # FIXME Should not be $item->checkout?
- $branchcode = $issue->{'branchcode'};
- }
- elsif ( $renewalbranch eq 'NULL' ) {
- $branchcode = '';
- }
- else {
- $branchcode = 'OPACRenew';
- }
- AddRenewal( $borrowernumber, $itemnumber, $branchcode, undef, undef );
+ AddRenewal( $borrowernumber, $itemnumber, undef, undef, undef );
push( @renewed, $itemnumber );
}
else {
diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
index 3a2d2a8c67..32364879b9 100644
--- a/t/db_dependent/Koha/Item.t
+++ b/t/db_dependent/Koha/Item.t
@@ -19,7 +19,7 @@
use Modern::Perl;
-use Test::More tests => 4;
+use Test::More tests => 5;
use C4::Biblio;
@@ -329,3 +329,54 @@ subtest 'pickup_locations' => sub {
$schema->storage->txn_rollback;
};
+
+subtest 'renewalbranch' => sub {
+ plan tests => 15;
+
+ $schema->storage->txn_begin;
+
+ my $item = $builder->build_sample_item();
+ my $branch = $builder->build_object({ class => 'Koha::Libraries' });
+ my $checkout = $builder->build_object({
+ class => 'Koha::Checkouts',
+ value => {
+ itemnumber => $item->itemnumber,
+ }
+ });
+
+
+ C4::Context->interface( 'intranet' );
+ t::lib::Mocks::mock_userenv({ branchcode => $branch->branchcode });
+
+ is( $item->renewalbranch, $branch->branchcode, "If interface not opac, we get the branch from context");
+ is( $item->renewalbranch({ branch => "PANDA"}), $branch->branchcode, "If interface not opac, we get the branch from context even if we pass one in");
+ C4::Context->set_userenv(51, 'userid4tests', undef, 'firstname', 'surname', undef, undef, 0, undef, undef, undef ); #mock userenv doesn't let us set null branch
+ is( $item->renewalbranch({ branch => "PANDA"}), "PANDA", "If interface not opac, we get the branch we pass one in if context not set");
+
+ C4::Context->interface( 'opac' );
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch', '');
+ is( $item->renewalbranch, 'OPACRenew', "If interface opac and OpacRenewalBranch blank, we get the OPACRenew");
+ is( $item->renewalbranch({branch=>'CHICKEN'}), 'OPACRenew', "If interface opac and OpacRenewalBranch blank, we get the OPACRenew even if branch passes");
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch', undef);
+ is( $item->renewalbranch, 'OPACRenew', "If interface opac and OpacRenewalBranch undef, we get OPACRenew");
+ is( $item->renewalbranch({branch=>'COW'}), 'OPACRenew', "If interface opac and OpacRenewalBranch undef, we get OPACRenew even if branch passed");
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch', 'NULL');
+ is( $item->renewalbranch, '', "If interface opac and OpacRenewalBranch is string 'NULL', we get blank string");
+ is( $item->renewalbranch({branch=>'COW'}), '', "If interface opac and OpacRenewalBranch is string 'NULL', we get blank string even if branch passed");
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch', 'checkoutbranch');
+ is( $item->renewalbranch, $checkout->branchcode, "If interface opac and OpacRenewalBranch set to checkoutbranch, we get branch of checkout");
+ is( $item->renewalbranch({branch=>'MONKEY'}), $checkout->branchcode, "If interface opac and OpacRenewalBranch set to checkoutbranch, we get branch of checkout even if branch passed");
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch','patronhomebranch');
+ is( $item->renewalbranch, $checkout->patron->branchcode, "If interface opac and OpacRenewalBranch set to patronbranch, we get branch of patron");
+ is( $item->renewalbranch({branch=>'TURKEY'}), $checkout->patron->branchcode, "If interface opac and OpacRenewalBranch set to patronbranch, we get branch of patron even if branch passed");
+
+ t::lib::Mocks::mock_preference('OpacRenewalBranch','itemhomebranch');
+ is( $item->renewalbranch, $item->homebranch, "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item");
+ is( $item->renewalbranch({branch=>'MANATEE'}), $item->homebranch, "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item even if branch passed");
+
+};
--
2.11.0