From 7efef13061ac27397c95e31c076a4b8c148a23f2 Mon Sep 17 00:00:00 2001 From: Arthur Suzuki Date: Wed, 22 Jul 2020 11:43:28 +0200 Subject: [PATCH] Bug 8137: (follow-up) --- Koha/CirculationRule.pm | 10 +++++ Koha/CirculationRules.pm | 30 +++++++++---- Koha/Schema/Result/CirculationRule.pm | 10 ++++- admin/smart-rules.pl | 29 +++++++++---- .../Bug_8137-add-column-has_priority.sql | 7 +-- installer/data/mysql/kohastructure.sql | 1 + .../prog/en/modules/admin/smart-rules.tt | 16 ++++--- t/db_dependent/Circulation/Branch.t | 50 ++++++++++++++++------ t/db_dependent/Koha/IssuingRules.t | 26 ++++++----- 9 files changed, 124 insertions(+), 55 deletions(-) diff --git a/Koha/CirculationRule.pm b/Koha/CirculationRule.pm index 59638a5aa2..818ca513e8 100644 --- a/Koha/CirculationRule.pm +++ b/Koha/CirculationRule.pm @@ -71,6 +71,16 @@ sub item_type { return $self->{item_type}; } +=head3 priority + +=cut + +sub priority { + my ($self) = @_; + + return $self->has_priority; +} + =head3 clone Clone a circulation rule to another branch diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index 9413d3d2d5..31b52fd7b4 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -59,9 +59,6 @@ our $RULE_KINDS = { max_holds => { scope => [ 'branchcode', 'categorycode' ], }, - has_priority => { - scope => [ 'branchcode', 'categorycode' ], - }, holdallowed => { scope => [ 'branchcode', 'itemtype' ], }, @@ -199,15 +196,12 @@ sub get_effective_rule { } my $order_by = $params->{order_by} - // { -desc => [ 'branchcode', 'categorycode', 'itemtype' ] }; + // { -desc => [ 'has_priority', 'branchcode', 'categorycode', 'itemtype' ] }; my $search_params; $search_params->{rule_name} = $rule_name; - $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; + $search_params->{has_priority} = 1; my $rule = $self->search( $search_params, @@ -217,6 +211,20 @@ sub get_effective_rule { } )->single; + return $rule if defined $rule; + + $search_params->{itemtype} = defined $itemtype ? [ $itemtype, undef ] : undef; + $search_params->{has_priority} = defined $has_priority ? [ $has_priority, undef ] : undef; + $search_params->{branchcode} = defined $branchcode ? [ $branchcode, undef ] : undef; + + $rule = $self->search( + $search_params, + { + order_by => $order_by, + rows => 1, + } + )->single; + return $rule; } @@ -281,6 +289,7 @@ sub set_rule { my $branchcode = $params->{branchcode}; my $categorycode = $params->{categorycode}; my $itemtype = $params->{itemtype}; + my $has_priority = $params->{has_priority}; my $rule_name = $params->{rule_name}; my $rule_value = $params->{rule_value}; @@ -293,6 +302,7 @@ sub set_rule { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + has_priority => $has_priority, } )->next(); @@ -312,6 +322,7 @@ sub set_rule { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + has_priority => $has_priority, rule_name => $rule_name, rule_value => $rule_value, } @@ -334,7 +345,8 @@ sub set_rules { $set_params{branchcode} = $params->{branchcode} if exists $params->{branchcode}; $set_params{categorycode} = $params->{categorycode} if exists $params->{categorycode}; $set_params{itemtype} = $params->{itemtype} if exists $params->{itemtype}; - my $rules = $params->{rules}; + $set_params{has_priority} = $params->{has_priority} if exists $params->{has_priority}; + my $rules = $params->{rules}; my $rule_objects = []; while ( my ( $rule_name, $rule_value ) = each %$rules ) { diff --git a/Koha/Schema/Result/CirculationRule.pm b/Koha/Schema/Result/CirculationRule.pm index 80e6cc2d5a..7be0ba1c2e 100644 --- a/Koha/Schema/Result/CirculationRule.pm +++ b/Koha/Schema/Result/CirculationRule.pm @@ -50,6 +50,11 @@ __PACKAGE__->table("circulation_rules"); is_nullable: 1 size: 10 +=head2 has_priority + + data_type: 'integer' + is_nullable: 1 + =head2 rule_name data_type: 'varchar' @@ -73,6 +78,7 @@ __PACKAGE__->add_columns( { data_type => "varchar", is_foreign_key => 1, is_nullable => 1, size => 10 }, "itemtype", { data_type => "varchar", is_foreign_key => 1, is_nullable => 1, size => 10 }, + "has_priority" => { is_boolean => 1 }, "rule_name", { data_type => "varchar", is_nullable => 0, size => 32 }, "rule_value", @@ -177,8 +183,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07042 @ 2018-10-05 14:29:17 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:QHMqvrtX0ohJe70PHUYZ0Q +# Created by DBIx::Class::Schema::Loader v0.07046 @ 2020-07-22 13:19:28 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:1mg3weVQdfPTJ+jX6X5K+Q sub koha_objects_class { 'Koha::CirculationRules'; diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 943c9a3c7b..1e1697ee5d 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -75,6 +75,7 @@ $cache->clear_from_cache( Koha::CirculationRules::GUESSED_ITEMTYPES_KEY ); if ($op eq 'delete') { my $itemtype = $input->param('itemtype'); my $categorycode = $input->param('categorycode'); + my $has_priority = $input->param('has_priority') ? 1 : undef; $debug and warn "deleting $1 $2 $branch"; Koha::CirculationRules->set_rules( @@ -82,6 +83,7 @@ if ($op eq 'delete') { categorycode => $categorycode eq '*' ? undef : $categorycode, branchcode => $branch eq '*' ? undef : $branch, itemtype => $itemtype eq '*' ? undef : $itemtype, + has_priority => $has_priority, rules => { maxissueqty => undef, maxonsiteissueqty => undef, @@ -119,12 +121,14 @@ if ($op eq 'delete') { } elsif ($op eq 'delete-branch-cat') { my $categorycode = $input->param('categorycode'); + my $has_priority = $input->param('has_priority') ? 1 : undef; if ($branch eq "*") { if ($categorycode eq "*") { Koha::CirculationRules->set_rules( { branchcode => undef, categorycode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -148,6 +152,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -161,6 +166,7 @@ elsif ($op eq 'delete-branch-cat') { { branchcode => $branch, categorycode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -184,6 +190,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => $branch, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -252,7 +259,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 $has_priority = $input->param('has_priority') ? 1 : undef; my $fine = $input->param('fine'); my $finedays = $input->param('finedays'); my $maxsuspensiondays = $input->param('maxsuspensiondays') || ''; @@ -297,7 +304,6 @@ elsif ($op eq 'add') { maxissueqty => $maxissueqty, maxonsiteissueqty => $maxonsiteissueqty, rentaldiscount => $rentaldiscount, - has_priority => $has_priority, fine => $fine, finedays => $finedays, maxsuspensiondays => $maxsuspensiondays, @@ -332,7 +338,7 @@ elsif ($op eq 'add') { categorycode => $bor eq '*' ? undef : $bor, itemtype => $itemtype eq '*' ? undef : $itemtype, branchcode => $br eq '*' ? undef : $br, - has_priority => $has_priority, + has_priority => $has_priority, rules => $rules, } ); @@ -347,6 +353,7 @@ elsif ($op eq "set-branch-defaults") { my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy'); my $returnbranch = $input->param('returnbranch'); my $max_holds = strip_non_numeric( scalar $input->param('max_holds') ); + my $has_priority = $input->param('has_priority') ? 1 : undef; $holdallowed =~ s/\s//g; $holdallowed = undef if $holdallowed !~ /^\d+/; @@ -355,6 +362,7 @@ elsif ($op eq "set-branch-defaults") { { itemtype => undef, branchcode => undef, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -366,9 +374,11 @@ elsif ($op eq "set-branch-defaults") { { categorycode => undef, branchcode => undef, + has_priority => $has_priority, rules => { patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, + patron_has_priority => $has_priority, } } ); @@ -377,6 +387,7 @@ elsif ($op eq "set-branch-defaults") { { itemtype => undef, branchcode => $branch, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -388,6 +399,7 @@ elsif ($op eq "set-branch-defaults") { { categorycode => undef, branchcode => $branch, + has_priority => $has_priority, rules => { patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, @@ -412,8 +424,7 @@ elsif ($op eq "add-branch-cat") { my $max_holds = $input->param('max_holds'); $max_holds =~ s/\s//g; $max_holds = undef if $max_holds !~ /^\d+/; - my $has_priority = $input->param('has_priority'); - $has_priority = $has_priority ? 1 : 0; + my $has_priority = $input->param('has_priority') ? 1 : undef; if ($branch eq "*") { if ($categorycode eq "*") { @@ -421,6 +432,7 @@ elsif ($op eq "add-branch-cat") { { categorycode => undef, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, @@ -434,11 +446,11 @@ elsif ($op eq "add-branch-cat") { { categorycode => $categorycode, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, - has_priority => $has_priority, } } ); @@ -448,11 +460,11 @@ elsif ($op eq "add-branch-cat") { { categorycode => undef, branchcode => $branch, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, - has_priority => $has_priority, } } ); @@ -461,11 +473,11 @@ elsif ($op eq "add-branch-cat") { { categorycode => $categorycode, branchcode => $branch, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, - has_priority => $has_priority, } } ); @@ -579,6 +591,7 @@ my $rules = {}; while ( my $r = $all_rules->next ) { $r = $r->unblessed; $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->{ $r->{rule_name} } = $r->{rule_value}; + $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->{ 'has_priority' } = $r->{has_priority}; } $template->param(show_branch_cat_rule_form => 1); diff --git a/installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.sql b/installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.sql index fc7c42fcc0..a368b4e69e 100644 --- a/installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.sql +++ b/installer/data/mysql/atomicupdate/Bug_8137-add-column-has_priority.sql @@ -1,5 +1,2 @@ -ALTER TABLE default_borrower_circ_rules ADD has_priority INT(1) DEFAULT 0; -ALTER TABLE default_borrower_circ_rules DROP foreign key borrower_borrower_circ_rules_ibfk_1; -ALTER TABLE default_borrower_circ_rules DROP primary key; -ALTER TABLE default_borrower_circ_rules ADD primary key(categorycode, has_priority); -ALTER TABLE default_borrower_circ_rules ADD CONSTRAINT borrower_borrower_circ_rules_ibfk_1 FOREIGN KEY (categorycode) REFERENCES categories (categorycode) ON DELETE CASCADE ON UPDATE CASCADE; \ No newline at end of file +ALTER TABLE circulation_rules ADD has_priority INT(1) DEFAULT NULL AFTER itemtype; +ALTER TABLE circulation_rules ADD KEY (branchcode , categorycode, itemtype, has_priority); diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 4f6a9c399d..414df49674 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -4359,6 +4359,7 @@ CREATE TABLE `circulation_rules` ( `branchcode` varchar(10) NULL default NULL, `categorycode` varchar(10) NULL default NULL, `itemtype` varchar(10) NULL default NULL, + `has_priority` tinyint(1) NULL default NULL, `rule_name` varchar(32) NOT NULL, `rule_value` varchar(32) NOT NULL, PRIMARY KEY (`id`), 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 aa2ee27908..ce28717880 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 @@ -129,6 +129,7 @@ [% FOREACH i IN itemtypes %] [% SET i = '' UNLESS i.defined %] [% SET note = all_rules.$c.$i.note %] + [% SET has_priority = all_rules.$c.$i.has_priority %] [% SET maxissueqty = all_rules.$c.$i.maxissueqty %] [% SET maxonsiteissueqty = all_rules.$c.$i.maxonsiteissueqty %] [% SET issuelength = all_rules.$c.$i.issuelength %] @@ -188,7 +189,7 @@ [% UNLESS humanbranch %] - [% IF rule.has_priority %] + [% IF has_priority.defined && has_priority %] [% ELSE %] @@ -666,7 +667,7 @@

Has priority: If checked, the rule will override those for all branches. Else - it behaves like a default one: used if no rule existe for the coresponding branch. + it behaves like a default one: used if no rule exists for the coresponding branch.

@@ -680,12 +681,13 @@ [% UNLESS humanbranch %]Has priority[% END %]   + [% SET i = '' %] [% FOREACH c IN categorycodes %] [% NEXT UNLESS c %] - [% SET patron_maxissueqty = CirculationRules.Search( branchcode, c, undef, 'patron_maxissueqty' ) %] - [% SET patron_maxonsiteissueqty = CirculationRules.Search( branchcode, c, undef, 'patron_maxonsiteissueqty' ) %] - [% SET max_holds = CirculationRules.Search( branchcode, c, undef, 'max_holds' ) %] - + [% SET patron_maxissueqty = all_rules.$c.$i.patron_maxissueqty %] + [% SET patron_maxonsiteissueqty = all_rules.$c.$i.patron_maxonsiteissueqty %] + [% SET max_holds = all_rules.$c.$i.max_holds %] + [% SET has_priority = all_rules.$c.$i.has_priority %] [% IF ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %] @@ -718,7 +720,7 @@ [% UNLESS humanbranch %] - [% IF branch_cat_rule_loo.has_priority %] + [% IF has_priority.defined && has_priority %] [% ELSE %] diff --git a/t/db_dependent/Circulation/Branch.t b/t/db_dependent/Circulation/Branch.t index fa4eb5bf76..abb71fa870 100644 --- a/t/db_dependent/Circulation/Branch.t +++ b/t/db_dependent/Circulation/Branch.t @@ -155,7 +155,7 @@ Koha::CirculationRules->set_rules( patron_maxissueqty => 5, patron_maxonsiteissueqty => 6, } - } + } ); @@ -232,12 +232,16 @@ Koha::CirculationRules->set_rules( } ); -$query = q| - INSERT INTO default_borrower_circ_rules - (categorycode, maxissueqty, maxonsiteissueqty, has_priority) - VALUES( ?, ?, ?, ? ) -|; -$dbh->do($query, {}, $samplecat->{categorycode}, 3, 3, 0); +Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => $samplecat->{categorycode}, + rules => { + patron_maxonsiteissueqty => 3, + patron_maxissueqty => 3, + } + } +); #Test GetBranchBorrowerCircRule # @@ -265,22 +269,40 @@ is_deeply( "GetBranchBorrower with wrong parameters returns the patron_maxissueqty and patron_maxonsiteissueqty of default_circ_rules" ); -$query = q| - INSERT INTO default_borrower_circ_rules - (categorycode, maxissueqty, maxonsiteissueqty, has_priority) - VALUES( ?, ?, ?, ? ) -|; -$dbh->do($query, {}, $samplecat->{categorycode}, 3, 3, 1); + +Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => $samplecat->{categorycode}, + has_priority => 1, + rules => { + patron_maxonsiteissueqty => 3, + patron_maxissueqty => 3, + } + } +); is_deeply( GetBranchBorrowerCircRule( $samplebranch1->{branchcode}, $samplecat->{categorycode} ), - { maxissueqty => 3, maxonsiteissueqty => 3, has_priority => 1 }, + { patron_maxissueqty => 3, patron_maxonsiteissueqty => 3 }, "GetBranchBorrower returns the rule having priority" ); +Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => $samplecat->{categorycode}, + has_priority => 0, + rules => { + patron_maxonsiteissueqty => 3, + patron_maxissueqty => 3, + } + } +); + #Test GetBranchItemRule my @lazy_any = ( 'hold_fulfillment_policy' => 'any' ); is_deeply( diff --git a/t/db_dependent/Koha/IssuingRules.t b/t/db_dependent/Koha/IssuingRules.t index 491f64cc9b..28e6d64a1d 100644 --- a/t/db_dependent/Koha/IssuingRules.t +++ b/t/db_dependent/Koha/IssuingRules.t @@ -35,7 +35,7 @@ $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; -subtest 'get_effective_issuing_rule' => sub { +subtest 'get_effective_rule' => sub { plan tests => 4; my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; @@ -43,30 +43,37 @@ subtest 'get_effective_issuing_rule' => sub { my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; subtest 'Priority rule' => sub { - plan tests => 3; + plan tests => 4; - Koha::IssuingRules->delete; + Koha::CirculationRules->delete; + is(Koha::CirculationRules->search->count, 0, 'There are no issuing rules.'); - ok(Koha::IssuingRule->new({ + ok(Koha::CirculationRule->new({ branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + rule_name => 'fine', + rule_value => 0, })->store, "Given I added an issuing rule branchcode => $branchcode,' .' categorycode => $categorycode, itemtype => $itemtype,"); - ok(Koha::IssuingRule->new({ - branchcode => '*', + ok(Koha::CirculationRule->new({ + branchcode => undef, categorycode => $categorycode, itemtype => $itemtype, has_priority => 1, + rule_name => 'fine', + rule_value => 1, })->store, "Add a priority rule."); - my $rule = Koha::IssuingRules->get_effective_issuing_rule({ + my $rule = Koha::CirculationRules->get_effective_rule({ branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + has_priority => 1, + rule_name => 'fine', }); - is($rule->has_priority, 1, 'Priority rule should be returned'); + is($rule->priority, 1, 'Priority rule should be returned'); }; subtest 'Call with undefined values' => sub { @@ -76,7 +83,7 @@ subtest 'get_effective_issuing_rule' => sub { Koha::CirculationRules->delete; is(Koha::CirculationRules->search->count, 0, 'There are no issuing rules.'); - # undef, undef, undef => 1 + # undef, undef, undef => 1w $rule = Koha::CirculationRules->get_effective_rule({ branchcode => undef, categorycode => undef, @@ -711,4 +718,3 @@ sub _is_row_match { } $schema->storage->txn_rollback; - -- 2.11.0