From e64981061cbed179e6e3708a061fb8d286716f5d Mon Sep 17 00:00:00 2001 From: Nick Clemens 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