From 0a586b9dbf1f517efae203871fe717a72c93c586 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 --- C4/Members.pm | 24 ------------------------ Koha/Patron.pm | 15 +++++++++++++++ members/deletemem.pl | 4 +++- misc/cronjobs/delete_patrons.pl | 36 +++++++++++++++++++----------------- t/db_dependent/Koha/Patrons.t | 16 +++++++++++++++- t/db_dependent/Members.t | 16 +--------------- tools/cleanborrowers.pl | 3 ++- 7 files changed, 55 insertions(+), 59 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 061172f..efd07ff 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -112,7 +112,6 @@ BEGIN { push @EXPORT, qw( &AddMember &AddMember_Opac - &MoveMemberToDeleted &ExtendMemberSubscriptionTo ); @@ -1520,29 +1519,6 @@ sub GetSortDetails { return ($sortvalue) unless ($lib); } -=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 42b066b..4089c8c 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -208,6 +208,21 @@ sub update_password { return 1; } +=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 fd53ecc..3b9d1fb 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -31,6 +31,7 @@ use C4::Auth; use C4::Members; use C4::Branch; # GetBranches use Module::Load; +use Koha::Patrons; use Koha::Patron::Images; if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) { load Koha::NorwegianPatronDB, qw( NLMarkForDeletion NLSync ); @@ -146,7 +147,8 @@ if ($countissues > 0 or $flags->{'CHARGES'} or $data->{'borrowernumber'} or $de output_html_with_http_headers $input, $cookie, $template->output; } else { - MoveMemberToDeleted($member); + my $patron = Koha::Patrons->find( $member ); + $patron->move_to_deleted; C4::Members::HandleDelBorrower($member); DelMember($member); 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 a76d1dd..4e991c1 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, $category_code, @@ -71,23 +72,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 980bf84..0b18270 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,9 +19,10 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 7; use Test::Warn; +use C4::Members; use Koha::Patron; use Koha::Patrons; use Koha::Database; @@ -127,6 +128,19 @@ subtest 'update_password' => sub { is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should not have logged' ); }; +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 006207b..36d491a 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 79; +use Test::More tests => 77; 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 d108d53..f37a28e 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -40,6 +40,7 @@ use C4::Output; use C4::Members; # GetBorrowersWhoHavexxxBorrowed. use C4::Circulation; # AnonymiseIssueHistory. use Koha::DateUtils qw( dt_from_string output_pref ); +use Koha::Patrons; use Date::Calc qw/Today Add_Delta_YM/; use Koha::List::Patron; @@ -128,7 +129,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.9.2