From efb7235d622e390f1f969d5fc5ce2472c360f644 Mon Sep 17 00:00:00 2001 From: Jesse Weaver Date: Tue, 17 Oct 2017 16:24:15 -0600 Subject: [PATCH] Bug 15522: Add API for circulation rules Test plan: 1) Run t/db_dependent/api/v1/circ_rules.t . --- Koha/CirculationRules.pm | 67 ++++++- Koha/Exceptions/Authorization.pm | 3 + Koha/REST/V1/CirculationRules.pm | 53 +++++ api/v1/swagger/definitions.json | 3 + api/v1/swagger/definitions/circ-rule-kind.json | 4 + api/v1/swagger/definitions/circ-rule.json | 31 +++ api/v1/swagger/paths.json | 3 + api/v1/swagger/paths/circulation-rules.json | 86 +++++++- api/v1/swagger/x-primitives.json | 4 + t/db_dependent/api/v1/circ_rules.t | 268 +++++++++++++++++++++++++ 10 files changed, 518 insertions(+), 4 deletions(-) create mode 100644 api/v1/swagger/definitions/circ-rule.json create mode 100644 t/db_dependent/api/v1/circ_rules.t diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index e3a4b8c0d0..fb37fed491 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -37,114 +37,153 @@ Koha::CirculationRules - Koha CirculationRule Object set class =head3 rule_kinds -This structure describes the possible rules that may be set, and what scopes they can be set at. +This structure describes the possible rules that may be set, what scopes they can be set at and any +default values. -Any attempt to set a rule with a nonsensical scope (for instance, setting the C for a branchcode and itemtype), is an error. +Any attempt to set a rule with a nonsensical scope (for instance, setting the C +for a branchcode and itemtype), is an error. + +These default values correspond to what would be stored in the database if nothing was changed on +the old circulation rules editor; this is a combination of the default values in the template and +the issuingrules table. =cut our $RULE_KINDS = { refund => { scope => [ 'branchcode' ], + default_value => 1, }, patron_maxissueqty => { scope => [ 'branchcode', 'categorycode' ], + default_value => "", }, patron_maxonsiteissueqty => { scope => [ 'branchcode', 'categorycode' ], + default_value => "", }, max_holds => { scope => [ 'branchcode', 'categorycode' ], + default_value => "", }, holdallowed => { scope => [ 'branchcode', 'itemtype' ], + default_value => 2, }, hold_fulfillment_policy => { scope => [ 'branchcode', 'itemtype' ], + default_value => 'any', }, returnbranch => { scope => [ 'branchcode', 'itemtype' ], + default_value => 'homebranch', }, article_requests => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 'no', }, auto_renew => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, cap_fine_to_replacement_price => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, chargeperiod => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, chargeperiod_charge_at => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, fine => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, finedays => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, firstremind => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, hardduedate => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, hardduedatecompare => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => -1, }, holds_per_record => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 1, }, issuelength => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, lengthunit => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 'days', }, maxissueqty => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, maxonsiteissueqty => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, maxsuspensiondays => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, no_auto_renewal_after => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, no_auto_renewal_after_hard_limit => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, norenewalbefore => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, onshelfholds => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, opacitemholds => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "N", }, overduefinescap => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => "", }, renewalperiod => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, renewalsallowed => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, rentaldiscount => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, reservesallowed => { scope => [ 'branchcode', 'categorycode', 'itemtype' ], + default_value => 0, }, # Not included (deprecated?): # * accountsent @@ -227,6 +266,25 @@ sub get_effective_rules { =head3 set_rule +=over 4 +Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + [ categorycode => $categorycode, ] + [ itemtype => $itemtype, ] + rule_name => $rule_name, + rule_value => $rule_value, + [ allow_null_out_of_scope => 1, ] +} ) +=back + +Sets a single circulation rule. + +It is an error to specify a C or C unless the given C can be set +at that scope; i.e., you cannot set a C for an itemtype. + +If C is passed, and any excess C or C is set to C, +they are ignored. + =cut sub set_rule { @@ -248,7 +306,10 @@ sub set_rule { unless exists $params->{$scope_level}; } else { croak "set_rule cannot set '$params->{rule_name}' for a '$scope_level'!" - if exists $params->{$scope_level}; + unless !exists $params->{$scope_level} || ( + $params->{allow_null_out_of_scope} && + !defined $params->{$scope_level} + ); } } diff --git a/Koha/Exceptions/Authorization.pm b/Koha/Exceptions/Authorization.pm index a3c625aca8..56c9d8b0cb 100644 --- a/Koha/Exceptions/Authorization.pm +++ b/Koha/Exceptions/Authorization.pm @@ -12,6 +12,9 @@ use Exception::Class ( description => 'Unauthorized', fields => ['required_permissions'] }, + 'Koha::Exceptions::Authorization::Restricted' => { + description => 'User can only modify settings for their branch' + }, ); diff --git a/Koha/REST/V1/CirculationRules.pm b/Koha/REST/V1/CirculationRules.pm index 539ed4e3ce..f0ad6c41cd 100644 --- a/Koha/REST/V1/CirculationRules.pm +++ b/Koha/REST/V1/CirculationRules.pm @@ -19,7 +19,12 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; +use C4::Auth; +use C4::Context; + use Koha::CirculationRules; +use Koha::Database; +use Koha::Exceptions::Authorization; use Try::Tiny; @@ -32,4 +37,52 @@ sub get_kinds { ); } +sub get_rules { + my $c = shift->openapi->valid_input or return; + + return $c->render( + status => 200, + openapi => [ Koha::CirculationRules->search ], + ); +} + +sub save_rules { + my $c = shift->openapi->valid_input or return; + + my $schema = Koha::Database->new->schema; + + my $uid = $c->stash( 'koha.user' )->userid; + my $restricted_to_library = $uid && haspermission( $uid, { parameters => 'manage_circ_rules_restricted' }, { no_inherit => 1 } ) ? $c->stash( 'koha.user' )->branchcode : ""; + + return try { + my $rules = $c->req->json; + + $schema->storage->txn_do( sub { + foreach my $rule ( @$rules ) { + if ( $restricted_to_library && ( !$rule->{branchcode} || $rule->{branchcode} ne $restricted_to_library ) ) { + Koha::Exceptions::Authorization::Restricted->throw( + error => 'User can only modify settings for their branch.' + ); + } + + Koha::CirculationRules->set_rule( { %$rule, allow_null_out_of_scope => 1 } ); + } + } ); + + return $c->render( status => 200, openapi => "" ); + } + catch { + if ( $_->isa('Koha::Exceptions::Authorization::Restricted') ) { + return $c->render( status => 403, + openapi => { error => $_->message } ); + } else { + warn $_; + + return $c->render( status => 500, + openapi => { error => "Something went wrong, check the logs."} ); + } + }; + +} + 1; diff --git a/api/v1/swagger/definitions.json b/api/v1/swagger/definitions.json index 93d8b09b4f..6711f46e5b 100644 --- a/api/v1/swagger/definitions.json +++ b/api/v1/swagger/definitions.json @@ -1,4 +1,7 @@ { + "circ-rule": { + "$ref": "definitions/circ-rule.json" + }, "circ-rule-kind": { "$ref": "definitions/circ-rule-kind.json" }, diff --git a/api/v1/swagger/definitions/circ-rule-kind.json b/api/v1/swagger/definitions/circ-rule-kind.json index 90b956f9f6..33cbbf292b 100644 --- a/api/v1/swagger/definitions/circ-rule-kind.json +++ b/api/v1/swagger/definitions/circ-rule-kind.json @@ -8,6 +8,10 @@ "type": "string", "enum": [ "branchcode", "categorycode", "itemtype" ] } + }, + "default_value": { + "description": "effective default if no rules are set", + "type": [ "string", "null" ] } }, "additionalProperties": false, diff --git a/api/v1/swagger/definitions/circ-rule.json b/api/v1/swagger/definitions/circ-rule.json new file mode 100644 index 0000000000..3125fecd10 --- /dev/null +++ b/api/v1/swagger/definitions/circ-rule.json @@ -0,0 +1,31 @@ +{ + "type": "object", + "properties": { + "id": { + "description": "internal identifier of this rule", + "type": "integer" + }, + "branchcode": { + "type": [ "string", "null" ], + "description": "branch that this rule applies to" + }, + "categorycode": { + "type": [ "string", "null" ], + "description": "category that this rule applies to" + }, + "itemtype": { + "type": [ "string", "null" ], + "description": "itemtype that this rule applies to" + }, + "rule_name": { + "type": "string", + "description": "name of the kind of this rule" + }, + "rule_value": { + "type": [ "string", "null" ], + "description": "value of the rule" + } + }, + "additionalProperties": false, + "required": [ "branchcode", "rule_name", "rule_value" ] +} diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index 408cd1a023..b405195e27 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -5,6 +5,9 @@ "/acquisitions/vendors/{vendor_id}": { "$ref": "paths/acquisitions_vendors.json#/~1acquisitions~1vendors~1{vendor_id}" }, + "/circulation-rules": { + "$ref": "paths/circulation-rules.json#/~1circulation-rules" + }, "/circulation-rules/kinds": { "$ref": "paths/circulation-rules.json#/~1circulation-rules~1kinds" }, diff --git a/api/v1/swagger/paths/circulation-rules.json b/api/v1/swagger/paths/circulation-rules.json index 0f118d344e..314b706b80 100644 --- a/api/v1/swagger/paths/circulation-rules.json +++ b/api/v1/swagger/paths/circulation-rules.json @@ -1,9 +1,93 @@ { + "/circulation-rules": { + "get": { + "x-mojo-to": "CirculationRules#get_rules", + "operationId": "getCirculationRules", + "tags": ["circulation-rules"], + "produces": [ + "application/json" + ], + "responses": { + "200": { + "description": "A list of defined rules", + "schema": { + "type": "array", + "items": { + "$ref": "../definitions.json#/circ-rule" + } + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "500": { + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } + } + } + }, + "post": { + "x-mojo-to": "CirculationRules#save_rules", + "operationId": "saveCirculationRules", + "tags": ["circulation-rules"], + "produces": [ + "application/json" + ], + "parameters": [{ + "name": "body", + "in": "body", + "description": "A JSON object containing rules to set; null for a rule_value means to delete the rule", + "required": true, + "schema": { + "type": "array", + "items": { + "$ref": "../definitions.json#/circ-rule" + } + } + }], + "responses": { + "200": { + "description": "Rules saved", + "schema": { + "type": "string" + } + }, + "401": { + "description": "Authentication required", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "500": { + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "permissions": { + "parameters": "manage_circ_rules" + } + } + } + }, "/circulation-rules/kinds": { "get": { "x-mojo-to": "CirculationRules#get_kinds", "operationId": "getCirculationRuleKinds", - "tags": ["cities"], + "tags": ["circulation-rules"], "produces": [ "application/json" ], diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index 12e5e9de39..4ff6ecb1ab 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -7,6 +7,10 @@ "type": "integer", "description": "internally assigned user identifier" }, + "branchcode": { + "type": "string", + "description": "library assigned branch identifier" + }, "cardnumber": { "type": ["string", "null"], "description": "library assigned user identifier" diff --git a/t/db_dependent/api/v1/circ_rules.t b/t/db_dependent/api/v1/circ_rules.t new file mode 100644 index 0000000000..688395644e --- /dev/null +++ b/t/db_dependent/api/v1/circ_rules.t @@ -0,0 +1,268 @@ +#!/usr/bin/env perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use Modern::Perl; + +use Test::More tests => 5; +use Test::Deep qw( cmp_deeply set superhashof ); +use Test::Mojo; +use Test::Warn; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +use C4::Auth; +use Koha::Acquisition::Booksellers; +use Koha::CirculationRules; +use Koha::Database; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +# FIXME: sessionStorage defaults to mysql, but it seems to break transaction handling +# this affects the other REST api tests +t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); + +my $remote_address = '127.0.0.1'; +my $t = Test::Mojo->new('Koha::REST::V1'); + +sub tx_maker { + my $args = shift; + + my ( $borrowernumber, $session_id ) = create_user_and_session( $args ); + + return sub { + my $tx = shift->ua->build_tx( @_ ); + $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + + return $tx; + } +} + +subtest 'rule_kinds' => sub { + plan tests => 3; + + my $tx_maker = tx_maker( { permissions => 'authorized' } ); + + my $tx = $tx_maker->( $t, GET => '/api/v1/circulation-rules/kinds' ); + $t->request_ok($tx) + ->status_is(200) + ->json_is( Koha::CirculationRules->rule_kinds ); +}; + +subtest 'get_rules' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + Koha::CirculationRules->search->delete; + + my $tx_maker = tx_maker( { permissions => 'authorized' } ); + + my $tx = $tx_maker->( $t, GET => '/api/v1/circulation-rules' ); + $t->request_ok($tx) + ->status_is(200) + ->json_is( [] ); + + my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my @rules = ( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 10, + }, + { + branchcode => $branchcode, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 20, + }, + ); + + Koha::CirculationRules->set_rule( $_ ) for ( @rules ); + + $tx = $tx_maker->( $t, GET => '/api/v1/circulation-rules' ); + $t->request_ok($tx) + ->status_is(200); + cmp_deeply( $tx->res->json, set( map { superhashof( $_ ) } @rules ), 'rules retrieved correctly' ); + + $schema->storage->txn_rollback; +}; + +subtest 'set_rules | unauthorized' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + Koha::CirculationRules->search->delete; + + my $tx_maker = tx_maker( { permissions => 'none' } ); + + my $rule = { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 10, + }; + + my $tx = $tx_maker->( $t, POST => '/api/v1/circulation-rules', json => $rule ); + $t->request_ok($tx) + ->status_is(403); + + $schema->storage->txn_rollback; +}; + +subtest 'set_rules' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + + Koha::CirculationRules->search->delete; + + my $tx_maker = tx_maker( { permissions => 'authorized' } ); + + my @rules = ( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 10, + }, + ); + + my $tx = $tx_maker->( $t, POST => '/api/v1/circulation-rules', json => \@rules ); + $t->request_ok($tx) + ->status_is(200); + + my $stored_rule = Koha::CirculationRules->search( { + branchcode => undef, + categorycode => undef, + itemtype => undef, + } )->next; + + cmp_deeply( + $stored_rule && $stored_rule->unblessed, + superhashof( $rules[0] ), + 'rule stored correctly' + ); + + $schema->storage->txn_rollback; +}; + +subtest 'set_rules | restricted' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + Koha::CirculationRules->search->delete; + + my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my $other_branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my $tx_maker = tx_maker( { permissions => 'authorized_restricted', branchcode => $branchcode } ); + + my @allowed_rules = ( + { + branchcode => $branchcode, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 10, + }, + ); + + my @forbidden_rules = ( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 10, + }, + { + branchcode => $other_branchcode, + categorycode => undef, + itemtype => undef, + rule_name => 'maxissueqty', + rule_value => 20, + }, + ); + + for my $rule ( @allowed_rules ) { + my $tx = $tx_maker->( $t, POST => '/api/v1/circulation-rules', json => [ $rule ] ); + $t->request_ok($tx) + ->status_is(200); + } + + for my $rule ( @forbidden_rules ) { + my $tx = $tx_maker->( $t, POST => '/api/v1/circulation-rules', json => [ $rule ] ); + $t->request_ok($tx) + ->status_is(403); + } + + $schema->storage->txn_rollback; +}; + +sub create_user_and_session { + + my $args = shift; + my $flags = { + none => 0, + # catalogue and parameters permissions + authorized => (1 << 2) | (1 << 3), + # Just catalogue + authorized_restricted => (1 << 2), + }->{ $args->{permissions} || ( $args->{authorized} ? 'authorized' : 'none' ) }; + + my %branch_flags; + $branch_flags{branchcode} = $args->{branchcode} if ( $args->{branchcode} ); + + # my $flags = ( $args->{authorized} ) ? $args->{authorized} : 0; + my $dbh = C4::Context->dbh; + + my $user = $builder->build( + { source => 'Borrower', + value => { flags => $flags, %branch_flags } + } + ); + + # Create a session for the authorized user + my $session = C4::Auth::get_session(''); + $session->param( 'number', $user->{borrowernumber} ); + $session->param( 'id', $user->{userid} ); + $session->param( 'ip', '127.0.0.1' ); + $session->param( 'lasttime', time() ); + $session->flush; + + if ( $args->{permissions} eq 'authorized_restricted' ) { + $dbh->do( + q{ + INSERT INTO user_permissions (borrowernumber,module_bit,code) + VALUES (?,3,'manage_circ_rules_restricted'), (?,3,'manage_circ_rules')}, + undef, $user->{borrowernumber}, $user->{borrowernumber} + ); + } + + return ( $user->{borrowernumber}, $session->id ); +} + +1; -- 2.15.1