Attachment 60668 Details for Bug 9988 – Bug 9988: Merge should have a parameter hash

[patch] Bug 9988: Merge should have a parameter hash

Bug-9988-Merge-should-have-a-parameter-hash.patch (text/plain), 12.14 KB, created by Marcel de Rooy on 2017-02-25 15:21:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2017-02-25 15:21:46 UTC

Size: 12.14 KB

patch

obsolete

>From b409d498db25fb368eec07e3d92a937313ee29d3 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Tue, 21 Feb 2017 08:39:24 +0100
>Subject: [PATCH] Bug 9988: Merge should have a parameter hash
>Content-Type: text/plain; charset=utf-8
>
>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>
>---
> 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 6e0a8fe..b980769 100755
>--- a/authorities/authorities-home.pl
>+++ b/authorities/authorities-home.pl
>@@ -58,7 +58,7 @@ if ( $op eq "delete" ) {
>             debug           => 1,
>         }
>     );
>-    &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 2b82796..71ea27b 100755
>--- a/tools/batch_delete_records.pl
>+++ b/tools/batch_delete_records.pl
>@@ -198,7 +198,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

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