From 3d3bcba5bf5367315a774a8724ce03f817cdb7ed Mon Sep 17 00:00:00 2001 From: Ere Maijala Date: Tue, 22 Jan 2019 14:38:32 +0200 Subject: [PATCH] Bug 21958: Fix bibliographic record field comparison with authority This makes the comparison between bibliographic field and authority field more robust and per subfield. This makes the comparison not consider the same e.g. the following fields: $a Test User $a Test $b User The actual issue cannot be as easily reproduced with the patches for bug 21826 applied, but here's a test plan anyway: 1. Make sure tests pass (especially t/db_dependent/AuthoritiesMarc*) 2. Make sure authority linking still works properly 3. Make sure authority and biblio frameworks allow subfield i 4. Make sure that even if you add subfield i to 700 in biblio, authority link is kept the same 5. Make sure that even if you add subfield i to the authority record, the authority link is kept the same --- C4/AuthoritiesMarc.pm | 121 +++++++++++---------- C4/Biblio.pm | 4 +- C4/Heading.pm | 2 + C4/Heading/MARC21.pm | 120 ++++++++++++++++++++ .../marc21/mandatory/authorities_normal_marc21.sql | 5 +- t/db_dependent/AuthoritiesMarc.t | 28 ++++- t/db_dependent/AuthoritiesMarc/MARC21.t | 91 ++++++++++++++++ 7 files changed, 308 insertions(+), 63 deletions(-) create mode 100644 t/db_dependent/AuthoritiesMarc/MARC21.t diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 0d90b37d67..b730d82ffd 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -1,6 +1,7 @@ package C4::AuthoritiesMarc; # Copyright 2000-2002 Katipo Communications +# Copyright 2018 The National Library of Finland, University of Helsinki # # This file is part of Koha. # @@ -359,10 +360,12 @@ sub GuessAuthTypeCode { '110'=>{authtypecode=>'CORPO_NAME'}, '111'=>{authtypecode=>'MEETI_NAME'}, '130'=>{authtypecode=>'UNIF_TITLE'}, + '147'=>{authtypecode=>'NAME_EVENT'}, '148'=>{authtypecode=>'CHRON_TERM'}, '150'=>{authtypecode=>'TOPIC_TERM'}, '151'=>{authtypecode=>'GEOGR_NAME'}, '155'=>{authtypecode=>'GENRE/FORM'}, + '162'=>{authtypecode=>'MED_PERFRM'}, '180'=>{authtypecode=>'GEN_SUBDIV'}, '181'=>{authtypecode=>'GEO_SUBDIV'}, '182'=>{authtypecode=>'CHRON_SUBD'}, @@ -938,6 +941,8 @@ sub BuildSummary { # construct MARC21 summary # FIXME - looping over 1XX is questionable # since MARC21 authority should have only one 1XX + use C4::Heading::MARC21; + my $handler = C4::Heading::MARC21->new(); my $subfields_to_report; foreach my $field ($record->field('1..')) { my $tag = $field->tag(); @@ -945,31 +950,9 @@ sub BuildSummary { # FIXME - 152 is not a good tag to use # in MARC21 -- purely local tags really ought to be # 9XX - if ($tag eq '100') { - $subfields_to_report = 'abcdefghjklmnopqrstvxyz'; - } elsif ($tag eq '110') { - $subfields_to_report = 'abcdefghklmnoprstvxyz'; - } elsif ($tag eq '111') { - $subfields_to_report = 'acdefghklnpqstvxyz'; - } elsif ($tag eq '130') { - $subfields_to_report = 'adfghklmnoprstvxyz'; - } elsif ($tag eq '148') { - $subfields_to_report = 'abvxyz'; - } elsif ($tag eq '150') { - $subfields_to_report = 'abvxyz'; - } elsif ($tag eq '151') { - $subfields_to_report = 'avxyz'; - } elsif ($tag eq '155') { - $subfields_to_report = 'abvxyz'; - } elsif ($tag eq '180') { - $subfields_to_report = 'vxyz'; - } elsif ($tag eq '181') { - $subfields_to_report = 'vxyz'; - } elsif ($tag eq '182') { - $subfields_to_report = 'vxyz'; - } elsif ($tag eq '185') { - $subfields_to_report = 'vxyz'; - } + + $subfields_to_report = $handler->get_auth_heading_subfields_to_report($tag); + if ($subfields_to_report) { push @authorized, { heading => $field->as_string($subfields_to_report), @@ -1093,44 +1076,47 @@ sub GetAuthorizedHeading { return $field->as_string('abcdefghijlmnopqrstuvwxyz'); } } else { + use C4::Heading::MARC21; + my $handler = C4::Heading::MARC21->new(); + foreach my $field ($record->field('1..')) { - my $tag = $field->tag(); - next if "152" eq $tag; -# FIXME - 152 is not a good tag to use -# in MARC21 -- purely local tags really ought to be -# 9XX - if ($tag eq '100') { - return $field->as_string('abcdefghjklmnopqrstvxyz68'); - } elsif ($tag eq '110') { - return $field->as_string('abcdefghklmnoprstvxyz68'); - } elsif ($tag eq '111') { - return $field->as_string('acdefghklnpqstvxyz68'); - } elsif ($tag eq '130') { - return $field->as_string('adfghklmnoprstvxyz68'); - } elsif ($tag eq '148') { - return $field->as_string('abvxyz68'); - } elsif ($tag eq '150') { - return $field->as_string('abvxyz68'); - } elsif ($tag eq '151') { - return $field->as_string('avxyz68'); - } elsif ($tag eq '155') { - return $field->as_string('abvxyz68'); - } elsif ($tag eq '180') { - return $field->as_string('vxyz68'); - } elsif ($tag eq '181') { - return $field->as_string('vxyz68'); - } elsif ($tag eq '182') { - return $field->as_string('vxyz68'); - } elsif ($tag eq '185') { - return $field->as_string('vxyz68'); - } else { - return $field->as_string(); - } + my $subfields = $handler->get_valid_bib_heading_subfields($field->tag()); + return $field->as_string($subfields) if ($subfields); } } return; } +=head2 CompareFieldWithAuthority + + $match = &CompareFieldWithAuthority({ field => $field, authid => $authid }) + +Takes a MARC::Field from a bibliographic record and an authid, and returns true if they match. + +=cut + +sub CompareFieldWithAuthority { + my $args = shift; + + my $record = GetAuthority($args->{authid}); + return unless (ref $record eq 'MARC::Record'); + if (C4::Context->preference('marcflavour') eq 'UNIMARC') { + # UNIMARC has same subfields for bibs and authorities + foreach my $field ($record->field('2..')) { + return compare_fields($field, $args->{field}, 'abcdefghijlmnopqrstuvwxyz'); + } + } else { + use C4::Heading::MARC21; + my $handler = C4::Heading::MARC21->new(); + + foreach my $field ($record->field('1..')) { + my $subfields = $handler->get_valid_bib_heading_subfields($field->tag()); + return compare_fields($field, $args->{field}, $subfields) if ($subfields); + } + } + return 0; +} + =head2 BuildAuthHierarchies $text= &BuildAuthHierarchies( $authid, $force) @@ -1592,6 +1578,26 @@ sub get_auth_type_location { } } +=head2 compare_fields + + my match = compare_fields($field1, $field2, 'abcde'); + +Compares the listed subfields of both fields and return true if they all match + +=cut + +sub compare_fields { + my ($field1, $field2, $subfields) = @_; + + foreach my $subfield (split(//, $subfields)) { + my $subfield1 = $field1->subfield($subfield) // ''; + my $subfield2 = $field2->subfield($subfield) // ''; + return 0 unless $subfield1 eq $subfield2; + } + return 1; +} + + END { } # module clean-up code here (global destructor) 1; @@ -1602,6 +1608,7 @@ __END__ Koha Development Team Paul POULAIN paul.poulain@free.fr +Ere Maijala ere.maijala@helsinki.fi =cut diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 958f7d9753..d6e423739e 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -629,9 +629,7 @@ sub _check_valid_auth_link { my ( $authid, $field ) = @_; require C4::AuthoritiesMarc; - my $authorized_heading = - C4::AuthoritiesMarc::GetAuthorizedHeading( { 'authid' => $authid } ) || ''; - return ($field->as_string('abcdefghijklmnopqrstuvwxyz') eq $authorized_heading); + return C4::AuthoritiesMarc::CompareFieldWithAuthority( { 'field' => $field, 'authid' => $authid } ); } =head2 GetRecordValue diff --git a/C4/Heading.pm b/C4/Heading.pm index 5dfd0fadb0..c4296a9393 100644 --- a/C4/Heading.pm +++ b/C4/Heading.pm @@ -174,6 +174,8 @@ sub preferred_authorities { if (C4::Heading::valid_bib_heading_subfield('100', 'e', '')) ... +Check if the given subfield is valid for the given field. + =cut sub valid_bib_heading_subfield { diff --git a/C4/Heading/MARC21.pm b/C4/Heading/MARC21.pm index fc4d576a45..2dfcbf799e 100644 --- a/C4/Heading/MARC21.pm +++ b/C4/Heading/MARC21.pm @@ -68,6 +68,52 @@ my $bib_heading_fields = { subfields => 'adfghklmnoprst', main_entry => 1 }, + '147' => { + auth_type => 'NAME_EVENT', + subfields => 'acdgvxyz68', + main_entry => 1 + }, + '148' => { + auth_type => 'CHRON_TERM', + subfields => 'abvxyz68', + main_entry => 1 + }, + '150' => { + auth_type => 'TOPIC_TERM', + subfields => 'abvxyz68', + main_entry => 1 + }, + '151' => { + auth_type => 'GEOGR_NAME', + subfields => 'avxyz68', + main_entry => 1 + }, + '155' => { + auth_type => 'GENRE/FORM', + subfields => 'abvxyz68', + main_entry => 1 + }, + '162' => { + auth_type => 'MED_PERFRM', + subfields => 'a68', + main_entry => 1 + }, + '180' => { + auth_type => 'TOPIC_TERM', + subfields => 'vxyz68' + }, + '181' => { + auth_type => 'GEOGR_NAME', + subfields => 'vxyz68' + }, + '182' => { + auth_type => 'CHRON_TERM', + subfields => 'vxyz68' + }, + '185' => { + auth_type => 'GENRE/FORM', + subfields => 'vxyz68' + }, '440' => { auth_type => 'UNIF_TITLE', subfields => 'anp', series => 1 }, '600' => { auth_type => 'PERSO_NAME', @@ -119,6 +165,56 @@ my $bib_heading_fields = { { auth_type => 'UNIF_TITLE', subfields => 'adfghklmnoprst', series => 1 }, }; + +=head2 auth_heading_fields + +=cut + +my $auth_heading_fields = { + '100' => { + subfields => 'abcdefghjklmnopqrstvxyz68', + }, + '110' => { + subfields => 'abcdefghklmnoprstvxyz68', + }, + '111' => { + subfields => 'acdefghklnpqstvxyz68', + }, + '130' => { + subfields => 'adfghklmnoprstvxyz68', + }, + '147' => { + subfields => 'acdgvxyz68', + }, + '148' => { + subfields => 'abvxyz68', + }, + '150' => { + subfields => 'abvxyz68', + }, + '151' => { + subfields => 'avxyz68', + }, + '155' => { + subfields => 'abvxyz68', + }, + '162' => { + subfields => 'a68', + }, + '180' => { + subfields => 'vxyz68', + }, + '181' => { + subfields => 'vxyz68', + }, + '182' => { + subfields => 'vxyz68', + }, + '185' => { + subfields => 'vxyz68', + }, +}; + =head2 subdivisions =cut @@ -176,6 +272,30 @@ sub valid_bib_heading_subfield { return 0; } +=head2 get_valid_bib_heading_subfields + +=cut + +sub get_valid_bib_heading_subfields { + my $self = shift; + my $tag = shift; + + return $bib_heading_fields->{$tag}->{subfields} // undef; +} + +=head2 get_auth_heading_subfields_to_report + +=cut + +sub get_auth_heading_subfields_to_report { + my $self = shift; + my $tag = shift; + + my $subfields = $auth_heading_fields->{$tag}->{subfields} // ''; + $subfields =~ s/[68]//; + return $subfields; +} + =head2 parse_heading =cut diff --git a/installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql b/installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql index f605303a78..3ba82f7888 100644 --- a/installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql +++ b/installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.sql @@ -28,11 +28,12 @@ INSERT INTO auth_types (authtypecode, authtypetext, auth_tag_to_report, summary) ('CORPO_NAME', 'Corporate Name', '110', 'Corporate Names'), ('MEETI_NAME', 'Meeting Name', '111', 'Meeting Name'), ('UNIF_TITLE', 'Uniform Title', '130', 'Uniform Title'), + ('NAME_EVENT', 'Named Event', '147', 'Named Event'), ('CHRON_TERM', 'Chronological Term', '148', 'Chronological Term'), ('TOPIC_TERM', 'Topical Term', '150', 'Topical Term'), ('GEOGR_NAME', 'Geographic Name', '151', 'Geographic Name'), - ('GENRE/FORM', 'Genre/Form Term', '155', 'Genre/Form Term'); - + ('GENRE/FORM', 'Genre/Form Term', '155', 'Genre/Form Term'), + ('MED_PERFRM', 'Medium of Performance', '162', 'Medium of Performance'); -- ****************************************************** -- KOHA UNSUPPORTED STANDARD MARC 21 AUTHORITY TYPES. diff --git a/t/db_dependent/AuthoritiesMarc.t b/t/db_dependent/AuthoritiesMarc.t index a24d917f50..97f118ccf4 100755 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@ -5,9 +5,10 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 10; use Test::MockModule; use Test::Warn; +use MARC::Field; use MARC::Record; use t::lib::Mocks; @@ -54,6 +55,11 @@ $module->mock('GetAuthority', sub { [ '151', ' ', ' ', a => 'New York (City)' ], [ '551', ' ', ' ', a => 'New York (State)', w => 'g' ] ); + } elsif ($authid eq '5') { + $record->add_fields( + [ '001', '5' ], + [ '100', ' ', ' ', a => 'Lastname, Firstname', b => 'b', c => 'c', i => 'i' ] + ); } else { undef $record; } @@ -214,4 +220,24 @@ subtest 'AddAuthority should respect AUTO_INCREMENT (BZ 18104)' => sub { is( $record->field('001')->data, $id3, 'Check updated 001' ); }; +subtest 'CompareFieldWithAuthority tests' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); + + $builder->build({ source => 'AuthType', value => { authtypecode => 'PERSO_NAME' }}); + + my $field = MARC::Field->new('100', 0, 0, a => 'Lastname, Firstname', b => 'b', c => 'c'); + + ok(C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Authority matches'); + + $field->add_subfields(i => 'X'); + + ok(C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Compare ignores unlisted subfields'); + + $field->add_subfields(d => 'd'); + + ok(!C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Authority does not match'); +}; + $schema->storage->txn_rollback; diff --git a/t/db_dependent/AuthoritiesMarc/MARC21.t b/t/db_dependent/AuthoritiesMarc/MARC21.t new file mode 100644 index 0000000000..77047e4bbc --- /dev/null +++ b/t/db_dependent/AuthoritiesMarc/MARC21.t @@ -0,0 +1,91 @@ +#!/usr/bin/perl +# + +use strict; +use warnings; + +use Test::MockModule; +use Test::More tests => 2; +use MARC::Field; +use MARC::Record; + +use t::lib::Mocks; +use t::lib::TestBuilder; + +BEGIN { + use_ok('C4::AuthoritiesMarc::MARC21'); +} + +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; +my $dbh = C4::Context->dbh; +my $builder = t::lib::TestBuilder->new; + +t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); + +subtest 'CompareFieldWithAuthority tests' => sub { + plan tests => 3; + + # We are now going to be testing the authorities hierarchy code, and + # therefore need to pretend that we have consistent data in our database + my $module = new Test::MockModule('C4::AuthoritiesMarc'); + $module->mock('GetHeaderAuthority', sub { + return {'authtrees' => ''}; + }); + $module->mock('AddAuthorityTrees', sub { + return; + }); + $module->mock('GetAuthority', sub { + my ($authid) = @_; + my $record = MARC::Record->new(); + if ($authid eq '1') { + $record->add_fields( + [ '001', '1' ], + [ '151', ' ', ' ', a => 'United States' ] + ); + } elsif ($authid eq '2') { + $record->add_fields( + [ '001', '2' ], + [ '151', ' ', ' ', a => 'New York (State)' ], + [ '551', ' ', ' ', a => 'United States', w => 'g', 9 => '1' ] + ); + } elsif ($authid eq '3') { + $record->add_fields( + [ '001', '3' ], + [ '151', ' ', ' ', a => 'New York (City)' ], + [ '551', ' ', ' ', a => 'New York (State)', w => 'g', 9 => '2' ] + ); + } elsif ($authid eq '4') { + $record->add_fields( + [ '001', '4' ], + [ '151', ' ', ' ', a => 'New York (City)' ], + [ '551', ' ', ' ', a => 'New York (State)', w => 'g' ] + ); + } elsif ($authid eq '5') { + $record->add_fields( + [ '001', '5' ], + [ '100', ' ', ' ', a => 'Lastname, Firstname', b => 'b', c => 'c', i => 'i' ] + ); + } else { + undef $record; + } + return $record; + }); + + $dbh->do('DELETE FROM auth_types'); + $builder->build({ source => 'AuthType', value => { authtypecode => 'PERSO_NAME' }}); + + my $field = MARC::Field->new('100', 0, 0, a => 'Lastname, Firstname', b => 'b', c => 'c'); + + ok(C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Authority matches'); + + $field->add_subfields(i => 'X'); + + ok(C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Compare ignores unlisted subfields'); + + $field->add_subfields(d => 'd'); + + ok(!C4::AuthoritiesMarc::CompareFieldWithAuthority({'field' => $field, 'authid' => 5}), 'Authority does not match'); +}; + +$schema->storage->txn_rollback; -- 2.11.0