Bug 14098 - Regression in Marc Modification Templates
Summary: Regression in Marc Modification Templates
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15095
  Show dependency treegraph
 
Reported: 2015-04-30 14:36 UTC by Kyle M Hall
Modified: 2016-06-21 21:40 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14098: FIX Copy a subfield should not update the original field (9.69 KB, patch)
2015-05-27 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14098: Remove unedeed subroutines (6.42 KB, patch)
2015-05-27 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14098: Add copy_and_replace action to MMT (4.17 KB, patch)
2015-05-27 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14098: Implement the copy_and_replace action for MTT (25.42 KB, patch)
2015-05-27 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14098: FIX Copy a subfield should not update the original field (9.76 KB, patch)
2015-05-27 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14098: Remove unedeed subroutines (6.49 KB, patch)
2015-05-27 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14098: Add copy_and_replace action to MMT (4.21 KB, patch)
2015-05-27 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14098: Implement the copy_and_replace action for MTT (25.49 KB, patch)
2015-05-27 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14098: FIX Copy a subfield should not update the original field (9.84 KB, patch)
2015-09-03 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14098: Remove unedeed subroutines (6.57 KB, patch)
2015-09-03 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14098: Add copy_and_replace action to MMT (4.32 KB, patch)
2015-09-03 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14098: Implement the copy_and_replace action for MTT (25.58 KB, patch)
2015-09-03 09:35 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 Kyle M Hall 2015-04-30 14:36:29 UTC
In previous versions of Koha, a marc modfication rules like "Copy field XXX$Y to 952$x" would add the field to an existing 952 field if there was one already, and only create a 952 field if none existed. Now an entirely new 952 field is created to store the subfield even if there is already one!
Comment 1 Jonathan Druart 2015-05-05 13:26:00 UTC
(In reply to Kyle M Hall from comment #0)
> In previous versions of Koha, a marc modfication rules like "Copy field
> XXX$Y to 952$x" would add the field to an existing 952 field if there was
> one already, and only create a 952 field if none existed. Now an entirely
> new 952 field is created to store the subfield even if there is already one!

Hello Kyle,
I would say it's the way it should work.
Or maybe should we create a new action and let the choice to the user?
Comment 2 Gaetan Boisson 2015-05-05 13:58:03 UTC
That seems quite tricky. We certainly should let the user decide, but it then becomes quite complicated to take into account all the possibilities.

Copying the value to a new 952 subfield sounds like a good default to me, right now i cannot think of a case where the result would be unclear/unexpected.

On the other hand copying to an existing field raises the question of what happens when we have multiple such fields in the record.

I can think of quite a few options from the top of my head:
- Add the subfield to all of the existing fields
- Do nothing
- Add only to the first such field that exists (probably not a very interesting option...)
- Add to the field matching this and this criteria on this other subfield
- Create a new field with a subfield having the given value (current behaviour)

In the end the process would be something like:

If the record has no such field already, choose between:
- Create the field with the subfield and the wanted value
- Do nothing

If the record has exactly one field of this type, choose between:
- Add the subfield to the existing field
- Create a new field with the subfield and the wanted value
- Do nothing

If the record has multiple fields of this type, choose between:
- Add the subfield to all the existing fields
- Create a new field with the subfield and the wanted value
- Add to the field matching this and this criteria on this other subfield
- Do nothing

Else:
- Timewarp

I feel like this is a bug, since existing templates are not working as they were before. But it seems to me that this action is very unclear in itself and probably deserves some enhancement to let users choose more clearly what will happen.
Comment 3 Kyle M Hall 2015-05-13 16:32:39 UTC
(In reply to Jonathan Druart from comment #1)
> (In reply to Kyle M Hall from comment #0)
> > In previous versions of Koha, a marc modfication rules like "Copy field
> > XXX$Y to 952$x" would add the field to an existing 952 field if there was
> > one already, and only create a 952 field if none existed. Now an entirely
> > new 952 field is created to store the subfield even if there is already one!
> 
> Hello Kyle,
> I would say it's the way it should work.
> Or maybe should we create a new action and let the choice to the user?

Owen has created a template that relied on the previous behavior, so I think it's a regression. However, if adding a new behavior would allow him to get his previous template to work by changing its' rules, that seems like a fine compromise.
Comment 4 Jonathan Druart 2015-05-24 11:27:58 UTC
I don't manage to provide a patch.
There is a lot of combinations.

How does it work now (3.20.00.000)?

Let's consider this control sample:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists

1/ Copy a field to a nonexistent field

Action: Copy the whole field 245 to 246:

Result:
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  246    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists

2/ Copy a subfield to a nonexistent field

Result:
Action: Copy the subfield 245$a to 11395 246$a:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  246    _aThe art of computer programming
  300    _aA_exists
         _bB_exists

3/ Copy a field to an existent field

Result:
Action: Copy the whole field 245 to 300:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists
  300    _aThe art of computer programming
         _cDonald E. Knuth.

4/ Copy a subfield to an existent field

Result:
Action: Copy the subfield 245$a to 300$a:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aThe art of computer programming
         _bB_exists

QUESTION: First, is that make sense to add a new field in 3 but erase the subfield in 4?
This is certainly wrong.


To support more use cases, I would like to create another action "copy and replace".

Let's consider the same control sample:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists

1/ Copy (and replace) a field to a nonexistent field

Action: Copy the whole field 245 to 246.

Result (same as copy):
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  246    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists

2/ Copy (and replace) a subfield to a nonexistent field

Action: Copy the subfield 245$a to 246$a.

Result (same as copy):
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  246    _aThe art of computer programming
  300    _aA_exists
         _bB_exists

3/ Copy (and replace) a field to an existent field

Action: Copy the whole field 245 to 300:

Result:
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aThe art of computer programming
         _cDonald E. Knuth.

4/ Copy (and replace) a field to an existent field

Action: Copy the subfield 245$a to 300$a:

Result:
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aThe art of computer programming
         _bB_exists

QUESTION: Does everybody agree with that?

Ok, That was the easy situations.

Now what should happen if we have this control sample?
  245    _aThe art of computer programming
         _cDonald E. Knuth.
  245    _aAnother title
         _cAnother author
  300    _aA_exists
         _bB_exists
  300    _aA_also_exists
         _bB_also_exists
Comment 5 Gaetan Boisson 2015-05-26 16:01:00 UTC
> QUESTION: First, is that make sense to add a new field in 3 but erase the
> subfield in 4?
> This is certainly wrong.

This sounds more like a rhetoric question than a true question ;), but i agree, this doesnt make a lot of sense to me!

> To support more use cases, I would like to create another action "copy and
> replace".
[...]
> QUESTION: Does everybody agree with that?

I do.

> 
> Ok, That was the easy situations.
> 
> Now what should happen if we have this control sample?
>   245    _aThe art of computer programming
>          _cDonald E. Knuth.
>   245    _aAnother title
>          _cAnother author
>   300    _aA_exists
>          _bB_exists
>   300    _aA_also_exists
>          _bB_also_exists

According to me, we need more than one action to manage this.

If we are copying a whole field, then it sounds rather simple (to me) to add a new field along the two existing 300, (copy in a new field) or to replace the existing one (update). I think this is already possible in the tool.

If we are copying a subfield, it's tricky. We could want to
- create a new field with the copied subfield (copy in a new field) (never ambiguous with the modified record)
- copy it to all the existing fields (update existing fields) but some records in the batch might not have a corresponding field already, so we would need a "conservative update" (don't do anything if there is no such field already) and a "force update" (create the field if none exists already). I think i could deal with the second option being the only one if this is stated clearly.

I hope this helps, i admit my ideas are a bit muddy on the topic.
Comment 6 Nicole C. Engard 2015-05-26 16:28:42 UTC
I think my bug report is related to this : http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13560 I think copy and add should be 2 different things.
Comment 7 Nicole C. Engard 2015-05-26 16:32:10 UTC
(In reply to Gaetan Boisson from comment #2)
> On the other hand copying to an existing field raises the question of what
> happens when we have multiple such fields in the record.
> 
> I can think of quite a few options from the top of my head:
> - Add the subfield to all of the existing fields
> - Do nothing
> - Add only to the first such field that exists (probably not a very
> interesting option...)
> - Add to the field matching this and this criteria on this other subfield
> - Create a new field with a subfield having the given value (current
> behaviour)
> 
> In the end the process would be something like:
> 
> If the record has no such field already, choose between:
> - Create the field with the subfield and the wanted value
> - Do nothing
> 
> If the record has exactly one field of this type, choose between:
> - Add the subfield to the existing field
> - Create a new field with the subfield and the wanted value
> - Do nothing
> 
> If the record has multiple fields of this type, choose between:
> - Add the subfield to all the existing fields
> - Create a new field with the subfield and the wanted value
> - Add to the field matching this and this criteria on this other subfield
> - Do nothing
> 
> Else:
> - Timewarp
> 
> I feel like this is a bug, since existing templates are not working as they
> were before. But it seems to me that this action is very unclear in itself
> and probably deserves some enhancement to let users choose more clearly what
> will happen.


I agree with this - we need more options and that's a development. But we should put things back the way they worked before before we start enhancing things.
Comment 8 Jonathan Druart 2015-05-27 12:15:58 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-05-27 12:16:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-05-27 12:16:09 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-05-27 12:16:14 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-05-27 13:25:08 UTC
Important note:

IMO this should be backported to 3.20.x with a warning in the updatedatabase entry if
    select count(*) from marc_modification_template_actions where action="copy_field";

returns > 0
Comment 13 Kyle M Hall 2015-05-27 16:33:18 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-05-27 16:33:24 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-05-27 16:33:27 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-05-27 16:33:29 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-05-27 16:36:31 UTC
Copy and replace appears to work as expected. 

Tried origin copy with 952$a to 952$b which created a second $b subfield on the 952$a. I'm not sure if this was the previous behavior or not.

Jonathan, can you reset the status to signed-off or failed qa?
Comment 18 Jonathan Druart 2015-05-27 16:43:38 UTC
(In reply to Kyle M Hall from comment #17)
> Copy and replace appears to work as expected. 
> 
> Tried origin copy with 952$a to 952$b which created a second $b subfield on
> the 952$a. I'm not sure if this was the previous behavior or not.
> 
> Jonathan, can you reset the status to signed-off or failed qa?

It's what I expect :)
Comment 19 Jonathan Druart 2015-06-03 08:43:33 UTC
According to my comment 12, I am raising the priority to get a quick QA review on this one.
Comment 20 Marcel de Rooy 2015-09-03 07:37:06 UTC
QA: Looking at this one now.
Comment 21 Marcel de Rooy 2015-09-03 08:17:29 UTC
I first made a rule copy 245a to 300a. That added another 300a in the same 300 field.
Then I changed this rule to replace 245a to 300a. This did NOTHING!
I checked the record in the table: the action field is EMPTY.

Failed QA
Comment 22 Marcel de Rooy 2015-09-03 09:06:08 UTC
did you execute the atomicupdate for bug 14098?
Comment 23 Marcel de Rooy 2015-09-03 09:11:31 UTC
Back to SO.. Continue QA session
Comment 24 Marcel de Rooy 2015-09-03 09:35:04 UTC
Created attachment 42282 [details] [review]
Bug 14098: FIX Copy a subfield should not update the original field

There is an inconsistency in the copy action:

Given the following control sample:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists

If we apply action (a) Copy the whole field 245 to 300, we get:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aA_exists
         _bB_exists
  300    _aThe art of computer programming
         _cDonald E. Knuth.

If we apply action (b) Copy the subfield 245$a to 300$a, we get:

  245    _aThe art of computer programming
         _cDonald E. Knuth.
  300    _aThe art of computer programming
         _bB_exists

In (a) the field is copied but in (b) the subfield is erased.

We should be consistent and don't erase the destination field.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2015-09-03 09:35:09 UTC
Created attachment 42283 [details] [review]
Bug 14098: Remove unedeed subroutines

Just some cleaning before to continue, _copy_field and _copy_subfield did not
do anything useful.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2015-09-03 09:35:13 UTC
Created attachment 42284 [details] [review]
Bug 14098: Add copy_and_replace action to MMT

This patch add the new value for the MTT action.
It updates the marc_modification_template_actions.action DB field to
allow 'copy_and_replace_field'.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2015-09-03 09:35:18 UTC
Created attachment 42285 [details] [review]
Bug 14098: Implement the copy_and_replace action for MTT

This patch implements the copy and replace action for the marc
modification templates.
Instead of copying a field/subfield, it will erase the destination
fields/subfields.

Test plan:
Find it yourself.
Compare the differences between the copy and the copy_and_replace
actions.
The easier way to test is to 1/ create a complete record, 2/create some
modification templates and 3/ use the batch record modification with the
"preview" function.

QA note: I kept the same tests as "copy" and, if no change were
expected, I noted them "(same as copy)", to be sure this new action won't
introduce regression on these tests.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2015-09-03 09:40:50 UTC
QA Comment:
Code looks good to me. Pass qa test.
Tried several combinations in web interface. The copy/replace whole field erases destination subfields which are not in the source. This is expected behavior, but should be noted somewhere in help text perhaps.
Impressive test coverage.
Only question: Does the name SimpleMarc still cover these growing files? :)
Comment 29 Tomás Cohen Arazi 2015-09-07 14:32:26 UTC
Patches pushed to master.

Good job Jonathan!!