Description
Nick Clemens (kidclamp)
2016-08-23 15:20:04 UTC
Created attachment 60279 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v perl update_patrons_category.pl -f PT -t J -v 4 - All patrons should have been switched to and from Juveniles 5 - Try with -n switch perl update_patrons_category.pl -f PT -t J -v -n 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without -n perl update_patrons_category.pl -f J -a -t PT -v -n perl update_patrons_category.pl -f J -a -t PT -v 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Created attachment 60280 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v perl update_patrons_category.pl -f PT -t J -v 4 - All patrons should have been switched to and from Juveniles 5 - Try with -n switch perl update_patrons_category.pl -f PT -t J -v -n 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without -n perl update_patrons_category.pl -f J -a -t PT -v -n perl update_patrons_category.pl -f J -a -t PT -v 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) update_patrons_category.pl --help shows Usage: update_patrons_category.pl [ -b=<branchcode> -f=<categorycode> -t=<categorycode> ] but Options: ... -f *required* defines the category to update. Expects the code from categories.categorycode. -t *required* defines the category patrons will be converted to. Expects the code from categories.categorycode. Square brackets indicate optional arguments. You don't need angle brackets surrounding <branchcode> and <categorycode>. Elipses show repeated arguments, and a pipe shows alternative arguments. Here's what I would expect for usage, given the options: Usage: update_patrons_category.pl -f=categorycode -t=categorycode [-b=branchcode] [-n] [-a] [-fo=X] [-fu=X] [-rb=date] [-ra=date] [-v] [--field column=value ...] update_patrons_category.pl --help | --man POD ERRORS Hey! The above document had some coding errors, which are explained below: Around line 94: '=item' outside of any '=over' Around line 100: '=item' outside of any '=over' Around line 106: '=item' outside of any '=over' Around line 112: '=item' outside of any '=over' Around line 118: '=item' outside of any '=over' Around line 124: '=item' outside of any '=over' The use of -n No Action. With this flag set, script will report changes but not actually execute them on the database. matches the options of j2a.pl, but I think that the use of --confirm Don't commit changes to the database unless the --confirm option is used Is preferred by the Koha community. When -n is used, so that status is not updated, the script still prints 22 patrons updated This is the kind of thing that makes my blood pressure go up until I double-check in the database. Yes, I see at the top of the run, 'No actions will be taken (test mode)', but I would still prefer 22 patrons would have been updated Just for my peace of mind. Created attachment 60665 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Hi Nick, I think this exists already in the form of j2a.pl, though it may not be as extensible as this verison. (In reply to Liz Rea from comment #8) > Hi Nick, > > I think this exists already in the form of j2a.pl, though it may not be as > extensible as this verison. Yup, the idea was initially to make a script to update new patrons to regular patrons after a period of time. I added as much flexibility as possible, and included the j2a functionality so we could have a single script instead of two Per Nick 'Yes, you can mark it as assigned - it needs rewrites to respect current guidelines' To be done: Move business/logic to a module Add unit tests Remove MySQLisms Created attachment 70921 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Created attachment 73012 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Patch doesn't apply, please rebase! Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 17168 - Add a command line script for updating patron category based on status Using index info to reconstruct a base tree... M Koha/Patrons.pm Falling back to patching base and 3-way merge... Auto-merging Koha/Patrons.pm CONFLICT (content): Merge conflict in Koha/Patrons.pm Failed to merge in the changes. Patch failed at 0001 Bug 17168 - Add a command line script for updating patron category based on status The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-17168---Add-a-command-line-script-for-updating-hwoO66.patch Created attachment 74936 [details] [review] Bug 17168 - Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Can you refresh this script a bit please? At first glance, at least: 1. Indentation 2. More readable options 3. More readable parameters 4. I do not think update_category should be a Koha::Patrons method, to me it belongs to Koha::Patron 5. Why? + my $remove_guarantor = ( $to_cat->category_type ne 'C' || $to_cat->category_type ne 'P' ) ? 1 : 0; 6. Date handling is not the way to do Created attachment 75698 [details] [review] Bug 17168: (follow-up) Tidy, clean params/options, use date tools (In reply to Jonathan Druart from comment #16) > 1. Indentation Tidied > 2. More readable options > 3. More readable parameters OK, longer names available > 4. I do not think update_category should be a Koha::Patrons method, to me it > belongs to Koha::Patron Hmm, I don't know that it does enough to a single patron to be a method, the utility is acting on many at once. I can move if you feel strongly > 5. Why? > + my $remove_guarantor = ( $to_cat->category_type ne 'C' || > $to_cat->category_type ne 'P' ) ? 1 : 0; When this was written adults couldn't have guarantors and we stripped them when updating - I suppose this can be removed? > 6. Date handling is not the way to do OK, fixed (In reply to Nick Clemens from comment #18) > > 5. Why? > > + my $remove_guarantor = ( $to_cat->category_type ne 'C' || > > $to_cat->category_type ne 'P' ) ? 1 : 0; > When this was written adults couldn't have guarantors and we stripped them > when updating - I suppose this can be removed? Ah, j2a still does this - I think removing for children makes sense, maybe not touch otherwise so maybe: my $remove_guarantor = ( $from_cat->category_type eq 'C'); (In reply to Nick Clemens from comment #18) > > 4. I do not think update_category should be a Koha::Patrons method, to me it > > belongs to Koha::Patron > Hmm, I don't know that it does enough to a single patron to be a method, the > utility is acting on many at once. I can move if you feel strongly The method iterates on the patrons, so to me it should be replaced with: while ( my $p = $patrons->next ) { $p->update_category($params); } and the method moved. As the signoff comes from Jesse I would prefer to wait for another QA point of view anyway. I intend on writing a much more generic 'filter_by' method for the Koha::Patrons class which could easily be shared between bulk deletions, bulk anonymizations and this bulk category change enhancement along with some others down the line.. it seems somewhat crazy to add a method for each type of filtering (feels like we're backing ourselves into the same position we ended up in C4).. I'll submit that against bug 11983 which I've adopted.. and likely build a small tree of bugs hanging off it that could use this.. Obviously this is just my opinionated ramblings, but thought it worth highlighting the intention to write similar code. As a hint at where I'm going with it.. I had to throw something together quickly for customers to satisfy GDPR https://gist.github.com/mrenvoize/a0c37b1350d1fe532143784dee124b80.. It's basically pulling out the construction of the $where and $attr 'select' clause that I'm looking to put in a `filter_by` method Sorry Nick, although I don't like the way this is done I would Pass QA on it as there's already precedent and we can refactor it once there's been discussion on bug 11983. Failing QA for because there's a fair number of issues reported by the QA script instead: OK Koha/Patrons.pm FAIL misc/cronjobs/update_patrons_category.pl FAIL critic "$min_dob" is declared but not used at line 157, column 1. Unused variables clutter code and make it harder to read. "$au" is declared but not used at line 155, column 1. Unused variables clutter code and make it harder to read. "$max_dob" is declared but not used at line 158, column 1. Unused variables clutter code and make it harder to read. "$ao" is declared but not used at line 156, column 1. Unused variables clutter code and make it harder to read. FAIL forbidden patterns forbidden pattern: Data::Dumper::Dumper (line 189) FAIL valid Global symbol "$ageunder" requires explicit package name (did you forget to declare "my $ageunder"?) Global symbol "$ageover" requires explicit package name (did you forget to declare "my $ageover"?) misc/cronjobs/update_patrons_category.pl had compilation errors. OK t/db_dependent/Patrons.t Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - a25919ad28 Created attachment 76974 [details] [review] Bug 17168: Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Created attachment 76975 [details] [review] Bug 17168: (follow-up) Tidy, clean params/options, use date tools Created attachment 76976 [details] [review] Bug 17168: (follow-up) Clean up QA errors Cleanup variable names Remove Dumper Comment on attachment 76974 [details] [review] Bug 17168: Add a command line script for updating patron category based on status Review of attachment 76974 [details] [review]: ----------------------------------------------------------------- A few more QA notes. ::: Koha/Patrons.pm @@ +209,4 @@ > return $nb_rows; > } > > +=head3 search_patrons_to_update This is a very generic name for a pretty specific task.. `search_patrons_to_update_category` might be better... @@ +210,5 @@ > } > > +=head3 search_patrons_to_update > + > + my $patrons = Koha::Patrons->search_patrons_to_anonymise( { This signature doesn't match the method name ;) @@ +218,5 @@ > + au => $au, > + ao => $ao, > + }); > + > +This method returns all patron who should be updated form one category to another meeting criteria: Typo 'form' -> 'from' @@ +223,5 @@ > + > +from - original category > +fine_min - with fines totaling at least this amount > +fine_max - with fines above this amount > +au - under the age limit for 'from' Looks like you've renamed this to 'ageunder', also the description could be clearer... it's a boolean right.. not an age in years that can be passed. I'd probably go with `under_age - boolean denoting whether to filter down to those patrons who are under the age limit as defined by their category` I may even go so far as to say can it be 'too_young' or 'is_under_age' adding the adverb clarifies the intention and boolean requirement. @@ +224,5 @@ > +from - original category > +fine_min - with fines totaling at least this amount > +fine_max - with fines above this amount > +au - under the age limit for 'from' > +ao - over the agelimit for 'from' As above but for overage @@ +254,5 @@ > + $query{group_by} = ["borrowernumber"]; > + $query{having}{total_fines}{'<='}=$params->{fine_max} if defined $params->{fine_max}; > + $query{having}{total_fines}{'>='}=$params->{fine_min} if defined $params->{fine_min}; > + } > + return Koha::Patrons->search($search_params,\%query); rather than returning a new Koha::Patrons object here you should use $self->search($search_params,\%query) to allow for future chaining. @@ +257,5 @@ > + } > + return Koha::Patrons->search($search_params,\%query); > +} > + > +=head3 update_category Hmm, it actually scares me a little that this isn't handled inside the 'store' routine. This isn't the only way to update a patrons category and as such the guarantor handling won't get triggered during over forms of the update which could lead to bad data and bugs down the line.. @@ +260,5 @@ > + > +=head3 update_category > + > + Koha::Patrons->search->update_category( { > + to => $to, feels an odd signature.. I'd suggest `->update_category_to($category_code);` also spotted search_params is undocumented for your search_for_update method.. (personally I would chain rather than try to merge search params inside the specialised search method anyway) Also... I don't like that we're very nearly duplicating the /misc/cronjobs/j2a.pl script.. could this not be merged into that one, act as an update to that one.? Also, many of these sorts of scripts are entirely undocumented past the perldoc within the script.. could we have a corresponding koha-manual patch just very quickly summarising that it's there and what it's for? Changing status to Failed QA to get answers to Martin's comments Created attachment 85870 [details] [review] Bug 17168: Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Created attachment 85871 [details] [review] Bug 17168: (follow-up) Tidy, clean params/options, use date tools Created attachment 85872 [details] [review] Bug 17168: (follow-up) Clean up QA errors Cleanup variable names Remove Dumper Created attachment 85873 [details] [review] Bug 17168: (follow-up) Address QA concerns and update for strict mode Created attachment 85878 [details] [review] Bug 17168: Remove j2a.pl script (In reply to Martin Renvoize from comment #29) > Also... I don't like that we're very nearly duplicating the > /misc/cronjobs/j2a.pl script.. could this not be merged into that one, act > as an update to that one.? This is intended to replace that one wholesale, i added a patch to remove and we can highlight largely in the release notes > Also, many of these sorts of scripts are entirely undocumented past the > perldoc within the script.. could we have a corresponding koha-manual patch > just very quickly summarising that it's there and what it's for? I added some documentation to the release notes, this should help for updating the manual once we get this in It seems that the code in Koha::Patrons->update_category_to must be the same as what is done in members/update-child.pl and so moved to Koha::Patron. Do not you think? (In reply to Jonathan Druart from comment #37) > It seems that the code in Koha::Patrons->update_category_to must be the same > as what is done in members/update-child.pl and so moved to Koha::Patron. Do > not you think? I agree on that, but I think that would be better handled on its own bug to decide whether it is always done (on store) or should be optional There is work to allow adults to have guarantors (bug 12446) or multiple guarantors (bug 14570) so want to work nicely with those Just a note: use Koha::Script -cron; should be added to cronjob script - see bug 22600 Also, this needs a rebase Created attachment 88809 [details] [review] Bug 17168: Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Created attachment 88810 [details] [review] Bug 17168: (follow-up) Tidy, clean params/options, use date tools Created attachment 88811 [details] [review] Bug 17168: (follow-up) Clean up QA errors Cleanup variable names Remove Dumper Created attachment 88812 [details] [review] Bug 17168: (follow-up) Address QA concerns and update for strict mode Created attachment 88813 [details] [review] Bug 17168: Add Koha:Script -cron, remove Dumper Created attachment 88814 [details] [review] Bug 17168: Remove j2a.pl Applying: Bug 17168: Remove j2a.pl script Using index info to reconstruct a base tree... M misc/cronjobs/j2a.pl Falling back to patching base and 3-way merge... CONFLICT (modify/delete): misc/cronjobs/j2a.pl deleted in Bug 17168: Remove j2a.pl script and modified in HEAD. Version HEAD of misc/cronjobs/j2a.pl left in tree. error: Failed to merge in the changes. (In reply to Marcel de Rooy from comment #46) > Applying: Bug 17168: Remove j2a.pl script > Using index info to reconstruct a base tree... > M misc/cronjobs/j2a.pl > Falling back to patching base and 3-way merge... > CONFLICT (modify/delete): misc/cronjobs/j2a.pl deleted in Bug 17168: Remove > j2a.pl script and modified in HEAD. Version HEAD of misc/cronjobs/j2a.pl > left in tree. > error: Failed to merge in the changes. Forget that. The first patch should have been obsoleted. + au => $au, + ao => $ao, You mean age under and age over. Not very clear names. Saw adjustments later.. Patch 1 + my ($y,$m,$d) = Today(); + if( $cat_from->dateofbirthrequired && $params->{au} ) { + my ($dy,$dm,$dd) =Add_Delta_YMD($y,$m,$d,-$cat_from->dateofbirthrequired,0,0); + $search_params->{dateofbirth}{'>'} = $dy."-".sprintf("%02d",$dm)."-".sprintf("%02d",$dd); + } Patch 2 + my $date_after = output_pref({ + dt => dt_from_string()->subtract( years => $cat_from->dateofbirthrequired), + dateonly => 1, + dateformat => 'sql' + }); + $search_params->{dateofbirth}{'>'} = $date_after; Instead of constructions like above, we normally use datetime_parser? my $dtf = Koha::Database->new->schema->storage->datetime_parser; my $rs = $class->_resultset->search( { returndate => { '<' => $dtf->format_datetime($older_than_date), }, perl misc/cronjobs/update_patrons_category.pl outputs: Use of uninitialized value $fromcat in concatenation (.) or string at misc/cronjobs/update_patrons_category.pl line 186. Use of uninitialized value $tocat in concatenation (.) or string at misc/cronjobs/update_patrons_category.pl line 186. v 0 c 0 f t at misc/cronjobs/update_patrons_category.pl line 186. I would have expected a more readable usage statement ;) warn "v $verbose c $doit f $fromcat t $tocat"; exit; Hmm Debugging leftovers Just not ready enough to get it further today. Sorry ;) Created attachment 91677 [details] [review] Bug 17168: (follow-up) QA concerns and rebase issues Created attachment 94665 [details] [review] Bug 17168: Add a command line script for updating patron category based on status This patch adds a new script update_patrons_category.pl which allows for updating patron categories in a batch upon meeting provided criteria. This script additionally can replace j2a.pl. To test: 1 - perl update_patrons_category.pl -h 2 - Ensure help text makes sense and covers all options 3 - Test converting patrons supplying only fromcat and tocat perl update_patrons_category.pl -f PT -t J -v --confirm perl update_patrons_category.pl -f J -t PT -v --confirm 4 - All patrons should have been switched to and from Juveniles 5 - Try without --confirm switch perl update_patrons_category.pl -f PT -t J -v 6 - Should list all patrons but not update 7 - Set the age for juvenile patrons to be outside the range provided in categories (or set the upper age limit for juveniles to '2') 8 - Test with verbosity and with without --confirm perl update_patrons_category.pl -f J -a -t PT -v perl update_patrons_category.pl -f J -a -t PT -v --confirm 9 - Repeat above and verify linked/unlinked guarantors are removed in above scenario 10 - Test various fine and registration limits 11 - Test matching on specific fields i.e. --field surname=acosta 12 - Sign off Sponsored by: Round Rock Public Library (https://www.roundrocktexas.gov/departments/library/) Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94666 [details] [review] Bug 17168: (follow-up) Tidy, clean params/options, use date tools Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94667 [details] [review] Bug 17168: (follow-up) Clean up QA errors Cleanup variable names Remove Dumper Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94668 [details] [review] Bug 17168: (follow-up) Address QA concerns and update for strict mode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94669 [details] [review] Bug 17168: Add Koha:Script -cron, remove Dumper Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94670 [details] [review] Bug 17168: Remove j2a.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94671 [details] [review] Bug 17168: (follow-up) QA concerns and rebase issues Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Going for pragmatism here.. as much as I still would love to see a nicer implementation I think that can happen down the line once we've agreed on method naming conventions and things. Passing QA as it seems to all work in testing and passes the QA scripts. Nice work! Pushed to master for 19.11.00 :(. I missed a lot here... this patchset needs updating for bug 14570 Created attachment 94790 [details] [review] Bug 17168: (RM follow-up) Update for bug 14570 Created attachment 94793 [details] [review] Bug 17168: (RM follow-up) Update for bug 14570 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 94794 [details] [review] Bug 17168: (QA follow-up) Only remove guarantor relationship if not Child AND not Professional Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Followups Pushed, thanks guys. |