Attachment 61920 Details for Bug 9988 – Bug 9988: Check the merge limit in sub merge

[patch] Bug 9988: Check the merge limit in sub merge

Bug-9988-Check-the-merge-limit-in-sub-merge.patch (text/plain), 14.55 KB, created by Jacek Ablewicz on 2017-04-05 20:10:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jacek Ablewicz

Created: 2017-04-05 20:10:11 UTC

Size: 14.55 KB

patch

obsolete

>From 88730d7d6c6881b375c08a851ce0c1c6c0908903 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Tue, 21 Feb 2017 11:54:23 +0100
>Subject: [PATCH] Bug 9988: Check the merge limit in sub merge
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>At this point, we are replacing dontmerge functionality by the new
>AuthorityMergeLimit logic. Instead of doing this check before calling
>merge, we just call merge and check it there. In order to let the cron
>job do the larger (postponed) merges, we add a parameter override_limit.
>
>A subtest is added in Merge.t to test the 'postponed merge' feature. Since
>merge now also calls get_usage_count, an additional mock is added. All
>references to dontmerge are removed.
>
>In merge two lines, initializing $dbh and $counteditbiblios, are moved.
>The dontmerge test in DelAuthority and ModAuthority is removed. Since this
>did not leave much in ModAuthority, I fixed the whitespace on the remaining
>lines rightaway (yes, I know).
>A minimal set of changes is applied to the cron script; it will get further
>attention on a next patch.
>
>Test plan:
>[1] Run t/db_dependent/Authorities/Merge.t
>[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
>    biblios. Check that the merge was done immediately.
>[3] Now modify an authority with more than 2 linked records.
>    Verify that the merge was postponed; a record must be inserted in
>    the need_merge_authorities table.
>[4] Testing of the merge cron job is *postponed* to a next patch.
>    Note: I tested a modification, but the script just needs more
>    attention.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Marc VÃ©ron <veron@veron.ch>
>Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
>---
> C4/AuthoritiesMarc.pm                   |   75 ++++++++++++++++---------------
> misc/migration_tools/merge_authority.pl |   19 ++++----
> t/db_dependent/Authorities/Merge.t      |   45 ++++++++++++++++---
> 3 files changed, 86 insertions(+), 53 deletions(-)
>
>diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
>index 4aa8845..8c6c273 100644
>--- a/C4/AuthoritiesMarc.pm
>+++ b/C4/AuthoritiesMarc.pm
>@@ -28,6 +28,7 @@ use C4::Charset;
> use C4::Log;
> use Koha::MetadataRecord::Authority;
> use Koha::Authorities;
>+use Koha::Authority::MergeRequest;
> use Koha::Authority::Types;
> use Koha::Authority;
> use Koha::SearchEngine;
>@@ -692,14 +693,7 @@ sub DelAuthority {
>     my ( $params ) = @_;
>     my $authid = $params->{authid} || return;
>     my $dbh=C4::Context->dbh;
>-
>-    unless( C4::Context->preference('dontmerge') eq '1' ) {
>-        &merge({ mergefrom => $authid, MARCfrom => GetAuthority($authid) });
>-    } else {
>-        # save a record in need_merge_authorities table
>-        my $sqlinsert="INSERT INTO need_merge_authorities (authid, done) VALUES (?,?)";
>-        $dbh->do( $sqlinsert, undef, $authid, 0 );
>-    }
>+    merge({ mergefrom => $authid, MARCfrom => GetAuthority($authid) });
>     $dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid );
>     logaction( "AUTHORITIES", "DELETE", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog");
>     ModZebra( $authid, "recordDelete", "authorityserver", undef);
>@@ -714,27 +708,13 @@ Modifies authority record, optionally updates attached biblios.
> =cut
> 
> sub ModAuthority {
>-  my ($authid,$record,$authtypecode)=@_; # deprecated $merge parameter removed
>-
>-  my $dbh=C4::Context->dbh;
>-  #Now rewrite the $record to table with an add
>-  my $oldrecord=GetAuthority($authid);
>-  $authid=AddAuthority($record,$authid,$authtypecode);
>-
>-  # If a library thinks that updating all biblios is a long process and wishes
>-  # to leave that to a cron job, use misc/migration_tools/merge_authority.pl.
>-  # In that case set system preference "dontmerge" to 1. Otherwise biblios will
>-  # be updated.
>-  unless(C4::Context->preference('dontmerge') eq '1'){
>-      &merge({ mergefrom => $authid, MARCfrom => $oldrecord, mergeto => $authid, MARCto => $record });
>-  } else {
>-      # save a record in need_merge_authorities table
>-      my $sqlinsert="INSERT INTO need_merge_authorities (authid, done) ".
>-	"VALUES (?,?)";
>-      $dbh->do($sqlinsert,undef,($authid,0));
>-  }
>-  logaction( "AUTHORITIES", "MODIFY", $authid, "authority BEFORE=>" . $oldrecord->as_formatted ) if C4::Context->preference("AuthoritiesLog");
>-  return $authid;
>+    my ( $authid, $record, $authtypecode ) = @_;
>+    my $oldrecord = GetAuthority($authid);
>+    #Now rewrite the $record to table with an add
>+    $authid = AddAuthority($record, $authid, $authtypecode);
>+    merge({ mergefrom => $authid, MARCfrom => $oldrecord, mergeto => $authid, MARCto => $record });
>+    logaction( "AUTHORITIES", "MODIFY", $authid, "authority BEFORE=>" . $oldrecord->as_formatted ) if C4::Context->preference("AuthoritiesLog");
>+    return $authid;
> }
> 
> =head2 GetAuthorityXML 
>@@ -1395,6 +1375,7 @@ sub AddAuthorityTrees{
>         [ mergeto => $mergeto, ]
>         [ MARCto => $MARCto, ]
>         [ biblionumbers => [ $a, $b, $c ], ]
>+        [ override_limit => 1, ]
>     });
> 
> Merge biblios linked to authority $mergefrom.
>@@ -1405,6 +1386,9 @@ 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.
> 
>+The parameter override_limit is used by the cron job to force larger
>+postponed merges.
>+
> Note: Although $mergefrom and $mergeto will normally be of the same
> authority type, merge also supports moving to another authority type.
> 
>@@ -1416,20 +1400,35 @@ sub merge {
>     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 });
>+    my $override_limit = $params->{override_limit};
>+
>+    # If we do not have biblionumbers, we get all linked biblios if the
>+    # number of linked records does not exceed the limit UNLESS we override.
>+    my @biblionumbers;
>+    if( $params->{biblionumbers} ) {
>+        @biblionumbers = @{ $params->{biblionumbers} };
>+    } elsif( $override_limit ) {
>+        @biblionumbers = Koha::Authorities->linked_biblionumbers({ authid => $mergefrom });
>+    } else { # now first check number of linked records
>+        my $max = C4::Context->preference('AuthorityMergeLimit') // 0;
>+        my $hits = Koha::Authorities->get_usage_count({ authid => $mergefrom });
>+        if( $hits > 0 && $hits <= $max ) {
>+            @biblionumbers = Koha::Authorities->linked_biblionumbers({ authid => $mergefrom });
>+        } elsif( $hits > $max ) { #postpone this merge to the cron job
>+            Koha::Authority::MergeRequest->new({
>+                authid => $mergefrom,
>+                oldrecord => $MARCfrom,
>+                authid_new => $mergeto,
>+            })->store;
>+        }
>+    }
>     return 0 if !@biblionumbers;
> 
>-    my $counteditedbiblio = 0;
>-    my $dbh = C4::Context->dbh;
>+    # Search authtypes and reporting tags
>     my $authfrom = Koha::Authorities->find($mergefrom);
>     my $authto = Koha::Authorities->find($mergeto);
>     my $authtypefrom = $authfrom ? Koha::Authority::Types->find($authfrom->authtypecode) : undef;
>     my $authtypeto   = $authto ? Koha::Authority::Types->find($authto->authtypecode) : undef;
>-
>-    # search the tag to report
>     my $auth_tag_to_report_from = $authtypefrom ? $authtypefrom->auth_tag_to_report : '';
>     my $auth_tag_to_report_to   = $authtypeto ? $authtypeto->auth_tag_to_report : '';
> 
>@@ -1441,6 +1440,7 @@ sub merge {
>     # 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
>+    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_new;
>@@ -1458,6 +1458,7 @@ sub merge {
>     # And we need to add $9 in order not to duplicate
>     $skip_subfields->{9} = 1 if !$overwrite;
> 
>+    my $counteditedbiblio = 0;
>     foreach my $biblionumber ( @biblionumbers ) {
>         my $marcrecord = GetMarcBiblio( $biblionumber );
>         next if !$marcrecord;
>diff --git a/misc/migration_tools/merge_authority.pl b/misc/migration_tools/merge_authority.pl
>index 3e6722f..9b755b5 100755
>--- a/misc/migration_tools/merge_authority.pl
>+++ b/misc/migration_tools/merge_authority.pl
>@@ -15,6 +15,8 @@ use C4::Context;
> use C4::Search;
> use C4::Biblio;
> use C4::AuthoritiesMarc;
>+use Koha::Authorities;
>+use Koha::Authority::MergeRequests;
> use Time::HiRes qw(gettimeofday);
> 
> use Getopt::Long;
>@@ -80,16 +82,15 @@ print "Merging\n" unless $noconfirm;
> if ($batch) {
>   my $authref;
>   $dbh->do("update need_merge_authorities set done=2 where done=0"); #temporary status 2 means: selected for merge
>-  $authref=$dbh->selectall_arrayref("select distinct authid from need_merge_authorities where done=2");
>-  foreach(@$authref) {
>-      my $authid=$_->[0];
>+  $authref=$dbh->selectall_arrayref("select id,authid,authid_new from need_merge_authorities where done=2");
>+  foreach my $row ( @$authref ) {
>+      my $req = Koha::Authority::MergeRequests->find( $row->[0] );
>+      my $marc = $req ? $req->oldmarc : undef;
>+      my $authid = $row->[1];
>+      my $authid_new = $row->[2];
>       print "managing $authid\n" if $verbose;
>-      my $MARCauth = GetAuthority( $authid );
>-      if( $MARCauth ) {
>-          merge({ mergefrom => $authid, MARCfrom => $MARCauth, mergeto => $authid, MARCto => $MARCauth });
>-      } else {
>-          merge({ mergefrom => $authid }); # handle a delete
>-      }
>+      # Following merge call handles both modifications and deletes
>+      merge({ mergefrom => $authid, MARCfrom => $marc, mergeto => $authid_new, MARCto => GetAuthority($authid_new), override_limit => 1 });
>   }
>   $dbh->do("update need_merge_authorities set done=1 where done=2"); #DONE
> } else {
>diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t
>index 5e8bcf9..d4c9cfe 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 => 5;
>+use Test::More tests => 6;
> 
> use Getopt::Long;
> use MARC::Record;
>@@ -15,6 +15,7 @@ use t::lib::TestBuilder;
> 
> use C4::Biblio;
> use Koha::Authorities;
>+use Koha::Authority::MergeRequests;
> use Koha::Database;
> 
> BEGIN {
>@@ -34,6 +35,36 @@ our @linkedrecords;
> my $mocks = set_mocks();
> our ( $authtype1, $authtype2 ) = modify_framework();
> 
>+subtest 'Test postponed merge feature' => sub {
>+    plan tests => 6;
>+
>+    # Set limit to zero, and call merge a few times
>+    t::lib::Mocks::mock_preference('AuthorityMergeLimit', 0);
>+    my $auth1 = t::lib::TestBuilder->new->build({ source => 'AuthHeader' });
>+    my $cnt = Koha::Authority::MergeRequests->count;
>+    merge({ mergefrom => '0' });
>+    is( Koha::Authority::MergeRequests->count, $cnt, 'No merge request added as expected' );
>+    merge({ mergefrom => $auth1->{authid} });
>+    is( Koha::Authority::MergeRequests->count, $cnt, 'No merge request added since we have zero hits' );
>+    @linkedrecords = ( 1, 2 ); # these biblionumbers do not matter
>+    merge({ mergefrom => $auth1->{authid} });
>+    is( Koha::Authority::MergeRequests->count, $cnt + 1, 'Merge request added as expected' );
>+
>+    # Set limit to two (starting with two records)
>+    t::lib::Mocks::mock_preference('AuthorityMergeLimit', 2);
>+    merge({ mergefrom => $auth1->{authid} });
>+    is( Koha::Authority::MergeRequests->count, $cnt + 1, 'No merge request added as we do not exceed the limit' );
>+    @linkedrecords = ( 1, 2, 3 ); # these biblionumbers do not matter
>+    merge({ mergefrom => $auth1->{authid} });
>+    is( Koha::Authority::MergeRequests->count, $cnt + 2, 'Merge request added as we do exceed the limit again' );
>+    # Now override
>+    merge({ mergefrom => $auth1->{authid}, override_limit => 1 });
>+    is( Koha::Authority::MergeRequests->count, $cnt + 2, 'No merge request added as we did override' );
>+
>+    # Set merge limit high enough for the other subtests
>+    t::lib::Mocks::mock_preference('AuthorityMergeLimit', 1000);
>+};
>+
> subtest 'Test merge A1 to A2 (within same authtype)' => sub {
> # Tests originate from bug 11700
>     plan tests => 9;
>@@ -215,9 +246,6 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub {
> subtest 'Merging authorities should handle deletes (BZ 18070)' => sub {
>     plan tests => 2;
> 
>-    # For this test we need dontmerge OFF
>-    t::lib::Mocks::mock_preference('dontmerge', '0');
>-
>     # Add authority and linked biblio, delete authority
>     my $auth1 = MARC::Record->new;
>     $auth1->append_fields( MARC::Field->new( '109', '', '', 'a' => 'DEL'));
>@@ -235,7 +263,7 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub {
>     my $marc1 = C4::Biblio::GetMarcBiblio( $biblionumber );
>     is( $marc1->field('609'), undef, 'Field 609 should be gone too' );
> 
>-    # Now we simulate the delete as done from the cron job (with dontmerge)
>+    # Now we simulate the delete as done in the cron job
>     # First, restore auth1 and add 609 back in bib1
>     $auth1 = MARC::Record->new;
>     $auth1->append_fields( MARC::Field->new( '109', '', '', 'a' => 'DEL'));
>@@ -247,7 +275,7 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub {
>     # Instead of going through DelAuthority, we manually delete the auth
>     # 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).
>+    # merge cron job.
>     # We use the biblionumbers parameter here and unmock linked_biblionumbers.
>     C4::Context->dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid1 );
>     @linkedrecords = ();
>@@ -260,10 +288,13 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub {
> 
> sub set_mocks {
>     # After we removed the Zebra code from merge, we only need to mock
>-    # linked_biblionumbers here.
>+    # get_usage_count and linked_biblionumbers here.
> 
>     my $mocks;
>     $mocks->{auth_mod} = Test::MockModule->new( 'Koha::Authorities' );
>+    $mocks->{auth_mod}->mock( 'get_usage_count', sub {
>+         return scalar @linkedrecords;
>+    });
>     $mocks->{auth_mod}->mock( 'linked_biblionumbers', sub {
>          return @linkedrecords;
>     });
>-- 
>1.7.10.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