Add ability to modify marc records while staging them for import.
Created attachment 9313 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 9329 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 9330 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 9331 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 10089 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
If asked to do a action(move) on subfield in which is repeated within a single field (exemplae : UNIMARC 200 or $ f $ G or $ 606 x ... etc.) the tool don't update all the subfield. Only the 1rt subfield is changed the others are deleted Example: move thedata from 200 $g to 200 $f the tool delete the $ g and recreated only one subfield $f corresponding to the first $g
Created attachment 11579 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist Hi Kyle, I tried to fix the problem raised by Stephane and I arrived to this patch. I don't know if it is the best way to do it. It seems to fix the copy and move actions if the number of values (in @values) is > number of subfields already present in the field. I Hope it will help you
Looks good to me. Thanks for the help! Kyle (In reply to comment #7) > Created attachment 11579 [details] [review] > Bug 8015: Followup: FIX cp and mv if subfields don't exist > > Hi Kyle, > I tried to fix the problem raised by Stephane and I arrived to this patch. I > don't know if it is the best way to do it. > It seems to fix the copy and move actions if the number of values (in > @values) is > number of subfields already present in the field. > > I Hope it will help you
Created attachment 11607 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 11608 [details] [review] [SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
wit the new patch In the template marc move all 200$g --> 200$f In the bibliographic notice before the import 200$fGeorg Friedrich Haendel, compositeur $gEnsemble Arcadia, ensemble instrumental$gAttilio Cremonesi, direction In the bibliographic notice after the import 200$fEnsemble Arcadia, ensemble instrumental $fAttilio Cremonesi, direction the 200$fGeorg Friedrich Haendel, compositeur have been deleted
Created attachment 12280 [details] [review] Bug 8015: Followup: On move, we cant delete existing fields
*Quick and dirty patch* :-/ But it could fix the issue. Kyle, what do you think about it ? It could introduces some regressions.
(In reply to comment #13) > *Quick and dirty patch* :-/ But it could fix the issue. > Kyle, what do you think about it ? It could introduces some regressions. I think that looks good to me. I don't think it will cause any problems.
Created attachment 12288 [details] [review] [SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
i want test the new patch in a sandbox but i have this message Applying: Bug 8015 - Add MARC Modifications Templates Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/ImportBatch.pm CONFLICT (content): Merge conflict in C4/ImportBatch.pm Auto-merging C4/Koha.pm Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql Auto-merging installer/data/mysql/kohastructure.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt Auto-merging tools/stage-marc-import.pl CONFLICT (content): Merge conflict in tools/stage-marc-import.pl Failed to merge in the changes. Patch failed at 0001 Bug 8015 - Add MARC Modifications Templates When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Bug 8015 - Add MARC Modifications Templates Bug 8015 - Add MARC Modifications Templates Apply? [yn] Patch left in /tmp/Bug-8015---Add-MARC-Modifications-Templates-42bN43.patch
Created attachment 12569 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 12570 [details] [review] [SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12571 [details] [review] [SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12572 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup
(In reply to comment #16) > i want test the new patch in a sandbox but i have this message The patches needed a rebase. You should be able to test it now.
After apply the patch in a sand box i have this message Software error: Can't locate Koha/MarcModificationTemplates.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .) at /home/koha/src/C4/ImportBatch.pm line 29. BEGIN failed--compilation aborted at /home/koha/src/C4/ImportBatch.pm line 29. Compilation failed in require at /home/koha/src/tools/stage-marc-import.pl line 40. BEGIN failed--compilation aborted at /home/koha/src/tools/stage-marc-import.pl line 40. from the url .../cgi-bin/koha/tools/stage-marc-import.pl
Created attachment 12839 [details] [review] Bug 8015: Followup Add MARC Modifications Templates
Created attachment 12856 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 12857 [details] [review] [SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12858 [details] [review] [SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12859 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup
Created attachment 12860 [details] [review] [SIGNED-OFF] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
When I try to import unimarc files with templates, I have some problems with unimarc format checking. I join a file bib-237.utf8 as an exemple : I am able to import it without template, but when a template is selected, Stage for import in /cgi-bin/koha/tools/stage-marc-import.pl page produces no job with template : - import of a iso 5426 file : not OK - export of a koha biblio in marc unicode/utf8 format and following import : not OK - export of a koha biblio in marc unicode/utf8,standard format and following import : OK In the log we found things as : errors in C4::Charset::SetUTF8Flag Field 686 must have indicators (use ' ' for empty indicators) at /home/koha/src/C4/Charset.pm line 142 As these files can be well imported without template, I think they should be as well be with a template
Created attachment 13121 [details] unimarc iso2709 file to test import
Created attachment 13138 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred.
Created attachment 13495 [details] [review] [SIGNED-OFF] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14030 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 14031 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14032 [details] [review] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14033 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup
Created attachment 14034 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14035 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Tested on sandbox8 marc template for import created import of an UTF8 file (the one joined in this bz) : OK, encoding problem solved marc modification according to template : OK
Created attachment 14040 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I've got a problem with the file to test import (bib-237.utf8). I'v added it to a basket from a staged file, but the price is not filled. I think the price is include in the file (947$c ?). There is no problem with quantity (10 books). After mys first test, I've noticed a software error. - Manage staged MARC records - create a new basket - add an order from a staged file - select file - clic on "add order" (to access ) = software error Can't call method "raw" on an undefined value at /home/koha/src/C4/Search.pm line 272. If I dont clic on "add order", there is no software error. Note : I've tested on a sandbox.
(In reply to comment #41) > I've got a problem with the file to test import (bib-237.utf8). I'v added it > to a basket from a staged file, but the price is not filled. I think the > price is include in the file (947$c ?). > There is no problem with quantity (10 books). Does it work on master? > After mys first test, I've noticed a software error. > - Manage staged MARC records > - create a new basket > - add an order from a staged file > - select file > - clic on "add order" (to access ) = software error > > Can't call method "raw" on an undefined value at /home/koha/src/C4/Search.pm > line 272. > > If I dont clic on "add order", there is no software error. I cannot reproduce, could you give me the url of the basket please?
Yep. Test on master (Koha version : 3.11.00.014) I've created a new basket to verify before adding a comment ^^ Ex : basketno=8 http://pro.pelardon.biblibre.com/cgi-bin/koha/acqui/addorderiso2709.pl?import_batch_id=402&basketno=8&booksellerid=4
The manipulations for Bug 3087 were not made on sandboxes. Now the problem is solved. Pierre, could you check that your problem does not exist anymore please?
Do I create a new sandbox ? Patch 8015 is already applied on Pelardon ^^
There is no problem with software error. But, when I've added an order with the file to test, vendor price and quantity are empty.
(In reply to comment #46) > There is no problem with software error. > > But, when I've added an order with the file to test, vendor price and > quantity are empty. I think it is the current behavior in master. So there is no regression here. (The import improvement is in the patch proposed for Bug 7180).
Ok. I will test again with differents options
Hi, I tested some simple use cases and found some errors/things that can be improved: The bibliographic record used for these tests has only one field 200 with various subfields (5abefg) and no field 300. 1/ Copy field 200 to 300 => Problem: No 300 field is created. Is this a bug or a missing feature? Same remark for 'Move' 2/ Copy field 200 to 300$a => Problem: 300$a = "MARC::Field::HASH(0x...)" Same for 'Move' 3/ Copy field 200$a to 300 => Problem: No 300 field is created but maybe not a bug. Same for 'Move' 4/ Copy field 200$a to 300$a => Problem: Two fields 300 are created, one is correct with 300$a = <value of 200$a> but one has just an empty 'a' subfield. Same for 'Move' but the empty 'a' subfield is now a subfield that contains the string "don't_erase". 5/ Add/Update field 200 with value 'blob' => Problem: create a subfield 'b' with value 'lob' 6/ There is also a problem with Regex. If they contain '\' characters, they are removed from text input when editing the action. So if I edit an action and update it without changing anything, regex like "/(word)/\U$1\E/" are saved as "/(word)/U$1E" Failing QA for 2, 4 and 6. For 1, 3 and 5 it would be great if Koha prevent the user to enter invalid action.
Created attachment 16330 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/
Created attachment 16331 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 16332 [details] [review] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 16333 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup
Created attachment 16334 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 16335 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 16363 [details] [review] Bug 8015: Fix template capitalization amd other template issues
Created attachment 16376 [details] [review] Bug 8015: Fix template capitalization amd other template issues
Created attachment 16377 [details] [review] Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
Created attachment 16378 [details] [review] Bug 8015: Fix bad ordering on function parameters
Created attachment 16379 [details] [review] Bug 8015: Escape escape characters for strings
> Failing QA for 2, 4 and 6. Fixed!
We've tested : - Move 1st field 200$a to 300$a : 200$a is note moved but is deleted - Update field 995$f with value bla if 995$f does not match : there is nothing.
Point of documentation: It is possible to operate on a field without a subfield if and only if that field does not have subfields according to the MARC specification. For example, we can copy 005 to 052$b, because 005 is just data without subfields. We can also copy 952$g to 300$c, as they are data points as well. The scenario that does not work is trying to move a field with subfields to a new field. That is, for example, we cannot move field 300 to field 500. What can can do is move 300$a to 500$a, 300$b to 500%b, and so on.
Created attachment 16413 [details] [review] Bug 8015: Fix bad parameter list for direct external call to update_field
Created attachment 16415 [details] [review] Bug 8015: Fix bad parameter list for direct external call to update_field
Created attachment 16423 [details] [review] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
> - Move 1st field 200$a to 300$a : 200$a is note moved but is deleted Fixed! > - Update field 995$f with value bla if 995$f does not match : there is > nothing. Fixed!
Patch tested with a sandbox, by Leila <koha.aixmarseille@gmail.com>
Created attachment 16455 [details] [review] [SIGNED-OFF] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com>
All is right : delete, move, add, copy are ok, even with a regEx.
Created attachment 16468 [details] [review] [SIGNED-OFF] Bug 8015 - Add MARC Modifications Templates
Created attachment 16470 [details] [review] [SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Created attachment 16471 [details] [review] [SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields
Created attachment 16472 [details] [review] [SIGNED-OFF] Bug 8015 - Add MARC Modifications Templates - Followup
Created attachment 16473 [details] [review] [SIGNED-OFF] Bug 8015: Followup Add MARC Modifications Templates
Created attachment 16475 [details] [review] [SIGNED-OFF] Bug 8015: Catch error in the SetUTF8Flag routine
Created attachment 16476 [details] [review] [SIGNED-OFF] Bug 8015: Fix template capitalization amd other template issues
Created attachment 16477 [details] [review] Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
Created attachment 16478 [details] [review] [SIGNED-OFF] Bug 8015: Fix bad ordering on function parameters
Created attachment 16479 [details] [review] [SIGNED-OFF] Bug 8015: Escape escape characters for strings
Created attachment 16480 [details] [review] [SIGNED-OFF] Bug 8015: Fix bad parameter list for direct external call to update_field
Created attachment 17036 [details] [review] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 17037 [details] [review] Bug 8015: FIX QA issues This patch fixes some stuffs failing qa tests: POD, indentation (tabs), perlcritic
Created attachment 17038 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Hi Kyle, I add some unit tests for these new routines/methods (QA requirements). One test does not pass and I think it should. I delete the internal routine (in SimpleMARC), I hope it is good for you. I am under the impression that the build of the @values array (@values = ($values[0]) x scalar( @fields ) if @values == 1;) is enough. I add a followup for QA issues (and add your name in order to keep the authorship). Could you have a look at these 2 UT files please? Feel free to add others tests if something is missing.
Jonathan, can you publish a git branch with all these patches applied somewhere? I'm having some problems applying all of them and I don't want to screw anything up.
Strange, I didn't get any problem to apply all of them against master. I just push a branch on our kohac repository: http://git.biblibre.com/?p=kohac;a=shortlog;h=refs/heads/BZ8015
Kyle, did you have a look at my unit tests?
(In reply to comment #88) > Kyle, did you have a look at my unit tests? These are fantastic! Everything passes except the last test for SimpleMARC.t Also, test 14 has no description. Thanks for pointing me to your git branch. ok 1 - use MARC::Field; ok 2 - use MARC::Record; ok 3 - use Koha::SimpleMARC; ok 4 - 650$a exists ok 5 - 650$b does not exist ok 6 - 650$a exists, field_exists returns the first one ok 7 - first 650$a ok 8 - second 650$a ok 9 - first 650$a bis ok 10 - second 650$a bis ok 11 - There is no 3 650$a ok 12 - After copy 245$a still exists ok 13 - 246$a is a new field ok 14 ok 15 - Copy multivalued field ok 16 - Copy first field 650$a ok 17 - Copy second field 650$a ok 18 - Copy field using regex ok 19 - Copy fields using regex on existing fields ok 20 - Copy first field using regex on existing fields without erase existing values ok 21 - Copy fields using regex on existing fields without erase existing values ok 22 - update existing subfield 952$p ok 23 - create subfield 952$p ok 24 - create subfield 952$k on existing 952 field ok 25 - update all subfields 952$p with the same value ok 26 - update all subfields 952$p with the different values ok 27 - copy 952$d into others 952 field ok 28 - The first 952$c has moved not ok 29 - Now the second 952$c exists
(In reply to comment #89) > These are fantastic! Everything passes except the last test for SimpleMARC.t > not ok 29 - Now the second 952$c exists Yep, it was I said in comment 85 ("One test does not pass and I think it should."). So is there a bug in move_field? Or my test is bad (but I don't think so).
I do not believe that is a valid test. I would just remove it. (In reply to comment #90) > (In reply to comment #89) > > These are fantastic! Everything passes except the last test for SimpleMARC.t > > > not ok 29 - Now the second 952$c exists > > Yep, it was I said in comment 85 ("One test does not pass and I think it > should."). > > So is there a bug in move_field? Or my test is bad (but I don't think so).
Created attachment 18372 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
I amended my ut patch in order to add some tests for the move_field routine. Now all tests pass.
Created attachment 18441 [details] [review] [SIGNED-OFF] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Jonathan, I looks like your git branch for this doesn't include the newest version of the last patch. I checked out your branch, updated the last patch, and tried to rebase it against master. However, I got a large merge conflict at "Fix template capitalization and other template issues". Would you care to try rebasing this patch series to master? Kyle
I updated the git branch just now. All patches (submitted on this report) are rebased against master (just 2 conflicts with the first patch on updatedatabase.pl and kohastructure.sql).
Thanks Jonathan! (In reply to Jonathan Druart from comment #96) > I updated the git branch just now. > > All patches (submitted on this report) are rebased against master (just 2 > conflicts with the first patch on updatedatabase.pl and kohastructure.sql).
Starting on this...
Ok, what I did was create 2 branches, one with all patches, and another with a squashed version to allow for easier code review. The QA script fails in several places. Bit worried about some of those, so I will wait for a follow up: QA SCRIPT testing 1 commit(s) (applied to 6fcb671 'Bug 10678 - Update addbooks help page') OK C4/Charset.pm OK pod OK forbidden patterns OK valid OK critic FAIL C4/ImportBatch.pm OK pod OK forbidden patterns FAIL valid Useless use of private variable in void context OK critic OK C4/Koha.pm OK pod OK forbidden patterns OK valid OK critic FAIL C4/MarcModificationTemplates.pm OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) FAIL valid Useless use of private variable in void context OK critic FAIL Koha/SimpleMARC.pm OK pod OK forbidden patterns OK valid FAIL critic Expression form of "eval" at line 89, column 7. See page 161 of PBP. Expression form of "eval" at line 242, column 12. See page 161 of PBP. OK installer/data/mysql/updatedatabase.pl OK pod OK forbidden patterns OK valid OK critic OK t/SimpleMARC.t OK pod OK forbidden patterns OK valid OK critic OK t/db_dependent/MarcModificationTemplates.t OK pod OK forbidden patterns OK valid OK critic FAIL tools/marc_modification_templates.pl OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 8) FAIL valid Useless use of private variable in void context OK critic FAIL tools/stage-marc-import.pl OK pod OK forbidden patterns FAIL valid Useless use of private variable in void context OK critic OK koha-tmpl/intranet-tmpl/prog/en/includes/tools-menu.inc OK forbidden patterns OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt OK forbidden patterns OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/marc_modification_templates.tt FAIL forbidden patterns forbidden pattern: intranet-tmpl should certainly replaced with [% interface %] (line 301) forbidden pattern: intranet-tmpl should certainly replaced with [% interface %] (line 305) forbidden pattern: intranet-tmpl should certainly replaced with [% interface %] (line 293) forbidden pattern: intranet-tmpl should certainly replaced with [% interface %] (line 297) OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt OK forbidden patterns OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt OK forbidden patterns OK tt_valid OK valid_template
(In reply to Katrin Fischer from comment #99) > Ok, what I did was create 2 branches, one with all patches, and another with > a squashed version to allow for easier code review. The QA script fails in > several places. Bit worried about some of those, so I will wait for a follow > up: Katrin, These failures are not critical for testing. * Useless use of private variable in void context Can be fixed replacing eval{$eval}; with eval $eval; * forbidden pattern: Koha is now under the GPLv3 license (line 9) is easy to fix, just c/p lines from the coding guidelines wiki page * Expression form of "eval" at line 89, column 7. See page 161 of PBP. I already see this one, but I don't know how to fix it.
I'd better leave it to you, Fridolyn or Kyle to fix - already lots of hands on this.
Jonathan, I'm having some trouble rebasing your git branch to master. Do you think you could do that? Thanks!
Hi Kyle, I used the patches on the bug and they applied fine apart from a database related conflict in the first patch that was easy to solve.
Hmm, my issues are odd then. Could you post your rebased patches? Thanks! (In reply to Katrin Fischer from comment #103) > Hi Kyle, I used the patches on the bug and they applied fine apart from a > database related conflict in the first patch that was easy to solve.
Sorry, I can't before late tonight - it's on my laptop.
Created attachment 21024 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/ Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21025 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21026 [details] [review] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21027 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21028 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21029 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21030 [details] [review] Bug 8015: Fix template capitalization amd other template issues Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21031 [details] [review] Bug 8015: Fix error where field object is returned instead of field value for fields without subfields Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21032 [details] [review] Bug 8015: Fix bad ordering on function parameters Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21033 [details] [review] Bug 8015: Escape escape characters for strings Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21034 [details] [review] Bug 8015: Fix bad parameter list for direct external call to update_field Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21035 [details] [review] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 21036 [details] [review] Bug 8015: FIX QA issues This patch fixes some stuffs failing qa tests: POD, indentation (tabs), perlcritic
Created attachment 21037 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 21038 [details] [review] Bug 8015: Fix complains from qa tools
Rebased patches. I have no idea how to remove 2 others warnings. It's a Perl Critic recommendation not to eval a literal.
Hm, can someone help us out here fixing the perl critic violations?
(In reply to Katrin Fischer from comment #122) > Hm, can someone help us out here fixing the perl critic violations? To me, there are only 2 ways to remove theses warnings: 1/ adds a rule to the perlcriticrc file. 2/ write an unreadable code. (i.e. eval "\$value =~ s$regex"; could be written splitting the $regex var on the first character (/ or |, etc.). In both case it is not a good thing to do. Perl::Critic provides recommendations, sometimes I think we are not obliged to follow them.
We have 12 people on the CC list for this bug, which makes me think it's kinda important :) Can someone weigh in about the perlcritic issues please?
(In reply to Katrin Fischer from comment #124) > We have 12 people on the CC list for this bug, which makes me think it's > kinda important :) Agreed, bulk modification of biblio IS important to libraries... > Can someone weigh in about the perlcritic issues please? ... much more than this perlcritic problem. PBP, page 161 says many things that are unclear to me. From the user point of view, though, I think this patch should go in !
Created attachment 21371 [details] [review] Bug 8015: Followup QA issues eval { $eval }; != eval $eval; my $r = q|/foo/fee/|; my @values = ("foo", "bar", "foobar"); for my $v ( @values ) { eval {"\$v =~ s$r"}; say $v; eval "\$v =~ s$r"; say $v; } will display: foo fee bar bar foobar feebar
Katrin, please reconsider your position = this improvement is very important for libraries, much more than a technical perlcritic violation. My opinion here is that we must decide that the user interest is more important than this single violation. (the law must serve us, we must not serve the law)
(In reply to Paul Poulain from comment #127) > Katrin, please reconsider your position = this improvement is very important > for libraries, much more than a technical perlcritic violation. > My opinion here is that we must decide that the user interest is more > important than this single violation. > (the law must serve us, we must not serve the law) This is not a minor violation. As far as I can tell, there is unsanitized user input being run directly. Consider the following code: eval { $eval }; != eval $eval; my $r = q|/foo/fee/; print "hi";|; my @values = ("foo", "bar", "foobar"); for my $v ( @values ) { eval {"\$v =~ s$r"}; say $v; eval "\$v =~ s$r"; say $v; } Result: "hi" is printed by each eval. This example is fairly benign, but what if the regex were as follows: my $r = q|/whatever/something/; require C4::Context; $dbh = C4::Context->dbh; $dbh->do("DELETE FROM issues;")|; I was unable to test this example, since I couldn't get MARC modification templates to work during a cursory test, but I'm sure you see my point.
(In reply to Jared Camins-Esakov from comment #128) > This is not a minor violation. As far as I can tell, there is unsanitized > user input being run directly. Consider the following code: <snip> > I was unable to test this example, since I couldn't get MARC modification > templates to work during a cursory test, but I'm sure you see my point. I hadn't checked what the eval was related to, I assumed it was safe. I agree with your point : failed QA, this could probably be exploited. Jared, would you be pleased if the parameters where sanitized, even if the eval is still here ?
(In reply to Paul Poulain from comment #129) > I hadn't checked what the eval was related to, I assumed it was safe. > I agree with your point : failed QA, this could probably be exploited. > > Jared, would you be pleased if the parameters where sanitized, even if the > eval is still here ? I see no way to sanitize the input while using the eval. The entire point of the eval is to allow arbitrary code to be run through the regex. Maybe it would be better to have two boxes, one for the match and one for the replacement?
(In reply to Paul Poulain from comment #129) > I hadn't checked what the eval was related to, I assumed it was safe. > I agree with your point : failed QA, this could probably be exploited. > > Jared, would you be pleased if the parameters where sanitized, even if the > eval is still here ? I think it would be better to drop the notion of using a string eval at all and just rewrite ModifyRecordWithTemplate to become a dispatcher that calls the modification subroutines directly. I say "just rewrite" because, for once, I think the work can be confined to a single routine. Also, the patchset currently includes no tests that run ModifyRecordWithTemplate directly -- rather a lack, IMO. Resolving that can go hand-in-hand with removing the eval.
(In reply to Galen Charlton from comment #131) > I think it would be better to drop the notion of using a string eval at all > and just rewrite ModifyRecordWithTemplate to become a dispatcher that calls > the modification subroutines directly. I say "just rewrite" because, for > once, I think the work can be confined to a single routine. > > Also, the patchset currently includes no tests that run > ModifyRecordWithTemplate directly -- rather a lack, IMO. Resolving that can > go hand-in-hand with removing the eval. I will try to do something.
Created attachment 21484 [details] [review] Bug 8015: Get rid of the eval in ModifyRecordWithTemplate This patch removes the use of eval in the C4::MarcModificationTemplates::ModifyRecordWithTemplate routine. Now this routine call the wanted modification routine with the list of parameters. This call is done only if the condition is respected.
Created attachment 21485 [details] [review] Bug 8015: Get rid of eval for evaluating =~ m// Koha::SimpleMarc::field_equals uses eval in order to check if a string matches a pattern. Now this eval is removed and the "regex" variable does not contain the regex separated character (/ or |). Regression: Before this patch, the user was able to user a modifier. Now it is not possible.
Created attachment 21486 [details] [review] Bug 8015: Get rid of the eval for substitution Before this patch, the regex substitution was contain into only one variable (e.g. my $regex = "/foo/bar/i"). Now each member of the regex is stored into a field in the marc_modification_template_actions sql table. In order to avoid a complex code, only modifiers i and g are take into account. Note: If you already add the mmta table, you have to drop it. This patch also adds a foreign key from mmta to mmt tables.
Created attachment 21487 [details] [review] Bug 8015: FIX ui issue
These 4 last patches remove the eval. All 19 patches pass qa tools. Test plan: - Retest *all* the feature with complex actions. - prove t/SimpleMarc.t - prove t/db_dependent/MarcModificationTemplates.t Have fun and good luck!
Patch applied cleanly, go forth and signoff
Is there anything to do before running tests? - prove t/SimpleMarc.t All tests successful. Files=1, Tests=37, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.04 cusr 0.00 csys = 0.06 CPU) Result: PASS - prove t/db_dependent/MarcModificationTemplates.t ... Test Summary Report ------------------- t/db_dependent/MarcModificationTemplates.t (Wstat: 3584 Tests: 74 Failed: 14) Failed tests: 25-28, 35-39, 48-51, 65 Non-zero exit status: 14 Files=1, Tests=74, 1 wallclock secs ( 0.04 usr 0.00 sys + 0.22 cusr 0.02 csys = 0.28 CPU) Result: FAIL
(In reply to Bernardo Gonzalez Kriegel from comment #139) > Is there anything to do before running tests? No! I will fix that asap
Created attachment 21808 [details] [review] Bug 8015: Get rid of the eval for substitution Before this patch, the regex substitution was contain into only one variable (e.g. my $regex = "/foo/bar/i"). Now each member of the regex is stored into a field in the marc_modification_template_actions sql table. In order to avoid a complex code, only modifiers i and g are take into account. Note: If you already add the mmta table, you have to drop it. This patch also adds a foreign key from mmta to mmt tables.
Created attachment 21809 [details] [review] Bug 8015: FIX ui issue
(In reply to Bernardo Gonzalez Kriegel from comment #139) I didn't change the unit test file after changing the table structure. Now it is fixed.
Testing this! Tests now run without problems :) For me this works, I've tried add, copy, delete, move, regex find and replace (text and also changing branch in 952ab using __BRANCHNAME__) What's missing: 1) Check that template name is non empty. If you press "Create template" can create as many "no name" templates as you wish. Avoiding duplicates could be cool. 2) Help File. This is important for prospective users and DocManager job. At last a minimal one could help testing
Created attachment 21876 [details] [review] Bug 8015: The template name is a required field Test plan: Try to add a template with an empty string as name.
(In reply to Bernardo Gonzalez Kriegel from comment #144) > What's missing: > > 1) Check that template name is non empty. If you press "Create template" > can create as many "no name" templates as you wish. Avoiding duplicates > could be cool. Done in the last patch. > 2) Help File. This is important for prospective users and DocManager job. > At last a minimal one could help testing I don't have the time to do that. Maybe Kyle?
(In reply to Jonathan Druart from comment #146) > > 2) Help File. This is important for prospective users and DocManager job. > > At last a minimal one could help testing > > I don't have the time to do that. Maybe Kyle? well... I would also add that your english is, how to say... a little bit french :D :D
Created attachment 21901 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/ Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described. No koha-qa errors One comment to rule them all (please squash this patchset!) I think I tested all possible cases: add, copy, delete, move, regex find and replace (text and also changing branch in 952ab using __BRANCHNAME__) Unit test run fine. No help file yet but not a functional requirement. Think is a good adition.
Created attachment 21902 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21903 [details] [review] Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21904 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21905 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21906 [details] [review] Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21907 [details] [review] Bug 8015: Fix template capitalization amd other template issues Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21908 [details] [review] Bug 8015: Fix error where field object is returned instead of field value for fields without subfields Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21909 [details] [review] Bug 8015: Fix bad ordering on function parameters Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21910 [details] [review] Bug 8015: Escape escape characters for strings Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21911 [details] [review] Bug 8015: Fix bad parameter list for direct external call to update_field Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21912 [details] [review] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21913 [details] [review] Bug 8015: FIX QA issues This patch fixes some stuffs failing qa tests: POD, indentation (tabs), perlcritic Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21914 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21915 [details] [review] Bug 8015: Fix complains from qa tools Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21916 [details] [review] Bug 8015: Get rid of the eval in ModifyRecordWithTemplate This patch removes the use of eval in the C4::MarcModificationTemplates::ModifyRecordWithTemplate routine. Now this routine call the wanted modification routine with the list of parameters. This call is done only if the condition is respected. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21917 [details] [review] Bug 8015: Get rid of eval for evaluating =~ m// Koha::SimpleMarc::field_equals uses eval in order to check if a string matches a pattern. Now this eval is removed and the "regex" variable does not contain the regex separated character (/ or |). Regression: Before this patch, the user was able to user a modifier. Now it is not possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21918 [details] [review] Bug 8015: Get rid of the eval for substitution Before this patch, the regex substitution was contain into only one variable (e.g. my $regex = "/foo/bar/i"). Now each member of the regex is stored into a field in the marc_modification_template_actions sql table. In order to avoid a complex code, only modifiers i and g are take into account. Note: If you already add the mmta table, you have to drop it. This patch also adds a foreign key from mmta to mmt tables. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21919 [details] [review] Bug 8015: FIX ui issue Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 21920 [details] [review] Bug 8015: The template name is a required field Test plan: Try to add a template with an empty string as name. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed! 1) Could you consider squashing this into one... or two patches. This is insane :) 2) Needed follow-up: Help 3) Needed follow-up: Update copyright headers as current usage
Created attachment 22185 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/ Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22186 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22187 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22188 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22189 [details] [review] Bug 8015: Fix template capitalization amd other template issues Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix error where field object is returned instead of field value for fields without subfields Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix bad ordering on function parameters Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Escape escape characters for strings Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix bad parameter list for direct external call to update_field Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: FIX QA issues This patch fixes some stuffs failing qa tests: POD, indentation (tabs), perlcritic
Created attachment 22190 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 8015: Fix complains from qa tools Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of the eval in ModifyRecordWithTemplate This patch removes the use of eval in the C4::MarcModificationTemplates::ModifyRecordWithTemplate routine. Now this routine call the wanted modification routine with the list of parameters. This call is done only if the condition is respected. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of eval for evaluating =~ m// Koha::SimpleMarc::field_equals uses eval in order to check if a string matches a pattern. Now this eval is removed and the "regex" variable does not contain the regex separated character (/ or |). Regression: Before this patch, the user was able to user a modifier. Now it is not possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of the eval for substitution Before this patch, the regex substitution was contain into only one variable (e.g. my $regex = "/foo/bar/i"). Now each member of the regex is stored into a field in the marc_modification_template_actions sql table. In order to avoid a complex code, only modifiers i and g are take into account. Note: If you already add the mmta table, you have to drop it. This patch also adds a foreign key from mmta to mmt tables. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: FIX ui issue Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: The template name is a required field Test plan: Try to add a template with an empty string as name. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
To help manage the large number of patches for this bug, I've squashed into groups, those patches by the same author that were submitted sequentially.
Created attachment 22284 [details] [review] Bug 8015: Add user permissions to all languages
Created attachment 22285 [details] [review] Bug 8015: Remove references to C4::Koha::Log
Created attachment 22290 [details] [review] Bug 8015: QA Followup * Add bug number to database update * Fix capitalization errors * Fix JavaScript string translation errors
Created attachment 22293 [details] [review] Bug 8015: QA Followup 2
Test report 1)OK Staging, importing, reverting and cleaning using - stage_file.pl and commit_file.pl - Stage and Staged MARC record management tools 2) (normal) Saving an empty action - fil 3) (minor) Feedback about mandatory fields not quite standard 4) (major) Rule: Copy 650 to 689 - no subfields entered [Wed Oct 23 00:07:58 2013] [error...] [client 127.0.0.1] [Wed Oct 23 00:07:58 2013] stage-marc-import.pl: data() is only for control fields (generally, just tags below 010) , use subfield() at /home/katrin/koha/Koha/SimpleMARC.pm line 188, referer: http://staff.master/cgi-bin/koha/tools/stage-marc-import.pl No feedback in the interface, we should make sure people know that whole fields can't be copied. Maybe subfield should be required to, if it's not a controlfield (Note: controlfields are different for MARC21 and UNIMARC) 5) (?) Rule: Copy 650 a to 689 a Works, but 689 does not exist in my framework - should this be possible? 6) OK Rule: Add/Update 952 $a CPL Add/Update 952 $b CPL Add/Update 952 $y BK Adds items, works nicely 7) OK Rule: Copy 245 $a to 500 $a Regex: s/e/foo Intfoormfoodiatfoo Pfoorl - Works nicely. 8) (normal) Also: Lots of messages in the logs [Wed Oct 23 00:09:46 2013] [error] [client 127.0.0.1] [Wed Oct 23 00:09:46 2013] marc_modification_templates.pl: Use of uninitialized value in string eq at /home/katrin/koha/tools/marc_modification_templates.pl line 70., referer: http://staff.master/cgi-bin/koha/tools/marc_modification_templates.pl [Wed Oct 23 00:10:06 2013] [error] [client 127.0.0.1] [Wed Oct 23 00:10:06 2013] stage-marc-import.pl: Use of uninitialized value $template_id in string eq at /home/katrin/koha/C4/MarcModificationTemplates.pm line 84., referer: http://staff.master/cgi-bin/koha/tools/stage-marc-import.pl X) Possible enhancements: - Allow applying of modification templates on staged and maybe also imported records - Warn people from deleting mandatory fields and subfields - The Add/Update option should have a regex - Ex. set serial flag on LDR Other problems found: - Staging MARC records uploads without selecting a file first Note to myself: File a bug about translation issues coursed logic in templates (building sentences) Kyle, can you please take a look? *hides*
Created attachment 22392 [details] [review] Bug 8015 - Add MARC Modifications Templates The MARC Modification Templates system gives Koha users the power to make alterations to MARC records automatically while staging MARC records for import. This tool is useful for altering MARC records from various venders work with your MARC framework. The system essentially allows one to create a basic script using actions to Copy, Move, Add, Update and Delete fields. Each action can also have an optional condition to check the value or existance of another field. The Copy & Move actions also support Regular Expressions, which can be used to automatically modify field values during the copy/move. An example would be to strip out the '$' character in field 020$c. Furthermore, the value for an update can include variables that change each time the template is used. Currently, the system supports two variables, __BRANCHCODE__ which is replaced with the branchcode of the library currently using the template, and __CURRENTDATE__ which is replaced with the current date in ISO format ( YYYY-MM-DD ). At its simplist, it can perform functions such as: Copy field 092$a to 952$c At its most complex it can run actions like: Copy field 020$c to 020$c using RegEx s/\$// if 020$c equals RegEx m/^\$/ Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22393 [details] [review] Bug 8015: Followup: FIX cp and mv if subfields don't exist. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Followup: On move, we cant delete existing fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22394 [details] [review] Bug 8015 - Add MARC Modifications Templates - Followup Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22395 [details] [review] Bug 8015: Followup Add MARC Modifications Templates Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Catch error in the SetUTF8Flag routine The eval avoids the interface to run endlessly if an error occurred. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Created attachment 22396 [details] [review] Bug 8015: Fix template capitalization amd other template issues Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix error where field object is returned instead of field value for fields without subfields Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix bad ordering on function parameters Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Escape escape characters for strings Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix bad parameter list for direct external call to update_field Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield Signed-off-by: Leila <koha.aixmarseille@gmail.com> Bug 8015: FIX QA issues This patch fixes some stuffs failing qa tests: POD, indentation (tabs), perlcritic
Created attachment 22397 [details] [review] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 8015: Fix complains from qa tools Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of the eval in ModifyRecordWithTemplate This patch removes the use of eval in the C4::MarcModificationTemplates::ModifyRecordWithTemplate routine. Now this routine call the wanted modification routine with the list of parameters. This call is done only if the condition is respected. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of eval for evaluating =~ m// Koha::SimpleMarc::field_equals uses eval in order to check if a string matches a pattern. Now this eval is removed and the "regex" variable does not contain the regex separated character (/ or |). Regression: Before this patch, the user was able to user a modifier. Now it is not possible. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: Get rid of the eval for substitution Before this patch, the regex substitution was contain into only one variable (e.g. my $regex = "/foo/bar/i"). Now each member of the regex is stored into a field in the marc_modification_template_actions sql table. In order to avoid a complex code, only modifiers i and g are take into account. Note: If you already add the mmta table, you have to drop it. This patch also adds a foreign key from mmta to mmt tables. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: FIX ui issue Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 8015: The template name is a required field Test plan: Try to add a template with an empty string as name. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 22398 [details] [review] Bug 8015: Add user permissions to all languages
Created attachment 22399 [details] [review] Bug 8015: Remove references to C4::Koha::Log
Created attachment 22400 [details] [review] Bug 8015: QA Followup * Add bug number to database update * Fix capitalization errors * Fix JavaScript string translation errors
Created attachment 22401 [details] [review] Bug 8015: QA Followup 2
Pushed to master after discussion with Katrin. Thanks, Kyle! That said, this still needs a lot of work, enough that this may end up getting an "experimental" label added to this feature prior to release. Things I've found: - As previously noted, there's no way to move an entire field with all of its subfields. - But if you try -- by specifying a tag but not a subfield -- you can *crash* it: data() is only for con trol fields (generally, just tags below 010) , use subfield() at /home/gmc/koha/Koha/SimpleMARC.pm - When moving/copying fields, it makes no attempt to put them in order. - I can't say I particularly care for the fact that the modification takes places during _staging_. It would be better if it could take place during committing (or at least have an option to do so, or to have a preview mode), to enable the user to adjust their modification rules without having the repeatedly stage the same file. - We need to be more consistent about where new stuff gets configured. There's no particularly good reason why the matching rules should be configured under admin and the modification templates under tools.
/!\ This feature is buggy if the condition and source fields are defined on the same field. See bug 11413.