Bug 6835

Updating Version : This ENH will be for Koha 3.8

Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)

Signed-off:
- little code review
- pertidy with no option on major part of the patch
- this patch is in production for one of BibLibre customer and has been validated

Claire, when you attach a signed-off patch with git bz, add the -e flag to obsolete the initial patch.
For example:
git bz attach -e 5281 HEAD

WARNING: you'll be presented your editor to choose which patch(es) to obsolete. Be very quick. You've less than 30 seconds. If you're not fast enough, you'll get a nasty python error bacause your connecion to bugzilla will have be closed

Could you please resolve:
CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl

Thanks.

Conflict solved.

QA Comments:
overdue_notices.pl line 50/51: in the pod comments you break the line between script name and pl extension
why do you set $email=1 on line 481 while you do not use it later anymore?
why repeat the warning on line 489 (see line 472) ?
redefinition of $letter: see lines 491/529; will raise warning
$itemcount on line 489 not defined; warning too
getletter on line 491 needs branchcode; warning too

Failed QA

This new patch fixes M. de Rooy QA comments, except for : 

"why do you set $email=1 on line 481 while you do not use it later anymore?"

$email is used line 515.

QA comments:
  * I don't see the necessity of GetValidEmailAddresses sub. GetMember will return all email addresses (with other informations)
  * passes perlcritic

Please answer my question #1 or provide a fix that does not create this new sub

Also note that, if this sub is needed, a test will be welcomed

QA Comment:
Do not want to step into your conversation ;)
I do not oppose this new sub, but Paul's question deserves an answer first.

(En réponse au commentaire 13)
> QA comments:
>   * I don't see the necessity of GetValidEmailAddresses sub. GetMember will
> return all email addresses (with other informations)

GetMember does a heavier query than GetValidEmailAddresses. To my opinion, that unique point make the creation of GetValidEmailAddresses relevant, at a time we're trying to optimize koha. 
Moreover, the fetching of email addresses is done in nested loops. In such case, having lightweight queries does seem important to me.
Do we agree on that ?

QA Comment:
All earlier points have been revisited and corrected.
Only discussion on routine left.
Your query is somewhat lighter, but we still speak about one record. No objection however to this new routine.
Let me put the status to Passed QA now and leave it to RM ;)

After discussing of this with Matthias (hackfest), I mark failed QA = the gain in terms of performance is tiny, and it would add a sub that isn't necessary. We're trying to gain speed, but also remove code or at least not intruduce useless one)

marking failed QA, sorry

This new patch removes the GetValidEmailAddresses function (and uses GetMember instead).

OK for changing existing routine.
Final question: The if- condition ( @emails && !$nomail ) is clear, but what about its else-part? If the nomail flag is set, I think you should not send a mail. So please check that condition too in the else part (make it a elsif).

In the former comment of June 11 a question was raised. But it is still unanswered. Changing status to reflect need for clarification/adjustment..

Last patch is just a rebased patch

This new patch fixes the $nomail issue.

Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.

Hi Marcel, do you have time to take another look at this?

Still two questions looking at the code again:

1) $email = C4::Members::GetFirstValidEmailAddress($borrowernumber);
                        push @emails_to_use, $email;
Shouldn't you test what you get back from this routine? Could be nothing.
Could you use a temp variable instead of $email? (see point 2)

2)  $email = 1 if (@emails_to_use);
Does this work later on? I would rather test later on @emails_to_use and not change a variable that is a record value used in the while loop (l.488).
See also line 535. And the template vars on 591/620.

Thanks.

1) 
push @emails_to_use, $email if ($email ne '');

I also added line 497/498:
push @emails_to_use, $memberinfos->{$_}
                              if ($memberinfos->{$_} ne '');


2) It does work, but you're right, it's messy:
-                        $email = 1 if (@emails_to_use);

-                    if ( ( !$email || $nomail ) && $PrintNoticesMaxLines && $j >= $PrintNoticesMaxLines ) {
+                    if ( ( scalar(@emails_to_use) == 0 || $nomail ) && $PrintNoticesMaxLines && $j >= $PrintNoticesMaxLines ) {

Looking at this one..

Created attachment 14727 [details] [review]
Patch

Tested the cronjob with various settings of email parameter.

QA Comment:
This patch unfortunately took some time to reach this point. But hopefully we do now. Just some minor comments. (Thanks for paying attention to various details along the way!)

The (double) use of $email in prepare_letter_for_printing is still little bit confusing. But could be taken care of in a followup. (In both cases there is no e-mail.) Could the email parameter be removed or just be empty in those two lines? 

Not an error, but just a thought:
If I run perl misc/cronjobs/overdue_notices.pl -email email,emailpro
I get:
Use of uninitialized value in string ne at misc/cronjobs/overdue_notices.pl line 491.
I know that I should do -email email -email emailpro and that works fine. But is it an idea to make it work too? Or should you better check the contents of this parameter and warn? (This may be somewhat hypothetical for a cronjob, I realize..) 

Passed QA

This patch has been pushed to master.