Attachment 75634 Details for Bug 18821 – Bug 18821: TrackLastPatronActivity is a performance killer

[patch] Bug 18821: TrackLastPatronActivity is a performance killer

Bug-18821-TrackLastPatronActivity-is-a-performance.patch (text/plain), 5.90 KB, created by Kyle M Hall (khall) on 2018-05-29 14:08:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2018-05-29 14:08:13 UTC

Size: 5.90 KB

patch

obsolete

>From 5bfd1ad99a2bf068921a51a35a6b0381290b5f98 Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Mon, 7 May 2018 17:29:12 +0000
>Subject: [PATCH] Bug 18821: TrackLastPatronActivity is a performance killer
>
>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/Auth.t
>
>Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
>---
> C4/Auth.pm            | 36 +++++++++++++++++++++++++++++-------
> t/db_dependent/Auth.t | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 77 insertions(+), 8 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index 7442b1dd78..9554cd8d6e 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -55,7 +55,7 @@ BEGIN {
>     @ISA       = qw(Exporter);
>     @EXPORT    = qw(&checkauth &get_template_and_user &haspermission &get_user_subpermissions);
>     @EXPORT_OK = qw(&check_api_auth &get_session &check_cookie_auth &checkpw &checkpw_internal &checkpw_hash
>-      &get_all_subpermissions &get_user_subpermissions
>+      &get_all_subpermissions &get_user_subpermissions track_login_for_session
>     );
>     %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] );
>     $ldap      = C4::Context->config('useldapserver') || 0;
>@@ -802,6 +802,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.
>@@ -827,7 +829,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 = '';
>@@ -1199,11 +1201,7 @@ sub checkauth {
>             );
>         }
> 
>-        if ( $userid ) {
>-            # track_login also depends on pref TrackLastPatronActivity
>-            my $patron = Koha::Patrons->find({ userid => $userid });
>-            $patron->track_login if $patron;
>-        }
>+        track_login_for_session( $userid, $session );
> 
>         return ( $userid, $cookie, $sessionID, $flags );
>     }
>@@ -2078,6 +2076,30 @@ sub getborrowernumber {
>     return 0;
> }
> 
>+=head2 track_login_for_session
>+
>+  track_login_for_session( $userid, $session );
>+
>+C<$userid> the userid of the member
>+C<$session> the CGI::Session object used to store the session's state.
>+
>+Wraps the call to $patron->track_login, the method used to update borrowers.lastseen.
>+
>+=cut
>+
>+sub track_login_for_session {
>+    my ( $userid, $session ) = @_;
>+
>+    if ( $userid && $session && !$session->param('tracked_for_session') ) {
>+        $session->param( 'tracked_for_session', 1 );
>+        $session->flush();
>+
>+        # track_login also depends on pref TrackLastPatronActivity
>+        my $patron = Koha::Patrons->find( { userid => $userid } );
>+        $patron->track_login if $patron;
>+    }
>+}
>+
> END { }    # module clean-up code here (global destructor)
> 1;
> __END__
>diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
>index c08060e93c..a9b5b17486 100644
>--- a/t/db_dependent/Auth.t
>+++ b/t/db_dependent/Auth.t
>@@ -10,7 +10,7 @@ use CGI qw ( -utf8 );
> use Test::MockObject;
> use Test::MockModule;
> use List::MoreUtils qw/all any none/;
>-use Test::More tests => 21;
>+use Test::More tests => 22;
> use Test::Warn;
> use t::lib::Mocks;
> use t::lib::TestBuilder;
>@@ -67,6 +67,53 @@ subtest 'checkauth() tests' => sub {
> 
> };
> 
>+subtest 'track_login_for_session() tests' => sub {
>+
>+    plan tests => 5;
>+
>+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
>+    my $userid = $patron->userid;
>+
>+    $patron->lastseen( undef );
>+    $patron->store();
>+
>+    # Mock a CGI object with real userid param
>+    my $cgi = Test::MockObject->new();
>+    $cgi->mock( 'param', sub { return $patron; } );
>+    $cgi->mock( 'cookie', sub { return; } );
>+
>+    my $session = new CGI::Session( undef, undef, { Directory => File::Spec->tmpdir } );
>+
>+    t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' );
>+
>+    C4::Auth::track_login_for_session( $userid );
>+    $patron->_result()->discard_changes();
>+    is( $patron->lastseen, undef, 'Patron last seen should be unchanged if no session is passed' );
>+
>+    C4::Auth::track_login_for_session( $userid, $session );
>+    $patron->_result()->discard_changes();
>+    isnt( $patron->lastseen, undef, 'Patron should have last seen set when TrackLastPatronActivity = 1' );
>+
>+    sleep(1); # We need to wait a tiny bit to make sure the timestamp will be different
>+    my $last_seen = $patron->lastseen;
>+    C4::Auth::track_login_for_session( $userid, $session );
>+    $patron->_result()->discard_changes();
>+    is( $patron->lastseen, $last_seen, 'Patron last seen should be unchanged if passed the same session' );
>+
>+    $session = new CGI::Session( undef, undef, { Directory => File::Spec->tmpdir } );
>+    C4::Auth::track_login_for_session( $userid, $session );
>+    $patron->_result()->discard_changes();
>+    isnt( $patron->lastseen, $last_seen, 'Patron last seen should be changed if given a new session' );
>+
>+    t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '0' );
>+    sleep(1);
>+    $last_seen = $patron->lastseen;
>+    C4::Auth::track_login_for_session( $userid, $session );
>+    $patron->_result()->discard_changes();
>+    is( $patron->lastseen, $last_seen, 'Patron should have last seen unchanged when TrackLastPatronActivity = 0' );
>+
>+};
>+
> my $hash1 = hash_password('password');
> my $hash2 = hash_password('password');
> 
>-- 
>2.15.1 (Apple Git-101)

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