From 7ce42d330bf64fd52193dedeaef2d3ac913f6563 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 9 Jun 2022 07:24:31 +0000 Subject: [PATCH] Bug 11889: (follow-up) Get rid of FIXME in Koha::Patron Instead of get_lists, I added ->virtualshelves. I removed the addition of the suffix to the shelf name here; if we really want that (doubts), we should do it pref based. Removing a staff member with public lists would result in weird names with shelf numbers on the OPAC. (Should be very easy to restore.) Used this opportunity to refactor the patron->delete subtest a bit: switching to build_object, removing the 'retrieved patrons', differentiating between staff patron and patron for sharing, adding a few tests. Test plan: Enable transfer ownership. Delete a patron with lists in interface. Check what happened. Run t/db_dependent/Koha/Patrons.t. Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize --- Koha/Patron.pm | 47 +++++++-------- t/db_dependent/Koha/Patrons.t | 108 +++++++++++++++------------------- 2 files changed, 73 insertions(+), 82 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 36e0a2cedc..edf2054782 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -388,32 +388,22 @@ sub delete { $hold->cancel; } - # FIXME Could be $patron->get_lists - # If ListOwnershipUponPatronDeletion = transfer, change ownership of all - # public and shared lists to the user who deleted them. - if ( C4::Context->preference('ListOwnershipUponPatronDeletion') eq 'transfer' ) { - my $userenv = C4::Context->userenv(); - my $usernumber = (ref($userenv) eq 'HASH') ? $userenv->{'number'} : 0; - my @publiclists = Koha::Virtualshelves->get_public_shelves->as_list; - my @sharedlists = Koha::Virtualshelves->get_shared_shelves({ borrowernumber => $self->borrowernumber })->as_list; - foreach my $plist ( @publiclists ) { - if ( $plist->owner == $self->borrowernumber ) { - my $unique_name = $plist->shelfname . '_' . $self->borrowernumber; - $plist->set({ owner => $usernumber, shelfname => $unique_name })->store; - } - } - foreach my $slist ( @sharedlists ) { - my $unique_name = $slist->shelfname . '_' . $self->borrowernumber; - $slist->set({ owner => $usernumber, shelfname => $unique_name })->store; - # if staff member had a share, remove it - $slist->remove_share( $usernumber ); + # Handle lists (virtualshelves) + my $userenv = C4::Context->userenv; + my $usernumber = ref($userenv) eq 'HASH' ? $userenv->{'number'} : 0; + my $lists = $self->virtualshelves; + while( my $list = $lists->next ) { + # if staff member had a share, remove it + $list->remove_share( $usernumber ) if $usernumber && $list->is_private; + if( C4::Context->preference('ListOwnershipUponPatronDeletion') eq 'transfer' && + $usernumber && ( $list->is_public || $list->is_shared )) + { + $list->set({ owner => $usernumber })->store; # transfer ownership of public/shared list + } else { # delete + $list->delete; } } - # Delete any remaining lists that this user is an owner of (always private lists, - # only public and shared lists if ListOwnershipUponPatronDeletion = delete) - $_->delete for Koha::Virtualshelves->search({ owner => $self->borrowernumber })->as_list; - # We cannot have a FK on borrower_modifications.borrowernumber, the table is also used # for patron selfreg $_->delete for Koha::Patron::Modifications->search( { borrowernumber => $self->borrowernumber } )->as_list; @@ -2196,6 +2186,17 @@ sub decoded_secret { return $self->secret; } +=head3 virtualshelves + + my $shelves = $patron->virtualshelves; + +=cut + +sub virtualshelves { + my $self = shift; + return Koha::Virtualshelves->_new_from_dbic( scalar $self->_result->virtualshelves ); +} + =head2 Internal methods =head3 _type diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index aeb5ce17ab..b7b544c269 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -460,91 +460,81 @@ subtest "move_to_deleted" => sub { }; subtest "delete" => sub { - plan tests => 11; + plan tests => 13; t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); t::lib::Mocks::mock_preference( 'ListOwnershipUponPatronDeletion', 'transfer' ); Koha::Virtualshelves->delete; + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my $patron_for_sharing = $builder->build_object({ class => 'Koha::Patrons' }); my $staff_patron = $builder->build_object({ class => 'Koha::Patrons' }); t::lib::Mocks::mock_userenv({ patron => $staff_patron }); - my $patron = $builder->build( { source => 'Borrower' } ); - my $patron_for_sharing = $staff_patron->borrowernumber; - my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} ); - my $hold = $builder->build( - { source => 'Reserve', - value => { borrowernumber => $patron->{borrowernumber} } - } - ); - my $modification = $builder->build_object({ class => 'Koha::Patron::Modifications', value => { borrowernumber => $patron->{borrowernumber} } }); + my $hold = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->borrowernumber } }); + my $modification = $builder->build_object({ class => 'Koha::Patron::Modifications', value => { borrowernumber => $patron->borrowernumber } }); my $private_list = Koha::Virtualshelf->new({ - shelfname => "private", - owner => $patron->{borrowernumber}, - public => 0, - } - )->store; + shelfname => "private", + owner => $patron->borrowernumber, + public => 0, + })->store; my $public_list = Koha::Virtualshelf->new({ - shelfname => "public", - owner => $patron->{borrowernumber}, - public => 1, - } - )->store; + shelfname => "public", + owner => $patron->borrowernumber, + public => 1, + })->store; my $list_to_share = Koha::Virtualshelf->new({ - shelfname => "shared", - owner => $patron->{borrowernumber}, - public => 0, - } - )->store; + shelfname => "shared", + owner => $patron->borrowernumber, + public => 0, + })->store; - my $shared_shelf = eval { $list_to_share->share("valid key")->accept("valid key", $patron_for_sharing) }; - my $deleted = $retrieved_patron->delete; + $list_to_share->share("valid key")->accept( "valid key", $patron_for_sharing->borrowernumber ); + $list_to_share->share("valid key")->accept( "valid key", $staff_patron->borrowernumber ); # this share should be removed at deletion too + my $deleted = $patron->delete; is( ref($deleted), 'Koha::Patron', 'Koha::Patron->delete should return the deleted patron object if the patron has been correctly deleted' ); + ok( $patron->borrowernumber, 'Still have the deleted borrowernumber' ); - 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::Old::Holds->search( { reserve_id => $hold->{ reserve_id } } )->count, 1, q|Koha::Patron->delete should have cancelled patron's holds| ); + is (Koha::Old::Holds->search({ reserve_id => $hold->reserve_id })->count, 1, q|Koha::Patron->delete should have cancelled patron's holds| ); - is( Koha::Holds->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have cancelled patron's holds 2| ); + is( Koha::Holds->search( { borrowernumber => $patron->borrowernumber } )->count, 0, q|Koha::Patron->delete should have cancelled patron's holds 2| ); - my $transferred_lists = Koha::Virtualshelves->search({ owner => $patron_for_sharing })->count; + my $transferred_lists = Koha::Virtualshelves->search({ owner => $staff_patron->borrowernumber })->count; is( $transferred_lists, 2, 'Public and shared lists should stay in database under a different owner with a unique name, while private lists delete, with ListOwnershipPatronDeletion set to Transfer'); - is( Koha::Virtualshelfshares->search({ borrowernumber => $patron_for_sharing })->count, 0, "New owner of list should have shares removed" ); - is( Koha::Virtualshelves->search({ owner => $patron->{borrowernumber} })->count, 0, q|Koha::Patron->delete should have deleted patron's lists/removed their ownership| ); + is( Koha::Virtualshelfshares->search({ borrowernumber => $staff_patron->borrowernumber })->count, 0, "New owner of list should have shares removed" ); + is( Koha::Virtualshelfshares->search({ borrowernumber => $patron_for_sharing->borrowernumber })->count, 1, "But the other share is still there" ); + is( Koha::Virtualshelves->search({ owner => $patron->borrowernumber })->count, 0, q|Koha::Patron->delete should have deleted patron's lists/removed their ownership| ); - is( Koha::Patron::Modifications->search( { borrowernumber => $patron->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's modifications| ); + is( Koha::Patron::Modifications->search( { borrowernumber => $patron->borrowernumber } )->count, 0, q|Koha::Patron->delete should have deleted patron's modifications| ); - my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'DELETE', object => $retrieved_patron->borrowernumber } )->count; + my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'DELETE', object => $patron->borrowernumber } )->count; is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->delete should have logged' ); t::lib::Mocks::mock_preference( 'ListOwnershipUponPatronDeletion', 'delete' ); - Koha::Virtualshelves->search({})->delete; - my $patron2 = $builder->build( { source => 'Borrower' } ); - my $retrieved_patron2 = Koha::Patrons->find( $patron2->{borrowernumber} ); + Koha::Virtualshelves->delete; + my $patron2 = $builder->build_object({ class => 'Koha::Patrons' }); my $private_list2 = Koha::Virtualshelf->new({ - shelfname => "private", - owner => $patron2->{borrowernumber}, - public => 0, - } - )->store; + shelfname => "private", + owner => $patron2->borrowernumber, + public => 0, + })->store; my $public_list2 = Koha::Virtualshelf->new({ - shelfname => "public", - owner => $patron2->{borrowernumber}, - public => 1, - } - )->store; + shelfname => "public", + owner => $patron2->borrowernumber, + public => 1, + })->store; my $list_to_share2 = Koha::Virtualshelf->new({ - shelfname => "shared", - owner => $patron2->{borrowernumber}, - public => 0, - } - )->store; - - my $shared_shelf2 = eval { $list_to_share2->share("valid key") }; - my $deleted2 = $retrieved_patron2->delete; + shelfname => "shared", + owner => $patron2->borrowernumber, + public => 0, + })->store; + $list_to_share2->share("valid key")->accept( "valid key", $patron_for_sharing->borrowernumber ); - is( Koha::Virtualshelves->search( { owner => $patron2->{borrowernumber} } )->count, 0, q|Koha::Patron->delete should have deleted patron's lists| ); - $transferred_lists = Koha::Virtualshelves->search({})->count; - is( $transferred_lists, 0, 'All lists should be deleted with ListOwnershipUponPatronDeletion set to Delete'); + # Delete patron2, check if shelves and shares are now empty + $patron2->delete; + is( Koha::Virtualshelves->count, 0, 'All lists should be gone now' ); + is( Koha::Virtualshelfshares->count, 0, 'All shares should be gone too' ); }; subtest 'Koha::Patrons->delete' => sub { -- 2.20.1