Bugzilla – Attachment 195126 Details for
Bug 40136
Record diff in action logs when modifying a patron
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40136: (follow-up) Unify patron log entries when attributes change with patron data
d58fdfd.patch (text/plain), 11.63 KB, created by
Martin Renvoize (ashimema)
on 2026-03-11 13:38:46 UTC
(
hide
)
Description:
Bug 40136: (follow-up) Unify patron log entries when attributes change with patron data
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-11 13:38:46 UTC
Size:
11.63 KB
patch
obsolete
>From d58fdfd5661f3306f3e23377d1bda762e561d80b Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40136
:
194872
|
194940
|
195036
|
195037
|
195046
|
195083
|
195084
|
195085
|
195086
|
195087
| 195126