From 5b16c0cd2ac080a2079c47562181cec7c5ddb301 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Fri, 11 Sep 2020 10:20:29 +0000 Subject: [PATCH] Bug 26814: Remove on-site specific circulation rules To apply: 1. perl installer/data/mysql/updatedatabase.pl To test: 1. Search for patron_maxonsiteissueqty rule $ grep -rn 'patron_maxonsiteissueqty' 2. Observe no results in Koha source files (excluding .git, atomicupdates, updatedatabase.pl, release notes, smart-rules.pl, smart-rules.tt) 3. Search for maxonsiteissueqty rule $ grep -rn 'maxonsiteissueqty' 4. Observe no results (excluding .git, atomicupdates and updatedatabase.pl, release notes, smart-rules.pl, smart-rules.tt) 5. Check modification made to C4::Circulation::GetBranchBorrowerCircRule(). Make sure the new parameter is being used everywhere. You can search the usage of this subroutine with similar grep commands as above. 6. Run the following tests: prove t/db_dependent/Circulation/Branch.t prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t prove t/db_dependent/Circulation/TooMany.t Sponsored-by: The National Library of Finland --- C4/Circulation.pm | 49 ++++++------------- Koha/CirculationRules.pm | 6 --- ...bug_26814_2_convert_circulation_rules.perl | 33 +++++++++++++ installer/onboarding.pl | 1 - t/db_dependent/Circulation/Branch.t | 47 ++++++++++++------ .../Circulation/SwitchOnSiteCheckouts.t | 26 +++++++++- t/db_dependent/Circulation/TooMany.t | 46 +++++++++++------ 7 files changed, 136 insertions(+), 72 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_26814_2_convert_circulation_rules.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 602bc54f6b..c5656705d5 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -418,6 +418,7 @@ sub TooMany { categorycode => $cat_borrower, itemtype => $parent_type, branchcode => $branch, + onsite_checkout => $onsite_checkout, rule_name => 'maxissueqty', } ) if $parent_type; @@ -429,20 +430,11 @@ sub TooMany { categorycode => $cat_borrower, itemtype => $type, branchcode => $branch, + onsite_checkout => $onsite_checkout, rule_name => 'maxissueqty', } ); - my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule( - { - categorycode => $cat_borrower, - itemtype => $type, - branchcode => $branch, - rule_name => 'maxonsiteissueqty', - } - ); - - my $patron = Koha::Patrons->find($borrower->{borrowernumber}); # if a rule is found and has a loan limit set, count # how many loans the patron already has that meet that @@ -478,6 +470,7 @@ sub TooMany { branchcode => $maxissueqty_rule->branchcode, categorycode => [ $maxissueqty_rule->categorycode, $cat_borrower ], itemtype => { '!=' => undef }, + onsite_checkout => $onsite_checkout, rule_name => 'maxissueqty' } )->get_column('itemtype'); @@ -513,7 +506,6 @@ sub TooMany { onsite_checkout_count => $onsite_checkout_count, onsite_checkout => $onsite_checkout, max_checkouts_allowed => $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef, - max_onsite_checkouts_allowed => $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : undef, switch_onsite_checkout => $switch_onsite_checkout, }; # If parent rules exists @@ -536,7 +528,7 @@ sub TooMany { } # Now count total loans against the limit for the branch - my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower); + my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower, $onsite_checkout ); if (defined($branch_borrower_circ_rule->{patron_maxissueqty}) and $branch_borrower_circ_rule->{patron_maxissueqty} ne '') { my $checkouts; if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { @@ -553,7 +545,6 @@ sub TooMany { my $checkout_count = $checkouts->count; my $onsite_checkout_count = $checkouts->search({ onsite_checkout => 1 })->count; my $max_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxissueqty}; - my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxonsiteissueqty} || undef; my $qty_over = _check_max_qty( { @@ -561,7 +552,6 @@ sub TooMany { onsite_checkout_count => $onsite_checkout_count, onsite_checkout => $onsite_checkout, max_checkouts_allowed => $max_checkouts_allowed, - max_onsite_checkouts_allowed => $max_onsite_checkouts_allowed, switch_onsite_checkout => $switch_onsite_checkout } ); @@ -582,16 +572,15 @@ sub _check_max_qty { my $onsite_checkout_count = $params->{onsite_checkout_count}; my $onsite_checkout = $params->{onsite_checkout}; my $max_checkouts_allowed = $params->{max_checkouts_allowed}; - my $max_onsite_checkouts_allowed = $params->{max_onsite_checkouts_allowed}; my $switch_onsite_checkout = $params->{switch_onsite_checkout}; - if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) { - if ( $max_onsite_checkouts_allowed eq '' ) { return; } - if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { + if ( $onsite_checkout and defined $max_checkouts_allowed ) { + if ( $max_checkouts_allowed eq '' ) { return; } + if ( $onsite_checkout_count >= $max_checkouts_allowed ) { return { reason => 'TOO_MANY_ONSITE_CHECKOUTS', count => $onsite_checkout_count, - max_allowed => $max_onsite_checkouts_allowed, + max_allowed => $max_checkouts_allowed, }; } } @@ -1731,31 +1720,23 @@ sub GetHardDueDate { =head2 GetBranchBorrowerCircRule - my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode); + my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode, $onsite_checkout); Retrieves circulation rule attributes that apply to the given -branch and patron category, regardless of item type. +branch, patron category and checkout type, regardless of item type. The return value is a hashref containing the following key: patron_maxissueqty - maximum number of loans that a patron of the given category can have at the given branch. If the value is undef, no limit. -patron_maxonsiteissueqty - maximum of on-site checkouts that a -patron of the given category can have at the given -branch. If the value is undef, no limit. - -This will check for different branch/category combinations in the following order: -branch and category -branch only -category only -default branch and category +The order in which rules are searched is defined in +Cget_effective_rule()> If no rule has been found in the database, it will default to the buillt in rule: patron_maxissueqty - undef -patron_maxonsiteissueqty - undef C<$branchcode> and C<$categorycode> should contain the literal branch code and patron category code, respectively - no @@ -1764,21 +1745,21 @@ wildcards. =cut sub GetBranchBorrowerCircRule { - my ( $branchcode, $categorycode ) = @_; + my ( $branchcode, $categorycode, $onsite_checkout ) = @_; # Initialize default values my $rules = { patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, }; # Search for rules! - foreach my $rule_name (qw( patron_maxissueqty patron_maxonsiteissueqty )) { + foreach my $rule_name (qw( patron_maxissueqty )) { my $rule = Koha::CirculationRules->get_effective_rule( { categorycode => $categorycode, itemtype => undef, branchcode => $branchcode, + onsite_checkout => $onsite_checkout, rule_name => $rule_name, } ); diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index b0eff0ac35..5aa7dd7ce9 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -53,9 +53,6 @@ our $RULE_KINDS = { patron_maxissueqty => { scope => [ 'branchcode', 'categorycode', 'onsite_checkout' ], }, - patron_maxonsiteissueqty => { - scope => [ 'branchcode', 'categorycode' ], - }, max_holds => { scope => [ 'branchcode', 'categorycode' ], }, @@ -121,9 +118,6 @@ our $RULE_KINDS = { maxissueqty => { scope => [ 'branchcode', 'categorycode', 'itemtype', 'onsite_checkout' ], }, - maxonsiteissueqty => { - scope => [ 'branchcode', 'categorycode', 'itemtype' ], - }, maxsuspensiondays => { scope => [ 'branchcode', 'categorycode', 'itemtype', 'onsite_checkout' ], }, diff --git a/installer/data/mysql/atomicupdate/bug_26814_2_convert_circulation_rules.perl b/installer/data/mysql/atomicupdate/bug_26814_2_convert_circulation_rules.perl new file mode 100644 index 0000000000..c7c7ba02db --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_26814_2_convert_circulation_rules.perl @@ -0,0 +1,33 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + + # First, convert maxonsiteissueqty to maxissueqty + my $update_sth = $dbh->prepare(q| + UPDATE circulation_rules + SET onsite_checkout = ?, + rule_name = ? + WHERE rule_name=? + |); + $update_sth->execute( + 1, + 'maxissueqty', + 'maxonsiteissueqty' + ); + + # Then, convert patron_maxonsiteissueqty to patron_maxissueqty + $update_sth = $dbh->prepare(q| + UPDATE circulation_rules + SET onsite_checkout = ?, + rule_name = ? + WHERE rule_name=? + |); + $update_sth->execute( + 1, + 'patron_maxissueqty', + 'patron_maxonsiteissueqty' + ); + + # Always end with this (adjust the bug info) + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 26814 - Convert maxonsiteissueqty to maxissueqty )\n"; +} diff --git a/installer/onboarding.pl b/installer/onboarding.pl index 6680cd94d9..d8626671eb 100755 --- a/installer/onboarding.pl +++ b/installer/onboarding.pl @@ -277,7 +277,6 @@ if ( $step == 5 ) { holds_per_day => $holds_per_day, holds_per_record => $holds_per_record, maxissueqty => $maxissueqty, - maxonsiteissueqty => "", maxsuspensiondays => "", no_auto_renewal_after => "", no_auto_renewal_after_hard_limit => "", diff --git a/t/db_dependent/Circulation/Branch.t b/t/db_dependent/Circulation/Branch.t index c8fe89dc97..e8bff550a4 100755 --- a/t/db_dependent/Circulation/Branch.t +++ b/t/db_dependent/Circulation/Branch.t @@ -25,7 +25,7 @@ use Koha::CirculationRules; use Koha::Patrons; -use Test::More tests => 15; +use Test::More tests => 16; use t::lib::Mocks; use t::lib::TestBuilder; @@ -143,29 +143,39 @@ my $borrower_id1 = Koha::Patron->new({ is_deeply( GetBranchBorrowerCircRule(), - { patron_maxissueqty => undef, patron_maxonsiteissueqty => undef }, -"Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty and patron_maxonsiteissueqty if no rules defined" + { patron_maxissueqty => undef }, +"Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty if no rules defined" ); Koha::CirculationRules->set_rules( { branchcode => $samplebranch1->{branchcode}, categorycode => $samplecat->{categorycode}, + onsite_checkout => undef, rules => { patron_maxissueqty => 5, - patron_maxonsiteissueqty => 6, } } ); +Koha::CirculationRules->set_rules( + { + branchcode => $samplebranch1->{branchcode}, + categorycode => $samplecat->{categorycode}, + onsite_checkout => 1, + rules => { + patron_maxissueqty => 6, + } + } +); Koha::CirculationRules->set_rules( { branchcode => $samplebranch2->{branchcode}, categorycode => undef, + onsite_checkout => undef, rules => { patron_maxissueqty => 3, - patron_maxonsiteissueqty => 2, } } ); @@ -184,9 +194,9 @@ Koha::CirculationRules->set_rules( { branchcode => undef, categorycode => undef, + onsite_checkout => undef, rules => { patron_maxissueqty => 4, - patron_maxonsiteissueqty => 5, } } ); @@ -235,26 +245,35 @@ Koha::CirculationRules->set_rules( #Test GetBranchBorrowerCircRule is_deeply( GetBranchBorrowerCircRule(), - { patron_maxissueqty => 4, patron_maxonsiteissueqty => 5 }, -"Without parameter, GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty of default_circ_rules" + { patron_maxissueqty => 4 }, +"Without parameter, GetBranchBorrower returns the patron_maxissueqty of default_circ_rules" ); is_deeply( GetBranchBorrowerCircRule( $samplebranch2->{branchcode} ), - { patron_maxissueqty => 3, patron_maxonsiteissueqty => 2 }, -"Without only the branchcode specified, GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty corresponding" + { patron_maxissueqty => 3 }, +"Without only the branchcode specified, GetBranchBorrower returns the patron_maxissueqty corresponding" ); is_deeply( GetBranchBorrowerCircRule( $samplebranch1->{branchcode}, $samplecat->{categorycode} ), - { patron_maxissueqty => 5, patron_maxonsiteissueqty => 6 }, - "GetBranchBorrower returns the patron_maxissueqty and patron_maxonsiteissueqty of the branch1 and the category1" + { patron_maxissueqty => 5 }, + "GetBranchBorrower returns the patron_maxissueqty of the branch1 and the category1" +); +is_deeply( + GetBranchBorrowerCircRule( + $samplebranch1->{branchcode}, + $samplecat->{categorycode}, + 1, + ), + { patron_maxissueqty => 6 }, + "GetBranchBorrower returns the patron_maxissueqty of the branch1 and the category1 and onsite_checkout on-site" ); is_deeply( GetBranchBorrowerCircRule( -1, -1 ), - { patron_maxissueqty => 4, patron_maxonsiteissueqty => 5 }, -"GetBranchBorrower with wrong parameters returns the patron_maxissueqty and patron_maxonsiteissueqty of default_circ_rules" + { patron_maxissueqty => 4 }, +"GetBranchBorrower with wrong parameters returns the patron_maxissueqty of default_circ_rules" ); #Test GetBranchItemRule diff --git a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t index 9abe3bdc8e..ed8c03f76f 100755 --- a/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t +++ b/t/db_dependent/Circulation/SwitchOnSiteCheckouts.t @@ -69,9 +69,9 @@ Koha::CirculationRules->set_rules( branchcode => $branch->{branchcode}, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 2, - maxonsiteissueqty => 1, lengthunit => 'days', issuelength => 5, hardduedate => undef, @@ -79,6 +79,17 @@ Koha::CirculationRules->set_rules( } } ); +Koha::CirculationRules->set_rules( + { + branchcode => $branch->{branchcode}, + categorycode => undef, + itemtype => undef, + onsite_checkout => 1, + rules => { + maxissueqty => 1, + } + } +); t::lib::Mocks::mock_userenv({ patron => $patron }); @@ -132,9 +143,20 @@ Koha::CirculationRules->set_rules( branchcode => $branch->{branchcode}, categorycode => undef, itemtype => undef, + onsite_checkout => undef, + rules => { + maxissueqty => 2, + } + } +); +Koha::CirculationRules->set_rules( + { + branchcode => $branch->{branchcode}, + categorycode => undef, + itemtype => undef, + onsite_checkout => 1, rules => { maxissueqty => 2, - maxonsiteissueqty => 1, } } ); diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index d1663d98a3..6ba677230c 100755 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -99,9 +99,9 @@ subtest '1 Issuingrule exist 0 0: no issue allowed' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 0, - maxonsiteissueqty => 0, } }, ); @@ -156,9 +156,20 @@ subtest '1 Issuingrule exist with onsiteissueqty=unlimited' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => undef, + } + }, + ); + Koha::CirculationRules->set_rules( + { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => undef, + onsite_checkout => 1, + rules => { + maxissueqty => undef, } }, ); @@ -211,9 +222,9 @@ subtest '1 Issuingrule exist 1 1: issue is allowed' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -251,9 +262,9 @@ subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed. Do a CO' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -307,9 +318,9 @@ subtest '1 Issuingrule exist: 1 CO allowed, 1 OSCO allowed, Do a OSCO' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -366,9 +377,9 @@ subtest '1 BranchBorrowerCircRule exist: 1 CO allowed, 1 OSCO allowed' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -418,9 +429,9 @@ subtest '1 BranchBorrowerCircRule exist: 1 CO allowed, 1 OSCO allowed' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -496,6 +507,7 @@ subtest 'General vs specific rules limit quantity correctly' => sub { categorycode => '*', itemtype => $itemtype->{itemtype}, branchcode => '*', + onsite_checkout => '*', rules => { issuelength => 1, firstremind => 1, # 1 day of grace @@ -510,9 +522,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub { { branchcode => $branch->{branchcode}, categorycode => '*', + onsite_checkout => undef, rules => { patron_maxissueqty => 1, - patron_maxonsiteissueqty => 1, } } ); @@ -523,9 +535,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub { branchcode => '*', categorycode => '*', itemtype => $itemtype->{itemtype}, + onsite_checkout => '*', rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -576,9 +588,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => $itemtype->{itemtype}, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -672,9 +684,9 @@ subtest 'General vs specific rules limit quantity correctly' => sub { branchcode => $branch2->{branchcode}, categorycode => $category->{categorycode}, itemtype => $itemtype->{itemtype}, + onsite_checkout => undef, rules => { maxissueqty => 1, - maxonsiteissueqty => 1, } } ); @@ -694,22 +706,22 @@ subtest 'empty string means unlimited' => sub { branchcode => '*', categorycode => '*', itemtype => '*', + onsite_checkout => '*', rules => { maxissueqty => '', - maxonsiteissueqty => '', } }, ); is( C4::Circulation::TooMany( $patron, $item ), undef, - 'maxissueqty="" should mean unlimited' + 'maxissueqty="" should mean unlimited for normal checkouts' ); is( C4::Circulation::TooMany( $patron, $item, { onsite_checkout => 1 } ), undef, - 'maxonsiteissueqty="" should mean unlimited' + 'maxissueqty="" should mean unlimited for onsite checkouts' ); }; @@ -722,9 +734,9 @@ subtest 'itemtype group tests' => sub { branchcode => '*', categorycode => '*', itemtype => '*', + onsite_checkout => '*', rules => { maxissueqty => '', - maxonsiteissueqty => '', issuelength => 1, firstremind => 1, # 1 day of grace finedays => 2, # 2 days of fine per day of overdue @@ -796,6 +808,7 @@ subtest 'itemtype group tests' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => undef, + onsite_checkout => undef, rule_name => 'maxissueqty', rule_value => 1 } @@ -818,6 +831,7 @@ subtest 'itemtype group tests' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => $parent_itype->{itemtype}, + onsite_checkout => undef, rule_name => 'maxissueqty', rule_value => 2 } @@ -834,6 +848,7 @@ subtest 'itemtype group tests' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => $child_itype_1->{itemtype}, + onsite_checkout => undef, rule_name => 'maxissueqty', rule_value => 1 } @@ -865,6 +880,7 @@ subtest 'itemtype group tests' => sub { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, itemtype => $child_itype_2->{itemtype}, + onsite_checkout => undef, rule_name => 'maxissueqty', rule_value => 3 } -- 2.17.1