Bugzilla – Attachment 81257 Details for
Bug 21337
Add Koha::Patrons->delete
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 21337: (QA follow-up) On the road to a perfect delete
Bug-21337-QA-follow-up-On-the-road-to-a-perfect-de.patch (text/plain), 4.41 KB, created by
Marcel de Rooy
on 2018-10-26 09:08:50 UTC
(
hide
)
Description:
Bug 21337: (QA follow-up) On the road to a perfect delete
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2018-10-26 09:08:50 UTC
Size:
4.41 KB
patch
obsolete
>From d7d7dbb217d8e21d6ff432143f1698692722d7e2 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Thu, 25 Oct 2018 14:17:16 +0200 >Subject: [PATCH] Bug 21337: (QA follow-up) On the road to a perfect delete >Content-Type: text/plain; charset=utf-8 > >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 <m.de.rooy@rijksmuseum.nl> >--- > 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 dedf4c4..51f37db 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 c552220..a78d08f 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.1.4
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 21337
:
78586
|
78619
|
78621
|
79461
|
79462
|
79708
|
79709
|
79915
|
79916
|
79917
|
80624
|
81251
|
81252
|
81257