From c4a923bdf7eacce4de1efd4c5bdd2fe4010ad4b1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Jul 2016 21:35:15 +0100 Subject: [PATCH] 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 Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- 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(-) diff --git a/C4/Members.pm b/C4/Members.pm index b183f39897..702e36d7f4 100644 --- a/C4/Members.pm +++ b/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); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 14a0ccf4db..6692348d87 100644 --- a/Koha/Patron.pm +++ b/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; } diff --git a/members/deletemem.pl b/members/deletemem.pl index d3c9fb5dd4..b51c721f2f 100755 --- a/members/deletemem.pl +++ b/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"); diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl index 2f9c82250b..ee015a6856 100755 --- a/misc/cronjobs/delete_patrons.pl +++ b/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; } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index ed12899145..a69941185f 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/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' ); }; diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index b4835144a8..a1c2135545 100755 --- a/tools/cleanborrowers.pl +++ b/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( -- 2.39.5