From fd90de9e49dd50b9a08a573d40870e64d06b6e9a Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Thu, 7 Nov 2024 21:14:32 -0300
Subject: [PATCH] Bug 31224: Move item field removal up to $metadata->record

This patch addresse the fact Koha needs to strip out stray item field in
MARC records coming from the DB. While this is not ideal, it has worked
so far, limiting the negative effects of bad data to Koha users.

I put a FIXME because I think it deserves to be revisited colectively at
some point. I filed bug 38406 to track the discussion around this.

The `marcmarcrecord2csv.t` tests cover this behavior and I thought
removing it was not in the scope of this bug.

I decided to move the removal one step up in the call chain so it
applies to all calls to `$metadata->record`, not only those that ask to
include items. This is the right thing to do while we keep this
behavior.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Record/marcrecord2csv.t
=> FAIL: Item information gets extracted from more items than expected
(i.e. the item in the MARC record is not stripped out so 'Withdrawn'
shows 3 times instead of 2).
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Run:
  k$ prove t/db_dependent/Koha/Biblio/Metadata.t
=> SUCCESS: Tests pass! The behavior change in $metadata->record works
as intended!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
---
 Koha/Biblio/Metadata.pm               | 11 ++++++-----
 t/db_dependent/Koha/Biblio/Metadata.t | 26 ++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm
index 1b6854f0bf..ff14735162 100644
--- a/Koha/Biblio/Metadata.pm
+++ b/Koha/Biblio/Metadata.pm
@@ -129,6 +129,12 @@ sub record {
             'Koha::Biblio::Metadata->record called on unhandled format: ' . $format );
     }
 
+    # FIXME: Remove existing items from the MARC record. This should be handled
+    #        at store() time or pre-filtering in {Add|Mod}Biblio. Remove the FIXME
+    #        once we reach some consensus on how to handle this.
+    my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField("items.itemnumber");
+    $record->delete_field( ( $record->field($itemtag) ) );
+
     if ( $embed_items ) {
         $self->_embed_items({ %$params, format => $format, record => $record });
     }
@@ -223,12 +229,7 @@ sub _embed_items {
 
     if ( $format eq 'marcxml' ) {
 
-        # First remove the existing items from the MARC record
         my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
-        foreach my $field ( $record->field($itemtag) ) {
-            $record->delete_field($field);
-        }
-
         my $biblio = Koha::Biblios->find($biblionumber);
 
         my $items = $biblio->items;
diff --git a/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t
index 172fd2bb04..67a09b40ce 100755
--- a/t/db_dependent/Koha/Biblio/Metadata.t
+++ b/t/db_dependent/Koha/Biblio/Metadata.t
@@ -19,6 +19,7 @@ use Modern::Perl;
 
 use Test::More tests => 5;
 use Test::Exception;
+use Test::MockModule;
 use Test::Warn;
 
 use t::lib::TestBuilder;
@@ -36,7 +37,7 @@ my $builder = t::lib::TestBuilder->new;
 
 subtest 'record() tests' => sub {
 
-    plan tests => 9;
+    plan tests => 11;
 
     $schema->storage->txn_begin;
 
@@ -45,12 +46,33 @@ subtest 'record() tests' => sub {
     # Create a valid record
     my $record = MARC::Record->new();
     my $field  = MARC::Field->new( '245', '', '', 'a' => $title );
-    $record->append_fields($field);
+    my $f952_1 = MARC::Field->new(
+        '952', '', '', 0 => '1',
+        y => 'BK',
+        c => 'GEN',
+        d => '2001-06-25',
+    );
+    my $f952_2 = MARC::Field->new(
+        '952', '', '', 0 => '1',
+        y => 'BK',
+        c => 'GEN',
+        d => '2001-06-25',
+    );
+    $record->append_fields( $field, $f952_1, $f952_2 );
     my ($biblio_id) = C4::Biblio::AddBiblio( $record, '' );
 
+    my @fields_952 = $record->field('952');
+    is( scalar @fields_952, 2, 'The record to be inserted contains 2 item fields' );
+
+    my $c4_biblio = Test::MockModule->new('C4::Biblio');
+    $c4_biblio->mock( 'GetMarcFromKohaField', sub { return '952'; } );
+
     my $metadata = Koha::Biblios->find($biblio_id)->metadata;
     my $record2  = $metadata->record;
 
+    @fields_952 = $record2->field('952');
+    is( scalar @fields_952, 0, 'Item fields stripped out then calling $metadata->record' );
+
     is( ref $record2, 'MARC::Record', 'Method record() returned a MARC::Record object' );
     is( $record2->field('245')->subfield("a"),
         $title, 'Title in 245$a matches title from original record object' );
-- 
2.39.5