Bug 15206

Hi Alex, I like thsi feature.

I played around with a 10 year old kid.

Today we have 18/11/2015

This are my results: 
18/11/2005 -> Age: 10 year(s) 2 day(s) 
18/11/2005 -> Age: 10 year(s) 1 day(s) 
20/11/2005 -> Age: 10 year(s)
21/11/2005 -> Age: 9 year(s) 11 month(s) 30 day(s)
22/11/2005 -> Age: 9 year(s) 11 month(s) 29 day(s)

There seems to be an offset of some days.

(In reply to Marc Véron from comment #2)
> Hi Alex, I like thsi feature.
> 
> I played around with a 10 year old kid.
> 
> Today we have 18/11/2015
> 
> This are my results: 
> 18/11/2005 -> Age: 10 year(s) 2 day(s) 
> 18/11/2005 -> Age: 10 year(s) 1 day(s) 
> 20/11/2005 -> Age: 10 year(s)
> 21/11/2005 -> Age: 9 year(s) 11 month(s) 30 day(s)
> 22/11/2005 -> Age: 9 year(s) 11 month(s) 29 day(s)
> 
> There seems to be an offset of some days.

Hello Marc,

Tanks for testing. Indeed there's a problem and i understand why. Patch is coming soon...

I removed the number of days because it's quite imprecise since months haven't the same number of days. If anyone can do better or knows a javascript library that can do it with more accurate feel free to tell me.

Code review:

You have:

    function CalculateAge(dateofbirth) {

Then, you don't use "dateofbirth".

You have:

  <input type="text" id="dateofbirth" name="dateofbirth" size="20"
   onchange="CalculateAge(document.form.dateofbirth);"
   value="[% UNLESS opduplicate %][% dateofbirth %][% END %]" />

You rather should use jQuery:

  $( "#dateofbirth" ).change(CalculateAge);

From an UI point of view, it seems awkward to have displayed firstly
"DD/MM/YYYY" under the Date of birth text box, and then having this info
replaced by the age. This way, it's not possible to see the age without
modifying the date of birth. Wouldn't it be better/simpler to display the age
on the detail page (moremember.pl), rather than on the editing page
(memberentry.pl)?

Have you seen that there is already a Perl function calculating age in Koha?
C4::Members::GetAge(). There is a risk that your javascript code and Perl
GetAge() produce a different age.

(In reply to Frédéric Demians from comment #6)
> Code review:
> 
> You have:
> 
>     function CalculateAge(dateofbirth) {
> 
> Then, you don't use "dateofbirth".
> 
> You have:
> 
>   <input type="text" id="dateofbirth" name="dateofbirth" size="20"
>    onchange="CalculateAge(document.form.dateofbirth);"
>    value="[% UNLESS opduplicate %][% dateofbirth %][% END %]" />
> 
> You rather should use jQuery:
> 
>   $( "#dateofbirth" ).change(CalculateAge);
You are right. I forgot to remove the function parameter. Using "$("#dateofbirth").datepicker( 'getDate' )" is bettre because it gives me the correct date format
> 
> From an UI point of view, it seems awkward to have displayed firstly
> "DD/MM/YYYY" under the Date of birth text box, and then having this info
> replaced by the age. This way, it's not possible to see the age without
> modifying the date of birth. Wouldn't it be better/simpler to display the age
> on the detail page (moremember.pl), rather than on the editing page
> (memberentry.pl)?
The main goal is to have directly the age under the datepiker while typing the date of birth. This allows librarian, for example, to change the category consequently
> 
> Have you seen that there is already a Perl function calculating age in Koha?
> C4::Members::GetAge(). There is a risk that your javascript code and Perl
> GetAge() produce a different age.
So, i should make an Ajax call to a perl script to use this existing sub. But why not

> The main goal is to have directly the age under the datepiker while typing
> the date of birth. This allows librarian, for example, to change the
> category consequently

OK. Interesting for librarian, indeed. Would be great to have it also on
borrower detail page.

> > Have you seen that there is already a Perl function calculating age in Koha?
> > C4::Members::GetAge(). There is a risk that your javascript code and Perl
> > GetAge() produce a different age.

> So, i should make an Ajax call to a perl script to use this existing sub.
> But why not

With Koha brand new REST API... Or you can alternatively do it JS but adding
some tests to guaranty consistency.

I even see in my crystal ball GetAge() modified to delegate to MySQL the age
calculation for stability reason. For example:

  SELECT TIMESTAMPDIFF(YEAR,'2010-09-10',CURDATE())

Test plan:

Create a new patron (members/memberentry.pl), enter a date of birth, patron'a age should be shown under

Got to a patron's detail page and check his age near date of birth.

Comment on attachment 45581 [details] [review]
Bug 15206 - Show patron's age in moremenber page near date of birth

Review of attachment 45581 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
@@ +225,4 @@
>          [% IF ( emailpro ) %]<li><span class="label">Secondary email: </span><a href="mailto:[% emailpro %]">[% emailpro %]</a></li>[% END %]
>      [% END %]
>      [% IF ( initials ) %]<li><span class="label">Initials: </span>[% initials %]</li>[% END %]
> +    [% IF ( dateofbirth ) %]<li><span class="label">Date of birth:</span>[% dateofbirth | $KohaDates %] ([% age  %])</li>[% END %]

I don't know how to have something translatable here.

::: members/moremember.pl
@@ +140,5 @@
>  $data->{'IS_ADULT'} = ( $data->{'categorycode'} ne 'I' );
>  
> +my @age;
> +my ($years, $months) = GetAge($data->{dateofbirth});
> +push @age, "$years year(s)" if $years;

This line, and the next one, will not be translatable.

Comment on attachment 45580 [details] [review]
Bug 15206 - CalculateAge in memberentry use C4::Members::GetAge()

Review of attachment 45580 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +163,5 @@
> +            data: 'dateofbirth=' + year + '-' + month + '-' + day,
> +            dataType: 'json',
> +            success : function(json, statut){
> +                var age_string = "Age: ";
> +                if (json.years) { age_string += json.years + " year(s) "; }

Translation issue here also. 

http://wiki.koha-community.org/wiki/Coding_Guidelines#JavaScript

Otherwise, jQuery ajax call seem great :-)

When I try setting a patron's birth date in the patron edit form I get a JavaScript error:

"NetworkError: 404 Not Found - /cgi-bin/koha/svc/members/age?dateofbirth=2015-11-9"

I also notice that the date I enter isn't the date which is passed in the AJAX call. With MM/DD/YYYY as my system's dateformat preference, I've tried these:

Typed 01/03/1990, date passed: 1990-0-3
Typed 12/13/1970, date passed: 1970-11-13
Typed 06/09/2003, date passed: 2003-5-9

(In reply to Owen Leonard from comment #16)
> When I try setting a patron's birth date in the patron edit form I get a
> JavaScript error:
> 
> "NetworkError: 404 Not Found -
> /cgi-bin/koha/svc/members/age?dateofbirth=2015-11-9"
Oops. Sorry, i forgot to commit the script svc/members/age
> 
> I also notice that the date I enter isn't the date which is passed in the
> AJAX call. With MM/DD/YYYY as my system's dateformat preference, I've tried
> these:
> 
> Typed 01/03/1990, date passed: 1990-0-3
> Typed 12/13/1970, date passed: 1970-11-13
> Typed 06/09/2003, date passed: 2003-5-9

It's not a pbm. Months start from 0 to 11. But the result is the same.

(In reply to Alex Arnaud from comment #17)
> (In reply to Owen Leonard from comment #16)
> > When I try setting a patron's birth date in the patron edit form I get a
> > JavaScript error:
> > 
> > "NetworkError: 404 Not Found -
> > /cgi-bin/koha/svc/members/age?dateofbirth=2015-11-9"
> Oops. Sorry, i forgot to commit the script svc/members/age
> > 
> > I also notice that the date I enter isn't the date which is passed in the
> > AJAX call. With MM/DD/YYYY as my system's dateformat preference, I've tried
> > these:
> > 
> > Typed 01/03/1990, date passed: 1990-0-3
> > Typed 12/13/1970, date passed: 1970-11-13
> > Typed 06/09/2003, date passed: 2003-5-9
> 
> It's not a pbm. Months start from 0 to 11. But the result is the same.

In fact you are right. Since C4::Members::GetAge() use months from 1 to 12 we have an intervall...

Fix is coming

Whaou, an ajax request looks like a bit overkill imo...
Anyway, now it's done.

Quick QA comments:
1/ catch error when retrieving the answer of the ajax request (empty .hint)
2/ CalculateAge should only be done if CheckDate return success (otherwise empty .hint)
3/ Use string_var.format(number) in JS code to display the string
4/ Why do you use gettext? pass age_month and age_year to the template and let it display/translate the string accordingly
5/ I think "year(s)" could be replaced with "year" :) And manage the plural for month/months should not be too difficult.

And if you could attach here a single patch rather than a sequence of patches...

(In reply to Jonathan Druart from comment #29)
> Whaou, an ajax request looks like a bit overkill imo...
> Anyway, now it's done.
> 
> Quick QA comments:
> 1/ catch error when retrieving the answer of the ajax request (empty .hint)
> 2/ CalculateAge should only be done if CheckDate return success (otherwise
> empty .hint)
right

> 3/ Use string_var.format(number) in JS code to display the string
can you be more explicit ?

> 4/ Why do you use gettext? pass age_month and age_year to the template and
> let it display/translate the string accordingly
I think it's better than use some [% if %]/[% else %] in templates that make them less readable imo. And now we have (thx Julian Maurice) Koha::I18N. Why don't use it ?

> 5/ I think "year(s)" could be replaced with "year" :) And manage the plural
> for month/months should not be too difficult.
?

(In reply to Alex Arnaud from comment #31)
> > 3/ Use string_var.format(number) in JS code to display the string
> can you be more explicit ?

  git grep '\.format('
will give you some examples.

> > 4/ Why do you use gettext? pass age_month and age_year to the template and
> > let it display/translate the string accordingly
> I think it's better than use some [% if %]/[% else %] in templates that make
> them less readable imo. And now we have (thx Julian Maurice) Koha::I18N. Why
> don't use it ?

You will have to test the plurial/existence of the vars in the pl, it's the same :)
There is absolutely no need to use Koha::I18N here. Pass the values to the template and let it displays the strings as it wants.

> > 5/ I think "year(s)" could be replaced with "year" :) And manage the plural
> > for month/months should not be too difficult.
> ?

if months == 1 then display 'month' elsif months > 1 then display 'months'

> > > 5/ I think "year(s)" could be replaced with "year" :) And manage the plural
> > > for month/months should not be too difficult.
> > ?

> if months == 1 then display 'month' elsif months > 1 then display 'months'

If possible, it should be avoided. Some languages have more than two nouns forms (one for unit, and one for plural). See:

http://www.unicode.org/cldr/charts/27/supplemental/language_plural_rules.html

Something like that would do it:

  Age: years: 10 / months: 2

(In reply to Frédéric Demians from comment #33)
(...)
> 
> Something like that would do it:
> 
>   Age: years: 10 / months: 2

+1 for better translatability

The last patch replace all others. It removes showing of age in patron's detail page (duplicate of bug 14763) and it remove the use of perl sub getAge() via ajax call (bad performances).
Also it handles better plural forms.

Alex,
Could you please:
1/ move the CalculateAge to a js file (to make it reusable)
2/ do not display "Age:" if the calculation fails
3/ Remove the checkdate param, it is not really needed. You should use Date_from_syspref defined in calendar.inc instead

(In reply to Jonathan Druart from comment #38)
> Alex,
> Could you please:
> 1/ move the CalculateAge to a js file (to make it reusable)
> 2/ do not display "Age:" if the calculation fails
> 3/ Remove the checkdate param, it is not really needed. You should use
> Date_from_syspref defined in calendar.inc instead

I don't need Date_from_syspref since i use datepicker( 'getDate' ) from datepicker component to get the correct format.

With "to make it reusable", I meant "Don't hardcode stuff in it"...

And don't call CheckDate, it's obsolete.
Date_from_syspref does that for any date format.

(In reply to Jonathan Druart from comment #41)
> With "to make it reusable", I meant "Don't hardcode stuff in it"...

oops...

(In reply to Jonathan Druart from comment #41)
> With "to make it reusable", I meant "Don't hardcode stuff in it"...

This means i could not reset the description (div .hint) under the datepicker with date format when datepicker is emptied.

I don't understand your issue, what about something like:

function CalculateAge(date) {
    // Caculate age
    return (years, months);
}

onchange(
    age = CalculateAge(date);
    add_age_string_to_the_div
);

There is a GetAge function already in C4::Members which I used to write a patch for Bug 14763. I am not saying this is a duplicate of that bug, because I feel this is slightly different to what my patch does, but is it possible to use that function for this?

(In reply to Aleisha Amohia from comment #46)
> There is a GetAge function already in C4::Members which I used to write a
> patch for Bug 14763. I am not saying this is a duplicate of that bug,
> because I feel this is slightly different to what my patch does, but is it
> possible to use that function for this?

I tried to use it by making an ajax call to a perl script but response times are too long.
Now CalculateAge function is quite simple and reusable.

Maybe "display_age" is a better function name than "write_age", I let the RM cut.

Patch no longer applies. Please rebase and re-set status go "Passed QA".

Created attachment 49032 [details] [review]
Bug 15206 - Show patron's age under date of birth in memberentry.pl

Test plan:

Create or edit a patron (members/memberentry.pl), enter a date of birth
(for a new patron), patron'a age should be shown under

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 49033 [details] [review]
Bug 15206 - Make CalculateAge a reusable function

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 49034 [details] [review]
Bug 15206: Make strings translatable

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to Master - Should be in the May 2016 release.  Thanks!