From 5712ec566f46b007d610c0d7cbdc40d43c444d24 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 MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Minna Kivinen Signed-off-by: Joonas Kylmälä Signed-off-by: Martin Renvoize --- C4/Circulation.pm | 4 +- admin/smart-rules.pl | 9 ++-- .../data/mysql/atomicupdate/bug_18936.perl | 4 +- .../prog/en/modules/admin/smart-rules.tt | 4 +- t/db_dependent/Circulation.t | 49 +++++++++++-------- t/db_dependent/Circulation/TooMany.t | 2 +- 6 files changed, 41 insertions(+), 31 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 6c10b3d905..0027f81a9e 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -473,7 +473,7 @@ sub TooMany { 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 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', @@ -2303,7 +2303,7 @@ sub _calculate_new_debar_dt { # 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 67f72e6c83..8a228075b1 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -253,6 +253,7 @@ elsif ($op eq 'add') { my $finedays = $input->param('finedays'); my $maxsuspensiondays = $input->param('maxsuspensiondays'); $maxsuspensiondays = undef if $maxsuspensiondays eq q||; + $maxsuspensiondays = '' if $maxsuspensiondays eq q||; my $suspension_chargeperiod = $input->param('suspension_chargeperiod') || 1; my $firstremind = $input->param('firstremind'); my $chargeperiod = $input->param('chargeperiod'); @@ -262,11 +263,11 @@ 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'); @@ -289,7 +290,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'; my $note = $input->param('note'); $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price"; 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 7ad46a2d62..8bb1381eb5 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 @@ -185,14 +185,14 @@ [% ELSE %] [% END %] - [% 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 cebe7dcb5d..2bff2e4fb1 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -914,7 +914,17 @@ subtest "CanBookBeRenewed tests" => sub { } }); - $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 ); @@ -2892,26 +2902,23 @@ 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, - issuelength, lengthunit, - renewalsallowed, renewalperiod, - norenewalbefore, auto_renew, - fine, chargeperiod) - VALUES (?, ?, ?, ?, - ?, ?, - ?, ?, - ?, ?, - ?, ? - ) - }, - {}, - '*', '*', '*', 25, - 14, 'days', - 1, 7, - undef, 0, - .10, 1 + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + reservesallowed => 25, + 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 7e4584435d..8a7cf3ae52 100644 --- a/t/db_dependent/Circulation/TooMany.t +++ b/t/db_dependent/Circulation/TooMany.t @@ -164,7 +164,7 @@ subtest '1 Issuingrule exist with onsiteissueqty=unlimited' => sub { { branchcode => $branch->{branchcode}, categorycode => $category->{categorycode}, - itemtype => '*', + itemtype => undef, rules => { maxissueqty => 1, maxonsiteissueqty => undef, -- 2.39.5