[Bug 43548] New: Convert misc/cronjobs/runreport.pl to the Getopt::Long::Descriptive convention (see bug 43546)
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Bug ID: 43548 Summary: Convert misc/cronjobs/runreport.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: runreport.pl uses plain Getopt::Long (GetOptions call around line 210). No option here is actually mandatory -- --format defaults to 'text' if omitted (around line 237); --to/--from/--email are only meaningful together and default from the KohaAdminEmailAddress system preference if unset. The script's real "you must supply something" requirement is a positional argument, not a named option: at least one saved report ID must be passed via @ARGV, enforced by "unless (scalar(@ARGV)) { pod2usage(1); }" around line 268. What this bug covers: convert the GetOptions call to the new convention, declaring each option's type and description explicitly, with none marked required (matching current behavior -- nothing here is a false negative to fix). Note for whoever picks this up: the positional report-ID requirement doesn't map onto a simple per-option required flag, so it will likely need to stay as manual validation after parsing, unless the convention from bug 43546 ends up handling leftover/positional arguments some other way -- check against whatever pattern that bug lands on. Test plan: - --help output covers all documented options and matches current behavior - Running with one or more report IDs still behaves exactly as before - Running with zero report IDs still fails with a clear error 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 the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Sponsorship status|--- |Unsponsored Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk Patch complexity|--- |Small patch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206080 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206080&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546). No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, which Getopt::Long::Descriptive can't express as a named option. This surfaced gaps in the bug 43546 convention, now added to Koha::Script->describe_options: - runreport.pl previously had --help (brief), --man (full Pod::Usage manual, since its POD is substantial), and --help+--verbose (also full manual). The new convention's --help covers the first case, but had no equivalent to --man. Rather than reinvent that per-script, --man is now added automatically alongside --help: it prints the calling script's own POD in full (Pod::Usage -verbose 2, defaulting to $0) and exits. --help+--verbose no longer triggers the full manual; --verbose reverts to its own, unrelated meaning of execution verbosity. Scripts using describe_options get --man for free going forward. - A positional-argument requirement (like this script's report ID(s)) has no per-option 'required' flag to declare it with, so it was initially left as manual post-parse validation. That is exactly the kind of ad hoc, unenforced, unparseable check bug 43546 exists to replace for named options -- it just had nicer error formatting. describe_options now accepts a declarative 'args' key (in the same trailing hashref as 'epilog'), e.g. C<args => { min => 1, name => 'reportID', variadic => 1 }>, enforced by describe_options itself with the same consistent error/usage output as a missing required option. This also gives static tooling (e.g. the koha-plugin-crontab plugin, which reads script source rather than executing it) a literal marker for a required positional argument, the same way it already reads 'required => 1' for named options. describe_options also returns ($opt, $usage) in list context (unchanged in scalar context) for any post-parse validation 'args' doesn't cover. Beyond the mechanical conversion, a few of runreport.pl's own option- handling clauses were simplified using more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which is a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Confirm the dependency (Getopt::Long::Descriptive) and helper (Koha::Script->describe_options) are already in place from bug 43546. 2. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 3. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 4. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 5. Create one or more saved SQL reports (Reports > Guided reports), note their IDs, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> <id2> misc/cronjobs/runreport.pl --separator ';' <id> Confirm output matches this script's behavior before this patch: tab-separated by default, comma-separated with a header row when requested, verbose logging of the SQL/argument count/result count, and a "Cannot specify separator if not using CSV format" warning (with default tab-separated output still produced) for the last case. 6. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected by the describe_options changes here. 7. Confirm koha-qa.pl passes for the changed files. 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=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206080|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=43548 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206082&action=edit Bug 43548: Add --man and positional-argument support to Koha::Script->describe_options Converting runreport.pl (next commit) surfaced two gaps in the describe_options convention introduced by bug 43546: - runreport.pl previously had --help (brief), --man (full Pod::Usage manual, since its POD is substantial), and --help+--verbose (also full manual). The new convention's --help covers the first case, but had no equivalent to --man. Rather than reinvent that per-script, --man is now added automatically alongside --help: it prints the calling script's own POD in full (Pod::Usage -verbose 2, defaulting to $0) and exits. --help+--verbose no longer triggers the full manual; --verbose reverts to its own, unrelated meaning of execution verbosity. Scripts using describe_options get --man for free going forward. - A positional-argument requirement (like runreport.pl's report ID(s)) has no per-option 'required' flag to declare it with. Left as manual post-parse validation, it's exactly the kind of ad hoc, unenforced, unparseable check bug 43546 exists to replace for named options -- it would just have nicer error formatting. describe_options now accepts a declarative 'args' key (in the same trailing hashref as 'epilog'), e.g. C<args =\> { min =\> 1, name =\> 'reportID', variadic =\> 1 }>, enforced by describe_options itself with the same consistent error/usage output as a missing required option. This also gives static tooling (e.g. the koha-plugin-crontab plugin, which reads script source rather than executing it) a literal marker for a required positional argument, the same way it already reads 'required =\> 1' for named options. describe_options also returns ($opt, $usage) in list context (unchanged in scalar context) for any post-parse validation 'args' doesn't cover. Test plan: 1. prove t/Koha/Script.t 2. koha-qa.pl passes for Koha/Script.pm (--man and args are exercised functionally in the next commit, which converts runreport.pl to use them) 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=43548 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206083 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206083&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. 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=43548 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206084 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206084&action=edit Bug 43548: Simplify runreport.pl's option-handling using Getopt::Long::Descriptive A few of runreport.pl's own option-handling clauses can lean on more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which would be a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Run misc/cronjobs/runreport.pl --separator ';' <id> (without --format csv) and confirm it still warns "Cannot specify separator if not using CSV format" and still produces default tab-separated output, unaffected by the warning. 2. Run misc/cronjobs/runreport.pl --format csv --separator ';' --quote "'" --csv-header <id> and confirm the custom separator/quote are applied. 3. Run misc/cronjobs/runreport.pl --to someone@example.org <id> and confirm it still attempts to email the report (implied by --to). 4. Run misc/cronjobs/runreport.pl <id> <id2> and confirm both reports run. 5. 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=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206083|0 |1 is obsolete| | Attachment #206084|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=43548 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206088 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206088&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. 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=43548 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206089 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206089&action=edit Bug 43548: Simplify runreport.pl's option-handling using Getopt::Long::Descriptive A few of runreport.pl's own option-handling clauses can lean on more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which would be a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Run misc/cronjobs/runreport.pl --separator ';' <id> (without --format csv) and confirm it still warns "Cannot specify separator if not using CSV format" and still produces default tab-separated output, unaffected by the warning. 2. Run misc/cronjobs/runreport.pl --format csv --separator ';' --quote "'" --csv-header <id> and confirm the custom separator/quote are applied. 3. Run misc/cronjobs/runreport.pl --to someone@example.org <id> and confirm it still attempts to email the report (implied by --to). 4. Run misc/cronjobs/runreport.pl <id> <id2> and confirm both reports run. 5. 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=43548 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Obsoleting this patch: the Koha::Script->describe_options --man/args support has moved to bug 43546 (it's part of establishing the convention itself, and other sibling bugs depending on 43546 need it too, not just this one). See bug 43546 comments for the updated patch. This bug's remaining two patches (runreport.pl conversion + follow-up) now depend on 43546's updated series instead of including this commit directly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206082|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=43548 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206088|0 |1 is obsolete| | Attachment #206089|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=43548 --- Comment #8 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 207232 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207232&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #9 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 207233 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207233&action=edit Bug 43548: Simplify runreport.pl's option-handling using Getopt::Long::Descriptive A few of runreport.pl's own option-handling clauses can lean on more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which would be a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Run misc/cronjobs/runreport.pl --separator ';' <id> (without --format csv) and confirm it still warns "Cannot specify separator if not using CSV format" and still produces default tab-separated output, unaffected by the warning. 2. Run misc/cronjobs/runreport.pl --format csv --separator ';' --quote "'" --csv-header <id> and confirm the custom separator/quote are applied. 3. Run misc/cronjobs/runreport.pl --to someone@example.org <id> and confirm it still attempts to email the report (implied by --to). 4. Run misc/cronjobs/runreport.pl <id> <id2> and confirm both reports run. 5. Confirm koha-qa.pl passes for the changed file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |andrew@bywatersolutions.com |y.org | Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207232|0 |1 is obsolete| | Attachment #207233|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=43548 --- Comment #10 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 208004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208004&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #11 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 208005 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208005&action=edit Bug 43548: Simplify runreport.pl's option-handling using Getopt::Long::Descriptive A few of runreport.pl's own option-handling clauses can lean on more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which would be a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Run misc/cronjobs/runreport.pl --separator ';' <id> (without --format csv) and confirm it still warns "Cannot specify separator if not using CSV format" and still produces default tab-separated output, unaffected by the warning. 2. Run misc/cronjobs/runreport.pl --format csv --separator ';' --quote "'" --csv-header <id> and confirm the custom separator/quote are applied. 3. Run misc/cronjobs/runreport.pl --to someone@example.org <id> and confirm it still attempts to email the report (implied by --to). 4. Run misc/cronjobs/runreport.pl <id> <id2> and confirm both reports run. 5. Confirm koha-qa.pl passes for the changed file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43813 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43813 [Bug 43813] Remove To Do from runreport.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_26_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Pushed to main for 26.11 with consent from Pedro whilst he's on annual leave -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to main |Needs documenting --- Comment #13 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Enhancement, no 26.05.x backport. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org