Attachment 33151 Details for Bug 11395 – Bug 11395: Fix field_numbers

[patch] Bug 11395: Fix field_numbers

Bug-11395-Fix-fieldnumbers.patch (text/plain), 13.25 KB, created by Jonathan Druart on 2014-11-03 14:10:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2014-11-03 14:10:07 UTC

Size: 13.25 KB

patch

obsolete

>From 1cfb938f857a68e181c846347eaedec974004e0c Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@biblibre.com>
>Date: Thu, 19 Jun 2014 18:15:36 +0200
>Subject: [PATCH] Bug 11395: Fix field_numbers
>
>This fix is a global fix for the MarcModificationTemplate feature.
>Some unit tests were missing and some behaviors were wrong.
>For instance, if you tried to update a non existent field, the script
>crashed.
>
>The following line was completely stupid:
>    if $from_field ne $to_subfield
>
>The field_number equals 1 if the user wants to update the first field
>and 0 for all fields.
>
>The field_numbers (note the s) variable contains the field numbers to
>update. This array is filled if a condition exists (field exists or
>field equals).
>
>Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
>---
> C4/MarcModificationTemplates.pm            |  34 +++++-
> Koha/SimpleMARC.pm                         |   6 +-
> t/db_dependent/MarcModificationTemplates.t | 189 +++++++++++++++++++++++++++--
> 3 files changed, 210 insertions(+), 19 deletions(-)
>
>diff --git a/C4/MarcModificationTemplates.pm b/C4/MarcModificationTemplates.pm
>index f0cccda..3af0013 100644
>--- a/C4/MarcModificationTemplates.pm
>+++ b/C4/MarcModificationTemplates.pm
>@@ -504,7 +504,7 @@ sub ModifyRecordWithTemplate {
> 
>     foreach my $a ( @actions ) {
>         my $action = $a->{'action'};
>-        my $field_number = $a->{'field_number'};
>+        my $field_number = $a->{'field_number'} // 1;
>         my $from_field = $a->{'from_field'};
>         my $from_subfield = $a->{'from_subfield'};
>         my $field_value = $a->{'field_value'};
>@@ -526,7 +526,7 @@ sub ModifyRecordWithTemplate {
>         }
> 
>         my $do = 1;
>-        my $field_numbers = [ $field_number ];
>+        my $field_numbers = [];
>         if ( $conditional ) {
>             if ( $conditional_comparison eq 'exists' ) {
>                 $field_numbers = field_exists({
>@@ -554,6 +554,7 @@ sub ModifyRecordWithTemplate {
>                     value => $conditional_value,
>                     field => $conditional_field,
>                     subfield => $conditional_subfield,
>+                    is_regex => $conditional_regex,
>                 });
>                 $do = $conditional eq 'if'
>                     ? @$field_numbers
>@@ -562,8 +563,10 @@ sub ModifyRecordWithTemplate {
>             elsif ( $conditional_comparison eq 'not_equals' ) {
>                 $field_numbers = field_equals({
>                     record => $record,
>+                    value => $conditional_value,
>                     field => $conditional_field,
>-                    subfield => $conditional_subfield
>+                    subfield => $conditional_subfield,
>+                    is_regex => $conditional_regex,
>                 });
>                 $do = $conditional eq 'if'
>                     ? not @$field_numbers
>@@ -572,9 +575,28 @@ sub ModifyRecordWithTemplate {
>         }
> 
>         if ( $do ) {
>-            @$field_numbers = ( $field_number )
>-                if $from_field ne $to_subfield
>-                    and $field_number;
>+
>+            # field_number == 0 if all field need to be updated
>+            # or 1 if only the first field need to be updated
>+
>+            # A condition has been given
>+            if ( @$field_numbers > 0 ) {
>+                if ( $field_number == 1 ) {
>+                    # We want only the first matching
>+                    $field_numbers = [ $field_numbers->[0] ];
>+                }
>+            }
>+            # There was no condition
>+            else {
>+                if ( $field_number == 1 ) {
>+                    # We want to process the first field
>+                    $field_numbers = [ 1 ];
>+                } elsif ( $to_field and $from_field ne $to_field ) {
>+                    # If the from and to fields are not the same, we only process the first field.
>+                    $field_numbers = [ 1 ];
>+                }
>+            }
>+
>             if ( $action eq 'copy_field' ) {
>                 copy_field({
>                     record => $record,
>diff --git a/Koha/SimpleMARC.pm b/Koha/SimpleMARC.pm
>index 0c2c403..3d99dec 100644
>--- a/Koha/SimpleMARC.pm
>+++ b/Koha/SimpleMARC.pm
>@@ -360,7 +360,7 @@ sub field_equals {
>   my $value = $params->{value};
>   my $fieldName = $params->{field};
>   my $subfieldName = $params->{subfield};
>-  my $regex = $params->{regex};
>+  my $is_regex = $params->{is_regex};
> 
>   if ( ! $record ) { return; }
> 
>@@ -374,7 +374,7 @@ sub field_equals {
>     SUBFIELDS: for my $subfield_value ( @subfield_values ) {
>       if (
>           (
>-              $regex and $subfield_value =~ m/$value/
>+              $is_regex and $subfield_value =~ m/$value/
>           ) or (
>               $subfield_value eq $value
>           )
>@@ -546,7 +546,7 @@ sub _copy_move_field {
>     my $fromFieldName = $params->{from_field};
>     my $toFieldName = $params->{to_field};
>     my $regex = $params->{regex};
>-    my $field_numbers = $params->{field_numbers};
>+    my $field_numbers = $params->{field_numbers} // [];
>     my $action = $params->{action} || 'copy';
> 
>     my @fields = $record->field( $fromFieldName );
>diff --git a/t/db_dependent/MarcModificationTemplates.t b/t/db_dependent/MarcModificationTemplates.t
>index e8bfcbc..f54527e 100644
>--- a/t/db_dependent/MarcModificationTemplates.t
>+++ b/t/db_dependent/MarcModificationTemplates.t
>@@ -1,6 +1,8 @@
> use Modern::Perl;
> 
>-use Test::More tests => 78;
>+use Test::More tests => 94;
>+
>+use Koha::SimpleMARC;
> 
> use_ok("MARC::Field");
> use_ok("MARC::Record");
>@@ -151,7 +153,7 @@ like( $template_id, qr|^\d+$|, "new template returns an id" );
> 
> is( AddModificationTemplateAction(
>     $template_id, 'delete_field', 0,
>-    '245', '', '', '', '', '',
>+    '245', '', '', '', '',
>     '', '', '',
>     'if', '245', 'a', 'equals', 'Bad title', '',
>     'Delete field 245 if 245$a eq "Bad title"'
>@@ -183,15 +185,15 @@ is( AddModificationTemplateAction(
> 
> is( AddModificationTemplateAction(
>     $template_id, 'move_field', 0,
>-    '952', 'd', '', '952', 'e', '',
>+    '952', 'd', '', '952', 'e',
>     '', '', '',
>     'if', '952', 'c', 'equals', '^GEN', '1',
>-    'Move field 952$d to 952$e if 952$c =~ /^GE/'
>-), 1, 'Add fifth action: move field 952$d to 952$e if 952$c =~ /^GE/');
>+    'Move field 952$d to 952$e if 952$c =~ /^GEN/'
>+), 1, 'Add fifth action: move field 952$d to 952$e if 952$c =~ /^GEN/');
> 
> is( AddModificationTemplateAction(
>     $template_id, 'update_field', 0,
>-    '650', 'a', 'Computer algorithms.', '', '', '',
>+    '650', 'a', 'Computer algorithms.', '', '',
>     '', '', '',
>     'if', '650', '9', 'equals', '499', '',
>     'Update field 650$a with "Computer algorithms." to 651 if 650$9 == 499'
>@@ -199,20 +201,141 @@ is( AddModificationTemplateAction(
> 
> is( AddModificationTemplateAction(
>     $template_id, 'move_field', 0,
>-    '650', '', '', '651', '', '',
>+    '650', '', '', '651', '',
>     '', '', '',
>     'if', '650', '9', 'equals', '499', '',
>     'Move field 650 to 651 if 650$9 == 499'
> ), 1, 'Add seventh action: move field 650 to 651 if 650$9 == 499');
> 
>+is( AddModificationTemplateAction(
>+    $template_id, 'update_field', 0,
>+    '999', 'a', 'non existent.', '', '',
>+    '', '', '',
>+    '', '', '', '', '', '',
>+    'Update non existent field 999$a with "non existent"'
>+), 1, 'Add eighth action: update field non existent 999$a with "non existent."');
> 
> my $record = new_record();
> 
> is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
> 
>-my $expected_record = expected_record();
>+my $expected_record = expected_record_1();
> is_deeply( $record, $expected_record, "Record modification as expected");
> 
>+$template_id = AddModificationTemplate("another_template_test");
>+
>+# Duplicate 245 => 3x245
>+is( AddModificationTemplateAction(
>+    $template_id, 'copy_field', 0,
>+    '245', '', '', '245', '',
>+    '', '', '',
>+    'if', '245', 'a', 'equals', 'Bad title', '',
>+    'Copy field 245 if 245$a eq "Bad title"'
>+), 1, 'Add action: copy field 245 if 245$a eq "Bad title"');
>+
>+$record = new_record();
>+is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
>+
>+my @fields_245a = Koha::SimpleMARC::read_field({
>+    record => $record,
>+    field => '245',
>+    subfield => 'a',
>+});
>+is_deeply( \@fields_245a, [
>+        'The art of computer programming',
>+        'Bad title',
>+        'Bad title',
>+    ], 'Copy field has copied the "Bad title"' );
>+
>+# Update first "Bad title"
>+is( AddModificationTemplateAction(
>+    $template_id, 'update_field', 1,
>+    '245', 'a', 'Bad title updated', '', '',
>+    '', '', '',
>+    'if', '245', 'a', 'equals', 'Bad title', '',
>+    'Update first 245$a matching "Bad title" with "Bad title updated"'
>+), 1, 'Add action: update field 245$a matching "Bad title" with "Bad title updated');
>+
>+$record = new_record();
>+is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
>+
>+@fields_245a = Koha::SimpleMARC::read_field({
>+    record => $record,
>+    field => '245',
>+    subfield => 'a',
>+});
>+is_deeply( \@fields_245a, [
>+        'The art of computer programming',
>+        'Bad title updated',
>+        'Bad title',
>+    ], 'update_field has update first the "Bad title"' );
>+
>+# Duplicate first 245 => 3x245
>+is( AddModificationTemplateAction(
>+    $template_id, 'copy_field', 1,
>+    '245', '', '', '245', '',
>+    '', '', '',
>+    'if', '245', 'a', 'equals', '^Bad title', '1',
>+    'Copy field 245 if 245$a =~ "^Bad title"'
>+), 1, 'Add action: copy field 245 if 245$a =~ "^Bad title"');
>+
>+$record = new_record();
>+is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
>+
>+@fields_245a = Koha::SimpleMARC::read_field({
>+    record => $record,
>+    field => '245',
>+    subfield => 'a',
>+});
>+is_deeply( \@fields_245a, [
>+        'The art of computer programming',
>+        'Bad title updated',
>+        'Bad title',
>+        'Bad title updated',
>+    ], 'Copy field has copied first "^Bad title"' );
>+
>+# Delete first ^Bad title
>+is( AddModificationTemplateAction(
>+    $template_id, 'delete_field', 1,
>+    '245', '', '', '', '',
>+    '', '', '',
>+    'if', '245', 'a', 'equals', '^Bad title', '1',
>+    'Delete first 245$a mathing ^Bad title'
>+), 1, 'Delete first 245$a mathing ^Bad title');
>+
>+$record = new_record();
>+is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
>+@fields_245a = Koha::SimpleMARC::read_field({
>+    record => $record,
>+    field => '245',
>+    subfield => 'a',
>+});
>+is_deeply( \@fields_245a, [
>+        'The art of computer programming',
>+        'Bad title',
>+        'Bad title updated',
>+    ], 'delete field has been deleted the right field"' );
>+
>+is( AddModificationTemplateAction(
>+    $template_id, 'delete_field', 0,
>+    '245', '', '', '', '',
>+    '', '', '',
>+    'if', '245', 'a', 'equals', 'updated$', '1',
>+    'Delete first 245$a mathing updated$'
>+), 1, 'Delete first 245$a mathing updated$');
>+
>+$record = new_record();
>+is( ModifyRecordWithTemplate( $template_id, $record ), undef, "The ModifyRecordWithTemplate returns undef" );
>+@fields_245a = Koha::SimpleMARC::read_field({
>+    record => $record,
>+    field => '245',
>+    subfield => 'a',
>+});
>+is_deeply( \@fields_245a, [
>+        'The art of computer programming',
>+        'Bad title'
>+    ], 'delete field has been deleted the right field"' );
>+
> sub new_record {
>     my $record = MARC::Record->new;
>     $record->leader('03174nam a2200445 a 4500');
>@@ -229,7 +352,7 @@ sub new_record {
>         ),
>         MARC::Field->new(
>             245, '1', '4',
>-            a => 'field to remove',
>+            a => 'Bad title',
>             c => 'Donald E. Knuth.',
>         ),
>         MARC::Field->new(
>@@ -254,7 +377,7 @@ sub new_record {
>     return $record;
> }
> 
>-sub expected_record {
>+sub expected_record_1 {
>     my $record = MARC::Record->new;
>     $record->leader('03174nam a2200445 a 4500');
>     my @fields = (
>@@ -288,11 +411,57 @@ sub expected_record {
>             a => 'Computer algorithms.',
>             9 => '499',
>         ),
>+        MARC::Field->new(
>+            999, ' ', ' ',
>+            a => 'non existent.',
>+        ),
>     );
>     $record->append_fields(@fields);
>     return $record;
> }
> 
>+sub expected_record_2 {
>+    my $record = MARC::Record->new;
>+    $record->leader('03174nam a2200445 a 4500');
>+    my @fields = (
>+        MARC::Field->new(
>+            100, '1', ' ',
>+            a => 'Knuth, Donald Ervin',
>+            d => '1938',
>+        ),
>+        MARC::Field->new(
>+            245, '1', '4',
>+            a => 'The art of computer programming',
>+            c => 'Donald E. Knuth.',
>+        ),
>+        MARC::Field->new(
>+            650, ' ', '0',
>+            9 => '462',
>+        ),
>+        MARC::Field->new(
>+            952, ' ', ' ',
>+            p => '3010023917_updated',
>+            y => 'BK',
>+            c => 'GEN',
>+            e => '2001-06-25',
>+        ),
>+        MARC::Field->new(
>+            246, '', ' ',
>+            a => 'The art of computer programming',
>+        ),
>+        MARC::Field->new(
>+            651, ' ', '0',
>+            a => 'Computer algorithms.',
>+            9 => '499',
>+        ),
>+        MARC::Field->new(
>+            999, ' ', ' ',
>+            a => 'non existent.',
>+        ),
>+    );
>+    $record->append_fields(@fields);
>+    return $record;
>+}
> 
> # C4::Context->userenv
> sub Mock_userenv {
>-- 
>2.1.0

Actions: View | Diff | Splinter Review

Attachments on bug 11395: 23502 | 23503 | 23504 | 23505 | 23506 | 23507 | 23508 | 23592 | 24634 | 26072 | 26981 | 28978 | 28999 | 29000 | 29001 | 29002 | 29003 | 29004 | 29056 | 29509 | 29569 | 31344 | 32181 | 32182 | 32184 | 32239 | 32240 | 32241 | 32242 | 32243 | 32244 | 32245 | 32246 | 32247 | 32248 | 32249 | 33145 | 33146 | 33147 | 33148 | 33149 | 33150 | 33151 | 33152 | 33153 | 33154 | 33155 | 34393 | 34394 | 34395 | 34396 | 34397 | 34398 | 34399 | 34400 | 34401 | 34402 | 34552 | 34553 | 34554 | 34555 | 34556 | 34557 | 34558 | 34559 | 34560 | 34561 | 34562 | 35783 | 35784 | 35785 | 35786 | 35787 | 35788 | 35789 | 35790 | 35791 | 35792 | 36295 | 36296 | 36297 | 36298 | 36299 | 36300 | 36301 | 36302 | 36303 | 36304