Bugzilla – Attachment 141869 Details for
Bug 29012
Some rules are not saved when left blank while editing a 'rule' line in smart-rules.pl
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 29012: Add default values for blank circulation rules that weren't saved to the database
Bug-29012-Add-default-values-for-blank-circulation.patch (text/plain), 13.54 KB, created by
Kevin Carnes
on 2022-10-14 09:20:31 UTC
(
hide
)
Description:
Bug 29012: Add default values for blank circulation rules that weren't saved to the database
Filename:
MIME Type:
Creator:
Kevin Carnes
Created:
2022-10-14 09:20:31 UTC
Size:
13.54 KB
patch
obsolete
>From b03f6c1b4a4da2081531c9607df92772046e8b0c Mon Sep 17 00:00:00 2001 >From: Kevin Carnes <kevin.carnes@ub.lu.se> >Date: Fri, 14 Oct 2022 10:51:47 +0200 >Subject: [PATCH] Bug 29012: Add default values for blank circulation rules > that weren't saved to the database > >There are 5 fields that are not set if no value is provided when saving/editing a rule in Administration->Circulation and fines rules >- issuelength >- hardduedate >- unseenrenewalsallowed >- rentaldiscount >- decreaseloanholds > >This is problematic because it gives the impression these rules are set as blank, but in reality they don't exist and the rule will fal back to the higher level > >To test: >1 - Set a rule for > Patron category: Teacher > Itemtype: All > Hard due date: (Today) > Lona period: 10 >2 - Set a rule for > Patron category: Teacher > Itemtype: Books > Hard due date: (leave blank) > Loan period: 10 >3 - Expected behaviour is Book item will checkout to teacher for 10 days, all other types will be due yesterday at 25:59:00 >4 - Checkout an non-book item type to teacher >5 - Hard due date applies >6 - Checkout a 'book' item type to teacher >7 - Hard due date applies - FAIL >--- > C4/Circulation.pm | 4 +- > admin/smart-rules.pl | 31 +++++++------- > .../data/mysql/atomicupdate/bug_29012.pl | 41 +++++++++++++++++++ > installer/onboarding.pl | 8 ++-- > .../prog/en/modules/admin/smart-rules.tt | 8 +++- > 5 files changed, 69 insertions(+), 23 deletions(-) > create mode 100755 installer/data/mysql/atomicupdate/bug_29012.pl > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 5b57575f3b..9cb56f79d3 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -2906,7 +2906,7 @@ sub CanBookBeRenewed { > > return ( 0, "too_unseen" ) > if C4::Context->preference('UnseenRenewals') && >- $issuing_rule->{unseen_renewals_allowed} && >+ looks_like_number($issuing_rule->{unseen_renewals_allowed}) && > $issuing_rule->{unseen_renewals_allowed} <= $issue->unseen_renewals; > > my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing'); >@@ -3149,7 +3149,7 @@ sub AddRenewal { > rule_name => 'unseen_renewals_allowed' > } > ); >- if (!$seen && $rule && $rule->rule_value) { >+ if (!$seen && $rule && looks_like_number($rule->rule_value)) { > $unseen_renewals++; > } else { > # If the renewal is seen, unseen should revert to 0 >diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl >index d8fffaf924..9d992cbe97 100755 >--- a/admin/smart-rules.pl >+++ b/admin/smart-rules.pl >@@ -256,7 +256,7 @@ elsif ($op eq 'add') { > my $itemtype = $input->param('itemtype'); # item type > my $fine = $input->param('fine'); > my $finedays = $input->param('finedays'); >- my $maxsuspensiondays = $input->param('maxsuspensiondays') || ''; >+ my $maxsuspensiondays = $input->param('maxsuspensiondays') || q{}; > my $suspension_chargeperiod = $input->param('suspension_chargeperiod') || 1; > my $firstremind = $input->param('firstremind'); > my $chargeperiod = $input->param('chargeperiod'); >@@ -264,35 +264,34 @@ elsif ($op eq 'add') { > my $maxissueqty = strip_non_numeric( scalar $input->param('maxissueqty') ); > my $maxonsiteissueqty = strip_non_numeric( scalar $input->param('maxonsiteissueqty') ); > my $renewalsallowed = $input->param('renewalsallowed'); >- my $unseen_renewals_allowed = $input->param('unseen_renewals_allowed'); >+ my $unseen_renewals_allowed = defined $input->param('unseen_renewals_allowed') ? strip_non_numeric( scalar $input->param('unseen_renewals_allowed') ) : q{}; > my $renewalperiod = $input->param('renewalperiod'); > my $norenewalbefore = $input->param('norenewalbefore'); >- $norenewalbefore = '' if $norenewalbefore =~ /^\s*$/; >+ $norenewalbefore = q{} 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 = '' 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 = q{} if $no_auto_renewal_after =~ /^\s*$/; >+ my $no_auto_renewal_after_hard_limit = $input->param('no_auto_renewal_after_hard_limit') || q{}; > $no_auto_renewal_after_hard_limit = eval { dt_from_string( scalar $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 = strip_non_numeric( scalar $input->param('reservesallowed') ); > my $holds_per_record = strip_non_numeric( scalar $input->param('holds_per_record') ); > my $holds_per_day = strip_non_numeric( scalar $input->param('holds_per_day') ); > my $onshelfholds = $input->param('onshelfholds') || 0; >- my $issuelength = $input->param('issuelength'); >- $issuelength = $issuelength eq q{} ? undef : $issuelength; >+ my $issuelength = $input->param('issuelength') || 0; > my $daysmode = $input->param('daysmode'); > my $lengthunit = $input->param('lengthunit'); >- my $hardduedate = $input->param('hardduedate') || undef; >+ my $hardduedate = $input->param('hardduedate') || q{}; > $hardduedate = eval { dt_from_string( scalar $hardduedate ) } if ( $hardduedate ); > $hardduedate = output_pref( { dt => $hardduedate, dateonly => 1, dateformat => 'iso' } ) if ( $hardduedate ); > my $hardduedatecompare = $input->param('hardduedatecompare'); >- my $rentaldiscount = $input->param('rentaldiscount'); >+ my $rentaldiscount = $input->param('rentaldiscount') || 0; > my $opacitemholds = $input->param('opacitemholds') || 0; > my $article_requests = $input->param('article_requests') || 'no'; >- my $overduefinescap = $input->param('overduefinescap') || ''; >- my $cap_fine_to_replacement_price = ($input->param('cap_fine_to_replacement_price') || '') eq 'on'; >+ my $overduefinescap = $input->param('overduefinescap') || q{}; >+ my $cap_fine_to_replacement_price = ($input->param('cap_fine_to_replacement_price') || q{}) eq 'on'; > my $note = $input->param('note'); >- my $decreaseloanholds = $input->param('decreaseloanholds') || undef; >+ my $decreaseloanholds = $input->param('decreaseloanholds') || q{}; > my $recalls_allowed = $input->param('recalls_allowed'); > my $recalls_per_record = $input->param('recalls_per_record'); > my $on_shelf_recalls = $input->param('on_shelf_recalls'); >@@ -482,7 +481,7 @@ elsif ( $op eq "add-open-article-requests-limit" ) { > > Koha::Exception->throw("No value passed for article request limit") > if not defined $open_article_requests_limit # There is a JS check for that >- || $open_article_requests_limit eq ''; >+ || $open_article_requests_limit eq q{}; > > if ( $branch eq "*" ) { > if ( $categorycode eq "*" ) { >@@ -556,7 +555,7 @@ elsif ( $op eq "set-article-request-fee" ) { > > Koha::Exception->throw("No value passed for article request fee") > if not defined $fee # There is a JS check for that >- || $fee eq ''; >+ || $fee eq q{}; > > Koha::CirculationRules->set_rules( > { categorycode => ( $category eq '*' ) ? undef : $category, >@@ -714,7 +713,7 @@ my $definedbranch = $all_rules->count ? 1 : 0; > my $rules = {}; > while ( my $r = $all_rules->next ) { > $r = $r->unblessed; >- $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->{ $r->{rule_name} } = $r->{rule_value}; >+ $rules->{ $r->{categorycode} // q{} }->{ $r->{itemtype} // q{} }->{ $r->{rule_name} } = $r->{rule_value}; > } > > $template->param(show_branch_cat_rule_form => 1); >@@ -764,6 +763,6 @@ sub by_itemtype { > sub strip_non_numeric { > my $string = shift; > $string =~ s/\s//g; >- $string = '' if $string !~ /^\d+/; >+ $string = q{} if $string !~ /^\d+/; > return $string; > } >diff --git a/installer/data/mysql/atomicupdate/bug_29012.pl b/installer/data/mysql/atomicupdate/bug_29012.pl >new file mode 100755 >index 0000000000..7bf5243f6f >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/bug_29012.pl >@@ -0,0 +1,41 @@ >+use Modern::Perl; >+ >+return { >+ bug_number => "29012", >+ description => "Some rules are not saved when left blank while editing a 'rule' line in smart-rules.pl", >+ up => sub { >+ my ($args) = @_; >+ my ($dbh, $out) = @$args{qw(dbh out)}; >+ my %default_rule_values = ( >+ issuelength => 0, >+ hardduedate => '', >+ unseen_renewals_allowed => '', >+ rentaldiscount => 0, >+ decreaseloanholds => '', >+ ); >+ while (my ($rule_name, $rule_value) = each (%default_rule_values)) { >+ $dbh->do(q{ >+ INSERT IGNORE INTO circulation_rules (branchcode, categorycode, itemtype, rule_name, rule_value) >+ SELECT branchcode, categorycode, itemtype, ?, ? FROM circulation_rules cr >+ WHERE EXISTS ( >+ SELECT * FROM circulation_rules cr2 >+ WHERE >+ cr2.rule_name="suspension_chargeperiod" >+ AND ( (cr2.branchcode=cr.branchcode) OR ( ISNULL(cr2.branchcode) AND ISNULL(cr.branchcode) ) ) >+ AND ( (cr2.categorycode=cr.categorycode) OR ( ISNULL(cr2.categorycode) AND ISNULL(cr.categorycode) ) ) >+ AND ( (cr2.itemtype=cr.itemtype) OR ( ISNULL(cr2.itemtype) AND ISNULL(cr.itemtype) ) ) >+ ) >+ AND NOT EXISTS ( >+ SELECT * FROM circulation_rules cr2 >+ WHERE >+ cr2.rule_name=? >+ AND ( (cr2.branchcode=cr.branchcode) OR ( ISNULL(cr2.branchcode) AND ISNULL(cr.branchcode) ) ) >+ AND ( (cr2.categorycode=cr.categorycode) OR ( ISNULL(cr2.categorycode) AND ISNULL(cr.categorycode) ) ) >+ AND ( (cr2.itemtype=cr.itemtype) OR ( ISNULL(cr2.itemtype) AND ISNULL(cr.itemtype) ) ) >+ ) >+ GROUP BY branchcode, categorycode, itemtype >+ }, undef, $rule_name, $rule_value, $rule_name); >+ } >+ say $out "Add default values for blank circulation rules that weren't saved to the database"; >+ }, >+} >diff --git a/installer/onboarding.pl b/installer/onboarding.pl >index 7c15b74c5b..95e2b7b28e 100755 >--- a/installer/onboarding.pl >+++ b/installer/onboarding.pl >@@ -242,7 +242,7 @@ if ( $step == 5 ) { > my $categorycode = $input->param('categorycode'); > my $itemtype = $input->param('itemtype'); > my $maxissueqty = $input->param('maxissueqty'); >- my $issuelength = $input->param('issuelength'); >+ my $issuelength = $input->param('issuelength') || 0; > my $lengthunit = $input->param('lengthunit'); > my $renewalsallowed = $input->param('renewalsallowed'); > my $renewalperiod = $input->param('renewalperiod'); >@@ -252,7 +252,6 @@ if ( $step == 5 ) { > my $onshelfholds = $input->param('onshelfholds') || 0; > $maxissueqty =~ s/\s//g; > $maxissueqty = undef if $maxissueqty !~ /^\d+/; >- $issuelength = $issuelength eq q{} ? undef : $issuelength; > > my $params = { > branchcode => $branchcode, >@@ -286,8 +285,9 @@ if ( $step == 5 ) { > overduefinescap => "", > rentaldiscount => 0, > reservesallowed => $reservesallowed, >- suspension_chargeperiod => undef, >- decreaseloanholds => undef, >+ suspension_chargeperiod => 1, >+ decreaseloanholds => "", >+ unseen_renewals_allowed => "", > recalls_allowed => undef, > recalls_per_record => undef, > on_shelf_recalls => undef, >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 6727885768..5df48a90d7 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 >@@ -303,7 +303,13 @@ > <td>[% suspension_chargeperiod | html %]</td> > <td>[% renewalsallowed | html %]</td> > [% IF Koha.Preference('UnseenRenewals') %] >- <td>[% unseenrenewalsallowed | html %]</td> >+ <td> >+ [% IF unseenrenewalsallowed.defined && unseenrenewalsallowed != '' %] >+ [% unseenrenewalsallowed | html %] >+ [% ELSE %] >+ <span>Unlimited</span> >+ [% END %] >+ </td> > [% END %] > <td>[% renewalperiod | html %]</td> > <td>[% norenewalbefore | html %]</td> >-- >2.25.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 29012
:
128205
|
128206
|
131043
|
131053
|
131054
|
135194
|
135998
|
138140
|
138309
|
138310
|
141869
|
141876
|
141877
|
141878
|
141879
|
141951
|
141952
|
142248
|
142249