Attachment 79917 Details for Bug 21337 – Bug 21337: (QA follow-up) Rollback for partial delete

[patch] Bug 21337: (QA follow-up) Rollback for partial delete

Bug-21337-QA-follow-up-Rollback-for-partial-delete.patch (text/plain), 4.58 KB, created by Josef Moravec on 2018-10-04 06:05:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Moravec

Created: 2018-10-04 06:05:52 UTC

Size: 4.58 KB

patch

obsolete

>From 162dbb76fa1858c26120175ec0e1f4156d1fe69c Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Mon, 1 Oct 2018 09:52:21 +0200
>Subject: [PATCH] Bug 21337: (QA follow-up) Rollback for partial delete
>
>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
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 21337: 78586 | 78619 | 78621 | 79461 | 79462 | 79708 | 79709 | 79915 | 79916 | 79917 | 80624 | 81251 | 81252 | 81257