From 36b272f2db96a671b50ac3f1a2a8847011ec9182 Mon Sep 17 00:00:00 2001 From: Alex Buckley Date: Fri, 2 Jul 2021 06:59:02 +0000 Subject: [PATCH] Bug 26346: (follow-up) Fixes in response to QA Fixes for the following comments on bug report 26346: 1. Comment #31 2. Comment #32 3. Comment #35 4. Comment #36 5. Comment #37 6. Comment #38 7. Comment #49 Test plan for comment #49 fix: 1. Create a 'staff only' public list and 'Anyone seeing this list' public list 2. Visit the OPAC. Do not login. Confirm you are able to see both lists created in comment #1 in the OPAC 3. Perform a search, select a search result checkbox and view the dropdown options in the 'Select titles to:' dropdown 3. Confirm both lists in step #1 are displayed in the dropdown 4. Try adding an item to the 'staff only' list. Observe you're prompted to login 5. Login as a user with staff client permissions and confirm you can add the item to the 'staff only' list 6. Logout, and repeat step 4. Login as a user with no permissionsi. Confirm you cannot add an item to a 'staff only' list 7. Confirm both staff users and users with no permissions can add items to the 'Anyone seeing the list' public list in the OPAC Sponsored-By: Catalyst IT --- Koha/Virtualshelf.pm | 10 +++++++--- Koha/Virtualshelves.pm | 37 +++++++++++-------------------------- opac/opac-shelves.pl | 3 ++- t/db_dependent/Virtualshelves.t | 10 +++++----- 4 files changed, 25 insertions(+), 35 deletions(-) diff --git a/Koha/Virtualshelf.pm b/Koha/Virtualshelf.pm index 676791ff02..e6cab20d73 100644 --- a/Koha/Virtualshelf.pm +++ b/Koha/Virtualshelf.pm @@ -187,7 +187,8 @@ sub add_biblio { return if $already_exists; # Check permissions - return unless ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) || ( $self->allow_change_from_staff && Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) || $self->allow_change_from_others; + my $patron = Koha::Patrons->find( $borrowernumber ) or return 0; + return unless ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) || ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) || $self->allow_change_from_others; my $content = Koha::Virtualshelfcontent->new( { @@ -209,8 +210,9 @@ sub remove_biblios { return unless @$biblionumbers; my $number_removed = 0; + my $patron = Koha::Patrons->find( $borrowernumber ) or return 0; if( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) - || ( $self->allow_change_from_staff && Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) + || ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) || $self->allow_change_from_others ) { $number_removed += $self->get_contents->search({ biblionumber => $biblionumbers, @@ -258,9 +260,10 @@ sub can_be_managed { sub can_biblios_be_added { my ( $self, $borrowernumber ) = @_; + my $patron = Koha::Patrons->find( $borrowernumber ) or return 0; return 1 if $borrowernumber - and ( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) or ( $self->allow_change_from_staff && Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) or $self->allow_change_from_others ); + and ( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) or ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) or $self->allow_change_from_others ); return 0; } @@ -274,3 +277,4 @@ sub _type { return 'Virtualshelve'; } +1; diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm index 8162dfd815..767fb8c39d 100644 --- a/Koha/Virtualshelves.pm +++ b/Koha/Virtualshelves.pm @@ -23,8 +23,6 @@ use Koha::Database; use Koha::Virtualshelf; -use C4::Auth; - use base qw(Koha::Objects); =head1 NAME @@ -90,30 +88,17 @@ sub get_some_shelves { my @conditions; if ( $add_allowed ) { - if ( Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) { - push @conditions, { - -or => - [ - { - "me.owner" => $borrowernumber, - "me.allow_change_from_owner" => 1, - }, - "me.allow_change_from_others" => 1, - "me.allow_change_from_staff" => 1 - ] - }; - } else { - push @conditions, { - -or => - [ - { - "me.owner" => $borrowernumber, - "me.allow_change_from_owner" => 1, - }, - "me.allow_change_from_others" => 1, - ] - }; - } + push @conditions, { + -or => + [ + { + "me.owner" => $borrowernumber, + "me.allow_change_from_owner" => 1, + }, + "me.allow_change_from_others" => 1, + "me.allow_change_from_staff" => 1, + ] + }; } if ( $category == 1 ) { push @conditions, { diff --git a/opac/opac-shelves.pl b/opac/opac-shelves.pl index 2914385d22..6d2def2a4f 100755 --- a/opac/opac-shelves.pl +++ b/opac/opac-shelves.pl @@ -436,7 +436,8 @@ if ( $op eq 'list' ) { ), ); } -my $staffuser = Koha::Patrons->find( $loggedinuser )->can_patron_change_staff_only_lists if $loggedinuser; +my $staffuser; +$staffuser = Koha::Patrons->find( $loggedinuser )->can_patron_change_staff_only_lists if $loggedinuser; $template->param( op => $op, referer => $referer, diff --git a/t/db_dependent/Virtualshelves.t b/t/db_dependent/Virtualshelves.t index 0977362d9c..5de065652f 100755 --- a/t/db_dependent/Virtualshelves.t +++ b/t/db_dependent/Virtualshelves.t @@ -381,27 +381,27 @@ subtest 'Shelf permissions' => sub { $private_shelf->allow_change_from_staff(1); $private_shelf->allow_change_from_others(0); $private_shelf->store; - is( $private_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view his list' ); + is( $private_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view their list' ); is( $private_shelf->can_be_viewed( $patron2->{borrowernumber} ), 0, 'Private list should not be viewed by another staff member' ); is( $private_shelf->can_be_viewed( $patron3->{borrowernumber} ), 0, 'Private list should not be viewed by someone with no special permissions' ); is( $private_shelf->can_be_viewed( $patron4->{borrowernumber} ), 0, 'Private list should not be viewed by someone with the edit_public_lists sub-permission checked' ); - is( $private_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete his list' ); + is( $private_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete their list' ); is( $private_shelf->can_be_deleted( $patron2->{borrowernumber} ), 0, 'Private list should not be deleted by another staff member' ); is( $private_shelf->can_be_deleted( $patron3->{borrowernumber} ), 0, 'Private list should not be deleted by someone with no special permissions' ); is( $private_shelf->can_be_deleted( $patron4->{borrowernumber} ), 0, 'Private list should not be deleted by someone with the edit_public_lists sub-permission checked' ); - is( $private_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage his list' ); + is( $private_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage their list' ); is( $private_shelf->can_be_managed( $patron2->{borrowernumber} ), 0, 'Private list should not be managed by another staff member' ); is( $private_shelf->can_be_managed( $patron3->{borrowernumber} ), 0, 'Private list should not be managed by someone with no special permissions' ); is( $private_shelf->can_be_managed( $patron4->{borrowernumber} ), 0, 'Private list should not be managed by someone with the edit_public_lists sub-permission checked' ); - is( $private_shelf->can_biblios_be_added( $patron1->{borrowernumber} ), 1, 'The owner should be able to add biblios to his list' ); + is( $private_shelf->can_biblios_be_added( $patron1->{borrowernumber} ), 1, 'The owner should be able to add biblios to their list' ); is( $private_shelf->can_biblios_be_added( $patron2->{borrowernumber} ), 1, 'Private list should not modified (add) by another staff member # individual check done later' ); is( $private_shelf->can_biblios_be_added( $patron3->{borrowernumber} ), 0, 'Private list should not be modified (add) by someone with no special permissions' ); is ( $private_shelf->can_biblios_be_added( $patron4->{borrowernumber} ), 0, 'Private list should not be modified (add) by someone with the edit_public_lists sub-permission checked' ); - is( $private_shelf->can_biblios_be_removed( $patron1->{borrowernumber} ), 1, 'The owner should be able to remove biblios to his list' ); + is( $private_shelf->can_biblios_be_removed( $patron1->{borrowernumber} ), 1, 'The owner should be able to remove biblios to their list' ); is( $private_shelf->can_biblios_be_removed( $patron2->{borrowernumber} ), 1, 'Private list should be modified (remove) by another staff member # individual check done later' ); is( $private_shelf->can_biblios_be_removed( $patron3->{borrowernumber} ), 0, 'Private list should not be modified (remove) by someone with no special permissions' ); is( $private_shelf->can_biblios_be_removed( $patron4->{borrowernumber} ), 0, 'Private list should not be modified (remove) by someone with the edit_public_lists sub-permission checked' ); -- 2.11.0