Bug 9988: Merge should have a parameter hash

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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
index 2f14c3a..9baaa26 100644 (file)
--- 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 (file)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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 (executable)
--- 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',