Attachment 73891 Details for Bug 18821 – Bug 18821 - TrackLastPatronActivity is a performance killer

[patch] Bug 18821 - TrackLastPatronActivity is a performance killer

Bug-18821---TrackLastPatronActivity-is-a-performan.patch (text/plain), 4.98 KB, created by Mark Tompsett on 2018-04-09 19:25:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Tompsett

Created: 2018-04-09 19:25:46 UTC

Size: 4.98 KB

patch

obsolete

>From 03a1f579c2fef93b4e13df3bd8d717959cb4bf9f Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Mon, 19 Jun 2017 09:49:00 -0400
>Subject: [PATCH] Bug 18821 - TrackLastPatronActivity is a performance killer
>
>Enabling TrackLastPatronActivity can cause an extreme performance hit, especially on the staff side during checkin and checkout. It makes more sense to only update the last activity timestamp on the first access of a given session. This will greatly improve performance while retaining the same basic information.
>
>Test Plan:
>1) Apply this patch
>2) Start a new session ( a private browser window works well )
>3) Note the lastseen column in the borrowers table is updated
>4) Browse a few pages, not the lastseen column is not updated again
>5) Close the browser window and repeat steps 2-4
>6) prove t/db_dependent/Members.t
>
>Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
>---
> C4/Auth.pm               |  6 ++++--
> Koha/Patron.pm           | 12 +++++++++---
> t/db_dependent/Members.t | 20 ++++++++++++++++++--
> 3 files changed, 31 insertions(+), 7 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index 13c4652..7a5b4e4 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -801,6 +801,8 @@ sub checkauth {
>     my $casparam = $query->param('cas');
>     my $q_userid = $query->param('userid') // '';
> 
>+    my $session;
>+
>     # Basic authentication is incompatible with the use of Shibboleth,
>     # as Shibboleth may return REMOTE_USER as a Shibboleth attribute,
>     # and it may not be the attribute we want to use to match the koha login.
>@@ -826,7 +828,7 @@ sub checkauth {
>     }
>     elsif ( $sessionID = $query->cookie("CGISESSID") )
>     {    # assignment, not comparison
>-        my $session = get_session($sessionID);
>+        $session = get_session($sessionID);
>         C4::Context->_new_userenv($sessionID);
>         my ( $ip, $lasttime, $sessiontype );
>         my $s_userid = '';
>@@ -1219,7 +1221,7 @@ sub checkauth {
>         if ( $userid ) {
>             # track_login also depends on pref TrackLastPatronActivity
>             my $patron = Koha::Patrons->find({ userid => $userid });
>-            $patron->track_login if $patron;
>+            $patron->track_login( { session => $session } ) if $patron;
>         }
> 
>         return ( $userid, $cookie, $sessionID, $flags );
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 2322131..fa02865 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -404,9 +404,15 @@ sub has_overdues {
> 
> sub track_login {
>     my ( $self, $params ) = @_;
>-    return if
>-        !$params->{force} &&
>-        !C4::Context->preference('TrackLastPatronActivity');
>+    my $session = $params->{session};
>+
>+    return unless C4::Context->preference('TrackLastPatronActivity') || $params->{force};
>+
>+    if ( $session ) {
>+        return if $session->param('tracked_for_session') && !$params->{force};
>+        $session->param('tracked_for_session', 1);
>+    }
>+
>     $self->lastseen( dt_from_string() )->store;
> }
> 
>diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
>index c7bc6b3..aada28b 100755
>--- a/t/db_dependent/Members.t
>+++ b/t/db_dependent/Members.t
>@@ -17,7 +17,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 60;
>+use Test::More tests => 62;
> use Test::MockModule;
> use Test::Exception;
> 
>@@ -350,12 +350,28 @@ $patstodel = GetBorrowersToExpunge( { last_seen => '2016-02-15' });
> is( scalar @$patstodel, 2, 'TrackLastPatronActivity - 2 patrons must be deleted' );
> $patstodel = GetBorrowersToExpunge( { last_seen => '2016-04-04' });
> is( scalar @$patstodel, 3, 'TrackLastPatronActivity - 3 patrons must be deleted' );
>+
>+
>+# Test method last_seen / TrackLastPatronActivity
> my $patron2 = $builder->build({ source => 'Borrower', value => { lastseen => undef } });
> t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '0' );
> Koha::Patrons->find( $patron2->{borrowernumber} )->track_login;
> is( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, undef, 'Lastseen should not be changed' );
>+
> Koha::Patrons->find( $patron2->{borrowernumber} )->track_login({ force => 1 });
>-isnt( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, undef, 'Lastseen should be changed now' );
>+my $last_seen = Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen;
>+isnt( $last_seen, undef, 'Lastseen should be changed now' );
>+
>+# Last seen shouldn't be updated a second time for this session
>+t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' );
>+Koha::Patrons->find( $patron2->{borrowernumber} )->track_login();
>+is( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, $last_seen, 'Lastseen should not be changed' );
>+
>+# If it's forced, it should still be updated
>+sleep(1); # We need to wait a tiny bit to make sure the timestamp will be different
>+Koha::Patrons->find( $patron2->{borrowernumber} )->track_login({ force => 1 });
>+isnt( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, $last_seen, 'Lastseen should be changed if forced' );
>+
> 
> # Regression tests for BZ13502
> ## Remove all entries with userid='' (should be only 1 max)
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 18821: 64425 | 64426 | 65687 | 73175 | 73706 | 73863 | 73891 | 73892 | 73893 | 75139 | 75288 | 75289 | 75427 | 75632 | 75633 | 75634 | 75635 | 75727 | 75728 | 75729 | 75733