Bug 29054 - Stop warns from advance_notices.pl if not running in verbose mode
Summary: Stop warns from advance_notices.pl if not running in verbose mode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Hayley Pelham
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-20 04:18 UTC by Hayley Pelham
Modified: 2022-12-12 21:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.11


Attachments
Bug 29054: Add verbose check for warn in advance_notices.pl (1.45 KB, patch)
2021-09-20 23:49 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 29054: Add verbose check for warn in advance_notices.pl (1.51 KB, patch)
2021-11-16 07:37 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29054: Add verbose check for warn in advance_notices.pl (1.60 KB, patch)
2022-01-10 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Hayley Pelham 2021-09-20 04:18:14 UTC
When the advance_notices.pl cronjob is run and the EnhancedMessagingPreferences syspref is set to don't allow, we get the following warning:

The "EnhancedMessagingPreferences" syspref is off.
Therefore, it is unlikely that this script will actually produce any messages to be sent.
To change this, edit the "EnhancedMessagingPreferences" syspref.

The advance_notices.pl script help says:

-v      Verbose. Without this flag set, only fatal errors are reported.

However, the above warning is shown regardless of whether the script is run with the verbose flag.

We need to add a verbosity check to this warn
Comment 1 Hayley Pelham 2021-09-20 23:49:16 UTC
Created attachment 125086 [details] [review]
Bug 29054: Add verbose check for warn in advance_notices.pl

To test:
1) Set EnhancedMessagingPreferences to Don't allow
2) In the koha-shell, run misc/cronjobs/advance_notices.pl -c
3) Note that you see the warning "The "EnhancedMessagingPreferences"
syspref is off... etc."
4) Apply the patch and restart services
5) In the koha-shell, run misc/cronjobs/advance_notices.pl -c and note
the warning no longer shows
6) Still in the shell, run misc/cronjobs/advance_notices.pl -c -v and
note the warning does show

Sponsored-by: Catalyst IT
Comment 2 Fridolin Somers 2021-11-16 07:36:33 UTC
I agree with that
Comment 3 Fridolin Somers 2021-11-16 07:37:15 UTC
Created attachment 127680 [details] [review]
Bug 29054: Add verbose check for warn in advance_notices.pl

To test:
1) Set EnhancedMessagingPreferences to Don't allow
2) In the koha-shell, run misc/cronjobs/advance_notices.pl -c
3) Note that you see the warning "The "EnhancedMessagingPreferences"
syspref is off... etc."
4) Apply the patch and restart services
5) In the koha-shell, run misc/cronjobs/advance_notices.pl -c and note
the warning no longer shows
6) Still in the shell, run misc/cronjobs/advance_notices.pl -c -v and
note the warning does show

Sponsored-by: Catalyst IT
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 4 Marcel de Rooy 2021-12-22 15:57:55 UTC
(In reply to Hayley Pelham from comment #0)
> When the advance_notices.pl cronjob is run and the
> EnhancedMessagingPreferences syspref is set to don't allow, we get the
> following warning:
> 
> The "EnhancedMessagingPreferences" syspref is off.
> Therefore, it is unlikely that this script will actually produce any
> messages to be sent.
> To change this, edit the "EnhancedMessagingPreferences" syspref.
> 
> The advance_notices.pl script help says:
> 
> -v      Verbose. Without this flag set, only fatal errors are reported.

Not sure if we should remove a warn here or edit the above description? Removing fatal or replacing it by a less severe term?

Needs feedback.
Comment 5 Katrin Fischer 2022-01-02 15:34:21 UTC
I agree with only giving this note with -v as at the moment if you have scheduled this on a server with multiple instances where the pref is turned off intentionally, it creates quite a bit of noise.

It's not a fatal one I think, so would fit the description of -v, just referring to a configuration issue.
Comment 6 Hayley Pelham 2022-01-09 21:21:24 UTC
Setting back to Signed off after feedback from Katrin - thanks Katrin!
Comment 7 Marcel de Rooy 2022-01-10 09:49:04 UTC
(In reply to Katrin Fischer from comment #5)
> I agree with only giving this note with -v as at the moment if you have
> scheduled this on a server with multiple instances where the pref is turned
> off intentionally, it creates quite a bit of noise.
> 
> It's not a fatal one I think, so would fit the description of -v, just
> referring to a configuration issue.

I dont oppose pushing it for the sake of multiple instances (where some instances would have enhanced messaging and others might not). But suppressing a warn that tells you that you are running a script that wouldnt do anything, is imo generally not what we should do.
Comment 8 Marcel de Rooy 2022-01-10 09:49:36 UTC
Created attachment 129242 [details] [review]
Bug 29054: Add verbose check for warn in advance_notices.pl

To test:
1) Set EnhancedMessagingPreferences to Don't allow
2) In the koha-shell, run misc/cronjobs/advance_notices.pl -c
3) Note that you see the warning "The "EnhancedMessagingPreferences"
syspref is off... etc."
4) Apply the patch and restart services
5) In the koha-shell, run misc/cronjobs/advance_notices.pl -c and note
the warning no longer shows
6) Still in the shell, run misc/cronjobs/advance_notices.pl -c -v and
note the warning does show

Sponsored-by: Catalyst IT
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Fridolin Somers 2022-01-22 06:27:28 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Katrin Fischer from comment #5)
> > I agree with only giving this note with -v as at the moment if you have
> > scheduled this on a server with multiple instances where the pref is turned
> > off intentionally, it creates quite a bit of noise.
> > 
> > It's not a fatal one I think, so would fit the description of -v, just
> > referring to a configuration issue.
> 
> I dont oppose pushing it for the sake of multiple instances (where some
> instances would have enhanced messaging and others might not). But
> suppressing a warn that tells you that you are running a script that wouldnt
> do anything, is imo generally not what we should do.

We have other examples of script.

membership_expiry.pl and preference MembershipExpiryDaysNotice:
https://git.koha-community.org/Koha-community/Koha/src/commit/e653effe5783e149f5ab69f388aa5509db454ef4/misc/cronjobs/membership_expiry.pl#L182

Using -v is a very common way to know what a script does ;)
Comment 10 Fridolin Somers 2022-01-22 06:33:06 UTC
Created Bug 29928
Comment 11 Fridolin Somers 2022-01-22 07:03:57 UTC
This isnt enhancement, I change to minor to encourage backport
Comment 12 Fridolin Somers 2022-01-22 07:40:27 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Kyle M Hall 2022-02-07 12:05:28 UTC
Pushed to 21.11.x for 21.11.03
Comment 14 Andrew Fuerste-Henry 2022-02-21 14:53:22 UTC
Pushed to 21.05.x for 21.05.11
Comment 15 Victor Grousset/tuxayo 2022-02-24 22:10:30 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.