From 742b4668488e8d2e3ef65ad0ebf13e9319bb586d Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Tue, 21 Feb 2017 08:39:24 +0100 Subject: [PATCH] Bug 9988: Merge should have a parameter hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will need a few additional parameters for merge later on. This patch puts the original parameters in a parameter hash. For the same reason DelAuthority gets a parameter hash here. Note: We remove the second parameter from the DelAuthority call in authorities/authorities-home.pl here. It was not used and could have presented problems in the future. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t. [2] Run t/db_dependent/Authorities/Merge.t. Signed-off-by: Marcel de Rooy Signed-off-by: Marc VĂ©ron --- C4/AuthoritiesMarc.pm | 23 +++++++++++++++++------ C4/ImportBatch.pm | 2 +- authorities/authorities-home.pl | 2 +- authorities/authorities.pl | 2 +- authorities/merge.pl | 4 ++-- misc/migration_tools/merge_authority.pl | 8 ++++---- misc/migration_tools/remove_unused_authorities.pl | 2 +- t/db_dependent/Authorities/Merge.t | 12 ++++++------ t/db_dependent/AuthoritiesMarc.t | 2 +- tools/batch_delete_records.pl | 2 +- 10 files changed, 35 insertions(+), 24 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 2f14c3a..9baaa26 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -682,18 +682,19 @@ sub AddAuthority { =head2 DelAuthority - DelAuthority( $authid ) + DelAuthority({ authid => $authid }); Deletes $authid and calls merge to cleanup in linked biblio records =cut sub DelAuthority { - my ($authid) = @_; + my ( $params ) = @_; + my $authid = $params->{authid} || return; my $dbh=C4::Context->dbh; unless( C4::Context->preference('dontmerge') eq '1' ) { - &merge( $authid, GetAuthority($authid) ); + &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 (?,?)"; @@ -725,7 +726,7 @@ sub ModAuthority { # In that case set system preference "dontmerge" to 1. Otherwise biblios will # be updated. unless(C4::Context->preference('dontmerge') eq '1'){ - &merge($authid,$oldrecord,$authid,$record); + &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) ". @@ -1388,7 +1389,12 @@ sub AddAuthorityTrees{ =head2 merge - $count = merge ( mergefrom, $MARCfrom, [$mergeto, $MARCto] ) + $count = merge({ + mergefrom => mergefrom, + MARCfrom => $MARCfrom, + [ mergeto => $mergeto, ] + [ MARCto => $MARCto, ] + }); Merge biblios linked to authority $mergefrom. If $mergeto equals mergefrom, the linked biblio field is updated. @@ -1401,7 +1407,12 @@ authority type, merge also supports moving to another authority type. =cut sub merge { - my ($mergefrom,$MARCfrom,$mergeto,$MARCto) = @_; + my ( $params ) = @_; + my $mergefrom = $params->{mergefrom}; + my $MARCfrom = $params->{MARCfrom}; + my $mergeto = $params->{mergeto}; + my $MARCto = $params->{MARCto}; + return 0 unless $mergefrom > 0; # prevent abuse my ($counteditedbiblio,$countunmodifiedbiblio,$counterrors)=(0,0,0); my $dbh=C4::Context->dbh; diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index 14364f4..a4f4c55 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -852,7 +852,7 @@ sub BatchRevertRecords { $num_items_deleted += BatchRevertItems($rowref->{'import_record_id'}, $rowref->{'matched_biblionumber'}); $error = DelBiblio($rowref->{'matched_biblionumber'}); } else { - DelAuthority( $rowref->{'matched_authid'} ); + DelAuthority({ authid => $rowref->{'matched_authid'} }); } if (defined $error) { $num_errors++; diff --git a/authorities/authorities-home.pl b/authorities/authorities-home.pl index 9b3a55e..d9c5ac8 100755 --- a/authorities/authorities-home.pl +++ b/authorities/authorities-home.pl @@ -65,7 +65,7 @@ if ( $op eq "delete" ) { token => scalar $query->param('csrf_token'), }); - &DelAuthority( $authid, 1 ); + DelAuthority({ authid => $authid }); if ( $query->param('operator') ) { # query contains search params so perform search diff --git a/authorities/authorities.pl b/authorities/authorities.pl index 1e49d96..21103e0 100755 --- a/authorities/authorities.pl +++ b/authorities/authorities.pl @@ -641,7 +641,7 @@ if ($op eq "add") { } } elsif ($op eq "delete") { #------------------------------------------------------------------------------------------------------------------------------ - &DelAuthority($authid); + DelAuthority({ authid => $authid }); if ($nonav){ print $input->redirect("auth_finder.pl"); }else{ diff --git a/authorities/merge.pl b/authorities/merge.pl index 9d00a1d..94bfc3d 100755 --- a/authorities/merge.pl +++ b/authorities/merge.pl @@ -64,10 +64,10 @@ if ($merge) { # Now merge for biblios attached to $recordid2 # We ignore dontmerge now, since recordid2 is deleted my $MARCfrom = GetAuthority( $recordid2 ); - merge( $recordid2, $MARCfrom, $recordid1, $record ); + merge({ mergefrom => $recordid2, MARCfrom => $MARCfrom, mergeto => $recordid1, MARCto => $record }); # Deleting the other record - DelAuthority( $recordid2 ); + DelAuthority({ authid => $recordid2 }); # Parameters $template->param( diff --git a/misc/migration_tools/merge_authority.pl b/misc/migration_tools/merge_authority.pl index 061a3bd..3e6722f 100755 --- a/misc/migration_tools/merge_authority.pl +++ b/misc/migration_tools/merge_authority.pl @@ -86,18 +86,18 @@ if ($batch) { print "managing $authid\n" if $verbose; my $MARCauth = GetAuthority( $authid ); if( $MARCauth ) { - merge( $authid, $MARCauth, $authid, $MARCauth ); + merge({ mergefrom => $authid, MARCfrom => $MARCauth, mergeto => $authid, MARCto => $MARCauth }); } else { - merge( $authid, undef ); # handle a delete + merge({ mergefrom => $authid }); # handle a delete } } $dbh->do("update need_merge_authorities set done=1 where done=2"); #DONE } else { my $MARCfrom = GetAuthority($mergefrom); my $MARCto = GetAuthority($mergeto); - &merge($mergefrom,$MARCfrom,$mergeto,$MARCto); + &merge({ mergefrom => $mergefrom, MARCfrom => $MARCfrom, mergeto => $mergeto, MARCto => $MARCto }); #Could add mergefrom authority to mergeto rejected forms before deletion - DelAuthority($mergefrom) if ($mergefrom != $mergeto); + DelAuthority({ authid => $mergefrom }) if ($mergefrom != $mergeto); } my $timeneeded = gettimeofday - $starttime; print "Done in $timeneeded seconds" unless $noconfirm; diff --git a/misc/migration_tools/remove_unused_authorities.pl b/misc/migration_tools/remove_unused_authorities.pl index 49bd1df..f4e0ecb 100755 --- a/misc/migration_tools/remove_unused_authorities.pl +++ b/misc/migration_tools/remove_unused_authorities.pl @@ -82,7 +82,7 @@ while (my $data=$rqselect->fetchrow_hashref){ print "$counter\n" unless $counter++ % 100; # if found, delete, otherwise, just count if ($used>=$thresholdmin and $used<=$thresholdmax){ - DelAuthority($data->{'authid'}) unless $test; + DelAuthority({ authid => $data->{'authid'} }) unless $test; $totdeleted++; } else { $totundeleted++; diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t index 7ee8cdb..be3e420 100755 --- a/t/db_dependent/Authorities/Merge.t +++ b/t/db_dependent/Authorities/Merge.t @@ -60,7 +60,7 @@ subtest 'Test merge A1 to A2 (within same authtype)' => sub { # Time to merge @zebrarecords = ( $biblio1, $biblio2 ); $index = 0; - my $rv = C4::AuthoritiesMarc::merge( $authid2, $auth2, $authid1, $auth1 ); + 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' ); # Check the results @@ -105,7 +105,7 @@ subtest 'Test merge A1 to modified A1, test strict mode' => sub { @zebrarecords = ( $MARC1, $MARC2 ); $index = 0; t::lib::Mocks::mock_preference('AuthorityMergeMode', 'loose'); - my $rv = C4::AuthoritiesMarc::merge( $authid1, $auth1old, $authid1, $auth1new ); + my $rv = C4::AuthoritiesMarc::merge({ mergefrom => $authid1, MARCfrom => $auth1old, mergeto => $authid1, MARCto => $auth1new }); is( $rv, 2, 'Both records are updated now' ); #Check the results @@ -125,7 +125,7 @@ subtest 'Test merge A1 to modified A1, test strict mode' => sub { ModBiblio( $MARC1, $biblionumber1, '' ); @zebrarecords = ( $MARC1 ); $index = 0; - $rv = C4::AuthoritiesMarc::merge( $authid1, $auth1old, $authid1, $auth1new ); + $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' ); compare_fields( $MARC1, $biblio1, { 609 => 1 }, 'count' ); @@ -172,7 +172,7 @@ subtest 'Test merge A1 to B1 (changing authtype)' => sub { # Time to merge @zebrarecords = ( $marc ); $index = 0; - my $retval = C4::AuthoritiesMarc::merge( $authid1, $auth1, $authid2, $auth2 ); + my $retval = C4::AuthoritiesMarc::merge({ mergefrom => $authid1, MARCfrom => $auth1, mergeto => $authid2, MARCto => $auth2 }); is( $retval, 1, 'We touched only one biblio' ); # Get new marc record for compares @@ -234,7 +234,7 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub { my ( $biblionumber ) = C4::Biblio::AddBiblio( $bib1, '' ); @zebrarecords = ( $bib1 ); $index = 0; - DelAuthority( $authid1 ); # this triggers a merge call + DelAuthority({ authid => $authid1 }); # this triggers a merge call # See what happened in the biblio record my $marc1 = C4::Biblio::GetMarcBiblio( $biblionumber ); @@ -256,7 +256,7 @@ subtest 'Merging authorities should handle deletes (BZ 18070)' => sub { C4::Context->dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid1 ); @zebrarecords = ( $marc1 ); $index = 0; - merge( $authid1, undef ); + merge({ mergefrom => $authid1 }); # Final check $marc1 = C4::Biblio::GetMarcBiblio( $biblionumber ); is( $marc1->field('609'), undef, 'Merge removed the 609 again even after deleting the authority record' ); diff --git a/t/db_dependent/AuthoritiesMarc.t b/t/db_dependent/AuthoritiesMarc.t index 57fcc88..c9b44b4 100755 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@ -197,7 +197,7 @@ subtest 'AddAuthority should respect AUTO_INCREMENT (BZ 18104)' => sub { t::lib::Mocks::mock_preference( 'marcflavour', 'MARC21' ); my $record = C4::AuthoritiesMarc::GetAuthority(1); my $id1 = AddAuthority( $record, undef, 'GEOGR_NAME' ); - DelAuthority( $id1 ); + DelAuthority({ authid => $id1 }); my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' ); isnt( $id1, $id2, 'Do not return the same id again' ); t::lib::Mocks::mock_preference( 'marcflavour', 'UNIMARC' ); diff --git a/tools/batch_delete_records.pl b/tools/batch_delete_records.pl index 46b9c88..4be2427 100755 --- a/tools/batch_delete_records.pl +++ b/tools/batch_delete_records.pl @@ -204,7 +204,7 @@ if ( $op eq 'form' ) { } else { # Authorities my $authid = $record_id; - eval { C4::AuthoritiesMarc::DelAuthority( $authid ) }; + eval { C4::AuthoritiesMarc::DelAuthority({ authid => $authid }) }; if ( $@ ) { push @messages, { type => 'error', -- 2.1.4