From 87071d8d5a9fa08456d74df34118ab104351cdca Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Mon, 1 Oct 2018 09:52:21 +0200
Subject: [PATCH] Bug 21337: (QA follow-up) Rollback for partial delete
Content-Type: text/plain; charset=utf-8

Puts delete loop in a txn_do.
Raises Koha::Exceptions::Patron::Delete when Patron->delete does not return
true (like 0 or -1).
Unit test adjusted accordingly.

Note: A follow-up report for raising exceptions in Object->delete could well
be considered. Not here please.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 Koha/Exceptions/Patron.pm     | 14 ++++++++++++++
 Koha/Patrons.pm               | 25 +++++++++++--------------
 t/db_dependent/Koha/Patrons.t | 20 ++++++++++++++++----
 3 files changed, 41 insertions(+), 18 deletions(-)
 create mode 100644 Koha/Exceptions/Patron.pm

diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm
new file mode 100644
index 0000000..754f004
--- /dev/null
+++ b/Koha/Exceptions/Patron.pm
@@ -0,0 +1,14 @@
+package Koha::Exceptions::Patron;
+
+use Modern::Perl;
+
+use Exception::Class (
+    'Koha::Exceptions::Patron' => {
+        description => "Something went wrong!"
+    },
+    'Koha::Exceptions::Patron::Delete' => {
+        description => "Deleting patron failed"
+    },
+);
+
+1;
diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm
index 37df417..0068f34 100644
--- a/Koha/Patrons.pm
+++ b/Koha/Patrons.pm
@@ -28,6 +28,7 @@ use Koha::DateUtils;
 use Koha::ArticleRequests;
 use Koha::ArticleRequest::Status;
 use Koha::Patron;
+use Koha::Exceptions::Patron;
 
 use base qw(Koha::Objects);
 
@@ -216,25 +217,21 @@ sub anonymise_issue_history {
     and let DBIx do the job without further housekeeping.)
     Includes a move to deletedborrowers if move flag set.
 
-    Return value (if relevant) is based on the individual return values.
+    Just like DBIx, the delete will only succeed when all entries could be
+    deleted. Returns true or throws an exception.
 
 =cut
 
 sub delete {
     my ( $self, $params ) = @_;
-    my (@res, $rv);
-    $rv = 1;
-    while( my $patron = $self->next ) {
-        $patron->move_to_deleted if $params->{move};
-        push @res, $patron->delete;
-        $rv=-1 if $res[-1]==-1;
-        $rv=0 if $rv==1 && $res[-1]==0;
-    }
-
-    # Return -1 if we encountered a single -1
-    # Return 0 if we encountered a single 0 (but not a -1)
-    # Return 1 if all individual deletes passed
-    return $rv;
+    $self->_resultset->result_source->schema->txn_do( sub {
+        my ( $set, $params ) = @_;
+        while( my $patron = $set->next ) {
+            $patron->move_to_deleted if $params->{move};
+            $patron->delete == 1 || Koha::Exceptions::Patron::Delete->throw;
+        }
+    }, $self, $params );
+    return 1;
 }
 
 =head3 _type
diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index dac7f52..ea8c17e 100644
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -22,6 +22,7 @@ use Modern::Perl;
 use Test::More tests => 33;
 use Test::Warn;
 use Test::Exception;
+use Test::MockModule;
 use Time::Fake;
 use DateTime;
 use JSON;
@@ -422,16 +423,27 @@ subtest "delete" => sub {
 };
 
 subtest 'Koha::Patrons->delete' => sub {
-    plan tests => 3;
+    plan tests => 4;
+
+    my $mod_patron = Test::MockModule->new( 'Koha::Patron' );
+    my $moved_to_deleted = 0;
+    $mod_patron->mock( 'move_to_deleted', sub { $moved_to_deleted++; } );
+
     my $patron1 = $builder->build_object({ class => 'Koha::Patrons' });
     my $patron2 = $builder->build_object({ class => 'Koha::Patrons' });
     my $id1 = $patron1->borrowernumber;
     my $set = Koha::Patrons->search({ borrowernumber => { '>=' => $id1 }});
     is( $set->count, 2, 'Two patrons found as expected' );
-    my $count1 = $schema->resultset('Deletedborrower')->count;
     is( $set->delete({ move => 1 }), 1, 'Two patrons deleted' );
-    my $count2 = $schema->resultset('Deletedborrower')->count;
-    is( $count2, $count1 + 2, 'Patrons moved to deletedborrowers' );
+    is( $moved_to_deleted, 2, 'Patrons moved to deletedborrowers' );
+
+    # Add again, test if we can raise an exception
+    $mod_patron->mock( 'delete', sub { return -1; } );
+    $patron1 = $builder->build_object({ class => 'Koha::Patrons' });
+    $id1 = $patron1->borrowernumber;
+    $set = Koha::Patrons->search({ borrowernumber => { '>=' => $id1 }});
+    throws_ok { $set->delete } 'Koha::Exceptions::Patron::Delete',
+        'Exception raised for deleting patron';
 };
 
 subtest 'add_enrolment_fee_if_needed' => sub {
-- 
2.1.4