Bugzilla – Attachment 56420 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 HandleDelBorrower to ->delete
PASSED-QA-Bug-16907-KohaPatrons---Move-HandleDelBo.patch (text/plain), 8.51 KB, created by
Martin Renvoize (ashimema)
on 2016-10-13 15:36:34 UTC
(
hide
)
Description:
[PASSED QA] Bug 16907: Koha::Patrons - Move HandleDelBorrower to ->delete
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2016-10-13 15:36:34 UTC
Size:
8.51 KB
patch
obsolete
>From 4adbe86c528ddb92c3cc506c687e0ff0cb8a9206 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Mon, 11 Jul 2016 21:35:15 +0100 >Subject: [PATCH] [PASSED QA] Bug 16907: Koha::Patrons - Move HandleDelBorrower > to ->delete > >This job should be done each time patron data are deleted. It's better >to do it just before deleting the patron than assuming the caller did >the job by itself. > >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > C4/Members.pm | 34 ---------------------------------- > Koha/Patron.pm | 22 +++++++++++++++++++++- > members/deletemem.pl | 1 - > misc/cronjobs/delete_patrons.pl | 18 +----------------- > t/db_dependent/Koha/Patrons.t | 13 +++++++++++-- > tools/cleanborrowers.pl | 1 - > 6 files changed, 33 insertions(+), 56 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index b183f39..702e36d 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -1303,40 +1303,6 @@ sub SetAge{ > return $borrower; > } # sub SetAge > >-=head2 HandleDelBorrower >- >- HandleDelBorrower($borrower); >- >-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. >- >-=cut >- >-sub HandleDelBorrower { >- my ($borrower)= @_; >- my $query; >- my $dbh = C4::Context->dbh; >- >- #Delete all lists and all shares of this borrower >- #Consistent with the approach Koha uses on deleting individual lists >- #Note that entries in virtualshelfcontents added by this borrower to >- #lists of others will be handled by a table constraint: the borrower >- #is set to NULL in those entries. >- $query="DELETE FROM virtualshelves WHERE owner=?"; >- $dbh->do($query,undef,($borrower)); >- >- #NOTE: >- #We could handle the above deletes via a constraint too. >- #But a new BZ report 11889 has been opened to discuss another approach. >- #Instead of deleting we could also disown lists (based on a pref). >- #In that way we could save shared and public lists. >- #The current table constraints support that idea now. >- #This pref should then govern the results of other routines/methods such as >- #Koha::Virtualshelf->new->delete too. >-} >- > =head2 GetHideLostItemsPreference > > $hidelostitemspref = &GetHideLostItemsPreference($borrowernumber); >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index 14a0ccf..6692348 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -32,6 +32,7 @@ use Koha::OldIssues; > use Koha::Patron::Categories; > use Koha::Patron::Images; > use Koha::Patrons; >+use Koha::Virtualshelves; > > use base qw(Koha::Object); > >@@ -49,7 +50,10 @@ Koha::Patron - Koha Patron Object class > > $patron->delete > >-Delete a patron. >+Delete patron's holds, lists and finally the patron. >+ >+Lists owned by the borrower are deleted, but entries from the borrower to >+other lists are kept. > > =cut > >@@ -63,6 +67,22 @@ sub delete { > # FIXME Should be $patron->get_holds > $_->delete for Koha::Holds->search( { borrowernumber => $self->borrowernumber } ); > >+ # Delete all lists and all shares of this borrower >+ # Consistent with the approach Koha uses on deleting individual lists >+ # Note that entries in virtualshelfcontents added by this borrower to >+ # lists of others will be handled by a table constraint: the borrower >+ # is set to NULL in those entries. >+ # NOTE: >+ # We could handle the above deletes via a constraint too. >+ # But a new BZ report 11889 has been opened to discuss another approach. >+ # Instead of deleting we could also disown lists (based on a pref). >+ # In that way we could save shared and public lists. >+ # The current table constraints support that idea now. >+ # This pref should then govern the results of other routines/methods such as >+ # Koha::Virtualshelf->new->delete too. >+ # FIXME Could be $patron->get_lists >+ $_->delete for Koha::Virtualshelves->search( { owner => $self->borrowernumber } ); >+ > logaction( "MEMBERS", "DELETE", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); > $deleted = $self->SUPER::delete; > } >diff --git a/members/deletemem.pl b/members/deletemem.pl >index d3c9fb5..b51c721 100755 >--- a/members/deletemem.pl >+++ b/members/deletemem.pl >@@ -163,7 +163,6 @@ 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); > $patron->delete; > # TODO Tell the user everything went ok > 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 2f9c822..ee015a6 100755 >--- a/misc/cronjobs/delete_patrons.pl >+++ b/misc/cronjobs/delete_patrons.pl >@@ -60,11 +60,6 @@ unless ($confirm) { > > say scalar(@$members) . " patrons to delete"; > >-my $dbh = C4::Context->dbh; >-$dbh->{RaiseError} = 1; >-$dbh->{PrintError} = 0; >- >-$dbh->{AutoCommit} = 0; # use transactions to avoid partial deletes > my $deleted = 0; > for my $member (@$members) { > print "Trying to delete patron $member->{borrowernumber}... " >@@ -82,26 +77,15 @@ for my $member (@$members) { > my $deleted = eval { $patron->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 { $patron->delete if $confirm; }; >+ eval { $patron->delete }; > if ($@) { > say "Failed to delete patron $borrowernumber: $@)"; >- $dbh->rollback; > next; > } > } >- $dbh->commit; > $deleted++; > say "OK" if $verbose; > } >diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t >index ed12899..a699411 100644 >--- a/t/db_dependent/Koha/Patrons.t >+++ b/t/db_dependent/Koha/Patrons.t >@@ -223,7 +223,7 @@ subtest "move_to_deleted" => sub { > }; > > subtest "delete" => sub { >- plan tests => 4; >+ plan tests => 5; > t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); > my $patron = $builder->build( { source => 'Borrower' } ); > my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} ); >@@ -232,12 +232,21 @@ subtest "delete" => sub { > value => { borrowernumber => $patron->{borrowernumber} } > } > ); >+ my $list = $builder->build( >+ { source => 'Virtualshelve', >+ value => { owner => $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::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| ); > >+ is( Koha::Virtualshelves->search( { owner => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's lists| ); >+ > 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' ); > }; >diff --git a/tools/cleanborrowers.pl b/tools/cleanborrowers.pl >index b483514..a1c2135 100755 >--- a/tools/cleanborrowers.pl >+++ b/tools/cleanborrowers.pl >@@ -149,7 +149,6 @@ elsif ( $step == 3 ) { > my $borrowernumber = $patrons_to_delete->[$i]->{'borrowernumber'}; > my $patron = Koha::Patrons->find($borrowernumber); > $radio eq 'trash' && $patron->move_to_deleted; >- C4::Members::HandleDelBorrower($borrowernumber); > $patron->delete; > } > $template->param( >-- >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