|
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. |