Summary: | Add verbose and test modes to the `automatic_renewals.pl` cronjob | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
Component: | Command-line Utilities | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | 1joynelson, andrewfh, dcook, jonathan.druart, martin.renvoize, robin |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
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
Bug 24526: Add verbose and commit options to automatic_renewals cronjob Bug 24526: (follow-up) clean up verbose conditional Bug 24526: Add verbose and commit options to automatic_renewals cronjob Bug 24526: (follow-up) clean up verbose conditional Bug 24526: Add verbose and commit options to automatic_renewals cronjob Bug 24526: (follow-up) clean up verbose conditional Bug 24526: Use sprintf for readability Bug 24526: Replace commit with confirm Bug 24526: Set verbose if confirm not set Bug 24526: Add the confirm flag to the cronjob files |
Description
Nick Clemens (kidclamp)
2020-01-28 14:26:47 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 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> 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. Created attachment 98081 [details] [review] Bug 24526: (follow-up) clean up verbose conditional 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> Created attachment 98083 [details] [review] Bug 24526: (follow-up) clean up verbose conditional Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> 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> 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> Created attachment 98499 [details] [review] Bug 24526: Use sprintf for readability Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> Created attachment 98501 [details] [review] Bug 24526: Set verbose if confirm not set Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nick, I submitted few follow-ups, do you agree with them? 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 (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! (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. Created attachment 98520 [details] [review] Bug 24526: Add the confirm flag to the cronjob files Awesome.. you even added notes for release without prompting! :) Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x 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. (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. |