Bugzilla – Attachment 194872 Details for
Bug 40136
Record diff in action logs when modifying a patron
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40136: Populate diff column for patron CREATE, MODIFY and DELETE logs
b166a23.patch (text/plain), 9.64 KB, created by
Martin Renvoize (ashimema)
on 2026-03-06 17:16:13 UTC
(
hide
)
Description:
Bug 40136: Populate diff column for patron CREATE, MODIFY and DELETE logs
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-06 17:16:13 UTC
Size:
9.64 KB
patch
obsolete
>From b166a23db39599fc04fa3a9e4c006650cf80f14a Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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. >--- > 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 960f1d1b375..7af330db6e8 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 <kyle@bywatersolutions.com> >diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t >index ac59a91f522..ed365362440 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.53.0 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40136
:
194872
|
194940
|
195036
|
195037