Bug 20436 - Add ability to specify itemtypes for longoverdue.pl
Summary: Add ability to specify itemtypes for longoverdue.pl
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords: additional_work_needed
Depends on:
Blocks:
 
Reported: 2018-03-16 11:40 UTC by Kyle M Hall
Modified: 2019-05-22 14:44 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20436 - Add ability to specify itemtypes for longoverdue.pl (6.02 KB, patch)
2018-03-16 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Works like a charm! Thx Kyle (6.07 KB, patch)
2018-10-22 15:19 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 20436 - Add ability to specify itemtypes for longoverdue.pl (6.09 KB, patch)
2018-10-23 13:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20436: Add ability to specify itemtypes for longoverdue.pl (6.12 KB, patch)
2019-03-25 16:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20436: (QA follow up) - reference and typo fixes (1.28 KB, patch)
2019-03-25 16:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20436: Add ability to specify itemtypes for longoverdue.pl (6.22 KB, patch)
2019-05-03 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20436: (QA follow up) - reference and typo fixes (1.38 KB, patch)
2019-05-03 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20436: (RM follow-up) Use Koha objects instead of mysql (2.32 KB, patch)
2019-05-22 14:44 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 Kyle M Hall 2018-03-16 11:40:04 UTC
It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.
Comment 1 Kyle M Hall 2018-03-16 11:44:13 UTC
Created attachment 73026 [details] [review]
Bug 20436 - Add ability to specify itemtypes for longoverdue.pl

It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.

Test Plan:
1) Set up a number of overdues of various itemtypes
2) Run longoverdue.pl with --list-itemtypes to show itemtypes
3) Run longoverdue.pl with --itemtype to verify it only operates on that itemtype
4) Run longoverdue.pl with multiple --itemtype switches, verify it only operates on those itemtypes
5) Run longoverdue.pl with --skip-itemtype to verify it does not operate on that itemtype
6) Run longoverdue.pl with multiple --skip-itemtype switches, verify that it does not operate on those itemtypes
Comment 2 Jesse Maseto 2018-10-22 15:19:19 UTC
Created attachment 80984 [details] [review]
Works like a charm! Thx Kyle

Bug 20436 - Add ability to specify itemtypes for longoverdue.pl

It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.

Test Plan:
1) Set up a number of overdues of various itemtypes
2) Run longoverdue.pl with --list-itemtypes to show itemtypes
3) Run longoverdue.pl with --itemtype to verify it only operates on that itemtype
4) Run longoverdue.pl with multiple --itemtype switches, verify it only operates on those itemtypes
5) Run longoverdue.pl with --skip-itemtype to verify it does not operate on that itemtype
6) Run longoverdue.pl with multiple --skip-itemtype switches, verify that it does not operate on those itemtypes

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 3 Kyle M Hall 2018-10-23 13:23:43 UTC
Created attachment 81024 [details] [review]
Bug 20436 - Add ability to specify itemtypes for longoverdue.pl

It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.

Test Plan:
1) Set up a number of overdues of various itemtypes
2) Run longoverdue.pl with --list-itemtypes to show itemtypes
3) Run longoverdue.pl with --itemtype to verify it only operates on that itemtype
4) Run longoverdue.pl with multiple --itemtype switches, verify it only operates on those itemtypes
5) Run longoverdue.pl with --skip-itemtype to verify it does not operate on that itemtype
6) Run longoverdue.pl with multiple --skip-itemtype switches, verify that it does not operate on those itemtypes

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 4 Marcel de Rooy 2018-11-02 09:22:04 UTC
* Commit title does not start with 'Bug XXXXX: ' - 62f2b5d

QA: Looking here
Comment 5 Marcel de Rooy 2018-11-02 09:25:43 UTC
+    'l|lost=s%'       => \$lost,

     'category=s'      => $borrower_category,
     'skip-category=s' => $skip_borrower_category,
     'list-categories' => \$list_categories,
+    'itemtype=s'      => $itemtype,
+    'skip-itemtype=s' => $skip_itemtype,
+    'list-itemtypes'  => \$list_itemtypes,

This is a confusing mix of handling options.
I think we should always pass a reference like name => \$option,
but as you can see we already deviate from that here.
But you are adding new cases.
Comment 6 Marcel de Rooy 2018-11-02 09:30:20 UTC
Typo "The options --itemtype and --skip-itemtype are mually exclusive.\n"

But I am not a native speaker, so I might be wrong?
Comment 7 Marcel de Rooy 2018-11-02 09:32:30 UTC
+=item B<--list-itemtypes>
+
+List itemtypes available for use by B<--itemtype> or
+B<--skip-itemtype>, and exit.

I am not a fan of such options. There are better ways to list them. One is already in and I would rather remove it.
Comment 8 Marcel de Rooy 2018-11-02 09:33:14 UTC
Changing status. Too much noise for me now
Comment 9 Liz Rea 2019-03-25 15:56:57 UTC
Just noting that this still applies and only needs attention to QA concerns.
Comment 10 Liz Rea 2019-03-25 16:11:40 UTC
Created attachment 86983 [details] [review]
Bug 20436: Add ability to specify itemtypes for longoverdue.pl

It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.

Test Plan:
1) Set up a number of overdues of various itemtypes
2) Run longoverdue.pl with --list-itemtypes to show itemtypes
3) Run longoverdue.pl with --itemtype to verify it only operates on that itemtype
4) Run longoverdue.pl with multiple --itemtype switches, verify it only operates on those itemtypes
5) Run longoverdue.pl with --skip-itemtype to verify it does not operate on that itemtype
6) Run longoverdue.pl with multiple --skip-itemtype switches, verify that it does not operate on those itemtypes

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 11 Liz Rea 2019-03-25 16:11:43 UTC
Created attachment 86984 [details] [review]
Bug 20436: (QA follow up) - reference and typo fixes
Comment 12 Liz Rea 2019-03-25 16:13:45 UTC
Fixed commit headlines on original patch, and addressed the reference and typo concerns. I leave the last one 

"+=item B<--list-itemtypes>
+
+List itemtypes available for use by B<--itemtype> or
+B<--skip-itemtype>, and exit.

I am not a fan of such options. There are better ways to list them. One is already in and I would rather remove it."

for Kyle.

Liz
Comment 13 Marcel de Rooy 2019-05-03 10:09:30 UTC
Created attachment 89304 [details] [review]
Bug 20436: Add ability to specify itemtypes for longoverdue.pl

It would be nice if we could filter long overdues based on itemtype the same way we can already filter based on borrower category code.

Test Plan:
1) Set up a number of overdues of various itemtypes
2) Run longoverdue.pl with --list-itemtypes to show itemtypes
3) Run longoverdue.pl with --itemtype to verify it only operates on that itemtype
4) Run longoverdue.pl with multiple --itemtype switches, verify it only operates on those itemtypes
5) Run longoverdue.pl with --skip-itemtype to verify it does not operate on that itemtype
6) Run longoverdue.pl with multiple --skip-itemtype switches, verify that it does not operate on those itemtypes

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2019-05-03 10:09:35 UTC
Created attachment 89305 [details] [review]
Bug 20436: (QA follow up) - reference and typo fixes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2019-05-03 10:10:25 UTC
Passing QA but frowning:

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

+my @available_itemtypes = map { uc $_->[0] } @{ $dbh->selectall_arrayref(q|SELECT itemtype FROM itemtypes|) };

Old school SQL..

The earlier mention of --list-itemtypes is no blocker for me.
Leaving final decision to RM ;)
Comment 16 Nick Clemens 2019-05-10 19:14:09 UTC
(In reply to Marcel de Rooy from comment #15)

This one can be done in a follow-up please Kyle? Pushing for now

> Passing QA but frowning:
> 
> +    my @itemtypes = sort map { uc $_->[0] } @{
> C4::Context->dbh->selectall_arrayref(q|SELECT itemtype FROM itemtypes|) };
> 
> +my @available_itemtypes = map { uc $_->[0] } @{
> $dbh->selectall_arrayref(q|SELECT itemtype FROM itemtypes|) };
> 
> Old school SQL..
> 

This seems okay to me, we should make a rule somewhere if we like this or not

> The earlier mention of --list-itemtypes is no blocker for me.
> Leaving final decision to RM ;)

Awesome work all!

Pushed to master for 19.05
Comment 17 Nick Clemens 2019-05-22 14:44:17 UTC
Created attachment 89962 [details] [review]
Bug 20436: (RM follow-up) Use Koha objects instead of mysql