Attachment 161428 Details for Bug 35386 – Bug 35386: Add RESTAPIRenewalBranch system preference

[patch] Bug 35386: Add RESTAPIRenewalBranch system preference

Bug-35386-Add-RESTAPIRenewalBranch-system-preferen.patch (text/plain), 16.61 KB, created by Martin Renvoize (ashimema) on 2024-01-25 15:31:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2024-01-25 15:31:20 UTC

Size: 16.61 KB

patch

obsolete

>From 75e9326d24c161c408f35f8725de07a35e6c2ce7 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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 <blawlor@clamsnet.org>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 35386: 159278 | 159282 | 161244 | 161428