Created attachment 33510 [details] [review] Bug 13242: DateUtils should manage separately 9999-* dates On bug 13226, we have seen that my $date = DateTime->new(year => 9999, month => 12, day => 31); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31' ); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31', 'America/Los_Angeles' ); => KO (~20sec on my laptop) For instance if you try to check in items for a patron which is restricted indefinitely (and there is no overdue on the item), the code in C4::Circulation::AddReturn l~1948 my $borrower_debar_dt = dt_from_string( $borrower->{debarred} ); will be called. dt_from_string should not called DateTime::Format::DateParse->parse_datetime with the time zone parameter in this case. Test plan: Create an indefinitely restriction for a patron and check an item in. Without this patch, it should take ~20 sec. With, ~2 or 3 sec.
Original code introduced by bug 8231.
Created attachment 33511 [details] [review] Bug 13242: DateUtils should manage separately 9999-* dates On bug 13226, we have seen that my $date = DateTime->new(year => 9999, month => 12, day => 31); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31' ); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31', 'America/Los_Angeles' ); => KO (~20sec on my laptop) For instance if you try to check in items for a patron which is restricted indefinitely (and there is no overdue on the item), the code in C4::Circulation::AddReturn l~1948 my $borrower_debar_dt = dt_from_string( $borrower->{debarred} ); will be called. dt_from_string should not called DateTime::Format::DateParse->parse_datetime with the time zone parameter in this case. Test plan: Create an indefinitely restriction for a patron and check an item in. Without this patch, it should take ~20 sec. With, ~2 or 3 sec.
It seems to come from here: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow Why is it necessary to use timezone for DateTime conversion process in the first place?
(In reply to Frédéric Demians from comment #4) > It seems to come from here: > > https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow > > Why is it necessary to use timezone for DateTime conversion process in the > first place? Salut Frédéric, I don't understand, what is "the first place"? we use DateTime::Format::DateParse->parse_datetime, not DateTime->new (maybe it calls it, I didn't deep into the code). All what I know is that it is useless to specify timezone for 9999-12-31 :) Note that $ENV{TZ} is not set on my local instance.
> I don't understand, what is "the first place"? I mean: Is it really necessary to use timezone in dt_from_string function? Otherwise, I fully agree that your patch solve the bug (ugly one). I would add a FIXME comment in the code to point to bug number, in order to avoid that someone else reintroduce the bug in the future.
(In reply to Frédéric Demians from comment #6) > > I don't understand, what is "the first place"? > > I mean: Is it really necessary to use timezone in dt_from_string function? I don't know. > Otherwise, I fully agree that your patch solve the bug (ugly one). I would > add a FIXME comment in the code to point to bug number, in order to avoid > that someone else reintroduce the bug in the future. Maybe a test based on localtime_before - localtime_after should be < 3 sec. But it's not really clean, it could depend on the load of the machine.
> Maybe a test based on localtime_before - localtime_after should be < 3 sec. > But it's not really clean, it could depend on the load of the machine. Very good suggestion. I'd try to add a test to t/DateUtils.t before signing your patch.
Created attachment 33560 [details] [review] Bug 13242 Add a UT to t/DateUtils.t for testing DateTime bug A bug in DateTime slow down drastically date parsing when the dates are in the far distant future: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow This UT tests this situation. TO TEST: - Apply the patch containing the UT - prove -v t/DateUtils.t - You see that parsing a 9999-01-01 that take forever (ie more than 1s) - Apply Jonathan patch - prove -v t/DateUtils.t - No more complain.
Created attachment 33561 [details] [review] Bug 13242: DateUtils should manage separately 9999-* dates On bug 13226, we have seen that my $date = DateTime->new(year => 9999, month => 12, day => 31); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31' ); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31', 'America/Los_Angeles' ); => KO (~20sec on my laptop) For instance if you try to check in items for a patron which is restricted indefinitely (and there is no overdue on the item), the code in C4::Circulation::AddReturn l~1948 my $borrower_debar_dt = dt_from_string( $borrower->{debarred} ); will be called. dt_from_string should not called DateTime::Format::DateParse->parse_datetime with the time zone parameter in this case. Test plan: Create an indefinitely restriction for a patron and check an item in. Without this patch, it should take ~20 sec. With, ~2 or 3 sec. Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Comment on attachment 33560 [details] [review] Bug 13242 Add a UT to t/DateUtils.t for testing DateTime bug Review of attachment 33560 [details] [review]: ----------------------------------------------------------------- ::: t/DateUtils.t @@ +7,2 @@ > use Test::MockModule; > +use Time::HiRes qw/ gettimeofday /; Time::HiRes seems to be a new dependency. Is not it possible to do the same without adding it?
> Time::HiRes seems to be a new dependency. > Is not it possible to do the same without adding it? I see Time::HiRes in core dependencies: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Installer/PerlDependencies.pm;h=09be2cda3e3ccc84777d1e50b9d2054d778d6a0f;hb=HEAD#l490
(In reply to Frédéric Demians from comment #12) > > Time::HiRes seems to be a new dependency. > > Is not it possible to do the same without adding it? > > I see Time::HiRes in core dependencies: > > http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Installer/ > PerlDependencies.pm;h=09be2cda3e3ccc84777d1e50b9d2054d778d6a0f;hb=HEAD#l490 Oops yes, sorry.
Created attachment 33564 [details] [review] Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug A bug in DateTime slow down drastically date parsing when the dates are in the far distant future: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow This UT tests this situation. TO TEST: - Apply the patch containing the UT - prove -v t/DateUtils.t - You see that parsing a 9999-01-01 that take forever (ie more than 1s) - Apply Jonathan patch - prove -v t/DateUtils.t - No more complain. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 33565 [details] [review] Bug 13242: DateUtils should manage separately 9999-* dates On bug 13226, we have seen that my $date = DateTime->new(year => 9999, month => 12, day => 31); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31' ); => OK DateTime::Format::DateParse->parse_datetime( '9999-12-31', 'America/Los_Angeles' ); => KO (~20sec on my laptop) For instance if you try to check in items for a patron which is restricted indefinitely (and there is no overdue on the item), the code in C4::Circulation::AddReturn l~1948 my $borrower_debar_dt = dt_from_string( $borrower->{debarred} ); will be called. dt_from_string should not called DateTime::Format::DateParse->parse_datetime with the time zone parameter in this case. Test plan: Create an indefinitely restriction for a patron and check an item in. Without this patch, it should take ~20 sec. With, ~2 or 3 sec. Signed-off-by: Frederic Demians <f.demians@tamil.fr>
I added my sign off on your patch.
I tested with and without patch and had the same problem (about 20 sec for checking in an item for an undefinitly restricted patron. With patch, I played around with the following line: return DateTime::Format::DateParse->parse_datetime($date_string); Same result without and with time zone parameter (e.g. 'floating' or '+0000'). I also tested and had the same result using the following code (with and without time_zone variants) return DateTime->new ( year=> 9999, month=> 12, day => 31, hour => 0, minute => 0, second => 0, time_zone => '+0000', ); The only thing that reduced time was to change the year e.g. to 2999 (for both methods).
(In reply to Marc Véron from comment #17) > I tested with and without patch and had the same problem (about 20 sec for > checking in an item for an undefinitly restricted patron. Did you test with bug 13226 (pushed in master 1 hour ago)?
No, I did not. Sorry, I can not test for the moment, just got a Windows 8.1 Update and that crashed my VM.
OK, deinstalled Windows update and re-installed VM... Tested again: If I check in the item using the button "Renew or return checked items" below the checkout table it is now < 5 sec => OK If I check in the item using the Check in form at top of the patrons checkout page (circulation.pl), it is about 30 sec. => not OK If I check in the item using the Check in form at top of other pages page (e.g. mainpage.pl) , it is < 5 sec. => OK
I see the same behaviour MARC noted - using the checkin tab on top of the patrons page is still slow. So is the normal check-in page - should this be fixed? Checking in from the patron account is ok, as tested for the earlier other patch.
(In reply to Katrin Fischer from comment #21) > I see the same behaviour MARC noted - using the checkin tab on top of the > patrons page is still slow. So is the normal check-in page - should this be > fixed? > > Checking in from the patron account is ok, as tested for the earlier other > patch. I can also see the slowness... I only occurs when checking in a non-overdue item. If the item is overdue, it's OK. - a debarred borrower - an item that isn't overdue - check-in from the circ/circulation-home.pl page
I will send a re-factored patch: - complete Jonathan fix to dt_from_string() with an equivalent fix to output_pref(). - separate the fix patch and the UT patch to facilitate testing and signing-off - new sign-off required
Created attachment 33589 [details] [review] Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug A bug in DateTime slow down drastically date parsing when the dates are in the far distant future: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow This UT tests this situation which affects Koha::DateUtils function dt_from_string() and output_pref(). TO TEST: - Apply the patch containing the UT - prove -v t/DateUtils.t - You see that parsing a 9999-01-01 that take forever (ie more than 1s) - Apply the patch containing the fix - prove -v t/DateUtils.t - No more complain.
Created attachment 33590 [details] [review] Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) TEST PLAN: - Method 1--with UT - Use the UT associated to this bug, without applying this patch, and then after applying this patch - Method 2--using Koha - Without this patch - Find a borrower with several checkouts that are not overdue. - Debarred the borrower - Go on circ/circulation-home.pl page - Select Check in tab, and do a check in - It required more than 20s to display the return.pl page - Apply the patch, and repeat previous steps => return.pl is immediately displayed.
Created attachment 33591 [details] [review] Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) TEST PLAN: - Method 1--with UT - Use the UT associated to this bug, without applying this patch, and then after applying this patch - Method 2--using Koha - Without this patch - Find a borrower with several checkouts that are not overdue. - Debarred the borrower - Go on circ/circulation-home.pl page - Select Check in tab, and do a check in - It required more than 20s to display the return.pl page - Apply the patch, and repeat previous steps => return.pl is immediately displayed. Followed method 2. Time problem no longer exists. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 33592 [details] [review] Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug A bug in DateTime slow down drastically date parsing when the dates are in the far distant future: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow This UT tests this situation which affects Koha::DateUtils function dt_from_string() and output_pref(). TO TEST: - Apply the patch containing the UT - prove -v t/DateUtils.t - You see that parsing a 9999-01-01 that take forever (ie more than 1s) - Apply the patch containing the fix - prove -v t/DateUtils.t - No more complain. Followed test plan. Test behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Note: While testing, I did not like the check in message for a user with indefinite restriction and changed it in Bug 13261 It was: Reminder: Patron was earlier restricted until 9999-12-31
Created attachment 33596 [details] [review] [PASSED QA] Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) TEST PLAN: - Method 1--with UT - Use the UT associated to this bug, without applying this patch, and then after applying this patch - Method 2--using Koha - Without this patch - Find a borrower with several checkouts that are not overdue. - Debarred the borrower - Go on circ/circulation-home.pl page - Select Check in tab, and do a check in - It required more than 20s to display the return.pl page - Apply the patch, and repeat previous steps => return.pl is immediately displayed. Followed method 2. Time problem no longer exists. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33597 [details] [review] [PASSED QA] Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug A bug in DateTime slow down drastically date parsing when the dates are in the far distant future: https://metacpan.org/pod/DateTime#Determining-the-Local-Time-Zone-Can-Be-Slow This UT tests this situation which affects Koha::DateUtils function dt_from_string() and output_pref(). TO TEST: - Apply the patch containing the UT - prove -v t/DateUtils.t - You see that parsing a 9999-01-01 that take forever (ie more than 1s) - Apply the patch containing the fix - prove -v t/DateUtils.t - No more complain. Followed test plan. Test behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Works as described - check-ins are now much faster. Passes tests and QA script.
Comment on attachment 33596 [details] [review] [PASSED QA] Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) Review of attachment 33596 [details] [review]: ----------------------------------------------------------------- ::: Koha/DateUtils.pm @@ +128,5 @@ > > return unless defined $dt; > > + # FIXME: see bug 13242 => no TZ for dates 'infinite' > + $dt->set_time_zone( C4::Context->tz ) if $dt->ymd !~ /^9999/; Fredéric, did you locate where output_pref is called with "9999-12-31"?
> Fredéric, did you locate where output_pref is called with "9999-12-31"? circ/returns.tt, line 423: [% IF ( errmsgloo.prevdebarred ) %] <p class="problem"><b>Reminder: </b>Patron was earlier restricted until [% errmsgloo.prevdebarred | $KohaDates %]</p> [% END %] The "| $KohaDate" directive calls the KohaDate TT plugin. Tracking this kind of issue is awful.
(In reply to Frédéric Demians from comment #32) Good catch! That means we need a specific alert message in this case.
Regarding alert message: Bug 13261 - Better check in message for patrons with indefinite restriction Signed off (first part), re-wording of message waiting for Sign-off Marc
Patches pushed to master. Thanks Frédéric!
Created attachment 33945 [details] [review] Bug 13242: Remove warning if no parameter given Without any parameter, dt_from_string should not raise a warning message. Test plan: Verify that the test file t/DateUtils.t displays a warning: Use of uninitialized value $date_string in pattern match (m//) at Koha/DateUtils.pm line 58 if the change in dt_from_string is not applied (manually edit the file).
Followup pushed to master. Thanks Jonathan!
Would it be possible to have it in 3.16.x ? Thanx
(In reply to Sophie MEYNIEUX from comment #38) > Would it be possible to have it in 3.16.x ? Thanx oops, i missed this bug i'll take a look for the next 3.16 release
Found checks like: if ( $dt->ymd !~ /^9999/ ) { $dt_params{year} < 9999 Note that if you would pick year 3000 or 9000, it still would take forever to let DateTime calculate things in a named timezone. If the date is more than a few days/weeks in the future, I suppose that we already could jump to floating or UTC instead of the named timezone?