Bugzilla – Attachment 71041 Details for
Bug 18936
Move issuingrules into circulation_rules
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules
Bug-18936-follow-up-fix-tests-null-vs-empty-behavi.patch (text/plain), 16.96 KB, created by
Jesse Weaver
on 2018-01-29 23:08:38 UTC
(
hide
)
Description:
Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules
Filename:
MIME Type:
Creator:
Jesse Weaver
Created:
2018-01-29 23:08:38 UTC
Size:
16.96 KB
patch
obsolete
>From ab74fa2b7ab072c3f341b90672c2c1b26c4e2256 Mon Sep 17 00:00:00 2001 >From: Jesse Weaver <pianohacker@gmail.com> >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 | 54 +++++++++++++++++----- > t/db_dependent/TestBuilder.t | 2 - > 8 files changed, 100 insertions(+), 59 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index b0ab5eeda5..bb2310eb7f 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -418,7 +418,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 >@@ -484,7 +484,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', >@@ -2314,7 +2314,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 4c280d660e..06dafbba67 100755 >--- a/admin/smart-rules.pl >+++ b/admin/smart-rules.pl >@@ -246,7 +246,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'); >@@ -255,20 +255,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'); >@@ -279,7 +279,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"; > >@@ -308,6 +308,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( >@@ -316,8 +318,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 353de4171d..37cdf8dba2 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 >@@ -171,14 +171,14 @@ > <a class="btn btn-default btn-xs delete" href="/cgi-bin/koha/admin/smart-rules.pl?op=delete&itemtype=[% rule.itemtype || '*' %]&categorycode=[% rule.categorycode || '*' %]&branch=[% current_branch %]"><i class="fa fa-trash"></i> Delete</a> > </td> > <td> >- [% IF maxissueqty %] >+ [% IF maxissueqty.defined && maxissueqty != '' %] > [% maxissueqty %] > [% ELSE %] > Unlimited > [% END %] > </td> > <td> >- [% 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 716371debf..ec577f18fb 100755 >--- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t >+++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t >@@ -7,7 +7,7 @@ use C4::Items; > use C4::Circulation; > use Koha::CirculationRules; > >-use Test::More tests => 4; >+use Test::More tests => 5; > > use t::lib::TestBuilder; > use t::lib::Mocks; >@@ -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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18936
:
65247
|
65248
|
65249
|
67629
|
67758
|
68236
|
71034
|
71035
|
71036
|
71037
|
71038
|
71039
|
71040
|
71041
|
72230
|
72231
|
72232
|
72233
|
72234
|
72235
|
72236
|
72237
|
72238
|
91953
|
91954
|
91955
|
91956
|
91957
|
91958
|
91959
|
91960
|
91961
|
91962
|
91963
|
91964
|
91965
|
91966
|
91967
|
91968
|
91969
|
91970
|
91971
|
91972
|
91973
|
91974
|
91975
|
94179
|
94180
|
94181
|
94182
|
94183
|
94184
|
94185
|
94186
|
94187
|
94188
|
94189
|
94190
|
94191
|
94192
|
94193
|
94194
|
94195
|
94196
|
94197
|
94198
|
94199
|
94200
|
98299
|
98300
|
98301
|
98302
|
98303
|
98304
|
98305
|
98306
|
98307
|
98308
|
98309
|
98310
|
98311
|
98312
|
98313
|
98314
|
98315
|
98316
|
98317
|
98318
|
98319
|
98320
|
98321
|
98322
|
98323
|
98324
|
98325
|
98326
|
98327
|
98328
|
98329
|
98330
|
98331
|
98332
|
98333
|
98334
|
98335
|
98336
|
98337
|
98338
|
98339
|
98340
|
98341
|
98342
|
98343
|
98344
|
98345
|
98346
|
98347
|
98407
|
98408