From d0919ce08c5f4f673cbc09ca06f0d1f736b43e0c Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 12 Oct 2023 14:10:51 +0000 Subject: [PATCH] Bug 35001: Simplify ->is_active in light of activity triggers Content-Type: text/plain; charset=utf-8 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 --- 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 5918e456af..9c268051a4 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 e5b835553b..8863bc5be0 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.30.2