From 6d4d6996741560b9bdf882a2bcfeca3c8f9462e7 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Tue, 31 Mar 2020 16:25:59 +0000 Subject: [PATCH] Bug 25089: 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 3. Search for maxonsiteissueqty rule $ grep -rn 'maxonsiteissueqty' 4. Observe no results (apart from .git) 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 --- admin/smart-rules.pl | 19 +------ ...bug_25089_2_convert_circulation_rules.perl | 33 +++++++++++++ installer/onboarding.pl | 2 - .../prog/en/modules/admin/smart-rules.tt | 37 ++------------ t/db_dependent/Circulation/Branch.t | 47 ++++++++++++------ .../Circulation/SwitchOnSiteCheckouts.t | 26 +++++++++- t/db_dependent/Circulation/TooMany.t | 36 +++++++++----- 9 files changed, 138 insertions(+), 117 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_25089_2_convert_circulation_rules.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index efdc2b15a0..acc1f14786 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -385,6 +385,11 @@ sub TooMany { # Get which branchcode we need $branch = _GetCircControlBranch($item_object->unblessed,$borrower); my $type = $item_object->effective_itemtype; + my $checkout_type = $onsite_checkout + ? $switch_onsite_checkout + ? $Koha::Checkouts::type->{checkout} + : $Koha::Checkouts::type->{onsite_checkout} + : $Koha::Checkouts::type->{checkout}; # given branch, patron category, and item type, determine # applicable issuing rule @@ -396,14 +401,6 @@ sub TooMany { rule_name => 'maxissueqty', } ); - my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule( - { - categorycode => $cat_borrower, - itemtype => $type, - branchcode => $branch, - rule_name => 'maxonsiteissueqty', - } - ); # if a rule is found and has a loan limit set, count @@ -473,14 +470,13 @@ sub TooMany { my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $count_query, {}, @bind_params ); my $max_checkouts_allowed = $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef; - my $max_onsite_checkouts_allowed = $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : undef; - if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) { - if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { + if ( $onsite_checkout and $max_checkouts_allowed ne '' ) { + 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, } } } @@ -505,7 +501,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, $checkout_type ); if (defined($branch_borrower_circ_rule->{patron_maxissueqty}) and $branch_borrower_circ_rule->{patron_maxissueqty} ne '') { my @bind_params = (); my $branch_count_query = qq| @@ -527,14 +523,13 @@ sub TooMany { } my ( $checkout_count, $onsite_checkout_count ) = $dbh->selectrow_array( $branch_count_query, {}, @bind_params ); my $max_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxissueqty}; - my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{patron_maxonsiteissueqty}; - if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) { - if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { + if ( $onsite_checkout and $max_checkouts_allowed ne '' ) { + 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, } } } @@ -1601,7 +1596,7 @@ sub GetHardDueDate { =head2 GetBranchBorrowerCircRule - my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode); + my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode, $checkout_type); Retrieves circulation rule attributes that apply to the given branch and patron category, regardless of item type. @@ -1611,21 +1606,13 @@ 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 circulation +rules user interface. 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 @@ -1634,21 +1621,21 @@ wildcards. =cut sub GetBranchBorrowerCircRule { - my ( $branchcode, $categorycode ) = @_; + my ( $branchcode, $categorycode, $checkout_type ) = @_; # 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, + checkout_type => $checkout_type, rule_name => $rule_name, } ); diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index 4b8d8d8675..c267930bda 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -53,9 +53,6 @@ our $RULE_KINDS = { patron_maxissueqty => { scope => [ 'branchcode', 'categorycode', 'checkout_type' ], }, - patron_maxonsiteissueqty => { - scope => [ 'branchcode', 'categorycode' ], - }, max_holds => { scope => [ 'branchcode', 'categorycode' ], }, @@ -115,9 +112,6 @@ our $RULE_KINDS = { maxissueqty => { scope => [ 'branchcode', 'categorycode', 'itemtype', 'checkout_type' ], }, - maxonsiteissueqty => { - scope => [ 'branchcode', 'categorycode', 'itemtype' ], - }, maxsuspensiondays => { scope => [ 'branchcode', 'categorycode', 'itemtype', 'checkout_type' ], }, diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 6bea920dd9..941abdae73 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -84,7 +84,6 @@ if ($op eq 'delete') { itemtype => $itemtype eq '*' ? undef : $itemtype, rules => { maxissueqty => undef, - maxonsiteissueqty => undef, rentaldiscount => undef, fine => undef, finedays => undef, @@ -127,7 +126,6 @@ elsif ($op eq 'delete-branch-cat') { rules => { max_holds => undef, patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, } } ); @@ -150,7 +148,6 @@ elsif ($op eq 'delete-branch-cat') { rules => { max_holds => undef, patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, } } ); @@ -162,7 +159,6 @@ elsif ($op eq 'delete-branch-cat') { categorycode => undef, rules => { patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, } } ); @@ -185,7 +181,6 @@ elsif ($op eq 'delete-branch-cat') { rules => { max_holds => undef, patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, } } ); @@ -260,7 +255,6 @@ elsif ($op eq 'add') { my $chargeperiod = $input->param('chargeperiod'); my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at'); my $maxissueqty = strip_non_numeric($input->param('maxissueqty')); - my $maxonsiteissueqty = strip_non_numeric($input->param('maxonsiteissueqty')); my $renewalsallowed = $input->param('renewalsallowed'); my $renewalperiod = $input->param('renewalperiod'); my $norenewalbefore = $input->param('norenewalbefore'); @@ -288,11 +282,10 @@ elsif ($op eq 'add') { my $overduefinescap = $input->param('overduefinescap') || ''; my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price') eq 'on'; my $note = $input->param('note'); - $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price"; + $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $cap_fine_to_replacement_price"; my $rules = { maxissueqty => $maxissueqty, - maxonsiteissueqty => $maxonsiteissueqty, rentaldiscount => $rentaldiscount, fine => $fine, finedays => $finedays, @@ -335,8 +328,6 @@ elsif ($op eq 'add') { elsif ($op eq "set-branch-defaults") { my $categorycode = $input->param('categorycode'); my $patron_maxissueqty = strip_non_numeric($input->param('patron_maxissueqty')); - my $patron_maxonsiteissueqty = $input->param('patron_maxonsiteissueqty'); - $patron_maxonsiteissueqty = strip_non_numeric($patron_maxonsiteissueqty); my $holdallowed = $input->param('holdallowed'); my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy'); my $returnbranch = $input->param('returnbranch'); @@ -362,7 +353,6 @@ elsif ($op eq "set-branch-defaults") { branchcode => undef, rules => { patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); @@ -384,7 +374,6 @@ elsif ($op eq "set-branch-defaults") { branchcode => $branch, rules => { patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); @@ -401,8 +390,6 @@ elsif ($op eq "set-branch-defaults") { elsif ($op eq "add-branch-cat") { my $categorycode = $input->param('categorycode'); my $patron_maxissueqty = strip_non_numeric($input->param('patron_maxissueqty')); - my $patron_maxonsiteissueqty = $input->param('patron_maxonsiteissueqty'); - $patron_maxonsiteissueqty = strip_non_numeric($patron_maxonsiteissueqty); my $max_holds = $input->param('max_holds'); $max_holds =~ s/\s//g; $max_holds = undef if $max_holds !~ /^\d+/; @@ -416,7 +403,6 @@ elsif ($op eq "add-branch-cat") { rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); @@ -428,7 +414,6 @@ elsif ($op eq "add-branch-cat") { rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); @@ -441,7 +426,6 @@ elsif ($op eq "add-branch-cat") { rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); @@ -453,7 +437,6 @@ elsif ($op eq "add-branch-cat") { rules => { max_holds => $max_holds, patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, } } ); diff --git a/installer/data/mysql/atomicupdate/bug_25089_2_convert_circulation_rules.perl b/installer/data/mysql/atomicupdate/bug_25089_2_convert_circulation_rules.perl new file mode 100644 index 0000000000..19c5a994d1 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_25089_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 checkout_type = ?, + rule_name = ? + WHERE rule_name=? + |); + $update_sth->execute( + 'OS', # checkout_type => 'OS' (on-site checkout) + 'maxissueqty', + 'maxonsiteissueqty' + ); + + # Then, convert patron_maxonsiteissueqty to patron_maxissueqty + $update_sth = $dbh->prepare(q| + UPDATE circulation_rules + SET checkout_type = ?, + rule_name = ? + WHERE rule_name=? + |); + $update_sth->execute( + 'OS', # checkout_type => 'OS' (on-site checkout) + 'patron_maxissueqty', + 'patron_maxonsiteissueqty' + ); + + # Always end with this (adjust the bug info) + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 25089 - Convert maxonsiteissueqty to maxissueqty )\n"; +} diff --git a/installer/onboarding.pl b/installer/onboarding.pl index 547df29e0d..dab7980ee9 100755 --- a/installer/onboarding.pl +++ b/installer/onboarding.pl @@ -273,7 +273,6 @@ if ( $step == 5 ) { holds_per_day => $holds_per_day, holds_per_record => $holds_per_record, maxissueqty => "", - maxonsiteissueqty => "", maxsuspensiondays => "", no_auto_renewal_after => "", no_auto_renewal_after_hard_limit => "", @@ -291,7 +290,6 @@ if ( $step == 5 ) { categorycode => $categorycode, rules => { patron_maxissueqty => "", - patron_maxonsiteissueqty => "", max_holds => "", } }; 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 5ae3bb94e1..60b0d19eac 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 @@ -91,7 +91,6 @@ Actions Note Current checkouts allowed - Current on-site checkouts allowed Loan period Unit Hard due date @@ -128,7 +127,6 @@ [% SET i = '' UNLESS i.defined %] [% SET note = all_rules.$c.$i.note %] [% SET maxissueqty = all_rules.$c.$i.maxissueqty %] - [% SET maxonsiteissueqty = all_rules.$c.$i.maxonsiteissueqty %] [% SET issuelength = all_rules.$c.$i.issuelength %] [% SET lengthunit = all_rules.$c.$i.lengthunit %] [% SET hardduedate = all_rules.$c.$i.hardduedate %] @@ -156,7 +154,7 @@ [% SET article_requests = all_rules.$c.$i.article_requests %] [% SET rentaldiscount = all_rules.$c.$i.rentaldiscount %] - [% SET show_rule = maxissueqty || maxonsiteissueqty || issuelength || lengthunit || hardduedate || hardduedatebefore || hardduedateexact || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalsallowed || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || article_requests %] + [% SET show_rule = maxissueqty || issuelength || lengthunit || hardduedate || hardduedatebefore || hardduedateexact || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalsallowed || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || article_requests %] [% IF show_rule %] [% SET row_count = row_count + 1 %] @@ -190,13 +188,6 @@ Unlimited [% END %] - - [% IF maxonsiteissueqty.defined && maxonsiteissueqty != '' %] - [% maxonsiteissueqty | html %] - [% ELSE %] - Unlimited - [% END %] - [% issuelength | html %] [% IF ( lengthunit == 'days' ) %] @@ -333,7 +324,6 @@ - - - [% SET patron_maxonsiteissueqty = CirculationRules.Search( current_branch, undef, undef, 'patron_maxonsiteissueqty' ) %] - - [% SET rule_value = CirculationRules.Search( current_branch, undef , undef, 'max_holds' ) %] @@ -635,17 +619,15 @@ Patron category Total current checkouts allowed - Total current on-site checkouts allowed Total holds allowed   [% 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' ) %] - [% IF ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %] + [% IF ( patron_maxissueqty.defined && patron_maxissueqty != '' ) || ( max_holds.defined && max_holds != '' ) %] [% IF c == undef %] @@ -661,13 +643,6 @@ Unlimited [% END %] - - [% IF patron_maxonsiteissueqty.defined && patron_maxonsiteissueqty != '' %] - [% patron_maxonsiteissueqty | html %] - [% ELSE %] - Unlimited - [% END %] - [% IF max_holds.defined && max_holds != '' %] [% max_holds | html %] @@ -691,7 +666,6 @@ -