From e64981061cbed179e6e3708a061fb8d286716f5d 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
---
 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 860d965cf1..07b7b050e7 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3032,14 +3032,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