From 20c44f005138d2f585eeb1336a93920629b52d3c Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Sat, 9 Jul 2016 16:03:11 +0100 Subject: [PATCH] Bug 16891: Move C4::Members::MoveMemberToDeleted to Koha::Patron->move_to_deleted This patch removes the C4::Members::MoveMemberToDeleted subroutine in order to replace it with the Koha::Patron->move_to_deleted method. Next after this change, we will move C4::Members::HandleDelBorrower and C4::Members::DelMember to the same module to simplify the code in members/deletemem.pl and misc/cronjobs/delete_patrons.pl Test plan: 1/ Delete a patron from the staff interface and make sure (s)he has been moved to the deletedborrowers table. 2/ Use the "Batch patron deletion" tool (tools/cleanborrowers.pl) to remove patron. Make sure the "Permanently delete these patrons" and "Move these patrons to the trash" options work as before 3/ Same as previously but using the cronjob misc/cronjobs/delete_patrons.pl. Signed-off-by: Tomas Cohen Arazi Tested the delete_patrons.pl script and cleanborrowers.pl too. Tests (are relevant and) pass and the qa scripts are happy too :-D Signed-off-by: Martin Renvoize --- C4/Members.pm | 24 ---------------------- Koha/Patron.pm | 15 ++++++++++++++ members/deletemem.pl | 4 +++- misc/cronjobs/delete_patrons.pl | 36 +++++++++++++++++---------------- t/db_dependent/Koha/Patrons.t | 18 ++++++++++++++++- t/db_dependent/Members.t | 16 +-------------- tools/cleanborrowers.pl | 3 ++- 7 files changed, 57 insertions(+), 59 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 4f004535ca..22cb083f32 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -107,7 +107,6 @@ BEGIN { push @EXPORT, qw( &AddMember &AddMember_Opac - &MoveMemberToDeleted ); #Check data @@ -1309,29 +1308,6 @@ sub SetAge{ return $borrower; } # sub SetAge -=head2 MoveMemberToDeleted - - $result = &MoveMemberToDeleted($borrowernumber); - -Copy the record from borrowers to deletedborrowers table. -The routine returns 1 for success, undef for failure. - -=cut - -sub MoveMemberToDeleted { - my ($member) = shift or return; - - my $schema = Koha::Database->new()->schema(); - my $borrowers_rs = $schema->resultset('Borrower'); - $borrowers_rs->result_class('DBIx::Class::ResultClass::HashRefInflator'); - my $borrower = $borrowers_rs->find($member); - return unless $borrower; - - my $deleted = $schema->resultset('Deletedborrower')->create($borrower); - - return $deleted ? 1 : undef; -} - =head2 DelMember DelMember($borrowernumber); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index b5449a30a3..da343fa67d 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -267,6 +267,21 @@ sub track_login { $self->lastseen( dt_from_string() )->store; } +=head2 move_to_deleted + +my $is_moved = $patron->move_to_deleted; + +Move a patron to the deletedborrowers table. +This can be done before deleting a patron, to make sure the data are not completely deleted. + +=cut + +sub move_to_deleted { + my ($self) = @_; + my $patron_infos = $self->unblessed; + return Koha::Database->new->schema->resultset('Deletedborrower')->create($patron_infos); +} + =head3 type =cut diff --git a/members/deletemem.pl b/members/deletemem.pl index 1f901e5add..7ab61bc4e5 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -31,6 +31,7 @@ use C4::Output; use C4::Auth; use C4::Members; use Module::Load; +use Koha::Patrons; use Koha::Patron::Images; use Koha::Token; @@ -160,7 +161,8 @@ if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'} or $is_ secret => md5_base64( C4::Context->config('pass') ), token => scalar $input->param('csrf_token'), }); - MoveMemberToDeleted($member); + my $patron = Koha::Patrons->find( $member ); + $patron->move_to_deleted; C4::Members::HandleDelBorrower($member); DelMember($member); # TODO Tell the user everything went ok diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl index 58b231125b..eea8f84280 100755 --- a/misc/cronjobs/delete_patrons.pl +++ b/misc/cronjobs/delete_patrons.pl @@ -7,6 +7,7 @@ use Getopt::Long; use C4::Members; use Koha::DateUtils; +use Koha::Patrons; use C4::Log; my ( $help, $verbose, $not_borrowed_since, $expired_before, $last_seen, @@ -76,23 +77,24 @@ for my $member (@$members) { next; } - eval { - C4::Members::MoveMemberToDeleted( $borrowernumber ) - if $confirm; - }; - if ($@) { - say "Failed to delete patron $borrowernumber, cannot move it: ($@)"; - $dbh->rollback; - next; - } - eval { - C4::Members::HandleDelBorrower( $borrowernumber ) - if $confirm; - }; - if ($@) { - say "Failed to delete patron $borrowernumber, error handling its lists: ($@)"; - $dbh->rollback; - next; + if ( $confirm ) { + my $deleted = eval { + Koha::Patrons->find( $borrowernumber )->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 { C4::Members::DelMember( $borrowernumber ) if $confirm; }; if ($@) { diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index f18e207e65..09f338655c 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,10 +19,13 @@ use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use Test::Warn; use C4::Circulation; + +use C4::Members; + use Koha::Patron; use Koha::Patrons; use Koha::Database; @@ -207,6 +210,19 @@ subtest 'renew_account' => sub { $retrieved_patron->delete; }; +subtest "move_to_deleted" => sub { + plan tests => 2; + my $patron = $builder->build( { source => 'Borrower' } ); + my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} ); + is( ref( $retrieved_patron->move_to_deleted ), 'Koha::Schema::Result::Deletedborrower', 'Koha::Patron->move_to_deleted should return the Deleted patron' ) + ; # FIXME This should be Koha::Deleted::Patron + my $deleted_patron = $schema->resultset('Deletedborrower') + ->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_1->delete; is( Koha::Patrons->search->count, $nb_of_patrons + 1, 'Delete should have deleted the patron' ); diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 8ab9069991..f0b0cb8d7f 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 84; +use Test::More tests => 82; use Test::MockModule; use Data::Dumper; use C4::Context; @@ -95,20 +95,6 @@ testAgeAccessors(\%data); #Age accessor tests don't touch the db so it is safe t my $addmem=AddMember(%data); ok($addmem, "AddMember()"); -# It's not really a Move, it's a Copy. -my $result = MoveMemberToDeleted($addmem); -ok($result,"MoveMemberToDeleted()"); - -my $sth = $dbh->prepare("SELECT * from borrowers WHERE borrowernumber=?"); -$sth->execute($addmem); -my $MemberAdded = $sth->fetchrow_hashref; - -$sth = $dbh->prepare("SELECT * from deletedborrowers WHERE borrowernumber=?"); -$sth->execute($addmem); -my $MemberMoved = $sth->fetchrow_hashref; - -is_deeply($MemberMoved,$MemberAdded,"Confirm MoveMemberToDeleted."); - my $member=GetMemberDetails("",$CARDNUMBER) or BAIL_OUT("Cannot read member with card $CARDNUMBER"); diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index 1b7e0f84d0..ceb5722374 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -40,6 +40,7 @@ use C4::Members; # GetBorrowersWhoHavexxxBorrowed. use C4::Circulation; # AnonymiseIssueHistory. use Koha::DateUtils qw( dt_from_string output_pref ); use Koha::Patron::Categories; +use Koha::Patrons; use Date::Calc qw/Today Add_Delta_YM/; use Koha::List::Patron; @@ -146,7 +147,7 @@ elsif ( $step == 3 ) { for ( my $i = 0 ; $i < $totalDel ; $i++ ) { $radio eq 'testrun' && last; my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'}; - $radio eq 'trash' && MoveMemberToDeleted($borrowernumber); + $radio eq 'trash' && Koha::Patrons->find($borrowernumber)->move_to_deleted; C4::Members::HandleDelBorrower($borrowernumber); DelMember($borrowernumber); } -- 2.39.5