From 09d64e7701e14ec190889f6d29b53aa9f1bd6da9 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 23 Sep 2025 10:09:31 +0100 Subject: [PATCH] Bug 32950: Fix subfield moving in repeatable MARC fields This patch fixes the issue where moving subfields between repeatable MARC fields could lose or duplicate values due to incorrect logic in the _update_subfield function. The problem was in the handling of multiple values when there were more values than target fields. The original code would add all remaining values to all target fields, causing duplication. Changes made: - Rewrote _update_subfield to handle one-to-one value mapping correctly - Ensures proper ordering when moving between fields - Creates new fields in correct position when needed - Removes excess fields when there are fewer values than existing fields - Maintains backwards compatibility for existing functionality Test plan: 1. Create a MARC record with multiple 020 fields containing both $a and $z subfields 2. Use MARC modification template to move 020$z to 024$a 3. Verify that values are transferred correctly without duplication 4. Verify that the order of values is preserved 5. Run existing SimpleMARC tests to ensure no regression Signed-off-by: Bernard Scaife --- Koha/SimpleMARC.pm | 59 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/Koha/SimpleMARC.pm b/Koha/SimpleMARC.pm index d5c5c791a22..798de7856ee 100644 --- a/Koha/SimpleMARC.pm +++ b/Koha/SimpleMARC.pm @@ -286,23 +286,64 @@ sub _update_subfield { if (@fields) { unless ($dont_erase) { - @values = ( $values[0] ) x scalar(@fields) - if @values == 1; - foreach my $field (@fields) { - $field->update( "$subfieldName" => $values[ $i++ ] ); + + # If we have only one value, apply it to all fields + if ( @values == 1 ) { + foreach my $field (@fields) { + $field->update( "$subfieldName" => $values[0] ); + } + } else { + + # For multiple values, we need to handle this differently to maintain order + # First, update existing fields one-to-one with values + my $values_per_field = scalar(@values) / scalar(@fields); + my $remainder = scalar(@values) % scalar(@fields); + + if ( scalar(@values) <= scalar(@fields) ) { + + # We have fewer or equal values than fields - update sequentially + for my $i ( 0 .. $#values ) { + $fields[$i]->update( "$subfieldName" => $values[$i] ); + } + + # Remove extra fields if we have more fields than values + for my $i ( scalar(@values) .. $#fields ) { + $fields[$i]->delete_subfield( code => $subfieldName ); + + # Remove field entirely if it has no subfields left + $record->delete_field( $fields[$i] ) unless $fields[$i]->subfields(); + } + } else { + + # We have more values than fields + # Update existing fields first + for my $i ( 0 .. $#fields ) { + $fields[$i]->update( "$subfieldName" => $values[$i] ); + } + + # Create new fields for remaining values, inserting them in the right position + for my $i ( scalar(@fields) .. $#values ) { + my $new_field = MARC::Field->new( $fieldName, '', '', "$subfieldName" => $values[$i] ); + + # Insert after the last existing field of this type + my $last_field = $fields[-1]; + $record->insert_fields_after( $last_field, $new_field ); + } + } } - } - if ( $i <= scalar(@values) - 1 ) { + } else { + + # dont_erase is true (copy mode), add values to existing fields without erasing foreach my $field (@fields) { - foreach my $j ( $i .. scalar(@values) - 1 ) { - $field->add_subfields( "$subfieldName" => $values[$j] ); + foreach my $value (@values) { + $field->add_subfields( "$subfieldName" => $value ); } } } } else { ## Field does not exist, create it. foreach my $value (@values) { - my $field = MARC::Field->new( $fieldName, '', '', "$subfieldName" => $values[ $i++ ] ); + my $field = MARC::Field->new( $fieldName, '', '', "$subfieldName" => $value ); $record->insert_fields_ordered($field); } } -- 2.51.0