Bug 25533 - Perl "not numeric" warning on the "Holds" page
Summary: Perl "not numeric" warning on the "Holds" page
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Slava Shishkin
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2020-05-18 16:11 UTC by Peter Vashchuk
Modified: 2022-05-17 11:57 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25533: Fix for 'Argument "" isn't numeric' warning (1.57 KB, patch)
2020-05-19 14:23 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25533: Fix for 'Argument "" isn't numeric' warning (1.62 KB, patch)
2020-06-18 05:51 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2020-05-18 16:11:16 UTC
Perl warning on the intranet "Holds" page (/cgi-bin/koha/reserve/request.pl):

Argument "" isn't numeric in numeric gt (>)
at .../C4/Reserves.pm line 2233.
Comment 1 Slava Shishkin 2020-05-19 14:23:56 UTC
Created attachment 105101 [details] [review]
Bug 25533: Fix for 'Argument "" isn't numeric' warning

Perl warning on the intranet "Holds" page
(/cgi-bin/koha/reserve/request.pl)

Argument "" isn't numeric in numeric gt (>)
at .../C4/Reserves.pm line 2233.

Fixed by assigning 0 for $holds_per_record when it has value that can
be considered as false.

To test:
    1) Ensure that values of "Holds per record" is set to Unlimited
        on /cgi-bin/koha/admin/smart-rules.pl
    2) Go to the intranet page (/cgi-bin/koha/reserve/request.pl).
    3) Open item record, like /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1
    4) Observe the warning in the log file.
    5) Apply patch.
    6) Repeat steps 2 and 3.
    7) Check that previous warning was suppressed.

Mentored-by: Peter Vashchuk <stalkernoid@gmail.com>
Comment 2 Emmi Takkinen 2020-06-18 05:51:37 UTC
Created attachment 105995 [details] [review]
Bug 25533: Fix for 'Argument "" isn't numeric' warning

Perl warning on the intranet "Holds" page
(/cgi-bin/koha/reserve/request.pl)

Argument "" isn't numeric in numeric gt (>)
at .../C4/Reserves.pm line 2233.

Fixed by assigning 0 for $holds_per_record when it has value that can
be considered as false.

To test:
    1) Ensure that values of "Holds per record" is set to Unlimited
        on /cgi-bin/koha/admin/smart-rules.pl
    2) Go to the intranet page (/cgi-bin/koha/reserve/request.pl).
    3) Open item record, like /cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1
    4) Observe the warning in the log file.
    5) Apply patch.
    6) Repeat steps 2 and 3.
    7) Check that previous warning was suppressed.

Mentored-by: Peter Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 3 Marcel de Rooy 2020-06-19 09:10:49 UTC
Thanks for your patch !
You remove the warning, but are you interpreting the value correctly?

-        my $holds_per_record = $rule ? $rule->{holds_per_record} : 0;
+        my $holds_per_record = $rule ? $rule->{holds_per_record} || 0 : 0;
         $max = $holds_per_record if $holds_per_record > $max;

If space or empty string means Unlimited and we replace it by 0, would our compares be correct ?
Note that perl now does the same thing but removing the warn will only hide the problem you found in this code !
I see that code is calculating the number of remaining holds:

 my $remaining_holds_for_record = $max_holds_for_record - $holds->count();

If max is 0 but should mean Unlimited, this definitely goes wrong ?
Comment 4 Martin Renvoize 2020-06-25 14:48:21 UTC
(In reply to Marcel de Rooy from comment #3)
> Thanks for your patch !
> You remove the warning, but are you interpreting the value correctly?
> 
> -        my $holds_per_record = $rule ? $rule->{holds_per_record} : 0;
> +        my $holds_per_record = $rule ? $rule->{holds_per_record} || 0 : 0;
>          $max = $holds_per_record if $holds_per_record > $max;
> 
> If space or empty string means Unlimited and we replace it by 0, would our
> compares be correct ?
> Note that perl now does the same thing but removing the warn will only hide
> the problem you found in this code !
> I see that code is calculating the number of remaining holds:
> 
>  my $remaining_holds_for_record = $max_holds_for_record - $holds->count();
> 
> If max is 0 but should mean Unlimited, this definitely goes wrong ?

Good catch
Comment 5 Andrew Nugged 2020-06-25 14:59:58 UTC
Yes, we will re-investigate and analyze/argue or re-create the fix, few days please :).