From 00c90f28d8ede5564300ff9945868ea36b37de36 Mon Sep 17 00:00:00 2001 From: Jesse Weaver Date: Mon, 29 Jan 2018 15:54:40 -0700 Subject: [PATCH] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules --- C4/Circulation.pm | 6 +-- admin/smart-rules.pl | 18 ++++---- installer/data/mysql/atomicupdate/bug_18936.perl | 4 +- .../prog/en/modules/admin/smart-rules.tt | 4 +- t/db_dependent/Circulation.t | 50 ++++++++++++--------- t/db_dependent/Circulation/TooMany.t | 21 ++++----- .../Holds/DisallowHoldIfItemsAvailable.t | 52 +++++++++++++++++----- t/db_dependent/TestBuilder.t | 2 - 8 files changed, 99 insertions(+), 58 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 94aff6f078..65913c25ab 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -405,7 +405,7 @@ sub TooMany { # if a rule is found and has a loan limit set, count # how many loans the patron already has that meet that # rule - if (defined($maxissueqty_rule) and defined($maxissueqty_rule->rule_value)) { + if (defined($maxissueqty_rule) and $maxissueqty_rule->rule_value ne '') { my @bind_params; my $count_query = q| SELECT COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts @@ -471,7 +471,7 @@ sub TooMany { my $max_checkouts_allowed = $maxissueqty_rule ? $maxissueqty_rule->rule_value : 0; my $max_onsite_checkouts_allowed = $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : 0; - if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) { + if ( $onsite_checkout and $max_onsite_checkouts_allowed ne '' ) { if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { return { reason => 'TOO_MANY_ONSITE_CHECKOUTS', @@ -2279,7 +2279,7 @@ sub _debar_user_on_return { # If the max suspension days is < than the suspension days # the suspension days is limited to this maximum period. my $max_sd = $issuing_rule->{maxsuspensiondays}; - if ( defined $max_sd ) { + if ( defined $max_sd && $max_sd ne '' ) { $max_sd = DateTime::Duration->new( days => $max_sd ); $suspension_days = $max_sd if DateTime::Duration->compare( $max_sd, $suspension_days ) < 0; diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index b9ee140c2d..e4a9bdd571 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -238,7 +238,7 @@ elsif ($op eq 'add') { my $fine = $input->param('fine'); my $finedays = $input->param('finedays'); my $maxsuspensiondays = $input->param('maxsuspensiondays'); - $maxsuspensiondays = undef if $maxsuspensiondays eq q||; + $maxsuspensiondays = '' if $maxsuspensiondays eq q||; my $firstremind = $input->param('firstremind'); my $chargeperiod = $input->param('chargeperiod'); my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at'); @@ -247,20 +247,20 @@ elsif ($op eq 'add') { my $renewalsallowed = $input->param('renewalsallowed'); my $renewalperiod = $input->param('renewalperiod'); my $norenewalbefore = $input->param('norenewalbefore'); - $norenewalbefore = undef if $norenewalbefore =~ /^\s*$/; + $norenewalbefore = '' if $norenewalbefore =~ /^\s*$/; my $auto_renew = $input->param('auto_renew') eq 'yes' ? 1 : 0; my $no_auto_renewal_after = $input->param('no_auto_renewal_after'); - $no_auto_renewal_after = undef if $no_auto_renewal_after =~ /^\s*$/; - my $no_auto_renewal_after_hard_limit = $input->param('no_auto_renewal_after_hard_limit') || undef; + $no_auto_renewal_after = '' if $no_auto_renewal_after =~ /^\s*$/; + my $no_auto_renewal_after_hard_limit = $input->param('no_auto_renewal_after_hard_limit') || ''; $no_auto_renewal_after_hard_limit = eval { dt_from_string( $input->param('no_auto_renewal_after_hard_limit') ) } if ( $no_auto_renewal_after_hard_limit ); $no_auto_renewal_after_hard_limit = output_pref( { dt => $no_auto_renewal_after_hard_limit, dateonly => 1, dateformat => 'iso' } ) if ( $no_auto_renewal_after_hard_limit ); my $reservesallowed = $input->param('reservesallowed'); my $holds_per_record = $input->param('holds_per_record'); my $onshelfholds = $input->param('onshelfholds') || 0; $maxissueqty =~ s/\s//g; - $maxissueqty = undef if $maxissueqty !~ /^\d+/; + $maxissueqty = '' if $maxissueqty !~ /^\d+/; $maxonsiteissueqty =~ s/\s//g; - $maxonsiteissueqty = undef if $maxonsiteissueqty !~ /^\d+/; + $maxonsiteissueqty = '' if $maxonsiteissueqty !~ /^\d+/; my $issuelength = $input->param('issuelength'); $issuelength = $issuelength eq q{} ? undef : $issuelength; my $lengthunit = $input->param('lengthunit'); @@ -271,7 +271,7 @@ elsif ($op eq 'add') { my $rentaldiscount = $input->param('rentaldiscount'); my $opacitemholds = $input->param('opacitemholds') || 0; my $article_requests = $input->param('article_requests') || 'no'; - my $overduefinescap = $input->param('overduefinescap') || undef; + my $overduefinescap = $input->param('overduefinescap') || ''; my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price') eq 'on'; warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price"; @@ -300,6 +300,8 @@ elsif ($op eq 'add') { overduefinescap => $overduefinescap, cap_fine_to_replacement_price => $cap_fine_to_replacement_price, article_requests => $article_requests, + maxissueqty => $maxissueqty, + maxonsiteissueqty => $maxonsiteissueqty, }; Koha::CirculationRules->set_rules( @@ -308,8 +310,6 @@ elsif ($op eq 'add') { itemtype => $itemtype eq '*' ? undef : $itemtype, branchcode => $br eq '*' ? undef : $br, rules => { - maxissueqty => $maxissueqty, - maxonsiteissueqty => $maxonsiteissueqty, %$params, } } diff --git a/installer/data/mysql/atomicupdate/bug_18936.perl b/installer/data/mysql/atomicupdate/bug_18936.perl index b420fce6f3..935f71844c 100644 --- a/installer/data/mysql/atomicupdate/bug_18936.perl +++ b/installer/data/mysql/atomicupdate/bug_18936.perl @@ -27,13 +27,15 @@ if( CheckVersion( $DBversion ) ) { onshelfholds opacitemholds article_requests + maxissueqty + maxonsiteissueqty ); if ( column_exists( 'issuingrules', 'categorycode' ) ) { foreach my $column ( @columns ) { $dbh->do(" INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) - SELECT categorycode, branchcode, itemtype, \'$column\', $column + SELECT categorycode, branchcode, itemtype, \'$column\', COALESCE( $column, '' ) FROM issuingrules "); } 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 0a21aac374..a5049ec744 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 @@ -169,14 +169,14 @@ Delete - [% IF maxissueqty %] + [% IF maxissueqty.defined && maxissueqty != '' %] [% maxissueqty %] [% ELSE %] Unlimited [% END %] - [% IF maxonsiteissueqty %] + [% IF maxonsiteissueqty.defined && maxonsiteissueqty != '' %] [% maxonsiteissueqty %] [% ELSE %] Unlimited diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index d03452d617..fae14a37a0 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -817,7 +817,17 @@ C4::Context->dbh->do("DELETE FROM accountlines"); } }); - $dbh->do('UPDATE issuingrules SET norenewalbefore = 10, no_auto_renewal_after = 11'); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + norenewalbefore => 10, + no_auto_renewal_after => 11, + } + } + ); my $ten_days_before = dt_from_string->add( days => -10 ); my $ten_days_ahead = dt_from_string->add( days => 10 ); @@ -2221,26 +2231,24 @@ subtest 'CanBookBeIssued | is_overdue' => sub { plan tests => 3; # Set a simple circ policy - $dbh->do('DELETE FROM issuingrules'); - $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, - maxissueqty, issuelength, lengthunit, - renewalsallowed, renewalperiod, - norenewalbefore, auto_renew, - fine, chargeperiod) - VALUES (?, ?, ?, ?, - ?, ?, ?, - ?, ?, - ?, ?, - ?, ? - ) - }, - {}, - '*', '*', '*', 25, - 1, 14, 'days', - 1, 7, - undef, 0, - .10, 1 + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + reservesallowed => 25, + maxissueqty => 1, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + } + } ); my $five_days_go = output_pref({ dt => dt_from_string->add( days => 5 ), dateonly => 1}); diff --git a/t/db_dependent/Circulation/TooMany.t b/t/db_dependent/Circulation/TooMany.t index 11a08945e6..5dce2ced49 100644 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -158,16 +158,17 @@ subtest '1 Issuingrule exist 0 0: no issue allowed' => sub { subtest '1 Issuingrule exist with onsiteissueqty=unlimited' => sub { plan tests => 4; - my $issuingrule = $builder->build({ - source => 'Issuingrule', - value => { - branchcode => $branch->{branchcode}, - categorycode => $category->{categorycode}, - itemtype => '*', - maxissueqty => 1, - maxonsiteissueqty => undef, - }, - }); + Koha::CirculationRules->set_rules( + { + branchcode => $branch->{branchcode}, + categorycode => $category->{categorycode}, + itemtype => undef, + rules => { + maxissueqty => 1, + maxonsiteissueqty => '', + } + } + ); my $issue = C4::Circulation::AddIssue( $patron, $item->{barcode}, dt_from_string() ); t::lib::Mocks::mock_preference('ConsiderOnSiteCheckoutsAsNormalCheckouts', 0); is_deeply( diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index c4842b4689..f72a2d9667 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -128,8 +128,16 @@ AddReturn( $item1->{barcode} ); my $hold_allowed_from_home_library = 1; my $hold_allowed_from_any_libraries = 2; - my $sth_delete_rules = $dbh->prepare(q|DELETE FROM default_circ_rules|); - my $sth_insert_rule = $dbh->prepare(q|INSERT INTO default_circ_rules(singleton, maxissueqty, maxonsiteissueqty, holdallowed, hold_fulfillment_policy, returnbranch) VALUES ('singleton', NULL, NULL, ?, 'any', 'homebranch');|); + Koha::CirculationRules->set_rules( + { + branchcode => undef, + itemtype => undef, + rules => { + hold_fulfillment_policy => 'any', + returnbranch => 'homebranch', + } + } + ); subtest 'Item is available at a different library' => sub { plan tests => 4; @@ -145,8 +153,14 @@ AddReturn( $item1->{barcode} ); #FIXME: ReservesControlBranch is not checked in these subs we are testing { - $sth_delete_rules->execute; - $sth_insert_rule->execute( $hold_allowed_from_home_library ); + Koha::CirculationRules->set_rule( + { + branchcode => undef, + itemtype => undef, + rule_name => 'holdallowed', + rule_value => $hold_allowed_from_home_library, + } + ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); $is = IsAvailableForItemLevelRequest( $item1, $borrower1); @@ -158,8 +172,14 @@ AddReturn( $item1->{barcode} ); } { - $sth_delete_rules->execute; - $sth_insert_rule->execute( $hold_allowed_from_any_libraries ); + Koha::CirculationRules->set_rule( + { + branchcode => undef, + itemtype => undef, + rule_name => 'holdallowed', + rule_value => $hold_allowed_from_any_libraries, + } + ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); $is = IsAvailableForItemLevelRequest( $item1, $borrower1); @@ -185,8 +205,14 @@ AddReturn( $item1->{barcode} ); #ReservesControlBranch is not checked in these subs we are testing? { - $sth_delete_rules->execute; - $sth_insert_rule->execute( $hold_allowed_from_home_library ); + Koha::CirculationRules->set_rule( + { + branchcode => undef, + itemtype => undef, + rule_name => 'holdallowed', + rule_value => $hold_allowed_from_home_library, + } + ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); $is = IsAvailableForItemLevelRequest( $item1, $borrower1); @@ -198,8 +224,14 @@ AddReturn( $item1->{barcode} ); } { - $sth_delete_rules->execute; - $sth_insert_rule->execute( $hold_allowed_from_any_libraries ); + Koha::CirculationRules->set_rule( + { + branchcode => undef, + itemtype => undef, + rule_name => 'holdallowed', + rule_value => $hold_allowed_from_any_libraries, + } + ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); $is = IsAvailableForItemLevelRequest( $item1, $borrower1); diff --git a/t/db_dependent/TestBuilder.t b/t/db_dependent/TestBuilder.t index 3b113ea3a7..c08a175adc 100644 --- a/t/db_dependent/TestBuilder.t +++ b/t/db_dependent/TestBuilder.t @@ -34,8 +34,6 @@ our $schema = Koha::Database->new->schema; $schema->storage->txn_begin; our $builder; -$schema->resultset('DefaultCircRule')->delete; # Is a singleton table - subtest 'Start with some trivial tests' => sub { plan tests => 7; -- 2.15.1