Bug 18228

Summary: Make list permissions easier to use/understand
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: ListsAssignee: Marcel de Rooy <m.de.rooy>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: aleisha, eric.gosselin.5, jonathan.druart, josef.moravec, katrin.fischer, m.de.rooy, oleonard, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10679
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18214
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10865
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7310
Whiteboard:
Change 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:
Bug Depends on: 18214    
Bug Blocks: 18834    
Attachments: Bug 18228: Table revision of virtualshelves
Bug 18228: DBIx schema changes for Virtualshelve.pm
Bug 18228: Implement the new columns in code
Bug 18228: Adjust Virtualshelves.t
New edit list form
Bug 18228: Adjust Virtualshelves.t
Bug 18228: Table revision of virtualshelves
Bug 18228: DBIx schema changes for Virtualshelve.pm
Bug 18228: Implement the new columns in code
Bug 18228: Adjust Virtualshelves.t
Bug 18228: Add missing comma in kohastructure.sql
Bug 18228: Table revision of virtualshelves
Bug 18228: DBIx schema changes for Virtualshelve.pm
Bug 18228: Implement the new columns in code
Bug 18228: Adjust Virtualshelves.t
Bug 18228: Add missing comma in kohastructure.sql

Description Marcel de Rooy 2017-03-08 13:07:19 UTC

    
Comment 1 Marcel de Rooy 2017-03-09 11:29:26 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-03-09 11:29:52 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-03-09 11:29:56 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-03-09 11:29:59 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-03-09 11:30:03 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-03-09 11:36:38 UTC
Owen and Katrin, please take a look here. Since you were involved on bug 10865, your feedback is appreciated.

Others: please test !
Comment 7 Marcel de Rooy 2017-03-09 11:39:01 UTC
Created attachment 60943 [details]
New edit list form
Comment 8 Eric 2017-03-22 15:27:08 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2017-03-22 15:34:24 UTC
(In reply to Eric from comment #8)
> Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com>

Thanks, Eric.
I only see your signoff on the last patch ?
Comment 10 Jesse Maseto 2017-03-22 16:05:32 UTC
Created attachment 61475 [details] [review]
Bug 18228: Table revision of virtualshelves

In order to make the permissions easier, we will replace the columns
allow_add, allow_delete_own and allow_delete_other by two new columns
allow_change_from_owner and allow_change_from_others.

The distinction between adding or deleting an entry is no longer made.
If you have change permission, you can do both. Also deleting an entry
does no longer depend on who added the entry.
Formerly, the owner could always add entries. It is now possible to
make a list readonly.

We will not use the combination of owner=no and other=yes. This will
leave us three possibilities:
[1] owner=no, other=no: The list is read-only. No one can change
    contents of the list. Naturally, the owner can edit permissions.
[2] owner=yes, other=no: Only the owner can change contents.
[3] owner=yes, other=yes: Anyone seeing the list can change contents.
    This especially applies to shared lists and public lists.

The two database columns will be presented in the interface as one
permission field offering the three abovementioned options.

Test plan:
[1] Run the db rev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 11 Jesse Maseto 2017-03-22 16:05:50 UTC
Created attachment 61476 [details] [review]
Bug 18228: DBIx schema changes for Virtualshelve.pm

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 12 Jesse Maseto 2017-03-22 16:05:54 UTC
Created attachment 61477 [details] [review]
Bug 18228: Implement the new columns in code

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 <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 13 Jesse Maseto 2017-03-22 16:05:59 UTC
Created attachment 61478 [details] [review]
Bug 18228: Adjust Virtualshelves.t

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 14 Marcel de Rooy 2017-03-22 16:27:10 UTC
Thanks Jesse
Comment 15 Jonathan Druart 2017-03-30 19:59:56 UTC
So you are suggesting to... loose features?
Comment 16 Marcel de Rooy 2017-03-31 06:01:11 UTC
(In reply to Jonathan Druart from comment #15)
> So you are suggesting to... loose features?

The title says clearly what it does: Make them easier to use and understand. Bug 10865 is a vivid illustration for that.
The permissions to add or delete are merged into a change permission.
Comment 17 Jonathan Druart 2017-03-31 13:39:20 UTC
If we merge these two permissions, we are limiting the possibilities.
Comment 18 Marcel de Rooy 2017-04-07 09:37:36 UTC
(In reply to Jonathan Druart from comment #17)
> If we merge these two permissions, we are limiting the possibilities.

Will still answer more fully, but the foregoing comments already explain why this is done. Do you have a specific objection?
Comment 19 Jonathan Druart 2017-04-07 13:46:26 UTC
I'd like to get users' opinions before removing features. I have no idea if these permissions are used or not.
I agree that it is not very clear, but we can still make the interface more easier to use/understand instead of remove features.
Comment 20 Owen Leonard 2017-04-10 16:41:18 UTC
From my perspective this simplification will make things easier and doesn't take away any functionality we use.

What is the purpose of the "Allow changes to contents from - Nobody" option? Why is it useful to create a list which you cannot add to, only delete altogether?

The combination of "Category - Private" and "Allow changes to contents from:
-- Anyone seeing this list" is confusing since the staff client lacks the "share" feature. Without knowing that you *could* share the list from the OPAC, allowing others to change a private list doesn't make sense.

It would be nice to get sharing working in the staff client.
Comment 21 Marcel de Rooy 2017-04-11 14:36:25 UTC
(In reply to Owen Leonard from comment #20)
> From my perspective this simplification will make things easier and doesn't
> take away any functionality we use.
> 
> What is the purpose of the "Allow changes to contents from - Nobody" option?
> Why is it useful to create a list which you cannot add to, only delete
> altogether?
> 
> The combination of "Category - Private" and "Allow changes to contents from:
> -- Anyone seeing this list" is confusing since the staff client lacks the
> "share" feature. Without knowing that you *could* share the list from the
> OPAC, allowing others to change a private list doesn't make sense.
> 
> It would be nice to get sharing working in the staff client.

Thanks for responding. Good to read that the simplification is not interpreted as feature loss.

The "Allow changes from Nobody" locks a list: you cannot add, but you cannot delete too. (A contents change is an add or a delete.) Some people may find it comfortable that the list is protected from accidental/incidental changes.

The combination of Private and "Anyone seeing" is indeed a bit confusing (altough still true). Might add a follow-up or a new report for it.

Sharing in the staff client certainly is a new report. I can at least open one to get some more feedback.
Comment 22 Marcel de Rooy 2017-04-11 14:58:33 UTC
(In reply to Jonathan Druart from comment #19)
> I'd like to get users' opinions before removing features. I have no idea if
> these permissions are used or not.
> I agree that it is not very clear, but we can still make the interface more
> easier to use/understand instead of remove features.

In response to comment17 and comment19 I wrote the Koha mailing list and asked for feedback on this report.
Comment 23 Jonathan Druart 2017-04-11 19:29:16 UTC
Basically with this patch we are going to remove the "Let people adds stuffs to this list but do not let them remove stuffs from this list (unless added by themself)" option.

If nobody uses it, this patch makes code and interface much more easy to read/understand.
Comment 24 Marcel de Rooy 2017-04-18 14:12:37 UTC
(In reply to Marcel de Rooy from comment #22)
> In response to comment17 and comment19 I wrote the Koha mailing list and
> asked for feedback on this report.

Second call on April 18
Comment 25 Marc VĂ©ron 2017-04-18 18:29:32 UTC
(In reply to Jonathan Druart from comment #23)
> Basically with this patch we are going to remove the "Let people adds stuffs
> to this list but do not let them remove stuffs from this list (unless added
> by themself)" option.
> 
> If nobody uses it, this patch makes code and interface much more easy to
> read/understand.

Agreed.
Comment 26 Josef Moravec 2017-04-26 11:17:37 UTC
From my point of view this does change the behaviour in the meaningful way, so I am OK with it ;)
Comment 27 Marcel de Rooy 2017-05-08 10:21:41 UTC
Can we move this further?
Comment 28 Jonathan Druart 2017-05-08 17:04:13 UTC
All looks good to me unless that it still sounds weird to loose this feature.
Comment 29 Marcel de Rooy 2017-05-15 08:23:45 UTC
(In reply to Jonathan Druart from comment #28)
> All looks good to me unless that it still sounds weird to loose this feature.

You seem to be the only one that cares about it. What can I still do?
Comment 30 Jonathan Druart 2017-05-15 13:43:20 UTC
Nothing, but I would wait for the next release to push. Feature freeze is passed anyway.
Comment 31 Jonathan Druart 2017-05-15 13:53:34 UTC
Created attachment 63474 [details] [review]
Bug 18228: Add missing comma in kohastructure.sql
Comment 32 Marcel de Rooy 2017-06-21 12:43:23 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2017-06-22 07:13:44 UTC
Rebasing this on top of 18214
Comment 34 Marcel de Rooy 2017-06-22 07:18:33 UTC
Created attachment 64505 [details] [review]
Bug 18228: Table revision of virtualshelves

In order to make the permissions easier, we will replace the columns
allow_add, allow_delete_own and allow_delete_other by two new columns
allow_change_from_owner and allow_change_from_others.

The distinction between adding or deleting an entry is no longer made.
If you have change permission, you can do both. Also deleting an entry
does no longer depend on who added the entry.
Formerly, the owner could always add entries. It is now possible to
make a list readonly.

We will not use the combination of owner=no and other=yes. This will
leave us three possibilities:
[1] owner=no, other=no: The list is read-only. No one can change
    contents of the list. Naturally, the owner can edit permissions.
[2] owner=yes, other=no: Only the owner can change contents.
[3] owner=yes, other=yes: Anyone seeing the list can change contents.
    This especially applies to shared lists and public lists.

The two database columns will be presented in the interface as one
permission field offering the three abovementioned options.

Test plan:
[1] Run the db rev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 35 Marcel de Rooy 2017-06-22 07:18:38 UTC
Created attachment 64506 [details] [review]
Bug 18228: DBIx schema changes for Virtualshelve.pm

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 36 Marcel de Rooy 2017-06-22 07:18:43 UTC
Created attachment 64507 [details] [review]
Bug 18228: Implement the new columns in code

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 <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 37 Marcel de Rooy 2017-06-22 07:18:48 UTC
Created attachment 64508 [details] [review]
Bug 18228: Adjust Virtualshelves.t

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 38 Marcel de Rooy 2017-06-22 07:18:52 UTC
Created attachment 64509 [details] [review]
Bug 18228: Add missing comma in kohastructure.sql

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2017-06-22 08:04:41 UTC
(In reply to Owen Leonard from comment #20)
> The combination of "Category - Private" and "Allow changes to contents from:
> -- Anyone seeing this list" is confusing since the staff client lacks the
> "share" feature. Without knowing that you *could* share the list from the
> OPAC, allowing others to change a private list doesn't make sense.

On bug 18834 I am removing the Anyone permission for strictly private lists and in staff I am adding a Type column that shows Shared for private lists with shares just like OPAC does already.
Comment 40 Marcel de Rooy 2017-06-22 09:40:07 UTC
Just a summary of the current status:

Although Jonathan planned to push this patch set shortly after release of 17.05, he still feels that it needs another QA opinion.
As can be seen in the previous comments, I asked feedback (repeatedly) on the community and the dev mailing list. Apart from the signoff by Jesse M and Eric G, positive comments have been added by Owen L, Marc V and Josef M. The only concern is from Jonathan about "losing a feature".

In short, this report simplifies the scheme of three options Allow add, Allow delete own entries and Allow delete other entries (with 2^3=8 possible combinations) by one option "Allow changes from" with three choices: Nobody, Owner or "Anyone seeing the list".

What we actually remove, is the distinction between Add and Delete. We replace it by Change. If you are allowed to change, you can add and/or delete entries. The concept is easier and nobody expressed interest in keeping it. Allowing only adds or only deletes was mostly theoretically interesting and probably not used.

The distinction between Delete own and Delete other was confusing, since Koha does not show who added an entry. It is not that interesting too probably. If you are allowed to change, you can delete any entry now.

Additionally, we now have the Nobody option. This allows you to make a list read-only. Even the owner cannot change. This may prevent accidental deletes.

Bug 18834 builds on top of this patch set and hides the Anyone permission for strictly private lists. Dynamically alters the permission combo. Fixes a bug for empty shelf name. And adds a Shared indication for a list in staff.

Please help to get this further.