Bug 13242 - Slow checkin for patrons restricted indefinitely
Summary: Slow checkin for patrons restricted indefinitely
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 8231
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-13 08:36 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
9 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 13242: DateUtils should manage separately 9999-* dates (1.57 KB, patch)
2014-11-13 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13242: DateUtils should manage separately 9999-* dates (2.53 KB, patch)
2014-11-13 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13242 Add a UT to t/DateUtils.t for testing DateTime bug (2.17 KB, patch)
2014-11-14 10:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13242: DateUtils should manage separately 9999-* dates (2.58 KB, patch)
2014-11-14 10:14 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug (2.23 KB, patch)
2014-11-14 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13242: DateUtils should manage separately 9999-* dates (2.58 KB, patch)
2014-11-14 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug (2.68 KB, patch)
2014-11-15 12:16 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) (1.70 KB, patch)
2014-11-15 12:17 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) (1.88 KB, patch)
2014-11-15 13:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug (2.86 KB, patch)
2014-11-15 13:11 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 13242: Fix DateUtils for 'infinite' dates (ie year 9999) (1.95 KB, patch)
2014-11-16 09:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13242: Add a UT to t/DateUtils.t for testing DateTime bug (3.01 KB, patch)
2014-11-16 09:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13242: Remove warning if no parameter given (1.92 KB, patch)
2014-11-26 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-11-13 08:36:07 UTC

    
Comment 1 Jonathan Druart 2014-11-13 08:47:34 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-11-13 08:49:12 UTC
Original code introduced by bug 8231.
Comment 3 Jonathan Druart 2014-11-13 08:50:00 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2014-11-13 12:38:33 UTC
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?
Comment 5 Jonathan Druart 2014-11-14 08:36:06 UTC
(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.
Comment 6 Frédéric Demians 2014-11-14 09:28:05 UTC
> 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.
Comment 7 Jonathan Druart 2014-11-14 09:32:45 UTC
(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.
Comment 8 Frédéric Demians 2014-11-14 09:44:31 UTC
> 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.
Comment 9 Frédéric Demians 2014-11-14 10:12:22 UTC Comment hidden (obsolete)
Comment 10 Frédéric Demians 2014-11-14 10:14:01 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-11-14 10:23:41 UTC
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?
Comment 12 Frédéric Demians 2014-11-14 10:34:20 UTC
> 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
Comment 13 Jonathan Druart 2014-11-14 11:06:23 UTC
(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.
Comment 14 Jonathan Druart 2014-11-14 11:08:39 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-11-14 11:08:44 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-11-14 11:09:25 UTC
I added my sign off on your patch.
Comment 17 Marc Véron 2014-11-14 15:35:49 UTC
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).
Comment 18 Jonathan Druart 2014-11-14 15:43:40 UTC
(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)?
Comment 19 Marc Véron 2014-11-14 16:36:24 UTC
No, I did not.

Sorry, I can not test for the moment, just got a Windows 8.1 Update and that crashed my VM.
Comment 20 Marc Véron 2014-11-14 17:07:38 UTC
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
Comment 21 Katrin Fischer 2014-11-15 09:22:04 UTC
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.
Comment 22 Frédéric Demians 2014-11-15 10:47:34 UTC
(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
Comment 23 Frédéric Demians 2014-11-15 12:14:53 UTC
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
Comment 24 Frédéric Demians 2014-11-15 12:16:34 UTC Comment hidden (obsolete)
Comment 25 Frédéric Demians 2014-11-15 12:17:00 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2014-11-15 13:04:21 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2014-11-15 13:11:22 UTC Comment hidden (obsolete)
Comment 28 Marc Véron 2014-11-15 16:24:54 UTC
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
Comment 29 Katrin Fischer 2014-11-16 09:12:31 UTC
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>
Comment 30 Katrin Fischer 2014-11-16 09:12:34 UTC
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 31 Jonathan Druart 2014-11-17 08:34:29 UTC
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"?
Comment 32 Frédéric Demians 2014-11-17 09:35:04 UTC
> 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.
Comment 33 Jonathan Druart 2014-11-17 09:43:39 UTC
(In reply to Frédéric Demians from comment #32)

Good catch!
That means we need a specific alert message in this case.
Comment 34 Marc Véron 2014-11-17 09:47:17 UTC
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
Comment 35 Tomás Cohen Arazi 2014-11-19 14:43:36 UTC
Patches pushed to master.

Thanks Frédéric!
Comment 36 Jonathan Druart 2014-11-26 08:46:29 UTC
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).
Comment 37 Tomás Cohen Arazi 2014-11-26 14:37:49 UTC
Followup pushed to master.

Thanks Jonathan!
Comment 38 Sophie MEYNIEUX 2015-05-19 13:19:15 UTC
Would it be possible to have it in 3.16.x ? Thanx
Comment 39 Mason James 2015-06-04 01:03:17 UTC
(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
Comment 40 Marcel de Rooy 2015-07-06 12:11:37 UTC
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?