|
Description
Fridolin Somers
2020-08-27 15:20:21 UTC
Created attachment 109239 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Created attachment 110106 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 110130 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110131 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #4) > Created attachment 110131 [details] [review] [review] > Bug 26311: (QA follow-up) Rephrase output message > > This is a suggestion for rephrasing the message slightly: > > Before: > * Patron borrowernumber=15 in category 'J' has invalid age '56' > > After: > * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' > > Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Yep better thanks I am wondering if we could not improve this a bit. 1. We could retrieve patrons with a dob 2. We could retrieve patrons from categories with dateofbirthrequired Looping on all patrons does not seem very efficient. What do you think? (In reply to Jonathan Druart from comment #6) I like calling $patron->is_valid_age so its the same behaviour as staff interface. > Looping on all patrons does not seem very efficient. I agree but actually script is looping on all biblio records, which already can be very long. But I'm open to changes ;) I think having a dob makes sense, because it's won't work otherwise, but I would not rely on dob required. (In reply to Jonathan Druart from comment #6) > I am wondering if we could not improve this a bit. > 1. We could retrieve patrons with a dob > 2. We could retrieve patrons from categories with dateofbirthrequired > > Looping on all patrons does not seem very efficient. > > What do you think? I agree about improving efficiency. Why not loop through the patron categories that have age limits, and then only retrieve the patrons for those categories? Depending on how inconsistencies want to be reported, you could then note patrons with null dateofbirth, or only select patrons where their dateofbirth is outside the allowed scope. Much better than looping through all patrons I think. Created attachment 110978 [details] [review] Bug 26311: Loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Created attachment 110979 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Good points. Here is a follow-up that limits on patrons : - having a Date Of Birth - in category having an age limit Patrons search is quite complex so tell me if its the correct syntax. Created attachment 110980 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit Oh I changed 'thier category' to 'his category'. I understand now its wrong = gender neutral pronoun I repush Created attachment 110998 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit No problem pllying the patchs, but I don't have any alert in the script. Maybe the problem comes from my side, I prefer not updating the status and let someone else try. Created attachment 128235 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 128236 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 128237 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit Testing notes and issues identified (using koha-testing-docker):
1. Database query for step 3: update borrowers set dateofbirth = '2004-11-01' where borrowernumber = 49;
2. Main patch test plan worked as expected.
3. Had issues with some of the tests in the third patch (see details below), others worked as expected:
. with no date of birth: doesn't seem to work if age is NULL
. with invalid age in category having age required: 'Use of uninitialized value in sprintf...' message
. with invalid age in category having upper age limit: 'Use of uninitialized value in sprintf...' message
with no date of birth
~~~~~~~~~~~~~~~~~~~~~
1. Edited a staff patron (Henry - 34) and removed date of birth
2. Changed the staff patron category (S) so age required = 18-99 years
3. Error message after running (note: dateofbirth is NULL in database):
=> Patron borrowernumber=34 has an invalid age of 104 for their category 'S' (18-99)
=> This looks wrong to me - with no date of birth I'm not sure how it works out they 104 years of age
4. If I update in the database so dateofbirth = '' (which becomes 0000-00-00) I get this message:
==> Can't call method "strftime" on an undefined value at /kohadevbox/koha/Koha/Patron.pm line 1149.
with invalid age in category having age required
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. Edited a patron (Mary Burton - 49) so that age is less than 24.
2. Changed patron category (PT) so age required = 24 and no maximum age.
3. Worked as expected except for 'Use of unitialized value...' message:
==> Use of uninitialized value in sprintf at misc/maintenance/search_for_data_inconsistencies.pl line 256.
* Patron borrowernumber=49 has an invalid age of 10 for their category 'PT' (24-)
4. If they have the correct age (changed so they were older than 24)
with invalid age in category having upper age limit
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. Edited a patron (Mary Burton - 49) so that age is more than 50.
2. Changed patron category (PT) so that no age required and upper age limit was 50.
3. Output as expected except for 'Use of uninitialized value in sprintf...' message:
==> Use of uninitialized value in sprintf at misc/maintenance/search_for_data_inconsistencies.pl line 256.
* Patron borrowernumber=49 has an invalid age of 71 for their category 'PT' (-50)
with invalid age in category having age required and upper age limit
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. Edited a patron (Mary Burton - 49) so that age is more than 60.
2. Changed patron category (PT) so that age required = 20 and upper age limit = 60.
3. Output as expected:
==> * Patron borrowernumber=44 has an invalid age of 70 for their category 'PT' (20-60)
Created attachment 131295 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 131296 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 131297 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit (In reply to David Nind from comment #20) > Testing notes and issues identified (using koha-testing-docker): Thanks a lot for testing David. I've found an error in patrons select. Fixed with prefetch of patron category. You may test again Created attachment 132978 [details] [review] Bug 30063: Fix missing TT filters Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Apologies that it has taken me so long to retest. Summary from testing: - test plan for first patch: worked as expected - with no date of birth: does not appear in the list, no warnings on the patron page in staff interface [1] - with invalid age in category having age required: unitilized value message if upper age limit blank [2] - with invalid age in category having upper age limit: unitilized value message if upper age limit blank [3] - with invalid age in category having age required and upper age limit: works as expected Hopefully I haven't misinterpreted what is expected here! [1] with no date of birth ~~~~~~~~~~~~~~~~~~~~~~~~~ 1. Edited a staff patron (Henry: patron id = 19) and removed date of birth using the staff interface (dateofbirth shows as NULL in database) 2. Changed the staff patron category (S) so age required = 18-99 years 3. Does not appear in the list after running the script [2] with invalid age in category having age required ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1. Edited a patron (Mary Burton - borrowernumber = 49) so that age is less than 24. 2. Changed patron category (PT) so age required = 24 and no maximum age (left blank). 3. Worked as expected except for 'Use of unitialized value...' message: ==> Use of uninitialized value in sprintf at misc/maintenance/search_for_data_inconsistencies.pl line 304. * Patron borrowernumber=49 has an invalid age of 21 for their category 'PT' (24-) 4. If they have the correct age (changed so they were older than 24), then no unitialized value message [3] with invalid age in category having upper age limit ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1. Edited a patron (Mary Burton - 49) so that age is more than 50. 2. Changed patron category (PT) so that no age required and upper age limit was 50. 3. Output as expected, except for 'Use of uninitialized value in sprintf...' message: ==> Use of uninitialized value in sprintf at misc/maintenance/search_for_data_inconsistencies.pl line 304. * Patron borrowernumber=49 has an invalid age of 58 for their category 'PT' (-50) Created attachment 136665 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 136666 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 136667 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit Created attachment 136668 [details] [review] Bug 26311: (follow-up) Fix undefined values in sprintf Avoid warns 'Use of uninitialized value in sprintf' by using '0' if lower age is undefined 'unlimited' if upper are is undefined Warns uninitialized value in sprintf removed. Thanks again for testing David :D (In reply to Fridolin Somers from comment #31) > Warns uninitialized value in sprintf removed. Thanks Fridolin - the warnings are now removed and things work as expected. The only thing that's not working for me is when the date of birth is empty. Is this supposed to show in the list of data inconsistencies? For example, if a patron has no date of birth but is in the patron category with an age range from 18-99, then I would expect it to show - as there is no way to know how old they are. (In reply to David Nind from comment #32) > (In reply to Fridolin Somers from comment #31) > > Warns uninitialized value in sprintf removed. > > Thanks Fridolin - the warnings are now removed and things work as expected. > > The only thing that's not working for me is when the date of birth is empty. > > Is this supposed to show in the list of data inconsistencies? For example, > if a patron has no date of birth but is in the patron category with an age > range from 18-99, then I would expect it to show - as there is no way to > know how old they are. Hi We decided in comment 6 to skip patron with no date of birth. So indeed they do not show in list of data inconsistencies. Some patron can be a virtual person (like a school) so no date of birth. Created attachment 136750 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Created attachment 136751 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Created attachment 136752 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit Signed-off-by: David Nind <david@davidnind.com> Created attachment 136753 [details] [review] Bug 26311: (follow-up) Fix undefined values in sprintf Avoid warns 'Use of uninitialized value in sprintf' by using '0' if lower age is undefined 'unlimited' if upper are is undefined Signed-off-by: David Nind <david@davidnind.com> (In reply to Fridolin Somers from comment #33) > We decided in comment 6 to skip patron with no date of birth. > So indeed they do not show in list of data inconsistencies. > Some patron can be a virtual person (like a school) so no date of birth. I didn't see that, and that makes sense. Now signed-off! Created attachment 137039 [details] [review] Bug 26311: Add patron invalid age to search_for_data_inconsistencies.pl Patron categories may have age limits. Add to script misc/maintenance/search_for_data_inconsistencies.pl the list of patrons which age is invalid regarding there category. Test plan : 1) Create an adult patron category limited to 18-99 years 2) Create a patron in the category 3) Edit in database its date of birth so that he is 17 years old 4) Run misc/maintenance/search_for_data_inconsistencies.pl => You see the patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 137040 [details] [review] Bug 26311: (QA follow-up) Rephrase output message This is a suggestion for rephrasing the message slightly: Before: * Patron borrowernumber=15 in category 'J' has invalid age '56' After: * Patron borrowernumber=37 has an invalid age of 37 for their category 'K' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 137041 [details] [review] Bug 26311: (follow-up) loop through the patron with date of birth and in categories having age limits Also added category limits in message, for example (12-18) Check patron : - with no date of birth - with invalid age in category having age required - with invalid age in category having upper age limit - with invalid age in category having age required and upper age limit Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 137042 [details] [review] Bug 26311: (follow-up) Fix undefined values in sprintf Avoid warns 'Use of uninitialized value in sprintf' by using '0' if lower age is undefined 'unlimited' if upper are is undefined Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 137043 [details] [review] Bug 26311: (QA follow-up) Update script documentation Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 22.11. Nice work everyone, thanks! Enhancement will not be backported to 22.05.x series |