Bugzilla – Attachment 128022 Details for
Bug 26346
Add option to make a public list editable by library staff only
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26346: (follow-up) Fixes in response to QA
Bug-26346-follow-up-Fixes-in-response-to-QA.patch (text/plain), 21.67 KB, created by
Alex Buckley
on 2021-11-25 20:52:56 UTC
(
hide
)
Description:
Bug 26346: (follow-up) Fixes in response to QA
Filename:
MIME Type:
Creator:
Alex Buckley
Created:
2021-11-25 20:52:56 UTC
Size:
21.67 KB
patch
obsolete
>From aceae3c81e1b93b89e7ec31ce3dacd37b3c2e181 Mon Sep 17 00:00:00 2001 >From: Alex Buckley <alexbuckley@catalyst.net.nz> >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 5f9b7bb153..e8f0f14180 100644 >--- a/Koha/Virtualshelf.pm >+++ b/Koha/Virtualshelf.pm >@@ -179,7 +179,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( > { >@@ -201,8 +202,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, >@@ -229,7 +231,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' } ); > >@@ -250,9 +252,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; > } > >@@ -272,3 +275,4 @@ sub _type { > return 'Virtualshelve'; > } > >+1; >diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm >index 8e678e25a3..981e8e9506 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 7143876816..f480e04065 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 b825c9cf6c..7d33bfc0de 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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=128022">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26346
:
119686
|
120663
|
120664
|
120665
|
120909
|
120910
|
120953
|
120954
|
121511
|
121512
|
122033
|
122034
|
122533
|
122535
|
122536
|
122537
|
122538
|
122539
|
122540
|
122685
|
122686
|
122687
|
122688
|
122689
|
122691
|
124010
|
124011
|
125436
|
125472
|
125914
|
125915
|
125916
|
125917
|
127556
|
127557
|
127558
|
127559
|
127560
|
128017
|
128018
|
128019
|
128020
|
128021
|
128022
|
128023
|
128383
|
128384
|
128385
|
131699
|
131700
|
131757
|
131764
|
132373
|
132475
|
132476
|
132477
|
132478
|
132479
|
132751
|
132752
|
132753
|
132754
|
132755