Bug 25440 - Remove undef and CGI warnings and fix template variables list in circulation rules
Summary: Remove undef and CGI warnings and fix template variables list in circulation ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Andrew Nugged
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2020-05-10 10:21 UTC by Andrew Nugged
Modified: 2020-07-16 05:01 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02


Attachments
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl (1.33 KB, patch)
2020-05-10 16:08 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Extra duplicated call to CGI->param method removed (1.68 KB, patch)
2020-05-10 16:08 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl (1.35 KB, patch)
2020-05-10 16:08 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl (5.11 KB, patch)
2020-05-10 16:09 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl (1.54 KB, patch)
2020-05-10 16:09 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fixed list of 'show_rule' forming variables in the template (2.92 KB, patch)
2020-05-10 16:09 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl (1.25 KB, patch)
2020-05-19 08:07 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl (1.50 KB, patch)
2020-05-19 08:43 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Fixed list of 'show_rule' forming variables in the template (2.99 KB, patch)
2020-06-17 20:31 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25440: Extra duplicated call to CGI->param method removed (1.74 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl (5.20 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl (1.60 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl (1.32 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl (1.56 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: Fixed list of 'show_rule' forming variables in the template (3.07 KB, patch)
2020-06-25 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25440: (QA follow-up) Correction for typo (2.28 KB, patch)
2020-06-29 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Nugged 2020-05-10 10:21:14 UTC
I am proposing a set of fixes, which I found when doing another mine complicated circulation rules task: I will post separate patches one-by-one with explanations here.
Comment 1 Andrew Nugged 2020-05-10 16:08:11 UTC Comment hidden (obsolete)
Comment 2 Andrew Nugged 2020-05-10 16:08:30 UTC
Created attachment 104636 [details] [review]
Bug 25440: Extra duplicated call to CGI->param method removed

In code CGI param 'no_auto_renewal_after_hard_limit' assigned to
"$no_auto_renewal_after_hard_limit" var, and then just in the next line
again variable "$no_auto_renewal_after_hard_limit" reassigned with
call to same "$input->param('no_auto_renewal_after_hard_limit')".

Fixed. No logic or results should be changed.
Comment 3 Andrew Nugged 2020-05-10 16:08:55 UTC Comment hidden (obsolete)
Comment 4 Andrew Nugged 2020-05-10 16:09:27 UTC
Created attachment 104638 [details] [review]
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl

This warning emitted:

CGI::param called in list context from /admin/smart-rules.pl line 262,
this can lead to vulnerabilities. See the warning in "Fetching the value
or values of a single named parameter" at CGI.pm line 412.

Explained here: https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter

And because all these params are not multi-params, so simple "scalar .."
forcing for CGI->param is the fix. Changes are transparent and same
values should be assigned as before, just no more warnings.
Comment 5 Andrew Nugged 2020-05-10 16:09:43 UTC
Created attachment 104639 [details] [review]
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl

This warning emitted:

Use of uninitialized value in hash element
at /admin/smart-rules.pl line 569.

that happened because we have NULLs in SQL results for 'categorycode'
and 'itemtype' which later used as 'any' kind of category/item in the
template, so for the template it passed this way:

    $rules->{ $r->{categorycode} }->{ $r->{itemtype} }->...

but undef will stringify as "" to become a hash key ("Hashes are
unordered collections of scalar values indexed by their associated
string key" https://perldoc.perl.org/perldata.html),

that's why "undef warning". To prevent warning here is the simple fix:

    $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->...
Comment 6 Andrew Nugged 2020-05-10 16:09:55 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2020-05-18 11:12:57 UTC
Comment on attachment 104637 [details] [review]
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl

Review of attachment 104637 [details] [review]:
-----------------------------------------------------------------

::: admin/smart-rules.pl
@@ +284,5 @@
>      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');
> +    $cap_fine_to_replacement_price = $cap_fine_to_replacement_price && $cap_fine_to_replacement_price eq 'on';

What about

cap_fine_to_replacement_price = ($cap_fine_to_replacement_price || '') eq 'on';
Comment 8 Jonathan Druart 2020-05-18 11:19:10 UTC
Comment on attachment 104635 [details] [review]
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl

Review of attachment 104635 [details] [review]:
-----------------------------------------------------------------

::: admin/smart-rules.pl
@@ -253,5 @@
>      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||;

Maybe a bit radical :)

This is coming from a bad merge resolution conflict, I think it should be:
my $maxsuspensiondays = $input->param('maxsuspensiondays') || q||;

Just a feeling, it should be tested.
Comment 9 Andrew Nugged 2020-05-19 07:04:50 UTC
(In reply to Jonathan Druart from comment #7)
> > +    my $cap_fine_to_replacement_price = $input->param('cap_fine_to_replacement_price');
> > +    $cap_fine_to_replacement_price = $cap_fine_to_replacement_price && $cap_fine_to_replacement_price eq 'on';
> 
> What about
> 
> cap_fine_to_replacement_price = ($cap_fine_to_replacement_price || '') eq
> 'on';

I just mimicked code style in this same block, but of course (and I also like that in Perl) we can undef-protect inline if you want. Should I recreate the patch?
Comment 10 Jonathan Druart 2020-05-19 07:43:10 UTC
Yes please :)
Comment 11 Andrew Nugged 2020-05-19 07:45:05 UTC
(In reply to Jonathan Druart from comment #8)
> > -    $maxsuspensiondays = undef if $maxsuspensiondays eq q||;
> > -    $maxsuspensiondays = '' if $maxsuspensiondays eq q||;
> 
> Maybe a bit radical :)
> 
> This is coming from a bad merge resolution conflict, I think it should be:
> my $maxsuspensiondays = $input->param('maxsuspensiondays') || q||;
> 
> Just a feeling, it should be tested.

In old db before circulation_rules table, it was "NULL-possible" value,
so I was confused too.

$input->param('maxsuspensiondays') here always comes as '' from posted form but if parameter absent so default might be

When I removed these both lines I found in code only one place where this key is used:

File: /opt/n/koha/git/KohaCommunity/C4/Circulation.pm
2326: my $max_sd = $issuing_rule->{maxsuspensiondays};
2327: if ( defined $max_sd && $max_sd ne '' ) {
2328:     $max_sd = DateTime::Duration->new( days => $max_sd );
2329:     $suspension_days = $max_sd
2330:       if DateTime::Duration->compare( $max_sd, $suspension_days ) < 0;
2331: }

and there both '' and undef is possible, so no matter which one we will have in db.

But because current circulation_rules not allow "NULLS" for key/val pairs but pair absence is assumed as null so it might be the consequences, that why we had few other fixes and slightly mess with default states for parameters after conversion from issuinrules to circulation_rules, but yes, I agree, let's stick with one value, let it be ''.

Recreating the patch now and will follow your proposal: and it works as on me.
Comment 12 Andrew Nugged 2020-05-19 08:07:28 UTC
Created attachment 105059 [details] [review]
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl

This warning emitted:

Use of uninitialized value in string eq at /admin/smart-rules.pl line 289.

It is solved by adding ".. || ''" to exclude comparison with 'on' string
when variable is 'undef'.
Comment 13 Andrew Nugged 2020-05-19 08:43:26 UTC
Created attachment 105063 [details] [review]
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl

This warning emitted:

Use of uninitialized value $maxsuspensiondays in string eq
at /admin/smart-rules.pl line 257.

But that not just undef-warning, there is broken logic,
these two lines are mutually contradictory and goes one-by-one:

    $maxsuspensiondays = undef if $maxsuspensiondays eq q||;
    $maxsuspensiondays = '' if $maxsuspensiondays eq q||;

Fix is simple: to make it '' if it comes undef.
Comment 14 Jonathan Druart 2020-05-19 09:02:24 UTC
This last patch makes sense to me.
Comment 15 Jonathan Druart 2020-06-17 12:22:07 UTC
(In reply to Andrew Nugged from comment #6)
> Created attachment 104640 [details] [review] [review]
> Bug 25440: Fixed list of 'show_rule' forming variables in the template

Can you rewrite this patch keeping the exact same order of the variable list before?
note, maxissueqty, maxonsiteissueqty, issuelength, etc.
Comment 16 Andrew Nugged 2020-06-17 13:50:53 UTC
Order in "SET show_rule = ..."? It is as before, but changes are:

1. "hardduedatebefore || hardduedateexact" obsolete and removed,
2. "renewalsallowed" duplicate removed,
3. "article_requests" duplicate removed,
4. and new missed list "note || hardduedatecompare || renewalperiod || rentaldiscount || " added on the beginning of the line,

... no order changed.

Can it be that was those p4 changes to the line start made you think that order changed?
Comment 17 Andrew Nugged 2020-06-17 13:52:10 UTC
(or you want this " || " set to be the exact match with lines 129-157 separate one-by-one sets list?)
Comment 18 Jonathan Druart 2020-06-17 14:33:02 UTC
You added:
  note || hardduedatecompare || renewalperiod || rentaldiscount
before maxissueqty.

I'd like the order of the variables in the "SET show_rule" statement to be the same as the order of the variables defined previously (l.129-157 yes)
Comment 19 Andrew Nugged 2020-06-17 20:31:15 UTC
Created attachment 105985 [details] [review]
Bug 25440: Fixed list of 'show_rule' forming variables in the template

In smart-rules.tt we have `SET show_rule = ...` section which filled
with 'all used in the loop' variables. Because if historical reasons it
seems that there are some missing, few old, and even doubled ones.
This list is fixed now by:

  - variable names 'article_requests' and 'renewalsallowed' repeated
    so duplicates are removed;

  - 'hardduedatebefore' and 'hardduedateexact' not present in the whole
    site code anywhere anymore;
    IMPORTANT NOTE: these 'hardduedatebefore/hardduedateexact' also
                    exists as remnants in .po-translation files, a lot.

  - 'note', 'hardduedatecompare', 'renewalperiod', 'rentaldiscount'
    template variables were missing from this 'show_rule =' checking
    code so they are added.

Order of fields updated to match with above "SET field = ..." pack.
Comment 20 Martin Renvoize 2020-06-25 14:41:24 UTC
Created attachment 106300 [details] [review]
Bug 25440: Extra duplicated call to CGI->param method removed

In code CGI param 'no_auto_renewal_after_hard_limit' assigned to
"$no_auto_renewal_after_hard_limit" var, and then just in the next line
again variable "$no_auto_renewal_after_hard_limit" reassigned with
call to same "$input->param('no_auto_renewal_after_hard_limit')".

Fixed. No logic or results should be changed.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2020-06-25 14:41:27 UTC
Created attachment 106301 [details] [review]
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl

This warning emitted:

CGI::param called in list context from /admin/smart-rules.pl line 262,
this can lead to vulnerabilities. See the warning in "Fetching the value
or values of a single named parameter" at CGI.pm line 412.

Explained here: https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter

And because all these params are not multi-params, so simple "scalar .."
forcing for CGI->param is the fix. Changes are transparent and same
values should be assigned as before, just no more warnings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2020-06-25 14:41:30 UTC
Created attachment 106302 [details] [review]
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl

This warning emitted:

Use of uninitialized value in hash element
at /admin/smart-rules.pl line 569.

that happened because we have NULLs in SQL results for 'categorycode'
and 'itemtype' which later used as 'any' kind of category/item in the
template, so for the template it passed this way:

    $rules->{ $r->{categorycode} }->{ $r->{itemtype} }->...

but undef will stringify as "" to become a hash key ("Hashes are
unordered collections of scalar values indexed by their associated
string key" https://perldoc.perl.org/perldata.html),

that's why "undef warning". To prevent warning here is the simple fix:

    $rules->{ $r->{categorycode} // '' }->{ $r->{itemtype} // '' }->...

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2020-06-25 14:41:33 UTC
Created attachment 106303 [details] [review]
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl

This warning emitted:

Use of uninitialized value in string eq at /admin/smart-rules.pl line 289.

It is solved by one line added to exclude comparison with 'on' string
when variable is 'undef'.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-06-25 14:41:36 UTC
Created attachment 106304 [details] [review]
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl

This warning emitted:

Use of uninitialized value $maxsuspensiondays in string eq
at /admin/smart-rules.pl line 257.

But that not just undef-warning, there is broken logic,
these two lines are mutually contradictory and goes one-by-one:

    $maxsuspensiondays = undef if $maxsuspensiondays eq q||;
    $maxsuspensiondays = '' if $maxsuspensiondays eq q||;

Fix is simple: to make it '' if it comes undef.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-06-25 14:41:39 UTC
Created attachment 106305 [details] [review]
Bug 25440: Fixed list of 'show_rule' forming variables in the template

In smart-rules.tt we have `SET show_rule = ...` section which filled
with 'all used in the loop' variables. Because if historical reasons it
seems that there are some missing, few old, and even doubled ones.
This list is fixed now by:

  - variable names 'article_requests' and 'renewalsallowed' repeated
    so duplicates are removed;

  - 'hardduedatebefore' and 'hardduedateexact' not present in the whole
    site code anywhere anymore;
    IMPORTANT NOTE: these 'hardduedatebefore/hardduedateexact' also
                    exists as remnants in .po-translation files, a lot.

  - 'note', 'hardduedatecompare', 'renewalperiod', 'rentaldiscount'
    template variables were missing from this 'show_rule =' checking
    code so they are added.

Order of fields updated to match with above "SET field = ..." pack.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2020-06-25 14:42:19 UTC
Trivial fixes, clear code.  All works as expected and qa scripts pass.

Going straight for a QA here :)
Comment 27 Martin Renvoize 2020-06-29 10:31:32 UTC
Created attachment 106388 [details] [review]
Bug 25440: (QA follow-up) Correction for typo

'engthunit -> lengthunit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Andrew Nugged 2020-06-29 10:32:31 UTC
<3
Comment 29 Jonathan Druart 2020-06-29 11:50:35 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 30 Lucas Gass 2020-07-13 17:10:42 UTC
backported to 20.05.x for 20.05.02
Comment 31 Aleisha Amohia 2020-07-16 05:01:46 UTC
Hi all, this does not apply cleanly on 19.11.x, please rebase if you'd like me to backport.