Attachment 152506 Details for Bug 15504 – Bug 15504: (follow-up) Migrate to one clear method

[patch] Bug 15504: (follow-up) Migrate to one clear method

Bug-15504-follow-up-Migrate-to-one-clear-method.patch (text/plain), 7.71 KB, created by Martin Renvoize (ashimema) on 2023-06-21 09:37:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2023-06-21 09:37:26 UTC

Size: 7.71 KB

patch

obsolete

>From fec140c569608866164dd88da7caa40ac224bc08 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Tue, 20 Jun 2023 19:23:11 +0100
>Subject: [PATCH] Bug 15504: (follow-up) Migrate to one clear method
>
>We were using a series of similarly named methods spread in distinct places
>around the codebase. This combines the logic of C4::Auth::track_login_daily
>and Koha::Patron->track_login into a new Koha::Patron->update_lastseen method.
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> C4/Auth.pm            | 34 +++-------------------------------
> C4/Circulation.pm     |  6 +++---
> C4/ILSDI/Services.pm  |  4 ++--
> C4/SIP/Sip/MsgType.pm |  6 +++++-
> Koha/Patron.pm        | 36 ++++++++++++++++++++++++------------
> 5 files changed, 37 insertions(+), 49 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index 041779854f..2c0dca8451 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -73,7 +73,7 @@ BEGIN {
> 
>     @EXPORT_OK = qw(
>       checkauth check_api_auth get_session check_cookie_auth checkpw checkpw_internal checkpw_hash
>-      get_all_subpermissions get_user_subpermissions track_login_daily in_iprange
>+      get_all_subpermissions get_user_subpermissions in_iprange
>       get_template_and_user haspermission create_basic_session
>     );
> 
>@@ -1306,7 +1306,8 @@ sub checkauth {
>             ));
>         }
> 
>-        track_login_daily( $userid, 'login' );
>+        my $patron = Koha::Patrons->find({ userid => $userid });
>+        $patron->update_lastseen('login');
> 
>         # In case, that this request was a login attempt, we want to prevent that users can repost the opac login
>         # request. We therefore redirect the user to the requested page again without the login parameters.
>@@ -2297,35 +2298,6 @@ sub getborrowernumber {
>     return 0;
> }
> 
>-=head2 track_login_daily
>-
>-    track_login_daily( $userid );
>-
>-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_daily {
>-    my $userid = shift;
>-    my $activity = shift;
>-    return if !$userid || !$activity || !C4::Context->preference('TrackLastPatronActivity');
>-
>-    my $cache     = Koha::Caches->get_instance();
>-    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;
>-
>-    my $tracked_activities = { map { (lc $_, 1); } split /\s*\,\s*/, C4::Context->preference('TrackLastPatronActivityTriggers') };
>-    return unless $tracked_activities->{$activity};
>-
>-    $patron->track_login;
>-    $cache->set_in_cache( $cache_key, $today );
>-}
>-
> END { }    # module clean-up code here (global destructor)
> 1;
> __END__
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 114ec32e7c..65bf5f21fb 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -1676,7 +1676,7 @@ sub AddIssue {
>                 )->store;
>             }
>             $issue->discard_changes;
>-            C4::Auth::track_login_daily( $borrower->{userid}, 'check_out' );
>+            $patron->update_lastseen('check_out');
>             if ( $item_object->location && $item_object->location eq 'CART'
>                 && ( !$item_object->permanent_location || $item_object->permanent_location ne 'CART' ) ) {
>             ## Item was moved to cart via UpdateItemLocationOnCheckin, anything issued should be taken off the cart.
>@@ -2471,7 +2471,7 @@ sub AddReturn {
>             if C4::Context->preference("ReturnLog");
> 
>         #Update borrowers.lastseen
>-        C4::Auth::track_login_daily( $patron->userid, 'check_in' );
>+        $patron->update_lastseen('check_in');
>     }
> 
>     # Check if this item belongs to a biblio record that is attached to an
>@@ -3311,7 +3311,7 @@ sub AddRenewal {
>             }
>         );
>         #Update borrowers.lastseen
>-        C4::Auth::track_login_daily( $patron_unblessed->{userid}, 'renewal' );
>+        $patron->update_lastseen('renewal');
> 
>         #Log the renewal
>         logaction("CIRCULATION", "RENEWAL", $borrowernumber, $itemnumber) if C4::Context->preference("RenewalLog");
>diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm
>index 36cad1269f..a15e694be2 100644
>--- a/C4/ILSDI/Services.pm
>+++ b/C4/ILSDI/Services.pm
>@@ -396,10 +396,10 @@ sub AuthenticatePatron {
>     my $password = $cgi->param('password');
>     my ($status, $cardnumber, $userid) = C4::Auth::checkpw( $username, $password );
>     if ( $status == 1 ) {
>-        # Track the login
>-        C4::Auth::track_login_daily( $userid, 'connection' );
>         # Get the borrower
>         my $patron = Koha::Patrons->find( { userid => $userid } );
>+        # Track the login
>+        $patron->update_lastseen('connection');
>         return { id => $patron->borrowernumber };
>     }
>     elsif ( $status == -2 ){
>diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
>index 6645fcf7ef..0539d287d9 100644
>--- a/C4/SIP/Sip/MsgType.pm
>+++ b/C4/SIP/Sip/MsgType.pm
>@@ -16,6 +16,7 @@ use C4::SIP::Sip::Checksum qw(verify_cksum);
> 
> use Data::Dumper;
> use CGI qw ( -utf8 );
>+use C4::Context;
> use C4::Auth qw(&check_api_auth);
> use C4::Items qw(ModDateLastSeen);
> 
>@@ -993,7 +994,10 @@ sub handle_patron_info {
> 
>     $resp = (PATRON_INFO_RESP);
>     if ($patron) {
>-        C4::Auth::track_login_daily( $patron->userid, 'connection' );
>+        if ( C4::Context->preference('TrackLastPatronActivity') ) {
>+            my $koha_patron = Koha::Patrons->find($patron->userid);
>+            $koha_patron->update_lastseen('connection');
>+        }
>         $resp .= patron_status_string( $patron, $server );
>         $resp .= ( defined($lang) and length($lang) == 3 ) ? $lang : $patron->language;
>         $resp .= timestamp();
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index e20eb6f966..3839ea18df 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -32,6 +32,7 @@ use Koha::Account;
> use Koha::ArticleRequests;
> use C4::Letters qw( GetPreparedLetter EnqueueLetter SendQueuedMessages );
> use Koha::AuthUtils;
>+use Koha::Caches;
> use Koha::Checkouts;
> use Koha::CirculationRules;
> use Koha::Club::Enrollments;
>@@ -998,24 +999,35 @@ sub has_overdues {
>     return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count;
> }
> 
>-=head3 track_login
>+=head3 update_lastseen
> 
>-    $patron->track_login;
>-    $patron->track_login({ force => 1 });
>+  $patron->update_lastseen('activity');
> 
>-    Tracks a (successful) login attempt.
>-    The preference TrackLastPatronActivity must be enabled. Or you
>-    should pass the force parameter.
>+Tracks a (successful) login attempt when the TrackLastPatronActivity preference is enabled
>+and the activity passed is in the TrackLastPatronActivityTriggers list.
> 
> =cut
> 
>-sub track_login {
>-    my ( $self, $params ) = @_;
>-    return if
>-        !$params->{force} &&
>-        !C4::Context->preference('TrackLastPatronActivity');
>+sub update_lastseen {
>+    my ( $self, $activity ) = @_;
>+    return $self if !C4::Context->preference('TrackLastPatronActivity');
>+
>+    my $tracked_activities = {
>+        map { ( lc $_, 1 ); } split /\s*\,\s*/,
>+        C4::Context->preference('TrackLastPatronActivityTriggers')
>+    };
>+    return $self unless $tracked_activities->{$activity};
>+
>+    my $cache     = Koha::Caches->get_instance();
>+    my $cache_key = "track_login_" . $self->userid;
>+    my $cached    = $cache->get_from_cache($cache_key);
>+    my $now       = dt_from_string();
>+    return if $cached && $cached eq $now->ymd;
>+
>     $self->make_column_dirty('updated_on');
>-    $self->lastseen( dt_from_string() )->store;
>+    $self->lastseen($now)->store;
>+    $cache->set_in_cache( $cache_key, $now->ymd );
>+    return $self;
> }
> 
> =head3 move_to_deleted
>-- 
>2.41.0

Actions: View | Diff | Splinter Review

Attachments on bug 15504: 152332 | 152333 | 152334 | 152335 | 152336 | 152500 | 152501 | 152502 | 152503 | 152504 | 152505 | 152506 | 155290 | 155291 | 155292 | 155293 | 155294 | 155295 | 155296 | 155534 | 155535 | 155536 | 155537 | 155538 | 155539 | 155540 | 156598 | 156599 | 156600 | 156601 | 156602 | 156603 | 156604 | 156605 | 156606 | 156607 | 156608 | 156742 | 156855 | 156856 | 156857 | 156858 | 156859 | 156860 | 156861 | 156862 | 156863 | 156864 | 156865 | 156866 | 156867 | 156868 | 156869 | 156870 | 156875 | 156879 | 156898 | 156899 | 156900 | 156901 | 156902 | 156903 | 156904 | 156905 | 156906 | 156907 | 156908 | 156909 | 156910 | 156911 | 156912 | 156913 | 156914 | 156915 | 156917 | 156918 | 156919 | 156920 | 156921 | 156922 | 156923 | 156924 | 156925 | 156926 | 156927 | 156928 | 156929 | 156930 | 156931 | 156932 | 156933 | 156934 | 156935 | 157602 | 157603 | 157604 | 157605 | 157606 | 157607 | 157608 | 157609 | 157610 | 157611 | 157612 | 157613 | 157614 | 157615 | 157616 | 157617 | 157618 | 157619 | 157620 | 157624