Bug 36515 - Amend MARC modification templates so control fields can be copied to subfields
Summary: Amend MARC modification templates so control fields can be copied to subfields
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Alex Buckley
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-04 03:41 UTC by Alex Buckley
Modified: 2025-03-25 14:00 UTC (History)
5 users (show)

See Also:
GIT URL:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
With this enhancement it's now possible to copy the content of MARC control fields to MARC subfields. Example: copy 001 to 035$a.
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 36515: Amend MARC modification templates so control fields can be copied to subfields (23.41 KB, patch)
2024-04-10 03:36 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 36515: Amend MARC modification templates so control fields can be copied to subfields (23.47 KB, patch)
2024-04-30 18:09 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 36515: Amend MARC modification templates so control fields can be copied to subfields (3.20 KB, patch)
2024-04-30 19:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 36515: Add unit tests (1.76 KB, patch)
2024-04-30 19:09 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 36515: Amend MARC modification templates so control fields can be copied to subfields (3.24 KB, patch)
2024-05-02 00:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 36515: Add unit tests (1.81 KB, patch)
2024-05-02 00:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 36515: Add unit tests (1.86 KB, patch)
2024-07-19 13:00 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 36515: (QA follow-up) Tidy code (2.42 KB, patch)
2024-07-19 13:00 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2024-04-04 03:41:25 UTC
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.
Comment 1 Alex Buckley 2024-04-10 03:36:16 UTC
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
Comment 2 Alex Buckley 2024-04-30 18:09:54 UTC
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
Comment 3 Alex Buckley 2024-04-30 19:08:55 UTC
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
Comment 4 Alex Buckley 2024-04-30 19:09:14 UTC
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
Comment 5 Alex Buckley 2024-04-30 19:10:12 UTC
Ready for testing.
Comment 6 David Nind 2024-05-02 00:19:41 UTC
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>
Comment 7 David Nind 2024-05-02 00:19:43 UTC
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>
Comment 8 Alex Buckley 2024-05-02 12:43:46 UTC
Thanks for testing David!
Comment 9 Kyle M Hall (khall) 2024-07-19 13:00:08 UTC
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>
Comment 10 Kyle M Hall (khall) 2024-07-19 13:00:16 UTC
Created attachment 169216 [details] [review]
Bug 36515: (QA follow-up) Tidy code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Katrin Fischer 2024-08-02 16:05:32 UTC
It looks like something happened here and one of the patches was obsoleted. Trying to fix.
Comment 12 Katrin Fischer 2024-08-02 17:01:20 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 13 Lucas Gass (lukeg) 2024-09-11 16:09:09 UTC
Enhancement will not be included in 24.05.x
Comment 14 Michał 2025-01-29 22:24:50 UTC
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.
Comment 15 Alex Buckley 2025-01-29 22:29:28 UTC
(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.
Comment 16 Michał 2025-01-29 22:36:23 UTC
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;
}
```
Comment 17 Michał 2025-01-29 22:39:05 UTC
And then I presume unit tests should have 6 variants total: two ways around x copy/move/copy_and_replace
Comment 18 Michał 2025-01-29 22:50:00 UTC
[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.
Comment 19 Alex Buckley 2025-01-30 04:49:41 UTC
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.