Bugzilla – Attachment 157743 Details for
Bug 35001
Simplify patron->is_active in light of TrackLastPatronActivityTriggers
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 35001: Simplify ->is_active in light of activity triggers
Bug-35001-Simplify--isactive-in-light-of-activity-.patch (text/plain), 8.16 KB, created by
Marcel de Rooy
on 2023-10-24 13:32:40 UTC
(
hide
)
Description:
Bug 35001: Simplify ->is_active in light of activity triggers
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2023-10-24 13:32:40 UTC
Size:
8.16 KB
patch
obsolete
>From da339b7f1a7d0b2fb87418724aa04078ae36da0d 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 >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 <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 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 621739b433..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 => 9, hours => 23 ); >- 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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=157743">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 35001
:
156971
|
157016
|
157034
|
157035
|
157036
|
157037
| 157743 |
157744