Bugzilla – Attachment 53524 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]
Bug 16907: Koha::Patrons - Move HandleDelBorrower to ->delete
Bug-16907-KohaPatrons---Move-HandleDelBorrower-to-.patch (text/plain), 8.51 KB, created by
Jonathan Druart
on 2016-07-20 14:14:40 UTC
(
hide
)
Description:
Bug 16907: Koha::Patrons - Move HandleDelBorrower to ->delete
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2016-07-20 14:14:40 UTC
Size:
8.51 KB
patch
obsolete
>From 0514ddb999f0e2183e7056d9c04703b5d0dff471 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] 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. >--- > C4/Members.pm | 34 ---------------------------------- > Koha/Patron.pm | 22 +++++++++++++++++++++- > members/deletemem.pl | 1 - > misc/cronjobs/delete_patrons.pl | 18 +----------------- > t/db_dependent/Koha/Patrons.t | 14 ++++++++++++-- > tools/cleanborrowers.pl | 1 - > 6 files changed, 34 insertions(+), 56 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index 7d482cd..a4f2530 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -1515,40 +1515,6 @@ sub GetSortDetails { > return ($sortvalue) unless ($lib); > } > >-=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 ExtendMemberSubscriptionTo (OUEST-PROVENCE) > > $date = ExtendMemberSubscriptionTo($borrowerid, $date); >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index b03e13d..513076c 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 6974169..fd1c05b 100755 >--- a/members/deletemem.pl >+++ b/members/deletemem.pl >@@ -149,7 +149,6 @@ output_html_with_http_headers $input, $cookie, $template->output; > } else { > my $patron = Koha::Patrons->find( $member ); > $patron->move_to_deleted; >- C4::Members::HandleDelBorrower($member); > $patron->delete; > 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 82a975f..4464670 100755 >--- a/misc/cronjobs/delete_patrons.pl >+++ b/misc/cronjobs/delete_patrons.pl >@@ -55,11 +55,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}... " >@@ -77,26 +72,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 e50bdaa..3612536 100644 >--- a/t/db_dependent/Koha/Patrons.t >+++ b/t/db_dependent/Koha/Patrons.t >@@ -26,6 +26,7 @@ use Koha::Holds; > use Koha::Patron; > use Koha::Patrons; > use Koha::Database; >+use Koha::Virtualshelves; > > use t::lib::TestBuilder; > use t::lib::Mocks; >@@ -142,7 +143,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} ); >@@ -151,12 +152,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 563f5c2..3ccafb3 100755 >--- a/tools/cleanborrowers.pl >+++ b/tools/cleanborrowers.pl >@@ -131,7 +131,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.8.1
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