It would be nice if MARC modification templates could work with control fields - specifically to copy control fields to subfields. A MARC control field is a field where the tag is less than 10, e.g. 001.
Created attachment 164575 [details] [review] Bug 36515: Amend MARC modification templates so control fields can be copied to subfields Test plan: 1. Apply patch and restart services 2. Create a MARC modification template to move 001 to 099$a 3. Perform a Batch record modification using your MARC modification template from #1 4. Confirm that the template has successfully moved the 001 control field value to the 099$a subfield Sponsored-by: Education Services Australia SCIS
Created attachment 165924 [details] [review] Bug 36515: Amend MARC modification templates so control fields can be copied to subfields Test plan: 1. Apply patch and restart services 2. Create a MARC modification template with the action: Copy and replace field 001 to 099$a unless 099$a exists 3. Perform a Batch record modification using your MARC modification template from #2 4. Confirm that the template has successfully moved the 001 control field value to the 099$a subfield Sponsored-by: Education Services Australia SCIS
Created attachment 165928 [details] [review] Bug 36515: Amend MARC modification templates so control fields can be copied to subfields Test plan: 1. Apply patch and restart services 2. Create a MARC modification template with the action: Copy and replace field 001 to 099$a unless 099$a exists 3. Perform a Batch record modification using your MARC modification template from #2 4. Confirm that the template has successfully moved the 001 control field value to the 099$a subfield Sponsored-by: Education Services Australia SCIS
Created attachment 165929 [details] [review] Bug 36515: Add unit tests Test plan: 1. Run unit tests ktd --shell prove t/SimpleMARC.t Sponsored-by: Education Services Australia SCIS
Ready for testing.
Created attachment 166026 [details] [review] Bug 36515: Amend MARC modification templates so control fields can be copied to subfields Test plan: 1. Apply patch and restart services 2. Create a MARC modification template with the action: Copy and replace field 001 to 099$a unless 099$a exists 3. Perform a Batch record modification using your MARC modification template from #2 4. Confirm that the template has successfully moved the 001 control field value to the 099$a subfield Sponsored-by: Education Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166027 [details] [review] Bug 36515: Add unit tests Test plan: 1. Run unit tests ktd --shell prove t/SimpleMARC.t Sponsored-by: Education Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Thanks for testing David!
Created attachment 169215 [details] [review] Bug 36515: Add unit tests Test plan: 1. Run unit tests ktd --shell prove t/SimpleMARC.t Sponsored-by: Education Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 169216 [details] [review] Bug 36515: (QA follow-up) Tidy code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
It looks like something happened here and one of the patches was obsoleted. Trying to fix.
Pushed for 24.11! Well done everyone, thank you!
Enhancement will not be included in 24.05.x
Something seems to be wrong with the logic there, I can enter to copy from 035a to 010 (without entering the `a`) subfield, and it doesn't trigger the alert in JS. if ( ( $("#to_field").val() >= 10 && $("#to_subfield").val().length > 0 ) && ( $("#from_field").val() >= 10 && $("#from_subfield").val().length > 0 ) ) { alert( __("Both subfield values should be filled or empty.") ); I believe the checks for `.length > 0` are erroneous here. It doesn't actually check anything, it could only trigger if subfield length was like 1 and 2, but it's impossible, since the HTML already validates it must be of length 1 max. The only possible lengths are 0 or 1, so logically it's impossible for both to be >0 (both be 1) while L1!=L2 in the check above. So that's a regression. Furthermore, in Koha/SimpleMARC.pm, while the logic in function `copy_field` was changed, the corresponding analogous one in `move_field` was not, so now the behavior is inconsistent. It also seems that trying to copy/move subfield to control field (the other way around) is broken.
(In reply to Michał from comment #14) > Something seems to be wrong with the logic there, I can enter to copy from > 035a to 010 (without entering the `a`) subfield, and it doesn't trigger the > alert in JS. > > > if ( ( $("#to_field").val() >= 10 && $("#to_subfield").val().length > > 0 ) && > ( $("#from_field").val() >= 10 && $("#from_subfield").val().length > 0 ) > ) { > alert( __("Both subfield values should be filled or empty.") ); > > > I believe the checks for `.length > 0` are erroneous here. It doesn't > actually check anything, it could only trigger if subfield length was like 1 > and 2, but it's impossible, since the HTML already validates it must be of > length 1 max. The only possible lengths are 0 or 1, so logically it's > impossible for both to be >0 (both be 1) while L1!=L2 in the check above. So > that's a regression. > > Furthermore, in Koha/SimpleMARC.pm, while the logic in function `copy_field` > was changed, the corresponding analogous one in `move_field` was not, so now > the behavior is inconsistent. > > It also seems that trying to copy/move subfield to control field (the other > way around) is broken. Hi Michal, Taking a look at this.
Thanks for quick response. Atm I can't wrap my head around why the other way around copying/moving doesn't work. Btw related bugs are: 22436, 27978 (they're both duplicates of each other that request copying between control/non-control both ways around). Btw I mislooked, it seems the func `copy_and_replace_field` was adjusted, while `copy_field` and `move_field` were not. As for JS side, I would propose such fix: ```js if ( ( $("#to_field").val() < 10 && $("#to_subfield").val().length > 0 ) || ( $("#from_field").val() < 10 && $("#from_subfield").val().length > 0 ) ) { alert( __("Control fields cannot have subfields.") ); return false; } else if ( $("#from_subfield").val().length != $("#to_subfield").val().length ) { alert( __("Both subfield values should be filled or empty.") ); return false; } ```
And then I presume unit tests should have 6 variants total: two ways around x copy/move/copy_and_replace
[sorry for mail spam in advance] By reading the code, it appears that the reason the other way around doesn't work is that extracting the old value works both from subfield and control field, but the function that sets new value - _update_subfield - doesn't seem to handle setting control field value. The code path that we use (but regrettably both would have to be fixed) calls this: my $field = MARC::Field->new( $fieldName, '', '', "$subfieldName" => $values[$i++] ); To fix this, it should probably check if $subfieldName is `( !defined $subfieldName or $subfieldName eq '' )` and then if the $fieldName is `< "010"`, in such case it should instead run: MARC::Field->new( $fieldName, $values[$i++] ) In fact, this is almost exactly what func `add_field` in the same file does: if ( $fieldName > 10 ) { foreach my $value ( @values ) { my $field = MARC::Field->new( $fieldName, '', '', "$subfieldName" => $value ); $record->insert_fields_ordered( $field ); } } else { foreach my $value ( @values ) { my $field = MARC::Field->new( $fieldName, $value ); $record->insert_fields_ordered( $field ); } Turns out `_update_subfield` has a sister `_update_field`, which is why they don't do such a check. So `_copy_move_subfield` has to be changed from: _update_subfield({ record => $record, field => $toFieldName, subfield => $toSubfieldName, values => \@values, dont_erase => $dont_erase }); to have a check that calls `_update_field` instead if it's control. Unfortunately I don't have ktd setup handy next to me right now, so I can't file a fix for the time being.
Hi Michal, Thanks very much for your notes, that makes sense. I think it would be best to create a new separate bug report that depends on this one for follow-up patches. Would you like to do that Michal? I won't have availability to write any follow-ups until mid next-week at the earliest, so please do feel free to go ahead if you would like.