From 253a3cb0d20dbf6ccffa7b94dce24fbdb09db3f4 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 11 Jul 2016 21:18:56 +0100 Subject: [PATCH] Bug 16907: Koha::Patrons - Move DelMember to ->delete This patch moves the C4::Members::DelMember subroutine to the Koha::Patron module. The delete method must be overwritten to permit handling of patron's holds. Test plan: (With the 2 patches applied) 1/ Create a patron with holds and owner of lists 2/ Delete patrons using the web interface: - More > Delete on a patron page - Batch patron deletion tools 3/ and the cronjob script - perl misc/cronjobs/delete_patrons.pl -c [more options] The patron should have been moved to the deletedborrowers table, his/her holds and lists should have been deleted. --- C4/Members.pm | 38 ++-------------------- Koha/Patron.pm | 27 +++++++++++++++ members/deletemem.pl | 2 +- misc/cronjobs/delete_expired_opac_registrations.pl | 1 - misc/cronjobs/delete_patrons.pl | 17 +++++----- .../delete_unverified_opac_registrations.pl | 1 - t/db_dependent/Auth_with_ldap.t | 4 ++- t/db_dependent/Koha/Patrons.t | 27 +++++++++++++-- t/db_dependent/Members.t | 19 ++--------- tools/cleanborrowers.pl | 5 +-- 10 files changed, 70 insertions(+), 71 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 31bc04d..02d96b0 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -105,11 +105,6 @@ BEGIN { &changepassword ); - #Delete data - push @EXPORT, qw( - &DelMember - ); - #Insert data push @EXPORT, qw( &AddMember @@ -1623,40 +1618,11 @@ sub GetSortDetails { return ($sortvalue) unless ($lib); } -=head2 DelMember - - DelMember($borrowernumber); - -This function remove directly a borrower whitout writing it on deleteborrower. -+ Deletes reserves for the borrower - -=cut - -sub DelMember { - my $dbh = C4::Context->dbh; - my $borrowernumber = shift; - #warn "in delmember with $borrowernumber"; - return unless $borrowernumber; # borrowernumber is mandatory. - # Delete Patron's holds - my @holds = Koha::Holds->search({ borrowernumber => $borrowernumber }); - $_->delete for @holds; - - my $query = " - DELETE - FROM borrowers - WHERE borrowernumber = ? - "; - my $sth = $dbh->prepare($query); - $sth->execute($borrowernumber); - logaction("MEMBERS", "DELETE", $borrowernumber, "") if C4::Context->preference("BorrowersLog"); - return $sth->rows; -} - =head2 HandleDelBorrower HandleDelBorrower($borrower); -When a member is deleted (DelMember in Members.pm), you should call me first. +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. @@ -2225,7 +2191,7 @@ WHERE categorycode = ? AND DATEDIFF( NOW(), dateenrolled ) > ? |; $sth->execute( $category_code, $delay ); my $cnt=0; while ( my ($borrowernumber) = $sth->fetchrow_array() ) { - DelMember($borrowernumber); + Koha::Patrons->find($borrowernumber)->delete; $cnt++; } return $cnt; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 8a6eb09..8b9296a 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -23,7 +23,9 @@ use Modern::Perl; use Carp; use C4::Context; +use C4::Log; use Koha::Database; +use Koha::Holds; use Koha::Issues; use Koha::OldIssues; use Koha::Patron::Categories; @@ -42,6 +44,31 @@ Koha::Patron - Koha Patron Object class =cut +=head3 delete + +$patron->delete + +Delete a patron. + +=cut + +sub delete { + my ($self) = @_; + + my $deleted; + $self->_result->result_source->schema->txn_do( + sub { + # Delete Patron's holds + # FIXME Should be $patron->get_holds + $_->delete for Koha::Holds->search( { borrowernumber => $self->borrowernumber } ); + + logaction( "MEMBERS", "DELETE", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); + $deleted = $self->SUPER::delete; + } + ); + return $deleted; +} + =head3 guarantor Returns a Koha::Patron object for this patron's guarantor diff --git a/members/deletemem.pl b/members/deletemem.pl index 3b9d1fb..6974169 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -150,7 +150,7 @@ output_html_with_http_headers $input, $cookie, $template->output; my $patron = Koha::Patrons->find( $member ); $patron->move_to_deleted; C4::Members::HandleDelBorrower($member); - DelMember($member); + $patron->delete; print $input->redirect("/cgi-bin/koha/members/members-home.pl"); } diff --git a/misc/cronjobs/delete_expired_opac_registrations.pl b/misc/cronjobs/delete_expired_opac_registrations.pl index 0f75a0f..66f9e16 100755 --- a/misc/cronjobs/delete_expired_opac_registrations.pl +++ b/misc/cronjobs/delete_expired_opac_registrations.pl @@ -29,7 +29,6 @@ BEGIN { } use C4::Context; -use C4::Members qw/ DelMember /; my $help; my $confirm; diff --git a/misc/cronjobs/delete_patrons.pl b/misc/cronjobs/delete_patrons.pl index 4e991c1..82a975f 100755 --- a/misc/cronjobs/delete_patrons.pl +++ b/misc/cronjobs/delete_patrons.pl @@ -73,9 +73,8 @@ for my $member (@$members) { } if ( $confirm ) { - my $deleted = eval { - Koha::Patrons->find( $borrowernumber )->move_to_deleted; - }; + my $patron = Koha::Patrons->find( $borrowernumber ); + my $deleted = eval { $patron->move_to_deleted; }; if ($@ or not $deleted) { say "Failed to delete patron $borrowernumber, cannot move it" . ( $@ ? ": ($@)" : "" ); $dbh->rollback; @@ -90,12 +89,12 @@ for my $member (@$members) { $dbh->rollback; next; } - } - eval { C4::Members::DelMember( $borrowernumber ) if $confirm; }; - if ($@) { - say "Failed to delete patron $borrowernumber: $@)"; - $dbh->rollback; - next; + eval { $patron->delete if $confirm; }; + if ($@) { + say "Failed to delete patron $borrowernumber: $@)"; + $dbh->rollback; + next; + } } $dbh->commit; $deleted++; diff --git a/misc/cronjobs/delete_unverified_opac_registrations.pl b/misc/cronjobs/delete_unverified_opac_registrations.pl index c31fa47..13d5c4b 100755 --- a/misc/cronjobs/delete_unverified_opac_registrations.pl +++ b/misc/cronjobs/delete_unverified_opac_registrations.pl @@ -29,7 +29,6 @@ BEGIN { } use C4::Context; -use C4::Members qw/ DelMember /; my $help; my $confirm; diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t index c925c70..6e20c69 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/t/db_dependent/Auth_with_ldap.t @@ -25,6 +25,8 @@ use Test::Warn; use C4::Context; +use Koha::Patrons; + my $dbh = C4::Context->dbh; # Start transaction @@ -179,7 +181,7 @@ subtest 'checkpw_ldap tests' => sub { $update = 0; $desired_count_result = 0; # user auth problem - C4::Members::DelMember( $borrower->{borrowernumber} ); + Koha::Patrons->find( $borrower->{borrowernumber} )->delete; reload_ldap_module(); is( C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', password => 'hey' ), diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 50181ca..8c0e590 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,14 +19,15 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 7; -use C4::Members; +use Koha::Holds; use Koha::Patron; use Koha::Patrons; use Koha::Database; use t::lib::TestBuilder; +use t::lib::Mocks; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; @@ -109,7 +110,27 @@ subtest "move_to_deleted" => sub { ->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->delete( $patron->{borrowernumber} ); # Cleanup +}; + +subtest "delete" => sub { + plan tests => 4; + t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + my $patron = $builder->build( { source => 'Borrower' } ); + my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} ); + my $hold = $builder->build( + { source => 'Reserve', + value => { borrowernumber => $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::Holds->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's holds| ); + + 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' ); }; $retrieved_patron_1->delete; diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index 36d491a..04a8a91 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 77; +use Test::More tests => 74; use Test::MockModule; use Data::Dumper; use C4::Context; @@ -154,21 +154,6 @@ ModMember(borrowernumber => $member->{'borrowernumber'}, dateexpiry => '2001-01- $member = GetMemberDetails($member->{'borrowernumber'}); ok($member->{is_expired}, "GetMemberDetails() indicates that patron is expired"); -# Create a reserve for the patron -$builder->build({ - source => 'Reserve', - value => { - borrowernumber => $member->{ borrowernumber } - } -}); -is( Koha::Holds->search({ borrowernumber => $member->{borrowernumber} })->count, - 1, 'Hold created correctly' ); -DelMember($member->{borrowernumber}); -my $borrower = GetMember( cardnumber => $CARDNUMBER ); -is( $borrower, undef, 'DelMember should remove the patron' ); -is( Koha::Holds->search({ borrowernumber => $member->{borrowernumber} })->count, - 0, 'Hold deleted correctly' ); - # Check_Userid tests %data = ( cardnumber => "123456789", @@ -192,7 +177,7 @@ is( Check_Userid( 'tomasito.none', '' ), 0, is( Check_Userid( 'tomasitoxxx', '' ), 1, 'non-existent userid -> unique (blank borrowernumber)' ); -$borrower = GetMember( borrowernumber => $borrowernumber ); +my $borrower = GetMember( borrowernumber => $borrowernumber ); is( $borrower->{dateofbirth}, undef, 'AddMember should undef dateofbirth if empty string is given'); is( $borrower->{debarred}, undef, 'AddMember should undef debarred if empty string is given'); isnt( $borrower->{dateexpiry}, '0000-00-00', 'AddMember should not set dateexpiry to 0000-00-00 if empty string is given'); diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl index f37a28e..563f5c2 100755 --- a/tools/cleanborrowers.pl +++ b/tools/cleanborrowers.pl @@ -129,9 +129,10 @@ elsif ( $step == 3 ) { for ( my $i = 0 ; $i < $totalDel ; $i++ ) { $radio eq 'testrun' && last; my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'}; - $radio eq 'trash' && Koha::Patrons->find($borrowernumber)->move_to_deleted; + my $patron = Koha::Patrons->find($borrowernumber); + $radio eq 'trash' && $patron->move_to_deleted; C4::Members::HandleDelBorrower($borrowernumber); - DelMember($borrowernumber); + $patron->delete; } $template->param( do_delete => '1', -- 2.8.1