From 52a9dee300e43873268bcd87b1908dc9f89fe9a2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 12 Dec 2019 14:28:03 +0100 Subject: [PATCH] Bug 21684: Fix delete methods and add more tests Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- Koha/Object.pm | 6 +- Koha/Patrons.pm | 8 +- t/db_dependent/Koha/Objects.t | 395 +++++++++++++++++++++++++++------- 3 files changed, 327 insertions(+), 82 deletions(-) diff --git a/Koha/Object.pm b/Koha/Object.pm index ce4c231591..2139e070b3 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -216,11 +216,7 @@ Returns: sub delete { my ($self) = @_; - # Deleting something not in storage throws an exception - return -1 unless $self->_result()->in_storage(); - - # Return a boolean for succcess - return $self->_result()->delete() ? 1 : 0; + return $self->_result()->delete; } =head3 $object->set( $properties_hashref ) diff --git a/Koha/Patrons.pm b/Koha/Patrons.pm index fbe696ec7a..f4ca8456a6 100644 --- a/Koha/Patrons.pm +++ b/Koha/Patrons.pm @@ -220,9 +220,13 @@ sub delete { $self->_resultset->result_source->schema->txn_do( sub { my ( $set, $params ) = @_; my $count = $set->count; - while( my $patron = $set->next ) { + while ( my $patron = $set->next ) { + + next unless $patron->in_storage; + $patron->move_to_deleted if $params->{move}; - $patron->delete == 1 || Koha::Exceptions::Patron::FailedDelete->throw; + $patron->delete; + $patrons_deleted++; } warn "Deleted $count patrons\n" if $params->{verbose}; diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index 87efa3caea..43810fde02 100644 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 17; +use Test::More tests => 18; use Test::Exception; use Test::Warn; @@ -341,86 +341,331 @@ subtest 'Return same values as DBIx::Class' => sub { $schema->storage->txn_begin; subtest 'Simple Koha::Objects - Koha::Cities' => sub { - plan tests => 3; - - my ( $r_us, $e_us, $r_them, $e_them ); - { - my $c = Koha::City->new({ city_name => 'city4test' }); - try {$r_us = $c->delete;} catch { $e_us = ref($_) }; - - - $c = $schema->resultset('City')->new({ city_name => 'city4test' }); - try {$r_them = $c->delete;} catch { $e_them = ref($_) }; - - is( $r_us, $r_them ); - is( $e_us, $e_them ); # FIXME This is need adjustment, we want to throw a Koha::Exception - } - - { - - my $city = $builder->build_object({ class => 'Koha::Cities' }); - try{ - $schema->storage->txn_do(sub{ - my $c = Koha::Cities->find($city->cityid); - $r_us = $c->delete; - $c->update({force_fail=>'foo'}); - });}; - try{ - $schema->storage->txn_do(sub{ - my $c = $schema->resultset('City')->find($city->cityid); - $r_them = $c->delete; - $c->update({force_fail=>'foo'}); - });}; - is( $r_us, $r_them ); - } - }; - - subtest 'Overwritten Koha::Objects->delete - Koha::Patrons' => sub { - plan tests => 4; - - my ( $r_us, $e_us, $r_them, $e_them ); - - my $patron = $builder->build_object({ class => 'Koha::Patrons' }); - my $patron_data = $patron->unblessed; - $patron->delete; - - { - my $p = Koha::Patron->new( $patron_data ); - try {$r_us = $p->delete;} catch { $e_us = ref($_) }; + plan tests => 2; + + subtest 'Koha::Object->delete' => sub { + + plan tests => 4; + + my ( $r_us, $e_us, $r_them, $e_them ); + + # CASE 1 - Delete an existing object + my $c = Koha::City->new( { city_name => 'city4test' } )->store; + try { $r_us = $c->delete; } catch { $e_us = $_ }; + $c = $schema->resultset('City')->new( { city_name => 'city4test_2' } )->update_or_insert; + try { $r_them = $c->delete; } catch { $e_them = $_ }; + ok( ref($r_us) && ref($r_them), + 'Successful delete should return the object ' ); + ok( !defined $e_us && !defined $e_them, + 'Successful delete should not raise an exception' ); + + # CASE 2 - Delete an object that is not in storage + try { $r_us = $r_us->delete; } catch { $e_us = $_ }; + try { $r_them = $r_them->delete; } catch { $e_them = $_ }; + ok( + defined $e_us && defined $e_them, + 'Delete an object that is not in storage should raise an exception' + ); + is( ref($e_us), 'DBIx::Class::Exception' ) + ; # FIXME This needs adjustement, we want to throw a Koha::Exception + + }; + + subtest 'Koha::Objects->delete' => sub { + + plan tests => 4; + + my ( $r_us, $e_us, $r_them, $e_them ); + + # CASE 1 - Delete existing objects + my $city_1 = $builder->build_object({ class => 'Koha::Cities' }); + my $city_2 = $builder->build_object({ class => 'Koha::Cities' }); + my $city_3 = $builder->build_object({ class => 'Koha::Cities' }); + my $cities = Koha::Cities->search( + { + cityid => { + -in => [ + $city_1->cityid, + $city_2->cityid, + $city_3->cityid, + ] + } + } + ); + + try { $r_us = $cities->delete; } catch { $e_us = $_ }; + + $city_1 = $builder->build_object({ class => 'Koha::Cities' }); + $city_2 = $builder->build_object({ class => 'Koha::Cities' }); + $city_3 = $builder->build_object({ class => 'Koha::Cities' }); + $cities = $schema->resultset('City')->search( + { + cityid => { + -in => [ + $city_1->cityid, + $city_2->cityid, + $city_3->cityid, + ] + } + } + ); + + try { $r_them = $cities->delete; } catch { $e_them = $_ }; + + ok( $r_us == 3 && $r_them == 3 ); + ok (!defined($e_us) && !defined($e_them)); + + # CASE 2 - One of the object is not in storage + $city_1 = $builder->build_object({ class => 'Koha::Cities' }); + $city_2 = $builder->build_object({ class => 'Koha::Cities' }); + $city_3 = $builder->build_object({ class => 'Koha::Cities' }); + $cities = Koha::Cities->search( + { + cityid => { + -in => [ + $city_1->cityid, + $city_2->cityid, + $city_3->cityid, + ] + } + } + ); + + $city_2->delete; # We delete one of the object + try { $r_us = $cities->delete; } catch { $e_us = $_ }; + + $city_1 = $builder->build_object({ class => 'Koha::Cities' }); + $city_2 = $builder->build_object({ class => 'Koha::Cities' }); + $city_3 = $builder->build_object({ class => 'Koha::Cities' }); + $cities = $schema->resultset('City')->search( + { + cityid => { + -in => [ + $city_1->cityid, + $city_2->cityid, + $city_3->cityid, + ] + } + } + ); + $city_2->delete; # We delete one of the object + try { $r_them = $cities->delete; } catch { $e_them = $_ }; - $p = $schema->resultset('Borrower')->new( $patron_data ); - try {$r_them = $p->delete;} catch { $e_them = ref($_) }; + ok( $r_us == 2 && $r_them == 2 ); + ok (!defined($e_us) && !defined($e_them)); + }; + }; - is( $r_us, $r_them ); - is( $e_us, $e_them ); # FIXME This is need adjustment, we want to throw a Koha::Exception - } + subtest 'Overwritten Koha::Objects->delete - Koha::Patrons' => sub { - { - try { - $schema->storage->txn_do( - sub { - my $p = Koha::Patrons->find( $patron->borrowernumber ); - $r_us = $p->delete; - $p->update( { force_fail => 'foo' } ); + plan tests => 2; + + subtest 'Koha::Object->delete' => sub { + + plan tests => 6; + + my ( $r_us, $e_us, $r_them, $e_them ); + + # CASE 1 - Delete an existing patron + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_data = $patron->unblessed; + $patron->delete; + + $patron = Koha::Patron->new( $patron_data )->store; + try {$r_us = $patron->delete;} catch { $e_us = $_ }; + $patron = $schema->resultset('Borrower')->new( $patron_data )->update_or_insert; + try {$r_them = $patron->delete;} catch { $e_them = $_ }; + ok( ref($r_us) && ref($r_them), + 'Successful delete should return the patron object' ); + ok( !defined $e_us && !defined $e_them, + 'Successful delete should not raise an exception' ); + + # CASE 2 - Delete a patron that is not in storage + try { $r_us = $r_us->delete; } catch { $e_us = $_ }; + try { $r_them = $r_them->delete; } catch { $e_them = $_ }; + ok( + defined $e_us && defined $e_them, + 'Delete a patron that is not in storage should raise an exception' + ); + is( ref($e_us), 'DBIx::Class::Exception' ) + ; # FIXME This needs adjustement, we want to throw a Koha::Exception + + # CASE 3 - Delete a patron that cannot be deleted (as a checkout) + $patron = Koha::Patron->new($patron_data)->store; + $builder->build_object( + { + class => 'Koha::Checkouts', + value => { borrowernumber => $patron->borrowernumber } + } + ); + try { $r_us = $r_us->delete; } catch { $e_us = $_ }; + $patron = $schema->resultset('Borrower')->find( $patron->borrowernumber ); + try { $r_them = $r_them->delete; } catch { $e_them = $_ }; + ok( + defined $e_us && defined $e_them, + 'Delete a patron that cannot be deleted should raise an exception' + ); + is( ref($e_us), 'DBIx::Class::Exception' ) + ; # FIXME This needs adjustement, we want to throw a Koha::Exception + }; + + subtest 'Koha::Objects->delete' => sub { + + plan tests => 9; + + my ( $r_us, $e_us, $r_them, $e_them ); + + # CASE 1 - Delete existing objects + my $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + my $patrons = Koha::Patrons->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] } - ); - }; - try { - $schema->storage->txn_do( - sub { - my $p = $schema->resultset('City')->find( $patron->borrowernumber ); - $r_them = $p->delete; - $p->update( { force_fail => 'foo' } ); + } + ); + + try { $r_us = $patrons->delete; } catch { $e_us = $_ }; + + $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + $patrons = $schema->resultset('Borrower')->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] } - ); - }; - is( $r_us, $r_them ); - } - - # TODO Test value of Koha::Object->delete when the row cannot be deleted - - # TODO Add tests for Koha::Objects->delete + } + ); + + try { $r_them = $patrons->delete; } catch { $e_them = $_ }; + + ok( $r_us == 3 && $r_them == 3, '->delete should return the number of deleted patrons' ); + ok (!defined($e_us) && !defined($e_them), '->delete should not raise exception if everything went well'); + + # CASE 2 - One of the patrons is not in storage + undef $_ for $r_us, $e_us, $r_them, $e_them; + $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + $patrons = Koha::Patrons->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] + } + } + ); + + $patron_2->delete; # We delete one of the patron + try { $r_us = $patrons->delete; } catch { $e_us = $_ }; + + $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + $patrons = $schema->resultset('Borrower')->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] + } + } + ); + + $patron_2->delete; # We delete one of the patron + try { $r_them = $patrons->delete; } catch { $e_them = $_ }; + + ok( $r_us == 2 && $r_them == 2, 'Delete patrons with one that was not in storage should delete the patrons' ); + ok (!defined($e_us) && !defined($e_them), 'no exception should be raised if at least one patron was not in storage'); + + # CASE 3 - Delete a set of patrons with one that that cannot be deleted (as a checkout) + undef $_ for $r_us, $e_us, $r_them, $e_them; + $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + $patrons = Koha::Patrons->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] + } + } + ); + + # Adding a checkout to patron_2 + $builder->build_object( + { + class => 'Koha::Checkouts', + value => { borrowernumber => $patron_2->borrowernumber } + } + ); + + warning_like { + try { $r_us = $patrons->delete; } catch { $e_us = $_ }; + } + qr{DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails}, + "Foreign key constraint DBI error should be logged"; + my $not_deleted_us = $patron_1->in_storage + $patron_2->in_storage + $patron_3->in_storage; + + $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); + $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); + $patrons = $schema->resultset('Borrower')->search( + { + borrowernumber => { + -in => [ + $patron_1->borrowernumber, + $patron_2->borrowernumber, + $patron_3->borrowernumber + ] + } + } + ); + + # Adding a checkout to patron_2 + $builder->build_object( + { + class => 'Koha::Checkouts', + value => { borrowernumber => $patron_2->borrowernumber } + } + ); + + warning_like { + try { $r_them = $patrons->delete; } catch { $e_them = $_ }; + } + qr{DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails}, + "Foreign key constraint DBI error should be logged"; + + my $not_deleted_them = $patron_1->in_storage + $patron_2->in_storage + $patron_3->in_storage; + ok( + defined $e_us && defined $e_them, + 'Delete patrons with one that cannot be deleted should raise an exception' + ); + is( ref($e_us), 'DBIx::Class::Exception' ) + ; # FIXME This needs adjustement, we want to throw a Koha::Exception + + ok($not_deleted_us == 3 && $not_deleted_them == 3, 'If one patron cannot be deleted, none should have been deleted'); + }; }; $schema->storage->txn_rollback; -- 2.39.5