From d1f62bc6f1bbba3898f1fadf014ff57888c7e2a6 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 6 Mar 2026 17:02:26 +0000 Subject: [PATCH] Bug 40136: Populate diff column for patron CREATE, MODIFY and DELETE logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously MEMBERS/CREATE and MEMBERS/DELETE logged empty info with no diff, and MEMBERS/MODIFY stored a custom {field:{before,after}} JSON in the info column with no diff column entry. This updates all three to use the standard logaction pattern: - CREATE: diff({}, patron_data) — all fields shown as added - MODIFY: diff(old_state, new_state) via Struct::Diff; info column now holds the full updated patron state (consistent with other modules) - DELETE: diff(patron_data, {}) — all fields shown as removed - MODIFY_CARDNUMBER: likewise uses standard pattern with diff column Password, lastseen and updated_on are excluded from all log payloads. DateTime objects (inflated date/datetime columns) are stringified to their ISO representation before JSON serialisation. Tests updated to verify changes via the diff column rather than the former custom info format. Signed-off-by: David Nind --- Koha/Patron.pm | 81 +++++++++++++++++++++++------------ t/db_dependent/Koha/Patrons.t | 18 +++++--- 2 files changed, 66 insertions(+), 33 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 960f1d1b37..7af330db6e 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -22,7 +22,7 @@ use Modern::Perl; use List::MoreUtils qw( any none uniq notall zip6); use JSON qw( to_json ); -use Scalar::Util qw(looks_like_number); +use Scalar::Util qw( blessed looks_like_number ); use Unicode::Normalize qw( NFKD ); use Try::Tiny; use DateTime (); @@ -335,8 +335,10 @@ sub store { $self->discard_changes; $self->update_lastseen('creation'); - logaction( "MEMBERS", "CREATE", $self->borrowernumber, "" ) - if C4::Context->preference("BorrowersLog"); + if ( C4::Context->preference("BorrowersLog") ) { + my $patron_data = $self->_unblessed_for_log; + logaction( "MEMBERS", "CREATE", $self->borrowernumber, $patron_data, undef, $patron_data ); + } } else { #ModMember my $self_from_storage = $self->get_from_storage; @@ -373,18 +375,18 @@ sub store { # Actionlogs if ( C4::Context->preference("BorrowersLog") || C4::Context->preference("CardnumberLog") ) { - my $info; my $from_storage = $self_from_storage->unblessed; my $from_object = $self->unblessed; # Object's dateexpiry is a DateTime object which stringifies to iso8601 datetime, - # but the column in only a date so we need to convert the datetime to just a date + # but the column is only a date so we need to convert the datetime to just a date # to know if it has actually changed. $from_object->{dateexpiry} = dt_from_string( $from_object->{dateexpiry} )->ymd if $from_object->{dateexpiry}; - my @skip_fields = (qw/lastseen updated_on/); + my @skip_fields = (qw/lastseen updated_on password/); my @keys = C4::Context->preference("BorrowersLog") ? keys %{$from_storage} : ('cardnumber'); + my $changed; for my $key (@keys) { next if any { /$key/ } @skip_fields; my $storage_value = $from_storage->{$key} // q{}; @@ -392,33 +394,37 @@ sub store { if ( ( $storage_value || $object_value ) && ( $storage_value ne $object_value ) ) { - $info->{$key} = { - before => $from_storage->{$key}, - after => $from_object->{$key} - }; + $changed->{$key} = 1; } } - if ( defined($info) ) { - logaction( - "MEMBERS", - "MODIFY", - $self->borrowernumber, - to_json( - $info, - { utf8 => 1, pretty => 1, canonical => 1 } - ) - ) if C4::Context->preference("BorrowersLog"); + if ( defined($changed) ) { + my %log_from = map { + my $v = $from_storage->{$_}; + $_ => ( blessed($v) ? "$v" : $v ) + } grep { + my $k = $_; + !any { /$k/ } @skip_fields + } keys %{$from_storage}; + my %log_to = map { + my $v = $from_object->{$_}; + $_ => ( blessed($v) ? "$v" : $v ) + } grep { + my $k = $_; + !any { /$k/ } @skip_fields + } keys %{$from_object}; + + logaction( "MEMBERS", "MODIFY", $self->borrowernumber, \%log_to, undef, \%log_from ) + 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"); - + { cardnumber => $from_object->{cardnumber} }, + undef, + { cardnumber => $from_storage->{cardnumber} } + ) if $changed->{cardnumber} && C4::Context->preference("CardnumberLog"); } } @@ -465,9 +471,12 @@ sub delete { # for patron selfreg $_->delete for Koha::Patron::Modifications->search( { borrowernumber => $self->borrowernumber } )->as_list; + my $patron_data = C4::Context->preference("BorrowersLog") ? $self->_unblessed_for_log : undef; + $self->SUPER::delete; - logaction( "MEMBERS", "DELETE", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); + logaction( "MEMBERS", "DELETE", $self->borrowernumber, $patron_data, undef, $patron_data ) + if C4::Context->preference("BorrowersLog"); } ); return $self; @@ -3667,6 +3676,24 @@ sub _type { return 'Borrower'; } +=head3 _unblessed_for_log + +Returns a plain hashref of patron column values safe for JSON serialisation. +Date/datetime columns inflated to DateTime objects are converted to their +ISO string representation, and sensitive/noisy fields are excluded. + +=cut + +sub _unblessed_for_log { + my ($self) = @_; + my $data = $self->unblessed; + delete @{$data}{qw(password lastseen updated_on)}; + for my $key ( keys %$data ) { + $data->{$key} = "$data->{$key}" if blessed( $data->{$key} ); + } + return $data; +} + =head1 AUTHORS Kyle M Hall diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index ac59a91f52..ed36536244 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -2200,9 +2200,9 @@ subtest 'BorrowersLog and CardnumberLog tests' => sub { my @logs = $schema->resultset('ActionLog') ->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } ); - my $log_info = from_json( $logs[0]->info ); - is( $log_info->{cardnumber}->{after}, 'TESTCARDNUMBER', 'Got correct new cardnumber' ); - is( $log_info->{cardnumber}->{before}, $cardnumber, 'Got correct old cardnumber' ); + my $log_diff = from_json( $logs[0]->diff ); + is( $log_diff->{D}->{cardnumber}->{N}, 'TESTCARDNUMBER', 'Got correct new cardnumber in diff' ); + is( $log_diff->{D}->{cardnumber}->{O}, $cardnumber, 'Got correct old cardnumber in diff' ); is( scalar @logs, 1, 'With BorrowersLog, one detailed MODIFY action should be logged for the modification.' ); t::lib::Mocks::mock_preference( 'TrackLastPatronActivityTriggers', 'connection' ); @@ -2254,9 +2254,15 @@ subtest 'BorrowersLog and CardnumberLog tests' => sub { 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' ); + my $cn_log_diff = from_json( $logs[1]->diff ); + is( + $cn_log_diff->{D}->{cardnumber}->{N}, 'TESTCARDNUMBER', + 'Got correct new cardnumber in MODIFY_CARDNUMBER diff' + ); + is( + $cn_log_diff->{D}->{cardnumber}->{O}, 'TESTCARDNUMBER_1', + 'Got correct old cardnumber in MODIFY_CARDNUMBER diff' + ); t::lib::Mocks::mock_preference( 'CardnumberLog', 0 ); $patron->set( { cardnumber => 'TESTCARDNUMBER_1' } )->store; -- 2.39.5