From b46b614b6ca0e4007e7ed1edc25e8d9c9b64a716 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 11 Oct 2023 18:43:31 +0100 Subject: [PATCH] Bug 15504: Remove PatronLastActivity preference This patch removes the older PatronLastActivity preference opting to replace it by the new PatronLastActivityTriggers preference. The feature can now be disabled by simply not selecting any triggers Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- C4/SIP/Sip/MsgType.pm | 2 +- Koha/Patron.pm | 10 +++++----- ...bug_15504-add-tracklastpatronactivityoptions.pl | 6 ++++++ installer/data/mysql/mandatory/sysprefs.sql | 1 - .../prog/en/modules/admin/preferences/patrons.pref | 8 +------- .../prog/en/modules/tools/cleanborrowers.tt | 6 +++--- misc/cronjobs/delete_patrons.pl | 7 +++---- t/db_dependent/ILSDI_Services.t | 5 ++--- t/db_dependent/Koha/Patron.t | 11 +++++------ t/db_dependent/Koha/Patrons.t | 3 +-- t/db_dependent/Members.t | 14 +++++--------- t/db_dependent/SIP/Message.t | 5 ++--- 12 files changed, 34 insertions(+), 44 deletions(-) diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index aa1ff45190..97ad633e31 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -996,7 +996,7 @@ sub handle_patron_info { $resp = (PATRON_INFO_RESP); if ($patron) { - if ( C4::Context->preference('TrackLastPatronActivity') ) { + if ( C4::Context->preference('TrackLastPatronActivityTriggers') ) { my $koha_patron = Koha::Patrons->find($patron->{borrowernumber}); $koha_patron->update_lastseen('connection'); } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 912d1aca20..ac87e66674 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -843,7 +843,7 @@ sub is_active { return 1 if DateTime->compare( dt_from_string( $self->dateenrolled ), $dt ) > -1; # Last seen? Updated each login when you track patron activity - if ( C4::Context->preference('TrackLastPatronActivity') ) { + if ( C4::Context->preference('TrackLastPatronActivityTriggers') ) { return 1 if DateTime->compare( dt_from_string( $self->lastseen ), $dt ) > -1; } @@ -1170,15 +1170,15 @@ sub _get_overdue_debarred_delay { $patron->update_lastseen('activity'); -Tracks a (successful) login attempt when the TrackLastPatronActivity preference is enabled -and the activity passed is in the TrackLastPatronActivityTriggers list. +Updates the lastseen field, limited to one update per day, whenever the activity passed is +listed in TrackLastPatronActivityTriggers. + +The method should be called upon successful completion of the activity. =cut 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') diff --git a/installer/data/mysql/atomicupdate/bug_15504-add-tracklastpatronactivityoptions.pl b/installer/data/mysql/atomicupdate/bug_15504-add-tracklastpatronactivityoptions.pl index b8daba1c6a..0ef83b0118 100755 --- a/installer/data/mysql/atomicupdate/bug_15504-add-tracklastpatronactivityoptions.pl +++ b/installer/data/mysql/atomicupdate/bug_15504-add-tracklastpatronactivityoptions.pl @@ -18,5 +18,11 @@ return { ); say $out "Added system preference 'TrackLastPatronActivityTriggers'"; + + $dbh->do(q{ + DELETE FROM systempreferences WHERE variable='TrackLastPatronActivity' + }); + + say $out "Removed system preference 'TrackLastPatronActivity'"; }, }; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 89e9e7175f..19ae5a901a 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -753,7 +753,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('TraceCompleteSubfields','0','0','Force subject tracings to only match complete subfields.','YesNo'), ('TraceSubjectSubdivisions','0','1','Create searches on all subdivisions for subject tracings.','YesNo'), ('TrackClicks','0',NULL,'Track links clicked','Integer'), -('TrackLastPatronActivity','0',NULL,'If set, the field borrowers.lastseen will be updated everytime a patron is seen','YesNo'), ('TrackLastPatronActivityTriggers','',NULL,'If set, the field borrowers.lastseen will be updated everytime a patron is does a selected option','multiple'), ('TransfersBlockCirc','1',NULL,'Should the transfer modal block circulation staff from continuing scanning items','YesNo'), ('TransfersMaxDaysWarning','3',NULL,'Define the days before a transfer is suspected of having a problem','Integer'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref index b50fc4c089..2abc79d47c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref @@ -99,13 +99,7 @@ Patrons: 0: "Don't" - charge a fee when a patron changes to a category with an enrollment fee. - - - pref: TrackLastPatronActivity - choices: - 1: Do - 0: "Don't" - - track last patron activity. Use TrackLastPatronActivityTriggers to select which activities should be tracked. - - - - Select which activities should be tracked. The first time each day that one of these activities occurs, borrowers.lastseen will update with the current date and time. + - Select which patron activities should be tracked to signify patron activity. The first time each day that one of these activities occurs, borrowers.lastseen will update with the current date and time. - pref: TrackLastPatronActivityTriggers multiple: login: "Login" diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt index a59b25712d..f07d9669e9 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/cleanborrowers.tt @@ -116,7 +116,7 @@ [% INCLUDE 'date-format.inc' %] - [% IF Koha.Preference('TrackLastPatronActivity') %] + [% IF Koha.Preference('TrackLastPatronActivityTriggers') %]
  • @@ -222,7 +222,7 @@ - [% IF Koha.Preference('TrackLastPatronActivity') %] + [% IF Koha.Preference('TrackLastPatronActivityTriggers') %] [% END %] @@ -299,7 +299,7 @@ */ function checkForm(form) { if((form.checkbox[0].checked)){ - if ( (!form.date1.value) && (!form.borrower_dateexpiry.value) [% IF Koha.Preference('TrackLastPatronActivity') %]&& (!form.borrower_lastseen.value) [% END %]&& (!form.borrower_categorycode.value) && (!form.patron_list_id.value)){ + if ( (!form.date1.value) && (!form.borrower_dateexpiry.value) [% IF Koha.Preference('TrackLastPatronActivityTriggers') %]&& (!form.borrower_lastseen.value) [% END %]&& (!form.borrower_categorycode.value) && (!form.patron_list_id.value)){ alert(_("Please enter at least one criterion for deletion!")); return false; } diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl index 6293bbf442..b9e54f8328 100755 --- a/misc/cronjobs/delete_patrons.pl +++ b/misc/cronjobs/delete_patrons.pl @@ -38,8 +38,8 @@ $not_borrowed_since = dt_from_string( $not_borrowed_since, 'iso' ) $expired_before = dt_from_string( $expired_before, 'iso' ) if $expired_before; -if ( $last_seen and not C4::Context->preference('TrackLastPatronActivity') ) { - pod2usage(q{The --last_seen will be ineffective if TrackLastPatronActivity turned off or there are not TrackLastPatronActivityTriggers defined}); +if ( $last_seen and not C4::Context->preference('TrackLastPatronActivityTriggers') ) { + pod2usage(q{The --last_seen will be ineffective if there are not TrackLastPatronActivityTriggers enabled}); } unless ( $not_borrowed_since or $expired_before or $last_seen or @category_code or $branchcode or $file ) { @@ -185,8 +185,7 @@ Delete patrons with an account expired before this date. Delete patrons who have not been active since this date. -The system preference TrackLastPatronActivity must be enabled and at least one TrackLastPatronActivityTriggers -must be set to use this option. +The system preference TrackLastPatronActivityTriggers must be contain least trigger to use this option. =item B<--category_code> diff --git a/t/db_dependent/ILSDI_Services.t b/t/db_dependent/ILSDI_Services.t index 39fb1d6a8a..1d4a41617a 100755 --- a/t/db_dependent/ILSDI_Services.t +++ b/t/db_dependent/ILSDI_Services.t @@ -47,7 +47,6 @@ subtest 'AuthenticatePatron test' => sub { $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); my $plain_password = 'tomasito'; @@ -71,7 +70,7 @@ subtest 'AuthenticatePatron test' => sub { $query->param( 'username', $borrower->{userid}); $query->param( 'password', $plain_password); - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '' ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', '' ); my $reply = C4::ILSDI::Services::AuthenticatePatron( $query ); is( $reply->{id}, $borrower->{borrowernumber}, "userid and password - Patron authenticated" ); is( $reply->{code}, undef, "Error code undef"); @@ -89,7 +88,7 @@ subtest 'AuthenticatePatron test' => sub { is( $reply->{code}, 'PatronNotFound', "non-existing userid - PatronNotFound" ); is( $reply->{id}, undef, "id undef"); - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); $query->param( 'username', uc( $borrower->{userid} )); $reply = C4::ILSDI::Services::AuthenticatePatron( $query ); my $now = dt_from_string; diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 0139d3825e..26cbd6cf10 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -338,14 +338,14 @@ subtest 'is_active' => sub { $patron->dateenrolled('2020-01-01')->store; # Check lastseen, test days parameter - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', 1 ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'login' ); $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( 'TrackLastPatronActivity', 0 ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', '' ); is( $patron->is_active( { days => 3 } ), 0, 'Pref disabled' ); # Look at holds, test with weeks @@ -1967,7 +1967,6 @@ subtest 'update_lastseen tests' => sub { my $cache_key = "track_activity_" . $patron->borrowernumber; $cache->clear_from_cache($cache_key); - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' ); t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'login,connection,check_in,check_out,renewal' ); is( $patron->lastseen, undef, 'Patron should have not last seen when newly created' ); @@ -1977,7 +1976,7 @@ subtest 'update_lastseen tests' => sub { $patron->update_lastseen('login'); $patron->_result()->discard_changes(); - isnt( $patron->lastseen, undef, 'Patron should have last seen set when TrackLastPatronActivity = 1' ); + isnt( $patron->lastseen, undef, 'Patron should have last seen set when TrackLastPatronActivityTriggers contains values' ); my $last_seen = $patron->lastseen; Time::Fake->offset( $now->epoch + 5 ); @@ -2076,12 +2075,12 @@ subtest 'update_lastseen tests' => sub { isnt( $patron->lastseen, $last_seen, 'Patron last seen should be changed after a renewal if we cleared the cache' ); # Check that the preference takes effect - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '0' ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', '' ); $patron->lastseen(undef)->store; $cache->clear_from_cache($cache_key); $patron->update_lastseen('login'); $patron->_result()->discard_changes(); - is( $patron->lastseen, undef, 'Patron should still have last seen unchanged when TrackLastPatronActivity = 0' ); + is( $patron->lastseen, undef, 'Patron should still have last seen unchanged when TrackLastPatronActivityTriggers is unset' ); Time::Fake->reset; $schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 23a9a2730c..1f32c749fd 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1619,10 +1619,9 @@ subtest 'BorrowersLog tests' => sub { is( scalar @logs, 1, 'With BorrowerLogs, one detailed MODIFY action should be logged for the modification.' ); t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', 1 ); $patron->update_lastseen('connection'); @logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); - is( scalar @logs, 1, 'With BorrowerLogs and TrackLastPatronActivity we should not spam the logs'); + is( scalar @logs, 1, 'With BorrowerLogs and TrackLastPatronActivityTriggers we should not spam the logs'); }; $schema->storage->txn_rollback; diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 8ae93ac7d0..ad679c67a5 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 50; +use Test::More tests => 49; use Test::MockModule; use Test::Exception; @@ -280,7 +280,7 @@ is( scalar(@$patstodel),2,'Borrowers without issues deleted by expiration_date a $patstodel = GetBorrowersToExpunge( {not_borrowed_since => '2016-01-02', patron_list_id => $list1->patron_list_id() } ); is( scalar(@$patstodel),2,'Borrowers without issues deleted by last issue date'); -# Test GetBorrowersToExpunge and TrackLastPatronActivity +# Test GetBorrowersToExpunge and TrackLastPatronActivityTriggers my $new_category = $builder->build_object( { class => 'Koha::Patron::Categories', @@ -312,11 +312,11 @@ $builder->build({ } }); $patstodel = GetBorrowersToExpunge( { category_code => $new_category->categorycode, last_seen => '1999-12-12' }); -is( scalar @$patstodel, 0, 'TrackLastPatronActivity - 0 patrons must be deleted' ); +is( scalar @$patstodel, 0, 'TrackLastPatronActivityTriggers - 0 patrons must be deleted' ); $patstodel = GetBorrowersToExpunge( { category_code => $new_category->categorycode, last_seen => '2016-02-15' }); -is( scalar @$patstodel, 2, 'TrackLastPatronActivity - 2 patrons must be deleted' ); +is( scalar @$patstodel, 2, 'TrackLastPatronActivityTriggers - 2 patrons must be deleted' ); $patstodel = GetBorrowersToExpunge( { category_code => $new_category->categorycode, last_seen => '2016-04-04' }); -is( scalar @$patstodel, 3, 'TrackLastPatronActivity - 3 patrons must be deleted' ); +is( scalar @$patstodel, 3, 'TrackLastPatronActivityTriggers - 3 patrons must be deleted' ); my $patron2 = $builder->build({ source => 'Borrower', value => { @@ -325,10 +325,6 @@ my $patron2 = $builder->build({ } }); t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); -t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '0' ); -Koha::Patrons->find( $patron2->{borrowernumber} )->update_lastseen('connection'); -is( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, undef, 'Lastseen should not be changed' ); -t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' ); Koha::Patrons->find( $patron2->{borrowernumber} )->update_lastseen('connection'); isnt( Koha::Patrons->find( $patron2->{borrowernumber} )->lastseen, undef, 'Lastseen should be changed now' ); diff --git a/t/db_dependent/SIP/Message.t b/t/db_dependent/SIP/Message.t index 0bf33b9438..c64c019e6e 100755 --- a/t/db_dependent/SIP/Message.t +++ b/t/db_dependent/SIP/Message.t @@ -194,7 +194,6 @@ subtest 'Lastseen response' => sub { my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); my $builder = t::lib::TestBuilder->new(); my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; @@ -220,7 +219,7 @@ subtest 'Lastseen response' => sub { my $server = { ils => $mocks->{ils} }; undef $response; - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '' ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', '' ); $msg->handle_patron_info( $server ); isnt( $response, undef, 'At least we got a response.' ); @@ -229,7 +228,7 @@ subtest 'Lastseen response' => sub { $seen_patron = Koha::Patrons->find({ cardnumber => $seen_patron->{cardnumber} }); is( output_pref({str => $seen_patron->lastseen(), dateonly => 1}), output_pref({str => '2001-01-01', dateonly => 1}),'Last seen not updated if not tracking patrons'); undef $response; - t::lib::Mocks::mock_preference( 'TrackLastPatronActivity', '1' ); + t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); $msg->handle_patron_info( $server ); isnt( $response, undef, 'At least we got a response.' ); -- 2.39.5