Attachment 61898 Details for Bug 9988 – Bug 9988: Remove the Zebra code from sub merge

[patch] Bug 9988: Remove the Zebra code from sub merge

Bug-9988-Remove-the-Zebra-code-from-sub-merge.patch (text/plain), 11.94 KB, created by Marc Véron on 2017-04-05 14:14:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marc Véron

Created: 2017-04-05 14:14:23 UTC

Size: 11.94 KB

patch

obsolete

>From 6b0d1d859cee6a460695f48fffb43c6c9134907a Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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=UTF-8
>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 <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Marc VÃ©ron <veron@veron.ch>
>---
> 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 9baaa26..4aa8845 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 be3e420..5e8bcf9 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.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 9988: 59158 | 59159 | 60256 | 60257 | 60258 | 60259 | 60594 | 60595 | 60596 | 60597 | 60598 | 60599 | 60600 | 60601 | 60602 | 60603 | 60604 | 60605 | 60606 | 60607 | 60668 | 60669 | 60670 | 60671 | 60672 | 60673 | 60674 | 60675 | 61306 | 61307 | 61308 | 61309 | 61779 | 61780 | 61781 | 61782 | 61783 | 61784 | 61785 | 61786 | 61787 | 61788 | 61789 | 61790 | 61791 | 61792 | 61890 | 61891 | 61892 | 61893 | 61894 | 61895 | 61897 | 61898 | 61899 | 61900 | 61901 | 61902 | 61903 | 61904 | 61912 | 61913 | 61914 | 61915 | 61916 | 61917 | 61918 | 61919 | 61920 | 61921 | 61922 | 61923 | 61924 | 61925 | 62055 | 62123 | 62124 | 62125 | 62126 | 62127 | 62128 | 62129 | 62130 | 62131 | 62132 | 62133 | 62134 | 62135 | 62136 | 62137