Bug 19712

Summary: Fix command line options of delete_records_via_leader.pl
Product: Koha Reporter: Magnus Enger <magnus>
Component: Command-line UtilitiesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, m.de.rooy, martin.renvoize, robin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11084    
Bug Blocks:    
Attachments: Bug 19712: Fix test mode for delete_records_via_leader.pl
Bug 19712: Followup adding missing use Pod::Usage
Bug 19712: Fix test mode for delete_records_via_leader.pl
Bug 19712: Followup adding missing use Pod::Usage
Bug 19712: Fix test mode for delete_records_via_leader.pl
Bug 19712: Followup adding missing use Pod::Usage

Description Magnus Enger 2017-11-29 13:55:25 UTC
1. This code:

  56  if ( $help || !$confirm ) {
  57 
...
  70     exit();
  71 }

means that unless you specify --confirm you will never be able to run the script - it will just print the usage and exit. That means --test is useless. 

2. The script has both of these options:

     -c --confirm: Script will do nothing without this parameter
     -t --test: Test mode, does not delete records.

What happens if you specify both? Or none? I think test mode should be the default (when no options are specified) and changes to the database should only be done when --confirm is specified. This would be consistent with other scripts, I think.
Comment 1 Jonathan Druart 2017-12-20 22:13:08 UTC
Created attachment 69981 [details] [review]
Bug 19712: Fix test mode for delete_records_via_leader.pl

Test plan:
perl misc/cronjobs/delete_records_via_leader.pl
=> Should display a warning
perl misc/cronjobs/delete_records_via_leader.pl --test
=> Should not display a warning and script should not apply changes
perl misc/cronjobs/delete_records_via_leader.pl --confirm
=> Should not display a warning and script should apply changes
Comment 2 Mark Tompsett 2018-01-19 01:57:21 UTC
Created attachment 70732 [details] [review]
Bug 19712: Followup adding missing use Pod::Usage

pod2usage call failed otherwise.
Comment 3 Brendan Gallagher 2018-03-15 10:41:12 UTC
Created attachment 72925 [details] [review]
Bug 19712: Fix test mode for delete_records_via_leader.pl

Test plan:
perl misc/cronjobs/delete_records_via_leader.pl
=> Should display a warning
perl misc/cronjobs/delete_records_via_leader.pl --test
=> Should not display a warning and script should not apply changes
perl misc/cronjobs/delete_records_via_leader.pl --confirm
=> Should not display a warning and script should apply changes

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 4 Brendan Gallagher 2018-03-15 10:41:23 UTC
Created attachment 72926 [details] [review]
Bug 19712: Followup adding missing use Pod::Usage

pod2usage call failed otherwise.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 5 Marcel de Rooy 2018-04-06 09:23:12 UTC
Created attachment 73752 [details] [review]
Bug 19712: Fix test mode for delete_records_via_leader.pl

Test plan:
perl misc/cronjobs/delete_records_via_leader.pl
=> Should display a warning
perl misc/cronjobs/delete_records_via_leader.pl --test
=> Should not display a warning and script should not apply changes
perl misc/cronjobs/delete_records_via_leader.pl --confirm
=> Should not display a warning and script should apply changes

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2018-04-06 09:23:17 UTC
Created attachment 73753 [details] [review]
Bug 19712: Followup adding missing use Pod::Usage

pod2usage call failed otherwise.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Jonathan Druart 2018-04-06 17:53:29 UTC
Pushed to master for 18.05, thanks to everybody involved!