From fdb7ed79c3e9fc9858f5dc57468a114102ca7830 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 Content-Type: text/plain; charset=utf-8 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 --- C4/AuthoritiesMarc.pm | 14 +++++++++++--- t/db_dependent/Authorities/Merge.t | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 87620b4..ea554f8 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/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t index b2cd97e..0df806f 100755 --- a/t/db_dependent/Authorities/Merge.t +++ b/t/db_dependent/Authorities/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.1.4