[Bug 43550] New: Convert misc/cronjobs/update_patrons_category.pl to Getopt::Long::Descriptive, with --from/--to as genuinely required options (see bug 43546)
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Bug ID: 43550 Summary: Convert misc/cronjobs/update_patrons_category.pl to Getopt::Long::Descriptive, with --from/--to as genuinely required options (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: update_patrons_category.pl uses plain Getopt::Long (GetOptions call around line 171). -f/--from and -t/--to (source and destination patron category codes) are effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category (around line 224), including when the option was never supplied at all -- but Getopt::Long has no way to express that, so nothing stops the script from running with neither supplied and proceeding through several other steps (including the --regbefore/--regafter date-range validation around lines 205-216) before that die is reached. This is a direct analog of cart_to_shelf.pl's -h/--hours case described in bug 43546 -- a genuinely mandatory option currently enforced only by an ad hoc post-parse die, well after other processing has already happened. What this bug covers: convert to the new convention, declaring --from/-f and --to/-t as required, so a missing category code is rejected immediately and consistently, before any of the date-range validation logic runs. Test plan: - --help output covers all documented options and matches current behavior - Omitting --from or --to now fails immediately with a clear, consistent error, before any other validation runs - Supplying both still behaves exactly as before, including the existing "Categories not found" check for invalid (but present) category codes 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=43550 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206087 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206087&action=edit Bug 43550: Convert update_patrons_category.pl to the Getopt::Long::Descriptive convention Converts update_patrons_category.pl's GetOptions call to Koha::Script->describe_options (bug 43546), declaring --from/-f and --to/-t as required. Both were already effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category, including when the option was never supplied at all. But the existing guard against that, if ( not $fromcat && $tocat ) { print "Must supply category from and to (-f & -t) ...\n"; pod2usage(1); } is itself buggy -- operator precedence makes it parse as "(not $fromcat) && $tocat", so it only catches the single case of --to given without --from. Missing both, or --from given without --to, both fell through this check entirely and ran the full --regbefore/--regafter date-range validation before hitting the "Categories not found" die. Declaring both options required replaces this ad hoc (and broken) check with declarative enforcement that correctly rejects all three missing-option combinations immediately, before any other validation runs -- the "Categories not found" die itself is unchanged and still guards against present-but-invalid category codes. The now-unused $remove_guarantors variable (declared but never wired to an option or referenced anywhere) is also dropped. Test plan: 1. Run: misc/cronjobs/update_patrons_category.pl --help Confirm it lists all documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/update_patrons_category.pl --man Confirm it prints the script's full existing POD and exits 0. 3. Run each of: misc/cronjobs/update_patrons_category.pl misc/cronjobs/update_patrons_category.pl --from PT misc/cronjobs/update_patrons_category.pl --to L Confirm each fails immediately with a clear "Mandatory parameter ... missing" error and the usage text, before any other processing, and exits non-zero. 4. Run: misc/cronjobs/update_patrons_category.pl --from NOPE1 --to NOPE2 Confirm it still fails with "Categories not found" (unchanged behavior for present-but-invalid codes). 5. Run: misc/cronjobs/update_patrons_category.pl --from PT --to L --verbose Confirm it reports (without updating) which patrons would move from PT to L, matching this script's behavior before this patch. Add --confirm to actually perform the update. 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 the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- 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=43550 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- 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=43550 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=43550 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206087|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=43550 --- Comment #2 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 207407 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207407&action=edit Bug 43550: Convert update_patrons_category.pl to the Getopt::Long::Descriptive convention Converts update_patrons_category.pl's GetOptions call to Koha::Script->describe_options (bug 43546), declaring --from/-f and --to/-t as required. Both were already effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category, including when the option was never supplied at all. But the existing guard against that, if ( not $fromcat && $tocat ) { print "Must supply category from and to (-f & -t) ...\n"; pod2usage(1); } is itself buggy -- operator precedence makes it parse as "(not $fromcat) && $tocat", so it only catches the single case of --to given without --from. Missing both, or --from given without --to, both fell through this check entirely and ran the full --regbefore/--regafter date-range validation before hitting the "Categories not found" die. Declaring both options required replaces this ad hoc (and broken) check with declarative enforcement that correctly rejects all three missing-option combinations immediately, before any other validation runs -- the "Categories not found" die itself is unchanged and still guards against present-but-invalid category codes. The now-unused $remove_guarantors variable (declared but never wired to an option or referenced anywhere) is also dropped. Test plan: 1. Run: misc/cronjobs/update_patrons_category.pl --help Confirm it lists all documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/update_patrons_category.pl --man Confirm it prints the script's full existing POD and exits 0. 3. Run each of: misc/cronjobs/update_patrons_category.pl misc/cronjobs/update_patrons_category.pl --from PT misc/cronjobs/update_patrons_category.pl --to L Confirm each fails immediately with a clear "Mandatory parameter ... missing" error and the usage text, before any other processing, and exits non-zero. 4. Run: misc/cronjobs/update_patrons_category.pl --from NOPE1 --to NOPE2 Confirm it still fails with "Categories not found" (unchanged behavior for present-but-invalid codes). 5. Run: misc/cronjobs/update_patrons_category.pl --from PT --to L --verbose Confirm it reports (without updating) which patrons would move from PT to L, matching this script's behavior before this patch. Add --confirm to actually perform the update. 6. 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=43550 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact|testopia@bugs.koha-communit |andrew@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207407|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=43550 --- Comment #3 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 208043 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208043&action=edit Bug 43550: Convert update_patrons_category.pl to the Getopt::Long::Descriptive convention Converts update_patrons_category.pl's GetOptions call to Koha::Script->describe_options (bug 43546), declaring --from/-f and --to/-t as required. Both were already effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category, including when the option was never supplied at all. But the existing guard against that, if ( not $fromcat && $tocat ) { print "Must supply category from and to (-f & -t) ...\n"; pod2usage(1); } is itself buggy -- operator precedence makes it parse as "(not $fromcat) && $tocat", so it only catches the single case of --to given without --from. Missing both, or --from given without --to, both fell through this check entirely and ran the full --regbefore/--regafter date-range validation before hitting the "Categories not found" die. Declaring both options required replaces this ad hoc (and broken) check with declarative enforcement that correctly rejects all three missing-option combinations immediately, before any other validation runs -- the "Categories not found" die itself is unchanged and still guards against present-but-invalid category codes. The now-unused $remove_guarantors variable (declared but never wired to an option or referenced anywhere) is also dropped. Test plan: 1. Run: misc/cronjobs/update_patrons_category.pl --help Confirm it lists all documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/update_patrons_category.pl --man Confirm it prints the script's full existing POD and exits 0. 3. Run each of: misc/cronjobs/update_patrons_category.pl misc/cronjobs/update_patrons_category.pl --from PT misc/cronjobs/update_patrons_category.pl --to L Confirm each fails immediately with a clear "Mandatory parameter ... missing" error and the usage text, before any other processing, and exits non-zero. 4. Run: misc/cronjobs/update_patrons_category.pl --from NOPE1 --to NOPE2 Confirm it still fails with "Categories not found" (unchanged behavior for present-but-invalid codes). 5. Run: misc/cronjobs/update_patrons_category.pl --from PT --to L --verbose Confirm it reports (without updating) which patrons would move from PT to L, matching this script's behavior before this patch. Add --confirm to actually perform the update. 6. 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=43550 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208043|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=43550 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208585&action=edit Bug 43550: Convert update_patrons_category.pl to the Getopt::Long::Descriptive convention Converts update_patrons_category.pl's GetOptions call to Koha::Script->describe_options (bug 43546), declaring --from/-f and --to/-t as required. Both were already effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category, including when the option was never supplied at all. But the existing guard against that, if ( not $fromcat && $tocat ) { print "Must supply category from and to (-f & -t) ...\n"; pod2usage(1); } is itself buggy -- operator precedence makes it parse as "(not $fromcat) && $tocat", so it only catches the single case of --to given without --from. Missing both, or --from given without --to, both fell through this check entirely and ran the full --regbefore/--regafter date-range validation before hitting the "Categories not found" die. Declaring both options required replaces this ad hoc (and broken) check with declarative enforcement that correctly rejects all three missing-option combinations immediately, before any other validation runs -- the "Categories not found" die itself is unchanged and still guards against present-but-invalid category codes. The now-unused $remove_guarantors variable (declared but never wired to an option or referenced anywhere) is also dropped. Test plan: 1. Run: misc/cronjobs/update_patrons_category.pl --help Confirm it lists all documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/update_patrons_category.pl --man Confirm it prints the script's full existing POD and exits 0. 3. Run each of: misc/cronjobs/update_patrons_category.pl misc/cronjobs/update_patrons_category.pl --from PT misc/cronjobs/update_patrons_category.pl --to L Confirm each fails immediately with a clear "Mandatory parameter ... missing" error and the usage text, before any other processing, and exits non-zero. 4. Run: misc/cronjobs/update_patrons_category.pl --from NOPE1 --to NOPE2 Confirm it still fails with "Categories not found" (unchanged behavior for present-but-invalid codes). 5. Run: misc/cronjobs/update_patrons_category.pl --from PT --to L --verbose Confirm it reports (without updating) which patrons would move from PT to L, matching this script's behavior before this patch. Add --confirm to actually perform the update. 6. 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=43550 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208585|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=43550 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208600 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208600&action=edit Bug 43550: Convert update_patrons_category.pl to the Getopt::Long::Descriptive convention Converts update_patrons_category.pl's GetOptions call to Koha::Script->describe_options (bug 43546), declaring --from/-f and --to/-t as required. Both were already effectively mandatory: the script dies with "Categories not found" if either fails to resolve to a real category, including when the option was never supplied at all. But the existing guard against that, if ( not $fromcat && $tocat ) { print "Must supply category from and to (-f & -t) ...\n"; pod2usage(1); } is itself buggy -- operator precedence makes it parse as "(not $fromcat) && $tocat", so it only catches the single case of --to given without --from. Missing both, or --from given without --to, both fell through this check entirely and ran the full --regbefore/--regafter date-range validation before hitting the "Categories not found" die. Declaring both options required replaces this ad hoc (and broken) check with declarative enforcement that correctly rejects all three missing-option combinations immediately, before any other validation runs -- the "Categories not found" die itself is unchanged and still guards against present-but-invalid category codes. The now-unused $remove_guarantors variable (declared but never wired to an option or referenced anywhere) is also dropped. Test plan: 1. Run: misc/cronjobs/update_patrons_category.pl --help Confirm it lists all documented options and usage examples, and exits 0. 2. Run: misc/cronjobs/update_patrons_category.pl --man Confirm it prints the script's full existing POD and exits 0. 3. Run each of: misc/cronjobs/update_patrons_category.pl misc/cronjobs/update_patrons_category.pl --from PT misc/cronjobs/update_patrons_category.pl --to L Confirm each fails immediately with a clear "Mandatory parameter ... missing" error and the usage text, before any other processing, and exits non-zero. 4. Run: misc/cronjobs/update_patrons_category.pl --from NOPE1 --to NOPE2 Confirm it still fails with "Categories not found" (unchanged behavior for present-but-invalid codes). 5. Run: misc/cronjobs/update_patrons_category.pl --from PT --to L --verbose Confirm it reports (without updating) which patrons would move from PT to L, matching this script's behavior before this patch. Add --confirm to actually perform the update. 6. 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.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org