Bug 40136: Populate diff column for patron CREATE, MODIFY and DELETE logs
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 <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This commit is contained in:
Martin Renvoize
parent
9d20ea636a
commit
f83424d3dd
|
|
@ -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>
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue