From 06647d29a7c02b6483f27f6c46e9d0e639236d6e Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 21 Feb 2017 09:22:59 +0100 Subject: [PATCH] Bug 9988: Remove the Zebra code from sub merge MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Since we can now call linked_biblionumbers, we can now remove all Zebra related code from merge. We also add a parameter biblionumbers; we use it in the test now, but it may be handy too later in the maintenance script when we want to trigger a merge for specific biblionumber(s). See bug report 18071. All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the merge test can now be replaced by one mock for linked_biblionumbers. Note that we test the biblionumbers parameter in the last subtest without that mock. Remove unused vars $countunmodifiedbiblio, $counterrors from merge. Renamed zebrarecords to linkedrecords in the Merge test. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] Modify an authority record. Check the linked biblio records. Signed-off-by: Marcel de Rooy Signed-off-by: Marc Véron Signed-off-by: Jacek Ablewicz Signed-off-by: Julian Maurice Signed-off-by: Kyle M Hall --- C4/AuthoritiesMarc.pm | 73 ++++++++---------------------- t/db_dependent/Authorities/Merge.t | 53 +++++++--------------- 2 files changed, 36 insertions(+), 90 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 9baaa26f7c..4aa8845a25 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -1394,6 +1394,7 @@ sub AddAuthorityTrees{ MARCfrom => $MARCfrom, [ mergeto => $mergeto, ] [ MARCto => $MARCto, ] + [ biblionumbers => [ $a, $b, $c ], ] }); Merge biblios linked to authority $mergefrom. @@ -1401,6 +1402,9 @@ 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. +Normally all biblio records linked to $mergefrom, will be considered. But +you can pass specific numbers via the biblionumbers parameter. + Note: Although $mergefrom and $mergeto will normally be of the same authority type, merge also supports moving to another authority type. @@ -1408,14 +1412,18 @@ authority type, merge also supports moving to another authority type. sub merge { my ( $params ) = @_; - my $mergefrom = $params->{mergefrom}; + my $mergefrom = $params->{mergefrom} || return; my $MARCfrom = $params->{MARCfrom}; my $mergeto = $params->{mergeto}; my $MARCto = $params->{MARCto}; + my @biblionumbers = $params->{biblionumbers} + # If we do not have biblionumbers, we get all linked biblios + ? @{ $params->{biblionumbers} } + : Koha::Authorities->linked_biblionumbers({ authid => $mergefrom }); + return 0 if !@biblionumbers; - return 0 unless $mergefrom > 0; # prevent abuse - my ($counteditedbiblio,$countunmodifiedbiblio,$counterrors)=(0,0,0); - my $dbh=C4::Context->dbh; + my $counteditedbiblio = 0; + my $dbh = C4::Context->dbh; my $authfrom = Koha::Authorities->find($mergefrom); my $authto = Koha::Authorities->find($mergeto); my $authtypefrom = $authfrom ? Koha::Authority::Types->find($authfrom->authtypecode) : undef; @@ -1429,36 +1437,7 @@ sub merge { @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); - - my @reccache; - # search all biblio tags using this authority. - #Getting marcbiblios impacted by the change. - #zebra connection - my $oConnection=C4::Context->Zconn("biblioserver",0); - # We used to use XML syntax here, but that no longer works. - # Thankfully, we don't need it. - my $query; - $query= "an=".$mergefrom; - my $oResult = $oConnection->search(new ZOOM::Query::CCL2RPN( $query, $oConnection )); - my $count = 0; - if ($oResult) { - $count=$oResult->size(); - } - my $z=0; - while ( $z<$count ) { - my $marcrecordzebra = C4::Search::new_record_from_zebra( - 'biblioserver', - $oResult->record($z)->raw() - ); - my ( $biblionumbertagfield, $biblionumbertagsubfield ) = &GetMarcFromKohaField( "biblio.biblionumber", '' ); - my $i = ($biblionumbertagfield < 10) - ? $marcrecordzebra->field( $biblionumbertagfield )->data - : $marcrecordzebra->subfield( $biblionumbertagfield, $biblionumbertagsubfield ); - my $marcrecorddb = GetMarcBiblio($i); - push @reccache, $marcrecorddb; - $z++; - } - $oResult->destroy(); + # 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 @@ -1479,7 +1458,9 @@ sub merge { # And we need to add $9 in order not to duplicate $skip_subfields->{9} = 1 if !$overwrite; - foreach my $marcrecord(@reccache){ + foreach my $biblionumber ( @biblionumbers ) { + my $marcrecord = GetMarcBiblio( $biblionumber ); + next if !$marcrecord; my $update = 0; foreach my $tagfield (@$tags_using_authtype) { my $countfrom = 0; # used in strict mode to remove duplicates @@ -1523,25 +1504,11 @@ sub merge { $update = 1; } } - my ($bibliotag,$bibliosubf) = GetMarcFromKohaField("biblio.biblionumber","") ; - my $biblionumber; - if ($bibliotag<10){ - $biblionumber=$marcrecord->field($bibliotag)->data; - } - else { - $biblionumber=$marcrecord->subfield($bibliotag,$bibliosubf); - } - unless ($biblionumber){ - warn "pas de numéro de notice bibliographique dans : ".$marcrecord->as_formatted; - next; - } - if ($update==1){ - &ModBiblio($marcrecord,$biblionumber,GetFrameworkCode($biblionumber)) ; - $counteditedbiblio++; - warn $counteditedbiblio if (($counteditedbiblio % 10) and $ENV{DEBUG}); - } + next if !$update; + ModBiblio($marcrecord, $biblionumber, GetFrameworkCode($biblionumber)); + $counteditedbiblio++; } - return $counteditedbiblio; + return $counteditedbiblio; } sub _merge_newtag { diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t index be3e420879..5e8bcf970f 100755 --- a/t/db_dependent/Authorities/Merge.t +++ b/t/db_dependent/Authorities/Merge.t @@ -9,12 +9,12 @@ use Test::More tests => 5; use Getopt::Long; use MARC::Record; use Test::MockModule; -use Test::MockObject; use t::lib::Mocks; use t::lib::TestBuilder; use C4::Biblio; +use Koha::Authorities; use Koha::Database; BEGIN { @@ -30,7 +30,7 @@ my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; # Global variables, mocking and framework modifications -our ( @zebrarecords, $index ); +our @linkedrecords; my $mocks = set_mocks(); our ( $authtype1, $authtype2 ) = modify_framework(); @@ -58,8 +58,7 @@ subtest 'Test merge A1 to A2 (within same authtype)' => sub { my ( $biblionumber2 ) = AddBiblio($biblio2, ''); # Time to merge - @zebrarecords = ( $biblio1, $biblio2 ); - $index = 0; + @linkedrecords = ( $biblionumber1, $biblionumber2 ); my $rv = C4::AuthoritiesMarc::merge({ mergefrom => $authid2, MARCfrom => $auth2, mergeto => $authid1, MARCto => $auth1 }); is( $rv, 1, 'We expect one biblio record (out of two) to be updated' ); @@ -102,8 +101,7 @@ subtest 'Test merge A1 to modified A1, test strict mode' => sub { my ( $biblionumber2 ) = AddBiblio( $MARC2, ''); # Time to merge in loose mode first - @zebrarecords = ( $MARC1, $MARC2 ); - $index = 0; + @linkedrecords = ( $biblionumber1, $biblionumber2 ); t::lib::Mocks::mock_preference('AuthorityMergeMode', 'loose'); my $rv = C4::AuthoritiesMarc::merge({ mergefrom => $authid1, MARCfrom => $auth1old, mergeto => $authid1, MARCto => $auth1new }); is( $rv, 2, 'Both records are updated now' ); @@ -123,8 +121,7 @@ subtest 'Test merge A1 to modified A1, test strict mode' => sub { # Merge again in strict mode t::lib::Mocks::mock_preference('AuthorityMergeMode', 'strict'); ModBiblio( $MARC1, $biblionumber1, '' ); - @zebrarecords = ( $MARC1 ); - $index = 0; + @linkedrecords = ( $biblionumber1 ); $rv = C4::AuthoritiesMarc::merge({ mergefrom => $authid1, MARCfrom => $auth1old, mergeto => $authid1, MARCto => $auth1new }); $biblio1 = GetMarcBiblio($biblionumber1); is( $biblio1->field(109)->subfield('b'), undef, 'Subfield overwritten in strict mode' ); @@ -170,8 +167,7 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub { my $oldbiblio = C4::Biblio::GetMarcBiblio( $biblionumber ); # Time to merge - @zebrarecords = ( $marc ); - $index = 0; + @linkedrecords = ( $biblionumber ); my $retval = C4::AuthoritiesMarc::merge({ mergefrom => $authid1, MARCfrom => $auth1, mergeto => $authid2, MARCto => $auth2 }); is( $retval, 1, 'We touched only one biblio' ); @@ -232,8 +228,7 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub { MARC::Field->new( '609', '', '', a => 'DEL', 9 => "$authid1" ), ); my ( $biblionumber ) = C4::Biblio::AddBiblio( $bib1, '' ); - @zebrarecords = ( $bib1 ); - $index = 0; + @linkedrecords = ( $biblionumber ); DelAuthority({ authid => $authid1 }); # this triggers a merge call # See what happened in the biblio record @@ -253,41 +248,25 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub { # record and call merge afterwards. # This mimics deleting an authority and calling merge later in the # merge_authority.pl cron job (when dontmerge is enabled). + # We use the biblionumbers parameter here and unmock linked_biblionumbers. C4::Context->dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid1 ); - @zebrarecords = ( $marc1 ); - $index = 0; - merge({ mergefrom => $authid1 }); + @linkedrecords = (); + $mocks->{auth_mod}->unmock_all; + merge({ mergefrom => $authid1, biblionumbers => [ $biblionumber ] }); # Final check $marc1 = C4::Biblio::GetMarcBiblio( $biblionumber ); is( $marc1->field('609'), undef, 'Merge removed the 609 again even after deleting the authority record' ); }; sub set_mocks { - # Mock ZOOM objects: They do nothing actually - # Get new_record_from_zebra to return the records + # After we removed the Zebra code from merge, we only need to mock + # linked_biblionumbers here. my $mocks; - $mocks->{context_mod} = Test::MockModule->new( 'C4::Context' ); - $mocks->{search_mod} = Test::MockModule->new( 'C4::Search' ); - $mocks->{zoom_mod} = Test::MockModule->new( 'ZOOM::Query::CCL2RPN', no_auto => 1 ); - $mocks->{conn_obj} = Test::MockObject->new; - $mocks->{zoom_obj} = Test::MockObject->new; - $mocks->{zoom_record_obj} = Test::MockObject->new; - - $mocks->{context_mod}->mock( 'Zconn', sub { $mocks->{conn_obj}; } ); - $mocks->{search_mod}->mock( 'new_record_from_zebra', sub { - return if $index >= @zebrarecords; - return $zebrarecords[ $index++ ]; + $mocks->{auth_mod} = Test::MockModule->new( 'Koha::Authorities' ); + $mocks->{auth_mod}->mock( 'linked_biblionumbers', sub { + return @linkedrecords; }); - $mocks->{zoom_mod}->mock( 'new', sub {} ); - - $mocks->{conn_obj}->mock( 'search', sub { $mocks->{zoom_obj}; } ); - $mocks->{zoom_obj}->mock( 'destroy', sub {} ); - $mocks->{zoom_obj}->mock( 'record', sub { $mocks->{zoom_record_obj}; } ); - $mocks->{zoom_obj}->mock( 'search', sub {} ); - $mocks->{zoom_obj}->mock( 'size', sub { @zebrarecords } ); - $mocks->{zoom_record_obj}->mock( 'raw', sub {} ); - return $mocks; } -- 2.39.5