Bug 26307 - Changing patron category should check age limits
Summary: Changing patron category should check age limits
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 27554 17168
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-27 12:12 UTC by Fridolin Somers
Modified: 2021-02-22 10:01 UTC (History)
4 users (show)

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


Attachments
Bug 26307: Reveal problem in unit test for too_old (5.09 KB, patch)
2020-08-27 14:05 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26307: Fix misc/cronjobs/update_patrons_category.pl --too_old acts a year too early (2.34 KB, patch)
2020-08-27 14:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26307: Add a unit test for too_young (5.05 KB, patch)
2020-08-27 14:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26307: Use same syntax in --too_young (1.21 KB, patch)
2020-08-27 14:06 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2020-08-27 12:12:48 UTC
misc/cronjobs/update_patrons_category.pl --too_old must change category when the age of the patron is greater than (exclusive) limit age of the category.
Actually it changes a year too early : he goes to a min 18 years category having reached 17 years.

Looks like the test suite is wrong not strong enought on Koha::Patrons->search_patrons_to_update_category().
Comment 1 Fridolin Somers 2020-08-27 12:13:31 UTC
We tested old script j2a.pl and seems this problem already existed.
Comment 2 Fridolin Somers 2020-08-27 14:05:37 UTC
Created attachment 109229 [details] [review]
Bug 26307: Reveal problem in unit test for too_old

Logic :
=======

Looking at Koha::Patron->get_age :
At birthday aniversary, patron has aged 1 year more.
For example :
 - today is 2020-08-27
 - patron with date of birth 2000-08-28 is 19 year old
 - patron with date of birth 2000-08-27 is 20 year old

Looking at Koha::Patron->is_valid_age :
Category min and max limits are inclusive.
For example max=17 means :
 - patron having 17 years old is valid
 - patron having 18 years old is not valid

Test suite :
===========

Changes child3 to have 18 years tomorrow.
Adds a child4 with 18 years today.

Test plan :
Run prove t/db_dependent/Patrons.t
See it fails on 'One under age patron in child category'
Comment 3 Fridolin Somers 2020-08-27 14:06:18 UTC
Created attachment 109230 [details] [review]
Bug 26307: Fix misc/cronjobs/update_patrons_category.pl --too_old acts a year too early

misc/cronjobs/update_patrons_category.pl --too_old must change category when the age of the patron is greater than (exclusive) limit age of the category.
Actually it changes a year too early : he goes to a min 18 years category having reached 17 years.

Patch also changes the compare to 'lower and equal' and uses date
without time
=> so that update occures at birthday aniversary day

See logic in previous patch.
And code in Koha::Patron : get_age() and is_valid_age.

Test plan :
1) Create a child patron category 'C' with upper limit 17 years
2) Create an adult patron category 'A'
3) Create a patron in child category with date of birth = today - 18
   years + 1 day
4) Check patron as age : 17 years 11 months
5) Without patch run :
misc/cronjobs/update_patrons_category.pl -f=ENF -t=ADULT --too_old -v
=> You see the patron
6) apply patch
7) Re-run 5)
=> You dont see the patron
8) Update database to change patron date of birth = today - 18 years
mysql > update borrowers set dateofbirth='YYYY-MM-DD' where borrowernumber=XXX;
9) Run :
misc/cronjobs/update_patrons_category.pl -f=ENF -t=ADULT --too_old -v --confirm
10) Check patron category is now 'A'
11) Check you dont see message "Patron's age is incorrect for their category"
Comment 4 Fridolin Somers 2020-08-27 14:06:32 UTC
Created attachment 109231 [details] [review]
Bug 26307: Add a unit test for too_young

In unit test for update patron categories :

Changes $child1 to bee 4 years 11 month old => is under age
Add $child1bis having exactly the lower age limit of 5 years => is not
under age

Run t/db_dependent/Patrons.t
Test is to renforce the test suite, it is OK event without next patch
Comment 5 Fridolin Somers 2020-08-27 14:06:57 UTC
Created attachment 109232 [details] [review]
Bug 26307: Use same syntax in --too_young

In too_young use dt->strftime("%Y-%m-%d") like in too_old.
This does not change the behavior.

Test plan :
Run prove t/db_dependent/Patrons.t
Comment 6 Nick Clemens 2020-08-27 19:30:08 UTC
I don't think this is a bug, or rather, I don't know

I see your point, if you have categories:
Child Upper Age Limit 17
Adult Minimum Age 18

You don't cross until you are 18, however, you can also read that there is a gap between 17 and 18

The way Koha has always worked you have:
Child Upper Age Limit 18
Adult Minimum Age 18

Now there is no gap and you switch at 18

I think changing a long standing behaviour will affect many people, and that makes me nervous. I know that I have helped libraries set this up in the way that works.

I think instead we need to clarify the explanation of the 'Limit'
Comment 7 Fridolin Somers 2020-08-31 06:35:51 UTC
(In reply to Nick Clemens from comment #6)
> I don't think this is a bug, or rather, I don't know
> 
> I see your point, if you have categories:
> Child Upper Age Limit 17
> Adult Minimum Age 18
> 
> You don't cross until you are 18, however, you can also read that there is a
> gap between 17 and 18
> 
> The way Koha has always worked you have:
> Child Upper Age Limit 18
> Adult Minimum Age 18
> 
> Now there is no gap and you switch at 18
> 
> I think changing a long standing behaviour will affect many people, and that
> makes me nervous. I know that I have helped libraries set this up in the way
> that works.
> 
> I think instead we need to clarify the explanation of the 'Limit'

OK this is the other solution ;)

In your example, a 18 years old patron can be in Child or Adult category without breaking the limits ?

For you limits are inclusive right ?
Comment 8 Nick Clemens 2020-08-31 10:07:56 UTC
(In reply to Fridolin SOMERS from comment #7)
> (In reply to Nick Clemens from comment #6)
> > I don't think this is a bug, or rather, I don't know
> > 
> > I see your point, if you have categories:
> > Child Upper Age Limit 17
> > Adult Minimum Age 18
> > 
> > You don't cross until you are 18, however, you can also read that there is a
> > gap between 17 and 18
> > 
> > The way Koha has always worked you have:
> > Child Upper Age Limit 18
> > Adult Minimum Age 18
> > 
> > Now there is no gap and you switch at 18
> > 
> > I think changing a long standing behaviour will affect many people, and that
> > makes me nervous. I know that I have helped libraries set this up in the way
> > that works.
> > 
> > I think instead we need to clarify the explanation of the 'Limit'
> 
> OK this is the other solution ;)
> 
> In your example, a 18 years old patron can be in Child or Adult category
> without breaking the limits ?
> 
> For you limits are inclusive right ?

I think the lower limit is inclusive, the upper is exclusive:
At least X years old
to 
Less than X years old

However, in testing, it was found that the scripts work on way, and the interface another.

Try to enter a patron into a category from 5-17. At 17 and 1 day they can still be entered, at 18 they are prohibited.
Comment 9 Fridolin Somers 2020-09-10 08:29:42 UTC
My colleges tell me that limits are inclusive and on purpose one must have one year in common between child and adult.

For example :
Child Upper Age Limit 18
Adult Minimum Age 18

So a patron having 18 is OK in both categories.
This makes sens if you manually switch patron category.

If you use update_patrons_category.pl patron will switch from child to adult the day he turns 18.
So he is not really "too old" for the category : he is at the upper limit not over it.

If this is the expected behaviour there is no bug ^^
Comment 10 Magnus Enger 2020-10-05 12:53:38 UTC
After thinking about it for a bit, I think the current behaviour makes sense. If CHILD has an upper limit of 18, it means the patron should change category on the day she turns 18. And ADULT has a lower limit of 18, because the day the patron turns 18 they should be moved to this category.
Comment 11 Fridolin Somers 2020-10-13 07:47:50 UTC
Ok but still I'm going to work on script to avoid changing category if age is not valid in new category.
Comment 12 Fridolin Somers 2021-02-22 10:01:59 UTC
Forget about that too much work.
Since we have Bug 26311 its OK.