From 28e0b29671e40506de54c355ee4e39893a3c84f4 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 | 11 ++++- 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 | 23 ++++++---- 9 files changed, 124 insertions(+), 53 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 b5f6a366ca..f1f404619e 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..4284c3894a 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,8 @@ __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", + { data_type => "integer", is_nullable => 1, is_boolean => 1 }, "rule_name", { data_type => "varchar", is_nullable => 0, size => 32 }, "rule_value", @@ -177,8 +184,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 cd21d3afb9..d02c3b05bd 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -73,6 +73,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( @@ -80,6 +81,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, @@ -117,12 +119,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, @@ -146,6 +150,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -159,6 +164,7 @@ elsif ($op eq 'delete-branch-cat') { { branchcode => $branch, categorycode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -182,6 +188,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => $branch, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -250,7 +257,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') || ''; @@ -295,7 +302,6 @@ elsif ($op eq 'add') { maxissueqty => $maxissueqty, maxonsiteissueqty => $maxonsiteissueqty, rentaldiscount => $rentaldiscount, - has_priority => $has_priority, fine => $fine, finedays => $finedays, maxsuspensiondays => $maxsuspensiondays, @@ -330,7 +336,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, } ); @@ -345,6 +351,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+/; @@ -353,6 +360,7 @@ elsif ($op eq "set-branch-defaults") { { itemtype => undef, branchcode => undef, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -364,9 +372,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, } } ); @@ -375,6 +385,7 @@ elsif ($op eq "set-branch-defaults") { { itemtype => undef, branchcode => $branch, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -386,6 +397,7 @@ elsif ($op eq "set-branch-defaults") { { categorycode => undef, branchcode => $branch, + has_priority => $has_priority, rules => { patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, @@ -410,8 +422,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 "*") { @@ -419,6 +430,7 @@ elsif ($op eq "add-branch-cat") { { categorycode => undef, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, @@ -432,11 +444,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, } } ); @@ -446,11 +458,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, } } ); @@ -459,11 +471,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, } } ); @@ -578,6 +590,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 8bfedd862e..d3ea17980e 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -4429,6 +4429,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 04cf7eb47b..866fb47d4b 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 @@ -135,6 +135,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 %] @@ -196,7 +197,7 @@ [% UNLESS humanbranch %] - [% IF rule.has_priority %] + [% IF has_priority.defined && has_priority %] [% ELSE %] @@ -690,7 +691,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.

@@ -704,12 +705,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 != '' ) %] @@ -742,7 +744,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 06f8c2fe0e..a9282e0178 100755 --- 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 7973c3bc08..d5953baca6 100755 --- a/t/db_dependent/Koha/IssuingRules.t +++ b/t/db_dependent/Koha/IssuingRules.t @@ -35,6 +35,7 @@ $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; + subtest 'get_effective_issuing_rule' => sub { plan tests => 3; @@ -43,30 +44,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 { @@ -416,4 +424,3 @@ sub _is_row_match { } $schema->storage->txn_rollback; - -- 2.11.0