From 0b93141680c3fd889637050d964a362a2818543b Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 23 Feb 2017 12:46:52 +0100 Subject: [PATCH] Bug 9988: Few subtle changes for postponed merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Marc Véron Signed-off-by: Jacek Ablewicz Signed-off-by: Julian Maurice --- C4/AuthoritiesMarc.pm | 42 ++++++++++++++++++++++++++------------ authorities/merge.pl | 5 +++-- t/db_dependent/Authorities/Merge.t | 2 +- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 8c6c273906..87620b45cf 100644 --- 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 4279fef307..0230dbf73b 100755 --- 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 0ae6124d6b..b2cd97ea74 100755 --- 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' ); -- 2.11.0