Bug 10810

Summary: Fix synopsis for -html option to overdue_notices.pl
Product: Koha Reporter: Magnus Enger <magnus>
Component: Command-line UtilitiesAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: bgkriegel, gmcharlt, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: String patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 10810 - Fix synopsis for -html option to overdue_notices.pl
[SIGNED-OFF] Bug 10810 - Fix synopsis for -html option to overdue_notices.pl
[PASSED QA] Bug 10810 - Fix synopsis for -html option to overdue_notices.pl

Description Magnus Enger 2013-08-31 13:24:11 UTC
See http://perldoc.koha-community.org/misc/cronjobs/overdue_notices.html

The synopsis says:

  -html         <filename>       Output html to file

But the description of the -html option says:

  -html

    Produces html data. if patron does not have a mail address or 
    -n (no mail) flag is set, an html file is generated in the specified 
    directory. This can be downloaded or futher processed by library staff.

If the -html option is given a filename as its argument it will fail without warning. 

Confusingly enough, the variable associated with the -html option is called $htmlfilename:

  'html:s' => \$htmlfilename,

But it is actually used as a path, and the filename is added to it:

  open $html_fh, ">",File::Spec->catdir ($htmlfilename,"notices-".$today->ymd().".html");

I'll do a patch that fixes the synopsis and makes the full description clearer, but leave the variable name alone, as changing it will not gain us much.
Comment 1 Magnus Enger 2013-08-31 13:37:49 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2013-09-08 01:28:19 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-09-10 21:07:32 UTC
Created attachment 20976 [details] [review]
[PASSED QA] Bug 10810 - Fix synopsis for -html option to overdue_notices.pl

The synopsis for the -html option used to be:
  -html <filename> Output html to file
but the argument to this option should actually be a directory,
not a filename. This patch fixes the synopsis.

To test:
- Run perldoc misc/cronjobs/overdue_notices.pl
- Check that the synopsis is as shown above
- Apply this patch
- Run perldoc misc/cronjobs/overdue_notices.pl
- Check that
  - the synopsis asks for a directory, not a filename
  - that the description of the -html option further down in the
    perldoc now mentions the filename that the HTML file will be
    created with.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Works as described. No koha-qa errors.

If feeded with a filename -html option fails with many messages like
print() on closed filehandle $html_fh at misc/cronjobs/overdue_notices.pl line 384.
Perhaps it could be improved, but that was the old behavior.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Improves documentation, no negative side effects found.
Comment 4 Galen Charlton 2013-09-16 15:40:54 UTC
Pushed to master.  Thanks, Magnus!
Comment 5 Tomás Cohen Arazi 2013-11-06 16:45:13 UTC
This patch has been pushed to 3.12.x, will be in 3.12.7.

Thanks Magnus!