Bug 16907: Koha::Patrons - Move DelMember to ->delete

This patch moves the C4::Members::DelMember subroutine to the
Koha::Patron module.
The delete method must be overwritten to permit handling of patron's
holds.

Test plan:
(With the 2 patches applied)
1/ Create a patron with holds and owner of lists
2/ Delete patrons using the web interface:
 - More > Delete on a patron page
 - Batch patron deletion tools
3/ and the cronjob script
 - perl misc/cronjobs/delete_patrons.pl -c [more options]

The patron should have been moved to the deletedborrowers table, his/her
holds and lists should have been deleted.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/Members.pm b/C4/Members.pm
index 22cb083..b183f39 100644 (file)
--- a/C4/Members.pm
+++ b/C4/Members.pm
@@ -98,11 +98,6 @@ BEGIN {
         &changepassword
     );
 
-    #Delete data
-    push @EXPORT, qw(
-        &DelMember
-    );
-
     #Insert data
     push @EXPORT, qw(
         &AddMember
@@ -1308,40 +1303,11 @@ sub SetAge{
     return $borrower;
 }    # sub SetAge
 
-=head2 DelMember
-
-    DelMember($borrowernumber);
-
-This function remove directly a borrower whitout writing it on deleteborrower.
-+ Deletes reserves for the borrower
-
-=cut
-
-sub DelMember {
-    my $dbh            = C4::Context->dbh;
-    my $borrowernumber = shift;
-    #warn "in delmember with $borrowernumber";
-    return unless $borrowernumber;    # borrowernumber is mandatory.
-    # Delete Patron's holds
-    my @holds = Koha::Holds->search({ borrowernumber => $borrowernumber });
-    $_->delete for @holds;
-
-    my $query = "
-       DELETE
-       FROM borrowers
-       WHERE borrowernumber = ?
-   ";
-    my $sth = $dbh->prepare($query);
-    $sth->execute($borrowernumber);
-    logaction("MEMBERS", "DELETE", $borrowernumber, "") if C4::Context->preference("BorrowersLog");
-    return $sth->rows;
-}
-
 =head2 HandleDelBorrower
 
      HandleDelBorrower($borrower);
 
-When a member is deleted (DelMember in Members.pm), you should call me first.
+When a member is deleted, you should call me first.
 This routine deletes/moves lists and entries for the deleted member/borrower.
 Lists owned by the borrower are deleted, but entries from the borrower to
 other lists are kept.
@@ -1800,7 +1766,7 @@ WHERE categorycode = ? AND DATEDIFF( NOW(), dateenrolled ) > ? |;
     $sth->execute( $category_code, $delay );
     my $cnt=0;
     while ( my ($borrowernumber) = $sth->fetchrow_array() ) {
-        DelMember($borrowernumber);
+        Koha::Patrons->find($borrowernumber)->delete;
         $cnt++;
     }
     return $cnt;

diff --git a/Koha/Patron.pm b/Koha/Patron.pm
index da343fa..14a0ccf 100644 (file)
--- a/Koha/Patron.pm
+++ b/Koha/Patron.pm
@@ -26,6 +26,7 @@ use C4::Context;
 use C4::Log;
 use Koha::Database;
 use Koha::DateUtils;
+use Koha::Holds;
 use Koha::Issues;
 use Koha::OldIssues;
 use Koha::Patron::Categories;
@@ -44,6 +45,31 @@ Koha::Patron - Koha Patron Object class
 
 =cut
 
+=head3 delete
+
+$patron->delete
+
+Delete a patron.
+
+=cut
+
+sub delete {
+    my ($self) = @_;
+
+    my $deleted;
+    $self->_result->result_source->schema->txn_do(
+        sub {
+            # Delete Patron's holds
+            # FIXME Should be $patron->get_holds
+            $_->delete for Koha::Holds->search( { borrowernumber => $self->borrowernumber } );
+
+            logaction( "MEMBERS", "DELETE", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog");
+            $deleted = $self->SUPER::delete;
+        }
+    );
+    return $deleted;
+}
+
 =head3 guarantor
 
 Returns a Koha::Patron object for this patron's guarantor

diff --git a/members/deletemem.pl b/members/deletemem.pl
index 7ab61bc..d3c9fb5 100755 (executable)
--- a/members/deletemem.pl
+++ b/members/deletemem.pl
@@ -164,7 +164,7 @@ if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'}  or $is_
     my $patron = Koha::Patrons->find( $member );
     $patron->move_to_deleted;
     C4::Members::HandleDelBorrower($member);
-    DelMember($member);
+    $patron->delete;
     # TODO Tell the user everything went ok
     print $input->redirect("/cgi-bin/koha/members/members-home.pl");
     exit 0; # Exit without error

diff --git a/misc/cronjobs/delete_expired_opac_registrations.pl b/misc/cronjobs/delete_expired_opac_registrations.pl
index 0f75a0f..66f9e16 100755 (executable)
--- a/misc/cronjobs/delete_expired_opac_registrations.pl
+++ b/misc/cronjobs/delete_expired_opac_registrations.pl
@@ -29,7 +29,6 @@ BEGIN {
 }
 
 use C4::Context;
-use C4::Members qw/ DelMember /;
 
 my $help;
 my $confirm;

diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl
index eea8f84..2f9c822 100755 (executable)
--- a/misc/cronjobs/delete_patrons.pl
+++ b/misc/cronjobs/delete_patrons.pl
@@ -78,9 +78,8 @@ for my $member (@$members) {
     }
 
     if ( $confirm ) {
-        my $deleted = eval {
-            Koha::Patrons->find( $borrowernumber )->move_to_deleted;
-        };
+        my $patron = Koha::Patrons->find( $borrowernumber );
+        my $deleted = eval { $patron->move_to_deleted; };
         if ($@ or not $deleted) {
             say "Failed to delete patron $borrowernumber, cannot move it" . ( $@ ? ": ($@)" : "" );
             $dbh->rollback;
@@ -95,12 +94,12 @@ for my $member (@$members) {
             $dbh->rollback;
             next;
         }
-    }
-    eval { C4::Members::DelMember( $borrowernumber ) if $confirm; };
-    if ($@) {
-        say "Failed to delete patron $borrowernumber: $@)";
-        $dbh->rollback;
-        next;
+        eval { $patron->delete if $confirm; };
+        if ($@) {
+            say "Failed to delete patron $borrowernumber: $@)";
+            $dbh->rollback;
+            next;
+        }
     }
     $dbh->commit;
     $deleted++;

diff --git a/misc/cronjobs/delete_unverified_opac_registrations.pl b/misc/cronjobs/delete_unverified_opac_registrations.pl
index c31fa47..13d5c4b 100755 (executable)
--- a/misc/cronjobs/delete_unverified_opac_registrations.pl
+++ b/misc/cronjobs/delete_unverified_opac_registrations.pl
@@ -29,7 +29,6 @@ BEGIN {
 }
 
 use C4::Context;
-use C4::Members qw/ DelMember /;
 
 my $help;
 my $confirm;

diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t
index c925c70..6e20c69 100755 (executable)
--- a/t/db_dependent/Auth_with_ldap.t
+++ b/t/db_dependent/Auth_with_ldap.t
@@ -25,6 +25,8 @@ use Test::Warn;
 
 use C4::Context;
 
+use Koha::Patrons;
+
 my $dbh = C4::Context->dbh;
 
 # Start transaction
@@ -179,7 +181,7 @@ subtest 'checkpw_ldap tests' => sub {
 
         $update               = 0;
         $desired_count_result = 0;    # user auth problem
-        C4::Members::DelMember( $borrower->{borrowernumber} );
+        Koha::Patrons->find( $borrower->{borrowernumber} )->delete;
         reload_ldap_module();
         is(
             C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' ),

diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
index 09f3386..ed12899 100644 (file)
--- a/t/db_dependent/Koha/Patrons.t
+++ b/t/db_dependent/Koha/Patrons.t
@@ -19,13 +19,12 @@
 
 use Modern::Perl;
 
-use Test::More tests => 9;
+use Test::More tests => 10;
 use Test::Warn;
 
-use C4::Circulation;
-
 use C4::Members;
 
+use Koha::Holds;
 use Koha::Patron;
 use Koha::Patrons;
 use Koha::Database;
@@ -220,7 +219,27 @@ subtest "move_to_deleted" => sub {
         ->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->delete( $patron->{borrowernumber} );    # Cleanup
+};
+
+subtest "delete" => sub {
+    plan tests => 4;
+    t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
+    my $patron           = $builder->build( { source => 'Borrower' } );
+    my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} );
+    my $hold             = $builder->build(
+        {   source => 'Reserve',
+            value  => { borrowernumber => $patron->{borrowernumber} }
+        }
+    );
+    my $deleted = $retrieved_patron->delete;
+    is( $deleted, 1, 'Koha::Patron->delete should return 1 if the patron has been correctly deleted' );
+    is( Koha::Patrons->find( $patron->{borrowernumber} ), undef, 'Koha::Patron->delete should have deleted the patron');
+
+    is( Koha::Holds->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's holds| );
+
+    my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'DELETE', object => $retrieved_patron->borrowernumber } )->count;
+    is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->delete should have logged' );
 };
 
 $retrieved_patron_1->delete;

diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t
index f0b0cb8..963d790 100755 (executable)
--- a/t/db_dependent/Members.t
+++ b/t/db_dependent/Members.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 82;
+use Test::More tests => 79;
 use Test::MockModule;
 use Data::Dumper;
 use C4::Context;
@@ -154,21 +154,6 @@ ModMember(borrowernumber => $member->{'borrowernumber'}, dateexpiry => '2001-01-
 $member = GetMemberDetails($member->{'borrowernumber'});
 ok($member->{is_expired}, "GetMemberDetails() indicates that patron is expired");
 
-# Create a reserve for the patron
-$builder->build({
-    source => 'Reserve',
-    value  => {
-        borrowernumber => $member->{ borrowernumber }
-    }
-});
-is( Koha::Holds->search({ borrowernumber => $member->{borrowernumber} })->count,
-    1, 'Hold created correctly' );
-DelMember($member->{borrowernumber});
-my $borrower = GetMember( cardnumber => $CARDNUMBER );
-is( $borrower, undef, 'DelMember should remove the patron' );
-is( Koha::Holds->search({ borrowernumber => $member->{borrowernumber} })->count,
-    0, 'Hold deleted correctly' );
-
 # Check_Userid tests
 %data = (
     cardnumber   => "123456789",
@@ -192,7 +177,7 @@ is( Check_Userid( 'tomasito.none', '' ), 0,
 is( Check_Userid( 'tomasitoxxx', '' ), 1,
     'non-existent userid -> unique (blank borrowernumber)' );
 
-$borrower = GetMember( borrowernumber => $borrowernumber );
+my $borrower = GetMember( borrowernumber => $borrowernumber );
 is( $borrower->{dateofbirth}, undef, 'AddMember should undef dateofbirth if empty string is given');
 is( $borrower->{debarred}, undef, 'AddMember should undef debarred if empty string is given');
 isnt( $borrower->{dateexpiry}, '0000-00-00', 'AddMember should not set dateexpiry to 0000-00-00 if empty string is given');

diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl
index ceb5722..b483514 100755 (executable)
--- a/tools/cleanborrowers.pl
+++ b/tools/cleanborrowers.pl
@@ -147,9 +147,10 @@ elsif ( $step == 3 ) {
         for ( my $i = 0 ; $i < $totalDel ; $i++ ) {
             $radio eq 'testrun' && last;
             my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'};
-            $radio eq 'trash' && Koha::Patrons->find($borrowernumber)->move_to_deleted;
+            my $patron = Koha::Patrons->find($borrowernumber);
+            $radio eq 'trash' && $patron->move_to_deleted;
             C4::Members::HandleDelBorrower($borrowernumber);
-            DelMember($borrowernumber);
+            $patron->delete;
         }
         $template->param(
             do_delete => '1',