Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
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 8c6c273..87620b4 100644 (file)
--- a/C4/AuthoritiesMarc.pm
+++ b/C4/AuthoritiesMarc.pm
@@ -1,4 +1,5 @@
 package C4::AuthoritiesMarc;
+
 # Copyright 2000-2002 Katipo Communications
 #
 # This file is part of Koha.
@@ -683,17 +684,20 @@ sub AddAuthority {
 
 =head2 DelAuthority
 
-    DelAuthority({ authid => $authid });
+    DelAuthority({ authid => $authid, [ skip_merge => 1 ] });
 
-Deletes $authid and calls merge to cleanup in linked biblio records
+Deletes $authid and calls merge to cleanup linked biblio records.
+Parameter skip_merge is used in authorities/merge.pl. You should normally not
+use it.
 
 =cut
 
 sub DelAuthority {
     my ( $params ) = @_;
     my $authid = $params->{authid} || return;
-    my $dbh=C4::Context->dbh;
-    merge({ mergefrom => $authid, MARCfrom => GetAuthority($authid) });
+    my $skip_merge = $params->{skip_merge};
+    my $dbh = C4::Context->dbh;
+    merge({ mergefrom => $authid }) if !$skip_merge;
     $dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid );
     logaction( "AUTHORITIES", "DELETE", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog");
     ModZebra( $authid, "recordDelete", "authorityserver", undef);
@@ -1370,19 +1374,24 @@ sub AddAuthorityTrees{
 =head2 merge
 
     $count = merge({
-        mergefrom => mergefrom,
-        MARCfrom => $MARCfrom,
+        mergefrom => $mergefrom,
+        [ MARCfrom => $MARCfrom, ]
         [ mergeto => $mergeto, ]
         [ MARCto => $MARCto, ]
         [ biblionumbers => [ $a, $b, $c ], ]
         [ override_limit => 1, ]
     });
 
-Merge biblios linked to authority $mergefrom.
+Merge biblios linked to authority $mergefrom (mandatory parameter).
 If $mergeto equals mergefrom, the linked biblio field is updated.
 If $mergeto is different, the biblio field will be linked to $mergeto.
 If $mergeto is missing, the biblio field is deleted.
 
+MARCfrom is used to determine if a cleared subfield in the authority record
+should be removed from a biblio. MARCto is used to populate the biblio
+record with the updated values; if you do not pass it, the biblio field
+will be deleted (same as missing mergeto).
+
 Normally all biblio records linked to $mergefrom, will be considered. But
 you can pass specific numbers via the biblionumbers parameter.
 
@@ -1435,16 +1444,22 @@ sub merge {
     my @record_to;
     @record_to = $MARCto->field($auth_tag_to_report_to)->subfields() if $auth_tag_to_report_to && $MARCto && $MARCto->field($auth_tag_to_report_to);
     my @record_from;
-    @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields() if $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from);
+    if( !$authfrom && $MARCfrom && $MARCfrom->field('1..','2..') ) {
+    # postponed merge, authfrom was deleted and MARCfrom only contains the old reporting tag (and possibly a 100 for UNIMARC)
+    # 2XX is for UNIMARC; we use -1 in order to skip 100 in UNIMARC; this will not impact MARC21, since there is only one tag
+        @record_from = ( $MARCfrom->field('1..','2..') )[-1]->subfields;
+    } elsif( $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from) ) {
+        @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields;
+    }
 
-    # Get All candidate Tags for the change 
+    # Get all candidate tags for the change
     # (This will reduce the search scope in marc records).
     # For a deleted authority record, we scan all auth controlled fields
     my $dbh = C4::Context->dbh;
     my $sql = "SELECT DISTINCT tagfield FROM marc_subfield_structure WHERE authtypecode=?";
     my $tags_using_authtype = $authtypefrom ? $dbh->selectcol_arrayref( $sql, undef, ( $authtypefrom->authtypecode )) : $dbh->selectcol_arrayref( "SELECT DISTINCT tagfield FROM marc_subfield_structure WHERE authtypecode IS NOT NULL AND authtypecode<>''" );
     my $tags_new;
-    if( $authtypefrom && $authtypeto && $authtypeto->authtypecode ne $authtypefrom->authtypecode ) {
+    if( $authtypeto && ( !$authtypefrom || $authtypeto->authtypecode ne $authtypefrom->authtypecode )) {
         $tags_new = $dbh->selectcol_arrayref( $sql, undef, ( $authtypeto->authtypecode ));
     }  
 
@@ -1470,9 +1485,10 @@ sub merge {
                 my $tag         = $field->tag();
                 next if !defined($auth_number) || $auth_number ne $mergefrom;
                 $countfrom++;
-                if ( !$mergeto || ( $overwrite && $countfrom > 1 ) ) {
-                    # if mergeto is missing, this indicates a delete
-                    # Or: remove this duplicate in strict mode
+                if ( !$mergeto || !@record_to ||
+                  ( $overwrite && $countfrom > 1 ) ) {
+                    # !mergeto or !record_to indicates a delete
+                    # Other condition: remove this duplicate in strict mode
                     $marcrecord->delete_field($field);
                     $update = 1;
                     next;

diff --git a/authorities/merge.pl b/authorities/merge.pl
index 4279fef..0230dbf 100755 (executable)
--- a/authorities/merge.pl
+++ b/authorities/merge.pl
@@ -65,8 +65,9 @@ if ($merge) {
     my $MARCfrom = GetAuthority( $recordid2 );
     merge({ mergefrom => $recordid2, MARCfrom => $MARCfrom, mergeto => $recordid1, MARCto => $record });
 
-    # Deleting the other record
-    DelAuthority({ authid => $recordid2 });
+    # Delete the other record. Do not merge. It is unneeded and could under
+    # special circumstances have unwanted side-effects.
+    DelAuthority({ authid => $recordid2, skip_merge => 1 });
 
     # Parameters
     $template->param(

diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t
index 0ae6124..b2cd97e 100755 (executable)
--- a/t/db_dependent/Authorities/Merge.t
+++ b/t/db_dependent/Authorities/Merge.t
@@ -323,7 +323,7 @@ subtest "Test some specific postponed merge issues" => sub {
     # The modify merge would be useless after that.
     @linkedrecords = ( $biblionumber );
     my $restored_mocks = set_mocks();
-    DelAuthority({ authid => $id }); # delete A
+    DelAuthority({ authid => $id, skip_merge => 1 }); # delete A
     $restored_mocks->{auth_mod}->unmock_all;
     $biblio = C4::Biblio::GetMarcBiblio( $biblionumber );
     is( $biblio->subfield('109', '9'), $id, 'If the 109 is no longer present, another modify merge would not bring it back' );