Bug 11413 - Marc modification template has not the expected behavior if the condition and source fields are on the same field
Summary: Marc modification template has not the expected behavior if the condition and...
Status: CLOSED FIXED
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: Marcel de Rooy
URL:
Whiteboard:
Keywords: rel_3_18_candidate
Depends on: 11319
Blocks: 11395
  Show dependency treegraph
 
Reported: 2013-12-18 10:47 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
7 users (show)

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


Attachments
Bug 11413: Change the field number logic (37.69 KB, patch)
2013-12-18 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.46 KB, patch)
2013-12-18 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.64 KB, patch)
2013-12-18 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Change the field number logic (37.90 KB, patch)
2014-03-03 09:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.46 KB, patch)
2014-03-03 09:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.65 KB, patch)
2014-03-03 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Change the field number logic (37.65 KB, patch)
2014-06-19 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.46 KB, patch)
2014-06-19 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.65 KB, patch)
2014-06-19 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Change the field number logic (37.56 KB, patch)
2014-06-19 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.46 KB, patch)
2014-06-19 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.65 KB, patch)
2014-06-19 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11413: Change the field number logic (37.63 KB, patch)
2014-07-03 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11413: UT to show up the issue (4.52 KB, patch)
2014-07-03 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11413: Reflect the changes to the interface (1.71 KB, patch)
2014-07-03 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11413: Change the field number logic (37.68 KB, patch)
2014-11-04 20:00 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.57 KB, patch)
2014-11-04 20:00 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.77 KB, patch)
2014-11-04 20:00 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11413: Fix return for ModifyRecordWithTemplate (1.85 KB, patch)
2014-11-12 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Fix field_numbers (13.25 KB, patch)
2014-11-12 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11413: Change the field number logic (37.77 KB, patch)
2014-11-14 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11413: UT to show up the issue (4.67 KB, patch)
2014-11-14 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11413: Reflect the changes to the interface (1.85 KB, patch)
2014-11-14 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11413: Fix return for ModifyRecordWithTemplate (1.96 KB, patch)
2014-11-14 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11413: Fix field_numbers (13.35 KB, patch)
2014-11-14 09:47 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-12-18 10:47:26 UTC
If you want to do an action (delete/update/move/...) on a multivalued field and if a condition is defined on the same field, it is highly probable the resulted record will not be what you expected.

For example:
Deleting All (or the first) fields 650 if 245$a="Bad title" works with the current code.

BUT if you want to delete All (or the first) fields 650 with a condition on 650$9=42, and if at least one field matches the condition :
- if you have selected all, all fields 650 will be deleted, even the ones who do not match the condition.
- if you have selected first, the first 650 field will be deleted, even if it does not match the condition.
The expected behavior is to delete the fields matching the condition (and not all the 650 fields).

Maybe this bug is a major one, lose or corrupt data is probable!
Comment 1 Jonathan Druart 2013-12-18 10:58:30 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-12-18 10:58:38 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-12-18 10:58:47 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-12-18 11:02:19 UTC
Since I already submitted a big refactoring for this tool (bug 11319), I developed this fix on top of it. So I mark them as dependent. But the bug will exist in the 3.14 branch.
Comment 5 Galen Charlton 2013-12-18 23:19:53 UTC
(In reply to Jonathan Druart from comment #4)
> Since I already submitted a big refactoring for this tool (bug 11319), I
> developed this fix on top of it. So I mark them as dependent. But the bug
> will exist in the 3.14 branch.

How feasible is it to make a patch for 3.14.x that does the minimum required to avoid data destruction without depending on bug 11319?
Comment 6 Jonathan Druart 2013-12-19 09:28:14 UTC
(In reply to Galen Charlton from comment #5)
> How feasible is it to make a patch for 3.14.x that does the minimum required
> to avoid data destruction without depending on bug 11319?

I proposed a patch on bug 11414 for 3.14.x.
It does not fix the problem but alerts the user.
Comment 7 Galen Charlton 2013-12-19 15:12:41 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Galen Charlton from comment #5)
> > How feasible is it to make a patch for 3.14.x that does the minimum required
> > to avoid data destruction without depending on bug 11319?
> 
> I proposed a patch on bug 11414 for 3.14.x.
> It does not fix the problem but alerts the user.

OK, I think that will suffice for 3.14.x.

Given the newness of the MARC modification templates feature, it's not beyond the realm of possibility that I would support also pushing the refactoring to 3.14.x.  We'll see after I review the code.
Comment 8 Kyle M Hall 2013-12-20 20:17:57 UTC
Applying: Bug 11413: Change the field number logic
fatal: sha1 information is lacking or useless (C4/MarcModificationTemplates.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11413: Change the field number logic
Comment 9 Jonathan Druart 2013-12-23 10:00:46 UTC
These patches apply cleanly on top of bug 11319.
Comment 10 Jonathan Druart 2014-03-03 09:21:37 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-03-03 09:21:53 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-03-03 09:22:01 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-03-03 09:23:36 UTC
Fix conflict with bug 11478.
Comment 14 Koha Team Lyon 3 2014-06-18 15:52:37 UTC
Some problems occurred applying patches from bug 11413:
<h1>Something went wrong !</h1>Applying: Bug 11413: Change the field number logic Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/MarcModificationTemplates.pm CONFLICT (content): Merge conflict in C4/MarcModificationTemplates.pm Auto-merging Koha/SimpleMARC.pm CONFLICT (content): Merge conflict in Koha/SimpleMARC.pm Auto-merging t/SimpleMARC.t CONFLICT (content): Merge conflict in t/SimpleMARC.t Failed to merge in the changes.
Patch failed at 0001 Bug 11413: Change the field number logic When you have resolved this problem run git bz apply --continue.
Comment 15 Jonathan Druart 2014-06-18 16:02:54 UTC
(In reply to Koha Team Lyon 3 from comment #14)
> Some problems occurred applying patches from bug 11413:

Apply cleanly on top of bug 11319.
Comment 16 Jonathan Druart 2014-06-19 10:31:25 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-06-19 10:31:30 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-06-19 10:31:34 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-06-19 10:41:52 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-06-19 10:41:56 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-06-19 10:41:59 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2014-07-03 17:46:00 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2014-07-03 17:46:20 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-07-03 17:46:23 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-07-28 12:00:51 UTC
Planning to QA this after 11319
Comment 26 Marcel de Rooy 2014-08-07 11:18:46 UTC
BLOCKED: Waits on report 11319 (which is in FQA)
Comment 27 Jonathan Druart 2014-09-16 08:26:26 UTC
This one needs QA (with 11319 and 11395).
Comment 28 Marcel de Rooy 2014-10-31 10:56:31 UTC
QA Comment:
This looks promising, but currently generates a problem:
stage-marc-import.pl: Can't call method "update" on an undefined value at /usr/share/koha/testclone/Koha/SimpleMARC.pm line 214, <GEN19> chunk 1.

Steps to reproduce:
Add/Update field 946 a with value "Hi 946".

Could you fix the error and add this specific test case?
In my case this field 946 does not exist and should be created.
Note that I did test this also under bug 11319 and it was ok. So this report introduces the error.

Failed QA
Comment 29 Jonathan Druart 2014-11-03 14:09:08 UTC
(In reply to M. de Rooy from comment #28)
> QA Comment:
> This looks promising, but currently generates a problem:
> stage-marc-import.pl: Can't call method "update" on an undefined value at
> /usr/share/koha/testclone/Koha/SimpleMARC.pm line 214, <GEN19> chunk 1.
> 
> Steps to reproduce:
> Add/Update field 946 a with value "Hi 946".

Could you please test this patch set with bug 11395 please?
I got the same error without patches from bug 11395.
I did not manage to maintain both bug reports bug free independently of each other.

They should be tested, QAed and pushed together.
Comment 30 Marcel de Rooy 2014-11-03 14:13:32 UTC
(In reply to Jonathan Druart from comment #29)
> (In reply to M. de Rooy from comment #28)
> > QA Comment:
> > This looks promising, but currently generates a problem:
> > stage-marc-import.pl: Can't call method "update" on an undefined value at
> > /usr/share/koha/testclone/Koha/SimpleMARC.pm line 214, <GEN19> chunk 1.
> > 
> > Steps to reproduce:
> > Add/Update field 946 a with value "Hi 946".
> 
> Could you please test this patch set with bug 11395 please?
> I got the same error without patches from bug 11395.
> I did not manage to maintain both bug reports bug free independently of each
> other.
> 
> They should be tested, QAed and pushed together.

If so, please merge them.
A squash of patches that edit the same lines multiple items would be helpful too.
I appreciate that you did a lot of work here. But QAing these sets takes time too. Just setting the status back to Signed off is not my solution..
Comment 31 Jonathan Druart 2014-11-03 14:54:28 UTC
(In reply to M. de Rooy from comment #30)
> If so, please merge them.
> A squash of patches that edit the same lines multiple items would be helpful
> too.

I would not prefer. One is a bugfix (this one), the other is a new feature (11395).
I could try to find what fix the problem, but I don't have enough time to do it soon and I don't think it is worth a try.
Comment 32 Brendan Gallagher 2014-11-04 20:00:35 UTC Comment hidden (obsolete)
Comment 33 Brendan Gallagher 2014-11-04 20:00:47 UTC Comment hidden (obsolete)
Comment 34 Brendan Gallagher 2014-11-04 20:00:58 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2014-11-05 12:13:08 UTC
This passed qa now, but is there any evidence that the problem reported does not occur anymore with adding the patches of 11395 on top?
What did you test, Brendan?
Comment 36 Brendan Gallagher 2014-11-05 23:00:05 UTC
(In reply to M. de Rooy from comment #35)
> This passed qa now, but is there any evidence that the problem reported does
> not occur anymore with adding the patches of 11395 on top?
> What did you test, Brendan?

I did not see the problems Marcel.
Comment 37 Brendan Gallagher 2014-11-05 23:04:41 UTC
(In reply to Brendan Gallagher from comment #36)
> (In reply to M. de Rooy from comment #35)
> > This passed qa now, but is there any evidence that the problem reported does
> > not occur anymore with adding the patches of 11395 on top?
> > What did you test, Brendan?
> 
> I did not see the problems Marcel.

Sorry more info - I was testing and QA-ing this in conjunction with Kyle while I was working on the coding for this one and Kyle was QA-ing/code 11395.  For 11413 (I test both with this patch plus 11319 and then with 11395 on top)
Comment 38 Marcel de Rooy 2014-11-06 07:43:31 UTC
(In reply to Brendan Gallagher from comment #37)
> (In reply to Brendan Gallagher from comment #36)
> > (In reply to M. de Rooy from comment #35)
> > > This passed qa now, but is there any evidence that the problem reported does
> > > not occur anymore with adding the patches of 11395 on top?
> > > What did you test, Brendan?
> > 
> > I did not see the problems Marcel.
> 
> Sorry more info - I was testing and QA-ing this in conjunction with Kyle
> while I was working on the coding for this one and Kyle was QA-ing/code
> 11395.  For 11413 (I test both with this patch plus 11319 and then with
> 11395 on top)

Thanks.
I verified now that the bug of comment28 is resolved by applying the patches of bug 11395 on top of this.
Comment 39 Jonathan Druart 2014-11-12 13:07:21 UTC
(In reply to M. de Rooy from comment #28)
> QA Comment:
> This looks promising, but currently generates a problem:
> stage-marc-import.pl: Can't call method "update" on an undefined value at
> /usr/share/koha/testclone/Koha/SimpleMARC.pm line 214, <GEN19> chunk 1.

It is fixed by Bug 11395: Fix field_numbers
"""
This fix is a global fix for the MarcModificationTemplate feature.
Some unit tests were missing and some behaviors were wrong.
For instance, if you tried to update a non existent field, the script
crashed.
"""
Comment 40 Jonathan Druart 2014-11-12 13:16:43 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2014-11-12 13:16:48 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2014-11-12 13:20:50 UTC
Last 2 patches were on but 11395, but need to be on this one.
They are not QA yet.

The first one removes a warning.
The second one fixes the issue raised by Marcel on comment 28.

Sorry, I didn't realize it was so easy to do!
Comment 43 Marcel de Rooy 2014-11-12 15:11:08 UTC
I will have a look again this week if someone else is not faster :)
Comment 44 Marcel de Rooy 2014-11-14 07:41:38 UTC
QA: Working on this one now.
Comment 45 Marcel de Rooy 2014-11-14 09:47:04 UTC
Created attachment 33555 [details] [review]
Bug 11413: Change the field number logic

This patch series is a bugfix for the Marc modification templates tool.

Bug description:
If you want to do an action (delete/update/move/...) on a multivalued
field and if a condition is defined on the same field, it is highly
probable the resulted record will not be what you expect.

For example:
Deleting All (or the first) fields 650 if 245$a="Bad title" works with
the current code.
BUT if you want to delete All (or the first) fields 650 with a condition
on 650$9=42, and if at least one field matches the condition :
- if you have selected all, all fields 650 will be deleted, even the
  ones who do not match the condition.
- if you have selected first, the first 650 field will be deleted, even
  if it does not match the condition.
The expected behavior is to delete the fields matching the
condition (and not all the 650 fields).

What this patch does:
This patch introduces 2 changes in the logic of Koha::SimpleMARC.
The first change is a change of the prototypes for the 2 routines
field_exists and field_equals. Now they return the "field number" of the
matching fields.
The second change is the type of the "n" parameter for all routines
using it in Koha::SimpleMARC. Before this patch, the "n" parameter was a
boolean in most cases. If 0, the action was done on all fields, if 1
on the first one only. Now it is possible to specify the "field numbers"
(so the array of field numbers which is returned by field_exists or
field_equals) for all routines which had the n parameter.

Test plan for the patch series:
Note: This test plan describes a specific example, feel free to create
your own one.
0/ Define a marc modification template with the following action:
  Delete field 245 if 245$9 = 42
1/ choose and export a record with several 245 fields.
For ex:
  245
    $a The art of computer programming
    $c Donald E. Knuth.
    $9 41
  245
    $a Bad title
    $c Bad author
    $9 42
2/ import it using the Stage MARC for import tool.
3/ verify the imported record does not contain any 245 field.
4/ apply all the patches from this bug report
5/ do again steps 2 and 3
6/ verify the imported record contains only one 245 field, the one with
245$9=41
7/ verify the unit tests passed:
  prove t/SimpleMARC.t
  prove t/db_dependent/MarcModificationTemplates.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2014-11-14 09:47:08 UTC
Created attachment 33556 [details] [review]
Bug 11413: UT to show up the issue

These UT reflect this change:
- deletion of the field 245 if 245$a='Bad title'
- move of the 650 field to 651 if 650$9=499

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2014-11-14 09:47:13 UTC
Created attachment 33557 [details] [review]
Bug 11413: Reflect the changes to the interface

Test plan:
- add/edit an action on the marc modification templates tools
- choose an action and define a condition
- define the source field as same as the condition field
- verify the All/1st dropdown list is changed to Every/1st

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2014-11-14 09:47:17 UTC
Created attachment 33558 [details] [review]
Bug 11413: Fix return for ModifyRecordWithTemplate

Make sure the ModifyRecordWithTemplate routine returns undef.

This patch also removes a warning if GetModificationTemplates is called
without parameter.

Verify
  prove t/db_dependent/MarcModificationTemplates.t
returns green.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2014-11-14 09:47:21 UTC
Created attachment 33559 [details] [review]
Bug 11413: Fix field_numbers

This fix is a global fix for the MarcModificationTemplate feature.
Some unit tests were missing and some behaviors were wrong.
For instance, if you tried to update a non existent field, the script
crashed.

The following line was completely stupid:
    if $from_field ne $to_subfield

The field_number equals 1 if the user wants to update the first field
and 0 for all fields.

The field_numbers (note the s) variable contains the field numbers to
update. This array is filled if a condition exists (field exists or
field equals).

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2014-11-14 09:48:48 UTC
QA Comment:
Great work!
No complaints from qa tools.
Adjusted tests pass.
Problem reported earlier has been resolved.
Thanks for improving variable names. This illustrates that bad variable naming is a bug source.
Also thanks for adding tests.

Small note: Lines (without comment) like
  @values = map { $_ <= @values ? $values[ $_ - 1 ] : () } @$field_numbers;
might be marked as somewhat obfuscating ;)

The All/Every change is not really needed imo, and does not seem to work always when changing a condition. But np ;)

This report does not deal with (nor introduce) this warning, but I just note that I would really like to get rid of it ;)
Maybe, Kyle could resolve that one in least time.. Something with forks and STDOUT etc.
stage-marc-import.pl: Filehandle STDOUT reopened as FH only for input at /usr/local/lib64/perl5/Template/Provider.pm line 964., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/tools/stage-marc-import.pl

Passed QA
Comment 51 Tomás Cohen Arazi 2014-11-14 16:16:50 UTC
Patches pushed to master.

Thanks Jonathan!