From 434162c1756b16b5fc715fd1cb762aaf8aff7b2b Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 4 Jul 2017 14:31:21 +0200 Subject: [PATCH] Bug 17380: Graceful resolution of missing reporting tag in merge Altough this patch deals with a mostly hypothetical case and this report makes it practically impossible anymore to merge with records in the Default framework (having no reporting tag), we can make the code of sub merge still a bit more robust here. If you would merge biblio records from one authtype to another and the new framework would not have a reporting tag, before this patch the result would be data loss. Merge would handle this request as a delete. This patch makes merge handle it differently: instead of clearing the biblio records, it keeps $a and $9 in order to make a future corrective merge possible. Note: The additional condition on line 1468 for $tags_using_authtype makes sure that we do not select all fields when the authtype should unexpectedly be empty string (Default). This prevents crashing on a "Control fields do not have subfields" error. Test plan: [1] Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens Signed-off-by: Jonathan Druart --- C4/AuthoritiesMarc.pm | 14 +++++++++++--- t/db_dependent/Authority/Merge.t | 33 +++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 8b9c6c1c8f..c1989122c7 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -1443,6 +1443,14 @@ 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); + # Exceptional: If MARCto and authtypeto exist but $auth_tag_to_report_to + # is empty, make sure that $9 and $a remain (instead of clearing the + # reference) in order to allow for data recovery. + # Note: We need $a too, since a single $9 does not pass ModBiblio. + if( $MARCto && $authtypeto && !@record_to ) { + push @record_to, [ 'a', ' ' ]; # do not remove the space + } + my @record_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) @@ -1457,7 +1465,7 @@ sub merge { # 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_using_authtype = $authtypefrom && $authtypefrom->authtypecode ? $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( $authtypeto && ( !$authtypefrom || $authtypeto->authtypecode ne $authtypefrom->authtypecode )) { $tags_new = $dbh->selectcol_arrayref( $sql, undef, ( $authtypeto->authtypecode )); @@ -1493,7 +1501,7 @@ sub merge { $update = 1; next; } - my $newtag = $tags_new + my $newtag = $tags_new && @$tags_new ? _merge_newtag( $tag, $tags_new ) : $tag; my $field_to = MARC::Field->new( @@ -1512,7 +1520,7 @@ sub merge { $field_to->add_subfields( $subfield->[0], $subfield->[1] ); } } - if ($tags_new) { + if ($tags_new && @$tags_new) { $marcrecord->delete_field($field); append_fields_ordered( $marcrecord, $field_to ); } else { diff --git a/t/db_dependent/Authority/Merge.t b/t/db_dependent/Authority/Merge.t index 29f6a804af..08a582c777 100755 --- a/t/db_dependent/Authority/Merge.t +++ b/t/db_dependent/Authority/Merge.t @@ -4,7 +4,7 @@ use Modern::Perl; -use Test::More tests => 7; +use Test::More tests => 8; use Getopt::Long; use MARC::Record; @@ -329,6 +329,37 @@ subtest "Test some specific postponed merge issues" => sub { is( $biblio->subfield('109', '9'), $id, 'If the 109 is no longer present, another modify merge would not bring it back' ); }; +subtest "Graceful resolution of missing reporting tag" => sub { + plan tests => 2; + + # Simulate merge with authority in Default fw without reporting tag + # We expect data loss in biblio, but we keep $a and the reference in $9 + # in order to allow a future merge to restore data. + + # Accomplish the above by clearing reporting tag in $authtype2 + my $fw2 = Koha::Authority::Types->find( $authtype2 ); + $fw2->auth_tag_to_report('')->store; + + my $authmarc = MARC::Record->new; + $authmarc->append_fields( MARC::Field->new( '109', '', '', 'a' => 'aa', b => 'bb' )); + my $id1 = AddAuthority( $authmarc, undef, $authtype1 ); + my $id2 = AddAuthority( $authmarc, undef, $authtype2 ); + + my $biblio = MARC::Record->new; + $biblio->append_fields( + MARC::Field->new( '609', '', '', a => 'aa', 9 => $id1 ), + ); + my ( $biblionumber ) = C4::Biblio::AddBiblio( $biblio, '' ); + + # Merge + merge({ mergefrom => $id1, MARCfrom => $authmarc, mergeto => $id2, MARCto => $authmarc, biblionumbers => [ $biblionumber ] }); + $biblio = C4::Biblio::GetMarcBiblio( $biblionumber ); + is( $biblio->subfield('612', '9'), $id2, 'id2 saved in $9' ); + is( $biblio->subfield('612', 'a'), ' ', 'Kept an empty $a too' ); + + $fw2->auth_tag_to_report('112')->store; +}; + sub set_mocks { # After we removed the Zebra code from merge, we only need to mock # get_usage_count and linked_biblionumbers here. -- 2.39.5