From 75e9326d24c161c408f35f8725de07a35e6c2ce7 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 --- 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 22eaceac948..424a9509ac2 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -1195,30 +1195,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 00000000000..e709362d3bf --- /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 7295744c4c0..87563bfb877 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -638,6 +638,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('RESTOAuth2ClientCredentials','0',NULL,'If enabled, the OAuth2 client credentials flow is enabled for the REST API.','YesNo'), ('RESTPublicAnonymousRequests','1',NULL,'If enabled, the API will allow anonymous access to public routes that don\'t require authenticated access.','YesNo'), ('RESTPublicAPI','1',NULL,'If enabled, the REST API will expose the /public endpoints.','YesNo'), +('RESTAPIRenewalBranch','apiuserbranch','itemhomebranch|patronhomebranch|checkoutbranch|apiuserbranch|none','Choose how the branch for an API renewal is recorded in statistics','Choice'), ('RestrictedPageLocalIPs','',NULL,'Beginning of IP addresses considered as local (comma separated ex: "127.0.0,127.0.2")','Free'), ('RestrictedPageContent','',NULL,'HTML content of the restricted page','TextArea'), ('RestrictedPageTitle','',NULL,'Title of the restricted page (breadcrumb and header)','Free'), 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 1fb5c999166..e8cff9a4290 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 branch 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 7a21db18f55..d18c66873c5 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.43.0