Bugzilla – Attachment 56415 Details for
Bug 16891
Move MoveMemberToDeleted to Koha::Patron->move_to_deleted
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] Bug 16891: Move C4::Members::MoveMemberToDeleted to Koha::Patron->move_to_deleted
PASSED-QA-Bug-16891-Move-C4MembersMoveMemberToDele.patch (text/plain), 9.07 KB, created by
Martin Renvoize (ashimema)
on 2016-10-13 15:14:01 UTC
(
hide
)
Description:
[PASSED QA] Bug 16891: Move C4::Members::MoveMemberToDeleted to Koha::Patron->move_to_deleted
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2016-10-13 15:14:01 UTC
Size:
9.07 KB
patch
obsolete
>From b57d64a9594331d11f6520e4f9110f9cb62e20e3 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Sat, 9 Jul 2016 16:03:11 +0100 >Subject: [PATCH] [PASSED QA] 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 <tomascohen@theke.io> >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 <martin.renvoize@ptfs-europe.com> >--- > 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 4b2d7ff..a4d4d77 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -108,7 +108,6 @@ BEGIN { > push @EXPORT, qw( > &AddMember > &AddMember_Opac >- &MoveMemberToDeleted > ); > > #Check data >@@ -1353,29 +1352,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 b5449a3..da343fa 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 1f901e5..7ab61bc 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 58b2311..eea8f84 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 f18e207..09f3386 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 8ab9069..f0b0cb8 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 1b7e0f8..ceb5722 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.1.4
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=56415">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16891
:
53249
|
53522
|
54178
|
56339
| 56415 |
56418