From df02ca05dfb6a471a91e9d5b3af7f06f1100ed4f Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Mon, 20 Feb 2017 21:51:39 +0000 Subject: [PATCH] Bug 11889: [FOLLOW-UP] Tests and new get_shared_shelves subroutine To test: 1) prove t/db_dependent/Virtualshelves.t 2) prove t/db_dependent/Koha/Patrons.t Sponsored-by: Catalyst IT --- Koha/Patron.pm | 11 ++++-- Koha/Virtualshelves.pm | 13 ++++++++ t/db_dependent/Koha/Patrons.t | 74 +++++++++++++++++++++++++++++++++++------ t/db_dependent/Virtualshelves.t | 16 +++++++-- 4 files changed, 97 insertions(+), 17 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 4c4a832..96dc8c9 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -68,18 +68,23 @@ sub delete { # Delete Patron's holds $self->holds->delete; + # 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; - my @sharedlists = Koha::Virtualshelves->search({ 'me.owner' => $self->borrowernumber, 'me.shelfnumber' => { -ident => 'virtualshelfshares.shelfnumber' } }, { prefetch => 'virtualshelfshares' }); + my @sharedlists = Koha::Virtualshelves->get_shared_shelves({ borrowernumber => $self->borrowernumber }); foreach my $plist ( @publiclists ) { - $plist->set({ owner => $usernumber })->store; + if ( $plist->owner == $self->borrowernumber ) { + my $unique_name = $plist->shelfname . '_' . $self->borrowernumber; + $plist->set({ owner => $usernumber, shelfname => $unique_name })->store; + } } foreach my $slist ( @sharedlists ) { - $slist->set({ owner => $usernumber })->store; + 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 ); } diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm index a8d1b20..20ba341 100644 --- a/Koha/Virtualshelves.pm +++ b/Koha/Virtualshelves.pm @@ -158,6 +158,19 @@ sub get_shelves_containing_record { ); } +sub get_shared_shelves { + my ( $self, $params ) = @_; + my $borrowernumber = $params->{borrowernumber} || 0; + + $self->search( + { + 'me.owner' => $borrowernumber, + 'me.shelfnumber' => { -ident => 'virtualshelfshares.shelfnumber' } + }, + { prefetch => 'virtualshelfshares' } + ); +} + sub _type { return 'Virtualshelve'; } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 0e6791a..1a88527 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -26,12 +26,14 @@ use DateTime; use C4::Biblio; use C4::Circulation; use C4::Members; +use C4::Context; use Koha::Holds; use Koha::Patron; use Koha::Patrons; use Koha::Database; use Koha::DateUtils; +use Koha::Virtualshelf; use Koha::Virtualshelves; use t::lib::TestBuilder; @@ -64,7 +66,7 @@ my $new_patron_2 = Koha::Patron->new( )->store; C4::Context->_new_userenv('xxx'); -C4::Context->set_userenv(0,0,0,'firstname','surname', $library->{branchcode}, 'Midway Public Library', '', '', ''); +C4::Context->set_userenv(1,'userid','usercnum','firstname','surname', $library->{branchcode}, 'Midway Public Library', '', '', ''); is( Koha::Patrons->search->count, $nb_of_patrons + 2, 'The 2 patrons should have been added' ); @@ -336,32 +338,82 @@ subtest "move_to_deleted" => sub { }; subtest "delete" => sub { - plan tests => 5; + plan tests => 9; t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + t::lib::Mocks::mock_preference( 'ListOwnershipUponPatronDeletion', 'transfer' ); + Koha::Virtualshelves->search({})->delete; + my $userenv = C4::Context->userenv(); my $patron = $builder->build( { source => 'Borrower' } ); + my $patron_for_sharing = (ref($userenv) eq 'HASH' ) ? $userenv->{'number'} : 0; my $retrieved_patron = Koha::Patrons->find( $patron->{borrowernumber} ); my $hold = $builder->build( { source => 'Reserve', - value => { borrowernumber => $patron->{borrowernumber} } + value => { borrowernumber => $patron->{borrowernumber} } } ); - my $list = $builder->build( - { source => 'Virtualshelve', - value => { owner => $patron->{borrowernumber} } + my $private_list = Koha::Virtualshelf->new({ + shelfname => "private", + owner => $patron->{borrowernumber}, + category => 1 } - ); + )->store; + my $public_list = Koha::Virtualshelf->new({ + shelfname => "public", + owner => $patron->{borrowernumber}, + category => 2 + } + )->store; + my $list_to_share = Koha::Virtualshelf->new({ + shelfname => "shared", + owner => $patron->{borrowernumber}, + category => 1 + } + )->store; + my $shared_shelf = eval { $list_to_share->share("valid key")->accept("valid key", $patron_for_sharing) }; my $deleted = $retrieved_patron->delete; - is( $deleted, 1, 'Koha::Patron->delete should return 1 if the patron has been correctly deleted' ); + 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::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 $transferred_lists = Koha::Virtualshelves->search({ owner => $patron_for_sharing })->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| ); - 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} ); + my $private_list2 = Koha::Virtualshelf->new({ + shelfname => "private", + owner => $patron2->{borrowernumber}, + category => 1 + } + )->store; + my $public_list2 = Koha::Virtualshelf->new({ + shelfname => "public", + owner => $patron2->{borrowernumber}, + category => 2 + } + )->store; + my $list_to_share2 = Koha::Virtualshelf->new({ + shelfname => "shared", + owner => $patron2->{borrowernumber}, + category => 1 + } + )->store; + + my $shared_shelf2 = eval { $list_to_share2->share("valid key") }; + my $deleted2 = $retrieved_patron2->delete; + + 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'); }; subtest 'add_enrolment_fee_if_needed' => sub { diff --git a/t/db_dependent/Virtualshelves.t b/t/db_dependent/Virtualshelves.t index c867056..7352dac 100644 --- a/t/db_dependent/Virtualshelves.t +++ b/t/db_dependent/Virtualshelves.t @@ -350,7 +350,7 @@ subtest 'Shelf permissions' => sub { }; subtest 'Get shelves' => sub { - plan tests => 4; + plan tests => 5; my $patron1 = $builder->build({ source => 'Borrower', }); @@ -388,19 +388,29 @@ subtest 'Get shelves' => sub { category => 2, } )->store; + my $shelf_to_share = Koha::Virtualshelf->new({ + shelfname => "shared shelf", + owner => $patron1->{borrowernumber}, + category => 1, + } + )->store; my $private_shelves = Koha::Virtualshelves->get_private_shelves; is( $private_shelves->count, 0, 'Without borrowernumber given, get_private_shelves should not return any shelf' ); $private_shelves = Koha::Virtualshelves->get_private_shelves({ borrowernumber => $patron1->{borrowernumber} }); - is( $private_shelves->count, 2, 'get_private_shelves should return all shelves for a given patron' ); + is( $private_shelves->count, 3, 'get_private_shelves should return all shelves for a given patron' ); $private_shelf2_1->share('a key')->accept('a key', $patron1->{borrowernumber}); $private_shelves = Koha::Virtualshelves->get_private_shelves({ borrowernumber => $patron1->{borrowernumber} }); - is( $private_shelves->count, 3, 'get_private_shelves should return all shelves for a given patron, even the shared ones' ); + is( $private_shelves->count, 4, 'get_private_shelves should return all shelves for a given patron, even the shared ones' ); my $public_shelves = Koha::Virtualshelves->get_public_shelves; is( $public_shelves->count, 2, 'get_public_shelves should return all public shelves, no matter who is the owner' ); + my $shared_shelf = eval { $shelf_to_share->share("valid key") }; + my $shared_shelves = Koha::Virtualshelves->get_shared_shelves({ borrowernumber => $patron1->{borrowernumber} }); + is( $shared_shelves->count, 1, 'get_shared_shelves should return shared shelves' ); + teardown(); }; -- 2.1.4