Bug 7157 - Improve the j2a.pl cronjob
Summary: Improve the j2a.pl cronjob
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Assignee: Liz Rea
QA Contact: Marcel de Rooy
URL:
Keywords:
: 3410 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-04 18:26 UTC by Liz Rea
Modified: 2013-05-23 06:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7157 - Improve the j2a.pl cronjob (10.96 KB, patch)
2011-11-04 18:41 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (10.98 KB, patch)
2011-11-07 23:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (3.82 KB, patch)
2011-12-12 23:01 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (11.26 KB, patch)
2011-12-12 23:05 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - followup - error catching for nonexistent categories. (1.19 KB, patch)
2011-12-13 17:32 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (11.38 KB, patch)
2012-01-01 18:02 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (11.45 KB, patch)
2012-01-01 18:08 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (11.47 KB, patch)
2012-01-13 08:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7157 : Follow up, fixing FSF address (1.05 KB, patch)
2012-01-13 08:06 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED] Followup for FSF address (1.15 KB, patch)
2012-01-13 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7157 - Improve the j2a.pl cronjob (11.66 KB, patch)
2012-01-17 16:45 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liz Rea 2011-11-04 18:26:48 UTC
The existing cronjob for transitioning juvenile patron types to adult patron types is limited to the point of uselessness. 

It needs to do the following:

- Calculate update date based on the upper age limit defined in the patron categories.
- Allow libraries to work on all branches or only one.
- Allow libraries to specify which patron category to update child categories to.
- Allow libraries to specify a single Child patron category to update to an adult category.

Nice to have, but not essential:

- Have a test mode to display what transforms would be done on the database without executing the changes.
- email changes completed to the admin email address for the branch upon job execution.
- allow more granular transitions - a "toddler" category to a "child" category, for example - one wouldn't need to change the patron type to A in this case.
- allow specifying multiple from and to categories.
- a way to do this from the UI, working in about the same way.
Comment 1 Liz Rea 2011-11-04 18:41:17 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-11-07 23:36:09 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2011-12-12 13:00:16 UTC
QA Comment: Code looks good. Did not test it, but has a solid signoff. Marked as Passed QA.
Comment 4 Marcel de Rooy 2011-12-12 13:00:55 UTC
(In reply to comment #3)
> QA Comment: Code looks good. Did not test it, but has a solid signoff. Marked
> as Passed QA.
Applies, but with whitespace errors.
Comment 5 Paul Poulain 2011-12-12 14:47:44 UTC
A small problem not noticed by Marcel or Chris: the syntax of the parameters:
http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm

=> 1 character parameters are called with a single -, others are with --

the -man means in fact -m -a -n
the -help means -h -e -l -p

You should have written --help and --man
Your script is working, but:
* a new option/parameter may result in strange results that will be hard to understand
* cronjob scripts are with -- for long options

So, please resubmit your patch with help=s and updated documentation

(great script though !)
Comment 6 Liz Rea 2011-12-12 23:01:44 UTC Comment hidden (obsolete)
Comment 7 Liz Rea 2011-12-12 23:02:52 UTC
Comment on attachment 6723 [details] [review]
Bug 7157 - Improve the j2a.pl cronjob

Gah. Too late in the day and I did it wrong.
Comment 8 Liz Rea 2011-12-12 23:05:22 UTC Comment hidden (obsolete)
Comment 9 Liz Rea 2011-12-13 17:32:06 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2011-12-31 07:19:53 UTC
Sorry liz

The script still has

-branch
-fromcat
-tocat

which should either be
-b
-f
-t

or
--branch
--fromcat
--tocat
Comment 11 Liz Rea 2012-01-01 17:56:31 UTC
Before I submit making them all -b -t -f

Is there a preference for which way they go? No landmines? ;)
Comment 12 Liz Rea 2012-01-01 18:02:28 UTC Comment hidden (obsolete)
Comment 13 Liz Rea 2012-01-01 18:08:26 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2012-01-13 08:05:19 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2012-01-13 08:06:59 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2012-01-13 14:09:34 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2012-01-13 14:20:58 UTC
QA:
On the first patch I see this construct a few times:
      SET categorycode='A',
                    guarantorid ='0',
                    categorycode =?

Could you correct that please? You only need to update the code once ;)
Comment 18 Marcel de Rooy 2012-01-13 14:27:40 UTC
Sorry, but yet another "dumb" question:
Couldn't this be done easier:
AND categorycode IN (select categorycode from categories where category_type='C' and categorycode=?)|;

Don't you mean categorycode=? Do you still need the check on type=C?
Comment 19 Liz Rea 2012-01-17 16:45:50 UTC
Created attachment 7201 [details] [review]
Bug 7157 - Improve the j2a.pl cronjob

- Calculates updates date based on the upper age limit defined in the patron categories.
- Allows libraries to work on all branches or only one.
- Allows libraries to specify which Adult patron category to update child categories to.
- Allows libraries to specify a single Child patron category to update to an adult category.
- Has a test mode to display what transforms would be done on the database without executing the changes.

Includes improved help, copyright statement, and uses warnings. Also incorporates Paul's suggestions regarding --help and --man, changes -fromcat and -tocat to -f and -t, and removes a redundant update to categorycode (per M. deRooy).

To test:

Create two patron categories, a child and an adult category. Make sure they
have an upper age limit.

Create or modify some patrons in multiple branches that fall into the category
of "my birthdate is less than or equal to today's date minus the upper age
limit"

1. Run the script with no flags - nothing should  happen, it will suggest you try the --help flag.
2. Run the script with the --help flag - you should see the help
3. Run the script with the -f=<child category> -t=<adult category> -v -n - should show you results from all branches but take no action and tell you what its computations are.
4. Run the script with the -f=<child category> -t=<adult category> -b=<branchcode> -v -n - should show you results from your specified branch, but take no action and tell you what it's computations are.
5. Run the script with the -f=<child category> -t=<adult category> -v -b=<branchcode> - should show you the computations and tell you how many patrons were modified in your single branch. It will not show you the information on which patrons were updated.
6. Run the script with the -f=<child category> -t=<adult category> -v - should show you the computations and tell you how many patrons were modified across all branches.
7. Run the script without the -v flag, if you care what the non-verbose output looks like.

Fixed in this revision: Known limitation - if you give it an unknown tocat, it will fail with a rather ugly error.

Minor changes to the commit message to reflect new longopts (which I missed the last time)
There is more this script could do, please feel free to take it and run.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Bug 7157 : Follow up, fixing FSF address

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2012-01-19 11:10:56 UTC
QA Comment:
Thank you, Liz, for patiently resubmitting new versions.
I am marking it again as Passed QA. Hopefully, Paul agrees too now ;)
Comment 21 Paul Poulain 2012-01-20 14:22:45 UTC
mmm... the patch changes the permissions of this script and remove the x (executable). Could you confirm it's a mistake, (I can fix it when pushing, no problem)
Comment 22 Liz Rea 2012-01-20 19:38:28 UTC
I am sure that is a mistake. :)
Comment 23 Kyle M Hall 2012-07-26 16:50:00 UTC
*** Bug 3410 has been marked as a duplicate of this bug. ***