From 9fbf1027174904250ee6e0bb03dc3165ff43b734 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 7 May 2025 13:59:54 +0000 Subject: [PATCH] Bug 39452: Add CardnumberLog to track changes to cardnumbers This patch adds a new preference 'CardnumberLog' which allows libraries to track cardnumebr changes for patrons. While these changes are currently captured in the BorrowersLog, some libraries would like to keep cardnumber chanegs specifically, and for a longer period than other borrower modifications. To test: 1 - Apply patches, update database 2 - Ensure BorrowersLog and CardnumberLog are enabled 3 - Edit a borrower, not thier cardnuber 4 - Confirm in Modification Log only one entry is recorded 5 - Edit again, changing cardnumber only 6 - Confirm 2 modifications are recorded, a MODIFY and a MODIFY_CARDNUMBER 7 - Disable BorrowersLog 8 - Edit patron changing something other than cardnumber 9 - Confirm no log generated 10 - Edit borrowers cardnumber 11 - Confirm one MODIFY_CARDNUMBER recorded 12 - Disable CardnumberLog 13 - Edit patrons cardnumber and other fields 14 - Confirm no logs generated Signed-off-by: PhilipOrr Signed-off-by: Martin Renvoize --- Koha/Patron.pm | 17 +++++- installer/data/mysql/mandatory/sysprefs.sql | 1 + .../prog/en/includes/action-logs.inc | 2 + .../en/modules/admin/preferences/logs.pref | 6 ++ .../prog/en/modules/tools/viewlog.tt | 2 +- .../bootstrap/en/modules/opac-detail.tt | 2 +- t/db_dependent/Koha/Patrons.t | 56 +++++++++++++++++-- 7 files changed, 77 insertions(+), 9 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 55e06599574..ce9538b7d4a 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -401,7 +401,7 @@ sub store { } # Actionlogs - if ( C4::Context->preference("BorrowersLog") ) { + if ( C4::Context->preference("BorrowersLog") || C4::Context->preference("CardnumberLog") ) { my $info; my $from_storage = $self_from_storage->unblessed; my $from_object = $self->unblessed; @@ -413,7 +413,8 @@ sub store { if $from_object->{dateexpiry}; my @skip_fields = (qw/lastseen updated_on/); - for my $key ( keys %{$from_storage} ) { + my @keys = C4::Context->preference("BorrowersLog") ? keys %{$from_storage} : ('cardnumber'); + for my $key (@keys) { next if any { /$key/ } @skip_fields; my $storage_value = $from_storage->{$key} // q{}; my $object_value = $from_object->{$key} // q{}; @@ -436,7 +437,17 @@ sub store { $info, { utf8 => 1, pretty => 1, canonical => 1 } ) - ); + ) if C4::Context->preference("BorrowersLog"); + logaction( + "MEMBERS", + "MODIFY_CARDNUMBER", + $self->borrowernumber, + to_json( + $info->{cardnumber}, + { utf8 => 1, pretty => 1, canonical => 1 } + ) + ) if defined $info->{cardnumber} && C4::Context->preference("CardnumberLog"); + } } diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 0083b925c40..9fea00c13b7 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -144,6 +144,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('CanMarkHoldsToPullAsLost','do_not_allow','do_not_allow|allow|allow_and_notify','Add a button to the "Holds to pull" screen to mark an item as lost and notify the patron.','Choice'), ('canreservefromotherbranches','1','','With Independent branches on, can a user from one library place a hold on an item from another library','YesNo'), ('CardnumberLength', '', '', 'Set a length for card numbers with a maximum of 32 characters.', 'Free'), +('CardnumberLog','1',NULL,'If ON, log edit actions on patron cardnumbers','YesNo'), ('casAuthentication','0','','Enable or disable CAS authentication','YesNo'), ('casLogout','0','','Does a logout from Koha should also log the user out of CAS?','YesNo'), ('casServerUrl','https://localhost:8443/cas','','URL of the cas server','Free'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc index 329a721c9fa..dae105f20e4 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc @@ -147,6 +147,8 @@ Overdue [% CASE 'EXPIRE' %] Expire + [% CASE 'MODIFY_CARDNUMBER' %] + Modify cardnumber [% CASE %] [% action | html %] [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref index 46100bb836a..c2598d19a45 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref @@ -12,6 +12,12 @@ Logging: 1: Log 0: "Don't log" - changes to patron records and patron restrictions. + - + - pref: CardnumberLog + choices: + 1: Log + 0: "Don't log" + - changes to patron cardnumbers. - - pref: CataloguingLog choices: diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt index cd6f64b2a3e..672b2db4133 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt @@ -147,7 +147,7 @@ [% END %] - [% FOREACH actx IN [ 'ADD' 'DELETE' 'MODIFY' 'ISSUE' 'RETURN' 'RENEW' 'CREATE' 'CANCEL' 'FILL' 'SUSPEND' 'RESUME' 'ADDCIRCMESSAGE' 'MODCIRCMESSAGE' 'DELCIRCMESSAGE' 'STATUS_CHANGE' 'PATRON_NOTICE' 'CHANGE PASS' 'RESET PASS' 'Run' 'End' 'EDIT_MAPPINGS' 'RESET_MAPPINGS' 'ADD_BASKET' 'MODIFY_BASKET' 'MODIFY_BASKET_HEADER' 'MODIFY_BASKET_USERS' 'CLOSE_BASKET' 'APPROVE_BASKET' 'REOPEN_BASKET' 'CANCEL_ORDER' 'CREATE_ORDER' 'MODIFY_ORDER' 'CREATE_INVOICE_ADJUSTMENT' 'UPDATE_INVOICE_ADJUSTMENT' 'DELETE_INVOICE_ADJUSTMENT' 'RECEIVE_ORDER' 'MODIFY_BUDGET' 'MODIFY_FUND' 'CREATE_FUND' 'DELETE_FUND' 'ACQUISITION CLAIM' 'ACQUISITION ORDER' 'OVERDUE' 'EXPIRE' 'CREATE_RESTRICTION' 'MODIFY_RESTRICTION' 'DELETE_RESTRICTION' ] %] + [% FOREACH actx IN [ 'ADD' 'DELETE' 'MODIFY' 'ISSUE' 'RETURN' 'RENEW' 'CREATE' 'CANCEL' 'FILL' 'SUSPEND' 'RESUME' 'ADDCIRCMESSAGE' 'MODCIRCMESSAGE' 'DELCIRCMESSAGE' 'STATUS_CHANGE' 'PATRON_NOTICE' 'CHANGE PASS' 'RESET PASS' 'Run' 'End' 'EDIT_MAPPINGS' 'RESET_MAPPINGS' 'ADD_BASKET' 'MODIFY_BASKET' 'MODIFY_BASKET_HEADER' 'MODIFY_BASKET_USERS' 'CLOSE_BASKET' 'APPROVE_BASKET' 'REOPEN_BASKET' 'CANCEL_ORDER' 'CREATE_ORDER' 'MODIFY_ORDER' 'CREATE_INVOICE_ADJUSTMENT' 'UPDATE_INVOICE_ADJUSTMENT' 'DELETE_INVOICE_ADJUSTMENT' 'RECEIVE_ORDER' 'MODIFY_BUDGET' 'MODIFY_FUND' 'CREATE_FUND' 'DELETE_FUND' 'ACQUISITION CLAIM' 'ACQUISITION ORDER' 'OVERDUE' 'EXPIRE' 'CREATE_RESTRICTION' 'MODIFY_RESTRICTION' 'DELETE_RESTRICTION' 'MODIFY_CARDNUMBER' ] %] [% IF actions.grep(actx).size %] [% ITEM_RESULT.barcode | html %] [% END %] - [% IF item_level_holds > 0 %] + [% IF item_level_holds > 0 || show_priority %] [% IF holds_count.defined %][% ITEM_RESULT.holds_count | html %][% END %] [% IF ITEM_RESULT.priority %] diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 3fee9fc2dfd..ed7ad7a3516 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -2019,10 +2019,11 @@ $nb_of_patrons = Koha::Patrons->search->count; $retrieved_patron_1->delete; is( Koha::Patrons->search->count, $nb_of_patrons - 1, 'Delete should have deleted the patron' ); -subtest 'BorrowersLog tests' => sub { - plan tests => 5; +subtest 'BorrowersLog and CardnumberLog tests' => sub { + plan tests => 13; - t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + t::lib::Mocks::mock_preference( 'CardnumberLog', 0 ); my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); my $cardnumber = $patron->cardnumber; @@ -2042,7 +2043,8 @@ subtest 'BorrowersLog tests' => sub { ->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); is( scalar @logs, 1, 'With BorrowerLogs and TrackLastPatronActivityTriggers we should not spam the logs' ); - Koha::ActionLogs->search()->delete(); + # Clear the logs for this patron + Koha::ActionLogs->search( { object => $patron->borrowernumber } )->delete(); $patron->get_from_storage(); $patron->set( { debarred => "" } ); $patron->store; @@ -2057,6 +2059,52 @@ subtest 'BorrowersLog tests' => sub { defined $log, "No action log generated where incoming changed column is empty string and value in storage is NULL" ); + + t::lib::Mocks::mock_preference( 'CardnumberLog', 1 ); + $patron->set( { cardnumber => 'TESTCARDNUMBER_1' } )->store; + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); + is( scalar @logs, 1, 'With BorrowersLog, one detailed MODIFY action should be logged for the modification.' ); + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY_CARDNUMBER', object => $patron->borrowernumber } ); + is( + scalar @logs, 1, + 'With CardnumberLog, one detailed MODIFY_CARDNUMBER action should be logged for the modification.' + ); + + t::lib::Mocks::mock_preference( 'BorrowersLog', 0 ); + $patron->set( { cardnumber => 'TESTCARDNUMBER' } )->store; + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); + is( + scalar @logs, 1, + 'With Cardnumberlog and not BorrowersLog, no more detailed MODIFY action should be logged for the modification.' + ); + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY_CARDNUMBER', object => $patron->borrowernumber } ); + is( + scalar @logs, 2, + 'With CardnumberLogs, one more detailed MODIFY_CARDNUMBER action should be logged for the modification.' + ); + $log_info = from_json( $logs[1]->info ); + is( $log_info->{after}, 'TESTCARDNUMBER', 'Got correct new cardnumber' ); + is( $log_info->{before}, 'TESTCARDNUMBER_1', 'Got correct old cardnumber' ); + + t::lib::Mocks::mock_preference( 'CardnumberLog', 0 ); + $patron->set( { cardnumber => 'TESTCARDNUMBER_1' } )->store; + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); + is( + scalar @logs, 1, + 'With neither Cardnumberlog nor BorrowersLog, no more detailed MODIFY action should be logged for the modification.' + ); + @logs = $schema->resultset('ActionLog') + ->search( { module => 'MEMBERS', action => 'MODIFY_CARDNUMBER', object => $patron->borrowernumber } ); + is( + scalar @logs, 2, + 'With neither CardnumberLog nor BorrowersLog, one detailed MODIFY_CARDNUMBER action should be logged for the modification.' + ); + }; $schema->storage->txn_rollback; -- 2.49.0