Attachment 61790 Details for Bug 9988 – Bug 9988: Few subtle changes for postponed merge

[patch] Bug 9988: Few subtle changes for postponed merge

Bug-9988-Few-subtle-changes-for-postponed-merge.patch (text/plain), 8.07 KB, created by Marcel de Rooy on 2017-04-03 08:31:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2017-04-03 08:31:52 UTC

Size: 8.07 KB

patch

obsolete

>From 28fff3764134166828995eb0d5a3c7768147ee0a Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 23 Feb 2017 12:46:52 +0100
>Subject: [PATCH] Bug 9988: Few subtle changes for postponed merge
>Content-Type: text/plain; charset=utf-8
>
>The fails in the previous test showed that we need the first three
>changes here. Some final polishing in points 4 to 6.
>
>[1] Sub merge: Refine the condition for initializing $tags_new.
>    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
>    defined when running merge later. When crossing the type boundary, we
>    need a new field too.
>[2] Sub merge: Add condition for an empty @record_to array.
>    This indicates also that a field should be removed, since we should
>    otherwise only add a $9 subfield.
>[3] Sub merge: Adjust initializing @record_from.
>    This change is tested by verifying a cleared subfield in the test.
>[4] DelAuthority: Adding a skipmerge parameter to allow the call from
>    authorities/merge.pl to skip an unneeded merge.
>    This also prevents that the 'delete-merge' would precede the
>    'modify-merge' under a hypothetical race condition.
>[5] DelAuthority: There is actually no need to call GetAuthority.
>    The subfields of the old record are not relevant in this case.
>[6] Added a few POD lines to merge.
>[7] Removed a trailing space in a comment line in merge.
>
>Test plan:
>[1] Run t/db_dependent/Authorities/Merge.t.
>    The last subtest should no longer fail now.
>[2] See test plan of next patch.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> C4/AuthoritiesMarc.pm              | 42 ++++++++++++++++++++++++++------------
> authorities/merge.pl               |  5 +++--
> t/db_dependent/Authorities/Merge.t |  2 +-
> 3 files changed, 33 insertions(+), 16 deletions(-)
>
>diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
>index 8c6c273..87620b4 100644
>--- a/C4/AuthoritiesMarc.pm
>+++ b/C4/AuthoritiesMarc.pm
>@@ -1,4 +1,5 @@
> package C4::AuthoritiesMarc;
>+
> # Copyright 2000-2002 Katipo Communications
> #
> # This file is part of Koha.
>@@ -683,17 +684,20 @@ sub AddAuthority {
> 
> =head2 DelAuthority
> 
>-    DelAuthority({ authid => $authid });
>+    DelAuthority({ authid => $authid, [ skip_merge => 1 ] });
> 
>-Deletes $authid and calls merge to cleanup in linked biblio records
>+Deletes $authid and calls merge to cleanup linked biblio records.
>+Parameter skip_merge is used in authorities/merge.pl. You should normally not
>+use it.
> 
> =cut
> 
> sub DelAuthority {
>     my ( $params ) = @_;
>     my $authid = $params->{authid} || return;
>-    my $dbh=C4::Context->dbh;
>-    merge({ mergefrom => $authid, MARCfrom => GetAuthority($authid) });
>+    my $skip_merge = $params->{skip_merge};
>+    my $dbh = C4::Context->dbh;
>+    merge({ mergefrom => $authid }) if !$skip_merge;
>     $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);
>@@ -1370,19 +1374,24 @@ sub AddAuthorityTrees{
> =head2 merge
> 
>     $count = merge({
>-        mergefrom => mergefrom,
>-        MARCfrom => $MARCfrom,
>+        mergefrom => $mergefrom,
>+        [ MARCfrom => $MARCfrom, ]
>         [ mergeto => $mergeto, ]
>         [ MARCto => $MARCto, ]
>         [ biblionumbers => [ $a, $b, $c ], ]
>         [ override_limit => 1, ]
>     });
> 
>-Merge biblios linked to authority $mergefrom.
>+Merge biblios linked to authority $mergefrom (mandatory parameter).
> If $mergeto equals mergefrom, the linked biblio field is updated.
> If $mergeto is different, the biblio field will be linked to $mergeto.
> If $mergeto is missing, the biblio field is deleted.
> 
>+MARCfrom is used to determine if a cleared subfield in the authority record
>+should be removed from a biblio. MARCto is used to populate the biblio
>+record with the updated values; if you do not pass it, the biblio field
>+will be deleted (same as missing mergeto).
>+
> Normally all biblio records linked to $mergefrom, will be considered. But
> you can pass specific numbers via the biblionumbers parameter.
> 
>@@ -1435,16 +1444,22 @@ sub merge {
>     my @record_to;
>     @record_to = $MARCto->field($auth_tag_to_report_to)->subfields() if $auth_tag_to_report_to && $MARCto && $MARCto->field($auth_tag_to_report_to);
>     my @record_from;
>-    @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields() if $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from);
>+    if( !$authfrom && $MARCfrom && $MARCfrom->field('1..','2..') ) {
>+    # postponed merge, authfrom was deleted and MARCfrom only contains the old reporting tag (and possibly a 100 for UNIMARC)
>+    # 2XX is for UNIMARC; we use -1 in order to skip 100 in UNIMARC; this will not impact MARC21, since there is only one tag
>+        @record_from = ( $MARCfrom->field('1..','2..') )[-1]->subfields;
>+    } elsif( $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from) ) {
>+        @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields;
>+    }
> 
>-    # Get All candidate Tags for the change 
>+    # 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;
>-    if( $authtypefrom && $authtypeto && $authtypeto->authtypecode ne $authtypefrom->authtypecode ) {
>+    if( $authtypeto && ( !$authtypefrom || $authtypeto->authtypecode ne $authtypefrom->authtypecode )) {
>         $tags_new = $dbh->selectcol_arrayref( $sql, undef, ( $authtypeto->authtypecode ));
>     }  
> 
>@@ -1470,9 +1485,10 @@ sub merge {
>                 my $tag         = $field->tag();
>                 next if !defined($auth_number) || $auth_number ne $mergefrom;
>                 $countfrom++;
>-                if ( !$mergeto || ( $overwrite && $countfrom > 1 ) ) {
>-                    # if mergeto is missing, this indicates a delete
>-                    # Or: remove this duplicate in strict mode
>+                if ( !$mergeto || !@record_to ||
>+                  ( $overwrite && $countfrom > 1 ) ) {
>+                    # !mergeto or !record_to indicates a delete
>+                    # Other condition: remove this duplicate in strict mode
>                     $marcrecord->delete_field($field);
>                     $update = 1;
>                     next;
>diff --git a/authorities/merge.pl b/authorities/merge.pl
>index 4279fef..0230dbf7 100755
>--- a/authorities/merge.pl
>+++ b/authorities/merge.pl
>@@ -65,8 +65,9 @@ if ($merge) {
>     my $MARCfrom = GetAuthority( $recordid2 );
>     merge({ mergefrom => $recordid2, MARCfrom => $MARCfrom, mergeto => $recordid1, MARCto => $record });
> 
>-    # Deleting the other record
>-    DelAuthority({ authid => $recordid2 });
>+    # Delete the other record. Do not merge. It is unneeded and could under
>+    # special circumstances have unwanted side-effects.
>+    DelAuthority({ authid => $recordid2, skip_merge => 1 });
> 
>     # Parameters
>     $template->param(
>diff --git a/t/db_dependent/Authorities/Merge.t b/t/db_dependent/Authorities/Merge.t
>index 0ae6124..b2cd97e 100755
>--- a/t/db_dependent/Authorities/Merge.t
>+++ b/t/db_dependent/Authorities/Merge.t
>@@ -323,7 +323,7 @@ subtest "Test some specific postponed merge issues" => sub {
>     # The modify merge would be useless after that.
>     @linkedrecords = ( $biblionumber );
>     my $restored_mocks = set_mocks();
>-    DelAuthority({ authid => $id }); # delete A
>+    DelAuthority({ authid => $id, skip_merge => 1 }); # delete A
>     $restored_mocks->{auth_mod}->unmock_all;
>     $biblio = C4::Biblio::GetMarcBiblio( $biblionumber );
>     is( $biblio->subfield('109', '9'), $id, 'If the 109 is no longer present, another modify merge would not bring it back' );
>-- 
>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