From b969aa3fa3d256e7a2adaf9a78df0ad8e99208e4 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 19 Jun 2014 18:15:36 +0200 Subject: [PATCH] Bug 11413: Fix field_numbers This fix is a global fix for the MarcModificationTemplate feature. Some unit tests were missing and some behaviors were wrong. For instance, if you tried to update a non existent field, the script crashed. The following line was completely stupid: if $from_field ne $to_subfield The field_number equals 1 if the user wants to update the first field and 0 for all fields. The field_numbers (note the s) variable contains the field numbers to update. This array is filled if a condition exists (field exists or field equals). Signed-off-by: Brendan Gallagher Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- 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 f0cccda7b6..3af00136d3 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 0c2c403ade..3d99deca06 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 e8bfcbc1c2..f54527e84f 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.39.5