From 1190945a928b36b5897cc4e8073883fad4e75ba2 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>
---
 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 1b6854f0bf4..ff14735162c 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 172fd2bb041..67a09b40cec 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.47.0