[Bug 43549] New: Convert misc/cronjobs/cleanup_database.pl to the Getopt::Long::Descriptive convention (see bug 43546)
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 Bug ID: 43549 Summary: Convert misc/cronjobs/cleanup_database.pl to the Getopt::Long::Descriptive convention (see bug 43546) Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Command-line Utilities Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org CC: jake.deery@openfifth.co.uk, robin@catalyst.net.nz Depends on: 43546 Target Milestone: --- This is one of a set of bugs filed alongside bug 43546 (Standardize command-line scripts on Getopt::Long::Descriptive) to convert individual, commonly-used cron scripts to whatever declarative option-parsing convention that bug settles on. Depends on 43546. Current state: cleanup_database.pl uses plain Getopt::Long (GetOptions call around line 198) with roughly 50 options. No single option is required; instead the script requires at least one of a large set of purge-type flags to be set, enforced by a long "unless ($sessions || $zebraqueue_days || ... )" condition after parsing (around line 280). What this bug covers: convert the GetOptions call to the new convention for consistent declaration and auto-generated --help output. None of these options map cleanly onto a simple per-option required constraint, since the actual requirement is "at least one of roughly 50 flags", not any single mandatory option -- that check should remain a manual post-parse validation, same as today. Given the option count, this is a larger and more involved conversion than the others in this set. Suggest treating it as lower priority -- do it once the convention is proven out on a smaller script first. Test plan: - --help output covers all documented options and matches current behavior - Running with no purge flags still fails with the same "nothing to do" message - Running with any single purge flag still behaves exactly as before Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 [Bug 43546] Standardize command-line scripts on Getopt::Long::Descriptive for safer, more consistent option handling -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk Sponsorship status|--- |Unsponsored Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206090 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206090&action=edit Bug 43549: Convert cleanup_database.pl to the Getopt::Long::Descriptive convention Converts cleanup_database.pl's GetOptions call to Koha::Script->describe_options (bug 43546). This is a larger, ~50-option script; none map cleanly onto a per-option required constraint, since the actual requirement is "at least one of roughly 50 purge-type flags", not any single mandatory option. That check remains manual post-parse validation, as does the mutually-exclusive --restrictions/--all-restrictions check and the --illrequests-days-requires---illrequests-status check -- none of these are expressible as per-option constraints. They now use $usage->die for output consistent with the rest of the convention, rather than the script's own hand-rolled usage() sub, which is removed along with its ~80-line duplicated option summary (describe_options generates the equivalent from each option's own description). The optional-numeric-with-bare-flag-means-zero pattern used throughout (e.g. 'import:i' bare vs. 'import:i 60') is unchanged -- it's a Getopt::Long behaviour that Getopt::Long::Descriptive passes straight through, verified against several options in this conversion. This script had no existing POD (its --help was a hand-rolled heredoc), so a minimal NAME section is added -- otherwise the new --man option (bug 43546) would have nothing to show and falls back to dumping the raw source. Test plan: 1. Run: misc/cronjobs/cleanup_database.pl --help Confirm it lists all ~50 documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/cleanup_database.pl --man Confirm it prints a NAME section (not a raw source dump) and exits 0. 3. Run: misc/cronjobs/cleanup_database.pl With the PurgeListShareInvitesOlderThan system preference empty, confirm it fails with "You did not specify any cleanup work for the script to do." and the usage text, and exits non-zero. (With that preference set, as in the KTD sample data, the script legitimately has purge work to do by default -- this is existing behaviour, unrelated to this conversion.) 4. Run: misc/cronjobs/cleanup_database.pl --restrictions 10 --all-restrictions misc/cronjobs/cleanup_database.pl --illrequests-days 10 Confirm both still fail with their existing, specific error messages. 5. Run a selection of purge flags without --confirm, e.g.: misc/cronjobs/cleanup_database.pl --sessions --verbose misc/cronjobs/cleanup_database.pl --old-issues 99999 --verbose misc/cronjobs/cleanup_database.pl --fees 5 --verbose misc/cronjobs/cleanup_database.pl --jobs-days --jobs-type foo --jobs-type bar --verbose Confirm dry-run reporting (including default-day substitution for bare numeric flags, e.g. bare --jobs-days defaulting to 1 day) matches this script's behavior before this patch. 6. Confirm koha-qa.pl passes for the changed file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|43546 |43557 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43546 [Bug 43546] Standardize command-line scripts on Getopt::Long::Descriptive for safer, more consistent option handling https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43557 [Bug 43557] Add declarative mutually-exclusive option support to Koha::Script->describe_options -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206090|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206180 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206180&action=edit Bug 43549: Convert cleanup_database.pl to the Getopt::Long::Descriptive convention Converts cleanup_database.pl's GetOptions call to Koha::Script->describe_options (bug 43546). This is a larger, ~50-option script; none map cleanly onto a per-option required constraint, since the actual requirement is "at least one of roughly 50 purge-type flags", not any single mandatory option. That check remains manual post-parse validation, as does the mutually-exclusive --restrictions/--all-restrictions check and the --illrequests-days-requires---illrequests-status check -- none of these are expressible as per-option constraints. They now use $usage->die for output consistent with the rest of the convention, rather than the script's own hand-rolled usage() sub, which is removed along with its ~80-line duplicated option summary (describe_options generates the equivalent from each option's own description). The optional-numeric-with-bare-flag-means-zero pattern used throughout (e.g. 'import:i' bare vs. 'import:i 60') is unchanged -- it's a Getopt::Long behaviour that Getopt::Long::Descriptive passes straight through, verified against several options in this conversion. This script had no existing POD (its --help was a hand-rolled heredoc), so a minimal NAME section is added -- otherwise the new --man option (bug 43546) would have nothing to show and falls back to dumping the raw source. Test plan: 1. Run: misc/cronjobs/cleanup_database.pl --help Confirm it lists all ~50 documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/cleanup_database.pl --man Confirm it prints a NAME section (not a raw source dump) and exits 0. 3. Run: misc/cronjobs/cleanup_database.pl With the PurgeListShareInvitesOlderThan system preference empty, confirm it fails with "You did not specify any cleanup work for the script to do." and the usage text, and exits non-zero. (With that preference set, as in the KTD sample data, the script legitimately has purge work to do by default -- this is existing behaviour, unrelated to this conversion.) 4. Run: misc/cronjobs/cleanup_database.pl --restrictions 10 --all-restrictions misc/cronjobs/cleanup_database.pl --illrequests-days 10 Confirm both still fail with their existing, specific error messages. 5. Run a selection of purge flags without --confirm, e.g.: misc/cronjobs/cleanup_database.pl --sessions --verbose misc/cronjobs/cleanup_database.pl --old-issues 99999 --verbose misc/cronjobs/cleanup_database.pl --fees 5 --verbose misc/cronjobs/cleanup_database.pl --jobs-days --jobs-type foo --jobs-type bar --verbose Confirm dry-run reporting (including default-day substitution for bare numeric flags, e.g. bare --jobs-days defaulting to 1 day) matches this script's behavior before this patch. 6. Confirm koha-qa.pl passes for the changed file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43549 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206181&action=edit Bug 43549: (follow-up) Use the new Koha::Script exclusive option support for --restrictions/--all-restrictions Replace the hand-rolled $usage->die() mutual-exclusivity check with the declarative exclusive key added to Koha::Script->describe_options on bug 43557, removing another piece of hand-rolled option validation. Test plan: 1. cleanup_database.pl --restrictions 10 --all-restrictions Still rejected as mutually exclusive, same message as before. 2. cleanup_database.pl --restrictions 10 Still runs (dry-run without --confirm), unaffected. 3. koha-qa.pl passes. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org