From 6f9fb7c575c1b1fca820ae0481df5d3cdcdd0d4e Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Thu, 26 Sep 2013 12:39:30 +0200
Subject: [PATCH] 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.
---
 C4/MarcModificationTemplates.pm            |  179 ++++++++++++++++------------
 Koha/SimpleMARC.pm                         |    2 +-
 t/SimpleMARC.t                             |    7 +-
 t/db_dependent/MarcModificationTemplates.t |  140 +++++++++++++++++++++-
 4 files changed, 245 insertions(+), 83 deletions(-)

diff --git a/C4/MarcModificationTemplates.pm b/C4/MarcModificationTemplates.pm
index 2c7bb97..ef48c3d 100644
--- a/C4/MarcModificationTemplates.pm
+++ b/C4/MarcModificationTemplates.pm
@@ -492,82 +492,111 @@ sub ModifyRecordsWithTemplate {
 =cut
 
 sub ModifyRecordWithTemplate {
-  my ( $template_id, $record ) = @_;
-  C4::Koha::Log( "C4::MarcModificationTemplates::ModifyRecordWithTemplate( $template_id, $record )" ) if DEBUG;
-  warn( "C4::MarcModificationTemplates::ModifyRecordWithTemplate( $template_id, $record )" ) if DEBUG;
-  C4::Koha::Log( "Unmodified Record:\n" . $record->as_formatted() ) if DEBUG >= 10;
-  warn( "Unmodified Record:\n" . $record->as_formatted() ) if DEBUG >= 10;
-
-  my $current_date = DateTime->now()->ymd();
-  my $branchcode = C4::Context->userenv->{branch};
-
-  my @actions = GetModificationTemplateActions( $template_id );
-
-  foreach my $a ( @actions ) {
-    my $action = $a->{'action'};
-    my $field_number = $a->{'field_number'};
-    my $from_field = $a->{'from_field'};
-    my $from_subfield = $a->{'from_subfield'};
-    my $field_value = $a->{'field_value'};
-    my $to_field = $a->{'to_field'};
-    my $to_subfield = $a->{'to_subfield'};
-    my $to_regex = $a->{'to_regex'};
-    my $conditional = $a->{'conditional'};
-    my $conditional_field = $a->{'conditional_field'};
-    my $conditional_subfield = $a->{'conditional_subfield'};
-    my $conditional_comparison = $a->{'conditional_comparison'};
-    my $conditional_value = $a->{'conditional_value'};
-    my $conditional_regex = $a->{'conditional_regex'};
-
-    my $eval = "$action( \$record, '$from_field', '$from_subfield', ";
-
-    if ( $field_value ) {
-      C4::Koha::Log( "Field value before replacements: $field_value" ) if ( DEBUG >= 3 );
-      warn( "Field value before replacements: $field_value" ) if ( DEBUG >= 3 );
-
-      $field_value =~ s/__CURRENTDATE__/$current_date/g;
-      $field_value =~ s/__BRANCHCODE__/$branchcode/g;
-
-      $eval .= " undef, " if ( $action eq 'update_field' );
-      $eval .= " '$field_value' ";
-
-      C4::Koha::Log( "Field value after replacements: $field_value" ) if ( DEBUG >= 3 );
-      warn( "Field value after replacements: $field_value" ) if ( DEBUG >= 3 );
-    } elsif ( $to_field ) {
-      $eval .= " '$to_field', '$to_subfield', '$to_regex' ";
-    }
-
-    $eval .= ", '$field_number' " if ( $field_number );
-    $eval .= ') ';
-
-    if ( $conditional ) {
-      $eval .= " $conditional ( ";
-
-      if ( $conditional_comparison eq 'exists' ) {
-        $eval .= "field_exists( \$record, '$conditional_field', '$conditional_subfield' )";
-
-      } elsif ( $conditional_comparison eq 'not_exists' ) {
-        $eval .= "!field_exists( \$record, '$conditional_field', '$conditional_subfield' )";
-
-      } elsif ( $conditional_comparison eq 'equals' ) {
-        $eval .= "field_equals( \$record, '$conditional_value', '$conditional_field', '$conditional_subfield', '$conditional_regex' ) ";
-
-      } elsif ( $conditional_comparison eq 'not_equals' ) {
-        $eval .= "!field_equals( \$record, '$conditional_value', '$conditional_field', '$conditional_subfield', '$conditional_regex' ) ";
-      }
-
-      $eval .= " )";
+    my ( $template_id, $record ) = @_;
+    C4::Koha::Log( "C4::MarcModificationTemplates::ModifyRecordWithTemplate( $template_id, $record )" ) if DEBUG;
+    warn( "C4::MarcModificationTemplates::ModifyRecordWithTemplate( $template_id, $record )" ) if DEBUG;
+    C4::Koha::Log( "Unmodified Record:\n" . $record->as_formatted() ) if DEBUG >= 10;
+    warn( "Unmodified Record:\n" . $record->as_formatted() ) if DEBUG >= 10;
+
+    my $current_date = DateTime->now()->ymd();
+    my $branchcode = C4::Context->userenv->{branch};
+
+    my @actions = GetModificationTemplateActions( $template_id );
+
+    foreach my $a ( @actions ) {
+        my $action = $a->{'action'};
+        my $field_number = $a->{'field_number'};
+        my $from_field = $a->{'from_field'};
+        my $from_subfield = $a->{'from_subfield'};
+        my $field_value = $a->{'field_value'};
+        my $to_field = $a->{'to_field'};
+        my $to_subfield = $a->{'to_subfield'};
+        my $to_regex = $a->{'to_regex'};
+        my $conditional = $a->{'conditional'};
+        my $conditional_field = $a->{'conditional_field'};
+        my $conditional_subfield = $a->{'conditional_subfield'};
+        my $conditional_comparison = $a->{'conditional_comparison'};
+        my $conditional_value = $a->{'conditional_value'};
+        my $conditional_regex = $a->{'conditional_regex'};
+
+        if ( $field_value ) {
+            $field_value =~ s/__CURRENTDATE__/$current_date/g;
+            $field_value =~ s/__BRANCHCODE__/$branchcode/g;
+        }
+
+        my @params = ( $record, $from_field, $from_subfield );
+        if ( $action eq 'update_field' ) {
+            push @params,
+                ( $field_value
+                    ? ( undef, $field_value )
+                    : ()
+                );
+        } else {
+            push @params,
+                ( $field_value
+                    ? $field_value
+                    : ()
+                );
+        }
+        push @params, (
+                ( ( not $field_value and $to_field )
+                    ? ( $to_field, $to_subfield, $to_regex )
+                    : () ),
+                ( $field_number
+                    ? $field_number
+                    : () )
+        );
+
+        my $do = 1;
+        if ( $conditional ) {
+            for ( $conditional_comparison ) {
+                when ( /^exists$/ ) {
+                    my $exists = field_exists( $record, $conditional_field, $conditional_subfield );
+                    $do = $conditional eq 'if'
+                        ? $exists
+                        : not $exists;
+                }
+                when ( /^not_exists$/ ) {
+                    my $exists = field_exists( $record, $conditional_field, $conditional_subfield );
+                    $do = $conditional eq 'if'
+                        ? not $exists
+                        : $exists;
+                }
+                when ( /^equals$/ ) {
+                    my $equals = field_equals( $record, $conditional_value, $conditional_field, $conditional_subfield, $conditional_regex );
+                    $do = $conditional eq 'if'
+                        ? $equals
+                        : not $equals;
+                }
+                when ( /^not_equals$/ ) {
+                    my $equals = field_equals( $record, $conditional_value, $conditional_field, $conditional_subfield, $conditional_regex );
+                    $do = $conditional eq 'if'
+                        ? not $equals
+                        : $equals;
+                }
+            }
+        }
+
+        if ( $do ) {
+            for ( $action ) {
+                when ( /^copy_field$/ ) {
+                    copy_field( @params );
+                }
+                when ( /^update_field$/ ) {
+                    update_field( @params );
+                }
+                when ( /^move_field$/ ) {
+                    move_field( @params );
+                }
+                when ( /^delete_field$/ ) {
+                    delete_field( @params );
+                }
+            }
+        }
+
+        C4::Koha::Log( $record->as_formatted() ) if DEBUG >= 10;
+        warn( $record->as_formatted() ) if DEBUG >= 10;
     }
-
-    $eval .= ";";
-
-    C4::Koha::Log("eval $eval") if DEBUG >= 2;
-    warn("eval $eval") if DEBUG >= 2;
-    eval {$eval};
-    C4::Koha::Log( $record->as_formatted() ) if DEBUG >= 10;
-    warn( $record->as_formatted() ) if DEBUG >= 10;
-
-  }
 }
 1;
 __END__
diff --git a/Koha/SimpleMARC.pm b/Koha/SimpleMARC.pm
index ef40694..e78fc92 100644
--- a/Koha/SimpleMARC.pm
+++ b/Koha/SimpleMARC.pm
@@ -125,7 +125,7 @@ sub update_field {
           $field->update( "$subfieldName" => $values[$i++] );
         }
       }
-      if ( $i <= scalar @values - 1 ) {
+      if ( $i <= scalar ( @values ) - 1 ) {
         foreach my $field ( @fields ) {
           foreach my $j ( $i .. scalar( @values ) - 1) {
             $field->add_subfields( "$subfieldName" => $values[$j] );
diff --git a/t/SimpleMARC.t b/t/SimpleMARC.t
index cfba615..def2edb 100644
--- a/t/SimpleMARC.t
+++ b/t/SimpleMARC.t
@@ -1,14 +1,11 @@
 use Modern::Perl;
 
-use Test::More;#tests => 1;
-
-use Data::Dumper;
+use Test::More tests => 33;
 
 use_ok("MARC::Field");
 use_ok("MARC::Record");
 use_ok("Koha::SimpleMARC");
 
-
 sub new_record {
     my $record = MARC::Record->new;
     $record->leader('03174nam a2200445 a 4500');
@@ -184,5 +181,3 @@ move_field( $record, '952', 'p', '954', 'p' ); # Move all field
 is_deeply( \@fields_952p, [], 'All 952$p have moved' );
 is_deeply( \@fields_954p, ['3010023917', '3010023917'], 'Now 2 954$p exist' );
 
-
-done_testing;
diff --git a/t/db_dependent/MarcModificationTemplates.t b/t/db_dependent/MarcModificationTemplates.t
index d253b82..c31983e 100644
--- a/t/db_dependent/MarcModificationTemplates.t
+++ b/t/db_dependent/MarcModificationTemplates.t
@@ -1,9 +1,18 @@
 use Modern::Perl;
 
-use Test::More tests => 66;
+use Test::More tests => 74;
 
+use_ok("MARC::Field");
+use_ok("MARC::Record");
 use_ok("C4::MarcModificationTemplates");
 
+my $dbh = C4::Context->dbh;
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
+
+$dbh->do(q|DELETE FROM marc_modification_template_actions|);
+$dbh->do(q|DELETE FROM marc_modification_templates|);
+
 # Creation
 my $template_id = AddModificationTemplate("template_name");
 like( $template_id, qr|^\d+$|, "new template returns an id" );
@@ -123,3 +132,132 @@ is( GetModificationTemplateAction( $actions[1]->{mmta_id} ), undef, "second acti
 is( GetModificationTemplateAction( $actions[2]->{mmta_id} ), undef, "third action does not exist anymore" );
 
 is( GetModificationTemplateActions( $template_id ), 0, "There is no action for deleted template" );
+
+# ModifyRecordWithTemplate
+my @USERENV = (
+    1,
+    'test',
+    'MASTERTEST',
+    'Test',
+    'Test',
+    't',
+    'Test',
+    0,
+);
+C4::Context->_new_userenv ('DUMMY_SESSION_ID');
+C4::Context->set_userenv ( @USERENV );
+
+$template_id = AddModificationTemplate("template_name");
+like( $template_id, qr|^\d+$|, "new template returns an id" );
+
+is( AddModificationTemplateAction(
+    $template_id, 'copy_field', 0,
+    '245', 'a', '',
+    '246', 'a', '',
+    '', '', '', '', '', '', '',
+    'copy field 245$a to 246$a'
+), 1, 'Add first action: copy 245$a to 246$a');
+
+is( AddModificationTemplateAction(
+    $template_id, 'delete_field', 0,
+    '650', 'a', '',
+    '', '', '',
+    'if', '650', '9', 'equals', '462', '',
+    'Delete field 650$a if 650$9=462'
+), 1, 'Add second action: delete field 650$a if 650$9=462');
+
+is( AddModificationTemplateAction(
+    $template_id, 'update_field', 0,
+    '952', 'p', '3010023917_updated',
+    '', '', '',
+    'unless', '650', '9', 'equals', '42', '',
+    'Update field 952$p with "3010023917_updated" if 650$9 != 42'
+), 1, 'Add third action: update field 952$p with "3010023917_updated" if 650$9 != 42');
+
+is( AddModificationTemplateAction(
+    $template_id, 'move_field', 0,
+    '952', 'd', '',
+    '952', 'e', '',
+    'if', '952', 'c', 'equals', '/^GEN/', '1',
+    'Move field 952$d to 952$e if 952$c =~ /^GE/'
+), 1, 'Add fourth action: move field 952$d to 952$e if 952$c =~ /^GE/');
+
+my $record = new_record();
+
+ModifyRecordWithTemplate( $template_id, $record );
+
+my $expected_record = expected_record();
+is_deeply( $record, $expected_record );
+
+done_testing;
+
+sub new_record {
+    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',
+            a => 'Computer programming.',
+            9 => '462',
+        ),
+        MARC::Field->new(
+            952, ' ', ' ',
+            p => '3010023917',
+            y => 'BK',
+            c => 'GEN',
+            d => '2001-06-25',
+        ),
+    );
+    $record->append_fields(@fields);
+    return $record;
+}
+
+sub expected_record {
+    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',
+        ),
+    );
+    $record->append_fields(@fields);
+    return $record;
+}
+
+
+# C4::Context->userenv
+sub Mock_userenv {
+    return { branchcode => 'CPL' };
+}
-- 
1.7.10.4