From c3104f4f1cb8ed1191e83ade5e9e4b55f50b2553 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 27 Nov 2023 16:31:41 +0000 Subject: [PATCH] Bug 35386: Add RESTAPIRenewalBranch system preference This patch adds a new system prefernce, RESTAPIRenewalBranch, analogous to the existing OpacRenewalBranch system preference. The preference allows choosing how the renewal branch is recorded in the statistics table. In order ot preserve existing behaviour, the default is to use the api user's branch. To test: 1 - Checkout some items to a patron 2 - Add an API user account with circulation permissions and a different homebranch 3 - POST a renewal to: http://localhost:8080/api/v1/checkouts/{checkout_id}/renewal 4 - Check statistics table and confirm the api users branch was used 5 - Apply patches, restart all 6 - Repeat API renewal, confirm same branch used 7 - Change the RESTAPIRenewal syspref 8 - Repeat API renewal and confirm specified branch is used 9 - Confirm the syspref works for all settings Signed-off-by: Brendan Lawlor Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer --- Koha/Item.pm | 42 +++-- .../data/mysql/atomicupdate/bug_35386.pl | 19 +++ installer/data/mysql/mandatory/sysprefs.sql | 1 + .../admin/preferences/web_services.pref | 11 ++ t/db_dependent/Koha/Item.t | 151 ++++++++++++++---- 5 files changed, 181 insertions(+), 43 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_35386.pl diff --git a/Koha/Item.pm b/Koha/Item.pm index 11e4fb89c4..59edeb752d 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -1161,30 +1161,46 @@ Returns the branchcode to be recorded in statistics renewal of the item sub renewal_branchcode { - my ($self, $params ) = @_; + my ( $self, $params ) = @_; my $interface = C4::Context->interface; my $branchcode; - if ( $interface eq 'opac' ){ - my $renewal_branchcode = C4::Context->preference('OpacRenewalBranch'); - if( !defined $renewal_branchcode || $renewal_branchcode eq 'opacrenew' ){ + my $renewal_branchcode; + + if ( $interface eq 'opac' ) { + $renewal_branchcode = C4::Context->preference('OpacRenewalBranch'); + if ( !defined $renewal_branchcode || $renewal_branchcode eq 'opacrenew' ) { $branchcode = 'OPACRenew'; } - elsif ( $renewal_branchcode eq 'itemhomebranch' ) { - $branchcode = $self->homebranch; + } elsif ( $interface eq 'api' ) { + $renewal_branchcode = C4::Context->preference('RESTAPIRenewalBranch'); + if ( !defined $renewal_branchcode || $renewal_branchcode eq 'apirenew' ) { + $branchcode = 'APIRenew'; } - elsif ( $renewal_branchcode eq 'patronhomebranch' ) { + } + + return $branchcode if $branchcode; + + if ($renewal_branchcode) { + if ( $renewal_branchcode eq 'itemhomebranch' ) { + $branchcode = $self->homebranch; + } elsif ( $renewal_branchcode eq 'patronhomebranch' ) { $branchcode = $self->checkout->patron->branchcode; - } - elsif ( $renewal_branchcode eq 'checkoutbranch' ) { + } elsif ( $renewal_branchcode eq 'checkoutbranch' ) { $branchcode = $self->checkout->branchcode; - } - else { + } elsif ( $renewal_branchcode eq 'apiuserbranch' ) { + $branchcode = + ( C4::Context->userenv && defined C4::Context->userenv->{branch} ) + ? C4::Context->userenv->{branch} + : $params->{branch}; + } else { $branchcode = ""; } } else { - $branchcode = ( C4::Context->userenv && defined C4::Context->userenv->{branch} ) - ? C4::Context->userenv->{branch} : $params->{branch}; + $branchcode = + ( C4::Context->userenv && defined C4::Context->userenv->{branch} ) + ? C4::Context->userenv->{branch} + : $params->{branch}; } return $branchcode; } diff --git a/installer/data/mysql/atomicupdate/bug_35386.pl b/installer/data/mysql/atomicupdate/bug_35386.pl new file mode 100755 index 0000000000..e709362d3b --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_35386.pl @@ -0,0 +1,19 @@ +use Modern::Perl; + +return { + bug_number => "35386", + description => "Add RESTAPIRenewalBranch preference", + up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('RESTAPIRenewalBranch','apiuserbranch','itemhomebranch|patronhomebranch|checkoutbranch|apiuserbranch|none','Choose how the branch for an API renewal is recorded in statistics','Choice') + } + ); + + say $out "Added new system preference 'RESTAPIRenewalBranch'"; + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 4057841a34..a5b6407304 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -650,6 +650,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('ReservesControlBranch','PatronLibrary','ItemHomeLibrary|PatronLibrary','Branch checked for members reservations rights','Choice'), ('ReservesMaxPickUpDelay','7','','Define the Maximum delay to pick up an item on hold','Integer'), ('ReservesNeedReturns','1','','If ON, a hold placed on an item available in this library must be checked-in, otherwise, a hold on a specific item, that is in the library & available is considered available','YesNo'), +('RESTAPIRenewalBranch','apiuserbranch','itemhomebranch|patronhomebranch|checkoutbranch|apiuserbranch|none','Choose how the branch for an API renewal is recorded in statistics','Choice'), ('RESTBasicAuth','0',NULL,'If enabled, Basic authentication is enabled for the REST API.','YesNo'), ('RESTdefaultPageSize','20','','Default page size for endpoints listing objects','Integer'), ('RESTOAuth2ClientCredentials','0',NULL,'If enabled, the OAuth2 client credentials flow is enabled for the REST API.','YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref index 1fb5c99916..d5c7c1feb2 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/web_services.pref @@ -35,6 +35,17 @@ Web services: 1: Enable 0: "Disable" - "the /public namespace of the API." + - + - Use + - pref: RESTAPIRenewalBranch + choices: + apiuserbranch: "the library of the API user" + itemhomebranch: "the item's home library" + patronhomebranch: "the patron's home library" + checkoutbranch: "the library the item was checked out from" + none: "NULL" + apirenew: "'APIRenew'" + - as branchcode to store in the statistics table. OAI-PMH: - - pref: OAI-PMH diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index e5a155f0f8..438436a501 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -1019,49 +1019,140 @@ subtest 'deletion' => sub { }; subtest 'renewal_branchcode' => sub { - plan tests => 13; + plan tests => 25; $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, + 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 } ); - C4::Context->interface( 'intranet' ); - t::lib::Mocks::mock_userenv({ branchcode => $branch->branchcode }); + is( $item->renewal_branchcode, $branch->branchcode, "If interface not opac, we get the branch from context" ); + is( + $item->renewal_branchcode( { 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->renewal_branchcode( { branch => "PANDA" } ), "PANDA", + "If interface not opac, we get the branch we pass one in if context not set" + ); - is( $item->renewal_branchcode, $branch->branchcode, "If interface not opac, we get the branch from context"); - is( $item->renewal_branchcode({ 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->renewal_branchcode({ branch => "PANDA"}), "PANDA", "If interface not opac, we get the branch we pass one in if context not set"); + C4::Context->interface('opac'); - C4::Context->interface( 'opac' ); + t::lib::Mocks::mock_preference( 'OpacRenewalBranch', undef ); + is( $item->renewal_branchcode, 'OPACRenew', "If interface opac and OpacRenewalBranch undef, we get OPACRenew" ); + is( + $item->renewal_branchcode( { branch => 'COW' } ), 'OPACRenew', + "If interface opac and OpacRenewalBranch undef, we get OPACRenew even if branch passed" + ); - t::lib::Mocks::mock_preference('OpacRenewalBranch', undef); - is( $item->renewal_branchcode, 'OPACRenew', "If interface opac and OpacRenewalBranch undef, we get OPACRenew"); - is( $item->renewal_branchcode({branch=>'COW'}), 'OPACRenew', "If interface opac and OpacRenewalBranch undef, we get OPACRenew even if branch passed"); + t::lib::Mocks::mock_preference( 'OpacRenewalBranch', 'none' ); + is( $item->renewal_branchcode, '', "If interface opac and OpacRenewalBranch is none, we get blank string" ); + is( + $item->renewal_branchcode( { branch => 'COW' } ), '', + "If interface opac and OpacRenewalBranch is none, we get blank string even if branch passed" + ); - t::lib::Mocks::mock_preference('OpacRenewalBranch', 'none'); - is( $item->renewal_branchcode, '', "If interface opac and OpacRenewalBranch is none, we get blank string"); - is( $item->renewal_branchcode({branch=>'COW'}), '', "If interface opac and OpacRenewalBranch is none, we get blank string even if branch passed"); + t::lib::Mocks::mock_preference( 'OpacRenewalBranch', 'checkoutbranch' ); + is( + $item->renewal_branchcode, $checkout->branchcode, + "If interface opac and OpacRenewalBranch set to checkoutbranch, we get branch of checkout" + ); + is( + $item->renewal_branchcode( { 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', 'checkoutbranch'); - is( $item->renewal_branchcode, $checkout->branchcode, "If interface opac and OpacRenewalBranch set to checkoutbranch, we get branch of checkout"); - is( $item->renewal_branchcode({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->renewal_branchcode, $checkout->patron->branchcode, + "If interface opac and OpacRenewalBranch set to patronbranch, we get branch of patron" + ); + is( + $item->renewal_branchcode( { 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','patronhomebranch'); - is( $item->renewal_branchcode, $checkout->patron->branchcode, "If interface opac and OpacRenewalBranch set to patronbranch, we get branch of patron"); - is( $item->renewal_branchcode({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->renewal_branchcode, $item->homebranch, + "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item" + ); + is( + $item->renewal_branchcode( { branch => 'MANATEE' } ), $item->homebranch, + "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item even if branch passed" + ); + + C4::Context->interface('api'); + + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', undef ); + is( $item->renewal_branchcode, 'APIRenew', "If interface api and RESTAPIRenewalBranch undef, we get APIRenew" ); + is( + $item->renewal_branchcode( { branch => 'COW' } ), 'APIRenew', + "If interface api and RESTAPIRenewalBranch undef, we get APIRenew even if branch passed" + ); + + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', 'none' ); + is( $item->renewal_branchcode, '', "If interface api and RESTAPIRenewalBranch is none, we get blank string" ); + is( + $item->renewal_branchcode( { branch => 'COW' } ), '', + "If interface api and RESTAPIRenewalBranch is none, we get blank string even if branch passed" + ); - t::lib::Mocks::mock_preference('OpacRenewalBranch','itemhomebranch'); - is( $item->renewal_branchcode, $item->homebranch, "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item"); - is( $item->renewal_branchcode({branch=>'MANATEE'}), $item->homebranch, "If interface opac and OpacRenewalBranch set to itemhomebranch, we get homebranch of item even if branch passed"); + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', 'checkoutbranch' ); + is( + $item->renewal_branchcode, $checkout->branchcode, + "If interface api and RESTAPIRenewalBranch set to checkoutbranch, we get branch of checkout" + ); + is( + $item->renewal_branchcode( { branch => 'MONKEY' } ), $checkout->branchcode, + "If interface api and RESTAPIRenewalBranch set to checkoutbranch, we get branch of checkout even if branch passed" + ); + + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', 'patronhomebranch' ); + is( + $item->renewal_branchcode, $checkout->patron->branchcode, + "If interface api and RESTAPIRenewalBranch set to patronbranch, we get branch of patron" + ); + is( + $item->renewal_branchcode( { branch => 'TURKEY' } ), $checkout->patron->branchcode, + "If interface api and RESTAPIRenewalBranch set to patronbranch, we get branch of patron even if branch passed" + ); + + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', 'itemhomebranch' ); + is( + $item->renewal_branchcode, $item->homebranch, + "If interface api and RESTAPIRenewalBranch set to itemhomebranch, we get homebranch of item" + ); + is( + $item->renewal_branchcode( { branch => 'MANATEE' } ), $item->homebranch, + "If interface api and RESTAPIRenewalBranch set to itemhomebranch, we get homebranch of item even if branch passed" + ); + + t::lib::Mocks::mock_userenv( { branchcode => $branch->branchcode } ); + t::lib::Mocks::mock_preference( 'RESTAPIRenewalBranch', 'apiuserbranch' ); + is( + $item->renewal_branchcode, $branch->branchcode, + "If interface api and RESTAPIRenewalBranch set to apiuserbranch, we get branch from userenv" + ); + is( + $item->renewal_branchcode( { branch => 'MANATEE' } ), $branch->branchcode, + "If interface api and RESTAPIRenewalBranch set to apiuserbranch, we get branch from userenv even if branch passed" + ); + C4::Context->set_userenv( 51, 'userid4tests', undef, 'firstname', 'surname', undef, undef, 0, undef, undef, undef ) + ; #mock userenv doesn't let us set null branch $schema->storage->txn_rollback; }; -- 2.20.1