Summary: | fines.pl creating a log file should be optional | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Command-line Utilities | Assignee: | Chris Cormack <chris> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | bgkriegel, chris, elliott, jonathan.druart |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: |
Bug 9656: Make logging to a file optional (for fines)
Bug 9656: Make logging to a file optional (for fines) [SIGNED-OFF] Bug 9656: Make logging to a file optional (for fines) Bug 9656: Followup Make logging to a file optional (for fines) [SIGNED-OFF] Bug 9656: Followup Make logging to a file optional (for fines) Bug 9656: Make logging to a file optional (for fines) Bug 9656: Followup Make logging to a file optional (for fines) |
Description
Katrin Fischer
2013-02-17 21:11:34 UTC
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 |