Bug 20537 - Warnings in overdue_notices.pl
Summary: Warnings in overdue_notices.pl
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: Hayley Mapley
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-06 11:22 UTC by Katrin Fischer
Modified: 2019-05-30 20:38 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 20537: Added check to remove warning from overdue_notices.pl (2.42 KB, patch)
2019-03-06 04:20 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 20537: Added checks to remove warning from overdue_notices.pl (2.30 KB, patch)
2019-05-06 02:00 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 20537: Added checks to remove warning from overdue_notices.pl (1.88 KB, patch)
2019-05-24 19:54 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20537: Added checks to remove warning from overdue_notices.pl (1.97 KB, patch)
2019-05-24 20:22 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2018-04-06 11:22:05 UTC
We've seen a warning from overdue_notices.pl:

Use of uninitialized value in concatenation (.) or string at /usr/share/koha/bin/cronjobs/overdue_notices.pl line 575.


I checked in our version and in master and think it happens for users withut a surname (organisations usually):

 576                 $borrowernumber = $data->{'borrowernumber'};
 577                 my $borr =
 578                     $data->{'firstname'} . ', '
 579                   . $data->{'surname'} . ' ('
 580                   . $borrowernumber . ')';
 581                 $verbose

Should we check here if firstname/surname are defined?
Comment 1 Jonathan Druart 2018-04-10 20:52:12 UTC
(In reply to Katrin Fischer from comment #0)
> Should we check here if firstname/surname are defined?

yes
Comment 2 Hayley Mapley 2019-03-06 04:20:12 UTC
Created attachment 86124 [details] [review]
Bug 20537: Added check to remove warning from overdue_notices.pl

When executing overdue_notices.pl on borrowers that lack a surname, we
see this error:
Use of uninitialized value in concatenation (.) or string at
/usr/share/koha/bin/cronjobs/overdue_notices.pl line 575.

This patch fixes this issue by setting the $borr variable
based on the information that has been defined.

To test:
1) Create a borrower and set its surname to null
2) Checkout an item to the borrower and set it to be overdue
3) Navigate to kohaclone/misc/cronjobs and enter a koha-shell
4) Run the script: ./overdue_notices.pl
5) Observe the error appears
6) Apply the patch
7) Repeat steps 3-4
8) Observe the error is gone
9) Sign off!
Comment 3 Jonathan Druart 2019-04-29 15:29:47 UTC
Hi Hayley,

It will work, but I am wondering if something like that would not be more readable:

my $borr = sprintf( "%s%s%s (%s)",
    $data->{surname} || '',
    $data->{firstname} && $data->{surname} ? ', ' : '',
    $data->{firstname} || '',
    $borrowernumber );

What do you think?
Comment 4 Hayley Mapley 2019-04-30 03:33:33 UTC
(In reply to Jonathan Druart from comment #3)
> Hi Hayley,
> 
> It will work, but I am wondering if something like that would not be more
> readable:
> 
> my $borr = sprintf( "%s%s%s (%s)",
>     $data->{surname} || '',
>     $data->{firstname} && $data->{surname} ? ', ' : '',
>     $data->{firstname} || '',
>     $borrowernumber );
> 
> What do you think?

Hi Jonathan,

I'm happy to implement your solution, however it looks like maybe this error is not present on master anymore. Would you mind confirming this?
Comment 5 Jonathan Druart 2019-05-04 13:52:37 UTC
The code is the same in master so I bet the warning is still there.
Comment 6 Hayley Mapley 2019-05-06 02:00:38 UTC
Created attachment 89368 [details] [review]
Bug 20537: Added checks to remove warning from overdue_notices.pl

When executing overdue_notices.pl on borrowers that lack a surname, we
see this error:
Use of uninitialized value in concatenation (.) or string at
/usr/share/koha/bin/cronjobs/overdue_notices.pl line 575.

This patch fixes this issue by setting the $borr variable
based on the information that has been defined.

To test:
1) Create a borrower and set its surname to null
2) Checkout an item to the borrower and set it to be overdue
3) Navigate to kohaclone/misc/cronjobs and enter a koha-shell
4) Run the script: ./overdue_notices.pl
5) Observe the error appears
6) Apply the patch
7) Repeat steps 3-4
8) Observe the error is gone
9) Sign off!

Sponsored-by: Catalyst IT
Comment 7 Liz Rea 2019-05-24 19:31:43 UTC
Hayley is right, the behaviour doesn't seem to appear in current master, even though that code is the same. 

I think we can call this one fixed?

Cheers,
Liz
Comment 8 Liz Rea 2019-05-24 19:35:47 UTC
Actually, I was wrong and we do see this - I didn't have a rule defined for overdue notices for the borrower I was looking at. Bum.

So, having another look. :)

Liz
Comment 9 Liz Rea 2019-05-24 19:54:20 UTC
Created attachment 90085 [details] [review]
Bug 20537: Added checks to remove warning from overdue_notices.pl

When executing overdue_notices.pl on borrowers that lack a surname, we
see this error:
Use of uninitialized value in concatenation (.) or string at
/usr/share/koha/bin/cronjobs/overdue_notices.pl line 575.

This patch fixes this issue by setting the $borr variable
based on the information that has been defined.

To test:
1) Create a borrower and set its surname to null
2) Checkout an item to the borrower and set it to be overdue
3) Navigate to kohaclone/misc/cronjobs and enter a koha-shell
4) Run the script: ./overdue_notices.pl
5) Observe the error appears
6) Apply the patch
7) Repeat steps 3-4
8) Observe the error is gone
9) Sign off!

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
patch wouldn't apply on current master, so I fixed that. Work is the same.
Comment 10 Jonathan Druart 2019-05-24 20:22:38 UTC
Created attachment 90086 [details] [review]
Bug 20537: Added checks to remove warning from overdue_notices.pl

When executing overdue_notices.pl on borrowers that lack a surname, we
see this error:
Use of uninitialized value in concatenation (.) or string at
/usr/share/koha/bin/cronjobs/overdue_notices.pl line 575.

This patch fixes this issue by setting the $borr variable
based on the information that has been defined.

To test:
1) Create a borrower and set its surname to null
2) Checkout an item to the borrower and set it to be overdue
3) Navigate to kohaclone/misc/cronjobs and enter a koha-shell
4) Run the script: ./overdue_notices.pl
5) Observe the error appears
6) Apply the patch
7) Repeat steps 3-4
8) Observe the error is gone
9) Sign off!

Sponsored-by: Catalyst IT

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Nick Clemens 2019-05-30 10:32:42 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2019-05-30 20:38:48 UTC
Pushed to 18.11.x for 18.11.06