Attachment 157034 Details for Bug 35001 – Bug 35001: Simplify ->is_active in light of activity triggers

[patch] Bug 35001: Simplify ->is_active in light of activity triggers

Bug-35001-Simplify--isactive-in-light-of-activity-.patch (text/plain), 8.11 KB, created by Martin Renvoize (ashimema) on 2023-10-13 08:26:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2023-10-13 08:26:31 UTC

Size: 8.11 KB

patch

obsolete

>From a315e0725f801bdb907f16c85fa42b64c1708955 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 12 Oct 2023 14:10:51 +0000
>Subject: [PATCH] Bug 35001: Simplify ->is_active in light of activity triggers
>
>The fact that we update lastseen for various triggers now, makes
>it a bit easier to let go of those 'hardcoded queries' ;)
>
>Test plan:
>Run t/db_dependent/Koha/Patron.t
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> Koha/Patron.pm               | 47 ++++++----------------
> t/db_dependent/Koha/Patron.t | 78 +++++++++---------------------------
> 2 files changed, 30 insertions(+), 95 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 5918e456af7..9c268051a4f 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -814,11 +814,16 @@ sub is_expired {
> 
> $patron->is_active({ [ since => $date ], [ days|weeks|months|years => $value ] })
> 
>-A patron is considered 'active' if their account did not expire, and has not been anonymized,
>-and patron logged in recently, or placed a hold, article request or borrowed a book recently.
>-A recent enrollment as new patron counts too.
>+A patron is considered 'active' if the following conditions hold:
> 
>-Recently is defined by $date or $value in days, weeks or months. You should
>+    - account did not expire
>+    - account has not been anonymized
>+    - enrollment or lastseen within period specified
>+
>+Note: lastseen is updated for triggers defined in preference
>+TrackLastPatronActivityTriggers. This includes logins, issues, holds, etc.
>+
>+The period to check is defined by $date or $value in days, weeks or months. You should
> pass one of those; otherwise an exception is thrown.
> 
> =cut
>@@ -842,37 +847,9 @@ sub is_active {
>     # Enrollment within this period?
>     return 1 if DateTime->compare( dt_from_string( $self->dateenrolled ), $dt ) > -1;
> 
>-    # Last seen? Updated for tracked patron activities
>-    if ( C4::Context->preference('TrackLastPatronActivityTriggers') ) {
>-        return 1 if DateTime->compare( dt_from_string( $self->lastseen ), $dt ) > -1;
>-    }
>-
>-    # Check holds, issues and article requests
>-    return 1 if $self->holds->filter_by_last_update(
>-        {
>-            timestamp_column_name => 'timestamp', from => $dt,
>-        }
>-    )->count;
>-    return 1 if $self->old_holds->filter_by_last_update(
>-        {
>-            timestamp_column_name => 'timestamp', from => $dt,
>-        }
>-    )->count;
>-    return 1 if $self->checkouts->filter_by_last_update(
>-        {
>-            timestamp_column_name => 'timestamp', from => $dt,
>-        }
>-    )->count;
>-    return 1 if $self->old_checkouts->filter_by_last_update(
>-        {
>-            timestamp_column_name => 'timestamp', from => $dt,
>-        }
>-    )->count;
>-    return 1 if $self->article_requests->filter_by_last_update(
>-        {
>-            timestamp_column_name => 'updated_on', from => $dt,
>-        }
>-    )->count;
>+    # We look at lastseen regardless of TrackLastPatronActivityTriggers. If lastseen is set
>+    # recently, the triggers may have been removed after that, etc.
>+    return 1 if $self->lastseen && DateTime->compare( dt_from_string( $self->lastseen ), $dt ) > -1;
> 
>     return 0;
> }
>diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
>index e5b835553b4..8863bc5be0a 100755
>--- a/t/db_dependent/Koha/Patron.t
>+++ b/t/db_dependent/Koha/Patron.t
>@@ -322,87 +322,45 @@ subtest 'Accessor tests' => sub {
> };
> 
> subtest 'is_active' => sub {
>-    plan tests => 19;
>+    plan tests => 12;
>     $schema->storage->txn_begin;
> 
>     my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
>     throws_ok { $patron->is_active } 'Koha::Exceptions::MissingParameter', 'Called without params';
> 
>     # Check expiry
>-    $patron->dateexpiry( dt_from_string->subtract( days => 1 ) )->store;
>+    $patron->dateexpiry( dt_from_string->subtract( days => 1 ) )->lastseen(undef)->store;
>     is( $patron->is_active( { days => 1 } ), 0, 'Expired patron is not active' );
>     $patron->dateexpiry(undef)->store;
>     is( $patron->is_active( { days => 1 } ), 1, 'Expiry date removed' );
> 
>+    # Check anonymized
>+    $patron->anonymized(1)->store;
>+    is( $patron->is_active( { days => 1 } ), 0, 'Anonymized patron is not active' );
>+    $patron->anonymized(0)->store;
>+
>     # Change enrolled date now
>     $patron->dateenrolled('2020-01-01')->store;
>+    is( $patron->is_active( { days => 1 } ), 0, 'No recent enrollment and lastseen still empty: not active' );
>+    $patron->dateenrolled( dt_from_string() )->store;
>+    is( $patron->is_active( { days => 1 } ), 1, 'Enrolled today: active' );
> 
>     # Check lastseen, test days parameter
>     t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'login' );
>+    $patron->dateenrolled('2020-01-01')->store;
>     $patron->update_lastseen('login');
>     is( $patron->is_active( { days => 1 } ), 1, 'Just logged in' );
>     my $ago = dt_from_string->subtract( days => 2 );
>     $patron->lastseen($ago)->store;
>     is( $patron->is_active( { days => 1 } ), 0, 'Not active since yesterday' );
>     is( $patron->is_active( { days => 3 } ), 1, 'Active within last 3 days' );
>-    t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', '' );
>-    is( $patron->is_active( { days => 3 } ), 0, 'Pref disabled' );
>-
>-    # Look at holds, test with weeks
>-    $ago = dt_from_string->subtract( weeks => 2 );
>-    my $hold = $builder->build_object(
>-        {
>-            class => 'Koha::Holds',
>-            value => { borrowernumber => $patron->id, timestamp => $ago },
>-        }
>-    );
>-    is( $patron->is_active( { weeks => 1 } ), 0, 'No holds in 1 weeks' );
>-    is( $patron->is_active( { weeks => 3 } ), 1, 'Hold in last 3 weeks' );
>-    $hold->delete;
>-    my $old_hold = $builder->build_object(
>-        {
>-            class => 'Koha::Old::Holds',
>-            value => { borrowernumber => $patron->id, timestamp => $ago },
>-        }
>-    );
>-    is( $patron->is_active( { weeks => 1 } ), 0, 'No old holds in 1 weeks' );
>-    is( $patron->is_active( { weeks => 3 } ), 1, 'Old hold in last 3 weeks' );
>-    $old_hold->delete;
>-
>-    # Look at checkouts, test with months
>-    $ago = dt_from_string->subtract( months => 2 );
>-    my $checkout = $builder->build_object(
>-        {
>-            class => 'Koha::Checkouts',
>-            value => { borrowernumber => $patron->id, timestamp => $ago },
>-        }
>-    );
>-    is( $patron->is_active( { months => 1 } ), 0, 'No checkouts in 1 months' );
>-    is( $patron->is_active( { months => 3 } ), 1, 'Checkout in last 3 months' );
>-    $checkout->delete;
>-    my $old_checkout = $builder->build_object(
>-        {
>-            class => 'Koha::Old::Checkouts',
>-            value => { borrowernumber => $patron->id, timestamp => $ago },
>-        }
>-    );
>-    is( $patron->is_active( { months => 1 } ), 0, 'No old checkouts in 1 months' );
>-    is( $patron->is_active( { months => 3 } ), 1, 'Old checkout in last 3 months' );
>-    $old_checkout->delete;
>-
>-    # Look at article_requests, test with since
>-    $ago = dt_from_string->subtract( days => 10 );
>-    my $article_request = $builder->build_object(
>-        {
>-            class => 'Koha::ArticleRequests',
>-            value => { borrowernumber => $patron->id, updated_on => $ago },
>-        }
>-    );
>-    is( $patron->is_active( { days  => 9 } ),                      0, 'No article requests in 9 days' );
>-    is( $patron->is_active( { days  => 10 } ),                     1, 'Article requests in 10 days' );
>-    is( $patron->is_active( { since => $ago } ),                   1, 'Article requests since ago' );
>-    is( $patron->is_active( { since => $ago->add( days => 1 ) } ), 0, 'No article requests since ago + 1 day' );
>-    $article_request->delete;
>+    # test since parameter
>+    my $dt = $ago->clone->add( hours => 1 );
>+    is( $patron->is_active( { since => $dt } ), 0, 'Inactive since ago + 1 hour' );
>+    $dt = $ago->clone->subtract( hours => 1 );
>+    is( $patron->is_active( { since => $dt } ), 1, 'Active since ago - 1 hour' );
>+    # test weeks parameter
>+    is( $patron->is_active( { weeks => 1 } ), 1, 'Active within last week' );
> 
>     $schema->storage->txn_rollback;
> };
>-- 
>2.41.0

Actions: View | Diff | Splinter Review

Attachments on bug 35001: 156971 | 157016 | 157034 | 157035 | 157036 | 157037 | 157743 | 157744