Bug 32950 - MARC modification template moving subfield can lose values for repeatable fields
Summary: MARC modification template moving subfield can lose values for repeatable fields
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords: Manual
Depends on:
Blocks:
 
Reported: 2023-02-13 15:48 UTC by Bernard
Modified: 2025-10-23 21:10 UTC (History)
5 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
MARC modification templates now correctly preserve existing values when moving subfields within repeatable fields. Previously, moving subfields could cause data loss or duplication when the source subfield didn't exist in all instances of the repeatable field. **The problem:** When using a MARC modification template to move a subfield within a repeatable field (for example, moving 020$z to 020$a), if some 020 fields had existing $a values but no $z values, those existing $a values would be overwritten or lost. **Example scenario:** Given multiple 020 fields: - 020$a with existing ISBN - 020$a with another existing ISBN - 020$z with cancelled ISBN (to be moved to $a) - 020$z with another cancelled ISBN (to be moved to $a) Previously, when moving 020$z to 020$a, the first two existing 020$a values would be replaced with values from the 020$z fields, causing data loss. **What's fixed:** - Existing subfield values in fields that don't contain the source subfield are now preserved - Source subfield values are only moved to the corresponding target positions in fields that actually contain the source subfield - The move operation correctly removes the source subfields after copying their values - Field order and other subfields are maintained correctly **For cataloguers:** MARC modification template "move" operations now work reliably with repeatable fields. When moving subfields, only the fields that contain the source subfield will be affected, and all other existing values in the repeatable fields will be preserved.
Version(s) released in:
25.11.00,25.05.05
Circulation function:


Attachments
mrc with multiple 020 fields including 020$z (3.47 KB, application/marc)
2023-02-13 15:48 UTC, Bernard
Details
Bug 32950: Add test for moving subfield in repeatable fields (4.06 KB, patch)
2025-09-23 09:51 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Fix moving subfield losing values for repeatable fields (3.01 KB, patch)
2025-09-23 09:51 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Add unit test for move_field with repeatable fields (2.97 KB, patch)
2025-10-01 12:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Fix subfield moving in repeatable MARC fields (4.81 KB, patch)
2025-10-01 12:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Add test for moving subfield in repeatable fields (4.12 KB, patch)
2025-10-01 12:39 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Fix moving subfield losing values for repeatable fields (3.07 KB, patch)
2025-10-01 12:39 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 32950: Add test for moving subfield in repeatable fields (4.12 KB, patch)
2025-10-06 09:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32950: Fix moving subfield losing values for repeatable fields (3.07 KB, patch)
2025-10-06 09:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32950: Remove unecessary DELETE (976 bytes, patch)
2025-10-06 09:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32950: (follow-up) Fix copy_field regression for cross-field operations (3.16 KB, patch)
2025-10-07 16:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernard 2023-02-13 15:48:06 UTC Comment hidden (obsolete)
Comment 1 Bernard 2023-02-13 15:53:31 UTC
In case it helps, I believe the problem occurs at line 616 in Koha/SimpleMARC.pm

    if ( @$field_numbers ) {
        @values = map { $_ <= @values ? $values[ $_ - 1 ] : () } @$field_numbers;
    }

@values is empty after these lines have run.
Comment 2 Martin Renvoize (ashimema) 2025-09-23 09:51:29 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize (ashimema) 2025-09-23 09:51:31 UTC Comment hidden (obsolete)
Comment 4 Bernard 2025-09-24 09:39:32 UTC
Didn't work I'm afraid :(

Trying "move" with option "1st" (instead of all fields) as in your prove test:


020			_a 9781032023175
_q (ebk)
020			_a 9781032023175
020			_a 9781032023175
_q (electronic bk. : EPUB)
020			_a 9781032023175
_q (electronic bk. : EPUB)
020			_a 9781032023175
_q (electronic bk. : PDF)
020			_a 9781032023175
_q (electronic bk. : PDF)
020			_q (hbk.)
_z 9781032023175
_a 9781032023175
020			_q (pbk.)
_z 9780367760380
_a 9781032023175

This is what I get regardless of whether the patch is installed. It seems to copy the first 020$z to all 020 fields and not delete the former either (which you'd expect at least for a move).

Weirdly the prove tests seem to work:


# Subtest: Bug 32950: Moving subfield preserves values in repeatable fields
    1..14
    ok 1 - Should still have 5 020 fields
    ok 2 - First field $a value preserved
    ok 3 - First field $q value preserved
    ok 4 - Second field $a value preserved
    ok 5 - Third field $a value preserved
    ok 6 - Third field $q value preserved
    ok 7 - Fourth field $z moved to $a
    ok 8 - Fourth field $q value preserved
    ok 9 - Fifth field $z moved to $a
    ok 10 - No $z subfields should remain
    ok 11 - No $z subfields should remain
    ok 12 - No $z subfields should remain
    ok 13 - No $z subfields should remain
    ok 14 - No $z subfields should remain
ok 130 - Bug 32950: Moving subfield preserves values in repeatable fields
Comment 5 Bernard 2025-09-27 11:20:29 UTC
Ignore comment 4. I forgot to restart_all so the patch didn't take effect :)

Works as expected.

Move (all) results in:

020			_a 9781003182870
_q (ebk)
020			_a 1003182879
020			_a 9781000407204
_q (electronic bk. : EPUB)
020			_a 1000407209
_q (electronic bk. : EPUB)
020			_a 9781000407167
_q (electronic bk. : PDF)
020			_a 1000407160
_q (electronic bk. : PDF)
020			_q (hbk.)
_a 9781032023175
020			_q (pbk.)
_a 9780367760380

.. which has correctly moved all the subfield z's to a's.

Also tested the other variants (Move 1st, copy (1st/all), copy and replace (1st/all). These work as I'd expect but could use some better documentation or examples in the manual so we know what these do:

1. Move (1st): Copied the 1st 020z it finds and replaced the 1st 020a it finds with that.
2. Move (all): [see above - already tested]
3. Copy (1st): Copied the 1st 020z into the 1st 020a but added it as another "a" instead of replacing the existing "a"
4 Copy (all): Copied the 020a 020z fields to any other 020 that has a z, but leaves the existing "a"'s too.
5. Copy and replace (1st): Copied the 1st 020z to the 1st 020a and overwrote the latter. Functionaly equivalent to Move (1st)
6. Copy and replace (all): Same as Move (all) but it left in place the 2 x "z"s which were copied from
Comment 6 Martin Renvoize (ashimema) 2025-10-01 12:36:34 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize (ashimema) 2025-10-01 12:36:36 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize (ashimema) 2025-10-01 12:39:04 UTC
Created attachment 187186 [details] [review]
Bug 32950: Add test for moving subfield in repeatable fields

This test verifies that when moving subfields between repeatable fields,
existing values in fields that don't contain the source subfield are
preserved.

The test creates multiple 020 fields:
- Some with existing $a values
- Some with $z values to be moved to $a

After applying the move template, it verifies:
- Existing $a values are preserved
- $z values are moved to $a only in fields that had $z
- $z subfields are properly removed after the move

Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk>
Comment 9 Martin Renvoize (ashimema) 2025-10-01 12:39:06 UTC
Created attachment 187187 [details] [review]
Bug 32950: Fix moving subfield losing values for repeatable fields

Moving one subfield to another was failing for repeatable fields where
the field to be moved doesn't exist in the first instance. When moving
subfields within the same field type (e.g. 020$z to 020$a), existing
subfield values in fields that didn't contain the source subfield were
being overwritten with values from other fields.

This patch fixes the issue by determining which fields actually contain
the source subfield and only updating those corresponding target fields,
preserving existing values in fields that don't have the source subfield.

The fix adds logic to _copy_move_subfield to:
1. Use field_exists() to identify which fields have the source subfield
2. Pass these field numbers to _update_subfield for targeted updates
3. Preserve all existing functionality while fixing the data loss bug

Test plan:
1. Create a MARC modification template to move 020$z to 020$a
2. Create a record with multiple 020 fields:
   - Some with existing $a values
   - Some with $z values to be moved
3. Apply the template
4. Verify existing $a values are preserved
5. Verify $z values are moved to $a only in appropriate fields
6. Verify $z subfields are removed after the move

Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk>
Comment 10 Jonathan Druart 2025-10-06 09:30:21 UTC
Created attachment 187450 [details] [review]
Bug 32950: Add test for moving subfield in repeatable fields

This test verifies that when moving subfields between repeatable fields,
existing values in fields that don't contain the source subfield are
preserved.

The test creates multiple 020 fields:
- Some with existing $a values
- Some with $z values to be moved to $a

After applying the move template, it verifies:
- Existing $a values are preserved
- $z values are moved to $a only in fields that had $z
- $z subfields are properly removed after the move

Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk>
Comment 11 Jonathan Druart 2025-10-06 09:30:23 UTC
Created attachment 187451 [details] [review]
Bug 32950: Fix moving subfield losing values for repeatable fields

Moving one subfield to another was failing for repeatable fields where
the field to be moved doesn't exist in the first instance. When moving
subfields within the same field type (e.g. 020$z to 020$a), existing
subfield values in fields that didn't contain the source subfield were
being overwritten with values from other fields.

This patch fixes the issue by determining which fields actually contain
the source subfield and only updating those corresponding target fields,
preserving existing values in fields that don't have the source subfield.

The fix adds logic to _copy_move_subfield to:
1. Use field_exists() to identify which fields have the source subfield
2. Pass these field numbers to _update_subfield for targeted updates
3. Preserve all existing functionality while fixing the data loss bug

Test plan:
1. Create a MARC modification template to move 020$z to 020$a
2. Create a record with multiple 020 fields:
   - Some with existing $a values
   - Some with $z values to be moved
3. Apply the template
4. Verify existing $a values are preserved
5. Verify $z values are moved to $a only in appropriate fields
6. Verify $z subfields are removed after the move

Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk>
Comment 12 Jonathan Druart 2025-10-06 09:30:25 UTC
Created attachment 187452 [details] [review]
Bug 32950: Remove unecessary DELETE
Comment 13 Lucas Gass (lukeg) 2025-10-06 15:16:32 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 14 Lucas Gass (lukeg) 2025-10-06 17:12:30 UTC
This is causing /kohadevbox/koha/t/SimpleMARC.t to fail now:


    # Subtest: copy and replace control field
        1..1
        ok 1 - Copy and replace - Update a subfield with content of control field
    ok 3 - copy and replace control field
    # Looks like you failed 1 test of 3.
not ok 8 - copy_and_replace_field

#   Failed test 'copy_and_replace_field'
#   at /kohadevbox/koha/t/SimpleMARC.t line 1331.
# Subtest: move_field
Comment 15 Martin Renvoize (ashimema) 2025-10-07 16:57:13 UTC
Created attachment 187538 [details] [review]
Bug 32950: (follow-up) Fix copy_field regression for cross-field operations

The bug 32950 fix introduced a regression that broke copy_field operations
when copying between different fields or when copying a subfield to itself
within the same field.

The original fix correctly handled moving subfields between different
subfields within the same field (e.g., 020$z to 020$a) by using
field_exists() to identify which fields have the source subfield.

However, this logic was applied too broadly:

1. Cross-field operations (e.g., 650$a to 651$a): The fix passed source
   field_numbers to _update_subfield for the target field, causing it to
   look for non-existent target field positions. This resulted in
   insert_fields_ordered creating new fields out of order.

2. Same-field, same-subfield operations (e.g., 952$d to 952$d): The fix
   prevented copying to fields that didn't have the source subfield, but
   the original behavior was correct for duplicating a subfield to itself.

This patch restricts the bug 32950 logic to only apply when:
- Source and target fields are the same AND
- Source and target subfields are different

Additionally, it ensures field_numbers are not passed to _update_subfield
when copying between different fields.

Test plan:
1. Run prove t/SimpleMARC.t
2. All tests should pass, including:
   - Cross-field copy operations (650$a to 651$a)
   - Same-field, different-subfield operations (020$z to 020$a)
   - Same-field, same-subfield operations (952$d to 952$d)

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 16 Lucas Gass (lukeg) 2025-10-07 17:02:10 UTC
Thanks for the speedy follow-up Martin! Pushed to main
Comment 17 Paul Derscheid 2025-10-23 21:10:35 UTC
Nice work everyone!

Pushed to 25.05.x