From 80d622c155434139a3b2948411e0408c05bf6813 Mon Sep 17 00:00:00 2001 From: Alex Arnaud Date: Fri, 7 Jul 2017 08:31:37 +0000 Subject: [PATCH] Bug 8137: Add default issuing rule for all branches. This patch does the same that for 'Default checkout limit by patron category'. test plan: - Create an issuing rule a patron category and itemtype. make it has priority, - create more permissive issuing rules for all other branches, - check that the priority rule is taken into account for this category and itemtype. Rebased: 2018-03-14, by: Alex Arnaud Rebased: 2020-07-34, by: Arthur Suzuki Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Kyle M Hall --- Koha/CirculationRules.pm | 7 +- admin/smart-rules.pl | 4 + .../prog/en/modules/admin/smart-rules.tt | 96 ++-- t/db_dependent/Koha/IssuingRules.t | 418 ++++++++++++++++++ 4 files changed, 491 insertions(+), 34 deletions(-) create mode 100755 t/db_dependent/Koha/IssuingRules.t diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index c29b83d041..52e0771ba2 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -65,7 +65,9 @@ our $RULE_KINDS = { max_holds => { scope => [ 'branchcode', 'categorycode' ], }, - + has_priority => { + scope => [ 'branchcode', 'categorycode' ], + }, holdallowed => { scope => [ 'branchcode', 'itemtype' ], can_be_blank => 0, @@ -242,11 +244,13 @@ sub get_effective_rule { $params->{categorycode} //= undef; $params->{branchcode} //= undef; $params->{itemtype} //= undef; + $params->{has_priority} //= undef; my $rule_name = $params->{rule_name}; my $categorycode = $params->{categorycode}; my $itemtype = $params->{itemtype}; my $branchcode = $params->{branchcode}; + my $has_priority = $params->{has_priority}; Koha::Exceptions::MissingParameter->throw( "Required parameter 'rule_name' missing") @@ -265,6 +269,7 @@ sub get_effective_rule { $search_params->{categorycode} = defined $categorycode ? [ $categorycode, undef ] : undef; $search_params->{itemtype} = defined $itemtype ? [ $itemtype, undef ] : undef; $search_params->{branchcode} = defined $branchcode ? [ $branchcode, undef ] : undef; + $search_params->{has_priority} = defined $has_priority ? [ $has_priority, undef ] : undef; my $rule = $self->search( $search_params, diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 4b6885c21e..4840d3bd22 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -254,6 +254,7 @@ elsif ($op eq 'add') { my $br = $branch; # branch my $bor = $input->param('categorycode'); # borrower category my $itemtype = $input->param('itemtype'); # item type + my $has_priority = $input->param('has_priority') ? 1 : 0; my $fine = $input->param('fine'); my $finedays = $input->param('finedays'); my $maxsuspensiondays = $input->param('maxsuspensiondays') || q{}; @@ -296,10 +297,12 @@ elsif ($op eq 'add') { my $recall_overdue_fine = $input->param('recall_overdue_fine'); my $recall_shelf_time = $input->param('recall_shelf_time'); + my $rules = { maxissueqty => $maxissueqty, maxonsiteissueqty => $maxonsiteissueqty, rentaldiscount => $rentaldiscount, + has_priority => $has_priority, fine => $fine, finedays => $finedays, maxsuspensiondays => $maxsuspensiondays, @@ -342,6 +345,7 @@ elsif ($op eq 'add') { categorycode => $bor eq '*' ? undef : $bor, itemtype => $itemtype eq '*' ? undef : $itemtype, branchcode => $br eq '*' ? undef : $br, + has_priority => $has_priority, rules => $rules, } ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt index 98ea388325..daca8e8ab0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt @@ -115,7 +115,8 @@ Item type Actions Note - Current checkouts allowed + [% UNLESS humanbranch %]Has priority[% END %] + Current checkouts allowed Current on-site checkouts allowed Loan period Days mode @@ -242,7 +243,16 @@ View note [% ELSE %] [% END %] - + [% UNLESS humanbranch %] + + [% IF rule.has_priority %] + + [% ELSE %] + + [% END %] + + [% END %] + [% IF maxissueqty.defined && maxissueqty != '' %] [% maxissueqty | html %] [% ELSE %] @@ -439,6 +449,11 @@ + [% UNLESS humanbranch %] + + + + [% END %] @@ -554,7 +569,8 @@ Item type   Note - Current checkouts allowed + [% UNLESS humanbranch %]Has priority[% END %] + Current checkouts allowed Current on-site checkouts allowed Loan period Days mode @@ -810,36 +826,45 @@ [% SET patron_maxonsiteissueqty = CirculationRules.Search( branchcode, c, undef, 'patron_maxonsiteissueqty' ) %] [% SET max_holds = CirculationRules.Search( branchcode, c, undef, 'max_holds' ) %] - [% IF ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %] - - - [% IF c == undef %] - Default - [% ELSE %] - [% Categories.GetName(c) | html %] - [% END %] - - - [% IF patron_maxissueqty.defined && patron_maxissueqty != '' %] - [% patron_maxissueqty | html %] - [% ELSE %] - Unlimited - [% END %] - - - [% IF patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' %] - [% patron_maxonsiteissueqty | html %] - [% ELSE %] - Unlimited - [% END %] - - - [% IF max_holds.defined && max_holds != '' %] - [% max_holds | html %] - [% ELSE %] - Unlimited - [% END %] - + [% IF ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %] + + + [% IF c == undef %] + Default + [% ELSE %] + [% Categories.GetName(c) | html %] + [% END %] + + + [% IF patron_maxissueqty.defined && patron_maxissueqty != '' %] + [% patron_maxissueqty | html %] + [% ELSE %] + Unlimited + [% END %] + + + [% IF patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' %] + [% patron_maxonsiteissueqty | html %] + [% ELSE %] + Unlimited + [% END %] + + + [% IF max_holds.defined && max_holds != '' %] + [% max_holds | html %] + [% ELSE %] + Unlimited + [% END %] + + [% UNLESS humanbranch %] + + [% IF branch_cat_rule_loo.has_priority %] + + [% ELSE %] + + [% END %] + + [% END %] Delete @@ -1620,6 +1645,10 @@ var cap_fine_to_replacement_price = $(this).find("input[type='checkbox']"); $('#cap_fine_to_replacement_price').prop('checked', cap_fine_to_replacement_price.is(':checked') ); $('#overduefinescap').prop('disabled', cap_fine_to_replacement_price.is(':checked') ); + } else if ( i == 3 ) { + var has_priority = $(this).find("input[type='checkbox']"); + $('#has_priority').prop('checked', has_priority.is(':checked') ); + //$('#overduefinescap').prop('disabled', cap_fine_to_replacement_price.is(':checked') ); } else { $(current_column).find("input[type='text']").val(itm_text); // unformat prices @@ -1650,6 +1679,7 @@ // - "Holds allowed (total)" // - "Holds allowed (daily)" // - "Holds per record (count)" + // If the value is not an integer for "Current checkouts allowed" or "Current on-site checkouts allowed" // The value is "Unlimited" (or an equivalent translated string) // an it should be set to an empty string if( !((parseFloat(itm_text) == parseInt(itm_text)) && !isNaN(itm_text)) ) { diff --git a/t/db_dependent/Koha/IssuingRules.t b/t/db_dependent/Koha/IssuingRules.t new file mode 100755 index 0000000000..d8c7ff4374 --- /dev/null +++ b/t/db_dependent/Koha/IssuingRules.t @@ -0,0 +1,418 @@ +#!/usr/bin/perl + +# Copyright 2016 Koha-Suomi Oy +# +# 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, see . + +use Modern::Perl; + +use Test::More tests => 3; +use Test::Deep qw( cmp_methods ); +use Test::Exception; + +use Benchmark; + +use Koha::CirculationRules; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; + +my $builder = t::lib::TestBuilder->new; + +subtest 'get_effective_issuing_rule' => sub { + plan tests => 3; + + my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; + my $itemtype = $builder->build({ source => 'Itemtype' })->{'itemtype'}; + my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + + subtest 'Priority rule' => sub { + plan tests => 3; + + Koha::IssuingRules->delete; + + ok(Koha::IssuingRule->new({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + })->store, "Given I added an issuing rule branchcode => $branchcode,' + .' categorycode => $categorycode, itemtype => $itemtype,"); + + ok(Koha::IssuingRule->new({ + branchcode => '*', + categorycode => $categorycode, + itemtype => $itemtype, + has_priority => 1, + })->store, "Add a priority rule."); + + my $rule = Koha::IssuingRules->get_effective_issuing_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + }); + is($rule->has_priority, 1, 'Priority rule should be returned'); + }; + + subtest 'Call with undefined values' => sub { + plan tests => 5; + + my $rule; + Koha::CirculationRules->delete; + + is(Koha::CirculationRules->search->count, 0, 'There are no issuing rules.'); + # undef, undef, undef => 1 + $rule = Koha::CirculationRules->get_effective_rule({ + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'fine', + rule_value => 1, + }); + is($rule, undef, 'When I attempt to get effective issuing rule by' + .' providing undefined values, then undef is returned.'); + + # undef, undef, undef => 2 + ok( + Koha::CirculationRule->new( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'fine', + rule_value => 2, + } + )->store, + 'Given I added an issuing rule branchcode => undef,' + .' categorycode => undef, itemtype => undef,'); + $rule = Koha::CirculationRules->get_effective_rule({ + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'fine', + }); + _is_row_match( + $rule, + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'fine', + rule_value => 2, + }, + 'When I attempt to get effective' + .' issuing rule by providing undefined values, then the above one is' + .' returned.' + ); + }; + + subtest 'Performance' => sub { + plan tests => 4; + + my $worst_case = timethis(500, + sub { Koha::CirculationRules->get_effective_rule({ + branchcode => 'nonexistent', + categorycode => 'nonexistent', + itemtype => 'nonexistent', + rule_name => 'nonexistent', + }); + } + ); + my $mid_case = timethis(500, + sub { Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => 'nonexistent', + itemtype => 'nonexistent', + rule_name => 'nonexistent', + }); + } + ); + my $sec_best_case = timethis(500, + sub { Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => 'nonexistent', + rule_name => 'nonexistent', + }); + } + ); + my $best_case = timethis(500, + sub { Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'nonexistent', + }); + } + ); + ok($worst_case, 'In worst case, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $worst_case->iters/$worst_case->cpu_a) + .' times per second.'); + ok($mid_case, 'In mid case, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $mid_case->iters/$mid_case->cpu_a) + .' times per second.'); + ok($sec_best_case, 'In second best case, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $sec_best_case->iters/$sec_best_case->cpu_a) + .' times per second.'); + ok($best_case, 'In best case, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $best_case->iters/$best_case->cpu_a) + .' times per second.'); + }; +}; + +subtest 'set_rule' => sub { + plan tests => 3; + + my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; + my $itemtype = $builder->build({ source => 'Itemtype' })->{'itemtype'}; + + subtest 'Correct call' => sub { + plan tests => 4; + + Koha::CirculationRules->delete; + + lives_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + rule_name => 'lostreturn', + rule_value => '', + } ); + }, 'setting lostreturn with branch' ); + + lives_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + rule_name => 'patron_maxissueqty', + rule_value => '', + } ); + }, 'setting patron_maxissueqty with branch/category succeeds' ); + + lives_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + itemtype => $itemtype, + rule_name => 'holdallowed', + rule_value => '', + } ); + }, 'setting holdallowed with branch/itemtype succeeds' ); + + lives_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + rule_value => '', + } ); + }, 'setting fine with branch/category/itemtype succeeds' ); + }; + + subtest 'Call with missing params' => sub { + plan tests => 4; + + Koha::CirculationRules->delete; + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + rule_name => 'lostreturn', + rule_value => '', + } ); + }, qr/branchcode/, 'setting lostreturn without branch fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + rule_name => 'patron_maxissueqty', + rule_value => '', + } ); + }, qr/categorycode/, 'setting patron_maxissueqty without categorycode fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + rule_name => 'holdallowed', + rule_value => '', + } ); + }, qr/itemtype/, 'setting holdallowed without itemtype fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + rule_name => 'fine', + rule_value => '', + } ); + }, qr/itemtype/, 'setting fine without itemtype fails' ); + }; + + subtest 'Call with extra params' => sub { + plan tests => 3; + + Koha::CirculationRules->delete; + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + rule_name => 'lostreturn', + rule_value => '', + } ); + }, qr/categorycode/, 'setting lostreturn with categorycode fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'patron_maxissueqty', + rule_value => '', + } ); + }, qr/itemtype/, 'setting patron_maxissueqty with itemtype fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + rule_name => 'holdallowed', + categorycode => $categorycode, + itemtype => $itemtype, + rule_value => '', + } ); + }, qr/categorycode/, 'setting holdallowed with categorycode fails' ); + }; +}; + +subtest 'clone' => sub { + plan tests => 2; + + my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; + my $itemtype = $builder->build({ source => 'Itemtype' })->{'itemtype'}; + + subtest 'Clone multiple rules' => sub { + plan tests => 4; + + Koha::CirculationRules->delete; + + Koha::CirculationRule->new({ + branchcode => undef, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + rule_value => 5, + })->store; + + Koha::CirculationRule->new({ + branchcode => undef, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'lengthunit', + rule_value => 'days', + })->store; + + Koha::CirculationRules->search({ branchcode => undef })->clone($branchcode); + + my $rule_fine = Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + }); + my $rule_lengthunit = Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'lengthunit', + }); + + _is_row_match( + $rule_fine, + { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + rule_value => 5, + }, + 'When I attempt to get cloned fine rule,' + .' then the above one is returned.' + ); + _is_row_match( + $rule_lengthunit, + { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'lengthunit', + rule_value => 'days', + }, + 'When I attempt to get cloned lengthunit rule,' + .' then the above one is returned.' + ); + + }; + + subtest 'Clone one rule' => sub { + plan tests => 2; + + Koha::CirculationRules->delete; + + Koha::CirculationRule->new({ + branchcode => undef, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + rule_value => 5, + })->store; + + my $rule = Koha::CirculationRules->search({ branchcode => undef })->next; + $rule->clone($branchcode); + + my $cloned_rule = Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + }); + + _is_row_match( + $cloned_rule, + { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + rule_name => 'fine', + rule_value => '5', + }, + 'When I attempt to get cloned fine rule,' + .' then the above one is returned.' + ); + + }; +}; + +sub _is_row_match { + my ( $rule, $expected, $message ) = @_; + + ok( $rule, $message ) ? + cmp_methods( $rule, [ %$expected ], $message ) : + fail( $message ); +} + +$schema->storage->txn_rollback; -- 2.30.2