gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 40136: (follow-up) Unify patron log entries when attributes change with patron data 

Three issues were reported with patron action logging:

1. CREATE followed by extended_attributes() produced two log entries
   (CREATE for patron fields + MODIFY for attributes) instead of one.
2. MODIFY that changed both regular fields and patron attributes produced
   two separate MODIFY entries instead of one unified entry.
3. DELETE log did not include the patron's extended attribute values.

Fix:
- logaction() now explicitly returns the stored Koha::ActionLog object so
  callers can capture the action_id.
- Koha::Patron::store() saves the action_id of the CREATE or MODIFY log it
  writes into $self->{_patron_log_id}.
- Koha::Patron::extended_attributes() setter checks for _patron_log_id. If
  set, it calls _merge_attribute_log() to fold the attribute changes into
  the existing log entry (updating its info and diff JSON) instead of
  creating a new MODIFY entry. This covers both the CREATE and MODIFY cases.
- Koha::Patron::delete() now iterates extended_attributes before deletion
  and adds attribute.CODE keys to the patron_data snapshot, so the DELETE
  diff includes the removed attributes.
- New private helper _merge_attribute_log() decodes an existing action log
  entry's info/diff JSON, merges in the attribute change data using
  Struct::Diff, and saves the updated entry.
- Tests added to t/db_dependent/Koha/Patrons.t covering all three scenarios.

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 2026-03-11 12:49:05 +00:00
parent 7287b091a4
commit f4db130ad4
No known key found for this signature in database
GPG Key ID: 2D86E0EAEC83711B
3 changed files with 162 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
C4/Log.pm
View File

@ -150,7 +150,7 @@ sub logaction {
$infos = encode_json($infos) if ( ref $infos eq 'HASH' );
Koha::ActionLog->new(
my $action_log = Koha::ActionLog->new(
{
timestamp => \'NOW()',
user => $usernumber,
@ -184,6 +184,8 @@ sub logaction {
);
}
);
return $action_log;
}
=item cronlogaction

70
Koha/Patron.pm
View File

@ -20,8 +20,10 @@ package Koha::Patron;
use Modern::Perl;
use JSON qw( encode_json decode_json );
use List::MoreUtils qw( any none uniq notall zip6);
use Scalar::Util qw( blessed looks_like_number );
use Struct::Diff qw( diff );
use Unicode::Normalize qw( NFKD );
use Try::Tiny;
use DateTime ();
@ -32,6 +34,7 @@ use C4::Letters qw( GetPreparedLetter EnqueueLetter SendQueuedMessages );
use C4::Log qw( logaction );
use C4::Scrubber;
use Koha::Account;
use Koha::ActionLogs;
use Koha::ArticleRequests;
use Koha::AuthUtils;
use Koha::Caches;
@ -336,7 +339,9 @@ sub store {
if ( C4::Context->preference("BorrowersLog") ) {
my $patron_data = $self->_unblessed_for_log;
logaction( "MEMBERS", "CREATE", $self->borrowernumber, $patron_data, undef, $patron_data );
my $create_log =
logaction( "MEMBERS", "CREATE", $self->borrowernumber, $patron_data, undef, $patron_data );
$self->{_patron_log_id} = $create_log->action_id;
}
} else { #ModMember
@ -393,8 +398,11 @@ sub store {
my %log_from = map { $_ => $from_storage->{$_} } keys %{$changed};
my %log_to = map { $_ => $from_object->{$_} } keys %{$changed};
logaction( "MEMBERS", "MODIFY", $self->borrowernumber, \%log_to, undef, \%log_from )
if C4::Context->preference("BorrowersLog");
if ( C4::Context->preference("BorrowersLog") ) {
my $modify_log =
logaction( "MEMBERS", "MODIFY", $self->borrowernumber, \%log_to, undef, \%log_from );
$self->{_patron_log_id} = $modify_log->action_id;
}
logaction(
"MEMBERS",
@ -450,7 +458,19 @@ 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;
my $patron_data;
if ( C4::Context->preference("BorrowersLog") ) {
$patron_data = $self->_unblessed_for_log;
for my $attr ( $self->extended_attributes->as_list ) {
my $key = "attribute." . $attr->code;
if ( $attr->type->repeatable ) {
$patron_data->{$key} //= [];
push @{ $patron_data->{$key} }, $attr->attribute;
} else {
$patron_data->{$key} = $attr->attribute;
}
}
}
$self->SUPER::delete;
@ -2681,7 +2701,13 @@ sub extended_attributes {
if ( %{$all_changes} ) {
my %log_from = map { $_ => $all_changes->{$_}->{before} } keys %{$all_changes};
my %log_to = map { $_ => $all_changes->{$_}->{after} } keys %{$all_changes};
logaction( "MEMBERS", "MODIFY", $self->borrowernumber, \%log_to, undef, \%log_from );
if ( my $log_id = delete $self->{_patron_log_id} ) {
$self->_merge_attribute_log( $log_id, \%log_to, \%log_from );
} else {
logaction( "MEMBERS", "MODIFY", $self->borrowernumber, \%log_to, undef, \%log_from );
}
} else {
delete $self->{_patron_log_id};
}
}
);
@ -3652,6 +3678,40 @@ sub _type {
return 'Borrower';
}
=head3 _merge_attribute_log
Merges extended attribute changes into an existing action log entry (identified
by action_id), combining field and attribute changes into a single log entry.
Used to unify CREATE/MODIFY log entries with subsequent attribute changes.
=cut
sub _merge_attribute_log {
my ( $self, $log_id, $log_to, $log_from ) = @_;
my $log = Koha::ActionLogs->find($log_id);
return unless $log;
my $info = {};
eval { $info = decode_json( $log->info ) } if $log->info;
%{$info} = ( %{$info}, %{$log_to} );
my $existing_diff = {};
eval { $existing_diff = decode_json( $log->diff ) } if $log->diff;
my $attr_diff = diff( $log_from, $log_to, noU => 1 );
if ( $attr_diff->{D} ) {
$existing_diff->{D} //= {};
%{ $existing_diff->{D} } = ( %{ $existing_diff->{D} }, %{ $attr_diff->{D} } );
}
$log->set(
{
info => encode_json($info),
diff => encode_json($existing_diff),
}
)->store;
}
=head3 _unblessed_for_log
Returns a plain hashref of patron column values safe for JSON serialisation.

106
t/db_dependent/Koha/Patrons.t
View File

@ -20,7 +20,7 @@
use Modern::Perl;
use Test::NoWarnings;
use Test::More tests => 49;
use Test::More tests => 50;
use Test::Warn;
use Test::Exception;
use Test::MockModule;
@ -502,7 +502,8 @@ subtest 'renew_account' => sub {
);
my $number_of_logs =
$schema->resultset('ActionLog')
->search( { module => 'MEMBERS', action => 'RENEW', object => $retrieved_patron->borrowernumber } )->count;
->search( { module => 'MEMBERS', action => 'RENEW', object => $retrieved_patron->borrowernumber } )
->count;
is( $number_of_logs, 1, 'With BorrowersLog, Koha::Patron->renew_account should have logged' );
t::lib::Mocks::mock_preference( 'BorrowerRenewalPeriodBase', 'now' );
@ -518,7 +519,8 @@ subtest 'renew_account' => sub {
is( dt_from_string($retrieved_expiry_date), $a_year_later, "today + 12 months must be $a_year_later" );
$number_of_logs =
$schema->resultset('ActionLog')
->search( { module => 'MEMBERS', action => 'RENEW', object => $retrieved_patron->borrowernumber } )->count;
->search( { module => 'MEMBERS', action => 'RENEW', object => $retrieved_patron->borrowernumber } )
->count;
is( $number_of_logs, 1, 'Without BorrowersLog, Koha::Patron->renew_account should not have logged' );
t::lib::Mocks::mock_preference( 'BorrowerRenewalPeriodBase', 'combination' );
@ -654,7 +656,8 @@ subtest "delete" => sub {
my $number_of_logs =
$schema->resultset('ActionLog')
->search( { module => 'MEMBERS', action => 'DELETE', object => $patron->borrowernumber } )->count;
->search( { module => 'MEMBERS', action => 'DELETE', object => $patron->borrowernumber } )
->count;
is( $number_of_logs, 1, 'With BorrowersLog, Koha::Patron->delete should have logged' );
# Test deletion with designated fallback owner
@ -727,7 +730,8 @@ subtest 'add_enrolment_fee_if_needed' => sub {
foreach ( keys %{$enrolmentfees} ) {
( Koha::Patron::Categories->find($_)
// $builder->build_object( { class => 'Koha::Patron::Categories', value => { categorycode => $_ } } ) )
->enrolmentfee( $enrolmentfees->{$_} )->store;
->enrolmentfee( $enrolmentfees->{$_} )
->store;
}
my $enrolmentfee_K = $enrolmentfees->{K};
my $enrolmentfee_J = $enrolmentfees->{J};
@ -2654,7 +2658,8 @@ subtest '->set_password' => sub {
my $number_of_logs =
$schema->resultset('ActionLog')
->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )->count;
->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )
->count;
is( $number_of_logs, 0, 'Without BorrowersLog, Koha::Patron->set_password doesn\'t log password changes' );
# Enable logging password changes
@ -3184,8 +3189,8 @@ subtest "search_patrons_to_update_category tests" => sub {
class => 'Koha::Patrons',
value => {
categorycode => $category_code,
dateofbirth =>
$current_dt->clone->subtract( years => $category->upperagelimit + 1 )->subtract( days => 1 )
dateofbirth => $current_dt->clone->subtract( years => $category->upperagelimit + 1 )
->subtract( days => 1 )
->ymd,
}
}
@ -3635,7 +3640,8 @@ subtest 'filter_by_have_permission' => sub {
is_deeply(
[
Koha::Patrons->search( { branchcode => $library->branchcode } )
->filter_by_have_permission('suggestions.suggestions_manage')->get_column('borrowernumber')
->filter_by_have_permission('suggestions.suggestions_manage')
->get_column('borrowernumber')
],
[ $patron_1->borrowernumber, $patron_2->borrowernumber ],
'Superlibrarian and patron with suggestions.suggestions_manage'
@ -3644,7 +3650,8 @@ subtest 'filter_by_have_permission' => sub {
is_deeply(
[
Koha::Patrons->search( { branchcode => $library->branchcode } )
->filter_by_have_permission('acquisition.order_manage')->get_column('borrowernumber')
->filter_by_have_permission('acquisition.order_manage')
->get_column('borrowernumber')
],
[ $patron_1->borrowernumber, $patron_3->borrowernumber ],
'Superlibrarian and patron with acquisition.order_manage'
@ -3653,7 +3660,8 @@ subtest 'filter_by_have_permission' => sub {
is_deeply(
[
Koha::Patrons->search( { branchcode => $library->branchcode } )
->filter_by_have_permission('parameters.manage_cities')->get_column('borrowernumber')
->filter_by_have_permission('parameters.manage_cities')
->get_column('borrowernumber')
],
[ $patron_1->borrowernumber ],
'Only Superlibrarian is returned'
@ -3661,7 +3669,8 @@ subtest 'filter_by_have_permission' => sub {
is_deeply(
[
Koha::Patrons->search( { branchcode => $library->branchcode } )->filter_by_have_permission('suggestions')
Koha::Patrons->search( { branchcode => $library->branchcode } )
->filter_by_have_permission('suggestions')
->get_column('borrowernumber')
],
[ $patron_1->borrowernumber, $patron_2->borrowernumber ],
@ -3836,4 +3845,77 @@ subtest 'check_for_existing_matches' => sub {
$schema->storage->txn_rollback;
};
subtest 'BorrowersLog with extended patron attributes' => sub {
plan tests => 12;
$schema->storage->txn_begin;
t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
t::lib::Mocks::mock_preference( 'ExtendedPatronAttributes', 1 );
my $attr_type = $builder->build_object(
{
class => 'Koha::Patron::Attribute::Types',
value => { repeatable => 0, unique_id => 0 },
}
);
# --- CREATE with attributes: one unified log entry ---
# Must use Koha::Patron->new()->store() (not build_object) to exercise the
# full Koha::Patron::store() code path that sets _patron_log_id.
# Build fresh library/category since the outer transaction was rolled back.
my $test_library = $builder->build_object( { class => 'Koha::Libraries' } );
my $test_category = $builder->build_object( { class => 'Koha::Patron::Categories' } );
my $patron = Koha::Patron->new(
{
cardnumber => 'test_attrs_log_cn',
surname => 'AttrLogTest',
branchcode => $test_library->branchcode,
categorycode => $test_category->categorycode,
}
)->store;
$patron->extended_attributes( [ { code => $attr_type->code, attribute => 'created_value' } ] );
my @all_logs = Koha::ActionLogs->search( { module => 'MEMBERS', object => $patron->borrowernumber } )->as_list;
is( scalar @all_logs, 1, 'CREATE + attribute set produces one log entry (not CREATE + MODIFY)' );
is( $all_logs[0]->action, 'CREATE', 'The single entry is a CREATE action' );
my $create_diff = from_json( $all_logs[0]->diff );
my $attr_key = 'attribute.' . $attr_type->code;
ok( exists $create_diff->{D}->{$attr_key}, 'CREATE diff includes the set attribute' );
is( $create_diff->{D}->{$attr_key}->{N}, 'created_value', 'CREATE diff has correct attribute value' );
# --- MODIFY fields + attributes: one unified log entry ---
$patron->extended_attributes( [ { code => $attr_type->code, attribute => 'original_value' } ] );
Koha::ActionLogs->search( { object => $patron->borrowernumber } )->delete;
$patron = $patron->get_from_storage;
my $new_surname = $patron->surname . '_changed';
$patron->set( { surname => $new_surname } )->store;
$patron->extended_attributes( [ { code => $attr_type->code, attribute => 'changed_value' } ] );
my @modify_logs =
Koha::ActionLogs->search( { module => 'MEMBERS', action => 'MODIFY', object => $patron->borrowernumber } )
->as_list;
is( scalar @modify_logs, 1, 'Field change + attribute change produces one MODIFY log entry' );
my $modify_diff = from_json( $modify_logs[0]->diff );
ok( exists $modify_diff->{D}->{surname}, 'MODIFY diff includes the field change' );
is( $modify_diff->{D}->{surname}->{N}, $new_surname, 'MODIFY diff has correct new field value' );
ok( exists $modify_diff->{D}->{$attr_key}, 'MODIFY diff includes the attribute change' );
is( $modify_diff->{D}->{$attr_key}->{N}, 'changed_value', 'MODIFY diff has correct new attribute value' );
# --- DELETE: log includes attribute data ---
Koha::ActionLogs->search( { object => $patron->borrowernumber } )->delete;
my $borrowernumber = $patron->borrowernumber;
$patron->delete;
my @delete_logs =
Koha::ActionLogs->search( { module => 'MEMBERS', action => 'DELETE', object => $borrowernumber } )->as_list;
is( scalar @delete_logs, 1, 'DELETE produces one log entry' );
my $delete_diff = from_json( $delete_logs[0]->diff );
ok( exists $delete_diff->{D}->{$attr_key}, 'DELETE diff includes attribute data' );
is( $delete_diff->{D}->{$attr_key}->{R}, 'changed_value', 'DELETE diff has correct removed attribute value' );
$schema->storage->txn_rollback;
};
$schema->storage->txn_rollback;