| Summary: | MARC modification template moving subfield can lose values for repeatable fields | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Bernard <bernard.scaife> |
| Component: | Tools | Assignee: | Martin Renvoize (ashimema) <martin.renvoize> |
| Status: | Pushed to stable --- | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | janet.mcgowan, jonathan.druart, lucas, martin.renvoize, nick |
| Version: | Main | Keywords: | Manual |
| Hardware: | All | ||
| OS: | All | ||
| 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
Bug 32950: Add test for moving subfield in repeatable fields Bug 32950: Fix moving subfield losing values for repeatable fields Bug 32950: Add unit test for move_field with repeatable fields Bug 32950: Fix subfield moving in repeatable MARC fields Bug 32950: Add test for moving subfield in repeatable fields Bug 32950: Fix moving subfield losing values for repeatable fields Bug 32950: Add test for moving subfield in repeatable fields Bug 32950: Fix moving subfield losing values for repeatable fields Bug 32950: Remove unecessary DELETE Bug 32950: (follow-up) Fix copy_field regression for cross-field operations |
||
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.
Created attachment 186781 [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 Created attachment 186782 [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 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 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 Created attachment 187184 [details] [review] Bug 32950: Add unit test for move_field with repeatable fields This test reproduces the issue where moving subfields between repeatable MARC fields can lose or duplicate values. The test verifies that when moving 020$z to 024$a fields, the values are correctly transferred in the proper order without duplication. Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk> Created attachment 187185 [details] [review] 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 <bernard.scaife@openfifth.co.uk> 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> 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> 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> 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> Created attachment 187452 [details] [review] Bug 32950: Remove unecessary DELETE Nice work everyone! Pushed to main for 25.11 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
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> Thanks for the speedy follow-up Martin! Pushed to main Nice work everyone! Pushed to 25.05.x |
Created attachment 146586 [details] mrc with multiple 020 fields including 020$z Moving one subfield to another fails for repeatable fields where the field to be moved doesn't exist in the first instance 1. Make marcmodtemplate as follows a. Move every 020 z field to field 020 a 2. Stage marc exchange file (attached) with following in 020 field (note only 7th and 8th have 020z and these are the ones we wish to move to subfield a) =020 \\$a9781003182870$q(ebk) =020 \\$a1003182879 =020 \\$a9781000407204$q(electronic bk. : EPUB) =020 \\$a1000407209$q(electronic bk. : EPUB) =020 \\$a9781000407167$q(electronic bk. : PDF) =020 \\$a1000407160$q(electronic bk. : PDF) =020 \\$z9781032023175$q(hbk.) =020 \\$z9780367760380$q(pbk.) 3. view staged record and observe fields 020 subfield a fields have all gone and subfield z has not been moved, but its value removed: 020 _a _q(ebk) 020 _a 020 _a _q(electronic bk. : EPUB) 020 _a _q(electronic bk. : EPUB) 020 _a _q(electronic bk. : PDF) 020 _a _q(electronic bk. : PDF) 020 _q(hbk.) _a 020 _q(pbk.) _a Note, adding a conditional to marcmodtemplate "only do this if 020 z subfield exists makes no difference.