Bug 24526 - Add verbose and test modes to the `automatic_renewals.pl` cronjob
Summary: Add verbose and test modes to the `automatic_renewals.pl` cronjob
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-28 14:26 UTC by Nick Clemens
Modified: 2020-05-18 14:14 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
This patchset adds new options to the automatic_renewals.pl script to allow test and verbose modes. **Important:** The patches make the --confirm switch required, without it script will be run in test mode. Existing scheduled cronjobs will need to be updated to supply this switch. Running without --confirm will default in verbose mode as well.
Version(s) released in:
20.05.00


Attachments
Bug 24526: Add verbose and commit options to automatic_renewals cronjob (3.87 KB, patch)
2020-01-28 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24526: Add verbose and commit options to automatic_renewals cronjob (3.92 KB, patch)
2020-01-29 10:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 24526: (follow-up) clean up verbose conditional (1.43 KB, patch)
2020-01-29 13:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24526: Add verbose and commit options to automatic_renewals cronjob (3.98 KB, patch)
2020-01-29 14:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24526: (follow-up) clean up verbose conditional (1.49 KB, patch)
2020-01-29 14:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24526: Add verbose and commit options to automatic_renewals cronjob (4.06 KB, patch)
2020-02-05 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24526: (follow-up) clean up verbose conditional (1.56 KB, patch)
2020-02-05 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24526: Use sprintf for readability (2.59 KB, patch)
2020-02-05 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24526: Replace commit with confirm (3.49 KB, patch)
2020-02-05 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24526: Set verbose if confirm not set (853 bytes, patch)
2020-02-05 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24526: Add the confirm flag to the cronjob files (2.75 KB, patch)
2020-02-06 10:33 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 Nick Clemens 2020-01-28 14:26:47 UTC

    
Comment 1 Nick Clemens 2020-01-28 14:30:07 UTC
Created attachment 98037 [details] [review]
Bug 24526: Add verbose and commit options to automatic_renewals cronjob

To test:
 1 - Apply patch
 2 - Have some items marked for auto-renewal
 3 - Run the job with no parameters
 4 - It should print 'Test mode'
 5 - Provide -v
 6 - It should print 'Test mode' then a line for each item
 7 - Provide -v -c
 8 - It should not say test mode, but should provide a line for each item
 9 - Provide -c
10 - Shoudl run as expected with no output
Comment 2 David Nind 2020-01-29 10:48:18 UTC
Created attachment 98059 [details] [review]
Bug 24526: Add verbose and commit options to automatic_renewals cronjob

To test:
 1 - Apply patch
 2 - Have some items marked for auto-renewal
 3 - Run the job with no parameters
 4 - It should print 'Test mode'
 5 - Provide -v
 6 - It should print 'Test mode' then a line for each item
 7 - Provide -v -c
 8 - It should not say test mode, but should provide a line for each item
 9 - Provide -c
10 - Shoudl run as expected with no output

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Andrew Fuerste-Henry 2020-01-29 13:39:59 UTC
Running the cron with only -c produces a line that just reads "would be renewed." Items renew as expected, so some weird text is a minor problem, but still a little weird.
Comment 4 Nick Clemens 2020-01-29 13:48:24 UTC
Created attachment 98081 [details] [review]
Bug 24526: (follow-up) clean up verbose conditional
Comment 5 ByWater Sandboxes 2020-01-29 14:06:33 UTC
Created attachment 98082 [details] [review]
Bug 24526: Add verbose and commit options to automatic_renewals cronjob

To test:
 1 - Apply patch
 2 - Have some items marked for auto-renewal
 3 - Run the job with no parameters
 4 - It should print 'Test mode'
 5 - Provide -v
 6 - It should print 'Test mode' then a line for each item
 7 - Provide -v -c
 8 - It should not say test mode, but should provide a line for each item
 9 - Provide -c
10 - Shoudl run as expected with no output

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 ByWater Sandboxes 2020-01-29 14:06:36 UTC
Created attachment 98083 [details] [review]
Bug 24526: (follow-up) clean up verbose conditional

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Jonathan Druart 2020-02-05 15:05:46 UTC
Created attachment 98497 [details] [review]
Bug 24526: Add verbose and commit options to automatic_renewals cronjob

To test:
 1 - Apply patch
 2 - Have some items marked for auto-renewal
 3 - Run the job with no parameters
 4 - It should print 'Test mode'
 5 - Provide -v
 6 - It should print 'Test mode' then a line for each item
 7 - Provide -v -c
 8 - It should not say test mode, but should provide a line for each item
 9 - Provide -c
10 - Shoudl run as expected with no output

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2020-02-05 15:05:49 UTC
Created attachment 98498 [details] [review]
Bug 24526: (follow-up) clean up verbose conditional

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2020-02-05 15:05:53 UTC
Created attachment 98499 [details] [review]
Bug 24526: Use sprintf for readability

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2020-02-05 15:05:56 UTC
Created attachment 98500 [details] [review]
Bug 24526: Replace commit with confirm

Usually commit takes a value

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2020-02-05 15:06:00 UTC
Created attachment 98501 [details] [review]
Bug 24526: Set verbose if confirm not set

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2020-02-05 15:06:59 UTC
Nick, I submitted few follow-ups, do you agree with them?
Comment 13 Nick Clemens 2020-02-05 16:15:39 UTC
All looks good to me - we wil need to highlight in the release notes that existing cronjobs will need to have the confirm switch added after upgrade
Comment 14 Jonathan Druart 2020-02-05 16:19:10 UTC
(In reply to Nick Clemens from comment #13)
> All looks good to me - we wil need to highlight in the release notes that
> existing cronjobs will need to have the confirm switch added after upgrade

You are right, we also need to update debian/koha-common.cron.daily!
Comment 15 Jonathan Druart 2020-02-05 16:20:11 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Nick Clemens from comment #13)
> > All looks good to me - we wil need to highlight in the release notes that
> > existing cronjobs will need to have the confirm switch added after upgrade
> 
> You are right, we also need to update debian/koha-common.cron.daily!

And the POD of the script. Not ready for pushing.
Comment 16 Jonathan Druart 2020-02-06 10:33:44 UTC
Created attachment 98520 [details] [review]
Bug 24526: Add the confirm flag to the cronjob files
Comment 17 Martin Renvoize 2020-02-10 10:14:14 UTC
Awesome.. you even added notes for release without prompting! :)
Comment 18 Martin Renvoize 2020-02-10 10:17:26 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 19 Joy Nelson 2020-02-11 00:24:51 UTC
enhancement not backported to 19.11.x
Comment 20 Katrin Fischer 2020-05-16 15:02:36 UTC
I wonder if we could highlight this breaking change a bit more in the release notes - currently it's almost lost:

The patches make the --confirm switch required, without it script will be run in test mode. Existing scheduled cronjobs will need to be updated to supply this switch.
Comment 21 David Cook 2020-05-17 23:26:29 UTC
(In reply to Katrin Fischer from comment #20)
> I wonder if we could highlight this breaking change a bit more in the
> release notes - currently it's almost lost:
> 
> The patches make the --confirm switch required, without it script will be
> run in test mode. Existing scheduled cronjobs will need to be updated to
> supply this switch.

Oh wow yeah that's a good idea. Easy to be caught out by that one.