Bug 10810 - Fix synopsis for -html option to overdue_notices.pl
Summary: Fix synopsis for -html option to overdue_notices.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Target Milestone: ---
Assignee: Magnus Enger
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-31 13:24 UTC by Magnus Enger
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: String patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10810 - Fix synopsis for -html option to overdue_notices.pl (2.10 KB, patch)
2013-08-31 13:37 UTC, Magnus Enger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10810 - Fix synopsis for -html option to overdue_notices.pl (2.42 KB, patch)
2013-09-08 01:28 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 10810 - Fix synopsis for -html option to overdue_notices.pl (2.53 KB, patch)
2013-09-10 21:07 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!