Running fines now always produces a log file with personal information about patrons in the tmp directory. This is independent from running it with or without the -v option. But only the -v option will tell you that a file has been generated: Fines assessment -- 2013-02-17T22:10:59 -- Saved to /tmp/koha_2013-02-17.log There shouldn't be files on your system with patron information you don't know about and having a separate script for deleting those files makes less sense then not generating them in the first place.
I agree, and I think it should be off by default. I have submitted a patch that has logging off by default and needs the -l switch on to log to a file.
Created attachment 15581 [details] [review] Bug 9656: Make logging to a file optional (for fines) To test: 1/ Before the patch run misc/cronjobs/fines.pl Notice that a file has been written to /tmp 2/ rm the file 3/ Apply the patch and run the script again Notice the file is not created 4/ run the script with -l Notice the file is created again
Test: 1) pre-patch, file created in tmp 2) post-patch, no args, file no created 3) post-patch, -l, errors: a) error message ~/kohaclone$ misc/cronjobs/fines.pl -l Can't use an undefined value as a symbol reference at misc/cronjobs/fines.pl line 141. b) file created, only column names cat /tmp/kohadev* cardnumber categorycode surname firstname email phone address citystate itemnumber barcode date_due type days_overdue fine
(In reply to comment #3) > Test: > 1) pre-patch, file created in tmp > > 2) post-patch, no args, file no created > > 3) post-patch, -l, errors: > a) error message > ~/kohaclone$ misc/cronjobs/fines.pl -l > Can't use an undefined value as a symbol reference at misc/cronjobs/fines.pl > line 141. > b) file created, only column names > cat /tmp/kohadev* > cardnumber categorycode surname firstname email phone > address citystate itemnumber barcode date_due type > days_overdue fine Fixed
Created attachment 15643 [details] [review] Bug 9656: Make logging to a file optional (for fines) To test: 1/ Before the patch run misc/cronjobs/fines.pl Notice that a file has been written to /tmp 2/ rm the file 3/ Apply the patch and run the script again Notice the file is not created 4/ run the script with -l Notice the file is created again
Created attachment 15644 [details] [review] [SIGNED-OFF] Bug 9656: Make logging to a file optional (for fines) To test: 1/ Before the patch run misc/cronjobs/fines.pl Notice that a file has been written to /tmp 2/ rm the file 3/ Apply the patch and run the script again Notice the file is not created 4/ run the script with -l Notice the file is created again Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Now it work as described. No errors.
QA comment: If the -o parameter is given, there is no logfile created, I think it is not consistent. To keep a consistency with current configuration, isn't it better just to produce a log file if the -o parameter exists?
I think we should not be consistent here. Libraries are most probably not aware of those files being created at all. So there might be quite a lot where all those files are still in temp, without anyone noticing or using them. In terms of data privacy this is quite horrible. Also, there is not really a need for those files apart from testing/debugging. All information is also available in Koha using action_log and fineslog system preference.
Hm, I think I might have misunderstood your comment... digging around a bit.
Oh ok - no opinion on the -o :)
Created attachment 15767 [details] [review] Bug 9656: Followup Make logging to a file optional (for fines) The -log option become optional if the -output_dir is given. Test plan: call the script with 1/ no one parameter : no log file 2/ -l : log file will be created in /tmp 3/ -o=/home/koha/var/log : log file with be created in the specified 4/ -o=/home/koha/var/log -l: Same as 3/
Created attachment 15814 [details] [review] [SIGNED-OFF] Bug 9656: Followup Make logging to a file optional (for fines) The -log option become optional if the -output_dir is given. Test plan: call the script with 1/ no one parameter : no log file 2/ -l : log file will be created in /tmp 3/ -o=/home/koha/var/log : log file with be created in the specified 4/ -o=/home/koha/var/log -l: Same as 3/ Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: All combination of options tested. Works well. No errors.
Created attachment 15860 [details] [review] Bug 9656: Make logging to a file optional (for fines) To test: 1/ Before the patch run misc/cronjobs/fines.pl Notice that a file has been written to /tmp 2/ rm the file 3/ Apply the patch and run the script again Notice the file is not created 4/ run the script with -l Notice the file is created again Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Now it work as described. No errors. Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
Created attachment 15861 [details] [review] Bug 9656: Followup Make logging to a file optional (for fines) The -log option become optional if the -output_dir is given. Test plan: call the script with 1/ no one parameter : no log file 2/ -l : log file will be created in /tmp 3/ -o=/home/koha/var/log : log file with be created in the specified 4/ -o=/home/koha/var/log -l: Same as 3/ Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: All combination of options tested. Works well. No errors. Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
This patch has been pushed to master. The output directory is not set according to an environment variable as claimed by the help, but the help was wrong prior to these patches so I am pushing them anyway.
Pushed to 3.10.x will be in 3.10.4