Bug 17168 - Add a command line script for updating patron category based on status
Summary: Add a command line script for updating patron category based on status
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 12446 25683 25853 26307 27554
  Show dependency treegraph
 
Reported: 2016-08-23 15:20 UTC by Nick Clemens
Modified: 2021-12-13 21:09 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
These patches introduce a new script to replace the `j2a.pl` script with a more flexible set of options. `misc/cronjobs/update_patrons_category.pl` can now be used to update patrons who are older or younger than their patron categories to a category chosen by the user. Additionally, this script allows users to specify a fine total to update patrons, to use any specified borrowers field, and to run by category and/or branch. The intention here is to assist in automating updating patrons for schools or libraries where patrons are regularly changed - patrons with fines can be moved to 'probational statuses' or patrons without fines can be moved to 'privileged statuses'. The flexibility should allow for various workflows to be automated via cronjob.
Version(s) released in:
19.11.00


Attachments
Bug 17168 - Add a command line script for updating patron category based on status (10.86 KB, patch)
2017-02-15 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168 - Add a command line script for updating patron category based on status (11.04 KB, patch)
2017-02-15 17:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168 - Add a command line script for updating patron category based on status (11.37 KB, patch)
2017-02-24 22:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168 - Add a command line script for updating patron category based on status (20.90 KB, patch)
2018-01-25 19:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168 - Add a command line script for updating patron category based on status (20.95 KB, patch)
2018-03-16 10:16 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 17168 - Add a command line script for updating patron category based on status (20.95 KB, patch)
2018-04-30 11:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Tidy, clean params/options, use date tools (15.51 KB, patch)
2018-05-31 10:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Add a command line script for updating patron category based on status (20.95 KB, patch)
2018-07-16 11:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Tidy, clean params/options, use date tools (15.51 KB, patch)
2018-07-16 11:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Clean up QA errors (1.05 KB, patch)
2018-07-16 11:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Add a command line script for updating patron category based on status (21.06 KB, patch)
2019-02-28 13:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Tidy, clean params/options, use date tools (15.53 KB, patch)
2019-02-28 13:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Clean up QA errors (1.05 KB, patch)
2019-02-28 13:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Address QA concerns and update for strict mode (16.21 KB, patch)
2019-02-28 13:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Remove j2a.pl script (9.36 KB, patch)
2019-02-28 13:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Add a command line script for updating patron category based on status (21.01 KB, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Tidy, clean params/options, use date tools (15.53 KB, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Clean up QA errors (1.05 KB, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) Address QA concerns and update for strict mode (16.06 KB, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Add Koha:Script -cron, remove Dumper (730 bytes, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Remove j2a.pl (9.38 KB, patch)
2019-04-25 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (follow-up) QA concerns and rebase issues (5.04 KB, patch)
2019-07-19 21:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: Add a command line script for updating patron category based on status (21.06 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (follow-up) Tidy, clean params/options, use date tools (15.59 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (follow-up) Clean up QA errors (1.12 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (follow-up) Address QA concerns and update for strict mode (16.11 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: Add Koha:Script -cron, remove Dumper (795 bytes, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: Remove j2a.pl (10.78 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (follow-up) QA concerns and rebase issues (5.09 KB, patch)
2019-10-24 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (RM follow-up) Update for bug 14570 (10.77 KB, patch)
2019-10-25 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17168: (RM follow-up) Update for bug 14570 (10.83 KB, patch)
2019-10-25 18:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17168: (QA follow-up) Only remove guarantor relationship if not Child AND not Professional (3.49 KB, patch)
2019-10-25 18:22 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2016-08-23 15:20:04 UTC
Work to be done:
* Create a script to run on a regular basis that updates patrons from one category to another based on parameters passed in as defined below
* checks fine amount
* checks registration date
* checks patron fields for specific value
Comment 1 Nick Clemens 2017-02-15 16:43:39 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
Comment 2 Nick Clemens 2017-02-15 17:03:06 UTC
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/)
Comment 3 Barton Chittenden 2017-02-17 17:01:41 UTC
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
Comment 4 Barton Chittenden 2017-02-17 18:22:16 UTC
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'
Comment 5 Barton Chittenden 2017-02-17 18:29:38 UTC
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.
Comment 6 Barton Chittenden 2017-02-17 18:49:39 UTC
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.
Comment 7 Nick Clemens 2017-02-24 22:21:10 UTC
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/)
Comment 8 Liz Rea 2017-03-02 00:33:35 UTC
Hi Nick, 

I think this exists already in the form of j2a.pl, though it may not be as extensible as this verison.
Comment 9 Nick Clemens 2017-03-02 10:32:28 UTC
(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
Comment 10 Barton Chittenden 2017-04-03 19:20:50 UTC
Per Nick 'Yes, you can mark it as assigned - it needs rewrites to respect current guidelines'
Comment 11 Nick Clemens 2017-04-10 16:00:33 UTC
To be done:
Move business/logic to a module
Add unit tests
Remove MySQLisms
Comment 12 Nick Clemens 2018-01-25 19:26:37 UTC
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/)
Comment 13 Jesse Maseto 2018-03-16 10:16:13 UTC
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>
Comment 14 Katrin Fischer 2018-04-22 10:37:53 UTC
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
Comment 15 Nick Clemens 2018-04-30 11:25:06 UTC
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>
Comment 16 Jonathan Druart 2018-05-30 21:01:36 UTC
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
Comment 17 Nick Clemens 2018-05-31 10:56:45 UTC
Created attachment 75698 [details] [review]
Bug 17168: (follow-up) Tidy, clean params/options, use date tools
Comment 18 Nick Clemens 2018-05-31 11:00:39 UTC
(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
Comment 19 Nick Clemens 2018-05-31 11:17:29 UTC
(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');
Comment 20 Jonathan Druart 2018-05-31 15:07:32 UTC
(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.
Comment 21 Martin Renvoize 2018-06-19 18:12:56 UTC
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.
Comment 22 Martin Renvoize 2018-06-19 18:21:20 UTC
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
Comment 23 Martin Renvoize 2018-07-11 19:21:47 UTC
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
Comment 24 Nick Clemens 2018-07-16 11:06:17 UTC
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>
Comment 25 Nick Clemens 2018-07-16 11:06:21 UTC
Created attachment 76975 [details] [review]
Bug 17168: (follow-up) Tidy, clean params/options, use date tools
Comment 26 Nick Clemens 2018-07-16 11:06:24 UTC
Created attachment 76976 [details] [review]
Bug 17168: (follow-up) Clean up QA errors

Cleanup variable names
Remove Dumper
Comment 27 Martin Renvoize 2018-07-19 08:05:14 UTC
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);`
Comment 28 Martin Renvoize 2018-07-19 08:09:27 UTC
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)
Comment 29 Martin Renvoize 2018-07-19 08:21:17 UTC
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?
Comment 30 Julian Maurice 2018-10-05 13:42:19 UTC
Changing status to Failed QA to get answers to Martin's comments
Comment 31 Nick Clemens 2019-02-28 13:56:45 UTC
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>
Comment 32 Nick Clemens 2019-02-28 13:56:49 UTC
Created attachment 85871 [details] [review]
Bug 17168: (follow-up) Tidy, clean params/options, use date tools
Comment 33 Nick Clemens 2019-02-28 13:56:53 UTC
Created attachment 85872 [details] [review]
Bug 17168: (follow-up) Clean up QA errors

Cleanup variable names
Remove Dumper
Comment 34 Nick Clemens 2019-02-28 13:56:56 UTC
Created attachment 85873 [details] [review]
Bug 17168: (follow-up) Address QA concerns and update for strict mode
Comment 35 Nick Clemens 2019-02-28 13:58:54 UTC
Created attachment 85878 [details] [review]
Bug 17168: Remove j2a.pl script
Comment 36 Nick Clemens 2019-02-28 14:05:30 UTC
(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
Comment 37 Jonathan Druart 2019-02-28 14:10:58 UTC
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?
Comment 38 Nick Clemens 2019-03-06 19:10:38 UTC
(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
Comment 39 Josef Moravec 2019-04-24 21:28:31 UTC
Just a note:

use Koha::Script -cron;

should be added to cronjob script - see bug 22600

Also, this needs a rebase
Comment 40 Nick Clemens 2019-04-25 19:25:04 UTC
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>
Comment 41 Nick Clemens 2019-04-25 19:25:08 UTC
Created attachment 88810 [details] [review]
Bug 17168: (follow-up) Tidy, clean params/options, use date tools
Comment 42 Nick Clemens 2019-04-25 19:25:12 UTC
Created attachment 88811 [details] [review]
Bug 17168: (follow-up) Clean up QA errors

Cleanup variable names
Remove Dumper
Comment 43 Nick Clemens 2019-04-25 19:25:15 UTC
Created attachment 88812 [details] [review]
Bug 17168: (follow-up) Address QA concerns and update for strict mode
Comment 44 Nick Clemens 2019-04-25 19:25:18 UTC
Created attachment 88813 [details] [review]
Bug 17168: Add Koha:Script -cron, remove Dumper
Comment 45 Nick Clemens 2019-04-25 19:25:22 UTC
Created attachment 88814 [details] [review]
Bug 17168: Remove j2a.pl
Comment 46 Marcel de Rooy 2019-05-10 09:57:39 UTC
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.
Comment 47 Marcel de Rooy 2019-05-10 09:58:56 UTC
(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.
Comment 48 Marcel de Rooy 2019-05-10 10:09:57 UTC
+                      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), },
Comment 49 Marcel de Rooy 2019-05-10 10:14:52 UTC
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 ;)
Comment 50 Marcel de Rooy 2019-05-10 10:21:40 UTC
warn "v $verbose c $doit f $fromcat t $tocat";
exit;

Hmm Debugging leftovers

Just not ready enough to get it further today. Sorry ;)
Comment 51 Nick Clemens 2019-07-19 21:22:57 UTC
Created attachment 91677 [details] [review]
Bug 17168: (follow-up) QA concerns and rebase issues
Comment 52 Martin Renvoize 2019-10-24 13:33:28 UTC
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>
Comment 53 Martin Renvoize 2019-10-24 13:33:32 UTC
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>
Comment 54 Martin Renvoize 2019-10-24 13:33:36 UTC
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>
Comment 55 Martin Renvoize 2019-10-24 13:33:40 UTC
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>
Comment 56 Martin Renvoize 2019-10-24 13:33:46 UTC
Created attachment 94669 [details] [review]
Bug 17168: Add Koha:Script -cron, remove Dumper

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2019-10-24 13:33:51 UTC
Created attachment 94670 [details] [review]
Bug 17168: Remove j2a.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2019-10-24 13:33:55 UTC
Created attachment 94671 [details] [review]
Bug 17168: (follow-up) QA concerns and rebase issues

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Martin Renvoize 2019-10-24 13:35:07 UTC
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.
Comment 60 Martin Renvoize 2019-10-25 12:21:04 UTC
Nice work!

Pushed to master for 19.11.00
Comment 61 Martin Renvoize 2019-10-25 15:13:57 UTC
:(. I missed a lot here... this patchset needs updating for bug 14570
Comment 62 Martin Renvoize 2019-10-25 16:03:49 UTC
Created attachment 94790 [details] [review]
Bug 17168: (RM follow-up) Update for bug 14570
Comment 63 Nick Clemens 2019-10-25 18:22:37 UTC
Created attachment 94793 [details] [review]
Bug 17168: (RM follow-up) Update for bug 14570

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens 2019-10-25 18:22:41 UTC
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>
Comment 65 Martin Renvoize 2019-10-26 06:50:46 UTC
Followups Pushed, thanks guys.