From d58fdfd5661f3306f3e23377d1bda762e561d80b Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 11 Mar 2026 12:49:05 +0000 Subject: [PATCH] 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. --- C4/Log.pm | 4 +- Koha/Patron.pm | 70 +++++++++++++++++++++++++++++--- t/db_dependent/Koha/Patrons.t | 75 ++++++++++++++++++++++++++++++++++- 3 files changed, 142 insertions(+), 7 deletions(-) diff --git a/C4/Log.pm b/C4/Log.pm index 00b7bcc8c53..64e0b70621a 100644 --- a/C4/Log.pm +++ b/C4/Log.pm @@ -142,7 +142,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, @@ -176,6 +176,8 @@ sub logaction { ); } ); + + return $action_log; } =item cronlogaction diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 7ad14075760..f2ac42f7577 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -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. diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index ed365362440..b2ad0aa807f 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -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; @@ -3845,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; -- 2.53.0