Bug 14292 - Add --category and --skip-category options to longoverdue.pl to include or exclude borrower categories.
Summary: Add --category and --skip-category options to longoverdue.pl to include or ex...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Barton Chittenden
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-28 21:42 UTC by Barton Chittenden
Modified: 2017-06-14 22:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Add patron category restrictions to longoverdue.pl (10.94 KB, patch)
2015-08-31 15:53 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14292: Add patron category restrictions to longoverdue.pl (11.00 KB, patch)
2015-09-16 15:22 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: QA Follow-up (4.42 KB, patch)
2015-09-25 20:10 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: Add patron category restrictions to longoverdue.pl (10.99 KB, patch)
2015-10-09 18:14 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: QA Follow-up (8.47 KB, patch)
2015-10-09 18:15 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: QA fixes - wording and trailing whitespaces (2.84 KB, patch)
2015-10-12 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: Simplify the category list (3.86 KB, patch)
2015-10-12 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 14292: Make --category and --skip-category case insensitive (959 bytes, patch)
2015-10-15 14:12 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: Add patron category restrictions to longoverdue.pl (11.12 KB, patch)
2015-10-16 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: QA Follow-up (8.60 KB, patch)
2015-10-16 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: QA fixes - wording and trailing whitespaces (2.97 KB, patch)
2015-10-16 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: Simplify the category list (4.00 KB, patch)
2015-10-16 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 14292: Make --category and --skip-category case insensitive (1.06 KB, patch)
2015-10-16 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: (follow-up) Make --category and --skip-category case insensitive (1.32 KB, patch)
2015-10-16 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14292: Add patron category restrictions to longoverdue.pl (12.18 KB, patch)
2015-12-01 21:19 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14292: [Alternate for 3.18] Add patron category restrictions to longoverdue.pl (12.22 KB, patch)
2015-12-01 21:41 UTC, Barton Chittenden
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2015-05-28 21:42:09 UTC
Intention of development:
To be able to mark items as long overdue if the patron is not of a given patron category

Plan of attack:
1) Add new option --category to list only patron categories to act upon
2) Add new option --skip-category to list any patron categories to *not* act upon
Comment 1 Barton Chittenden 2015-08-31 15:53:57 UTC Comment hidden (obsolete)
Comment 2 Barton Chittenden 2015-08-31 16:18:38 UTC
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 3 Barton Chittenden 2015-09-16 15:22:28 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-09-24 12:10:03 UTC
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?
Comment 5 Barton Chittenden 2015-09-25 20:10:12 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-09-28 07:23:59 UTC
(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?
Comment 7 Barton Chittenden 2015-09-28 12:10:51 UTC
(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.
Comment 8 Jonathan Druart 2015-10-06 11:03:20 UTC
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.
Comment 9 Barton Chittenden 2015-10-09 18:14:42 UTC Comment hidden (obsolete)
Comment 10 Barton Chittenden 2015-10-09 18:15:13 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-10-12 10:40:56 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-10-12 10:41:01 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-10-12 10:42:24 UTC
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?
Comment 14 Barton Chittenden 2015-10-14 12:10:59 UTC
(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.
Comment 15 Jonathan Druart 2015-10-15 09:53:41 UTC
Barton, Ok, waiting for your signoff :)
Comment 16 Barton Chittenden 2015-10-15 14:12:22 UTC Comment hidden (obsolete)
Comment 17 Barton Chittenden 2015-10-15 14:14:05 UTC
Made small tweak: --category and --skip-category are now case insensitive. Tested and Signed off.
Comment 18 Jonathan Druart 2015-10-16 10:39:28 UTC
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
Comment 19 Jonathan Druart 2015-10-16 10:43:45 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-10-16 10:43:51 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-10-16 10:43:55 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-10-16 10:43:59 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-10-16 10:44:03 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-10-16 10:44:07 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-10-16 10:44:55 UTC
(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 :))
Comment 26 Barton Chittenden 2015-12-01 21:19:53 UTC
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>
Comment 27 Barton Chittenden 2015-12-01 21:41:46 UTC
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>
Comment 28 Kyle M Hall 2015-12-31 14:57:17 UTC
Pushed to master, thanks Barton!