Attachment 155723 Details for Bug 24480 – Bug 24480: (follow-up) Shift new fields into array and add after all are copied

[patch] Bug 24480: (follow-up) Shift new fields into array and add after all are copied

Bug-24480-follow-up-Shift-new-fields-into-array-an.patch (text/plain), 2.58 KB, created by Phil Ringnalda on 2023-09-15 21:35:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Phil Ringnalda

Created: 2023-09-15 21:35:08 UTC

Size: 2.58 KB

patch

obsolete

>From c4b9279873bfd4c87cab864b34846e6f1882fd38 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Fri, 15 Sep 2023 19:59:57 +0000
>Subject: [PATCH] Bug 24480: (follow-up) Shift new fields into array and add
> after all are copied
>
>The updated code removed the pushing of fields to an array - this caused a bug.
>When multiple fields were copied and replaced and there were more fields added than existed originally we were adding the new field to the end, then removing the first occurence of the original field. If we tried to move 2 650 fields to 651 and the record had no 651 we would:
>- Delete the first 651, there were none, so nothing happened
>- Take the first 650, add it to the end of the 651 group (there were none, so it became the first 651)
>- Delete the first 651, which was the field we just copied
>- Take the second 650 and add it to the end of the 651 group (whihc had none, because we deleted it)
>
>I re-add the line, but do as suggesed by Phil and reverse the order (unshift vs push)
>
>To test:
>1 - Apply other patches
>2 - prove -v t/SimpleMARC.t
>3 - It fails
>4 - Apply this patch
>5 - prove -v t/SimpleMARC.t
>6 - It still fails, but more tests pass (there's another patch to follow)
>
>Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
>---
> Koha/SimpleMARC.pm | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
>diff --git a/Koha/SimpleMARC.pm b/Koha/SimpleMARC.pm
>index d3c05647ff..a77ce9ea9e 100644
>--- a/Koha/SimpleMARC.pm
>+++ b/Koha/SimpleMARC.pm
>@@ -575,13 +575,7 @@ sub _copy_move_field {
>         @from_fields = map { $_ <= @from_fields ? $from_fields[ $_ - 1 ] : () } @$field_numbers;
>     }
> 
>-    if ( $action eq 'replace' ) {
>-        my @to_fields = $record->field( $toFieldName );
>-        for my $to_field ( @to_fields ) {
>-            $record->delete_field( $to_field );
>-        }
>-    }
>-
>+    my @new_fields;
>     for my $from_field ( @from_fields ) {
>         my $new_field = $from_field->clone;
>         $new_field->{_tag} = $toFieldName; # Should be replaced by set_tag, introduced by MARC::Field 2.0.4
>@@ -595,8 +589,15 @@ sub _copy_move_field {
>         if ( $action eq 'move' ) {
>             $record->delete_field( $from_field )
>         }
>-        $record->insert_grouped_field( $new_field );
>+        elsif ( $action eq 'replace' ) {
>+            my @to_fields = $record->field( $toFieldName );
>+            if( @to_fields ) {
>+                $record->delete_field( $to_fields[0] );
>+            }
>+        }
>+        unshift @new_fields, $new_field;
>     }
>+    $record->insert_fields_ordered( @new_fields );
> }
> 
> sub _copy_move_subfield {
>-- 
>2.40.0

Actions: View | Diff | Splinter Review

Attachments on bug 24480: 97695 | 97944 | 97945 | 98975 | 98976 | 106621 | 112311 | 112312 | 150665 | 150666 | 155714 | 155715 | 155716 | 155717 | 155721 | 155722 | 155723 | 155724 | 158130 | 158131 | 158132 | 158133