From ea8e4cbb564bb0ae9403e4be5f2c6b9b77fcfedb Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 8 Mar 2017 15:31:25 +0100 Subject: [PATCH] Bug 18228: Implement the new columns in code Content-Type: text/plain; charset=utf-8 The two new columns as mentioned in the commit message of the table revision must be used in the codebase now. Highlighting some changes in Koha::VirtualShel[f|ves]: [1] Additional methods is_public and is_private. [2] Method add_biblio did not check permissions. Does now. No impact on the interface, but one call in the unit test was affected. [3] Method remove_biblios is signficantly simplified. Removed a FIXME. [4] Method can_biblios_be_removed now redirects to can_biblios_be_added. A followup report may deal with unifying those routines. [5] Condition in get_some_shelves changed. [6] The reference to allow_add in get_shelves_containing_record can simply be removed. opac-shelves.pl and shelves.pl now pass the default setting of Owner only to the template. Templates shelves.tt and opac-shelves.tt now include the new permission field with three choices as mentioned in the table revision patch. opac-addbybiblionumber.pl and addbybiblionumber now need a check on allow_change_from_owner; search conditions slightly adjusted to the new permission scheme. Test plan: When we refer to visibility in the test plan, please check the Add to-combo on opac search results and staff results. And check opac-addbybiblionumber by clicking Save to Lists from opac results. The step 'Check delete' means: open the list in opac and check if you see the Delete button below the entries (only check, do not delete). [ 1] Create private list I01 (perm=Owner) [ 2] Check visibility: Seen. [ 3] Add a book. (Change by owner should be allowed.) [ 4] Check delete: Yes. [ 5] Edit list I01, set perm=Nobody [ 6] Check visibility: Not seen. [ 7] Check delete: No. [ 8] Share list I01 with another patron. [ 9] Check visibility for the other patron: Not seen. [10] Check delete for the other patron: No. [11] Change permission of list I01 to Anyone (by owner). [12] Check visibility for the other patron: Seen. [13] Let other patron add a book (change is allowed). [14] Let owner delete the same book again (change allowed). [15] Create public list U01 (perm=Owner) [16] Check visibility: Seen. [17] Add a book. (Change by owner should be allowed.) [18] Login as other user. Check visibility: Not seen. Check delete: No. [19] Change permission of U01 to Nobody (by owner) [20] As owner: Check visibility: Not seen. Check delete: No. [21] As other user: Check visibility: Not seen. Check delete: No. [22] Create public list U02 (perm=Anyone) [23] Add a book by owner. [24] Delete the same book by other user. Add another book. Signed-off-by: Marcel de Rooy --- Koha/Virtualshelf.pm | 74 +++++++++------------- Koha/Virtualshelves.pm | 12 ++-- .../prog/en/modules/virtualshelves/shelves.tt | 35 +++------- .../opac-tmpl/bootstrap/en/modules/opac-shelves.tt | 42 ++++-------- opac/opac-addbybiblionumber.pl | 17 ++--- opac/opac-shelves.pl | 17 +++-- virtualshelves/addbybiblionumber.pl | 17 ++--- virtualshelves/shelves.pl | 17 +++-- 8 files changed, 98 insertions(+), 133 deletions(-) diff --git a/Koha/Virtualshelf.pm b/Koha/Virtualshelf.pm index 4e45c31..a914e4a 100644 --- a/Koha/Virtualshelf.pm +++ b/Koha/Virtualshelf.pm @@ -60,18 +60,26 @@ sub store { Koha::Exceptions::Virtualshelves::DuplicateObject->throw; } - $self->allow_add( 0 ) - unless defined $self->allow_add; - $self->allow_delete_own( 1 ) - unless defined $self->allow_delete_own; - $self->allow_delete_other( 0 ) - unless defined $self->allow_delete_other; + $self->allow_change_from_owner( 1 ) + unless defined $self->allow_change_from_owner; + $self->allow_change_from_others( 0 ) + unless defined $self->allow_change_from_others; $self->created_on( dt_from_string ); return $self->SUPER::store( $self ); } +sub is_public { + my ( $self ) = @_; + return $self->category == $PUBLIC; +} + +sub is_private { + my ( $self ) = @_; + return $self->category == $PRIVATE; +} + sub is_shelfname_valid { my ( $self ) = @_; @@ -80,14 +88,14 @@ sub is_shelfname_valid { ( $self->shelfnumber ? ( "me.shelfnumber" => { '!=', $self->shelfnumber } ) : () ), }; - if ( $self->category == $PRIVATE and defined $self->owner ) { + if ( $self->is_private and defined $self->owner ) { $conditions->{-or} = { "virtualshelfshares.borrowernumber" => $self->owner, "me.owner" => $self->owner, }; $conditions->{category} = $PRIVATE; } - elsif ( $self->category == $PRIVATE and not defined $self->owner ) { + elsif ( $self->is_private and not defined $self->owner ) { $conditions->{owner} = undef; $conditions->{category} = $PRIVATE; } @@ -174,6 +182,10 @@ sub add_biblio { } )->count; return if $already_exists; + + # Check permissions + return unless ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) || $self->allow_change_from_others; + my $content = Koha::Virtualshelfcontent->new( { shelfnumber => $self->shelfnumber, @@ -194,38 +206,18 @@ sub remove_biblios { return unless @$biblionumbers; my $number_removed = 0; - for my $biblionumber ( @$biblionumbers ) { - if ( $self->owner == $borrowernumber or $self->allow_delete_own ) { - $number_removed += $self->get_contents->search( - { - biblionumber => $biblionumber, - borrowernumber => $borrowernumber, - } - )->delete; - } - if ( $self->allow_delete_other ) { - $number_removed += $self->get_contents->search( - { - biblionumber => $biblionumber, - # FIXME - # This does not make sense, but it's has been backported from DelFromShelf. - # Why shouldn't we allow to remove his own contribution if allow_delete_other is on? - borrowernumber => { - -or => { - '!=' => $borrowernumber, - '=' => undef - } - }, - } - )->delete; - } + if( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) + || $self->allow_change_from_others ) { + $number_removed += $self->get_contents->search({ + biblionumber => $biblionumbers, + })->delete; } return $number_removed; } sub can_be_viewed { my ( $self, $borrowernumber ) = @_; - return 1 if $self->category == $PUBLIC; + return 1 if $self->is_public; return 0 unless $borrowernumber; return 1 if $self->owner == $borrowernumber; return $self->get_shares->search( @@ -243,7 +235,7 @@ sub can_be_deleted { my $patron = Koha::Patrons->find( $borrowernumber ); - return 1 if $self->category == $PUBLIC and C4::Auth::haspermission( $patron->userid, { lists => 'delete_public_lists' } ); + return 1 if $self->is_public and C4::Auth::haspermission( $patron->userid, { lists => 'delete_public_lists' } ); return 0; } @@ -260,20 +252,14 @@ sub can_biblios_be_added { return 1 if $borrowernumber - and ( $self->owner == $borrowernumber - or $self->allow_add ); + and ( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) or $self->allow_change_from_others ); return 0; } sub can_biblios_be_removed { my ( $self, $borrowernumber ) = @_; - - return 1 - if $borrowernumber - and ( $self->owner == $borrowernumber - or $self->allow_delete_own - or $self->allow_delete_other ); - return 0; + return $self->can_biblios_be_added( $borrowernumber ); + # Same answer since bug 18228 } sub _type { diff --git a/Koha/Virtualshelves.pm b/Koha/Virtualshelves.pm index a8d1b20..195376c 100644 --- a/Koha/Virtualshelves.pm +++ b/Koha/Virtualshelves.pm @@ -90,10 +90,13 @@ sub get_some_shelves { if ( $add_allowed ) { push @conditions, { -or => - { - "me.allow_add" => 1, - "me.owner" => $borrowernumber, - } + [ + { + "me.owner" => $borrowernumber, + "me.allow_change_from_owner" => 1, + }, + "me.allow_change_from_others" => 1, + ] }; } if ( $category == 1 ) { @@ -135,7 +138,6 @@ sub get_shelves_containing_record { 'me.owner' => $borrowernumber, -or => { 'virtualshelfshares.borrowernumber' => $borrowernumber, - "me.allow_add" => 1, }, } }, 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 2044754..d28250a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt @@ -299,13 +299,13 @@ function placeHold () { [% ELSE %] Lists [% END %] - [% IF shelf AND shelf.category == PRIVATE %] › + [% IF shelf AND shelf.is_private %] › [% IF op == 'view' OR op == 'edit_form' %] Your lists [% ELSE %] Your lists [% END %] - [% ELSIF shelf AND shelf.category == PUBLIC %] › + [% ELSIF shelf AND shelf.is_public %] › [% IF op == 'view' %] Public lists [% ELSE %] @@ -537,12 +537,12 @@ function placeHold () {
  • - - + [% IF !shelf.allow_change_from_owner %][% ELSE %][% END %] + [% IF shelf.allow_change_from_owner && !shelf.allow_change_from_others %][% ELSE %][% END %] + [% IF shelf.allow_change_from_owner && shelf.allow_change_from_others %][% ELSE %][% END %] -  anyone else to add entries. -
  • -
  • - - -  anyone to remove their own contributed entries. -
  • -
  • - - -  anyone to remove other contributed entries.
  • + 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 0e20fcf..2c162c0 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt @@ -12,28 +12,12 @@ [% BLOCK list_permissions %]
  • - - + [% IF !shelf.allow_change_from_owner %][% ELSE %][% END %] + [% IF shelf.allow_change_from_owner && !shelf.allow_change_from_others %][% ELSE %][% END %] + [% IF shelf.allow_change_from_owner && shelf.allow_change_from_others %][% ELSE %][% END %] -  anyone else to add entries. -
  • -
  • - - -  anyone to remove their own contributed entries. -
  • -
  • - - -  anyone to remove other contributed entries.
  • [% END %] @@ -51,13 +35,13 @@
  • Lists
  • [% END %] - [% IF shelf and shelf.category == PRIVATE %] + [% IF shelf and shelf.is_private %] [% IF op == 'view' OR op == 'edit_form' %]
  • Your lists
  • [% ELSE %]
  • Your lists
  • [% END %] - [% ELSIF shelf AND shelf.category == PUBLIC %] + [% ELSIF shelf AND shelf.is_public %] [% IF op == 'view' %]
  • Public lists
  • [% ELSE %] @@ -586,23 +570,23 @@
  • - [% IF shelf.category == PUBLIC AND NOT Koha.Preference('OpacAllowPublicListCreation') %] + [% IF shelf.is_public AND NOT Koha.Preference('OpacAllowPublicListCreation') %] The library has disabled the ability for patrons to create new public lists. If you make your list private, you will not be able to make it public again. [% END %]
  • - [% INCLUDE list_permissions %] [% END %] + [% INCLUDE list_permissions %] [% UNLESS Koha.Preference('OpacAllowPublicListCreation') OR category == PUBLIC %] @@ -669,7 +653,7 @@ [% s.shelfname |html %] [% IF contents.count %][% contents.count %] [% IF contents.count == 1 %]item[% ELSE %]items[% END %][% ELSE %]Empty[% END %] - [% IF s.category == PRIVATE %] + [% IF s.is_private %] [% IF s.is_shared %]Shared[% ELSE %]Private[% END %] [% ELSE %] Public @@ -694,7 +678,7 @@ [% END %] - [% IF s.category == PRIVATE AND s.can_be_managed( loggedinusernumber ) AND Koha.Preference('OpacAllowSharingPrivateLists') %] + [% IF s.is_private AND s.can_be_managed( loggedinusernumber ) AND Koha.Preference('OpacAllowSharingPrivateLists') %] Share [% END %] [% IF s.is_shared_with( loggedinusernumber ) %] diff --git a/opac/opac-addbybiblionumber.pl b/opac/opac-addbybiblionumber.pl index 9227fa0..ddebf4f 100755 --- a/opac/opac-addbybiblionumber.pl +++ b/opac/opac-addbybiblionumber.pl @@ -107,25 +107,26 @@ if ($newvirtualshelf) { my $private_shelves = Koha::Virtualshelves->search( { category => 1, owner => $loggedinuser, + allow_change_from_owner => 1, }, { order_by => 'shelfname' } ); my $shelves_shared_with_me = Koha::Virtualshelves->search( { category => 1, 'virtualshelfshares.borrowernumber' => $loggedinuser, - -or => { - allow_add => 1, - owner => $loggedinuser, - } + allow_change_from_others => 1, }, { join => 'virtualshelfshares', } ); my $public_shelves = Koha::Virtualshelves->search( { category => 2, - -or => { - allow_add => 1, - owner => $loggedinuser, - } + -or => [ + -and => { + allow_change_from_owner => 1, + owner => $loggedinuser, + }, + allow_change_from_others => 1, + ], }, { order_by => 'shelfname' } ); diff --git a/opac/opac-shelves.pl b/opac/opac-shelves.pl index 3240506..aa004c3 100755 --- a/opac/opac-shelves.pl +++ b/opac/opac-shelves.pl @@ -33,6 +33,8 @@ use Koha::Biblioitems; use Koha::Virtualshelves; use Koha::RecordProcessor; +use constant ANYONE => 2; + my $query = new CGI; my $template_name = $query->param('rss') ? "opac-shelves-rss.tt" : "opac-shelves.tt"; @@ -56,7 +58,8 @@ my $category = $query->param('category') || 1; my ( $shelf, $shelfnumber, @messages ); if ( $op eq 'add_form' ) { - # Nothing to do + # Only pass default + $shelf = { allow_change_from_owner => 1 }; } elsif ( $op eq 'edit_form' ) { $shelfnumber = $query->param('shelfnumber'); $shelf = Koha::Virtualshelves->find($shelfnumber); @@ -74,14 +77,14 @@ if ( $op eq 'add_form' ) { } } elsif ( $op eq 'add' ) { if ( $loggedinuser ) { + my $allow_changes_from = $query->param('allow_changes_from'); eval { $shelf = Koha::Virtualshelf->new( { shelfname => scalar $query->param('shelfname'), sortfield => scalar $query->param('sortfield'), category => scalar $query->param('category') || 1, - allow_add => scalar $query->param('allow_add'), - allow_delete_own => scalar $query->param('allow_delete_own'), - allow_delete_other => scalar $query->param('allow_delete_other'), + allow_change_from_owner => $allow_changes_from > 0, + allow_change_from_others => $allow_changes_from == ANYONE, owner => scalar $loggedinuser, } ); @@ -110,9 +113,9 @@ if ( $op eq 'add_form' ) { if ( $shelf->can_be_managed( $loggedinuser ) ) { $shelf->shelfname( scalar $query->param('shelfname') ); $shelf->sortfield( $sortfield ); - $shelf->allow_add( scalar $query->param('allow_add') ); - $shelf->allow_delete_own( scalar $query->param('allow_delete_own') ); - $shelf->allow_delete_other( scalar $query->param('allow_delete_other') ); + 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->category( scalar $query->param('category') ); eval { $shelf->store }; diff --git a/virtualshelves/addbybiblionumber.pl b/virtualshelves/addbybiblionumber.pl index 6e058e9..210e153 100755 --- a/virtualshelves/addbybiblionumber.pl +++ b/virtualshelves/addbybiblionumber.pl @@ -152,25 +152,26 @@ if ($newvirtualshelf) { my $private_shelves = Koha::Virtualshelves->search( { category => 1, owner => $loggedinuser, + allow_change_from_owner => 1, }, { order_by => 'shelfname' } ); my $shelves_shared_with_me = Koha::Virtualshelves->search( { category => 1, 'virtualshelfshares.borrowernumber' => $loggedinuser, - -or => { - allow_add => 1, - owner => $loggedinuser, - } + allow_change_from_others => 1, }, { join => 'virtualshelfshares', } ); my $public_shelves = Koha::Virtualshelves->search( { category => 2, - -or => { - allow_add => 1, - owner => $loggedinuser, - } + -or => [ + -and => { + allow_change_from_owner => 1, + owner => $loggedinuser, + }, + allow_change_from_others => 1, + ], }, { order_by => 'shelfname' } ); diff --git a/virtualshelves/shelves.pl b/virtualshelves/shelves.pl index c328869..d4a9730 100755 --- a/virtualshelves/shelves.pl +++ b/virtualshelves/shelves.pl @@ -32,6 +32,8 @@ use Koha::Biblioitems; use Koha::CsvProfiles; use Koha::Virtualshelves; +use constant ANYONE => 2; + my $query = new CGI; my ( $template, $loggedinuser, $cookie ) = get_template_and_user( @@ -49,7 +51,8 @@ my $category = $query->param('category') || 1; my ( $shelf, $shelfnumber, @messages ); if ( $op eq 'add_form' ) { - # Nothing to do + # Only pass default + $shelf = { allow_change_from_owner => 1 }; } elsif ( $op eq 'edit_form' ) { $shelfnumber = $query->param('shelfnumber'); $shelf = Koha::Virtualshelves->find($shelfnumber); @@ -66,14 +69,14 @@ if ( $op eq 'add_form' ) { push @messages, { type => 'alert', code => 'does_not_exist' }; } } elsif ( $op eq 'add' ) { + my $allow_changes_from = $query->param('allow_changes_from'); eval { $shelf = Koha::Virtualshelf->new( { shelfname => scalar $query->param('shelfname'), sortfield => scalar $query->param('sortfield'), category => scalar $query->param('category'), - allow_add => scalar $query->param('allow_add'), - allow_delete_own => scalar $query->param('allow_delete_own'), - allow_delete_other => scalar $query->param('allow_delete_other'), + allow_change_from_owner => $allow_changes_from > 0, + allow_change_from_others => $allow_changes_from == ANYONE, owner => scalar $query->param('owner'), } ); @@ -100,9 +103,9 @@ if ( $op eq 'add_form' ) { if ( $shelf->can_be_managed( $loggedinuser ) ) { $shelf->shelfname( scalar $query->param('shelfname') ); $shelf->sortfield( $sortfield ); - $shelf->allow_add( scalar $query->param('allow_add') ); - $shelf->allow_delete_own( scalar $query->param('allow_delete_own') ); - $shelf->allow_delete_other( scalar $query->param('allow_delete_other') ); + 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->category( scalar $query->param('category') ); eval { $shelf->store }; -- 2.1.4