From aa424c01b9a80fe1abbb1a31ea2f96c388fd5495 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) Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Kyle M Hall --- 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 e1f1e8fb36..938ff92cd4 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' ], can_be_blank => 0, @@ -208,15 +205,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, @@ -226,6 +220,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; } @@ -290,6 +298,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}; my $can_be_blank = defined $kind_info->{can_be_blank} ? $kind_info->{can_be_blank} : 1; @@ -304,6 +313,7 @@ sub set_rule { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + has_priority => $has_priority, } )->next(); @@ -323,6 +333,7 @@ sub set_rule { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + has_priority => $has_priority, rule_name => $rule_name, rule_value => $rule_value, } @@ -345,7 +356,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 b0ff861658..7b2dbbe1c4 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, @@ -118,12 +120,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, @@ -147,6 +151,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -160,6 +165,7 @@ elsif ($op eq 'delete-branch-cat') { { branchcode => $branch, categorycode => undef, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -183,6 +189,7 @@ elsif ($op eq 'delete-branch-cat') { { categorycode => $categorycode, branchcode => $branch, + has_priority => $has_priority, rules => { max_holds => undef, patron_maxissueqty => undef, @@ -251,7 +258,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') || ''; @@ -298,7 +305,6 @@ elsif ($op eq 'add') { maxissueqty => $maxissueqty, maxonsiteissueqty => $maxonsiteissueqty, rentaldiscount => $rentaldiscount, - has_priority => $has_priority, fine => $fine, finedays => $finedays, maxsuspensiondays => $maxsuspensiondays, @@ -335,7 +341,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, } ); @@ -350,12 +356,14 @@ 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; if ($branch eq "*") { Koha::CirculationRules->set_rules( { itemtype => undef, branchcode => undef, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -367,9 +375,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, } } ); @@ -378,6 +388,7 @@ elsif ($op eq "set-branch-defaults") { { itemtype => undef, branchcode => $branch, + has_priority => $has_priority, rules => { holdallowed => $holdallowed, hold_fulfillment_policy => $hold_fulfillment_policy, @@ -389,6 +400,7 @@ elsif ($op eq "set-branch-defaults") { { categorycode => undef, branchcode => $branch, + has_priority => $has_priority, rules => { patron_maxissueqty => $patron_maxissueqty, patron_maxonsiteissueqty => $patron_maxonsiteissueqty, @@ -413,8 +425,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 "*") { @@ -422,6 +433,7 @@ elsif ($op eq "add-branch-cat") { { categorycode => undef, branchcode => undef, + has_priority => $has_priority, rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, @@ -435,11 +447,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, } } ); @@ -449,11 +461,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, } } ); @@ -462,11 +474,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 952cd9d296..fcccad2d9e 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1663,6 +1663,7 @@ CREATE TABLE `circulation_rules` ( `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL, `categorycode` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL, `itemtype` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL, + `has_priority` tinyint(1) NULL default NULL, `rule_name` varchar(32) COLLATE utf8mb4_unicode_ci NOT NULL, `rule_value` varchar(32) COLLATE utf8mb4_unicode_ci 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 4d5f555fba..e29d182214 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 @@ -139,6 +139,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 %] @@ -202,7 +203,7 @@ [% UNLESS humanbranch %] - [% IF rule.has_priority %] + [% IF has_priority.defined && has_priority %] [% ELSE %] @@ -708,7 +709,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.

@@ -722,12 +723,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 != '' ) %] @@ -760,7 +762,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 ef7eda779d..5fe97505d9 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.24.1 (Apple Git-126)