Attachment 156875 Details for Bug 15504 – Bug 15504: Remove PatronLastActivity preference

[patch] Bug 15504: Remove PatronLastActivity preference

Bug-15504-Remove-PatronLastActivity-preference.patch (text/plain), 18.72 KB, created by Martin Renvoize (ashimema) on 2023-10-11 18:38:28 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2023-10-11 18:38:28 UTC

Size: 18.72 KB

patch

obsolete

>From a330227bf3fb89194d70f874b4208d3b815faf43 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>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
>---
> 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                             | 12 ++++--------
> t/db_dependent/SIP/Message.t                         |  5 ++---
> 12 files changed, 33 insertions(+), 43 deletions(-)
>
>diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
>index aa1ff451907..97ad633e318 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 912d1aca209..d1caca21113 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 b8daba1c6aa..0ef83b01187 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 3f1658c5271..17a86c11b6d 100644
>--- a/installer/data/mysql/mandatory/sysprefs.sql
>+++ b/installer/data/mysql/mandatory/sysprefs.sql
>@@ -747,7 +747,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 412669153de..9e80c862652 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
>@@ -93,13 +93,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 a59b25712d2..f07d9669e97 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 @@
>                     <input size="10" id="borrower_dateexpiry" name="borrower_dateexpiry" type="text" class="flatpickr" />
>                     <span class="hint">[% INCLUDE 'date-format.inc' %]</span>
>                 </li>
>-                [% IF Koha.Preference('TrackLastPatronActivity') %]
>+                [% IF Koha.Preference('TrackLastPatronActivityTriggers') %]
>                     <li>
>                         <label for="borrower_lastseen">who have not been active since:</label>
>                         <input size="10" id="borrower_lastseen" name="borrower_lastseen" type="text" class="flatpickr" />
>@@ -222,7 +222,7 @@
>             <input type="hidden" name="not_borrowed_since" value="[% not_borrowed_since | $KohaDates dateformat => 'iso' %]" />
>             <input type="hidden" name="last_issue_date" value="[% last_issue_date | $KohaDates dateformat => 'iso' %]" />
>             <input type="hidden" name="borrower_dateexpiry" value="[% borrower_dateexpiry | $KohaDates dateformat => 'iso' %]" />
>-            [% IF Koha.Preference('TrackLastPatronActivity') %]
>+            [% IF Koha.Preference('TrackLastPatronActivityTriggers') %]
>                 <input type="hidden" name="borrower_lastseen" value="[% borrower_lastseen | $KohaDates dateformat => 'iso' %]" />
>             [% END %]
>             <input type="hidden" name="borrower_categorycode" value="[% borrower_categorycode | html %]" />
>@@ -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 6293bbf4421..b9e54f83280 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 39fb1d6a8a2..1d4a41617a6 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 7a9ffdbe204..02a06df75f9 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 23a9a2730c3..1f32c749fd7 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 9c65ccb8f80..8d5cc1aa148 100755
>--- a/t/db_dependent/Members.t
>+++ b/t/db_dependent/Members.t
>@@ -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
> $dbh->do(q|UPDATE borrowers SET lastseen=NULL|);
> $builder->build({
>     source => 'Borrower',
>@@ -307,11 +307,11 @@ $builder->build({
>     }
> });
> $patstodel = GetBorrowersToExpunge( { 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( { 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( { 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 => {
>@@ -320,10 +320,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 0bf33b9438b..c64c019e6e7 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.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