From 270436023603fbbb79b5af3a126fc12cbe7b2b2e Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 25 Mar 2025 19:54:49 +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 --- Koha/Patron.pm | 17 ++++++-- .../bug_39452_add_cardnumberlog.pl | 19 +++++++++ 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 | 40 +++++++++++++++++-- 8 files changed, 81 insertions(+), 8 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_39452_add_cardnumberlog.pl diff --git a/Koha/Patron.pm b/Koha/Patron.pm index fd7cf85fc21..c7a9d795c79 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -394,7 +394,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; @@ -406,7 +406,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{}; @@ -429,7 +430,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/atomicupdate/bug_39452_add_cardnumberlog.pl b/installer/data/mysql/atomicupdate/bug_39452_add_cardnumberlog.pl new file mode 100755 index 00000000000..a1373ff951b --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_39452_add_cardnumberlog.pl @@ -0,0 +1,19 @@ +use Modern::Perl; +use Koha::Installer::Output qw(say_warning say_success say_info); + +return { + bug_number => "39452", + description => "Add CardnumberLog Preference", + up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + # Do you stuffs here + $dbh->do(q{ + INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('CardnumberLog','1',NULL,'If ON, log edit actions on patron cardnumbers','YesNo') + }); + + say $out "Added new system preference 'CardnumberLog'"; + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 2bf9d0b6486..5f8b4f57ddb 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -142,6 +142,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 73d5a819e63..2a88eee9a8a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc @@ -145,6 +145,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 8b0231098a9..3df0a02d3e9 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 23b9c7b8764..7d26235f377 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 1cb7d7563c6..e1ee1051b6d 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( '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,38 @@ 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 niether CardnumberLog nor BorrowersLog, one detailed MODIFY_CARDNUMBER action should be logged for the modification.' ); + + }; $schema->storage->txn_rollback; -- 2.39.5