Bugzilla – Attachment 96218 Details for
Bug 21684
Koha::Object[s]->delete methods must behave identically as the corresponding DBIx::Class ones
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 21684: Fix delete methods and add more tests
Bug-21684-Fix-delete-methods-and-add-more-tests.patch (text/plain), 19.83 KB, created by
Jonathan Druart
on 2019-12-12 13:28:36 UTC
(
hide
)
Description:
Bug 21684: Fix delete methods and add more tests
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2019-12-12 13:28:36 UTC
Size:
19.83 KB
patch
obsolete
>From a5e07251a5eab694fb59a4e45271e8fab753d848 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 12 Dec 2019 14:28:03 +0100 >Subject: [PATCH] Bug 21684: Fix delete methods and add more tests > >--- > 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.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 21684
:
81317
|
81318
|
81319
|
90937
|
96218
|
96254
|
96270
|
96271
|
96272
|
96429
|
96430
|
96431
|
96872
|
96888
|
96895
|
96898