Bug 17913: Do not keep a cleared subfield in loose merge mode

If you modify an authority and clear a specific subfield, you expect that
merge respects your edit and clears this subfield too in the biblio
records. It does in the new strict mode, but it does not yet in the
default loose mode.

This patch fixes that by adjusting the code around $exclude so that it
uses a new hash skip_subfields, built from the reporting tags from the old
and the new authority record.

This is supported again by some changes in the unit test.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
index 53be7d2..72d64f2 100644 (file)
--- a/C4/AuthoritiesMarc.pm
+++ b/C4/AuthoritiesMarc.pm
@@ -1467,6 +1467,12 @@ sub merge {
     # BulkEdit marc records
     # May be used as a template for a bulkedit field  
     my $overwrite = C4::Context->preference( 'AuthorityMergeMode' ) eq 'strict';
+    my $skip_subfields = $overwrite
+        # This hash contains all subfields from the authority report fields
+        # Including $MARCfrom as well as $MARCto
+        # We only need it in loose merge mode; replaces the former $exclude
+        ? {}
+        : { map { ( $_->[0], 1 ); } ( @record_from, @record_to ) };
     foreach my $marcrecord(@reccache){
         my $update = 0;
         foreach my $tagfield (@$tags_using_authtype){
@@ -1485,17 +1491,16 @@ sub merge {
                         $field->indicator(2),
                         "9" => $mergeto,
                     );
-               my $exclude='9';
                 foreach my $subfield (grep {$_->[0] ne '9'} @record_to) {
                     $field_to->add_subfields($subfield->[0] =>$subfield->[1]);
-                   $exclude.= $subfield->[0];
                 }
-               $exclude='['.$exclude.']';
-#              add subfields in $field not included in @record_to
-        my @restore= $overwrite ? () : grep {$_->[0]!~/$exclude/} $field->subfields();
-                foreach my $subfield (@restore) {
-                   $field_to->add_subfields($subfield->[0] =>$subfield->[1]);
-               }
+                if( !$overwrite ) {
+                    # add subfields back in loose mode, check skip_subfields
+                    foreach my $subfield ( $field->subfields ) {
+                        next if $skip_subfields->{ $subfield->[0] };
+                        $field_to->add_subfields( $subfield->[0], $subfield->[1] );
+                    }
+                }
             if( $tags_new ) {
                 $marcrecord->delete_field( $field );
                 append_fields_ordered( $marcrecord, $field_to );

diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t
index 8ea7817..dce2530 100755 (executable)
--- a/t/db_dependent/Authorities/Merge.t
+++ b/t/db_dependent/Authorities/Merge.t
@@ -137,7 +137,7 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub {
 # Tests were aimed for bug 9988, moved to 17909 in adjusted form
 # Would not encourage this type of merge, but we should test what we offer
 # The merge routine still needs the fixes on bug 17913
-    plan tests => 12;
+    plan tests => 13;
 
     # create two auth recs of different type
     my $auth1 = MARC::Record->new;
@@ -193,6 +193,11 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub {
     is( $newbiblio->subfield( '112', 'c' ),
         $auth2->subfield( '112', 'c' ), 'Check new 112c' );
 
+    # Check 112b; this subfield was cleared when moving from 109 to 112
+    # Note that this fix only applies to the current loose mode only
+    is( $newbiblio->subfield( '112', 'b' ), undef,
+        'Merge respects a cleared subfield in loose mode' );
+
     # Check the original 612
     is( ( $newbiblio->field('612') )[0]->subfield( 'a' ),
         $oldbiblio->subfield( '612', 'a' ), 'Check untouched 612a' );