From 310b9b00b9966c334234cb333cb8e091f06766d9 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Tue, 3 Apr 2018 03:51:51 +0000
Subject: [PATCH] Bug 9302: (QA follow-up) Merge should be a transaction

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 Koha/Patrons.pm                               | 28 +++++++-------
 .../prog/en/modules/members/merge-patrons.tt  | 38 ++++++++++---------
 members/merge-patrons.pl                      |  9 ++++-
 3 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm
index 1afb440e08..0ef5f4f0b9 100644
--- a/Koha/Patrons.pm
+++ b/Koha/Patrons.pm
@@ -260,23 +260,25 @@ sub merge {
 
     my $results;
 
-    foreach my $borrowernumber (@borrowernumbers) {
-        my $patron = Koha::Patrons->find( $borrowernumber );
+    $self->_resultset->result_source->schema->txn_do( sub {
+        foreach my $borrowernumber (@borrowernumbers) {
+            my $patron = Koha::Patrons->find( $borrowernumber );
 
-        next unless $patron;
+            next unless $patron;
 
-        # Unbless for safety, the patron will end up being deleted
-        $results->{merged}->{$borrowernumber}->{patron} = $patron->unblessed;
+            # Unbless for safety, the patron will end up being deleted
+            $results->{merged}->{$borrowernumber}->{patron} = $patron->unblessed;
 
-        while (my ($r, $field) = each(%$RESULTSET_PATRON_ID_MAPPING)) {
-            my $rs = $schema->resultset($r)->search({ $field => $borrowernumber} );
-            $results->{merged}->{ $borrowernumber }->{updated}->{$r} = $rs->count();
-            $rs->update( { $field => $keeper });
-        }
+            while (my ($r, $field) = each(%$RESULTSET_PATRON_ID_MAPPING)) {
+                my $rs = $schema->resultset($r)->search({ $field => $borrowernumber} );
+                $results->{merged}->{ $borrowernumber }->{updated}->{$r} = $rs->count();
+                $rs->update( { $field => $keeper });
+            }
 
-        $patron->move_to_deleted();
-        $patron->delete();
-    }
+            $patron->move_to_deleted();
+            $patron->delete();
+        }
+    });
 
     $results->{keeper} = $patron_to_keep;
 
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt
index 78a61d97c3..24ee5520dc 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt
@@ -104,26 +104,30 @@ $(document).ready(function() {
             [% ELSIF action == 'merge' %]
                 <h4>Results</h4>
 
-                <p>
-                    Patron records merged into <a href="moremember.pl?borrowernumber=[% results.keeper.id %]">[% results.keeper.firstname %] [% results.keeper.surname %] ([% results.keeper.cardnumber | html %])</a>
-                </p>
+                [% IF error %]
+                    <div class="dialog alert">Merge failed! The following error was reported: [% error %].</div>
+                [% ELSE %]
+                    <p>
+                        Patron records merged into <a href="moremember.pl?borrowernumber=[% results.keeper.id %]">[% results.keeper.firstname %] [% results.keeper.surname %] ([% results.keeper.cardnumber | html %])</a>
+                    </p>
 
-                [% FOREACH pair IN results.merged.pairs %]
-                    [% SET patron = pair.value.patron %]
+                    [% FOREACH pair IN results.merged.pairs %]
+                        [% SET patron = pair.value.patron %]
 
-                    <h5>[% patron.firstname %] [% patron.surname %] ([% patron.cardnumber %])</h5>
+                        <h5>[% patron.firstname %] [% patron.surname %] ([% patron.cardnumber %])</h5>
 
-                    [% USE Dumper %]
-                    [% FOREACH r IN pair.value.updated.pairs %]
-                        [% SET name = r.key %]
-                        [% SET count = r.value %]
-                        [% IF count %]
-                            <p>
-                                [% count %] [% PROCESS display_names rs = name %] transferred.
-                                [% IF name == 'Reserve' %]
-                                    <strong>It is advisable to check for and resolve duplicate holds due to merging.</strong>
-                                [% END %]
-                            </p>
+                        [% USE Dumper %]
+                        [% FOREACH r IN pair.value.updated.pairs %]
+                            [% SET name = r.key %]
+                            [% SET count = r.value %]
+                            [% IF count %]
+                                <p>
+                                    [% count %] [% PROCESS display_names rs = name %] transferred.
+                                    [% IF name == 'Reserve' %]
+                                        <strong>It is advisable to check for and resolve duplicate holds due to merging.</strong>
+                                    [% END %]
+                                </p>
+                            [% END %]
                         [% END %]
                     [% END %]
                 [% END %]
diff --git a/members/merge-patrons.pl b/members/merge-patrons.pl
index 8ad2cfbd9d..a35fe83f29 100755
--- a/members/merge-patrons.pl
+++ b/members/merge-patrons.pl
@@ -47,8 +47,13 @@ if ( $action eq 'show' ) {
     $template->param( patrons => $patrons );
 } elsif ( $action eq 'merge' ) {
     my $keeper = $cgi->param('keeper');
-    my $results = Koha::Patrons->merge( { keeper => $keeper, patrons => \@ids } );
-    $template->param( results => $results );
+    my $results;
+    eval { $results = Koha::Patrons->merge( { keeper => $keeper, patrons => \@ids } ); };
+    if ($@) {
+        $template->param( results => $results );
+    } else {
+        $template->param( error => $@ );
+    }
 }
 
 $template->param( action => $action );
-- 
2.39.5