Bug 25440

Summary: Remove undef and CGI warnings and fix template variables list in circulation rules
Product: Koha Reporter: Andrii Nugged <nugged>
Component: CirculationAssignee: Andrii Nugged <nugged>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: aleisha, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02
Bug Depends on:    
Bug Blocks: 25790    
Attachments: Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl
Bug 25440: Extra duplicated call to CGI->param method removed
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl
Bug 25440: Fixed list of 'show_rule' forming variables in the template
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl
Bug 25440: Fixed list of 'show_rule' forming variables in the template
Bug 25440: Extra duplicated call to CGI->param method removed
Bug 25440: Fix for "CGI::param called in list context" in smart-rules.pl
Bug 25440: Fix for "uninitialized value in hash" warning in smart-rules.pl
Bug 25440: Fix for "uninitialized value in string eq" in smart-rules.pl
Bug 25440: Fix for "uninitialized $maxsuspensiondays" in smart-rules.pl
Bug 25440: Fixed list of 'show_rule' forming variables in the template
Bug 25440: (QA follow-up) Correction for typo

Description Andrii 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 Andrii Nugged 2020-05-10 16:08:11 UTC Comment hidden (obsolete)
Comment 2 Andrii 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 Andrii Nugged 2020-05-10 16:08:55 UTC Comment hidden (obsolete)
Comment 4 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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 Andrii 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.