Bugzilla – Attachment 56419 Details for
Bug 16907
Move DelMember and HandleDelBorrower to Koha::Patron
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] Bug 16907: Koha::Patrons - Move DelMember to ->delete
PASSED-QA-Bug-16907-KohaPatrons---Move-DelMember-t.patch (text/plain), 12.25 KB, created by
Martin Renvoize (ashimema)
on 2016-10-13 15:36:20 UTC
(
hide
)
Description:
[PASSED QA] Bug 16907: Koha::Patrons - Move DelMember to ->delete
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2016-10-13 15:36:20 UTC
Size:
12.25 KB
patch
obsolete
>From d407b7053b8bd5d99465e0a67b5e3f821fe4c801 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Mon, 11 Jul 2016 21:18:56 +0100 >Subject: [PATCH] [PASSED QA] 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. > >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > C4/Members.pm | 38 ++-------------------- > Koha/Patron.pm | 26 +++++++++++++++ > 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, 68 insertions(+), 72 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index 22cb083..b183f39 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -98,11 +98,6 @@ BEGIN { > &changepassword > ); > >- #Delete data >- push @EXPORT, qw( >- &DelMember >- ); >- > #Insert data > push @EXPORT, qw( > &AddMember >@@ -1308,40 +1303,11 @@ sub SetAge{ > return $borrower; > } # sub SetAge > >-=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. >@@ -1800,7 +1766,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 da343fa..14a0ccf 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -26,6 +26,7 @@ use C4::Context; > use C4::Log; > use Koha::Database; > use Koha::DateUtils; >+use Koha::Holds; > use Koha::Issues; > use Koha::OldIssues; > use Koha::Patron::Categories; >@@ -44,6 +45,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 7ab61bc..d3c9fb5 100755 >--- a/members/deletemem.pl >+++ b/members/deletemem.pl >@@ -164,7 +164,7 @@ 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); >- DelMember($member); >+ $patron->delete; > # TODO Tell the user everything went ok > print $input->redirect("/cgi-bin/koha/members/members-home.pl"); > exit 0; # Exit without error >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 eea8f84..2f9c822 100755 >--- a/misc/cronjobs/delete_patrons.pl >+++ b/misc/cronjobs/delete_patrons.pl >@@ -78,9 +78,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; >@@ -95,12 +94,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 09f3386..ed12899 100644 >--- a/t/db_dependent/Koha/Patrons.t >+++ b/t/db_dependent/Koha/Patrons.t >@@ -19,13 +19,12 @@ > > use Modern::Perl; > >-use Test::More tests => 9; >+use Test::More tests => 10; > use Test::Warn; > >-use C4::Circulation; >- > use C4::Members; > >+use Koha::Holds; > use Koha::Patron; > use Koha::Patrons; > use Koha::Database; >@@ -220,7 +219,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 f0b0cb8..963d790 100755 >--- a/t/db_dependent/Members.t >+++ b/t/db_dependent/Members.t >@@ -17,7 +17,7 @@ > > use Modern::Perl; > >-use Test::More tests => 82; >+use Test::More tests => 79; > 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 ceb5722..b483514 100755 >--- a/tools/cleanborrowers.pl >+++ b/tools/cleanborrowers.pl >@@ -147,9 +147,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.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16907
:
53327
|
53328
|
53523
|
53524
|
54184
|
54401
|
54402
|
54403
|
56342
|
56343
|
56344
| 56419 |
56420
|
56421