Bug 22245 - MARC modification templates does not allow move or copy control fields
Summary: MARC modification templates does not allow move or copy control fields
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11395
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-31 11:28 UTC by Fridolin SOMERS
Modified: 2020-03-24 15:20 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04, 19.05.09


Attachments
Bug 22245: Allow copy/move from and to control fields (1.51 KB, patch)
2019-02-16 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22245: Allow copy/move from and to control fields (1.57 KB, patch)
2020-01-23 22:33 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22245: Allow copy/move from and to control fields (1.62 KB, patch)
2020-02-14 21:09 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 22245: Allow copy/move from and to control fields (1.69 KB, patch)
2020-02-18 19:11 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2019-01-31 11:28:04 UTC
MARC modification templates does not allow an action with move, copy nor copy and replace when both from and to fields are control fields.
This should be allowed.
Looks like there just some work to do on RegExp use since it calls $field->subfields.
Comment 1 Jonathan Druart 2019-02-16 14:48:14 UTC
Created attachment 85209 [details] [review]
Bug 22245: Allow copy/move from and to control fields

The JS conditions were wrong, we want to allow copy and move between
control fields.

Test plan:
Create a new MARC modification template action using control fields.
Confirm that you are allowed to copy/move a control field to another one
Comment 2 Jonathan Druart 2019-02-16 14:49:21 UTC
See also
  commit 1079f971e3bc659f3053d9608dbde0b856531111
  Bug 11395: Raise an alert if control field is used

    If a control field is used with a copy or move action, the interface
    should block the user in some cases:
    - If a control field is used with a regular field
    - The subfield should be empty
Comment 3 Fridolin SOMERS 2019-02-26 09:45:35 UTC
1)
With this patch, if I set "from" with control field (ie 003) and "to" with a regular field (ie 998$e) :

I get message :
MSG_MMT_SUBFIELDS_MATCH = _("Both subfield values should be filled or empty.");

I think this message is only for regular fields in "from" and "to", so you don't move a entire field to a subfield for example.

2)
This does not allow copy/move of a control field in the subfield of a regular field, ie 005 => 998$d.
This should be allowed ?
What is surely not allowed is : 005 => 998
Comment 4 Jonathan Druart 2019-02-26 16:54:30 UTC
(In reply to Fridolin SOMERS from comment #3)
> 1)
> With this patch, if I set "from" with control field (ie 003) and "to" with a
> regular field (ie 998$e) :
> 
> I get message :
> MSG_MMT_SUBFIELDS_MATCH = _("Both subfield values should be filled or
> empty.");
> 
> I think this message is only for regular fields in "from" and "to", so you
> don't move a entire field to a subfield for example.

I would not add complexity to the JS code, the message is correct even if it could be more exact. You can provide a follow-up if you think it's really needed, I personally think it's not that important.

> 2)
> This does not allow copy/move of a control field in the subfield of a
> regular field, ie 005 => 998$d.
> This should be allowed ?
> What is surely not allowed is : 005 => 998

Maybe, maybe not.
This limitation has been added by commit 1079f971e3bc "Raise an alert if control field is used". Added on bug 11395 comment 36 without explanations. I cannot remember why, maybe you can find it on the BibLibre bug tracker?
Anyway it's for another bug report.
Comment 5 Katrin Fischer 2019-02-26 17:01:34 UTC
> > 2)
> > This does not allow copy/move of a control field in the subfield of a
> > regular field, ie 005 => 998$d.
> > This should be allowed ?

A very common use case would be 001 > 035$a + Adding a prefix in MARC21. So it would be nice if that worked.

> > What is surely not allowed is : 005 => 998

I agree, in this case the 998 should have a subfield. Not sure the standard has a say about this, but a lot of code I think assumed only <10 have no subfields.
Comment 6 Fridolin SOMERS 2019-02-28 16:14:36 UTC
Ah OK, actually control field can not be used with the subfield of a regular field.

I see is not implemented in Koha::SimpleMARC :
https://gitlab.com/koha-community/Koha/blob/f355788b340b19912891fd41c3168bac9c951aff/Koha/SimpleMARC.pm#L130

So this patch is good.

Just a problem :
When using 2 control fields, looks like move or copy with RexExp does not work.
I think its from :
https://gitlab.com/koha-community/Koha/blob/f355788b340b19912891fd41c3168bac9c951aff/Koha/SimpleMARC.pm#L570
Comment 7 Fridolin SOMERS 2019-02-28 16:20:36 UTC
22436(In reply to Katrin Fischer from comment #5)
> > > 2)
> > > This does not allow copy/move of a control field in the subfield of a
> > > regular field, ie 005 => 998$d.
> > > This should be allowed ?
> 
> A very common use case would be 001 > 035$a + Adding a prefix in MARC21. So
> it would be nice if that worked.
> 
> > > What is surely not allowed is : 005 => 998
> 
> I agree, in this case the 998 should have a subfield. Not sure the standard
> has a say about this, but a lot of code I think assumed only <10 have no
> subfields.

I have opened Bug 22436 for that.
Comment 8 Hayley Mapley 2020-01-23 22:33:12 UTC
Created attachment 97847 [details] [review]
Bug 22245: Allow copy/move from and to control fields

The JS conditions were wrong, we want to allow copy and move between
control fields.

Test plan:
Create a new MARC modification template action using control fields.
Confirm that you are allowed to copy/move a control field to another one

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 9 Bouzid Fergani 2020-02-14 21:09:04 UTC
Created attachment 99044 [details] [review]
Bug 22245: Allow copy/move from and to control fields

The JS conditions were wrong, we want to allow copy and move between
control fields.

Test plan:
Create a new MARC modification template action using control fields.
Confirm that you are allowed to copy/move a control field to another one

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 10 Martin Renvoize 2020-02-18 19:11:52 UTC
Created attachment 99218 [details] [review]
Bug 22245: Allow copy/move from and to control fields

The JS conditions were wrong, we want to allow copy and move between
control fields.

Test plan:
Create a new MARC modification template action using control fields.
Confirm that you are allowed to copy/move a control field to another one

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-02-18 19:12:39 UTC
No regressions found and bug opened for followup work I see..

QA scripts are happy and I believe this patch moves us forward.

Passing QA
Comment 12 Martin Renvoize 2020-02-19 11:34:20 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-03-13 00:17:20 UTC
Pushed to 19.11.x branch for 19.11.04
Comment 14 Lucas Gass 2020-03-18 21:32:03 UTC
backported to 19.05.x for 19.05.09