Bug 16039 - Quiet flag support for share_usage_with_koha_community.pl
Summary: Quiet flag support for share_usage_with_koha_community.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-10 06:01 UTC by Srdjan Jankovic
Modified: 2016-12-05 21:27 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_16039 Added -q flag to share_usage_with_koha_community.pl (1.95 KB, patch)
2016-03-10 06:05 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_16039 Added -q flag to share_usage_with_koha_community.pl (1.94 KB, patch)
2016-03-10 06:35 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16039: Added -q flag to share_usage_with_koha_community.pl (2.32 KB, patch)
2016-03-21 02:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16039: Added -q flag to share_usage_with_koha_community.pl (2.39 KB, patch)
2016-03-21 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16039: Remove useless exit (897 bytes, patch)
2016-03-21 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2016-03-10 06:01:30 UTC

    
Comment 1 Srdjan Jankovic 2016-03-10 06:05:57 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2016-03-10 06:11:15 UTC
To test:

With UsageStats syspref set to No:

* run misc/cronjobs/share_usage_with_koha_community.pl (withiout -q) - "The UsageStats system preference is not set." message with usage info should be output

* run misc/cronjobs/share_usage_with_koha_community.pl -q - the output should be quiet
Comment 3 Srdjan Jankovic 2016-03-10 06:35:27 UTC Comment hidden (obsolete)
Comment 4 Liz Rea 2016-03-10 20:23:37 UTC
Just noting the reasons for this patch - 

The people who get the cron mail for a given Koha server may or may not have anything to do with the library, or may be a support company who get many copies of this message. Being able to quiet this message means we can suppress unwanted cron email without missing out on other operational messages.

It also allows the job to continue running, in the event that someone affiliated with the library does decide to change the syspref - the job will work immediately instead of requiring the system admin to re-enable the job.
Comment 5 Mark Tompsett 2016-03-21 02:34:51 UTC
Comment on attachment 48926 [details] [review]
bug_16039 Added -q flag to share_usage_with_koha_community.pl

Review of attachment 48926 [details] [review]:
-----------------------------------------------------------------

Avoid scope creep.

::: misc/cronjobs/share_usage_with_koha_community.pl
@@ +88,4 @@
>  
>  =item B<-v|--verbose>
>  
> +Verbose mode

Why remove the period? It's not related to this patch?

@@ +92,4 @@
>  
>  =item B<-f|--force>
>  
> +Force the update

Why remove the period? It's not related to this patch?
Comment 6 Mark Tompsett 2016-03-21 02:48:50 UTC
Comment on attachment 48926 [details] [review]
bug_16039 Added -q flag to share_usage_with_koha_community.pl

Review of attachment 48926 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/share_usage_with_koha_community.pl
@@ +19,5 @@
>  ) || pod2usage(1);
>  
>  if ($help) {
>      pod2usage(1);
> +    exit;

Not required.

Each of the following invocations of pod2usage() will print the "SYNOPSIS" section and any "OPTIONS" and/or "ARGUMENTS" sections to STDOUT and will exit with a status of 1:
    pod2usage(1);
Comment 7 Mark Tompsett 2016-03-21 02:50:23 UTC
Other than scope creep (period removals), and a not required exit, this works.
Comment 8 Mark Tompsett 2016-03-21 02:53:20 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2016-03-21 02:59:52 UTC
No it is not related. However, some of the descriptions have dots, and some don't, which is inconsistent. My view is "polish while you're there", rather than creating a bug for two dots, especially when it incurs no cost. But I can remove that bit if you wish.

Exit is there for two reasons:
1. if help is requested no point doing anything, and
2. without it exit status is non-0 (not that important but still)
Comment 10 Mark Tompsett 2016-03-21 03:22:23 UTC
(In reply to Srdjan Jankovic from comment #9)
> Exit is there for two reasons:
> 1. if help is requested no point doing anything, and
> 2. without it exit status is non-0 (not that important but still)

Good point. exit; is 0 status which is correct in my opinion.
Okay. Whoever, take the sign off and run with it. :)
Comment 11 Jonathan Druart 2016-03-21 15:08:19 UTC
Created attachment 49391 [details] [review]
Bug 16039: Added -q flag to share_usage_with_koha_community.pl

To test:

With UsageStats syspref set to No:

* run misc/cronjobs/share_usage_with_koha_community.pl
  (without -q)
  - "The UsageStats system preference is not set." message
    with usage info should be output

* run misc/cronjobs/share_usage_with_koha_community.pl -q
  - the output should be quiet

NOTE: See comment #7.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2016-03-21 15:08:22 UTC
Created attachment 49392 [details] [review]
Bug 16039: Remove useless exit

pod2usage will exit with the status given in parameter.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Mark Tompsett 2016-03-21 16:33:53 UTC
Comment on attachment 49392 [details] [review]
Bug 16039: Remove useless exit

Review of attachment 49392 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/share_usage_with_koha_community.pl
@@ +19,4 @@
>  ) || pod2usage(1);
>  
>  if ($help) {
> +    pod2usage(0);

Ooooo.... much cleaner.
Comment 14 Brendan Gallagher 2016-03-23 20:57:56 UTC
Pushed to Master - should be in the May 2016 release.  Thanks!