From 0aa5eec2332f0e0ae7f6adefebc274b02f8830c5 Mon Sep 17 00:00:00 2001 From: Kyle M Hall 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 Signed-off-by: Marcel de Rooy Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- 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.39.5