From 04263912c7e413c5dbf13fe61dcdfa4b4912ab56 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy 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=utf8 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 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 | 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 4aa8845a25..8c6c273906 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 3e6722fe8c..9b755b5632 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 5e8bcf970f..d4c9cfe642 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; }); -- 2.39.5