Attachment 132477 Details for Bug 26346 – Bug 26346: Add option to make public lists editable by all staff

[patch] Bug 26346: Add option to make public lists editable by all staff

Bug-26346-Add-option-to-make-public-lists-editable.patch (text/plain), 44.78 KB, created by Lucas Gass (lukeg) on 2022-03-29 16:22:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lucas Gass (lukeg)

Created: 2022-03-29 16:22:30 UTC

Size: 44.78 KB

patch

obsolete

>From 6dfc72a083f4796bbf1f356050a3a0a16d1ae277 Mon Sep 17 00:00:00 2001
>From: Alex Buckley <alexbuckley@catalyst.net.nz>
>Date: Wed, 14 Apr 2021 14:06:48 +1200
>Subject: [PATCH] Bug 26346: Add option to make public lists editable by all
> staff
>
>If a staff member has access to the staff client (either because
>'catalogue' permission is enabled or they're a superlibrarian
>then that user can add items (from OPAC or staff client) to a list
>marked 'Staff only'
>
>Test plan:
>1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
>from' there are three options: Nobody, Owner only, Anyone seeing this
>list
>
>2. Apply first 3 patches and run updatedatabase.pl
>cd installer/data/mysql
>sudo koha-shell <instance>
>./updatedatabase.pl
>
>3. Restart memcached and plack
>
>4. Create 4 patron accounts:
>- User A : Superlibrarian permissions
>- User B : 'Staff access, allows viewing of catalogue in staff interface
>(catalogue)'
>- User C : No permissions
>- User D : 'Staff access, allows viewing of catalogue in staff
>interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission
>
>5. Login to staff client as User A.
>Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'
>
>6. Log into the staff client as User B.
>Confirm you can add items to the list from the following staff client pages:
>- Individual list page using the 'Add items' button
>- Staff client search result page
>- Staff client biblio detail page
>
>7. Confirm you can remove items from the list
>
>8. Confirm you can perform an OPAC search when not logged in
>
>9. Log into the OPAC as User B. Confirm you can add items to the list
>from the following OPAC pages:
>- OPAC search result page
>- OPAC biblio detail page
>
>10. Log into the OPAC as User C. Do an OPAC search and confirm you
>can view the list, but not add items to it
>
>11. Login to the staff client as User B. Create a new list with the
>following settings:
>- 'Category'='Private',
>- 'Allow changes to contents from'='Staff only'
>
>Notice a red hint message is displayed.
>Change 'Category'='Public' and notice the hint is removed
>
>12. Log into the OPAC as User C. Notice the 'Staff only' option is not
>available when creating a list
>
>13. Log into the OPAC as User B. Repeat step 11. Confirm the same
>outcome
>
>14. Log into the staff client as User A. Create a list with the
>following settings:
>
>- Public = 'Public'
>- Allow changes to contents from = 'owner only'
>
>15. Log into the staff client as User D. Edit the list from step 14
>confirm you can edit the list to have 'Allow changes to contents from' =
>'Staff only'
>
>16. Run Patron.t and Virtualshelves.t unit tests:
>sudo koha-shell <instance>
>prove t/db_dependent/Koha/Patron.t
>prove t/db_dependent/Virtualshelves.t
>
>Sponsored-by: Horowhenua District Council, New Zealand
>
>Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
>
>Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
>---
> Koha/Patron.pm | 15 ++
> Koha/Virtualshelf.pm | 18 +-
> Koha/Virtualshelves.pm | 41 +++-
> .../prog/en/includes/permissions.inc | 5 +
> .../prog/en/modules/virtualshelves/shelves.tt | 14 +-
> .../bootstrap/en/modules/opac-shelves.tt | 14 +-
> opac/opac-addbybiblionumber.pl | 55 ++++--
> opac/opac-shelves.pl | 7 +-
> t/db_dependent/Koha/Patron.t | 32 +++-
> t/db_dependent/Virtualshelves.t | 177 +++++++++++++-----
> virtualshelves/shelves.pl | 3 +
> 11 files changed, 307 insertions(+), 74 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 0c16079fa9..175247ca5b 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -2063,6 +2063,21 @@ sub recalls {
> return Koha::Recalls->search({ borrowernumber => $self->borrowernumber });
> }
> 
>+=head3 can_patron_change_staff_only_lists
>+
>+$patron->can_patron_change_staff_only_lists;
>+
>+Return 1 if a patron has 'Superlibrarian' or 'Catalogue' permission.
>+Otherwise, return 0.
>+
>+=cut
>+
>+sub can_patron_change_staff_only_lists {
>+ my ( $self, $params ) = @_;
>+ return 1 if C4::Auth::haspermission( $self->userid, { 'catalogue' => 1 });
>+ return 0;
>+}
>+
> =head2 Internal methods
> 
> =head3 _type
>diff --git a/Koha/Virtualshelf.pm b/Koha/Virtualshelf.pm
>index a7273a95d9..e8f0f14180 100644
>--- a/Koha/Virtualshelf.pm
>+++ b/Koha/Virtualshelf.pm
>@@ -56,6 +56,8 @@ sub store {
> unless defined $self->allow_change_from_owner;
> $self->allow_change_from_others( 0 )
> unless defined $self->allow_change_from_others;
>+ $self->allow_change_from_staff( 0 )
>+ unless defined $self->allow_change_from_staff;
> 
> $self->created_on( dt_from_string )
> unless defined $self->created_on;
>@@ -177,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_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(
> {
>@@ -199,7 +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 && $patron->can_patron_change_staff_only_lists )
> || $self->allow_change_from_others ) {
> $number_removed += $self->get_contents->search({
> biblionumber => $biblionumbers,
>@@ -226,9 +231,9 @@ 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 C4::Auth::haspermission( $patron->userid, { lists => 'delete_public_lists' } );
>+ return 1 if $self->is_public and haspermission( $patron->userid, { lists => 'delete_public_lists' } );
> 
> return 0;
> }
>@@ -237,15 +242,20 @@ sub can_be_managed {
> my ( $self, $borrowernumber ) = @_;
> return 1
> if $borrowernumber and $self->owner == $borrowernumber;
>+
>+ my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
>+ return 1
>+ if $self->is_public and haspermission( $patron->userid, { lists => 'edit_public_lists' } );
> return 0;
> }
> 
> 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_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;
> }
> 
>diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm
>index df75c3b59a..981e8e9506 100644
>--- a/Koha/Virtualshelves.pm
>+++ b/Koha/Virtualshelves.pm
>@@ -22,6 +22,7 @@ use Koha::Database;
> 
> use Koha::Virtualshelf;
> 
>+
> use base qw(Koha::Objects);
> 
> =head1 NAME
>@@ -86,17 +87,37 @@ 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 ) {
>- push @conditions, {
>- -or =>
>- [
>- {
>- "me.owner" => $borrowernumber,
>- "me.allow_change_from_owner" => 1,
>- },
>- "me.allow_change_from_others" => 1,
>- ]
>- };
>+ if ( $staffuser ) {
>+ 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,
>+ ]
>+ };
>+ }
> }
> if ( !$public ) {
> push @conditions, {
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
>index 4c54d74c3a..9ad12124d7 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
>+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
>@@ -678,6 +678,11 @@
> Delete public lists
> 
> ([% name | html %])
>+ [%- CASE 'edit_public_lists' -%]
>+ 
>+ Edit public lists
>+ 
>+ ([% name | html %])
> [%- CASE 'upload_general_files' -%]
> 
> Upload any file
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
>index b5462a8ac3..2590f93592 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
>@@ -33,8 +33,11 @@
> 
> [% IF shelf.allow_change_from_others %]<option value="2" selected="selected">Anyone seeing this list</option>[% ELSE %]<option value="2">Anyone seeing this list</option>[% END %]
> 
>+ [% IF shelf.allow_change_from_staff %]<option value="3" selected="selected">Staff only</option>[% ELSE %]<option value="3">Staff only</option>[% END %]
>+
> </select>
> &emsp; The Anyone permission has no actual effect while this list is strictly private.
>+ &emsp; The Staff only permission has no actual effect while this list is strictly private.
> </li>
> [% END %]
> </head>
>@@ -801,16 +804,25 @@
> 
> if( perms < 2 ) {
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> } else if( public==0 ) {
> // If we move to Private (without shares), show Anyone remark
> // Note: the number of shares is not tested real-time
> [% IF !shelf.is_shared %]
>- $("#anyone_remark").show();
>+ if( perms== 2) {
>+ $("#anyone_remark").show();
>+ $("#staff_remark").hide();
>+ } else if ( perms==3 ) {
>+ $("#anyone_remark").hide();
>+ $("#staff_remark").show();
>+ }
> [% ELSE %]
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> [% END %]
> } else { // public==1
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> }
> }
> [% IF op == 'view' %]
>diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
>index 18e9ac2e70..e21d7680e6 100644
>--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
>+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
>@@ -50,8 +50,11 @@
> 
> [% IF shelf.allow_change_from_others %]<option value="2" selected="selected">Anyone seeing this list</option>[% ELSE %]<option value="2">Anyone seeing this list</option>[% END %]
> 
>+ [% IF staffuser == 1 %][% IF shelf.allow_change_from_staff %]<option value="3" selected="selected">Staff only</option>[% ELSE %]<option value="3">Staff only</option>[% END %][% END %]
>+
> </select>
> &emsp; The "Anyone" permission has no actual effect while this list is strictly private.
>+ &emsp; The "Staff only" permission has no actual effect while this list is strictly private.
> </li>
> [% END %]
> 
>@@ -1042,16 +1045,25 @@ function AdjustRemark() {
> 
> if( perms < 2 ) {
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> } else if( public==0 ) {
> // If we move to Private (without shares), show Anyone remark
> // Note: the number of shares is not tested real-time
> [% IF !shelf.is_shared %]
>- $("#anyone_remark").show();
>+ if ( perms==2 ) {
>+ $("#anyone_remark").show();
>+ $("#staff_remark").hide();
>+ } else if ( perms==3 ) {
>+ $("#anyone_remark").hide();
>+ $("#staff_remark").show();
>+ }
> [% ELSE %]
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> [% END %]
> } else { // public==1
> $("#anyone_remark").hide();
>+ $("#staff_remark").hide();
> }
> }
> </script>
>diff --git a/opac/opac-addbybiblionumber.pl b/opac/opac-addbybiblionumber.pl
>index 7dfe42cc14..4edbd133e7 100755
>--- a/opac/opac-addbybiblionumber.pl
>+++ b/opac/opac-addbybiblionumber.pl
>@@ -117,18 +117,51 @@ if ($newvirtualshelf) {
> },
> { join => 'virtualshelfshares', }
> );
>- my $public_shelves = Koha::Virtualshelves->search(
>- { public => 1,
>- -or => [
>- -and => {
>- allow_change_from_owner => 1,
>- owner => $loggedinuser,
>+ my $public_shelves;
>+ if ( $loggedinuser ) {
>+ if ( Koha::Patrons->find( $loggedinuser )->can_patron_change_staff_only_lists ) {
>+ $public_shelves = Koha::Virtualshelves->search(
>+ { public => 1,
>+ -or => [
>+ -and => {
>+ allow_change_from_owner => 1,
>+ owner => $loggedinuser,
>+ },
>+ allow_change_from_others => 1,
>+ allow_change_from_staff => 1
>+ ],
> },
>- allow_change_from_others => 1,
>- ],
>- },
>- { order_by => 'shelfname' }
>- );
>+ { order_by => 'shelfname' }
>+ );
>+ } else {
>+ $public_shelves = Koha::Virtualshelves->search(
>+ { public => 1,
>+ -or => [
>+ -and => {
>+ allow_change_from_owner => 1,
>+ owner => $loggedinuser,
>+ },
>+ allow_change_from_others => 1,
>+ ],
>+ },
>+ {order_by => 'shelfname' }
>+ );
>+ }
>+ } else {
>+ $public_shelves = Koha::Virtualshelves->search(
>+ { public => 1,
>+ -or => [
>+ -and => {
>+ allow_change_from_owner => 1,
>+ owner => $loggedinuser,
>+ },
>+ allow_change_from_others => 1,
>+ ],
>+ },
>+ {order_by => 'shelfname' }
>+ );
>+ }
>+
> $template->param(
> private_shelves => $private_shelves,
> private_shelves_shared_with_me => $shelves_shared_with_me,
>diff --git a/opac/opac-shelves.pl b/opac/opac-shelves.pl
>index 4c76116bca..42a889869d 100755
>--- a/opac/opac-shelves.pl
>+++ b/opac/opac-shelves.pl
>@@ -45,6 +45,7 @@ use Koha::Virtualshelves;
> use Koha::RecordProcessor;
> 
> use constant ANYONE => 2;
>+use constant STAFF => 3;
> 
> my $query = CGI->new;
> 
>@@ -116,6 +117,7 @@ if ( $op eq 'add_form' ) {
> public => $public,
> allow_change_from_owner => $allow_changes_from > 0,
> allow_change_from_others => $allow_changes_from == ANYONE,
>+ allow_change_from_staff => $allow_changes_from == STAFF,
> owner => scalar $loggedinuser,
> }
> );
>@@ -147,6 +149,7 @@ if ( $op eq 'add_form' ) {
> my $allow_changes_from = $query->param('allow_changes_from');
> $shelf->allow_change_from_owner( $allow_changes_from > 0 );
> $shelf->allow_change_from_others( $allow_changes_from == ANYONE );
>+ $shelf->allow_change_from_staff( $allow_changes_from == STAFF );
> $shelf->public( $public );
> eval { $shelf->store };
> 
>@@ -441,7 +444,8 @@ if ( $op eq 'list' ) {
> ),
> );
> }
>-
>+my $staffuser;
>+$staffuser = Koha::Patrons->find( $loggedinuser )->can_patron_change_staff_only_lists if $loggedinuser;
> $template->param(
> op => $op,
> referer => $referer,
>@@ -450,6 +454,7 @@ $template->param(
> public => $public,
> print => scalar $query->param('print') || 0,
> listsview => 1,
>+ staffuser => $staffuser,
> );
> 
> my $content_type = $query->param('rss')? 'rss' : 'html';
>diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
>index a1aff0035e..6418d6066f 100755
>--- a/t/db_dependent/Koha/Patron.t
>+++ b/t/db_dependent/Koha/Patron.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 15;
>+use Test::More tests => 16;
> use Test::Exception;
> use Test::Warn;
> 
>@@ -849,6 +849,36 @@ subtest 'article_requests() tests' => sub {
> $article_requests = $patron->article_requests;
> is( $article_requests->count, 4, '4 article requests' );
> 
>+ $schema->storage->txn_rollback;
>+
>+};
>+
>+subtest 'can_patron_change_staff_only_lists() tests' => sub {
>+
>+ plan tests => 3;
>+
>+ $schema->storage->txn_begin;
>+
>+ # make a user with no special permissions
>+ my $patron = $builder->build_object(
>+ {
>+ class => 'Koha::Patrons',
>+ value => {
>+ flags => undef
>+ }
>+ }
>+ );
>+ is( $patron->can_patron_change_staff_only_lists(), 0, 'Patron without permissions cannot change staff only lists');
>+
>+ # make it a 'Catalogue' permission
>+ $patron->set({ flags => 4 })->store->discard_changes;
>+ is( $patron->can_patron_change_staff_only_lists(), 1, 'Catalogue patron can change staff only lists');
>+
>+
>+ # make it a superlibrarian
>+ $patron->set({ flags => 1 })->store->discard_changes;
>+ is( $patron->can_patron_change_staff_only_lists(), 1, 'Superlibrarian patron can change staff only lists');
>+
> $schema->storage->txn_rollback;
> };
> 
>diff --git a/t/db_dependent/Virtualshelves.t b/t/db_dependent/Virtualshelves.t
>index a31d84cc49..7d33bfc0de 100755
>--- a/t/db_dependent/Virtualshelves.t
>+++ b/t/db_dependent/Virtualshelves.t
>@@ -22,7 +22,7 @@ my $dbh = C4::Context->dbh;
> teardown();
> 
> subtest 'CRUD' => sub {
>- plan tests => 13;
>+ plan tests => 14;
> my $patron = $builder->build({
> source => 'Borrower',
> });
>@@ -44,6 +44,7 @@ subtest 'CRUD' => sub {
> is( $number_of_shelves, 1, '1 shelf should have been inserted' );
> is( $shelf->allow_change_from_owner, 1, 'The default value for allow_change_from_owner should be 1' );
> is( $shelf->allow_change_from_others, 0, 'The default value for allow_change_from_others should be 0' );
>+ is ( $shelf->allow_change_from_staff, 0, 'The default value for allow_change_from_staff should be 0');
> is( t::lib::Dates::compare( $shelf->created_on, dt_from_string), 0, 'The creation time should have been set to today' );
> 
> # Test if creation date will not be overwritten by store
>@@ -171,9 +172,10 @@ subtest 'Sharing' => sub {
> 
> subtest 'Shelf content' => sub {
> 
>- plan tests => 18;
>+ plan tests => 21;
> my $patron1 = $builder->build( { source => 'Borrower', } );
> my $patron2 = $builder->build( { source => 'Borrower', } );
>+ my $patron3 = $builder->build( { source => 'Borrower', value => {flags => 1} });
> my $biblio1 = $builder->build_sample_biblio;
> my $biblio2 = $builder->build_sample_biblio;
> my $biblio3 = $builder->build_sample_biblio;
>@@ -247,18 +249,35 @@ subtest 'Shelf content' => sub {
> $number_of_contents = Koha::Virtualshelfcontents->search->count;
> is( $number_of_contents, 3, 'Back to three entries' );
> 
>+ # allow_change_from_staff == 1 and allow_change_from_others == 0
>+ $shelf->allow_change_from_staff( 1 );
>+ $shelf->allow_change_from_others( 0 );
>+ $content4 = $shelf->add_biblio( $biblio3->biblionumber, $patron3->{borrowernumber} );
>+ $number_of_contents = Koha::Virtualshelfcontents->search->count;
>+ is( $number_of_contents, 4, 'The biblio should have been added to the shelf by patron 2');
>+ $number_of_deleted_biblios = $shelf->remove_biblios( { biblionumbers => [ $biblio3->biblionumber ], borrowernumber => $patron3->{borrowernumber} } );
>+ is( $number_of_deleted_biblios, 1, 'Biblio 3 deleted by patron 2' );
>+ $number_of_contents = Koha::Virtualshelfcontents->search->count;
>+ is( $number_of_contents, 3, 'Back to three entries' );
>+
> teardown();
> };
> 
> subtest 'Shelf permissions' => sub {
> 
>- plan tests => 40;
>+ plan tests => 100;
> my $patron1 = $builder->build( { source => 'Borrower', value => { flags => '2096766' } } ); # 2096766 is everything checked but not superlibrarian
> my $patron2 = $builder->build( { source => 'Borrower', value => { flags => '1048190' } } ); # 1048190 is everything checked but not superlibrarian and delete_public_lists
>+ my $patron3 = $builder->build( { source => 'Borrower', value => { flags => '0' } } ); # this is a patron with no special permissions
>+ my $patron4 = $builder->build( { source => 'Borrower', value => { flags => '0' } } );
>+ my $sth = $dbh->prepare("INSERT INTO user_permissions (borrowernumber, module_bit, code) VALUES (?,?,?)");
>+ $sth->execute($patron4->{borrowernumber}, 20, 'edit_public_lists'); # $patron4 only has the edit_public_lists sub-permission checked
>+
> my $biblio1 = $builder->build_sample_biblio;
> my $biblio2 = $builder->build_sample_biblio;
> my $biblio3 = $builder->build_sample_biblio;
> my $biblio4 = $builder->build_sample_biblio;
>+ my $biblio5 = $builder->build_sample_biblio;
> 
> my $public_shelf = Koha::Virtualshelf->new(
> { shelfname => "my first shelf",
>@@ -266,43 +285,62 @@ subtest 'Shelf permissions' => sub {
> public => 1,
> allow_change_from_owner => 0,
> allow_change_from_others => 0,
>+ allow_change_from_staff => 0,
> }
> )->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( $patron2->{borrowernumber} ), 1, 'Public list should be viewed by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Public list should not be deleted by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Public list should not be managed by someone else' );
>+ 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' );
> 
> is( $public_shelf->can_biblios_be_added( $patron1->{borrowernumber} ), 0, 'The owner should not 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 someone else' );
>+ 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} ), 0, 'The owner should not 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 someone else' );
>-
>+ 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 (removed) by someone with no special permissions' );
>+ is( $public_shelf->can_biblios_be_removed( $patron4->{borrowernumber} ), 0, 'Public list should not be modified (removed) by someone with the edit_public_lists sub-permission checked' );
> 
> $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( $patron2->{borrowernumber} ), 1, 'Public list should be viewed by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Public list should not be deleted by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Public list should not be managed by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Public list should not be modified (add) by someone else' );
>-
>- 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( $patron2->{borrowernumber} ), 0, 'Public list should not be modified (remove) by someone else' );
>+ 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 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' );
> 
> my $private_shelf = Koha::Virtualshelf->new(
> { shelfname => "my first shelf",
>@@ -310,43 +348,92 @@ subtest 'Shelf permissions' => sub {
> public => 0,
> allow_change_from_owner => 0,
> allow_change_from_others => 0,
>+ allow_change_from_staff => 0,
> }
> )->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( $patron2->{borrowernumber} ), 0, 'Private list should not be viewed by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Private list should not be deleted by someone else' );
>+ 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( $patron2->{borrowernumber} ), 0, 'Private list should not be managed by someone else' );
>+ 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} ), 0, 'The owner should not be able to add biblios to their list' );
>- is( $private_shelf->can_biblios_be_added( $patron2->{borrowernumber} ), 0, 'Private list should not be modified (add) by someone else' );
>+ is( $private_shelf->can_biblios_be_added( $patron2->{borrowernumber} ), 0, 'Private list should not be modified (add) by another staff member' );
>+ 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} ), 0, 'The owner should not be able to remove biblios to their list' );
>- is( $private_shelf->can_biblios_be_removed( $patron2->{borrowernumber} ), 0, 'Private list should not be modified (remove) by someone else' );
>+ is( $private_shelf->can_biblios_be_removed( $patron2->{borrowernumber} ), 0, 'Private list should not be modified (remove) by another staff member' );
>+ 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-permissions' );
> 
>+ $private_shelf->allow_change_from_owner(1);
>+ $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 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 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 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 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 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' );
> 
> $private_shelf->allow_change_from_owner(1);
> $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( $patron2->{borrowernumber} ), 0, 'Private list should not be viewed by someone else' );
>-
>- is( $private_shelf->can_be_deleted( $patron1->{borrowernumber} ), 1, 'The owner should be able to delete his list' );
>- is( $private_shelf->can_be_deleted( $patron2->{borrowernumber} ), 0, 'Private list should not be deleted by someone else' );
>-
>- is( $private_shelf->can_be_managed( $patron1->{borrowernumber} ), 1, 'The owner should be able to manage his list' );
>- is( $private_shelf->can_be_managed( $patron2->{borrowernumber} ), 0, 'Private list should not be managed by someone else' );
>-
>- 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( $patron2->{borrowernumber} ), 1, 'Private list could be modified (add) by someone else # individual check done later' );
>-
>- 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( $patron2->{borrowernumber} ), 1, 'Private list could be modified (remove) by someone else # individual check done later' );
>+ 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 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 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 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 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' );
> 
> teardown();
> };
>diff --git a/virtualshelves/shelves.pl b/virtualshelves/shelves.pl
>index 0ff8d9d332..a722326ad0 100755
>--- a/virtualshelves/shelves.pl
>+++ b/virtualshelves/shelves.pl
>@@ -41,6 +41,7 @@ use Koha::Patrons;
> use Koha::Virtualshelves;
> 
> use constant ANYONE => 2;
>+use constant STAFF => 3;
> 
> my $query = CGI->new;
> 
>@@ -84,6 +85,7 @@ if ( $op eq 'add_form' ) {
> public => $public,
> allow_change_from_owner => $allow_changes_from > 0,
> allow_change_from_others => $allow_changes_from == ANYONE,
>+ allow_change_from_staff => $allow_changes_from == STAFF,
> owner => scalar $query->param('owner'),
> }
> );
>@@ -113,6 +115,7 @@ if ( $op eq 'add_form' ) {
> my $allow_changes_from = $query->param('allow_changes_from');
> $shelf->allow_change_from_owner( $allow_changes_from > 0 );
> $shelf->allow_change_from_others( $allow_changes_from == ANYONE );
>+ $shelf->allow_change_from_staff( $allow_changes_from == STAFF );
> $shelf->public( scalar $query->param('public') );
> eval { $shelf->store };
> 
>-- 
>2.20.1

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