Bug 6835 - Ability to specify types of email address in overdue_notices.pl
Summary: Ability to specify types of email address in overdue_notices.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Alex Arnaud
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-02 09:37 UTC by Alex Arnaud
Modified: 2014-05-26 21:05 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Ability to specify types of email address in overdue_notices.pl (5.30 KB, patch)
2011-09-02 09:44 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.63 KB, patch)
2011-11-04 15:49 UTC, claire.hernandez@biblibre.com
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (6.11 KB, patch)
2012-03-30 09:09 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (6.34 KB, patch)
2012-05-24 09:40 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Patch 6835 (6.39 KB, patch)
2012-06-09 12:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.17 KB, patch)
2012-06-10 10:28 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.12 KB, patch)
2012-09-07 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.12 KB, patch)
2012-09-19 10:02 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.22 KB, patch)
2012-09-19 10:08 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 6835 - Ability to specify types of email address in overdue_notices.pl (5.69 KB, patch)
2012-12-13 10:50 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Patch (5.78 KB, patch)
2013-01-20 18:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2011-09-02 09:37:56 UTC
This feature allows to specify an / some types of email address we want send to.

It adds a '--email' parameter in overdue_notices.pl

i.e "overdue_notices.pl --email B_email emailpro"

patch is coming soon...
Comment 1 Alex Arnaud 2011-09-02 09:44:18 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-10-24 11:38:17 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 3 Paul Poulain 2011-10-25 15:05:53 UTC
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)
Comment 4 claire.hernandez@biblibre.com 2011-11-04 15:49:16 UTC Comment hidden (obsolete)
Comment 5 claire.hernandez@biblibre.com 2011-11-04 15:51:41 UTC
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
Comment 6 Paul Poulain 2011-12-06 19:18:11 UTC
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
Comment 7 Marcel de Rooy 2012-01-04 15:34:21 UTC
Could you please resolve:
CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl

Thanks.
Comment 8 Matthias Meusburger 2012-03-30 09:09:24 UTC Comment hidden (obsolete)
Comment 9 Matthias Meusburger 2012-03-30 09:09:53 UTC
Conflict solved.
Comment 10 Marcel de Rooy 2012-05-10 07:02:32 UTC
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
Comment 11 Matthias Meusburger 2012-05-24 09:40:39 UTC Comment hidden (obsolete)
Comment 12 Matthias Meusburger 2012-05-24 09:47:24 UTC
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.
Comment 13 Paul Poulain 2012-05-28 14:24:33 UTC
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
Comment 14 Marcel de Rooy 2012-06-09 12:37:36 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2012-06-09 12:44:41 UTC
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.
Comment 16 Matthias Meusburger 2012-06-09 12:57:12 UTC
(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 ?
Comment 17 Marcel de Rooy 2012-06-09 14:26:28 UTC
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 ;)
Comment 18 Paul Poulain 2012-06-09 14:39:35 UTC
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
Comment 19 Matthias Meusburger 2012-06-10 10:28:36 UTC Comment hidden (obsolete)
Comment 20 Matthias Meusburger 2012-06-10 10:30:00 UTC
This new patch removes the GetValidEmailAddresses function (and uses GetMember instead).
Comment 21 Marcel de Rooy 2012-06-11 10:06:46 UTC
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).
Comment 22 Marcel de Rooy 2012-07-05 09:21:31 UTC
In the former comment of June 11 a question was raised. But it is still unanswered. Changing status to reflect need for clarification/adjustment..
Comment 23 Jonathan Druart 2012-09-07 12:19:19 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-09-07 12:19:37 UTC
Last patch is just a rebased patch
Comment 25 Matthias Meusburger 2012-09-19 10:02:10 UTC Comment hidden (obsolete)
Comment 26 Matthias Meusburger 2012-09-19 10:03:39 UTC
This new patch fixes the $nomail issue.
Comment 27 Matthias Meusburger 2012-09-19 10:08:24 UTC Comment hidden (obsolete)
Comment 28 Jared Camins-Esakov 2012-10-06 04:20:28 UTC
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.
Comment 29 Katrin Fischer 2012-12-02 21:22:13 UTC
Hi Marcel, do you have time to take another look at this?
Comment 30 Marcel de Rooy 2012-12-03 10:49:38 UTC
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.
Comment 31 Matthias Meusburger 2012-12-13 10:46:02 UTC
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 ) {
Comment 32 Matthias Meusburger 2012-12-13 10:50:04 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2013-01-20 18:22:19 UTC
Looking at this one..
Comment 34 Marcel de Rooy 2013-01-20 18:41:11 UTC
Created attachment 14727 [details] [review]
Patch
Comment 35 Marcel de Rooy 2013-01-20 18:41:49 UTC
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
Comment 36 Jared Camins-Esakov 2013-01-24 16:07:49 UTC
This patch has been pushed to master.