Attachment 53522 Details for Bug 16891 – Bug 16891: Move C4::Members::MoveMemberToDeleted to Koha::Patron->move_to_deleted

[patch] Bug 16891: Move C4::Members::MoveMemberToDeleted to Koha::Patron->move_to_deleted

Bug-16891-Move-C4MembersMoveMemberToDeleted-to-Koh.patch (text/plain), 8.99 KB, created by Jonathan Druart on 2016-07-20 14:09:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2016-07-20 14:09:19 UTC

Size: 8.99 KB

patch

obsolete

>From 839ba08e6e0c3d01b0d063f98fa8ca07b37035c0 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] 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.
>---
> 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 63e3f36..b1c3c83 100644
>--- a/C4/Members.pm
>+++ b/C4/Members.pm
>@@ -113,7 +113,6 @@ BEGIN {
>     push @EXPORT, qw(
>         &AddMember
>         &AddMember_Opac
>-        &MoveMemberToDeleted
>         &ExtendMemberSubscriptionTo
>     );
> 
>@@ -1521,29 +1520,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.8.1

Actions: View | Diff | Splinter Review

Attachments on bug 16891: 53249 | 53522 | 54178 | 56339 | 56415 | 56418