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.
Created attachment 26778 [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 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.
A default kohagit as misc/cronjobs/runreports.pl not tools/runreports.pl
This patch fixes the problem for package users, and makes it no worse for other users. I request reconsideration. Liz
Created attachment 26802 [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.
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.
(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.
(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.
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
Created attachment 26803 [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.
(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.
Created attachment 26847 [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 and also corrects some path issues created by Debian packaging/Standard installs vs git installs Note: This patch makes it so that email sending is supported by default for package and standard debian tarball installs. For everyone else, if you specify a configuration option pointing to where your cronjobs are kept, in the form of <supportdir>/path/to/koha/misc</supportdir> task scheduling will be working in your install. Example: My runreports.pl is in /home/liz/koha-src/koha/misc/cronjobs my <supportdir> would be as follows: <supportdir>/home/liz/koha-src/koha/misc</supportdir> To test: 1. make sure that your user is not in at.deny, and (if applicable/necessary) is in at.allow 1a. set up your <supportdir> if required for your test installation type. 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 (handy tip, if you schedule them a few days away you can see all of your tasks listed with the command used to invoke the job for comparison) 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. add (or remove) your <supportdir> directive from your koha-conf.xml, schedule another task, note how the command is changed yet again.
Should probably add that the config option belongs in koha-conf.xml in the <config> section.
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.
Don't bother I'll fix it.
Created attachment 26849 [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.
Created attachment 26850 [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 and also corrects some path issues created by Debian packaging/Standard installs vs git installs Note: This patch makes it so that email sending is supported by default for package and standard debian tarball installs. For everyone else, if you specify a configuration option pointing to where your cronjobs are kept, in the form of <supportdir>/path/to/koha/misc</supportdir> task scheduling will be working in your install. Example: My runreports.pl is in /home/liz/koha-src/koha/misc/cronjobs my <supportdir> would be as follows: <supportdir>/home/liz/koha-src/koha/misc</supportdir> To test: 1. make sure that your user is not in at.deny, and (if applicable/necessary) is in at.allow 1a. set up your <supportdir> if required for your test installation type. 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 (handy tip, if you schedule them a few days away you can see all of your tasks listed with the command used to invoke the job for comparison) 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. add (or remove) your <supportdir> directive from your koha-conf.xml, schedule another task, note how the command is changed yet again.
Created attachment 26851 [details] [review] [SIGNED OFF] 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.
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.
Shouldn't <supportdir>/path/to/koha/misc</supportdir> be added by the installer so it is automatically generated for future installations?
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";
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
(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. :)
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.
Created attachment 28783 [details] [review] Bug 12031 - Task Scheduler not sending mail
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
Created attachment 29593 [details] [review] QA followup 12031 - adding supportdir to koha-conf.xml
Should probably add that the last patch will only add the supportdir configuration option for new installs.
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.
(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
(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.
(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..
Created attachment 30452 [details] [review] [SIGNED OFF] 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>
Created attachment 30453 [details] [review] [SIGNED OFF] 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>
I think I'll leave my idea in comment #28 for another bug report. Here's the sign off, Marcel. :)
(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)
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>
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>
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.
Patches pushed to master. Thanks Liz!
no, thank YOU. :) Liz