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.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tested the delete_patrons.pl script and cleanborrowers.pl too.
Tests (are relevant and) pass and the qa scripts are happy too :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

C4/Members.pm

@@ -107,7 +107,6 @@ BEGIN {
     push @EXPORT, qw(
         &AddMember
         &AddMember_Opac
-        &MoveMemberToDeleted
     );
 
     #Check data
@@ -1309,29 +1308,6 @@ sub SetAge{
     return $borrower;
 }    # sub SetAge
 
-=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);

Koha/Patron.pm

@@ -267,6 +267,21 @@ sub track_login {
     $self->lastseen( dt_from_string() )->store;
 }
 
+=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

members/deletemem.pl

@@ -31,6 +31,7 @@ use C4::Output;
 use C4::Auth;
 use C4::Members;
 use Module::Load;
+use Koha::Patrons;
 use Koha::Patron::Images;
 use Koha::Token;
 
@@ -160,7 +161,8 @@ if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'}  or $is_
             secret => md5_base64( C4::Context->config('pass') ),
             token  => scalar $input->param('csrf_token'),
         });
-    MoveMemberToDeleted($member);
+    my $patron = Koha::Patrons->find( $member );
+    $patron->move_to_deleted;
     C4::Members::HandleDelBorrower($member);
     DelMember($member);
     # TODO Tell the user everything went ok

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, $last_seen,
@@ -76,23 +77,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 ($@) {

t/db_dependent/Koha/Patrons.t

@@ -19,10 +19,13 @@
 
 use Modern::Perl;
 
-use Test::More tests => 8;
+use Test::More tests => 9;
 use Test::Warn;
 
 use C4::Circulation;
+
+use C4::Members;
+
 use Koha::Patron;
 use Koha::Patrons;
 use Koha::Database;
@@ -207,6 +210,19 @@ subtest 'renew_account' => sub {
     $retrieved_patron->delete;
 };
 
+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' );
 

t/db_dependent/Members.t

@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 84;
+use Test::More tests => 82;
 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");
 

tools/cleanborrowers.pl

@@ -40,6 +40,7 @@ use C4::Members;        # GetBorrowersWhoHavexxxBorrowed.
 use C4::Circulation;    # AnonymiseIssueHistory.
 use Koha::DateUtils qw( dt_from_string output_pref );
 use Koha::Patron::Categories;
+use Koha::Patrons;
 use Date::Calc qw/Today Add_Delta_YM/;
 use Koha::List::Patron;
 
@@ -146,7 +147,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);
         }