[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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k Status|Passed QA |Failed QA --- Comment #6 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi all, multi-letter options no longer work with a single dash. Before the patch, both -fo=5 and --fo=5 worked; after it, only the double-dash form does. This affects the short aliases documented in the script's POD (-fo, -fu, -rb, -ra) as well as full names like -verbose, -too_old and -branch. To reproduce in KTD (no --confirm, so nothing is changed): 1) Before applying the patch, run: misc/cronjobs/update_patrons_category.pl -f PT -t L -fo=5 -v 2) Confirm it says "Will update patrons from PT to L" with condition "Total fines more than 5" 3) Apply the patch and run the same command again 4) Confirm it now says "Will update patrons from o=5 to L" and dies with "Categories not found" 5) Run it with a double dash instead: misc/cronjobs/update_patrons_category.pl -f PT -t L --fo=5 -v 6) Confirm this works as before Existing cron lines using single-dash multi-letter options would stop working on upgrade. Some fail loudly (-rb, -ra, -verbose, -too_old), but -branch=CPL silently filters on a branch called "ranch=CPL" instead. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |43557 Status|Failed QA |Passed QA --- Comment #7 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Adding bug 43557 as a dependency here as it prevents the regression I mentioned before. Referenced Bugs: 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=43550 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | 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=43550 --- Comment #8 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- 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 ---------------------------------------------------------------------------- Depends on|43546 | 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43550 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to main |Needs documenting --- Comment #9 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