|
Description
Jonathan Druart
2013-12-18 10:47:26 UTC
Created attachment 23622 [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 Created attachment 23623 [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 Created attachment 23624 [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 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. (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? (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. (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. 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 Created attachment 25758 [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 Created attachment 25759 [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 Created attachment 25760 [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 Fix conflict with bug 11478. 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. (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. Created attachment 28993 [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 Created attachment 28994 [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 Created attachment 28995 [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 Created attachment 28996 [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 Created attachment 28997 [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 Created attachment 28998 [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 Created attachment 29502 [details] [review] [SIGNED-OFF] 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> Created attachment 29503 [details] [review] [SIGNED-OFF] 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> Created attachment 29504 [details] [review] [SIGNED-OFF] 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> Planning to QA this after 11319 BLOCKED: Waits on report 11319 (which is in FQA) This one needs QA (with 11319 and 11395). 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 (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. (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.. (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. Created attachment 33213 [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> Created attachment 33214 [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> Created attachment 33215 [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> 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? (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. (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) (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. (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. """ Created attachment 33482 [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> Created attachment 33483 [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> 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! I will have a look again this week if someone else is not faster :) QA: Working on this one now. 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> 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> 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> 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> 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> 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
Patches pushed to master. Thanks Jonathan! |