From 01edef829a2ed6af4f8addf3d75a00c7ff2fcf86 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 9 Mar 2026 18:00:31 +0000 Subject: [PATCH] Bug 42032: Include MARC field changes in AUTHORITIES diff logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AUTHORITIES action log entries for authority operations previously stored minimal data: ADD and DELETE logged just the string "authority", while MODIFY logged a raw MARC-formatted dump of the pre-change record as a plain text string ("authority BEFORE=>..."). None of these populated the diff column. This patch updates AddAuthority, ModAuthority and DelAuthority in C4/AuthoritiesMarc.pm to follow the same pattern introduced for biblios in bug 40135: passing combined hashrefs to logaction so that the diff column is automatically populated with Struct::Diff JSON. The combined hashref contains the auth_header table columns (authtypecode, heading, origincode, etc. — excluding the raw marcxml blob) plus a _marc key built by the new _marc_record_to_diffable helper, which converts a MARC::Record into a tag => array-of-formatted-strings structure readable by the existing diff viewer. A new _authority_log_data helper packages the authority unblessed data and MARC record into the combined hashref, keeping the three call sites concise. For ADD, the logaction is now emitted after the authority record has been fully stored (after _after_authority_action_hooks), so the diff captures the complete saved state. For MODIFY, the before-state is captured from Koha::Authorities before calling AddAuthority, and the after-state is read back once AddAuthority has committed the new data. For DELETE, the MARC is captured before the authority is removed. The diff column for MODIFY will now show both authority table column changes (e.g. heading) and MARC field-level changes (e.g. _marc.151.0) in the existing diff viewer table in the log viewer. Tests are added to verify that ADD, MODIFY, and DELETE all populate the diff column and that _marc content is captured correctly. Signed-off-by: David Nind --- C4/AuthoritiesMarc.pm | 67 +++++++++++++++++++++++++++++--- t/db_dependent/AuthoritiesMarc.t | 49 ++++++++++++++++++++++- 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index c2070f4560..2e55a90bb3 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -685,7 +685,6 @@ sub AddAuthority { )->store(); $authority->discard_changes(); $authid = $authority->authid; - logaction( "AUTHORITIES", "ADD", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog"); } else { $action = 'modify'; $authority = Koha::Authorities->find($authid); @@ -715,6 +714,15 @@ sub AddAuthority { } _after_authority_action_hooks( { action => $action, authority_id => $authid } ); + + if ( $action eq 'create' && C4::Context->preference("AuthoritiesLog") ) { + $authority->discard_changes; + logaction( + "AUTHORITIES", "ADD", $authid, "authority", undef, + _authority_log_data( $authority, $record ) + ); + } + return ($authid); } @@ -744,7 +752,13 @@ sub DelAuthority { Koha::Authority::MergeRequests->search($condition)->delete; merge( { mergefrom => $authid } ) if !$skip_merge; - my $authority = Koha::Authorities->find($authid); + my $authority = Koha::Authorities->find($authid); + my $deleted_marc = eval { $authority->record } if C4::Context->preference("AuthoritiesLog"); + my $deleted_data = + C4::Context->preference("AuthoritiesLog") + ? _authority_log_data( $authority, $deleted_marc ) + : undef; + $schema->txn_do( sub { $authority->move_to_deleted; #FIXME We should define 'move' .. @@ -752,7 +766,8 @@ sub DelAuthority { } ); - logaction( "AUTHORITIES", "DELETE", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog"); + logaction( "AUTHORITIES", "DELETE", $authid, "authority", undef, $deleted_data ) + if C4::Context->preference("AuthoritiesLog"); unless ($skip_record_index) { my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::AUTHORITIES_INDEX } ); $indexer->index_records( $authid, "recordDelete", "authorityserver", undef ); @@ -784,13 +799,26 @@ sub ModAuthority { my $skip_record_index = $params->{skip_record_index} || 0; my $oldrecord = GetAuthority($authid); + my ( $original_data, $original_marc ); + if ( C4::Context->preference("AuthoritiesLog") ) { + my $old_authority = Koha::Authorities->find($authid); + $original_data = _authority_log_data( $old_authority, $oldrecord ); + } #Now rewrite the $record to table with an add $authid = AddAuthority( $record, $authid, $authtypecode, { skip_record_index => $skip_record_index } ); merge( { mergefrom => $authid, MARCfrom => $oldrecord, mergeto => $authid, MARCto => $record } ) if !$params->{skip_merge}; - logaction( "AUTHORITIES", "MODIFY", $authid, "authority BEFORE=>" . $oldrecord->as_formatted ) - if C4::Context->preference("AuthoritiesLog"); + + if ( C4::Context->preference("AuthoritiesLog") ) { + my $new_authority = Koha::Authorities->find($authid); + logaction( + "AUTHORITIES", "MODIFY", $authid, + _authority_log_data( $new_authority, $record ), + undef, $original_data + ); + } + return $authid; } @@ -1495,6 +1523,35 @@ sub _get_authid_subfield { return $field->subfield('9') || $field->subfield('3'); } +sub _marc_record_to_diffable { + my ($record) = @_; + return {} unless defined $record; + my %marc; + for my $field ( $record->fields ) { + my $tag = $field->tag; + if ( $field->is_control_field ) { + $marc{$tag} = $field->data; + } else { + my $ind1 = $field->indicator(1); + my $ind2 = $field->indicator(2); + my $formatted = "$ind1$ind2"; + for my $subfield ( $field->subfields ) { + $formatted .= " \$$subfield->[0] $subfield->[1]"; + } + push @{ $marc{$tag} }, $formatted; + } + } + return \%marc; +} + +sub _authority_log_data { + my ( $authority, $marc_record ) = @_; + my %data = %{ $authority->unblessed }; + delete $data{marcxml}; + $data{_marc} = _marc_record_to_diffable($marc_record); + return \%data; +} + =head2 GetHeaderAuthority $ref= &GetHeaderAuthority( $authid) diff --git a/t/db_dependent/AuthoritiesMarc.t b/t/db_dependent/AuthoritiesMarc.t index e223ddba34..ac33398bf7 100755 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@ -6,7 +6,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 14; +use Test::More tests => 15; use Test::MockModule; use Test::Warn; use MARC::Field; @@ -14,6 +14,7 @@ use MARC::Record; use t::lib::Mocks; use t::lib::TestBuilder; +use Koha::ActionLogs; use Koha::Database; use Koha::Authority::Types; @@ -398,3 +399,49 @@ subtest 'DelAuthority() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'Authority action logs include MARC diff' => sub { + plan tests => 12; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'AuthoritiesLog', 1 ); + + my $auth_type = 'GEOGR_NAME'; + + # ADD + my $add_record = MARC::Record->new; + $add_record->add_fields( [ '151', ' ', ' ', a => 'France' ] ); + my $auth_id = AddAuthority( $add_record, undef, $auth_type ); + + my $add_logs = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'ADD' } ); + is( $add_logs->count, 1, 'ADD action logged' ); + my $add_log = $add_logs->next; + ok( defined $add_log->diff, 'ADD: diff column populated' ); + like( $add_log->diff, qr/_marc/, 'ADD: diff contains MARC field data' ); + like( $add_log->diff, qr/France/, 'ADD: diff contains heading content' ); + + # MODIFY - change the heading + my $mod_record = MARC::Record->new; + $mod_record->add_fields( [ '151', ' ', ' ', a => 'France (updated)' ] ); + ModAuthority( $auth_id, $mod_record, $auth_type, { skip_merge => 1 } ); + + my $mod_logs = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'MODIFY' } ); + is( $mod_logs->count, 1, 'MODIFY action logged' ); + my $mod_log = $mod_logs->next; + ok( defined $mod_log->diff, 'MODIFY: diff column populated' ); + like( $mod_log->diff, qr/_marc/, 'MODIFY: diff contains MARC field data' ); + like( $mod_log->diff, qr/France/, 'MODIFY: diff captures before value' ); + like( $mod_log->diff, qr/France.*updated/s, 'MODIFY: diff captures after value' ); + + # DELETE + DelAuthority( { authid => $auth_id, skip_merge => 1 } ); + + my $del_logs = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'DELETE' } ); + is( $del_logs->count, 1, 'DELETE action logged' ); + my $del_log = $del_logs->next; + ok( defined $del_log->diff, 'DELETE: diff column populated' ); + like( $del_log->diff, qr/_marc/, 'DELETE: diff contains MARC field data' ); + + $schema->storage->txn_rollback; +}; -- 2.39.5