Bug 18834

Summary: Show "Anyone seeing this list" permission only for shared and public lists
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: ListsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart+koha, m.de.rooy
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10865
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18838
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18980
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18228    
Bug Blocks:    
Attachments: Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists
Bug 18834: Add js to dynamically alter the permission options
Bug 18834: Remove js function Check from opac-shelves
Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists
Bug 18834: Add js to dynamically alter the permission options
Bug 18834: Remove js function Check from opac-shelves
Bug 18834: Show distinction between shared and private lists in staff
Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists
Bug 18834: Add js to dynamically alter the permission options
Bug 18834: Remove js function Check from opac-shelves
Bug 18834: Show distinction between shared and private lists in staff
Bug 18834: Show distinction between shared and private lists in staff
Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists
Bug 18834: Add js to dynamically alter the permission options
Bug 18834: Remove js function Check from opac-shelves
Bug 18834: Show distinction between shared and private lists in staff

Description Marcel de Rooy 2017-06-21 12:58:44 UTC
This is a follow-up of report 18228 that simplifies the permissions for lists.
It reapplies the idea of bug 10865, but with one difference: It does not completely hide all permissions if adding public or shared lists is not allowed; it only hides the Anyone-permission if it is not relevant.

Note: The Anyone permission is not relevant if it is a private list that has no shares. Note that looking at the prefs OpacAllowPublicListCreation and/or OpacAllowSharingPrivateLists is not decisive. You should look at list permissions and shares in the database; turning off the Sharing pref does not automatically remove all shares in the system. It only blocks creating new shares.
Comment 1 Marcel de Rooy 2017-06-21 15:00:43 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-06-21 15:00:47 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-06-21 15:00:51 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-06-22 07:23:49 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-06-22 07:23:53 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-06-22 07:23:57 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-06-22 07:24:21 UTC
Rebased (see 18214)
Comment 8 Marcel de Rooy 2017-06-22 08:17:21 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2017-06-22 08:18:13 UTC
Created attachment 64521 [details] [review]
Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists

This is a follow-up of report 18228 that simplifies the permissions for
lists. It reapplies the idea of bug 10865, but with one difference: It does
not completely hide all permissions if adding public or shared lists is not
allowed; it only hides the Anyone-permission if it is not relevant.

Note: The Anyone permission is not relevant if it is a private list that
has no shares. Note that looking at the prefs OpacAllowPublicListCreation
and/or OpacAllowSharingPrivateLists is not decisive. You should look at
list permissions and shares in the database; turning off the Sharing pref
does not automatically remove all shares in the system. It only blocks
creating new shares.

We only need changes in opac-shelves.tt and virtualshelves/shelves.tt.

Test plan:
[1] Disable OpacAllowPublicListCreation and OpacAllowSharingPrivateLists.
[2] Edit a public list in staff view. Toggle permissions, save and reopen
    to check the value you saved. Is Anyone permission included?
[3] Edit a private list (without shares) in staff. Is Anyone excluded?
[4] Edit the same private list in OPAC. Is Anyone excluded too?
[5] Enable OpacAllowPublicListCreation. Create a public list in OPAC.
[6] Edit this list in OPAC. Do you see Anyone? Save with Anyone.
[7] Change category to Private. Save and reopen. Is Owner selected now,
    and Anyone excluded?
[8] Enable sharing. Share a list and accept it with another user.
    [You can also manually insert a record in virtualshelfshares.]
    Edit the list in OPAC as owner. Do you see Anyone?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-06-22 08:18:17 UTC
Created attachment 64522 [details] [review]
Bug 18834: Add js to dynamically alter the permission options

If we we move from Private to Public or vice versa, this actually has an
impact on the permissions combo. If we go back to a private list without
shares, we could remove Anyone. If we switch to public and the list had
no shares, we could add Anyone.

Handled in a js sub AdjustPerms, triggered by onchange of the category
combo.

Test plan:
[1] Edit a private list without shares in OPAC. Toggle category. Check
    if the permissions combo changes accordingly.
[2] Edit a public list in staff. Toggle category and check.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-06-22 08:18:20 UTC
Created attachment 64523 [details] [review]
Bug 18834: Remove js function Check from opac-shelves

This function only serves to check if the shelf name is not empty. It
does not even work since it incorrectly refers to f.addshelf (while it
should test #shelfname).
We can solve this and improve consistency by doing the same as in the
staff template with html5 required.

Test plan:
[1] Without this patch it was possible to add a list without a name in
    OPAC (with js error: TypeError: f.addshelf is undefined).
[2] With this patch, verify that you cannot do this anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-06-22 08:18:23 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-06-22 08:21:54 UTC
Created attachment 64525 [details] [review]
Bug 18834: Show distinction between shared and private lists in staff

Just as we show this distinction in OPAC, this patch adds a type column
in the Your lists tab that displays Private or Shared. It always contains
Public in the other tab.

Test plan:
[1] Check if you see Shared for a private lists with shares in staff.
[2] Run t/db_dependent/Utils/Datatables_Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Lee Jamison 2017-06-22 14:32:10 UTC
Created attachment 64545 [details] [review]
Bug 18834: Show "Anyone seeing this list" permission only for shared and public lists

This is a follow-up of report 18228 that simplifies the permissions for
lists. It reapplies the idea of bug 10865, but with one difference: It does
not completely hide all permissions if adding public or shared lists is not
allowed; it only hides the Anyone-permission if it is not relevant.

Note: The Anyone permission is not relevant if it is a private list that
has no shares. Note that looking at the prefs OpacAllowPublicListCreation
and/or OpacAllowSharingPrivateLists is not decisive. You should look at
list permissions and shares in the database; turning off the Sharing pref
does not automatically remove all shares in the system. It only blocks
creating new shares.

We only need changes in opac-shelves.tt and virtualshelves/shelves.tt.

Test plan:
[1] Disable OpacAllowPublicListCreation and OpacAllowSharingPrivateLists.
[2] Edit a public list in staff view. Toggle permissions, save and reopen
    to check the value you saved. Is Anyone permission included?
[3] Edit a private list (without shares) in staff. Is Anyone excluded?
[4] Edit the same private list in OPAC. Is Anyone excluded too?
[5] Enable OpacAllowPublicListCreation. Create a public list in OPAC.
[6] Edit this list in OPAC. Do you see Anyone? Save with Anyone.
[7] Change category to Private. Save and reopen. Is Owner selected now,
    and Anyone excluded?
[8] Enable sharing. Share a list and accept it with another user.
    [You can also manually insert a record in virtualshelfshares.]
    Edit the list in OPAC as owner. Do you see Anyone?

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

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Works as directed in the test plan. updatedatabase.pl must be
run in order for Datatables_Virtualshelves.t to pass
Comment 15 Lee Jamison 2017-06-22 14:32:26 UTC
Created attachment 64546 [details] [review]
Bug 18834: Add js to dynamically alter the permission options

If we we move from Private to Public or vice versa, this actually has an
impact on the permissions combo. If we go back to a private list without
shares, we could remove Anyone. If we switch to public and the list had
no shares, we could add Anyone.

Handled in a js sub AdjustPerms, triggered by onchange of the category
combo.

Test plan:
[1] Edit a private list without shares in OPAC. Toggle category. Check
    if the permissions combo changes accordingly.
[2] Edit a public list in staff. Toggle category and check.

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

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 16 Lee Jamison 2017-06-22 14:32:30 UTC
Created attachment 64547 [details] [review]
Bug 18834: Remove js function Check from opac-shelves

This function only serves to check if the shelf name is not empty. It
does not even work since it incorrectly refers to f.addshelf (while it
should test #shelfname).
We can solve this and improve consistency by doing the same as in the
staff template with html5 required.

Test plan:
[1] Without this patch it was possible to add a list without a name in
    OPAC (with js error: TypeError: f.addshelf is undefined).
[2] With this patch, verify that you cannot do this anymore.

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

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 17 Lee Jamison 2017-06-22 14:32:34 UTC
Created attachment 64548 [details] [review]
Bug 18834: Show distinction between shared and private lists in staff

Just as we show this distinction in OPAC, this patch adds a type column
in the Your lists tab that displays Private or Shared. It always contains
Public in the other tab.

Test plan:
[1] Check if you see Shared for a private lists with shares in staff.
[2] Run t/db_dependent/Utils/Datatables_Virtualshelves.t

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

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 18 Marcel de Rooy 2017-06-23 05:39:38 UTC
(In reply to Lee Jamison from comment #17)
> Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Great! Thanks, Lee.
Comment 19 Jonathan Druart 2017-07-13 19:35:44 UTC
Marcel, I do not think we should hide "Anyone seeing this list" for private lists.

First, you are not allowed to select it before sharing it. That can be a bit annoying if you know you are going to share it after you created it.

Then imagine the following workflow:
- Create a private list
- Share it
- Edit the list to let your friend adding items to this list
- Remove the share
=> When you edit the list you do not see the correct value, "Anyone seeing this list" is not displayed whereas it is the value of the field.
Comment 20 Marcel de Rooy 2017-07-14 07:38:35 UTC
(In reply to Jonathan Druart from comment #19)
> Marcel, I do not think we should hide "Anyone seeing this list" for private
> lists.
> 
> First, you are not allowed to select it before sharing it. That can be a bit
> annoying if you know you are going to share it after you created it.
> 
> Then imagine the following workflow:
> - Create a private list
> - Share it
> - Edit the list to let your friend adding items to this list
> - Remove the share
> => When you edit the list you do not see the correct value, "Anyone seeing
> this list" is not displayed whereas it is the value of the field.

I added it because users apparently get confused from options that are not relevant. See bug 10865 in the past.

We could choose another approach too:
[1] Tell the user that no one else sees the list (when this applies).
[2] Replace Anyone by Owner when we delete the last share. This does not resolve your first remark.

Any feedback ?
Comment 21 Jonathan Druart 2017-07-24 15:50:59 UTC
I would let the option anyway and add a note if it does not apply currently.
Comment 22 Marcel de Rooy 2017-07-25 13:04:31 UTC
(In reply to Jonathan Druart from comment #21)
> I would let the option anyway and add a note if it does not apply currently.

Will put this approach under report 18980
Comment 23 Marcel de Rooy 2017-08-21 12:20:41 UTC
Bug 18980 has been pushed. This bug can be closed.