From 3ca746f4fe5393bac52a7d6311b789d2ef44a8be Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 22 Apr 2026 07:30:51 +0100 Subject: [PATCH] Bug 42032: (follow-up) Use MARC-in-JSON for authority action log diffs Mirrors the bug 42188 main commit for biblios. Replace the custom _marc_record_to_diffable helper with MARC::Record::MiJ's to_mij_structure so AUTHORITIES action log diffs use the same MARC-in-JSON shape as CATALOGUING diffs. Add Koha::Authority::_unblessed_for_log to expose only the auth_header columns that exist independently of the MARC record (authtypecode, origincode, linkid), avoiding duplication of MARC-derived data (heading, authtrees), raw marcxml, and system timestamps. The explicit whitelist also removes the need for the post-hoc empty-field stripping added in a previous follow-up. Test plan: 1. prove t/db_dependent/AuthoritiesMarc.t 2. Enable AuthoritiesLog. Create, edit, and delete an authority via the staff UI, then inspect action_logs.diff in the DB: - _marc contains MiJ shape { leader: "...", fields: [ { tag: {...} }, ... ] } - Changes to heading fields produce diffs on _marc.fields[N].TAG.subfields[0].a - The diff no longer carries a redundant top-level heading entry (it's MARC-derived and now only appears in _marc). 3. prove t/db_dependent/Log.t 4. prove t/db_dependent/Koha/Authority.t 5. prove t/db_dependent/Koha/Biblio.t Signed-off-by: Lisette Scheer Signed-off-by: Lucas Gass --- C4/AuthoritiesMarc.pm | 44 ++++++-------------------- Koha/Authority.pm | 30 ++++++++++++++++++ t/db_dependent/AuthoritiesMarc.t | 53 +++++++++++++++++++++----------- 3 files changed, 74 insertions(+), 53 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 545644f59a..85c022b63b 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -53,6 +53,7 @@ BEGIN { } use MARC::Field; +use MARC::Record::MiJ; use Scalar::Util qw(blessed); use Try::Tiny qw( try catch ); @@ -1537,48 +1538,21 @@ sub _get_authid_subfield { return $field->subfield('9') || $field->subfield('3'); } -=head2 _marc_record_to_diffable - -Return the required fields for the action logs to store a diff - -=cut - -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; -} - =head2 _authority_log_data -Return the data needed by _marc_record_to_diffable +Returns a hashref combining the authority columns that are independent of +the MARC record with a C<_marc> key containing the record in MARC-in-JSON +format, suitable for passing to C as either the C<$infos> or +C<$original> argument. =cut sub _authority_log_data { my ( $authority, $marc_record ) = @_; - my %data = %{ $authority->unblessed }; - delete $data{marcxml}; - for my $key ( keys %data ) { - delete $data{$key} unless defined $data{$key} && $data{$key} ne ''; - } - $data{_marc} = _marc_record_to_diffable($marc_record); - return \%data; + return { + %{ $authority->_unblessed_for_log }, + _marc => ( $marc_record ? $marc_record->to_mij_structure : {} ), + }; } =head2 GetHeaderAuthority diff --git a/Koha/Authority.pm b/Koha/Authority.pm index 5723a55412..b110c871d4 100644 --- a/Koha/Authority.pm +++ b/Koha/Authority.pm @@ -19,6 +19,8 @@ package Koha::Authority; use Modern::Perl; +use Scalar::Util qw( blessed ); + use base qw(Koha::Object); use Koha::Authority::ControlledIndicators; @@ -380,6 +382,34 @@ sub record_strip_nonxml { =head2 Internal methods +=head3 _unblessed_for_log + +Returns a plain hashref of authority column values that are independent of +the attached MARC record, for use in action log diffing. + +Columns derived from MARC (C, C), the raw C +blob, and system-managed timestamps (C, C) +are omitted because they either duplicate MARC data already captured in +the C<_marc> key of the log entry or add no diff signal. Only columns +carrying independent Koha metadata are retained: C, +C, and C. + +=cut + +# auth_header columns that exist independently of the MARC record. +my @_LOG_FIELDS = qw( authtypecode origincode linkid ); + +sub _unblessed_for_log { + my ($self) = @_; + my $full = $self->unblessed; + my %data; + for my $key (@_LOG_FIELDS) { + next unless exists $full->{$key} && defined $full->{$key}; + $data{$key} = blessed( $full->{$key} ) ? "$full->{$key}" : $full->{$key}; + } + return \%data; +} + =head3 _type =cut diff --git a/t/db_dependent/AuthoritiesMarc.t b/t/db_dependent/AuthoritiesMarc.t index ac33398bf7..2a4c66f683 100755 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@ -11,6 +11,7 @@ use Test::MockModule; use Test::Warn; use MARC::Field; use MARC::Record; +use JSON qw( from_json ); use t::lib::Mocks; use t::lib::TestBuilder; @@ -400,8 +401,8 @@ subtest 'DelAuthority() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'Authority action logs include MARC diff' => sub { - plan tests => 12; +subtest 'Authority action logs include MARC-in-JSON diff' => sub { + plan tests => 20; $schema->storage->txn_begin; @@ -409,39 +410,55 @@ subtest 'Authority action logs include MARC diff' => sub { my $auth_type = 'GEOGR_NAME'; - # ADD + # --- 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; + my $add_log = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'ADD' } )->next; + ok( defined $add_log, 'ADD action logged' ); 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 $add_diff = from_json( $add_log->diff ); + my $add_added = $add_diff->{D}{_marc}{A}; + ok( defined $add_added, 'ADD diff contains _marc key' ); + ok( exists $add_added->{leader}, '_marc has leader' ); + ok( ref $add_added->{fields} eq 'ARRAY', '_marc.fields is an array (MiJ shape)' ); + + my ($f151) = grep { exists $_->{'151'} } @{ $add_added->{fields} }; + ok( defined $f151, '151 field present in _marc.fields' ); + is( $f151->{'151'}{subfields}[0]{a}, 'France', '151 $a correct' ); + + ok( !exists $add_diff->{D}{heading}, 'MARC-derived heading column absent from diff' ); + ok( !exists $add_diff->{D}{authtrees}, 'MARC-derived authtrees column absent from diff' ); + + # --- MODIFY --- 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; + my $mod_log = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'MODIFY' } )->next; + ok( defined $mod_log, 'MODIFY action logged' ); ok( defined $mod_log->diff, 'MODIFY: diff column populated' ); - like( $mod_log->diff, qr/_marc/, 'MODIFY: diff contains MARC field data' ); + + my $mod_diff = from_json( $mod_log->diff ); + ok( exists $mod_diff->{D}{_marc}, 'MODIFY diff contains _marc key' ); + ok( !exists $mod_diff->{D}{heading}, 'MARC-derived heading column absent from MODIFY diff' ); + ok( !exists $mod_diff->{D}{authtrees}, 'MARC-derived authtrees column absent from MODIFY diff' ); 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 + # --- 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; + my $del_log = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'DELETE' } )->next; + ok( defined $del_log, 'DELETE action logged' ); ok( defined $del_log->diff, 'DELETE: diff column populated' ); - like( $del_log->diff, qr/_marc/, 'DELETE: diff contains MARC field data' ); + + my $del_diff = from_json( $del_log->diff ); + my $del_removed = $del_diff->{D}{_marc}{R}; + ok( defined $del_removed, 'DELETE diff contains _marc key' ); + ok( ref $del_removed->{fields} eq 'ARRAY', '_marc.fields is an array in DELETE diff' ); $schema->storage->txn_rollback; };