Bug 10679 - Lists interface offers delete option when user doesn't have permission
Summary: Lists interface offers delete option when user doesn't have permission
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-02 19:08 UTC by Owen Leonard
Modified: 2017-03-09 11:31 UTC (History)
2 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10679: Hide delete option for lists if user does not have permission (1.73 KB, patch)
2017-02-15 01:03 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 10679: Hide delete option for lists if user does not have permission and has not added any items to that list (2.15 KB, patch)
2017-02-16 20:43 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2013-08-02 19:08:49 UTC
In both the OPAC and the staff client a "allowremovingitems" variable is used in the template to determine whether the logged in user should be able to delete items from a list. In both the OPAC and staff client this variable is true when it should be false. Fortunately the user can't actually remove something from the list. Instead the page returns an error message. However, the "remove" option should not be visible in that case.

To confirm this bug, log in to the OPAC or staff client and open a public list which was created by another user. The "remove from list" option will be available but submitting the form will return an error like "You could not delete any selected items from this shelf."
Comment 1 Marcel de Rooy 2013-08-04 20:04:13 UTC
Just from recall: Isn't it so that the variable is set to true (and the remove button is displayed) as long as it is possible that this user might be able to delete at least some items of this list?
The permissions now differentiate deleting items you added yourself and items of someone else. It would not be very effective to do this check on item level every time you display a list?
BICBW..
Comment 2 Owen Leonard 2013-08-05 12:59:37 UTC
(In reply to M. de Rooy from comment #1)
> Isn't it so that the variable is set to true
> as long as it is possible that this user might
> be able to delete at least some items of this list?

Why would it be possible that I might be able to delete some items from a list which I didn't create and which wasn't created with permission for others to add items?
Comment 3 Marcel de Rooy 2013-08-05 14:55:49 UTC
(In reply to Owen Leonard from comment #2)
> Why would it be possible that I might be able to delete some items from a
> list which I didn't create and which wasn't created with permission for
> others to add items?

Just thinking: Permissions can be changed at any time. So if I just disallowed adding items by others on my list, there could still be items added earlier on?
Comment 4 Aleisha Amohia 2017-02-15 01:03:18 UTC
Created attachment 60228 [details] [review]
Bug 10679: Hide delete option for lists if user does not have permission

The reason the delete button was still showing is because, by default,
we allow anyone to remove THEIR OWN contributions from a list. So, the
delete button will always show. This patch makes sure that the delete
button will only show if permission for anyone else to add or delete
their own entries is granted.

To test:
1) Make a public list setting all permissions to 'do not allow' EXCEPT
'remove own contributed entries' (set this to Allow). This should be
done by default, but just make sure
2) Log in as a different user and go to that list. notice the 'remove
selected' button shows, but attempting to delete any records fails.
3) Apply patch and refresh the page
4) Notice that the delete button is now gone.
5) Log back in as the list owner and set the first two permissions to
'Allow' (add entries, remove their own contributed entries)
6) Log back in as a different user and go to the list. The delete button
should now be showing and work as expected.

Sponsored-by: Catalyst IT
Comment 5 Marcel de Rooy 2017-02-16 08:26:33 UTC
(In reply to Aleisha Amohia from comment #4)
> The reason the delete button was still showing is because, by default,
> we allow anyone to remove THEIR OWN contributions from a list. So, the
> delete button will always show. This patch makes sure that the delete
> button will only show if permission for anyone else to add or delete
> their own entries is granted.

The permission to add is not relevant here.
The only way to make sure if you can delete is check permissions and optionally check if loggedinuser 'owns' entries on that list (or perhaps on the page we show).

> To test:
> 1) Make a public list setting all permissions to 'do not allow' EXCEPT
> 'remove own contributed entries' (set this to Allow). This should be
> done by default, but just make sure
> 2) Log in as a different user and go to that list. notice the 'remove
> selected' button shows, but attempting to delete any records fails.

This assumption is not correct. If I added an entry to a public list and the 
owner later sets the list to 'D-A-D', I should still be able to delete my entry. But you removed the button !
Comment 6 Aleisha Amohia 2017-02-16 20:43:55 UTC
Created attachment 60364 [details] [review]
Bug 10679: Hide delete option for lists if user does not have permission and has not added any items to that list

Taking from Comment 5, if a list once had permissions for people to add
and remove entries, but then later the owner changes it so that others
can no longer add entries, users should be able to delete the entries
they added previously.

This patch checks that the logged in user is the same account as the
borrower who added the entry to the list. If they are, the button to
remove should show. Attempting to delete any records they did not add
will fail, but attempting to delete the records they did add will
succeed. If they did not add any records to a list, the button to remove
will not show.

To test:
1) Make TWO public lists. Set all permissions to 'Allow' on one list and
all permissions to 'Do not allow' on the other list.
2) Add one or two items to both lists.
3) Log in as a different user. Add one or two items to the list that you
have permission to add to.
4) Log back in as the original user and set all permissions on the
'Allow' list to 'Do not allow'
5) Log back in as the different user. Go to the list you added entries
to
6) The button to remove should show and you should be able to delete
your entries only.
7) There should be no button to remove on the other list that you have
added nothing to.

Sponsored-by: Catalyst IT
Comment 7 Marcel de Rooy 2017-02-17 07:02:31 UTC
We're not there yet. But we will!
Will submit a more detailed comment later.
Comment 8 Marcel de Rooy 2017-02-17 10:54:57 UTC
Instead of looping through the search results of Koha::Virtualshelfcontents->search({ shelfnumber => $self->shelfnumber })
please search for the column borrowernumber too.

The algorithm should be something as:

if list->count==0 return false
if owner return true
if delete_own AND delete_other return true (in that case you could delete all)
if delete_own AND there are 'own' entries return true (so search here!)
if delete_other AND there are 'other' entries (count - countown > 0) then return true

Maybe you understand now why I did not write that code :)
We should perhaps consider: show the button Remove selected only if you are the owner or allowed to delete all entries. And show a link on the individual entries if this is not the case but you can delete an individual one. Because if you see the button, it could well be that you are only allowed to delete one record out of 20. Which one?
So you would need a routine can_biblio_be_removed (singular)

Or simplify the permissions: Add/Delete on owner level (or staff member, moderator?) and Add/Delete for other users.
Comment 9 Marcel de Rooy 2017-03-09 11:31:20 UTC
Please look at my proposal on bug 18228 to make things a bit easier.