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: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-28 14:26 UTC by Nick Clemens (kidclamp)
Modified: 2020-11-30 21:45 UTC (History)
6 users (show)

See Also:
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
Circulation function:

Attachments
Bug 24526: Add verbose and commit options to automatic_renewals cronjob (3.87 KB, patch)
2020-01-28 14:30 UTC, Nick Clemens (kidclamp) 		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 (kidclamp) 		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
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2020-01-28 14:26:47 UTC 

    


    


      



        
          Comment 1
        

        
          Nick Clemens (kidclamp)

        

        
        

        
          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 (kidclamp)

        

        
        

        
          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 (kidclamp)

        

        
        

        
          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 (ashimema)

        

        
        

        
          2020-02-10 10:14:14 UTC
        

      






Awesome.. you even added notes for release without prompting! :)

    


    


      



        
          Comment 18
        

        
          Martin Renvoize (ashimema)

        

        
        

        
          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.