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 <m.de.rooy@rijksmuseum.nl>
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 <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(cherry picked from commit 2bae585fcd4d5a5e31c0503cc8ea1cd3ea0f29ba)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
(cherry picked from commit f0159f8f5712a526f2c1af44b4f9ecc0d82c15a6)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

diff --git a/C4/Auth.pm b/C4/Auth.pm
index 0b9ce4d..38f7c95 100644 (file)
--- 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;
@@ -1189,7 +1189,7 @@ sub checkauth {
             );
         }
 
-        track_login_for_session( $userid );
+        track_login_daily( $userid );
 
         return ( $userid, $cookie, $sessionID, $flags );
     }
@@ -2096,30 +2096,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 686811b..b96c342 100644 (file)
--- 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' );
 
 };