From 839ba08e6e0c3d01b0d063f98fa8ca07b37035c0 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Sat, 9 Jul 2016 16:03:11 +0100
Subject: [PATCH] Bug 16891: Move C4::Members::MoveMemberToDeleted to
 Koha::Patron->move_to_deleted

This patch removes the C4::Members::MoveMemberToDeleted subroutine in
order to replace it with the Koha::Patron->move_to_deleted method.
Next after this change, we will move C4::Members::HandleDelBorrower and
C4::Members::DelMember to the same module to simplify the code in
members/deletemem.pl and misc/cronjobs/delete_patrons.pl

Test plan:
1/ Delete a patron from the staff interface and make sure (s)he has been moved to
the deletedborrowers table.
2/ Use the "Batch patron deletion" tool (tools/cleanborrowers.pl) to
remove patron. Make sure the "Permanently delete these patrons" and "Move
these patrons to the trash" options work as before
3/ Same as previously but using the cronjob
misc/cronjobs/delete_patrons.pl.
---
 C4/Members.pm                   | 24 ------------------------
 Koha/Patron.pm                  | 15 +++++++++++++++
 members/deletemem.pl            |  4 +++-
 misc/cronjobs/delete_patrons.pl | 36 +++++++++++++++++++-----------------
 t/db_dependent/Koha/Patrons.t   | 16 +++++++++++++++-
 t/db_dependent/Members.t        | 16 +---------------
 tools/cleanborrowers.pl         |  3 ++-
 7 files changed, 55 insertions(+), 59 deletions(-)

diff --git a/C4/Members.pm b/C4/Members.pm
index 63e3f36..b1c3c83 100644
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -113,7 +113,6 @@ BEGIN {
     push @EXPORT, qw(
         &AddMember
         &AddMember_Opac
-        &MoveMemberToDeleted
         &ExtendMemberSubscriptionTo
     );
 
@@ -1521,29 +1520,6 @@ sub GetSortDetails {
     return ($sortvalue) unless ($lib);
 }
 
-=head2 MoveMemberToDeleted
-
-  $result = &MoveMemberToDeleted($borrowernumber);
-
-Copy the record from borrowers to deletedborrowers table.
-The routine returns 1 for success, undef for failure.
-
-=cut
-
-sub MoveMemberToDeleted {
-    my ($member) = shift or return;
-
-    my $schema       = Koha::Database->new()->schema();
-    my $borrowers_rs = $schema->resultset('Borrower');
-    $borrowers_rs->result_class('DBIx::Class::ResultClass::HashRefInflator');
-    my $borrower = $borrowers_rs->find($member);
-    return unless $borrower;
-
-    my $deleted = $schema->resultset('Deletedborrower')->create($borrower);
-
-    return $deleted ? 1 : undef;
-}
-
 =head2 DelMember
 
     DelMember($borrowernumber);
diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index 42b066b..4089c8c 100644
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -208,6 +208,21 @@ sub update_password {
     return 1;
 }
 
+=head2 move_to_deleted
+
+my $is_moved = $patron->move_to_deleted;
+
+Move a patron to the deletedborrowers table.
+This can be done before deleting a patron, to make sure the data are not completely deleted.
+
+=cut
+
+sub move_to_deleted {
+    my ($self) = @_;
+    my $patron_infos = $self->unblessed;
+    return Koha::Database->new->schema->resultset('Deletedborrower')->create($patron_infos);
+}
+
 =head3 type
 
 =cut
diff --git a/members/deletemem.pl b/members/deletemem.pl
index fd53ecc..3b9d1fb 100755
--- a/members/deletemem.pl
+++ b/members/deletemem.pl
@@ -31,6 +31,7 @@ use C4::Auth;
 use C4::Members;
 use C4::Branch; # GetBranches
 use Module::Load;
+use Koha::Patrons;
 use Koha::Patron::Images;
 if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) {
     load Koha::NorwegianPatronDB, qw( NLMarkForDeletion NLSync );
@@ -146,7 +147,8 @@ if ($countissues > 0 or $flags->{'CHARGES'}  or $data->{'borrowernumber'} or $de
 output_html_with_http_headers $input, $cookie, $template->output;
 
 } else {
-    MoveMemberToDeleted($member);
+    my $patron = Koha::Patrons->find( $member );
+    $patron->move_to_deleted;
     C4::Members::HandleDelBorrower($member);
     DelMember($member);
     print $input->redirect("/cgi-bin/koha/members/members-home.pl");
diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl
index a76d1dd..4e991c1 100755
--- a/misc/cronjobs/delete_patrons.pl
+++ b/misc/cronjobs/delete_patrons.pl
@@ -7,6 +7,7 @@ use Getopt::Long;
 
 use C4::Members;
 use Koha::DateUtils;
+use Koha::Patrons;
 use C4::Log;
 
 my ( $help, $verbose, $not_borrowed_since, $expired_before, $category_code,
@@ -71,23 +72,24 @@ for my $member (@$members) {
         next;
     }
 
-    eval {
-        C4::Members::MoveMemberToDeleted( $borrowernumber )
-          if $confirm;
-    };
-    if ($@) {
-        say "Failed to delete patron $borrowernumber, cannot move it: ($@)";
-        $dbh->rollback;
-        next;
-    }
-    eval {
-        C4::Members::HandleDelBorrower( $borrowernumber )
-          if $confirm;
-    };
-    if ($@) {
-        say "Failed to delete patron $borrowernumber, error handling its lists: ($@)";
-        $dbh->rollback;
-        next;
+    if ( $confirm ) {
+        my $deleted = eval {
+            Koha::Patrons->find( $borrowernumber )->move_to_deleted;
+        };
+        if ($@ or not $deleted) {
+            say "Failed to delete patron $borrowernumber, cannot move it" . ( $@ ? ": ($@)" : "" );
+            $dbh->rollback;
+            next;
+        }
+
+        eval {
+            C4::Members::HandleDelBorrower( $borrowernumber );
+        };
+        if ($@) {
+            say "Failed to delete patron $borrowernumber, error handling its lists: ($@)";
+            $dbh->rollback;
+            next;
+        }
     }
     eval { C4::Members::DelMember( $borrowernumber ) if $confirm; };
     if ($@) {
diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 980bf84..0b18270 100644
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -19,9 +19,10 @@
 
 use Modern::Perl;
 
-use Test::More tests => 6;
+use Test::More tests => 7;
 use Test::Warn;
 
+use C4::Members;
 use Koha::Patron;
 use Koha::Patrons;
 use Koha::Database;
@@ -127,6 +128,19 @@ subtest 'update_password' => sub {
     is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should not have logged' );
 };
 
+subtest "move_to_deleted" => sub {
+    plan tests => 2;
+    my $patron = $builder->build( { source => 'Borrower' } );
+    my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} );
+    is( ref( $retrieved_patron->move_to_deleted ), 'Koha::Schema::Result::Deletedborrower', 'Koha::Patron->move_to_deleted should return the Deleted patron' )
+      ;    # FIXME This should be Koha::Deleted::Patron
+    my $deleted_patron = $schema->resultset('Deletedborrower')
+        ->search( { borrowernumber => $patron->{borrowernumber} }, { result_class => 'DBIx::Class::ResultClass::HashRefInflator' } )
+        ->next;
+    is_deeply( $deleted_patron, $patron, 'Koha::Patron->move_to_deleted should have correctly moved the patron to the deleted table' );
+    C4::Members::DelMember( $patron->{borrowernumber} );    # Cleanup
+};
+
 $retrieved_patron_1->delete;
 is( Koha::Patrons->search->count, $nb_of_patrons + 1, 'Delete should have deleted the patron' );
 
diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
index 006207b..36d491a 100755
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 79;
+use Test::More tests => 77;
 use Test::MockModule;
 use Data::Dumper;
 use C4::Context;
@@ -95,20 +95,6 @@ testAgeAccessors(\%data); #Age accessor tests don't touch the db so it is safe t
 my $addmem=AddMember(%data);
 ok($addmem, "AddMember()");
 
-# It's not really a Move, it's a Copy.
-my $result = MoveMemberToDeleted($addmem);
-ok($result,"MoveMemberToDeleted()");
-
-my $sth = $dbh->prepare("SELECT * from borrowers WHERE borrowernumber=?");
-$sth->execute($addmem);
-my $MemberAdded = $sth->fetchrow_hashref;
-
-$sth = $dbh->prepare("SELECT * from deletedborrowers WHERE borrowernumber=?");
-$sth->execute($addmem);
-my $MemberMoved = $sth->fetchrow_hashref;
-
-is_deeply($MemberMoved,$MemberAdded,"Confirm MoveMemberToDeleted.");
-
 my $member=GetMemberDetails("",$CARDNUMBER)
   or BAIL_OUT("Cannot read member with card $CARDNUMBER");
 
diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl
index d108d53..f37a28e 100755
--- a/tools/cleanborrowers.pl
+++ b/tools/cleanborrowers.pl
@@ -40,6 +40,7 @@ use C4::Output;
 use C4::Members;        # GetBorrowersWhoHavexxxBorrowed.
 use C4::Circulation;    # AnonymiseIssueHistory.
 use Koha::DateUtils qw( dt_from_string output_pref );
+use Koha::Patrons;
 use Date::Calc qw/Today Add_Delta_YM/;
 use Koha::List::Patron;
 
@@ -128,7 +129,7 @@ elsif ( $step == 3 ) {
         for ( my $i = 0 ; $i < $totalDel ; $i++ ) {
             $radio eq 'testrun' && last;
             my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'};
-            $radio eq 'trash' && MoveMemberToDeleted($borrowernumber);
+            $radio eq 'trash' && Koha::Patrons->find($borrowernumber)->move_to_deleted;
             C4::Members::HandleDelBorrower($borrowernumber);
             DelMember($borrowernumber);
         }
-- 
2.8.1