Bug 14098

(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?

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.

(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.

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

> 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.

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.

(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.

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

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?

(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 :)

According to my comment 12, I am raising the priority to get a quick QA review on this one.

QA: Looking at this one now.

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

did you execute the atomicupdate for bug 14098?

Back to SO.. Continue QA session

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>

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>

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>

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>

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? :)

Patches pushed to master.

Good job Jonathan!!