Prerequisites for testing:

Several borrower categories defined (Let's call them ADULT, STAFF, CHILD)
Borrowers defined in each category (AA -- ADULT, SS -- STAFF, CC -- CHILD)
Items available for checkout.
Make sure that you are using the default authorised value 2 as "Long Overdue (Lost)"
Apply patch
Make sure that $KOHA_CONF and $PERL5LIB are set correctly.

Test plan:
 1) Check out items to AA, SS and CC. Backdate the check-ins so that the due date more than 90 days prior (2015-05-01 is a practical date, if testing in 2015). Note the borrower number and item number for each checkout.
 2) Run longoverdue.pl in verbose without the --confirm option:

./misc/cronjobs/longoverdue.pl -l 90=2 -v
### TEST MODE -- NO ACTIONS TAKEN ###

Range 1
Due  90 - 366 days ago (2014-08-30 to 2015-06-02), lost => 2
Due 2015-04-01 23:59:00: item     # from borrower    ## to lost: 2
Due 2015-05-01 23:59:00: item     # from borrower    ## to lost: 2
Due 2015-05-01 23:59:00: item     # from borrower    ## to lost: 2

 3) Run longoverdue.pl without --confirm using --category STAFF:

./misc/cronjobs/longoverdue.pl -l 90=2 -v --category STAFF

You should see the item checked out to STAFF set as lost, but the other two items will not be processed.

 4)  Run longoverdue.pl without --confirm using --skip-category STAFF

./misc/cronjobs/longoverdue.pl -l 90=2 -v --skip-category STAFF

You should see the items checked out to ADULT and CHILD set as lost, but STAFF will not be processed.

Comment on attachment 42614 [details] [review]
[SIGNED-OFF] Bug 14292: Add patron category restrictions to longoverdue.pl

Review of attachment 42614 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/longoverdue.pl
@@ +52,5 @@
> +GetOptions(
> +    'lost=s%'         => \$lost,
> +    'c|charge=s'      => \$charge,
> +    'confirm'         => \$confirm,
> +    'verbose'         => \$verbose,

Doc says v|verbose

@@ +56,5 @@
> +    'verbose'         => \$verbose,
> +    'quiet'           => \$quiet,
> +    'maxdays=s'       => \$endrange,
> +    'mark-returned'   => \$mark_returned,
> +    'help'            => \$help,

Doc says h|help

@@ +77,5 @@
> +
> +   longoverdue.pl [ --help | -h | --man ]
> +   longoverdue.pl --lost | -l DAYS=LOST_CODE [ --charge | -c CHARGE_CODE ] [ --verbose | -v ] [ --quiet ]
> +                  [ --maxdays MAX_DAYS ] [ --mark-returned ] [ --category BORROWER_CATEGOERY ] ...
> +                  [ --skip-category BORROWER_CATEGOERY ] ... [ --commit ]

Typo 'CATEGOERY'

@@ +141,5 @@
>  
> +=back
> +
> +All 'category' options will be applied before any 'skip-category' options, meaning that the only categories available to skip
> +are those which have already been specified on the command line.

Is this really useful? I would say --category and --skip-category could be mutually exclusive.
That would simplify the related code.
But it seems to work, so keep it if you think it can be useful.

@@ +188,1 @@
>          die "ERROR: No --lost (-l) option defined";

This won't work, pod2usage will exist before the die statement.
You should use something like:

pod2usage({
    -exitval => 1,
    -msg => q|ERROR: No --lost (-l) option defined|,
});

@@ +224,5 @@
>  }
>  
> +# The following two functions can and should replaced by a call to
> +# Koha::Database.
> +sub borrower_categories_sth {

This can be avoided, we don't need a subroutine to prepare the query :)

@@ +230,5 @@
> +    return C4::Context->dbh->prepare($query);
> +}
> +
> +sub defined_borrower_categories {
> +    my $sth = borrower_categories_sth();

Actually I think both sub could be replace with:
    my $dbh = C4::Context->dbh;
    return map { $_->[0] }
        @{ $dbh->selectall_arrayref(q|SELECT categorycode FROM categories|) };

@@ +273,4 @@
>  #FIXME - Should add a 'system' user and get suitable userenv for it for logging, etc.
>  
>  my $count;
> +# my @ranges = map {

I would simply remove it :)

@@ +294,5 @@
>          $sth_items->execute($startrange, $endrange, $lostvalue);
>          $count=0;
> +        ITEM: while (my $row=$sth_items->fetchrow_hashref) {
> +            if( $filter_borrower_categories ) {
> +                my $category = uc Koha::Database->new()->schema->resultset('Borrower')->find( $row->{borrowernumber} )->get_column('categorycode');

Why don't you use Koha::Borrowers?

(In reply to Barton Chittenden from comment #5)
> Created attachment 42892 [details] [review] [review]
> Bug 14292: QA Follow-up

In this case it is always good to explain what does the follow-up. Either in a comment or, better, in the commit message.
Should I suppose all my comments have been taken into account in it?

(In reply to Jonathan Druart from comment #4)
> Comment on attachment 42614 [details] [review] [review]
> [SIGNED-OFF] Bug 14292: Add patron category restrictions to longoverdue.pl
> 
> Review of attachment 42614 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/cronjobs/longoverdue.pl
> @@ +52,5 @@
> > +GetOptions(
> > +    'lost=s%'         => \$lost,
> > +    'c|charge=s'      => \$charge,
> > +    'confirm'         => \$confirm,
> > +    'verbose'         => \$verbose,
> 
> Doc says v|verbose
> 
> @@ +56,5 @@
> > +    'verbose'         => \$verbose,
> > +    'quiet'           => \$quiet,
> > +    'maxdays=s'       => \$endrange,
> > +    'mark-returned'   => \$mark_returned,
> > +    'help'            => \$help,
> 
> Doc says h|help
> 
> @@ +77,5 @@
> > +
> > +   longoverdue.pl [ --help | -h | --man ]
> > +   longoverdue.pl --lost | -l DAYS=LOST_CODE [ --charge | -c CHARGE_CODE ] [ --verbose | -v ] [ --quiet ]
> > +                  [ --maxdays MAX_DAYS ] [ --mark-returned ] [ --category BORROWER_CATEGOERY ] ...
> > +                  [ --skip-category BORROWER_CATEGOERY ] ... [ --commit ]
> 
> Typo 'CATEGOERY'
> 

Fixed v|verbose and h|help options, as well as 'BORROWER_CATEGOERY' typo.

I left the --category and --skip-category options as-is. I don't have a good reason to keep them, but they're not hurting anything either; we can re-visit if they cause confusion.

Changed 'die' to

> pod2usage({
>     -exitval => 1,
>     -msg => q|ERROR: No --lost (-l) option defined|,
> });


Removed borrower_categories_sth in favor of

    my $dbh = C4::Context->dbh;
    my @categories = map { uc $_->[0] } @{ $dbh->selectall_arrayref(q|SELECT categorycode FROM categories|) };


> @@ +273,4 @@
> >  #FIXME - Should add a 'system' user and get suitable userenv for it for logging, etc.
> >  
> >  my $count;
> > +# my @ranges = map {
> 
> I would simply remove it :)

I wasn't clear about this comment, but I removed the #FIXME comment and the

# my @ranges = map {

statement, both of which seemed useless.

> 
> @@ +294,5 @@
> >          $sth_items->execute($startrange, $endrange, $lostvalue);
> >          $count=0;
> > +        ITEM: while (my $row=$sth_items->fetchrow_hashref) {
> > +            if( $filter_borrower_categories ) {
> > +                my $category = uc Koha::Database->new()->schema->resultset('Borrower')->find( $row->{borrowernumber} )->get_column('categorycode');
> 
> Why don't you use Koha::Borrowers?

Done.

IMO the TODO about the category handling should be addressed in this patch set.
I let the needs QA status if another QAer think it can go as it.

Barton,
I am sorry but I think there was a misunderstanding.
The idea was not to add more complexity but simplify the --category and --skip-category options.
Could you please have a look at the last patch and, if you agree with this change, add your signoff on it?

(In reply to Jonathan Druart from comment #13)
> Barton,
> I am sorry but I think there was a misunderstanding.
> The idea was not to add more complexity but simplify the --category and
> --skip-category options.
> Could you please have a look at the last patch and, if you agree with this
> change, add your signoff on it?

I eyeballed it; it looks good. I'll run a quick test and sign off.

Barton, Ok, waiting for your signoff :)

Made small tweak: --category and --skip-category are now case insensitive. Tested and Signed off.

No it doesn't, the script explodes!

 FAIL   misc/cronjobs/longoverdue.pl
   FAIL   valid
                "my" variable $borrower_category masks earlier declaration in same scope 
                "my" variable $skip_borrower_category masks earlier declaration in same scope

(In reply to Jonathan Druart from comment #18)
> No it doesn't, the script explodes!
> 
>  FAIL   misc/cronjobs/longoverdue.pl
>    FAIL   valid
>                 "my" variable $borrower_category masks earlier declaration
> in same scope 
>                 "my" variable $skip_borrower_category masks earlier
> declaration in same scope

I have submitted a follow-up, marked as passed QA.
(Have a look at the QA script :))

Created attachment 45293 [details] [review]
Bug 14292: Add patron category restrictions to longoverdue.pl

Squashed prevoius patch set.

Add command line options --category and --skip-category

Convert usage() to pod2usage. (This was previous FIXME)

Make borrower category handling more user friendly:

    Make --category and --skip-category mutally exclusive

    Check that categories specified by --category
    and --skip-category exist in categories table.

    Add --list-categories opttion to show available
    borrower categories.

http://bugs.koha-community.org/show_bug.cgi?id=14292

QA Follow-up

QA fixes - wording and trailing whitespaces

Simplify the category list

Basically we have 3 lists of categories:
1/ The full one, in the DB
2/ The one we want and list with --category
3/ The one we don't want and list with --skip-category

The list of categories to process is:
 * 1 if none --category or --skip-category options are provided
 * 2 if --category is provided
 * 1 - 3 if --skip-category is provided

(follow-up) Make --category and --skip-category case insensitive

"my" variable $borrower_category masks earlier declaration in same scope
"my" variable $skip_borrower_category masks earlier declaration in same scope

Signed-off-by: hbraum@nekls.org

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 45296 [details] [review]
Bug 14292: [Alternate for 3.18] Add patron category restrictions to longoverdue.pl

This is the same patch set, rebased on 3.18.x

Add command line options --category and --skip-category

Convert usage() to pod2usage. (This was previous FIXME)

Make borrower category handling more user friendly:

    Make --category and --skip-category mutally exclusive

    Check that categories specified by --category
    and --skip-category exist in categories table.

    Add --list-categories opttion to show available
    borrower categories.

http://bugs.koha-community.org/show_bug.cgi?id=14292

QA Follow-up

QA fixes - wording and trailing whitespaces

Simplify the category list

Basically we have 3 lists of categories:
1/ The full one, in the DB
2/ The one we want and list with --category
3/ The one we don't want and list with --skip-category

The list of categories to process is:
 * 1 if none --category or --skip-category options are provided
 * 2 if --category is provided
 * 1 - 3 if --skip-category is provided

(follow-up) Make --category and --skip-category case insensitive

"my" variable $borrower_category masks earlier declaration in same scope
"my" variable $skip_borrower_category masks earlier declaration in same scope

Signed-off-by: hbraum@nekls.org

Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master, thanks Barton!