@@ -, +, @@ to ->delete --- C4/Members.pm | 34 ---------------------------------- Koha/Patron.pm | 22 +++++++++++++++++++++- members/deletemem.pl | 1 - misc/cronjobs/delete_patrons.pl | 18 +----------------- t/db_dependent/Koha/Patrons.t | 13 +++++++++++-- tools/cleanborrowers.pl | 1 - 6 files changed, 33 insertions(+), 56 deletions(-) --- a/C4/Members.pm +++ a/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); --- a/Koha/Patron.pm +++ a/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; } --- a/members/deletemem.pl +++ a/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"); --- a/misc/cronjobs/delete_patrons.pl +++ a/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; } --- a/t/db_dependent/Koha/Patrons.t +++ a/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' ); }; --- a/tools/cleanborrowers.pl +++ a/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( --