Bug 2389 - overdue_notices.pl needs a test mode
Summary: overdue_notices.pl needs a test mode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: PC All
: P3 normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-23 01:13 UTC by Chris Cormack
Modified: 2017-06-14 22:13 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 2389: Add --test switch to overdue_notices.pl (2.84 KB, patch)
2016-09-02 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 2389: Add --test switch to overdue_notices.pl (2.89 KB, patch)
2016-09-08 23:33 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 2389: (followup) Add documentation and fix use case (1.79 KB, patch)
2016-09-13 17:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 2389: Add --test switch to overdue_notices.pl (2.96 KB, patch)
2016-09-23 16:58 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 2389: (followup) Add documentation and fix use case (1.86 KB, patch)
2016-09-23 16:58 UTC, Barton Chittenden
Details | Diff | Splinter Review
[PASSED QA] Bug 2389: Add --test switch to overdue_notices.pl (3.02 KB, patch)
2016-09-26 05:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 2389: (followup) Add documentation and fix use case (1.92 KB, patch)
2016-09-26 05:06 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 Chris Cormack 2010-05-21 00:50:38 UTC


---- Reported by Andrew.moore@liblime.com 2008-07-23 13:13:10 ----

The misc/cronjobs/overdue_noties.pl script needs a test mode of some kind. -n gets close, in that it sends no emails, but it can still debar people, and maybe even have other side-effects. We need one mode that tells you what it's going to do, but then doesn't do it.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:50 UTC  ---

This bug was previously known as _bug_ 2389 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2389

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was Andrew.moore@liblime.com.

Comment 1 MJ Ray (software.coop) 2011-05-19 17:11:08 UTC
This is still needed.
Comment 2 Liz Rea 2013-07-02 07:09:18 UTC
Agreed - this is still needed.
Comment 3 Barton Chittenden 2014-05-11 19:31:44 UTC
Additional side effect: when run with -t (trigger notices) and -n (no email), no emails are sent, the list of emails rendered is limited to the set that are triggered, but the triggers themselves are not re-set; the next time that overdue_notices.pl is run with the -t option, any notices triggered on the original run are *not* emailed.

This was definitely unexpected behavior.
Comment 4 Barton Chittenden 2015-06-16 20:19:17 UTC
I've escalated the status on this -- this causes real pain during the trouble-shooting of overdue notices, and it's been languishing for years.
Comment 5 Katrin Fischer 2015-06-16 20:47:35 UTC
(In reply to Barton Chittenden from comment #3)
> Additional side effect: when run with -t (trigger notices) and -n (no
> email), no emails are sent, the list of emails rendered is limited to the
> set that are triggered, but the triggers themselves are not re-set; the next
> time that overdue_notices.pl is run with the -t option, any notices
> triggered on the original run are *not* emailed.
> 
> This was definitely unexpected behavior.

I don't think that this behaviour is possible - the triggers are just calculations on the due date, there is nothing to be reset in the database?

I am afraid escalating this from enh to bug won't make it move faster, unless someone spends time to implement and test this.

There is a similar bug about a test mode for notices - bug 8000 - waiting for sign-off. But it won't stop it from debarring I think.
Comment 6 Barton Chittenden 2015-06-17 15:31:20 UTC
(In reply to Katrin Fischer from comment #5)
> (In reply to Barton Chittenden from comment #3)
> > Additional side effect: when run with -t (trigger notices) and -n (no
> > email), no emails are sent, the list of emails rendered is limited to the
> > set that are triggered, but the triggers themselves are not re-set; the next
> > time that overdue_notices.pl is run with the -t option, any notices
> > triggered on the original run are *not* emailed.
> > 
> > This was definitely unexpected behavior.
> 
> I don't think that this behaviour is possible - the triggers are just
> calculations on the due date, there is nothing to be reset in the database?
> 
> I am afraid escalating this from enh to bug won't make it move faster,
> unless someone spends time to implement and test this.
> 
> There is a similar bug about a test mode for notices - bug 8000 - waiting
> for sign-off. But it won't stop it from debarring I think.

It's entirely possible that this was a mis-understanding on my part -- perhaps I did the second run on a different day.

I read through bug 8000; it doesn't really cover my concerns with overdue_notices.pl -- the script is old enough that it contains a lot of code that should by all rights be in a library -- it has complicated internal logic which queries the circ rules and the issues table to determine how many days overdue an item is. The fact that I can't run this safely by hand for trouble-shooting purposes due to documented and possibly un-documented side effects just makes my job hard.

Furthermore, my understanding (even though I can't find anything on the wiki to back this up) is that koha command line scripts are *supposed* to not make any changes to system state unless a '--commit' argument is supplied. I'm not sure that I want to go that far, simply because that  would break a lot of crontabs, but in the spirit of that standard, I think that we should at least have an option to run the script without un-intended consequence.

I'll see if I can make time to add a test-mode switch myself, I definitely did want to bring this back up for discussion again though, simply because I'm currently in a position where I'd like to run the script by hand, and am once again frustrated by the fact that I can't.
Comment 7 Katrin Fischer 2015-06-17 16:03:45 UTC
I didn't intend to say this was not a valid problem - it is. The script is known to be problematic and I think we can all agree about the points you made there.
Comment 8 Nicole C. Engard 2016-06-03 18:27:18 UTC
ByWater is going to sponsor this development
Comment 9 Tomás Cohen Arazi 2016-09-02 19:11:22 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2016-09-08 23:33:42 UTC
Created attachment 55383 [details] [review]
Bug 2389: Add --test switch to overdue_notices.pl

This patch adds the --test option switch to the overdue_notices.pl script
so it can be ran without doing any actual action.

To test:
- Have a patron with overdue items (simulate a checkout for a past date. Note it implies
  that the circ rules are defined so the patron is overdue)
- Run:
  $ sudo koha-shell kohadev
  koha-dev$ misc/cronjobs/overdue_notices.pl --test
=> SUCCESS: The script is ran but the patron isn't debarred and no notice messages are queued.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 11 Katrin Fischer 2016-09-10 10:58:16 UTC
Hi, great to see this moving. 2 things:

1) Please make sure the new option shows when running the script with -h and -man. Otherwise this will be easily overlooked.

2) When runnning this report for a patron with no email address on the patron record the email to the library is still queued.
Comment 12 Tomás Cohen Arazi 2016-09-13 17:43:13 UTC
Created attachment 55547 [details] [review]
Bug 2389: (followup) Add documentation and fix use case

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Barton Chittenden 2016-09-23 16:58:06 UTC
Created attachment 55782 [details] [review]
Bug 2389: Add --test switch to overdue_notices.pl

This patch adds the --test option switch to the overdue_notices.pl script
so it can be ran without doing any actual action.

To test:
- Have a patron with overdue items (simulate a checkout for a past date. Note it implies
  that the circ rules are defined so the patron is overdue)
- Run:
  $ sudo koha-shell kohadev
  koha-dev$ misc/cronjobs/overdue_notices.pl --test
=> SUCCESS: The script is ran but the patron isn't debarred and no notice messages are queued.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>
Comment 14 Barton Chittenden 2016-09-23 16:58:16 UTC
Created attachment 55783 [details] [review]
Bug 2389: (followup) Add documentation and fix use case

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>
Comment 15 Barton Chittenden 2016-09-23 17:11:39 UTC
(In reply to Barton Chittenden from comment #14)
> Created attachment 55783 [details] [review] [review]
> Bug 2389: (followup) Add documentation and fix use case
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> 
> Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

I tested the case where no email address was present. Still no messages queued. Since the notice emailed to the branch email or kohaadmin email is sent through the message queue, the patch fixed that issue.

Also checked documentation for overdue_notices.pl; --help option is documented.

I also tried checking the 'restrict' and 'print' check boxes in the triggers.

The borrower was neither restricted nor was a print notice generated.

All is well,

I've signed off.
Comment 16 Katrin Fischer 2016-09-26 05:05:27 UTC
Created attachment 55812 [details] [review]
[PASSED QA] Bug 2389: Add --test switch to overdue_notices.pl

This patch adds the --test option switch to the overdue_notices.pl script
so it can be ran without doing any actual action.

To test:
- Have a patron with overdue items (simulate a checkout for a past date. Note it implies
  that the circ rules are defined so the patron is overdue)
- Run:
  $ sudo koha-shell kohadev
  koha-dev$ misc/cronjobs/overdue_notices.pl --test
=> SUCCESS: The script is ran but the patron isn't debarred and no notice messages are queued.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 17 Katrin Fischer 2016-09-26 05:06:09 UTC
Created attachment 55813 [details] [review]
[PASSED QA] Bug 2389: (followup) Add documentation and fix use case

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 18 Kyle M Hall 2016-09-26 13:52:18 UTC
Pushed to master for 16.11, thanks Tomas!
Comment 19 Frédéric Demians 2016-09-30 07:51:16 UTC
Pushed in 16.05. Will be in 16.05.05.