From 4adbe86c528ddb92c3cc506c687e0ff0cb8a9206 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Jul 2016 21:35:15 +0100 Subject: [PATCH] [PASSED QA] 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 --- 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 b183f39..702e36d 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 14a0ccf..6692348 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 d3c9fb5..b51c721 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 2f9c822..ee015a6 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 ed12899..a699411 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 b483514..a1c2135 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.1.4