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;