gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 42032: (follow-up) Eliminate phantom indicator diffs on authority MODIFY 

Authority MODIFY action log diffs were emitting bogus entries for every
field with a default ' ' indicator on every save, producing output like:

    _marc.fields.1.040.ind1   ' '   ' '
    _marc.fields.1.040.ind2   ' '   ' '
    _marc.fields.2.150.ind1   ' '   ' '
    _marc.fields.3.667.ind1   ' '   ' '
    ...

even when only a single subfield was edited.

Root cause: Struct::Diff treats a utf8-flagged ' ' as different from a
non-utf8 ' ' SV, even though Perl's eq returns true. ModAuthority was
sourcing the two sides of the diff from different parse paths:

  - $oldrecord = GetAuthority($authid)
      -> MARC::Record->new_from_xml(...) -> indicators arrive utf8-flagged
  - $record (the "after" side) was the in-memory MARC built by the
      editor from CGI parameters -> indicators are bare byte strings

The XML round-trip through AddAuthority normalises the stored copy, but
the in-memory $record we passed to logaction had not been re-parsed, so
every default indicator looked like a change to Struct::Diff.

Fix: after AddAuthority commits the new state, re-read it via
$new_authority->record and pass that to _authority_log_data. Both sides
of the diff now traverse the same new_from_xml path, utf8 flags match,
and Struct::Diff falls silent on unchanged indicators.

This is the symmetric counterpart of how biblios already work in
C4::Biblio: ModBiblio sources both $original_marc and $updated_marc_log
through Koha::Biblio::Metadata::record_strip_nonxml (which also calls
new_from_xml), so biblios don't hit this.

The deeper asymmetry between in-memory editor MARC and stored MARC
(default indicators only get normalised on the XML round-trip) is left
for a separate bug; this patch keeps the action log diff clean without
touching the storage path.

Test plan:
1. prove t/db_dependent/AuthoritiesMarc.t
2. Enable AuthoritiesLog. Edit any authority via the staff UI making a
   single subfield change. Open Tools -> Log viewer for that authority
   and verify the diff column lists only the changed subfield (plus the
   005 timestamp), with no rows for ind1/ind2 of untouched fields.
3. Re-run the same edit a second time changing nothing else; the diff
   should again be limited to 005 + the touched subfield.

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize 2026-04-28 15:05:30 +01:00 committed by Lucas Gass
parent 9836498611
commit 4cead7f389
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
2 changed files with 41 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
C4/AuthoritiesMarc.pm
View File

@ -821,9 +821,16 @@ sub ModAuthority {
if ( C4::Context->preference("AuthoritiesLog") ) {
my $new_authority = Koha::Authorities->find($authid);
# Re-read the stored MARC so both sides of the diff come from the
# same XML parse path. The in-memory $record carries non-utf8 SVs
# for default indicators while GetAuthority/->record go through
# MARC::Record->new_from_xml which utf8-flags them; Struct::Diff
# treats those as different and emits phantom indicator entries.
my $new_record = eval { $new_authority->record } // $record;
logaction(
"AUTHORITIES", "MODIFY", $authid,
_authority_log_data( $new_authority, $record ),
_authority_log_data( $new_authority, $new_record ),
undef, $original_data
);
}

36
t/db_dependent/AuthoritiesMarc.t
View File

@ -402,7 +402,7 @@ subtest 'DelAuthority() tests' => sub {
};
subtest 'Authority action logs include MARC-in-JSON diff' => sub {
plan tests => 23;
plan tests => 25;
$schema->storage->txn_begin;
@ -412,7 +412,10 @@ subtest 'Authority action logs include MARC-in-JSON diff' => sub {
# --- ADD ---
my $add_record = MARC::Record->new;
$add_record->add_fields( [ '151', ' ', ' ', a => 'France' ] );
$add_record->add_fields(
[ '151', ' ', ' ', a => 'France' ],
[ '670', ' ', ' ', a => 'Source note' ],
);
my $auth_id = AddAuthority( $add_record, undef, $auth_type );
my $add_log = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'ADD' } )->next;
@ -437,8 +440,16 @@ subtest 'Authority action logs include MARC-in-JSON diff' => sub {
ok( !exists $add_diff->{D}{authtrees}, 'MARC-derived authtrees column absent from diff' );
# --- MODIFY ---
# Indicators below are bare ASCII strings (not utf8-flagged), as a CGI
# form would supply them. The stored MARC, once re-read via
# MARC::Record->new_from_xml, comes back utf8-flagged. Without the
# symmetric-reload in ModAuthority, Struct::Diff would emit phantom
# entries for every unchanged indicator.
my $mod_record = MARC::Record->new;
$mod_record->add_fields( [ '151', ' ', ' ', a => 'France (updated)' ] );
$mod_record->add_fields(
[ '151', ' ', ' ', a => 'France (updated)' ],
[ '670', ' ', ' ', a => 'Source note' ],
);
ModAuthority( $auth_id, $mod_record, $auth_type, { skip_merge => 1 } );
my $mod_log = Koha::ActionLogs->search( { object => $auth_id, module => 'AUTHORITIES', action => 'MODIFY' } )->next;
@ -456,6 +467,25 @@ subtest 'Authority action logs include MARC-in-JSON diff' => sub {
like( $mod_log->diff, qr/France/, 'MODIFY: diff captures before value' );
like( $mod_log->diff, qr/France.*updated/s, 'MODIFY: diff captures after value' );
my @changed_tags;
for my $field_entry ( @{ $mod_diff->{D}{_marc}{D}{fields}{D} || [] } ) {
next unless ref $field_entry->{D} eq 'HASH';
push @changed_tags, keys %{ $field_entry->{D} };
}
# 005 (timestamp) updates on every save and is expected; 151 is the
# field we actually edited. Anything else is a phantom (typically
# ind1/ind2 utf8-flag mismatches on untouched fields).
my @phantom_tags = grep { $_ ne '005' && $_ ne '151' } @changed_tags;
is_deeply(
[ sort @phantom_tags ], [],
'MODIFY: untouched fields produce no phantom diff entries'
);
unlike(
$mod_log->diff, qr/Source note/,
'MODIFY: untouched 670 field absent from diff'
);
# --- DELETE ---
DelAuthority( { authid => $auth_id, skip_merge => 1 } );