From 9e68d22ea7db30b8c15d8b626930f16eb93afd45 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy 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 --- 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