From ef8a234a62fb16f1e812acff1c0019e9a19adc32 Mon Sep 17 00:00:00 2001 From: Alex Buckley Date: Thu, 23 Sep 2021 14:23:44 +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 8. Comment #59 9. Comment #61 Test plan for comment #49 fix: 1. Log into the staff client as User A. Create one of each of the following types of public lists: - 'Owner only' - 'staff only' - 'Anyone seeing this list' 2. Visit the OPAC. Do not login. Confirm you are able to see all three public lists under the 'Lists' dropdown in the OPAC header 3. Perform a search. Select a search result checkbox and view the dropdown options in the 'Select titles to:' dropdown 4. Confirm only the 'Anyone seeing this list' is displayed as an option 5. Log into the OPAC as a user B (a user with no permissions). Confirm you can see all three public lists under the header 'Lists' dropdown 6. Repeat steps 3, and 4 as User B with the same results as un-authenticated user 6. Log into the OPAC as user C (a user with access to the staff client). Confirm you can see all three public lists under the header 'Lists' dropdown 7. Repeat steps 3, and 4 but this time you should see the 'staff only' and 'Anyone seeing this list' public lists in the dropdown 8. Log into the OPAC as user A. Confirm you can see all three poublic lists under the header 'Lists' dropdown 9. Repeat steps 3, and 4 but this time you should see the 'owner only', 'staff only' and 'Anyone seeing this list' public lists in the dropdown Sponsored-By: Catalyst IT --- Koha/Virtualshelf.pm | 12 ++++++++---- Koha/Virtualshelves.pm | 9 +++++++-- opac/opac-shelves.pl | 3 ++- t/db_dependent/Virtualshelves.t | 42 ++++++++++++++++++++--------------------- 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/Koha/Virtualshelf.pm b/Koha/Virtualshelf.pm index 1400bbdcc8..20e7fdbfb9 100644 --- a/Koha/Virtualshelf.pm +++ b/Koha/Virtualshelf.pm @@ -186,7 +186,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 0 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( { @@ -208,8 +209,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, @@ -236,7 +238,7 @@ sub can_be_deleted { return 0 unless $borrowernumber; return 1 if $self->owner == $borrowernumber; - my $patron = Koha::Patrons->find( $borrowernumber ); + my $patron = Koha::Patrons->find( $borrowernumber ) or return 0; return 1 if $self->is_public and haspermission( $patron->userid, { lists => 'delete_public_lists' } ); @@ -257,9 +259,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; } @@ -273,3 +276,4 @@ sub _type { return 'Virtualshelve'; } +1; diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm index 5625214a94..d5fc6b0513 100644 --- a/Koha/Virtualshelves.pm +++ b/Koha/Virtualshelves.pm @@ -22,7 +22,6 @@ use Koha::Database; use Koha::Virtualshelf; -use C4::Auth; use base qw(Koha::Objects); @@ -88,8 +87,14 @@ sub get_some_shelves { my $add_allowed = $params->{add_allowed}; my @conditions; + my $patron; + my $staffuser = 0; + if ( $borrowernumber != 0 ) { + $patron = Koha::Patrons->find( $borrowernumber ); + $staffuser = $patron->can_patron_change_staff_only_lists; + } if ( $add_allowed ) { - if ( Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) { + if ( $staffuser ) { push @conditions, { -or => [ diff --git a/opac/opac-shelves.pl b/opac/opac-shelves.pl index 6b186ffd31..b8209c00d0 100755 --- a/opac/opac-shelves.pl +++ b/opac/opac-shelves.pl @@ -442,7 +442,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 3345c828e8..47da6061fe 100755 --- a/t/db_dependent/Virtualshelves.t +++ b/t/db_dependent/Virtualshelves.t @@ -289,17 +289,17 @@ subtest 'Shelf permissions' => sub { } )->store; - is( $public_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view his public list' ); + is( $public_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view their public list' ); is( $public_shelf->can_be_viewed( $patron2->{borrowernumber} ), 1, 'Public list should be viewed by another staff member'); is( $public_shelf->can_be_viewed( $patron3->{borrowernumber} ), 1, 'Public list should be viewed by someone with no special permissions' ); is( $public_shelf->can_be_viewed( $patron4->{borrowernumber} ), 1, 'Public list should be viewed by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete his list' ); + is( $public_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete their list' ); is( $public_shelf->can_be_deleted( $patron2->{borrowernumber} ), 0, 'Public list should not be deleted by another staff member' ); is( $public_shelf->can_be_deleted( $patron3->{borrowernumber} ), 0, 'Public list should not be deleted by someone with no special permissions' ); is( $public_shelf->can_be_deleted( $patron4->{borrowernumber} ), 0, 'Public list should not be deleted by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage his list' ); + is( $public_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage their list' ); is( $public_shelf->can_be_managed( $patron2->{borrowernumber} ), 0, 'Public list should not be managed by another staff member' ); is( $public_shelf->can_be_managed( $patron3->{borrowernumber} ), 0, 'Public list should not be managed by someone with no special permissions' ); is( $public_shelf->can_be_managed( $patron4->{borrowernumber} ), 1, 'Public list should be managed by someone with the edit_public_lists sub-permission checked' ); @@ -317,27 +317,27 @@ subtest 'Shelf permissions' => sub { $public_shelf->allow_change_from_owner(1); $public_shelf->store; - is( $public_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view his public list' ); + is( $public_shelf->can_be_viewed( $patron1->{borrowernumber} ), 1, 'The owner should be able to view their public list' ); is( $public_shelf->can_be_viewed( $patron2->{borrowernumber} ), 1, 'Public list should be viewed by staff member' ); is( $public_shelf->can_be_viewed( $patron3->{borrowernumber} ), 1, 'Public list should be viewed by someone with no special permissions' ); is( $public_shelf->can_be_viewed( $patron4->{borrowernumber} ), 1, 'Public list should be viewable by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete his list' ); + is( $public_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete their list' ); is( $public_shelf->can_be_deleted( $patron2->{borrowernumber} ), 0, 'Public list should not be deleted by another staff member' ); is( $public_shelf->can_be_deleted( $patron3->{borrowernumber} ), 0, 'Public list should not be deleted by someone with no special permissions' ); is( $public_shelf->can_be_deleted( $patron4->{borrowernumber} ), 0, 'Public list should not be deleted by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage his list' ); + is( $public_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage thier list' ); is( $public_shelf->can_be_managed( $patron2->{borrowernumber} ), 0, 'Public list should not be managed by another staff member' ); is( $public_shelf->can_be_managed( $patron3->{borrowernumber} ), 0, 'Public list should not be managed by someone with no special permissions' ); is( $public_shelf->can_be_managed( $patron4->{borrowernumber} ), 1, 'Public list should be managed by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_biblios_be_added( $patron1->{borrowernumber} ), 1, 'The owner should be able to add biblios to his list' ); + is( $public_shelf->can_biblios_be_added( $patron1->{borrowernumber} ), 1, 'The owner should be able to add biblios to their list' ); is( $public_shelf->can_biblios_be_added( $patron2->{borrowernumber} ), 0, 'Public list should not be modified (add) by another staff member' ); is( $public_shelf->can_biblios_be_added( $patron3->{borrowernumber} ), 0, 'Public list should not be modified (add) by someone with no special permissions' ); is( $public_shelf->can_biblios_be_added( $patron4->{borrowernumber} ), 0, 'Public list should not be modified (add) by someone with the edit_public_lists sub-permission checked' ); - is( $public_shelf->can_biblios_be_removed( $patron1->{borrowernumber} ), 1, 'The owner should be able to remove biblios to his list' ); + is( $public_shelf->can_biblios_be_removed( $patron1->{borrowernumber} ), 1, 'The owner should be able to remove biblios to their list' ); is( $public_shelf->can_biblios_be_removed( $patron2->{borrowernumber} ), 0, 'Public list should not be modified (remove) by another staff member' ); is( $public_shelf->can_biblios_be_removed( $patron3->{borrowernumber} ), 0, 'Public list should not be modified (remove) by someone with no special permissions' ); is( $public_shelf->can_biblios_be_removed( $patron4->{borrowernumber} ), 0, 'Public list should not be modified (remove) by someone with the edit_public_list sub-permission checked' ); @@ -352,17 +352,17 @@ subtest 'Shelf permissions' => sub { } )->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' ); @@ -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' ); @@ -410,27 +410,27 @@ subtest 'Shelf permissions' => sub { $private_shelf->allow_change_from_others(1); $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 could be modified (add) by another staff member # individual check done later' ); is( $private_shelf->can_biblios_be_added( $patron3->{borrowernumber} ), 1, 'Private list could be modified (add) by someone with no special permissions' ); is( $private_shelf->can_biblios_be_added( $patron4->{borrowernumber} ), 1, 'Private list could 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 could be modified (remove) by another staff member # individual check done later' ); is( $private_shelf->can_biblios_be_removed( $patron3->{borrowernumber} ), 1, 'Private list could be modified (remove) by someone with no special permissions' ); is( $private_shelf->can_biblios_be_removed( $patron4->{borrowernumber} ), 1, 'Private list could be modified (remove) by someone with the edit_public_lists sub-permission checked' ); -- 2.11.0