Bug 9656 - fines.pl creating a log file should be optional
Summary: fines.pl creating a log file should be optional
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Chris Cormack
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-17 21:11 UTC by Katrin Fischer
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
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) (3.20 KB, patch)
2013-02-21 09:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9656: Make logging to a file optional (for fines) (3.20 KB, patch)
2013-02-23 06:03 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9656: Make logging to a file optional (for fines) (3.32 KB, patch)
2013-02-23 10:34 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9656: Followup Make logging to a file optional (for fines) (2.18 KB, patch)
2013-02-28 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9656: Followup Make logging to a file optional (for fines) (2.32 KB, patch)
2013-03-01 21:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9656: Make logging to a file optional (for fines) (3.36 KB, patch)
2013-03-04 19:04 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 9656: Followup Make logging to a file optional (for fines) (2.36 KB, patch)
2013-03-04 19:04 UTC, Elliott Davis
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-02-17 21:11:34 UTC
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.
Comment 1 Chris Cormack 2013-02-21 09:14:59 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.
Comment 2 Chris Cormack 2013-02-21 09:18:43 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2013-02-23 03:01:07 UTC
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
Comment 4 Chris Cormack 2013-02-23 06:03:15 UTC
(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
Comment 5 Chris Cormack 2013-02-23 06:03:44 UTC Comment hidden (obsolete)
Comment 6 Bernardo Gonzalez Kriegel 2013-02-23 10:34:25 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-02-28 10:13:15 UTC
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?
Comment 8 Katrin Fischer 2013-02-28 10:23:20 UTC
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.
Comment 9 Katrin Fischer 2013-02-28 10:24:15 UTC
Hm, I think I might have misunderstood your comment... digging around a bit.
Comment 10 Katrin Fischer 2013-02-28 10:36:10 UTC
Oh ok - no opinion on the -o :)
Comment 11 Jonathan Druart 2013-02-28 13:12:23 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-03-01 21:03:02 UTC Comment hidden (obsolete)
Comment 13 Elliott Davis 2013-03-04 19:04:46 UTC
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>
Comment 14 Elliott Davis 2013-03-04 19:04:58 UTC
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>
Comment 15 Jared Camins-Esakov 2013-03-07 14:27:43 UTC
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.
Comment 16 Chris Cormack 2013-03-11 09:32:54 UTC
Pushed to 3.10.x will be in 3.10.4