From 9881825779542234f868bada8bac403c0f73591f Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 25 Oct 2018 14:17:16 +0200 Subject: [PATCH] Bug 21684: (QA follow-up) On the road to a perfect delete Adjusting Koha::Patrons->delete: We should not throw an exception if we get a -1 from Patron->delete. A zero should trigger an exception. And an undef too since it is probably a DBIx exception.. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy --- Koha/Patrons.pm | 20 +++++++++++++++----- t/db_dependent/Koha/Patrons.t | 26 ++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index dedf4c4a65..51f37db03c 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -218,22 +218,32 @@ sub anonymise_issue_history { Includes a move to deletedborrowers if move flag set. Just like DBIx, the delete will only succeed when all entries could be - deleted. Returns true or throws an exception. + deleted. Returns number of deletes or throws an exception. A number of + zero deletes is represented as the true value '0E0'. =cut sub delete { my ( $self, $params ) = @_; - my $patrons_deleted; + my $patrons_deleted = 0; $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::FailedDelete->throw; - $patrons_deleted++; + my $delete = eval { $patron->delete }; + if( defined($delete) && $delete == 1 ) { + $patrons_deleted++; + } elsif( defined($delete) && $delete == -1 ) { + next; # not in storage: no reason to stop + } elsif( defined($delete) && $delete == 0 ) { + Koha::Exceptions::Patron::FailedDelete->throw; + } else { # an undef is a bit unclear, add exception to be safe + Koha::Exceptions::Patron::FailedDelete->throw; + } } }, $self, $params ); - return $patrons_deleted; + return if $@; + return $patrons_deleted || '0E0'; } =head3 _type diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index c5522209fb..a78d08f654 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -424,7 +424,7 @@ subtest "delete" => sub { }; subtest 'Koha::Patrons->delete' => sub { - plan tests => 4; + plan tests => 6; my $mod_patron = Test::MockModule->new( 'Koha::Patron' ); my $moved_to_deleted = 0; @@ -438,13 +438,31 @@ subtest 'Koha::Patrons->delete' => sub { is( $set->delete({ move => 1 }), 2, 'Two patrons deleted' ); 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; } ); + # Check return value / exception, rollback + my $mod_object = Test::MockModule->new( 'Koha::Object' ); + my $mock_ret = -1; # no rollback expected + $mod_object->mock( 'delete', sub { return $mock_ret; } ); $patron1 = $builder->build_object({ class => 'Koha::Patrons' }); $id1 = $patron1->borrowernumber; $set = Koha::Patrons->search({ borrowernumber => { '>=' => $id1 }}); + is( $set->delete, '0E0', 'No exception for return value -1' ); + + $mock_ret = 0; # this should trigger rollback/exception + $set->_resultset->reset; + throws_ok { $set->delete } 'Koha::Exceptions::Patron::FailedDelete', + 'Exception raised for deleting patron on return value 0'; + + # We now trigger a FK error by deleting patron with issue + $mod_object->unmock_all; + my $issue = $builder->build_object({ class => 'Koha::Checkouts' }); + my $id2 = $issue->borrowernumber; + $set->_resultset->reset; throws_ok { $set->delete } 'Koha::Exceptions::Patron::FailedDelete', - 'Exception raised for deleting patron'; + 'Exception raised for deleting patron with issue'; + # FIXME And here we should love to check the number of patrons, + # BUT unfortunately we cannot. Why? Since Koha::Patrons' txn_do creates + # a nested transaction, the rollback does nothing in a test. + # So we should *TRUST* txn_do here.. }; subtest 'add_enrolment_fee_if_needed' => sub { -- 2.11.0