From 2bae585fcd4d5a5e31c0503cc8ea1cd3ea0f29ba Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 1 Jun 2018 11:00:57 +0200 Subject: [PATCH] Bug 18821: (QA follow-up) Last tweaks for performance [1] passing unsafe has no use since it is a scalar, removed it to unconfuse [2] remove caching when pref is disabled [3] caching userid removes the need for calling Patron->find each time [4] subsequent changes in unit test [5] cosmetic renames to move from session to daily basis (changed dev angle) Signed-off-by: Marcel de Rooy First call going thru Koha::Patron takes about 0.0150 sec. Subsequent calls only use caching and take about 0.0006 sec. Signed-off-by: Kyle M Hall Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- C4/Auth.pm | 26 +++++++++++--------------- t/db_dependent/Auth.t | 22 +++++++++++----------- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index ae2e7f6c61..60c8c87911 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -56,7 +56,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 track_login_for_session + &get_all_subpermissions &get_user_subpermissions track_login_daily ); %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] ); $ldap = C4::Context->config('useldapserver') || 0; @@ -1202,7 +1202,7 @@ sub checkauth { ); } - track_login_for_session( $userid ); + track_login_daily( $userid ); return ( $userid, $cookie, $sessionID, $flags ); } @@ -2077,30 +2077,26 @@ sub getborrowernumber { return 0; } -=head2 track_login_for_session +=head2 track_login_daily - track_login_for_session( $userid ); + track_login_daily( $userid ); -C<$userid> the userid of the member - -Wraps the call to $patron->track_login, the method used to update borrowers.lastseen. +Wraps the call to $patron->track_login, the method used to update borrowers.lastseen. We only call track_login once a day. =cut -sub track_login_for_session { +sub track_login_daily { my $userid = shift; - return unless $userid; - - my $patron = Koha::Patrons->find( { userid => $userid } ); - return unless $patron; + return if !$userid || !C4::Context->preference('TrackLastPatronActivity'); my $cache = Koha::Caches->get_instance(); - my $cache_key = "seen-for-session-" . $patron->id; - my $cached = $cache->get_from_cache( $cache_key, { unsafe => 1 } ); - + my $cache_key = "track_login_" . $userid; + my $cached = $cache->get_from_cache($cache_key); my $today = dt_from_string()->ymd; return if $cached && $cached eq $today; + my $patron = Koha::Patrons->find({ userid => $userid }); + return unless $patron; $patron->track_login; $cache->set_in_cache( $cache_key, $today ); } diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index cfff2f7a2c..5b7ce1888a 100644 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -67,7 +67,7 @@ subtest 'checkauth() tests' => sub { }; -subtest 'track_login_for_session() tests' => sub { +subtest 'track_login_daily tests' => sub { plan tests => 5; @@ -78,34 +78,34 @@ subtest 'track_login_for_session() tests' => sub { $patron->store(); my $cache = Koha::Caches->get_instance(); - my $cache_key = "seen-for-session-" . $patron->id; + my $cache_key = "track_login_" . $patron->userid; $cache->clear_from_cache($cache_key); t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' ); is( $patron->lastseen, undef, 'Patron should have not last seen when newly created' ); - C4::Auth::track_login_for_session( $userid ); + C4::Auth::track_login_daily( $userid ); $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 ); + C4::Auth::track_login_daily( $userid ); $patron->_result()->discard_changes(); - is( $patron->lastseen, $last_seen, 'Patron last seen should be unchanged if passed the same session' ); + is( $patron->lastseen, $last_seen, 'Patron last seen should still be unchanged' ); $cache->clear_from_cache($cache_key); - C4::Auth::track_login_for_session( $userid ); + C4::Auth::track_login_daily( $userid ); $patron->_result()->discard_changes(); - isnt( $patron->lastseen, $last_seen, 'Patron last seen should be changed if given a new session' ); + isnt( $patron->lastseen, $last_seen, 'Patron last seen should be changed if we cleared the cache' ); t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '0' ); - sleep(1); - $last_seen = $patron->lastseen; - C4::Auth::track_login_for_session( $userid ); + $patron->lastseen( undef )->store; + $cache->clear_from_cache($cache_key); + C4::Auth::track_login_daily( $userid ); $patron->_result()->discard_changes(); - is( $patron->lastseen, $last_seen, 'Patron should have last seen unchanged when TrackLastPatronActivity = 0' ); + is( $patron->lastseen, undef, 'Patron should still have last seen unchanged when TrackLastPatronActivity = 0' ); }; -- 2.39.5