From f4db130ad4f9a956dfee804711bb2134b8905f6c 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. Signed-off-by: Lisette Scheer Signed-off-by: Martin Renvoize --- C4/Log.pm | 4 +- Koha/Patron.pm | 70 ++++++++++++++++++++-- t/db_dependent/Koha/Patrons.t | 106 ++++++++++++++++++++++++++++++---- 3 files changed, 162 insertions(+), 18 deletions(-) diff --git a/C4/Log.pm b/C4/Log.pm index ba3765cf31..f65a5ee3d4 100644 --- a/C4/Log.pm +++ b/C4/Log.pm @@ -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 diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 7ad1407576..f2ac42f757 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 fbea293337..b2ad0aa807 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; @@ -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;