From 99cef606b57a1f9fc9fe5480cf446047722e9a23 Mon Sep 17 00:00:00 2001 From: Janusz Kaczmarek Date: Mon, 25 Jul 2022 22:27:32 +0200 Subject: [PATCH 1/2] Bug 19693: Add test sub to Merge.t In those rare cases when authorities are updated by an external agency (or even internally, by reviewing and correcting an exported authority file) when the heading tag will be changed (seems odd but happens: 111 Congress ==> 110 Corporate body.Congress ; 100 Person ==> 110 Corporate body (a company named with person's name ; 151 City--object ==> 150 Object (city) etc.) and then the authority record in Koha database will be updated with bulkmarcimport or by calling directly ModAuthority from a custom script, the merge function "doesn't know" that the change to the authority type has been made and, consequently, doesn't adequately change the tag in related fields in biblio records (as it would if two different records with different authtypecode were merged with Koha interface). This is because at the moment when merge function is being called by ModAuthority: Koha::Authority::Types->find($autfrom->authtypecode) Koha::Authority::Types->find($authto->authtypecode) both have the same value (because $mergefrom == $mergeto). Therefore in case when $mergefrom == $mergeto and the heading tag changes, $authtypefrom and $authfrom calculated in merge in the ordinar way are misleading and should not be taken unto consideration. Test plan: ========== 1. run t/db_dependent/Authority/Merge.t 2. you should see problems in "Test update A with modified heading tag (changing authtype)" 3. apply the patch 4. run t/db_dependent/Authority/Merge.t again 5. the test should pass Alternatively: 1. have an authority record used in biblio; export it to file; change 1XX heading tag to a different (but reasonable) value and possibly change also the content of the heading (one can delete also 942 but it doesn't matter); make bulkmarcimport.pl -a -update -file and see that the tag in biblio record has not been changed (whereas the type of authority record did change); 2. make orders in database (so that the authority type and the tag of the field in biblio record correspond); apply the patch; 3. repeat the test from 1. --- t/db_dependent/Authority/Merge.t | 84 +++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/t/db_dependent/Authority/Merge.t b/t/db_dependent/Authority/Merge.t index 3041662f50..0720ab329b 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 => 10; +use Test::More tests => 11; use Getopt::Long; use MARC::Record; @@ -21,7 +21,7 @@ use Koha::Biblios; use Koha::Database; BEGIN { - use_ok('C4::AuthoritiesMarc', qw( merge AddAuthority compare_fields DelAuthority )); + use_ok('C4::AuthoritiesMarc', qw( merge AddAuthority compare_fields DelAuthority ModAuthority )); } # Optionally change marc flavour @@ -245,6 +245,86 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub { 'Check 612x' ); }; +subtest 'Test update A with modified heading tag (changing authtype)' => sub { +# Bug 19693 +# This would happen rarely when updating authorities from authority file +# when the tag of a heading field is being changed (and thus also +# the authtype) + plan tests => 13; + + # Get back to loose mode now + t::lib::Mocks::mock_preference('AuthorityMergeMode', 'loose'); + + # create an auth rec + my $auth1 = MARC::Record->new; + $auth1->append_fields( MARC::Field->new( '109', '0', '0', 'a' => 'George Orwell', b => 'bb' )); + my $authid1 = AddAuthority( $auth1, undef, $authtype1 ); + + # create a biblio with one 109 and two 609s to be touched + # seems exceptional see bug 13760 comment10 + my $marc = MARC::Record->new; + $marc->append_fields( + MARC::Field->new( '003', 'some_003' ), + MARC::Field->new( '109', '', '', a => 'G. Orwell', b => 'bb', d => 'd', 9 => $authid1 ), + MARC::Field->new( '245', '', '', a => 'My title' ), + MARC::Field->new( '609', '', '', a => 'Orwell', 9 => "$authid1" ), + MARC::Field->new( '609', '', '', a => 'Orwell', x => 'xx', 9 => "$authid1" ), + MARC::Field->new( '611', '', '', a => 'Added for testing order' ), + MARC::Field->new( '612', '', '', a => 'unrelated', 9 => 'other' ), + ); + my ( $biblionumber ) = C4::Biblio::AddBiblio( $marc, '' ); + my $oldbiblio = Koha::Biblios->find($biblionumber)->metadata->record; + + # Time to merge + @linkedrecords = ( $biblionumber ); + my $auth2 = MARC::Record->new; + $auth2->append_fields( MARC::Field->new( '112', '0', '0', 'a' => 'Batman', c => 'cc' )); + my $authid2 = ModAuthority($authid1, $auth2, $authtype2 ); + # inside ModAuthority the following is executed: + # merge({ mergefrom => $authid1, MARCfrom => $auth1, mergeto => $authid1, MARCto => $auth2 }); + is( $authid2, $authid1, 'authid after ModAuthority OK' ); + + # Get new marc record for compares + my $newbiblio = Koha::Biblios->find($biblionumber)->metadata->record; + compare_fields( $oldbiblio, $newbiblio, {}, 'count' ); + # Exclude 109/609 and 112/612 in comparing order + my $excl = { '109' => 1, '112' => 1, '609' => 1, '612' => 1 }; + compare_fields( $oldbiblio, $newbiblio, $excl, 'order' ); + # Check position of 612s in the new record + my $full_order = join q/,/, map { $_->tag } $newbiblio->fields; + is( $full_order =~ /611(,612){3}/, 1, 'Check position of all 612s' ); + + # Check some fields + is( $newbiblio->field('003')->data, + $oldbiblio->field('003')->data, + 'Check contents of a control field not expected to be touched' ); + is( $newbiblio->subfield( '245', 'a' ), + $oldbiblio->subfield( '245', 'a' ), + 'Check contents of a data field not expected to be touched' ); + is( $newbiblio->subfield( '112', 'a' ), + $auth2->subfield( '112', 'a' ), 'Check modified 112a' ); + 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' ); + # Check second 612 + is( ( $newbiblio->field('612') )[1]->subfield( 'a' ), + $auth2->subfield( '112', 'a' ), 'Check second touched 612a' ); + # Check second new 612ax (in LOOSE mode) + is( ( $newbiblio->field('612') )[2]->subfield( 'a' ), + $auth2->subfield( '112', 'a' ), 'Check touched 612a' ); + is( ( $newbiblio->field('612') )[2]->subfield( 'x' ), + ( $oldbiblio->field('609') )[1]->subfield('x'), + 'Check 612x' ); +}; + subtest 'Merging authorities should handle deletes (BZ 18070)' => sub { plan tests => 2; -- 2.30.2