Bug 12031 - Task Scheduler not sending mail
Summary: Task Scheduler not sending mail
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Task Scheduler (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Liz Rea
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-03 01:55 UTC by Liz Rea
Modified: 2015-06-04 23:33 UTC (History)
11 users (show)

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


Attachments
Bug 12031 - Task Scheduler not sending mail (1.86 KB, patch)
2014-04-03 02:03 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12031 - Task Scheduler not sending mail (1.97 KB, patch)
2014-04-03 21:16 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12031 - Task Scheduler not sending mail (1.98 KB, patch)
2014-04-03 23:00 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12031 - Task scheduler not sending mail (3.10 KB, patch)
2014-04-06 22:01 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12031 - Task Scheduler not sending mail (2.36 KB, patch)
2014-04-07 01:49 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12031 - Task scheduler not sending mail (3.18 KB, patch)
2014-04-07 01:51 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12031 - Task Scheduler not sending mail (2.83 KB, patch)
2014-04-07 02:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12031 - Task Scheduler not sending mail (2.94 KB, patch)
2014-06-11 13:54 UTC, Paola Rossi
Details | Diff | Splinter Review
QA followup 12031 - adding supportdir to koha-conf.xml (1.12 KB, patch)
2014-07-10 00:09 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12031 - Task Scheduler not sending mail (2.93 KB, patch)
2014-08-01 11:44 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] QA followup 12031 - adding supportdir to koha-conf.xml (1.50 KB, patch)
2014-08-01 11:44 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12031 - Task Scheduler not sending mail (3.01 KB, patch)
2014-08-04 12:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
QA followup 12031 - adding supportdir to koha-conf.xml (1.59 KB, patch)
2014-08-04 12:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12031: [QA Follow-up] Undefined routine and change to koha-conf.xml (5.10 KB, patch)
2014-08-04 12:13 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 Liz Rea 2014-04-03 01:55:55 UTC
The task scheduler to schedule reports from the UI is not passing the correct command to runreport.pl, so it doesn't send email correctly. 

To replicate:

1. make sure that your user is not in at.deny, and (if applicable/necessary) is in at.allow

Useful, but not required step: make sure email can be sent from your server, also helpful is access to the Koha user's mail box (to see the bounce messages)

2. create a saved sql report to run

3. Go to Tools -> Task Scheduler and schedule a job to be run, and results emailed to you

4. note that you do not get an email, also note that the Koha user has received a bounce message with the report contents and an error message: 

[Thu Apr  3 14:02:00 2014] runreport.pl: ERROR: No saved report report-recipient@the.email.address.you.specified found at /usr/share/koha/intranet/cgi-bin/tools/runreport.pl line 193.
Comment 1 Liz Rea 2014-04-03 02:03:45 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-04-03 04:31:08 UTC
A default kohagit as misc/cronjobs/runreports.pl not tools/runreports.pl
Comment 3 Liz Rea 2014-04-03 21:06:26 UTC
This patch fixes the problem for package users, and makes it no worse for other users. 

I request reconsideration.

Liz
Comment 4 Liz Rea 2014-04-03 21:16:57 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-04-03 21:18:49 UTC
I think as this patch doesn't actually change the path, but fix the options, we could have this is as a first step to more improvements.
Comment 6 Galen Charlton 2014-04-03 21:19:23 UTC
(In reply to Liz Rea from comment #3)
> This patch fixes the problem for package users, and makes it no worse for
> other users. 
> 
> I request reconsideration.

Done.

Mark, your comment 2 is too brief -- brief enough to not be comprehensible, frankly.  As an incremental improvement is acceptable until such time that somebody steps up to provide a better scheduler, I've removed the dependency on bug 1993 as well.
Comment 7 David Cook 2014-04-03 22:34:08 UTC
(In reply to Liz Rea from comment #3)
> This patch fixes the problem for package users, and makes it no worse for
> other users. 
> 
> I request reconsideration.
> 
> Liz

I'm confused about how it would fix the problem for package users :(.

When I check my package install, there is no "tools/runreports.pl" either, so I'm not sure how it could work.

Does Catalyst add in the script for its packages or maybe adds a symlink? 

If this patch fixes the problem for package users, but makes it no worse for other users...what happens when a patch fixes the problem for non-package users but breaks it for package users?

I'm not opposed to this patch, but I am a bit...bewildered.
Comment 8 Liz Rea 2014-04-03 22:42:20 UTC
Now that is actually legitimate - I hadn't noticed that there is in fact a symlink on that system, and it does actually need to be changed for this to work - so I'll fix that too.

Cheers,
Liz
Comment 9 Liz Rea 2014-04-03 23:00:57 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-04-03 23:48:52 UTC
(In reply to Galen Charlton from comment #6)
> Mark, your comment 2 is too brief -- brief enough to not be comprehensible,
> frankly.

Oops...

$ cd /usr/share/koha
$ find . | grep runreport
./bin/cronjobs/runreport.pl
./man/man3/misc::cronjobs::runreport.3pm

$ cd ~/kohaclone
$ find . | grep runreport
./misc/cronjobs/runreport.pl

The idea I was getting at was captured in the third patch which is currently obsoleted. Though, perhaps FindBin logic should be added?


> As an incremental improvement is acceptable until such time that
> somebody steps up to provide a better scheduler, I've removed the dependency
> on bug 1993 as well.

Did I put it as a depends? I meant to put it as a See Also, because bug 1993 is related.
Comment 11 Liz Rea 2014-04-06 22:01:04 UTC Comment hidden (obsolete)
Comment 12 Liz Rea 2014-04-06 22:06:29 UTC
Should probably add that the config option belongs in koha-conf.xml in the <config> section.
Comment 13 Mark Tompsett 2014-04-07 01:42:36 UTC
Comment on attachment 26847 [details] [review]
Bug 12031 - Task scheduler not sending mail

Review of attachment 26847 [details] [review]:
-----------------------------------------------------------------

::: tools/scheduler.pl
@@ +35,5 @@
>  
>  my $input = new CGI;
> +my $base;
> +
> +if ( C4::Context::->config('supportdir') ) {

::-> ?! Nope. Should be just ->. I'm seeing if I can generate a patch with your name on it that fixes this. I have a sha1 hash issue.
Comment 14 Liz Rea 2014-04-07 01:46:33 UTC
Don't bother I'll fix it.
Comment 15 Liz Rea 2014-04-07 01:49:33 UTC Comment hidden (obsolete)
Comment 16 Liz Rea 2014-04-07 01:51:40 UTC Comment hidden (obsolete)
Comment 17 Mark Tompsett 2014-04-07 02:00:53 UTC Comment hidden (obsolete)
Comment 18 Liz Rea 2014-04-07 02:08:43 UTC
The commit message is a bit wrong on the signed off version - that's my fault I noticed it but Mark had already gotten to it before I uploaded a corrected commit message.

Anyway, attachment 26850 [details] [review] has the correct commit message if anyone really feels it bears getting a new signoff. The content of the patch is the same.
Comment 19 Kyle M Hall 2014-04-11 11:42:12 UTC
Shouldn't <supportdir>/path/to/koha/misc</supportdir> be added by the installer so it is automatically generated for future installations?
Comment 20 Mark Tompsett 2014-04-11 17:16:45 UTC
Comment on attachment 26851 [details] [review]
[SIGNED OFF] Bug 12031 - Task Scheduler not sending mail

Review of attachment 26851 [details] [review]:
-----------------------------------------------------------------

::: tools/scheduler.pl
@@ +40,5 @@
> +     $base = C4::Context->config('supportdir');
> +}
> +else {
> +     $base        = "/usr/share/koha/bin";
> +}

This block could be simplified:
my $base = C4::Context->config('supportdir') // "/usr/share/koha/bin";
Comment 21 Liz Rea 2014-04-13 21:25:11 UTC
Kyle,

No, I don't think so - it should only be added for git installations and probably then by hand.

If someone wants to submit a follow up for that they are welcome to.

Mark, I think that is hard to read and that's not the way I would write it. You are welcome to submit a follow up that changes it.

Cheers,
Liz
Comment 22 Mark Tompsett 2014-04-14 00:46:23 UTC
(In reply to Liz Rea from comment #21)
> No, I don't think so - it should only be added for git installations and
> probably then by hand.

The intent is for correcting package installations, and the default value is correct for that.


> If someone wants to submit a follow up for that they are welcome to.

Though, I still think there may be merit to having something added, at least somewhere, so a person using a git install knows what to tweak. That's the thing I really dislike about LDAP configuration. It's not in the koha-conf.xml file by default. A commented out line would be nice.


> Mark, I think that is hard to read and that's not the way I would write it.
> You are welcome to submit a follow up that changes it.

Granted, your code is easier to read for people unfamiliar with the // operator -- as I was a few months ago. :)
Comment 23 Paola Rossi 2014-06-11 13:52:45 UTC
I apply against master 3.17.00.007

For antispam, it's useful set pref KohaAdminEmailAddress from root@localhost to the user's email address.

In debian I need to remove the line 
www-data 
in file /etc/at.deny .

In my installation, because I don't use /usr/share/koha/bin but I use [my path], I add the line:
 <supportdir>[my path]/misc</supportdir>
 
in the file [my path]/etc/koha-conf.xml .

So everything's OK.

I pass the patch to "Signed Off" status.
Comment 24 Paola Rossi 2014-06-11 13:54:35 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-07-09 14:24:52 UTC
QA Comment:
Supportdir does not exist in the standard koha-conf.xml. Referring to it in the codebase does not look good.
I also see the hardcoded /usr/share/koha/bin. This is a package related fallback, but will not work in other installs.

Failed QA
Comment 26 Liz Rea 2014-07-10 00:09:54 UTC Comment hidden (obsolete)
Comment 27 Liz Rea 2014-07-10 02:55:38 UTC
Should probably add that the last patch will only add the supportdir configuration option for new installs.
Comment 28 Mark Tompsett 2014-07-10 03:39:44 UTC
Actually, if you look at
...net-tmpl/prog/en/modules/tools/scheduler.tt
you will see:
[% IF ( job_add_failed ) %]
<div class="dialog message">Failed to add scheduled task</div>
[% END %]

I think something could be added into tools/scheduler that displays a configuration required type message when the defaulting $base directory doesn't exist. This would then cause people to look at the koha-conf.xml which now has that useful message in it based on your latest patch.

Leaving status untouched, because I'm not sure if Marcel would consider the new patch alone sufficient, or if implementing this idea I just gave would be required too.
Comment 29 Chris Cormack 2014-08-01 00:19:41 UTC
(In reply to M. Tompsett from comment #28)
> Actually, if you look at
> ...net-tmpl/prog/en/modules/tools/scheduler.tt
> you will see:
> [% IF ( job_add_failed ) %]
> <div class="dialog message">Failed to add scheduled task</div>
> [% END %]
> 
> I think something could be added into tools/scheduler that displays a
> configuration required type message when the defaulting $base directory
> doesn't exist. This would then cause people to look at the koha-conf.xml
> which now has that useful message in it based on your latest patch.
> 
> Leaving status untouched, because I'm not sure if Marcel would consider the
> new patch alone sufficient, or if implementing this idea I just gave would
> be required too.

If you come up with ideas like that, you really should send a patch too, or you scope creep the bug to a point it never gets signed off IMHO
Comment 30 Mark Tompsett 2014-08-01 01:53:13 UTC
(In reply to Chris Cormack from comment #29)
[SNIP]
> If you come up with ideas like that, you really should send a patch too, or
> you scope creep the bug to a point it never gets signed off IMHO

To be honest, I had hoped Marcel, who Failed QA the bug, would have followed up with a comment about it being sufficient or insufficient. If the later, I would have provided a patch then. I suppose I shall in the next day or two.
Comment 31 Marcel de Rooy 2014-08-01 08:47:25 UTC
(In reply to M. Tompsett from comment #30)
> To be honest, I had hoped Marcel, who Failed QA the bug, would have followed
> up with a comment about it being sufficient or insufficient. If the later, I
> would have provided a patch then. I suppose I shall in the next day or two.

Oops. I just waited for a signoff before re-QAing it..
Comment 32 Mark Tompsett 2014-08-01 11:44:00 UTC Comment hidden (obsolete)
Comment 33 Mark Tompsett 2014-08-01 11:44:15 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2014-08-01 11:45:45 UTC
I think I'll leave my idea in comment #28 for another bug report. Here's the sign off, Marcel. :)
Comment 35 Marcel de Rooy 2014-08-04 12:09:37 UTC
(In reply to M. Tompsett from comment #34)
> I think I'll leave my idea in comment #28 for another bug report. Here's the
> sign off, Marcel. :)

OK. I will pass QA on it (adding a follow-up)
Comment 36 Marcel de Rooy 2014-08-04 12:13:39 UTC
Created attachment 30527 [details] [review]
Bug 12031 - Task Scheduler not sending mail

At some stage the runreport.pl cronjob was changed to accept longopts, but the scheduler script wasn't changed along with it. This patch fixes the mismatch.

To test:

1. make sure that your user is not in at.deny, and (if applicable/necessary) is in at.allow

1.5 test this on a package install, sending mails will still not work for git installs, even with this patch.

Useful, but not required step: make sure email can be sent from your server, also helpful is access to the Koha user's mail box (to see the bounce messages)

2. create a saved sql report to run

3. Go to Tools -> Task Scheduler and schedule a job to be run, and results emailed to you

4. note that you do not (without the patch) get an email. Make a note of the command it reports it is going to run.

5. apply the patch

6. schedule another job, you will notice that the command to run the scheduled job is slightly changed compared to the one in step 4.

7. Receiving an email when your job runs means the patch worked. Seeing --format= and --to= in the command also means the patch worked, as this is the command that runreport.pl is expecting.

8. Run the koha QA test tool.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

NOTE: The line that changes is visible in the bottom section,
      "Jobs already entered", with the corrected parameters and
      path used. If you do not have the <supportdir> config
      parameter set up in koha-conf.xml, it defaults to the
      typical package path. If you have the parameter set up,
      the path given matches what you tell it. Only tested in
      git.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2014-08-04 12:13:44 UTC
Created attachment 30528 [details] [review]
QA followup 12031 - adding supportdir to koha-conf.xml

By adding a supportdir, this allows for configuring use in a
non-package install environment, such as git.

Seeing as I only tested git, I clearly had this defined.
Further testing should include packaging up an installation, and
installing a package version without setting the supportdir
configuration value.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2014-08-04 12:13:48 UTC
Created attachment 30529 [details] [review]
Bug 12031: [QA Follow-up] Undefined routine and change to koha-conf.xml

[1] Routine add_cron_job was added in 2007 but has not been defined.
    Parameter Recurring is not used.
[2] Made some changes to koha-conf.xml. Instead of an example to edit,
    I replaced it by the SCRIPT_NONDEV_DIR install variable.
[3] SCRIPT_NONDEV_DIR had to be included in rewrite-config.pl and the
    path had to be corrected for dev installs in Makefile.PL

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested single and dev install for supportdir change.
Compared installations with and without the patches for this report.
Comment 39 Tomás Cohen Arazi 2014-10-27 14:17:39 UTC
Patches pushed to master.

Thanks Liz!
Comment 40 Liz Rea 2014-10-27 20:08:33 UTC
no, thank YOU. :)

Liz