Bug 8015

Summary: Add MARC Modifications Templates
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: AcquisitionsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: new feature    
Priority: P4 CC: bgkriegel, chris, claire.hernandez, gitbot, gmcharlt, jonathan.druart, julian.maurice, katrin.fischer, koha.aixmarseille, kyle, laurence.rault, marjorie.barry-vila, mathsabypro, paul.poulain, stephane.delaye, tredok.pierre
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11395
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11319
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14098
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5725, 11383, 15814, 16295    
Attachments: Bug 8015 - Add MARC Modifications Templates
Bug 8015 - Add MARC Modifications Templates
Bug 8015 - Add MARC Modifications Templates
Bug 8015 - Add MARC Modifications Templates
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
[SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015: Followup: On move, we cant delete existing fields
[SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates
[SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist.
[SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015 - Add MARC Modifications Templates
[SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist.
[SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
[SIGNED-OFF] Bug 8015: Followup Add MARC Modifications Templates
unimarc iso2709 file to test import
Bug 8015: Catch error in the SetUTF8Flag routine
[SIGNED-OFF] Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
Bug 8015: Fix bad ordering on function parameters
Bug 8015: Escape escape characters for strings
Bug 8015: Fix bad parameter list for direct external call to update_field
Bug 8015: Fix bad parameter list for direct external call to update_field
Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
[SIGNED-OFF] Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
[SIGNED-OFF] Bug 8015 - Add MARC Modifications Templates
[SIGNED-OFF] Bug 8015: Followup: FIX cp and mv if subfields don't exist.
[SIGNED-OFF] Bug 8015: Followup: On move, we cant delete existing fields
[SIGNED-OFF] Bug 8015 - Add MARC Modifications Templates - Followup
[SIGNED-OFF] Bug 8015: Followup Add MARC Modifications Templates
[SIGNED-OFF] Bug 8015: Catch error in the SetUTF8Flag routine
[SIGNED-OFF] Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
[SIGNED-OFF] Bug 8015: Fix bad ordering on function parameters
[SIGNED-OFF] Bug 8015: Escape escape characters for strings
[SIGNED-OFF] Bug 8015: Fix bad parameter list for direct external call to update_field
Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
Bug 8015: FIX QA issues
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
[SIGNED-OFF] Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
Bug 8015: Fix bad ordering on function parameters
Bug 8015: Escape escape characters for strings
Bug 8015: Fix bad parameter list for direct external call to update_field
Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
Bug 8015: FIX QA issues
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015: Fix complains from qa tools
Bug 8015: Followup QA issues
Bug 8015: Get rid of the eval in ModifyRecordWithTemplate
Bug 8015: Get rid of eval for evaluating =~ m//
Bug 8015: Get rid of the eval for substitution
Bug 8015: FIX ui issue
Bug 8015: Get rid of the eval for substitution
Bug 8015: FIX ui issue
Bug 8015: The template name is a required field
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015: Followup: On move, we cant delete existing fields
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Catch error in the SetUTF8Flag routine
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Fix error where field object is returned instead of field value for fields without subfields
Bug 8015: Fix bad ordering on function parameters
Bug 8015: Escape escape characters for strings
Bug 8015: Fix bad parameter list for direct external call to update_field
Bug 8015: Fix problem with moving existing subfield value to nonexistent field/subfield
Bug 8015: FIX QA issues
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015: Fix complains from qa tools
Bug 8015: Get rid of the eval in ModifyRecordWithTemplate
Bug 8015: Get rid of eval for evaluating =~ m//
Bug 8015: Get rid of the eval for substitution
Bug 8015: FIX ui issue
Bug 8015: The template name is a required field
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015: Add user permissions to all languages
Bug 8015: Remove references to C4::Koha::Log
Bug 8015: QA Followup
Bug 8015: QA Followup 2
Bug 8015 - Add MARC Modifications Templates
Bug 8015: Followup: FIX cp and mv if subfields don't exist.
Bug 8015 - Add MARC Modifications Templates - Followup
Bug 8015: Followup Add MARC Modifications Templates
Bug 8015: Fix template capitalization amd other template issues
Bug 8015: Add unit tests for SimpleMARC and MarcModificationTemplates routines
Bug 8015: Add user permissions to all languages
Bug 8015: Remove references to C4::Koha::Log
Bug 8015: QA Followup
Bug 8015: QA Followup 2

Description Kyle M Hall 2012-04-27 14:20:39 UTC
Add ability to modify marc records while staging them for import.
Comment 1 Kyle M Hall 2012-04-27 14:21:41 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-04-27 20:28:06 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-04-27 20:29:04 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-04-27 20:39:36 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-06-10 11:21:22 UTC Comment hidden (obsolete)
Comment 6 delaye 2012-08-08 13:49:28 UTC
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
Comment 7 Jonathan Druart 2012-08-13 15:09:20 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-08-13 15:25:59 UTC
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
Comment 9 Kyle M Hall 2012-08-14 14:26:47 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-08-14 14:27:20 UTC Comment hidden (obsolete)
Comment 11 delaye 2012-08-20 08:15:30 UTC
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
Comment 12 Jonathan Druart 2012-09-17 15:00:00 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2012-09-17 15:01:26 UTC
*Quick and dirty patch* :-/ But it could fix the issue.
Kyle, what do you think about it ? It could introduces some regressions.
Comment 14 Kyle M Hall 2012-09-17 17:40:02 UTC
(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.
Comment 15 Kyle M Hall 2012-09-17 17:56:37 UTC Comment hidden (obsolete)
Comment 16 delaye 2012-09-28 07:32:44 UTC
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
Comment 17 Kyle M Hall 2012-09-28 12:40:22 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-09-28 12:40:47 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-09-28 12:41:05 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2012-09-28 12:41:14 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2012-09-28 12:41:51 UTC
(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.
Comment 22 delaye 2012-09-28 12:57:07 UTC
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
Comment 23 Jonathan Druart 2012-10-16 07:36:55 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-10-16 17:00:01 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2012-10-16 17:01:20 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-10-16 17:01:39 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2012-10-16 17:01:59 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2012-10-16 17:03:05 UTC Comment hidden (obsolete)
Comment 29 Laurence Rault 2012-10-30 16:37:38 UTC
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
Comment 30 Laurence Rault 2012-10-30 16:38:53 UTC
Created attachment 13121 [details]
unimarc iso2709 file to test import
Comment 31 Jonathan Druart 2012-10-31 13:35:03 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2012-11-16 19:46:42 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2012-12-12 10:51:18 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2012-12-12 10:51:33 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2012-12-12 10:51:37 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2012-12-12 10:51:40 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2012-12-12 10:51:44 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2012-12-12 10:51:51 UTC Comment hidden (obsolete)
Comment 39 Laurence Rault 2012-12-12 11:32:47 UTC
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
Comment 40 Kyle M Hall 2012-12-12 14:47:10 UTC Comment hidden (obsolete)
Comment 41 Pierre Angot 2012-12-28 09:53:47 UTC
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.
Comment 42 Jonathan Druart 2012-12-28 14:02:25 UTC
(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?
Comment 43 Pierre Angot 2012-12-28 19:31:14 UTC
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
Comment 44 Jonathan Druart 2013-01-04 11:25:37 UTC
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?
Comment 45 Pierre Angot 2013-01-04 12:02:10 UTC
Do I create a new sandbox ? Patch 8015 is already applied on Pelardon ^^
Comment 46 Pierre Angot 2013-01-08 09:24:42 UTC
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.
Comment 47 Jonathan Druart 2013-01-08 09:54:37 UTC
(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).
Comment 48 Pierre Angot 2013-01-08 10:01:25 UTC
Ok. I will test again with differents options
Comment 49 Julian Maurice 2013-02-01 13:40:43 UTC
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.
Comment 50 Kyle M Hall 2013-03-19 09:15:26 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2013-03-19 09:15:41 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2013-03-19 09:15:49 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2013-03-19 09:15:57 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2013-03-19 09:16:05 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2013-03-19 09:16:15 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2013-03-19 10:29:42 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2013-03-19 12:45:23 UTC Comment hidden (obsolete)
Comment 58 Kyle M Hall 2013-03-19 12:45:32 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2013-03-19 12:45:44 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2013-03-19 12:45:55 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2013-03-19 12:48:11 UTC
> Failing QA for 2, 4 and 6.

Fixed!
Comment 62 Pierre Angot 2013-03-19 14:20:38 UTC
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.
Comment 63 Kyle M Hall 2013-03-19 15:10:11 UTC
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.
Comment 64 Kyle M Hall 2013-03-19 15:40:31 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2013-03-19 15:42:35 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2013-03-19 16:25:22 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2013-03-19 16:26:11 UTC
> - 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!
Comment 68 Paul Poulain 2013-03-20 08:43:04 UTC
Patch tested with a sandbox, by Leila <koha.aixmarseille@gmail.com>
Comment 69 Paul Poulain 2013-03-20 08:44:04 UTC Comment hidden (obsolete)
Comment 70 Koha Team AMU 2013-03-20 08:45:18 UTC
All is right : delete, move, add, copy are ok, even with a regEx.
Comment 71 Fridolin Somers 2013-03-20 10:04:26 UTC Comment hidden (obsolete)
Comment 72 Fridolin Somers 2013-03-20 10:07:40 UTC Comment hidden (obsolete)
Comment 73 Fridolin Somers 2013-03-20 10:08:55 UTC Comment hidden (obsolete)
Comment 74 Fridolin Somers 2013-03-20 10:09:34 UTC Comment hidden (obsolete)
Comment 75 Fridolin Somers 2013-03-20 10:10:23 UTC Comment hidden (obsolete)
Comment 76 Fridolin Somers 2013-03-20 10:12:23 UTC Comment hidden (obsolete)
Comment 77 Fridolin Somers 2013-03-20 10:13:13 UTC Comment hidden (obsolete)
Comment 78 Fridolin Somers 2013-03-20 10:13:56 UTC Comment hidden (obsolete)
Comment 79 Fridolin Somers 2013-03-20 10:14:37 UTC Comment hidden (obsolete)
Comment 80 Fridolin Somers 2013-03-20 10:15:20 UTC Comment hidden (obsolete)
Comment 81 Fridolin Somers 2013-03-20 10:15:56 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2013-03-29 16:38:37 UTC Comment hidden (obsolete)
Comment 83 Jonathan Druart 2013-03-29 16:39:57 UTC Comment hidden (obsolete)
Comment 84 Jonathan Druart 2013-03-29 16:40:03 UTC Comment hidden (obsolete)
Comment 85 Jonathan Druart 2013-03-29 16:42:27 UTC
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.
Comment 86 Kyle M Hall 2013-04-01 16:27:13 UTC
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.
Comment 87 Jonathan Druart 2013-04-02 07:49:15 UTC
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
Comment 88 Jonathan Druart 2013-05-22 09:13:27 UTC
Kyle, did you have a look at my unit tests?
Comment 89 Kyle M Hall 2013-05-22 12:00:11 UTC
(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
Comment 90 Jonathan Druart 2013-05-22 12:21:15 UTC
(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).
Comment 91 Kyle M Hall 2013-05-23 14:09:21 UTC
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).
Comment 92 Jonathan Druart 2013-05-24 09:59:40 UTC Comment hidden (obsolete)
Comment 93 Jonathan Druart 2013-05-24 10:00:41 UTC
I amended my ut patch in order to add some tests for the move_field routine.

Now all tests pass.
Comment 94 Kyle M Hall 2013-05-28 18:21:20 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2013-05-28 18:45:14 UTC
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
Comment 96 Jonathan Druart 2013-05-29 07:18:53 UTC
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).
Comment 97 Kyle M Hall 2013-08-16 11:15:35 UTC
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).
Comment 98 Katrin Fischer 2013-09-12 06:09:28 UTC
Starting on this...
Comment 99 Katrin Fischer 2013-09-12 06:13:09 UTC
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
Comment 100 Jonathan Druart 2013-09-12 08:05:52 UTC
(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.
Comment 101 Katrin Fischer 2013-09-12 08:07:26 UTC
I'd better leave it to you, Fridolyn or Kyle to fix - already lots of hands on this.
Comment 102 Kyle M Hall 2013-09-12 12:22:41 UTC
Jonathan, I'm having some trouble rebasing your git branch to master. Do you think you could do that? Thanks!
Comment 103 Katrin Fischer 2013-09-12 12:32:04 UTC
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.
Comment 104 Kyle M Hall 2013-09-12 12:47:57 UTC
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.
Comment 105 Katrin Fischer 2013-09-12 13:00:50 UTC
Sorry, I can't before late tonight - it's on my laptop.
Comment 106 Jonathan Druart 2013-09-12 13:42:38 UTC Comment hidden (obsolete)
Comment 107 Jonathan Druart 2013-09-12 13:42:45 UTC Comment hidden (obsolete)
Comment 108 Jonathan Druart 2013-09-12 13:42:52 UTC Comment hidden (obsolete)
Comment 109 Jonathan Druart 2013-09-12 13:42:58 UTC Comment hidden (obsolete)
Comment 110 Jonathan Druart 2013-09-12 13:43:04 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2013-09-12 13:43:09 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2013-09-12 13:43:14 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2013-09-12 13:43:19 UTC Comment hidden (obsolete)
Comment 114 Jonathan Druart 2013-09-12 13:43:24 UTC Comment hidden (obsolete)
Comment 115 Jonathan Druart 2013-09-12 13:43:29 UTC Comment hidden (obsolete)
Comment 116 Jonathan Druart 2013-09-12 13:43:36 UTC Comment hidden (obsolete)
Comment 117 Jonathan Druart 2013-09-12 13:43:40 UTC Comment hidden (obsolete)
Comment 118 Jonathan Druart 2013-09-12 13:43:45 UTC Comment hidden (obsolete)
Comment 119 Jonathan Druart 2013-09-12 13:43:53 UTC Comment hidden (obsolete)
Comment 120 Jonathan Druart 2013-09-12 13:43:58 UTC Comment hidden (obsolete)
Comment 121 Jonathan Druart 2013-09-12 13:44:52 UTC
Rebased patches.
I have no idea how to remove 2 others warnings.
It's a Perl Critic recommendation not to eval a literal.
Comment 122 Katrin Fischer 2013-09-14 13:27:50 UTC
Hm, can someone help us out here fixing the perl critic violations?
Comment 123 Jonathan Druart 2013-09-16 09:49:18 UTC
(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.
Comment 124 Katrin Fischer 2013-09-22 17:32:43 UTC
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?
Comment 125 Paul Poulain 2013-09-23 12:08:36 UTC
(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 !
Comment 126 Jonathan Druart 2013-09-23 12:39:19 UTC Comment hidden (obsolete)
Comment 127 Paul Poulain 2013-09-25 08:27:59 UTC
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)
Comment 128 Jared Camins-Esakov 2013-09-25 13:22:22 UTC
(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.
Comment 129 Paul Poulain 2013-09-25 13:47:10 UTC
(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 ?
Comment 130 Jared Camins-Esakov 2013-09-25 13:54:53 UTC
(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?
Comment 131 Galen Charlton 2013-09-25 17:23:45 UTC
(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.
Comment 132 Jonathan Druart 2013-09-26 08:43:45 UTC
(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.
Comment 133 Jonathan Druart 2013-09-26 13:54:19 UTC Comment hidden (obsolete)
Comment 134 Jonathan Druart 2013-09-26 13:54:27 UTC Comment hidden (obsolete)
Comment 135 Jonathan Druart 2013-09-26 13:54:33 UTC Comment hidden (obsolete)
Comment 136 Jonathan Druart 2013-09-26 13:54:39 UTC Comment hidden (obsolete)
Comment 137 Jonathan Druart 2013-09-26 13:56:38 UTC
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!
Comment 138 I'm just a bot 2013-09-29 06:20:25 UTC
Patch applied cleanly, go forth and signoff
Comment 139 Bernardo Gonzalez Kriegel 2013-10-04 13:28:22 UTC
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
Comment 140 Jonathan Druart 2013-10-04 13:45:48 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #139)
> Is there anything to do before running tests?
No! I will fix that asap
Comment 141 Jonathan Druart 2013-10-04 14:04:38 UTC Comment hidden (obsolete)
Comment 142 Jonathan Druart 2013-10-04 14:04:44 UTC Comment hidden (obsolete)
Comment 143 Jonathan Druart 2013-10-04 14:05:49 UTC
(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.
Comment 144 Bernardo Gonzalez Kriegel 2013-10-05 12:43:22 UTC
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
Comment 145 Jonathan Druart 2013-10-08 13:45:48 UTC Comment hidden (obsolete)
Comment 146 Jonathan Druart 2013-10-08 13:48:21 UTC
(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?
Comment 147 Paul Poulain 2013-10-08 13:51:36 UTC
(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
Comment 148 Bernardo Gonzalez Kriegel 2013-10-09 14:18:43 UTC Comment hidden (obsolete)
Comment 149 Bernardo Gonzalez Kriegel 2013-10-09 14:19:20 UTC Comment hidden (obsolete)
Comment 150 Bernardo Gonzalez Kriegel 2013-10-09 14:19:53 UTC Comment hidden (obsolete)
Comment 151 Bernardo Gonzalez Kriegel 2013-10-09 14:20:23 UTC Comment hidden (obsolete)
Comment 152 Bernardo Gonzalez Kriegel 2013-10-09 14:21:01 UTC Comment hidden (obsolete)
Comment 153 Bernardo Gonzalez Kriegel 2013-10-09 14:21:35 UTC Comment hidden (obsolete)
Comment 154 Bernardo Gonzalez Kriegel 2013-10-09 14:22:04 UTC Comment hidden (obsolete)
Comment 155 Bernardo Gonzalez Kriegel 2013-10-09 14:22:34 UTC Comment hidden (obsolete)
Comment 156 Bernardo Gonzalez Kriegel 2013-10-09 14:23:10 UTC Comment hidden (obsolete)
Comment 157 Bernardo Gonzalez Kriegel 2013-10-09 14:23:41 UTC Comment hidden (obsolete)
Comment 158 Bernardo Gonzalez Kriegel 2013-10-09 14:24:16 UTC Comment hidden (obsolete)
Comment 159 Bernardo Gonzalez Kriegel 2013-10-09 14:24:47 UTC Comment hidden (obsolete)
Comment 160 Bernardo Gonzalez Kriegel 2013-10-09 14:25:24 UTC Comment hidden (obsolete)
Comment 161 Bernardo Gonzalez Kriegel 2013-10-09 14:26:05 UTC Comment hidden (obsolete)
Comment 162 Bernardo Gonzalez Kriegel 2013-10-09 14:26:41 UTC Comment hidden (obsolete)
Comment 163 Bernardo Gonzalez Kriegel 2013-10-09 14:27:32 UTC Comment hidden (obsolete)
Comment 164 Bernardo Gonzalez Kriegel 2013-10-09 14:28:21 UTC Comment hidden (obsolete)
Comment 165 Bernardo Gonzalez Kriegel 2013-10-09 14:28:51 UTC Comment hidden (obsolete)
Comment 166 Bernardo Gonzalez Kriegel 2013-10-09 14:29:27 UTC Comment hidden (obsolete)
Comment 167 Bernardo Gonzalez Kriegel 2013-10-09 14:29:55 UTC Comment hidden (obsolete)
Comment 168 Bernardo Gonzalez Kriegel 2013-10-09 14:32:27 UTC
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
Comment 169 Kyle M Hall 2013-10-21 21:15:45 UTC Comment hidden (obsolete)
Comment 170 Kyle M Hall 2013-10-21 21:16:47 UTC Comment hidden (obsolete)
Comment 171 Kyle M Hall 2013-10-21 21:17:07 UTC Comment hidden (obsolete)
Comment 172 Kyle M Hall 2013-10-21 21:17:24 UTC Comment hidden (obsolete)
Comment 173 Kyle M Hall 2013-10-21 21:17:45 UTC Comment hidden (obsolete)
Comment 174 Kyle M Hall 2013-10-21 21:18:01 UTC Comment hidden (obsolete)
Comment 175 Kyle M Hall 2013-10-21 21:19:27 UTC
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.
Comment 176 Kyle M Hall 2013-10-22 16:19:50 UTC Comment hidden (obsolete)
Comment 177 Kyle M Hall 2013-10-22 16:24:36 UTC Comment hidden (obsolete)
Comment 178 Kyle M Hall 2013-10-22 17:21:17 UTC Comment hidden (obsolete)
Comment 179 Kyle M Hall 2013-10-22 18:39:33 UTC Comment hidden (obsolete)
Comment 180 Katrin Fischer 2013-10-22 22:56:11 UTC
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*
Comment 181 Kyle M Hall 2013-10-25 13:30:19 UTC
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>
Comment 182 Kyle M Hall 2013-10-25 13:30:40 UTC
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>
Comment 183 Kyle M Hall 2013-10-25 13:31:00 UTC
Created attachment 22394 [details] [review]
Bug 8015 - Add MARC Modifications Templates - Followup

Signed-off-by: Leila <koha.aixmarseille@gmail.com>
Comment 184 Kyle M Hall 2013-10-25 13:31:18 UTC
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>
Comment 185 Kyle M Hall 2013-10-25 13:31:35 UTC
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
Comment 186 Kyle M Hall 2013-10-25 13:31:49 UTC
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>
Comment 187 Kyle M Hall 2013-10-25 13:32:06 UTC
Created attachment 22398 [details] [review]
Bug 8015: Add user permissions to all languages
Comment 188 Kyle M Hall 2013-10-25 13:32:21 UTC
Created attachment 22399 [details] [review]
Bug 8015: Remove references to C4::Koha::Log
Comment 189 Kyle M Hall 2013-10-25 13:32:37 UTC
Created attachment 22400 [details] [review]
Bug 8015: QA Followup

* Add bug number to database update
* Fix capitalization errors
* Fix JavaScript string translation errors
Comment 190 Kyle M Hall 2013-10-25 13:32:52 UTC
Created attachment 22401 [details] [review]
Bug 8015: QA Followup 2
Comment 191 Galen Charlton 2013-10-31 23:00:39 UTC
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.
Comment 192 Jonathan Druart 2013-12-18 10:49:47 UTC
/!\ This feature is buggy if the condition and source fields are defined on the same field.

See bug 11413.