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

This job should be done each time patron data are deleted. It's better
to do it just before deleting the patron than assuming the caller did
the job by itself.

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>

C4/Members.pm

@@ -1303,40 +1303,6 @@ sub SetAge{
     return $borrower;
 }    # sub SetAge
 
-=head2 HandleDelBorrower
-
-     HandleDelBorrower($borrower);
-
-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.
-
-=cut
-
-sub HandleDelBorrower {
-    my ($borrower)= @_;
-    my $query;
-    my $dbh = C4::Context->dbh;
-
-    #Delete all lists and all shares of this borrower
-    #Consistent with the approach Koha uses on deleting individual lists
-    #Note that entries in virtualshelfcontents added by this borrower to
-    #lists of others will be handled by a table constraint: the borrower
-    #is set to NULL in those entries.
-    $query="DELETE FROM virtualshelves WHERE owner=?";
-    $dbh->do($query,undef,($borrower));
-
-    #NOTE:
-    #We could handle the above deletes via a constraint too.
-    #But a new BZ report 11889 has been opened to discuss another approach.
-    #Instead of deleting we could also disown lists (based on a pref).
-    #In that way we could save shared and public lists.
-    #The current table constraints support that idea now.
-    #This pref should then govern the results of other routines/methods such as
-    #Koha::Virtualshelf->new->delete too.
-}
-
 =head2 GetHideLostItemsPreference
 
   $hidelostitemspref = &GetHideLostItemsPreference($borrowernumber);

Koha/Patron.pm

@@ -32,6 +32,7 @@ use Koha::OldIssues;
 use Koha::Patron::Categories;
 use Koha::Patron::Images;
 use Koha::Patrons;
+use Koha::Virtualshelves;
 
 use base qw(Koha::Object);
 
@@ -49,7 +50,10 @@ Koha::Patron - Koha Patron Object class
 
 $patron->delete
 
-Delete a patron.
+Delete patron's holds, lists and finally the patron.
+
+Lists owned by the borrower are deleted, but entries from the borrower to
+other lists are kept.
 
 =cut
 
@@ -63,6 +67,22 @@ sub delete {
             # FIXME Should be $patron->get_holds
             $_->delete for Koha::Holds->search( { borrowernumber => $self->borrowernumber } );
 
+            # Delete all lists and all shares of this borrower
+            # Consistent with the approach Koha uses on deleting individual lists
+            # Note that entries in virtualshelfcontents added by this borrower to
+            # lists of others will be handled by a table constraint: the borrower
+            # is set to NULL in those entries.
+            # NOTE:
+            # We could handle the above deletes via a constraint too.
+            # But a new BZ report 11889 has been opened to discuss another approach.
+            # Instead of deleting we could also disown lists (based on a pref).
+            # In that way we could save shared and public lists.
+            # The current table constraints support that idea now.
+            # This pref should then govern the results of other routines/methods such as
+            # Koha::Virtualshelf->new->delete too.
+            # FIXME Could be $patron->get_lists
+            $_->delete for Koha::Virtualshelves->search( { owner => $self->borrowernumber } );
+
             logaction( "MEMBERS", "DELETE", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog");
             $deleted = $self->SUPER::delete;
         }

members/deletemem.pl

@@ -163,7 +163,6 @@ 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);
     $patron->delete;
     # TODO Tell the user everything went ok
     print $input->redirect("/cgi-bin/koha/members/members-home.pl");

misc/cronjobs/delete_patrons.pl

@@ -60,11 +60,6 @@ unless ($confirm) {
 
 say scalar(@$members) . " patrons to delete";
 
-my $dbh = C4::Context->dbh;
-$dbh->{RaiseError} = 1;
-$dbh->{PrintError} = 0;
-
-$dbh->{AutoCommit} = 0; # use transactions to avoid partial deletes
 my $deleted = 0;
 for my $member (@$members) {
     print "Trying to delete patron $member->{borrowernumber}... "
@@ -82,26 +77,15 @@ for my $member (@$members) {
         my $deleted = eval { $patron->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 { $patron->delete if $confirm; };
+        eval { $patron->delete };
         if ($@) {
             say "Failed to delete patron $borrowernumber: $@)";
-            $dbh->rollback;
             next;
         }
     }
-    $dbh->commit;
     $deleted++;
     say "OK" if $verbose;
 }

t/db_dependent/Koha/Patrons.t

@@ -223,7 +223,7 @@ subtest "move_to_deleted" => sub {
 };
 
 subtest "delete" => sub {
-    plan tests => 4;
+    plan tests => 5;
     t::lib::Mocks::mock_preference( 'BorrowersLog', 1 );
     my $patron           = $builder->build( { source => 'Borrower' } );
     my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} );
@@ -232,12 +232,21 @@ subtest "delete" => sub {
             value  => { borrowernumber => $patron->{borrowernumber} }
         }
     );
+    my $list = $builder->build(
+        {   source => 'Virtualshelve',
+            value  => { owner => $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::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| );
 
+    is( Koha::Virtualshelves->search( { owner => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's lists| );
+
     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' );
 };

tools/cleanborrowers.pl

@@ -149,7 +149,6 @@ elsif ( $step == 3 ) {
             my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'};
             my $patron = Koha::Patrons->find($borrowernumber);
             $radio eq 'trash' && $patron->move_to_deleted;
-            C4::Members::HandleDelBorrower($borrowernumber);
             $patron->delete;
         }
         $template->param(