Bug 19712 - Fix command line options of delete_records_via_leader.pl
Summary: Fix command line options of delete_records_via_leader.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11084
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-29 13:55 UTC by Magnus Enger
Modified: 2019-10-14 19:57 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19712: Fix test mode for delete_records_via_leader.pl (1.41 KB, patch)
2017-12-20 22:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19712: Followup adding missing use Pod::Usage (717 bytes, patch)
2018-01-19 01:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19712: Fix test mode for delete_records_via_leader.pl (1.47 KB, patch)
2018-03-15 10:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19712: Followup adding missing use Pod::Usage (782 bytes, patch)
2018-03-15 10:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19712: Fix test mode for delete_records_via_leader.pl (1.57 KB, patch)
2018-04-06 09:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19712: Followup adding missing use Pod::Usage (880 bytes, patch)
2018-04-06 09:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!