Bug 11319 - Marc modification templates improvements
Summary: Marc modification templates improvements
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 11413
  Show dependency treegraph
 
Reported: 2013-11-28 11:34 UTC by Jonathan Druart
Modified: 2016-12-05 21:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11319: POC Marc modification templates improvements (20.25 KB, patch)
2013-11-28 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.62 KB, patch)
2013-12-11 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Koha::SimpleMARC should take a hashref for parameters (25.11 KB, patch)
2013-12-11 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add UT for the fields management (37.12 KB, patch)
2013-12-11 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add the field management for Koha::SimpleMARC (18.60 KB, patch)
2013-12-11 16:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add specific UT for nonexistent field/subfield (5.53 KB, patch)
2013-12-11 16:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.62 KB, patch)
2013-12-11 16:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.75 KB, patch)
2014-01-23 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Koha::SimpleMARC should take a hashref for parameters (25.66 KB, patch)
2014-03-03 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add UT for the fields management (37.12 KB, patch)
2014-03-03 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add the field management for Koha::SimpleMARC (18.52 KB, patch)
2014-03-03 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add specific UT for nonexistent field/subfield (5.53 KB, patch)
2014-03-03 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.75 KB, patch)
2014-03-03 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Koha::SimpleMARC should take a hashref for parameters (25.51 KB, patch)
2014-06-19 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add UT for the fields management (37.12 KB, patch)
2014-06-19 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add the field management for Koha::SimpleMARC (18.52 KB, patch)
2014-06-19 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Add specific UT for nonexistent field/subfield (5.53 KB, patch)
2014-06-19 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.75 KB, patch)
2014-06-19 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: Koha::SimpleMARC should take a hashref for parameters (25.58 KB, patch)
2014-07-03 17:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11319: Add UT for the fields management (37.19 KB, patch)
2014-07-03 17:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11319: Add the field management for Koha::SimpleMARC (18.58 KB, patch)
2014-07-03 17:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11319: Add specific UT for nonexistent field/subfield (5.60 KB, patch)
2014-07-03 17:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.81 KB, patch)
2014-07-03 17:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11395: Raise an alert if control field is used (1.84 KB, patch)
2014-09-03 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11319: [QA follow-up] Add test message to MarcModificationTemplates.t (975 bytes, patch)
2014-10-31 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: [QA follow-up] Remove a warning from GetModificationTemplates (1.72 KB, patch)
2014-10-31 09:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: Koha::SimpleMARC should take a hashref for parameters (25.66 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: Add UT for the fields management (37.27 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: Add the field management for Koha::SimpleMARC (18.67 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: Add specific UT for nonexistent field/subfield (5.68 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: Template modifications (3.89 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: [QA follow-up] Add test message to MarcModificationTemplates.t (1.01 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11319: [QA follow-up] Remove a warning from GetModificationTemplates (1.72 KB, patch)
2014-10-31 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-11-28 11:34:29 UTC
Bug 8015 is pushed and I plan to take over bug 5725.
The idea will be to provide a batch modification tools for records (biblios and authorities).
But before, I have to prepare the ground: Koha::SimpleMARC does not split clearly fields and subfields manipulations. The API should provide routines like copy_field and copy_subfield for example.
Later the UI will have to reflect this change.
Comment 1 Jonathan Druart 2013-11-28 11:38:36 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-11-28 11:40:09 UTC
Kyle and Galen, I really would like to have your opinion on this patch please.
Comment 3 Kyle M Hall 2013-12-09 16:31:10 UTC
As a POC, it looks pretty good to me! The only suggestion I would have would be not keep the export for read_field, and modify that sub to check for a value in the subfield value. That is, instead of having read, read_field, and read_subfield, keep all the code in read_field and just do an if/then/else. That way the exported name is still read_field, and thus doesn't change the API.

It's entirely possible for SimpleMARC to be used in custom scripts, so adding to the API is fine, but removing/renaming api subs should be avoided. I know that such an occurence is unlikely at this point, but even in the future it will confuse users to have update_field, copy_field, move_field, read, and delete. It just breaks the naming convention.

The other option would be to remove "_field" from all the subroutines. I have no problem with either choice. Let's just keep the convention standardized!

Other than that, it looks good to me!

Kyle

(In reply to Jonathan Druart from comment #2)
> Kyle and Galen, I really would like to have your opinion on this patch
> please.
Comment 4 Jonathan Druart 2013-12-09 17:13:51 UTC
(In reply to Kyle M Hall from comment #3)

Kyle, Thank you for your feedback.

> As a POC, it looks pretty good to me! The only suggestion I would have would
> be not keep the export for read_field, and modify that sub to check for a
> value in the subfield value. That is, instead of having read, read_field,
> and read_subfield, keep all the code in read_field and just do an
> if/then/else. That way the exported name is still read_field, and thus
> doesn't change the API.

This way, I am afraid that routines like copy_field and update_field will be really big. It's why I think it would be better to split them.

> It's entirely possible for SimpleMARC to be used in custom scripts, so
> adding to the API is fine, but removing/renaming api subs should be avoided.
> I know that such an occurence is unlikely at this point, but even in the
> future it will confuse users to have update_field, copy_field, move_field,
> read, and delete. It just breaks the naming convention.

Yes of course, but as I said, this patch only implements the read and delete routines. The goal is to do the same work for all other subroutines in the SimpleMarc module (update_field, copy_field, move_field).

> The other option would be to remove "_field" from all the subroutines. I
> have no problem with either choice. Let's just keep the convention
> standardized!

Maybe should we only keep a call to delete_field which will call either the private _delete_field or _delete_subfield routines depending on the subfield value. Like that, we would not have to change the API.
Comment 5 Jonathan Druart 2013-12-11 16:41:48 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-12-11 16:42:45 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-12-11 16:42:57 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-12-11 16:43:11 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-12-11 16:43:23 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-12-11 16:43:39 UTC Comment hidden (obsolete)
Comment 11 cedric.vita 2014-01-23 08:53:51 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 11319:
<h1>Something went wrong !</h1>Applying: Bug 11319: Koha::SimpleMARC should take a
hashref for parameters
Applying: Bug 11319: Add UT for the fields management
Applying: Bug 11319: Add the field management for Koha::SimpleMARC
Applying: Bug 11319: Add specific UT for nonexistent field/subfield
Applying: Bug 11319: Template modifications
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging
koha-tmpl/intranet-tmpl/prog/en/modules/tools/marc_modification_templates.tt
CONFLICT (content): Merge conflict in
koha-tmpl/intranet-tmpl/prog/en/modules/tools/marc_modification_templates.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 11319: Template modifications
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 11319 - Marc modification templates improvements

23415 - Bug 11319: Koha::SimpleMARC should take a hashref for parameters
23416 - Bug 11319: Add UT for the fields management
23417 - Bug 11319: Add the field management for Koha::SimpleMARC
23418 - Bug 11319: Add specific UT for nonexistent field/subfield
23419 - Bug 11319: Template modifications

Apply? [(y)es, (n)o, (i)nteractive] Patch left in
/tmp/Bug-11319-Template-modifications-Yo0tWr.patch
Comment 12 Jonathan Druart 2014-01-23 09:18:41 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-01-23 09:19:02 UTC
Fix conflict with bug 11414.
Comment 14 Jonathan Druart 2014-03-03 09:08:29 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-03-03 09:08:38 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-03-03 09:08:43 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-03-03 09:08:48 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-03-03 09:08:53 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-03-03 09:09:50 UTC
Fix conflict with bug 11478.
Comment 20 Jonathan Druart 2014-06-19 10:24:43 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-06-19 10:24:51 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-06-19 10:24:55 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-06-19 10:25:00 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-06-19 10:25:04 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-06-19 10:25:53 UTC
Last patch set fixes silly error introduced by previous rebase (c/p error).
Comment 26 Kyle M Hall 2014-07-03 17:23:44 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-07-03 17:32:39 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-07-03 17:32:43 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2014-07-03 17:32:45 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2014-07-03 17:32:48 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2014-07-28 11:58:34 UTC
Planning QA for this week
Comment 32 Marcel de Rooy 2014-07-28 11:59:57 UTC
Planning to QA this after 11319
Comment 33 Marcel de Rooy 2014-07-28 12:00:27 UTC
(In reply to M. de Rooy from comment #32)
> Planning to QA this after 11319

Somehow a backspace got in between :)
Comment 34 Marcel de Rooy 2014-07-30 15:04:58 UTC
Started QA on this one; will have some comments tomorrow.
Comment 35 Marcel de Rooy 2014-07-31 11:06:03 UTC
QA Comment: 
In general these patches look good to me. I have the impression that this feature still needs some work however.
Here are some comments to start with:

[1] The last patch (template mods) adds the restriction: A move/copy/update should have a subfield. If I want to update control field 003 to some text, it should be possible. There is no subfield. (Blocker.) 
It also says: move/copy source and dest should have either a subfield or not. I agree that it is normal behavior. But suppose that I want to copy some field to a controlfield? This is somewhat unlikely, but it would not be wrong. (No blocker since I do not have a real application for it at hand.) 

[2] Some code in the third patch for update_field: 
+    if ( not $subfieldName or $subfieldName eq '' ) {
+        # FIXME I'm not sure the actual implementation is correct.
+        die "This action is not implemented yet";
+        #_update_field({ record => $record, field => $fieldName, values => \@values });
This actually is the situation I was referring to (I want to update a control field without subfield).
At this moment you are blocking an update without subfield in the interface. This makes it hard to reach this die statement..

[3] Just looking at some code in SimpleMarc.pm, I notice the following:
field_equals just picks the first field value and runs the regex. If I do not pass a subfield, it does not run the regex over the whole field just one subfield. If I pass a subfield and the record contains repeated subfields, it just checks the first one. This is not the behavior that I would (intuitively) expect from the interface. Note that your patch does not really touch that part of code, but you do touch the routine. Is improving that functionality not even more important than passing a hashref? 

[4] BTW See some warnings in the log:
[Wed Jul 30 16:37:51 2014] stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at Template/Provider.pm line 964., referer: /cgi-bin/koha/tools/stage-marc-import.pl
[Wed Jul 30 16:37:51 2014] stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at Template/Provider.pm line 964., referer: /cgi-bin/koha/tools/stage-marc-import.pl
[Wed Jul 30 16:53:18 2014] marc_modification_templates.pl: Use of uninitialized value in string eq at tools/marc_modification_templates.pl line 70., referer: /cgi-bin/koha/tools/marc_modification_templates.pl?template_id=3&op=delete_action&mmta_id=8
[Wed Jul 30 16:53:31 2014] marc_modification_templates.pl: Use of uninitialized value $template_id in string eq at C4/MarcModificationTemplates.pm line 84., referer: /cgi-bin/koha/tools/marc_modification_templates.pl

Setting status to Failed QA
Comment 36 Jonathan Druart 2014-08-19 09:26:12 UTC
(In reply to M. de Rooy from comment #35)
> QA Comment: 

Marcel,
Thanks a lot to had a look at this.

> In general these patches look good to me. I have the impression that this
> feature still needs some work however.
> Here are some comments to start with:
> 
> [1] The last patch (template mods) adds the restriction: A move/copy/update
> should have a subfield. 
> [...]
> [2]
> At this moment you are blocking an update without subfield in the interface.
> This makes it hard to reach this die statement..

It is more or less intended.
All changes done on these patches (and linked) are not trivial and I don't want to add complexity for now.
This limitation will exist until the entire feature will be pushed.
(Not enough time at the moment to work on it without knowing when it will be pushed...).

> [3] Just looking at some code in SimpleMarc.pm, I notice the following:
> field_equals just picks the first field value and runs the regex. If I do
> not pass a subfield, it does not run the regex over the whole field just one
> subfield. If I pass a subfield and the record contains repeated subfields,
> it just checks the first one. This is not the behavior that I would
> (intuitively) expect from the interface. Note that your patch does not
> really touch that part of code, but you do touch the routine. Is improving
> that functionality not even more important than passing a hashref? 

Could you please retest with *all* patches applied?
Actually I split patches into different bug reports but all patches should be QA at once (patches from bug 11413 and bug 11395).
I think this is fixed by the patch "Bug 11413: Change the field number logic".

> [4] BTW See some warnings in the log:
> [Wed Jul 30 16:37:51 2014] stage-marc-import.pl: Filehandle STDOUT reopened
> as FH only for input at Template/Provider.pm line 964., referer:
> /cgi-bin/koha/tools/stage-marc-import.pl
> [Wed Jul 30 16:37:51 2014] stage-marc-import.pl: Filehandle STDOUT reopened
> as FH only for input at Template/Provider.pm line 964., referer:
> /cgi-bin/koha/tools/stage-marc-import.pl
> [Wed Jul 30 16:53:18 2014] marc_modification_templates.pl: Use of
> uninitialized value in string eq at tools/marc_modification_templates.pl
> line 70., referer:
> /cgi-bin/koha/tools/marc_modification_templates.
> pl?template_id=3&op=delete_action&mmta_id=8
> [Wed Jul 30 16:53:31 2014] marc_modification_templates.pl: Use of
> uninitialized value $template_id in string eq at
> C4/MarcModificationTemplates.pm line 84., referer:
> /cgi-bin/koha/tools/marc_modification_templates.pl

These warnings existed before this patch set isn't it?

I can open another bug report to get rid of warnings if they still exist after applying all patches.
Comment 37 Marcel de Rooy 2014-09-03 12:22:48 UTC
(In reply to Jonathan Druart from comment #36)
> It is more or less intended.
> All changes done on these patches (and linked) are not trivial and I don't
> want to add complexity for now.
> This limitation will exist until the entire feature will be pushed.
> (Not enough time at the moment to work on it without knowing when it will be
> pushed...).

> Could you please retest with *all* patches applied?
> Actually I split patches into different bug reports but all patches should
> be QA at once (patches from bug 11413 and bug 11395).
> I think this is fixed by the patch "Bug 11413: Change the field number
> logic".

I applied 11319, 11413 and 11395 together now.
The limitation seems to not be the same anymore.
But it is still not possible to move from a control field to a regular subfield. If I move from a control field to a regular field, the marc modification will not work without warning the user. Say from 001 to 029. Only a error like this one could be found in the koha logs:
stage-marc-import.pl: ERROR occurred in SetUTF8Flag Field 029 must have indicators (use ' ' for empty indicators)at /usr/share/koha/testclone/C4/Charset.pm line 146.
Note that the field 001 is cleared, but the contents does not reach 029.
Comment 38 Jonathan Druart 2014-09-03 12:46:54 UTC
(In reply to M. de Rooy from comment #37)
> I applied 11319, 11413 and 11395 together now.
> The limitation seems to not be the same anymore.
> But it is still not possible to move from a control field to a regular
> subfield. If I move from a control field to a regular field, the marc
> modification will not work without warning the user.

What do you expect?
You mean this should not be possible to create this rule, that's it?
Comment 39 Jonathan Druart 2014-09-03 13:11:11 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-09-03 13:13:16 UTC
Marcel, I submitted a patch on bug 11395 comment 39. Could you confirm it's what you expected?
Comment 41 Jonathan Druart 2014-09-03 13:14:09 UTC
(In reply to Jonathan Druart from comment #40)
> Marcel, I submitted a patch on bug 11395 comment 39. Could you confirm it's
> what you expected?

It is bug 11395 comment 36.
Comment 42 Marcel de Rooy 2014-09-03 14:20:28 UTC
(In reply to Jonathan Druart from comment #41)
> (In reply to Jonathan Druart from comment #40)
> > Marcel, I submitted a patch on bug 11395 comment 39. Could you confirm it's
> > what you expected?
> 
> It is bug 11395 comment 36.

Will have a look soon.
Comment 43 Marcel de Rooy 2014-10-02 13:03:07 UTC
(In reply to M. de Rooy from comment #42)
> (In reply to Jonathan Druart from comment #41)
> > (In reply to Jonathan Druart from comment #40)
> > > Marcel, I submitted a patch on bug 11395 comment 39. Could you confirm it's
> > > what you expected?
> > 
> > It is bug 11395 comment 36.
> 
> Will have a look soon.

Soon was kind of relative. Try to get back at this now..
Comment 44 Marcel de Rooy 2014-10-31 08:35:13 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2014-10-31 09:40:26 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2014-10-31 10:27:48 UTC
QA Comment bug 11319:
Generally looks good. The current implementation in master is not error free. This will not resolve all bugs.
5 patches, no koha-qa complaints, added 2 qa follow-ups.
I pass QA on this patch (taking into account the amount of work), but we are not ready yet. Discussion continues on follow-up reports..

From patch 3 (as well as the restrictions mentioned in patch 5) -- Actually, I repeat myself here:
> Note: I don't see the meaning for the add/update action if no subfield is given. So the call without subfield raises an error.
Moving 001 to 029a should be possible. But is not allowed now. Moving 001 to 029 should not be possible (since 001 has no subfields, and 029 has). This is possible, but will generate an invalid MARC record.
I will not block this patch however for that reason, because current master does not implement it correctly either. Current master moves the 001 to <subfield code="001"></subfield> in field 029. Also invalid MARC !

No blocker, but a welcome improvement: Fields are inserted at the end. I would rather insert the fields in the same group of fields 0XX, 1XX, etc. You could use insert_grouped_field for that.

Still see some related warnings (resolved another one):
marc_modification_templates.pl: Use of uninitialized value in string eq at marc_modification_templates.pl line 70.
stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at /usr/local/lib64/perl5/Template/Provider.pm line 964.

Passed QA
Comment 47 Marcel de Rooy 2014-10-31 10:30:20 UTC
Created attachment 33038 [details] [review]
Bug 11319: Koha::SimpleMARC should take a hashref for parameters

In order to avoid a long list of parameters, it should be better to
pass all of them into a hashref.

This patch does not add or modify a behavior.

Test plan:
Verify the unit tests still pass
- prove t/SimpleMARC.t
- prove t/db_dependent/MarcModificationTemplates.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2014-10-31 10:30:25 UTC
Created attachment 33039 [details] [review]
Bug 11319: Add UT for the fields management

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2014-10-31 10:30:28 UTC
Created attachment 33040 [details] [review]
Bug 11319: Add the field management for Koha::SimpleMARC

Currently the Koha::SimpleMARC module call a "field" a "subfield".
And the way to manage field is not implemented for all routines.

This patch does not modify the API. The routine's names are kept. It
just creates 2 privates routines for each action (e.g. delete_field
will call _delete_field if the action affects field and _delete_subfield
if the action affects subfields).

Before this patch the move action was authorised by the interface but
caused an error if executed.

Note: I don't see the meaning for the add/update action if no subfield
is given. So the call without subfield raises an error.

Test plan:
- apply all patches
- create or modify an existent template
- try at least the correct behavior for the following actions:
  * delete subfield and field
  * add new subfield to an existing field
  * add new subfield to an nonexisting field
  * move a subfield
  * move an entire field
  * copy a subfield
  * copy an entire field
- import a biblio and use this template
- verify the imported biblio matches actions defined.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2014-10-31 10:30:32 UTC
Created attachment 33041 [details] [review]
Bug 11319: Add specific UT for nonexistent field/subfield

This patch only adds unit tests for the copy and move actions.
They test if the action does not create a field/subfield if the source
did not exist.

Also it adds a unit tests for the existing behavior (in order not to
lost it): we can use the '^' and the '$' character in regex for
substituing. For example: Copy 245$a to 245$a with the regex s/^/BEGIN /
This will add the string "BEGIN " at the beginning of the 245$a fields.

To test: prove t/SimpleMARC.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2014-10-31 10:30:37 UTC
Created attachment 33042 [details] [review]
Bug 11319: Template modifications

This patch add template modifications for the restrictions:
- the source field is always mandatory
- on move and copy, the source and destination subfield should be both
  filled or blank.
- on move and copy, the destination subfield should be filled.
- on update, the subfield value should be filled.

Test plan:
Verify you are not able to create/modify template actions according to
these restrictions.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2014-10-31 10:30:41 UTC
Created attachment 33043 [details] [review]
Bug 11319: [QA follow-up] Add test message to MarcModificationTemplates.t

The last test (#74) did not print anything. It now does..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2014-10-31 10:30:44 UTC
Created attachment 33044 [details] [review]
Bug 11319: [QA follow-up] Remove a warning from GetModificationTemplates

Removes this warning: Use of uninitialized value $template_id in string eq
at C4/MarcModificationTemplates.pm line 84.

GetModificationTemplates has no template_id if called from
marc_modification_templates.pl without operation (first click from
interface) and from tools/stage-marc-import.pl.

Slightly adjusted the POD lines accordingly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Tomás Cohen Arazi 2014-11-12 17:58:38 UTC
Patches pushed to master.

Thanks Jonathan and Marcel!